Fix Uint256 deserialization (#2786)

* Change base_fee_per_gas to Uint256

* Add custom (de)serialization to ExecutionPayload

* Fix errors

* Add a quoted_u256 module

* Remove unused function

* lint

* Add test

* Remove extra line

Co-authored-by: Paul Hauner <paul@paulhauner.com>

Cargo.lock

@@ -2709,7 +2709,6 @@ dependencies = [
  "eth2_ssz 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "eth2_wallet",
  "genesis",
- "int_to_bytes",
  "lighthouse_network",
  "lighthouse_version",
  "log",

beacon_node/execution_layer/src/engine_api/http.rs

@@ -326,7 +326,7 @@ impl<T: EthSpec> From<ExecutionPayload<T>> for JsonExecutionPayload<T> {
             gas_used: e.gas_used,
             timestamp: e.timestamp,
             extra_data: e.extra_data,
-            base_fee_per_gas: Uint256::from_little_endian(e.base_fee_per_gas.as_bytes()),
+            base_fee_per_gas: e.base_fee_per_gas,
             block_hash: e.block_hash,
             transactions: e.transactions,
         }
@@ -347,19 +347,13 @@ impl<T: EthSpec> From<JsonExecutionPayload<T>> for ExecutionPayload<T> {
             gas_used: e.gas_used,
             timestamp: e.timestamp,
             extra_data: e.extra_data,
-            base_fee_per_gas: uint256_to_hash256(e.base_fee_per_gas),
+            base_fee_per_gas: e.base_fee_per_gas,
             block_hash: e.block_hash,
             transactions: e.transactions,
         }
     }
 }
 
-fn uint256_to_hash256(u: Uint256) -> Hash256 {
-    let mut bytes = [0; 32];
-    u.to_little_endian(&mut bytes);
-    Hash256::from_slice(&bytes)
-}
-
 #[derive(Debug, PartialEq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct JsonConsensusValidatedRequest {
@@ -797,7 +791,7 @@ mod test {
                             gas_used: 2,
                             timestamp: 42,
                             extra_data: vec![].into(),
-                            base_fee_per_gas: uint256_to_hash256(Uint256::from(1)),
+                            base_fee_per_gas: Uint256::from(1),
                             block_hash: Hash256::repeat_byte(1),
                             transactions: vec![].into(),
                         })
@@ -960,7 +954,7 @@ mod test {
                             gas_used: 0,
                             timestamp: 5,
                             extra_data: vec![].into(),
-                            base_fee_per_gas: uint256_to_hash256(Uint256::from(0)),
+                            base_fee_per_gas: Uint256::from(0),
                             block_hash: Hash256::from_str("0xb084c10440f05f5a23a55d1d7ebcb1b3892935fb56f23cdc9a7f42c348eed174").unwrap(),
                             transactions: vec![].into(),
                         };
@@ -984,7 +978,7 @@ mod test {
                             gas_used: 0,
                             timestamp: 5,
                             extra_data: vec![].into(),
-                            base_fee_per_gas: uint256_to_hash256(Uint256::from(0)),
+                            base_fee_per_gas: Uint256::from(0),
                             block_hash: Hash256::from_str("0xb084c10440f05f5a23a55d1d7ebcb1b3892935fb56f23cdc9a7f42c348eed174").unwrap(),
                             transactions: vec![].into(),
                         })

beacon_node/execution_layer/src/test_utils/execution_block_generator.rs

@@ -250,7 +250,7 @@ impl<T: EthSpec> ExecutionBlockGenerator<T> {
             gas_used: GAS_USED,
             timestamp: payload.timestamp,
             extra_data: "block gen was here".as_bytes().to_vec().into(),
-            base_fee_per_gas: Hash256::from_low_u64_le(1),
+            base_fee_per_gas: Uint256::one(),
             block_hash: Hash256::zero(),
             transactions: vec![].into(),
         };

consensus/serde_utils/Cargo.toml

@@ -10,6 +10,7 @@ license = "Apache-2.0"
 serde = { version = "1.0.116", features = ["derive"] }
 serde_derive = "1.0.116"
 hex = "0.4.2"
+ethereum-types = "0.12.1"
 
 [dev-dependencies]
 serde_json = "1.0.58"

consensus/serde_utils/src/lib.rs

@@ -9,4 +9,4 @@ pub mod u32_hex;
 pub mod u64_hex_be;
 pub mod u8_hex;
 
-pub use quoted_int::{quoted_u32, quoted_u64, quoted_u8};
+pub use quoted_int::{quoted_u256, quoted_u32, quoted_u64, quoted_u8};

consensus/serde_utils/src/quoted_int.rs

@@ -4,6 +4,7 @@
 //!
 //! Quotes can be optional during decoding.
 
+use ethereum_types::U256;
 use serde::{Deserializer, Serializer};
 use serde_derive::{Deserialize, Serialize};
 use std::convert::TryFrom;
@@ -56,6 +57,17 @@ macro_rules! define_mod {
             }
         }
 
