From 24966c059d2a6cde9edbb404bc1362fe37fc276e Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Wed, 10 Nov 2021 15:57:44 -0800 Subject: [PATCH] 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 --- Cargo.lock | 1 - .../execution_layer/src/engine_api/http.rs | 16 ++-- .../test_utils/execution_block_generator.rs | 2 +- consensus/serde_utils/Cargo.toml | 1 + consensus/serde_utils/src/lib.rs | 2 +- consensus/serde_utils/src/quoted_int.rs | 75 +++++++++++++++++++ consensus/types/Cargo.toml | 2 +- consensus/types/src/execution_payload.rs | 5 +- .../types/src/execution_payload_header.rs | 3 +- lcli/src/create_payload_header.rs | 7 +- 10 files changed, 90 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 65796c686..602bfc261 100644 --- a/Cargo.lock +++ b/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", diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 8b393d93a..fabe9a437 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -326,7 +326,7 @@ impl From> for JsonExecutionPayload { 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 From> for ExecutionPayload { 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(), }) diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index ae7924e90..6d33e497c 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -250,7 +250,7 @@ impl ExecutionBlockGenerator { 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(), }; diff --git a/consensus/serde_utils/Cargo.toml b/consensus/serde_utils/Cargo.toml index 2cda517a6..e1b32e936 100644 --- a/consensus/serde_utils/Cargo.toml +++ b/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" diff --git a/consensus/serde_utils/src/lib.rs b/consensus/serde_utils/src/lib.rs index 541a86d89..77cee4c24 100644 --- a/consensus/serde_utils/src/lib.rs +++ b/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}; diff --git a/consensus/serde_utils/src/quoted_int.rs b/consensus/serde_utils/src/quoted_int.rs index 24edf1ebe..822acb5ee 100644 --- a/consensus/serde_utils/src/quoted_int.rs +++ b/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 + where + T: From<$int> + Into<$int> + Copy + TryFrom, + { + #[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(self, v: &str) -> Result + where + E: serde::de::Error, + { + U256::from_dec_str(v).map_err(serde::de::Error::custom) + } + } + + /// Serialize with quotes. + pub fn serialize(value: &U256, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&format!("{}", value)) + } + + /// Deserialize with quotes. + pub fn deserialize<'de, D>(deserializer: D) -> Result + 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::("\"1\"").unwrap(), + WrappedU256(U256::one()) + ); + } + + #[test] + fn u256_without_quotes() { + serde_json::from_str::("1").unwrap_err(); + } +} diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index 1f9ea10c6..3886e57cb 100644 --- a/consensus/types/Cargo.toml +++ b/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" diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 8f16893ac..7b6357551 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -29,7 +29,8 @@ pub struct ExecutionPayload { pub timestamp: u64, #[serde(with = "ssz_types::serde_utils::hex_var_list")] pub extra_data: VariableList, - 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 ExecutionPayload { 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(), } diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index e9876d89b..d214ba0ff 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -26,7 +26,8 @@ pub struct ExecutionPayloadHeader { pub timestamp: u64, #[serde(with = "ssz_types::serde_utils::hex_var_list")] pub extra_data: VariableList, - 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, } diff --git a/lcli/src/create_payload_header.rs b/lcli/src/create_payload_header.rs index 31157d4b3..814a57f26 100644 --- a/lcli/src/create_payload_header.rs +++ b/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(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( - matches, - "base-fee-per-gas", - )?)); + let base_fee_per_gas = parse_required(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")?;