+        /// Compositional wrapper type that allows quotes or no quotes.
+        #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Deserialize, Serialize)]
+        #[serde(transparent)]
+        pub struct MaybeQuoted<T>
+        where
+            T: From<$int> + Into<$int> + Copy + TryFrom<u64>,
+        {
+            #[serde(with = "self")]
+            pub value: T,
+        }
+
         /// Wrapper type for requiring quotes on a `$int`-like type.
         ///
         /// Unlike using `serde(with = "quoted_$int::require_quotes")` this is composable, and can be nested
@@ -142,3 +154,66 @@ pub mod quoted_u64 {
 
     define_mod!(u64, visit_u64);
 }
+
+pub mod quoted_u256 {
+    use super::*;
+
+    struct U256Visitor;
+
+    impl<'de> serde::de::Visitor<'de> for U256Visitor {
+        type Value = U256;
+
+        fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+            formatter.write_str("a quoted U256 integer")
+        }
+
+        fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+        where
+            E: serde::de::Error,
+        {
+            U256::from_dec_str(v).map_err(serde::de::Error::custom)
+        }
+    }
+
+    /// Serialize with quotes.
+    pub fn serialize<S>(value: &U256, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        serializer.serialize_str(&format!("{}", value))
+    }
+
+    /// Deserialize with quotes.
+    pub fn deserialize<'de, D>(deserializer: D) -> Result<U256, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        deserializer.deserialize_str(U256Visitor)
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[derive(Debug, PartialEq, Serialize, Deserialize)]
+    #[serde(transparent)]
+    struct WrappedU256(#[serde(with = "quoted_u256")] U256);
+
+    #[test]
+    fn u256_with_quotes() {
+        assert_eq!(
+            &serde_json::to_string(&WrappedU256(U256::one())).unwrap(),
+            "\"1\""
+        );
+        assert_eq!(
+            serde_json::from_str::<WrappedU256>("\"1\"").unwrap(),
+            WrappedU256(U256::one())
+        );
+    }
+
+    #[test]
+    fn u256_without_quotes() {
+        serde_json::from_str::<WrappedU256>("1").unwrap_err();
+    }
+}

consensus/types/Cargo.toml

@@ -38,7 +38,7 @@ tempfile = "3.1.0"
 derivative = "2.1.1"
 rusqlite = { version = "0.25.3", features = ["bundled"], optional = true }
 arbitrary = { version = "1.0", features = ["derive"], optional = true }
-eth2_serde_utils = "0.1.0"
+eth2_serde_utils = { path = "../serde_utils" }
 regex = "1.3.9"
 lazy_static = "1.4.0"
 parking_lot = "0.11.1"

consensus/types/src/execution_payload.rs

@@ -29,7 +29,8 @@ pub struct ExecutionPayload<T: EthSpec> {
     pub timestamp: u64,
     #[serde(with = "ssz_types::serde_utils::hex_var_list")]
     pub extra_data: VariableList<u8, T::MaxExtraDataBytes>,
-    pub base_fee_per_gas: Hash256,
+    #[serde(with = "eth2_serde_utils::quoted_u256")]
+    pub base_fee_per_gas: Uint256,
     pub block_hash: Hash256,
     #[serde(with = "ssz_types::serde_utils::list_of_hex_var_list")]
     pub transactions:
@@ -51,7 +52,7 @@ impl<T: EthSpec> ExecutionPayload<T> {
             gas_used: 0,
             timestamp: 0,
             extra_data: VariableList::empty(),
-            base_fee_per_gas: Hash256::zero(),
+            base_fee_per_gas: Uint256::zero(),
             block_hash: Hash256::zero(),
             transactions: VariableList::empty(),
         }

consensus/types/src/execution_payload_header.rs

@@ -26,7 +26,8 @@ pub struct ExecutionPayloadHeader<T: EthSpec> {
     pub timestamp: u64,
     #[serde(with = "ssz_types::serde_utils::hex_var_list")]
     pub extra_data: VariableList<u8, T::MaxExtraDataBytes>,
-    pub base_fee_per_gas: Hash256,
+    #[serde(with = "eth2_serde_utils::quoted_u256")]
+    pub base_fee_per_gas: Uint256,
     pub block_hash: Hash256,
     pub transactions_root: Hash256,
 }

lcli/src/create_payload_header.rs

@@ -1,7 +1,5 @@
-use bls::Hash256;
 use clap::ArgMatches;
 use clap_utils::{parse_optional, parse_required};
-use int_to_bytes::int_to_bytes32;
 use ssz::Encode;
 use std::fs::File;
 use std::io::Write;
@@ -16,10 +14,7 @@ pub fn run<T: EthSpec>(matches: &ArgMatches) -> Result<(), String> {
             .map_err(|e| format!("Unable to get time: {:?}", e))?
             .as_secs(),
     );
-    let base_fee_per_gas = Hash256::from_slice(&int_to_bytes32(parse_required(
+    let base_fee_per_gas = parse_required(matches, "base-fee-per-gas")?;
-        matches,
-        "base-fee-per-gas",
-    )?));
     let gas_limit = parse_required(matches, "gas-limit")?;
     let file_name = matches.value_of("file").ok_or("No file supplied")?;