From 51b44290a39d8f4539591fabe59dda338ace3016 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 22 Nov 2022 17:22:46 -0500 Subject: [PATCH 1/2] rename excess blobs and fix json serialization/deserialization --- beacon_node/execution_layer/src/engine_api.rs | 6 +- .../src/engine_api/json_structures.rs | 132 ++++++-------- beacon_node/execution_layer/src/lib.rs | 2 +- consensus/serde_utils/src/lib.rs | 1 + consensus/serde_utils/src/u256_hex_be_opt.rs | 169 ++++++++++++++++++ consensus/types/src/execution_payload.rs | 4 +- .../types/src/execution_payload_header.rs | 8 +- 7 files changed, 236 insertions(+), 86 deletions(-) create mode 100644 consensus/serde_utils/src/u256_hex_be_opt.rs diff --git a/beacon_node/execution_layer/src/engine_api.rs b/beacon_node/execution_layer/src/engine_api.rs index 128f23386..b1a3cfa41 100644 --- a/beacon_node/execution_layer/src/engine_api.rs +++ b/beacon_node/execution_layer/src/engine_api.rs @@ -154,8 +154,8 @@ pub struct ExecutionBlockWithTransactions { pub extra_data: VariableList, pub base_fee_per_gas: Uint256, #[superstruct(only(Eip4844))] - #[serde(with = "eth2_serde_utils::u64_hex_be")] - pub excess_blobs: u64, + #[serde(with = "eth2_serde_utils::u256_hex_be")] + pub excess_data_gas: Uint256, #[serde(rename = "hash")] pub block_hash: ExecutionBlockHash, pub transactions: Vec, @@ -227,7 +227,7 @@ impl From> for ExecutionBlockWithTransactions timestamp: block.timestamp, extra_data: block.extra_data, base_fee_per_gas: block.base_fee_per_gas, - excess_blobs: block.excess_blobs, + excess_data_gas: block.excess_data_gas, block_hash: block.block_hash, transactions: block .transactions diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 99459ec2b..afce579ca 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -64,7 +64,7 @@ pub struct JsonPayloadIdResponse { } #[superstruct( - variants(V1, V2, V3), + variants(V1, V2), variant_attributes( derive(Debug, PartialEq, Default, Serialize, Deserialize,), serde(bound = "T: EthSpec", rename_all = "camelCase"), @@ -94,15 +94,18 @@ pub struct JsonExecutionPayload { pub extra_data: VariableList, #[serde(with = "eth2_serde_utils::u256_hex_be")] pub base_fee_per_gas: Uint256, - #[superstruct(only(V3))] - // FIXME: can't easily make this an option because of custom deserialization.. - #[serde(with = "eth2_serde_utils::u64_hex_be")] - pub excess_blobs: u64, + #[superstruct(only(V2))] + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + #[serde(with = "eth2_serde_utils::u256_hex_be_opt")] + pub excess_data_gas: Option, pub block_hash: ExecutionBlockHash, #[serde(with = "ssz_types::serde_utils::list_of_hex_var_list")] pub transactions: VariableList, T::MaxTransactionsPerPayload>, - #[superstruct(only(V2, V3))] + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + #[superstruct(only(V2))] pub withdrawals: Option>, } @@ -175,43 +178,24 @@ impl JsonExecutionPayload { }) .ok_or(Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV2 -> ExecutionPayloadCapella".to_string()))? })), - ForkName::Eip4844 => Err(Error::UnsupportedForkVariant("JsonExecutionPayloadV2 -> ExecutionPayloadEip4844 not implemented yet as it might never be".to_string())), - _ => Err(Error::UnsupportedForkVariant(format!("Unsupported conversion from JsonExecutionPayloadV2 for {}", fork_name))), - } - JsonExecutionPayload::V3(v3) => match fork_name { - ForkName::Merge => Ok(ExecutionPayload::Merge(ExecutionPayloadMerge { - parent_hash: v3.parent_hash, - fee_recipient: v3.fee_recipient, - state_root: v3.state_root, - receipts_root: v3.receipts_root, - logs_bloom: v3.logs_bloom, - prev_randao: v3.prev_randao, - block_number: v3.block_number, - gas_limit: v3.gas_limit, - gas_used: v3.gas_used, - timestamp: v3.timestamp, - extra_data: v3.extra_data, - base_fee_per_gas: v3.base_fee_per_gas, - block_hash: v3.block_hash, - transactions: v3.transactions, - })), - ForkName::Capella => Ok(ExecutionPayload::Capella(ExecutionPayloadCapella { - parent_hash: v3.parent_hash, - fee_recipient: v3.fee_recipient, - state_root: v3.state_root, - receipts_root: v3.receipts_root, - logs_bloom: v3.logs_bloom, - prev_randao: v3.prev_randao, - block_number: v3.block_number, - gas_limit: v3.gas_limit, - gas_used: v3.gas_used, - timestamp: v3.timestamp, - extra_data: v3.extra_data, - base_fee_per_gas: v3.base_fee_per_gas, - block_hash: v3.block_hash, - transactions: v3.transactions, + ForkName::Eip4844 => Ok(ExecutionPayload::Eip4844(ExecutionPayloadEip4844 { + parent_hash: v2.parent_hash, + fee_recipient: v2.fee_recipient, + state_root: v2.state_root, + receipts_root: v2.receipts_root, + logs_bloom: v2.logs_bloom, + prev_randao: v2.prev_randao, + block_number: v2.block_number, + gas_limit: v2.gas_limit, + gas_used: v2.gas_used, + timestamp: v2.timestamp, + extra_data: v2.extra_data, + base_fee_per_gas: v2.base_fee_per_gas, + excess_data_gas: v2.excess_data_gas.ok_or(Error::BadConversion("Null `excess_data_gas` field converting JsonExecutionPayloadV2 -> ExecutionPayloadEip4844".to_string()))?, + block_hash: v2.block_hash, + transactions: v2.transactions, #[cfg(feature = "withdrawals")] - withdrawals: v3 + withdrawals: v2 .withdrawals .map(|v| { Into::>::into(v) @@ -220,36 +204,7 @@ impl JsonExecutionPayload { .collect::>() .into() }) - .ok_or(Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV3 -> ExecutionPayloadCapella".to_string()))? - })), - ForkName::Eip4844 => Ok(ExecutionPayload::Eip4844(ExecutionPayloadEip4844 { - parent_hash: v3.parent_hash, - fee_recipient: v3.fee_recipient, - state_root: v3.state_root, - receipts_root: v3.receipts_root, - logs_bloom: v3.logs_bloom, - prev_randao: v3.prev_randao, - block_number: v3.block_number, - gas_limit: v3.gas_limit, - gas_used: v3.gas_used, - timestamp: v3.timestamp, - extra_data: v3.extra_data, - base_fee_per_gas: v3.base_fee_per_gas, - // FIXME: excess_blobs probably will be an option whenever the engine API is finalized - excess_blobs: v3.excess_blobs, - block_hash: v3.block_hash, - transactions: v3.transactions, - #[cfg(feature = "withdrawals")] - withdrawals: v3 - .withdrawals - .map(|v| { - Vec::from(v) - .into_iter() - .map(Into::into) - .collect::>() - .into() - }) - .ok_or(Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV3 -> ExecutionPayloadEip4844".to_string()))?, + .ok_or(Error::BadConversion("Null withdrawal field converting JsonExecutionPayloadV2 -> ExecutionPayloadEip4844".to_string()))? })), _ => Err(Error::UnsupportedForkVariant(format!("Unsupported conversion from JsonExecutionPayloadV2 for {}", fork_name))), } @@ -306,6 +261,7 @@ impl TryFrom> for JsonExecutionPayloadV2 { timestamp: merge.timestamp, extra_data: merge.extra_data, base_fee_per_gas: merge.base_fee_per_gas, + excess_data_gas: None, block_hash: merge.block_hash, transactions: merge.transactions, withdrawals: None, @@ -323,6 +279,7 @@ impl TryFrom> for JsonExecutionPayloadV2 { timestamp: capella.timestamp, extra_data: capella.extra_data, base_fee_per_gas: capella.base_fee_per_gas, + excess_data_gas: None, block_hash: capella.block_hash, transactions: capella.transactions, #[cfg(feature = "withdrawals")] @@ -336,10 +293,33 @@ impl TryFrom> for JsonExecutionPayloadV2 { #[cfg(not(feature = "withdrawals"))] withdrawals: None, }), - ExecutionPayload::Eip4844(_) => Err(Error::UnsupportedForkVariant(format!( - "Unsupported conversion to JsonExecutionPayloadV1 for {}", - ForkName::Eip4844 - ))), + ExecutionPayload::Eip4844(eip4844) => Ok(JsonExecutionPayloadV2 { + parent_hash: eip4844.parent_hash, + fee_recipient: eip4844.fee_recipient, + state_root: eip4844.state_root, + receipts_root: eip4844.receipts_root, + logs_bloom: eip4844.logs_bloom, + prev_randao: eip4844.prev_randao, + block_number: eip4844.block_number, + gas_limit: eip4844.gas_limit, + gas_used: eip4844.gas_used, + timestamp: eip4844.timestamp, + extra_data: eip4844.extra_data, + base_fee_per_gas: eip4844.base_fee_per_gas, + excess_data_gas: Some(eip4844.excess_data_gas), + block_hash: eip4844.block_hash, + transactions: eip4844.transactions, + #[cfg(feature = "withdrawals")] + withdrawals: Some( + Vec::from(eip4844.withdrawals) + .into_iter() + .map(Into::into) + .collect::>() + .into(), + ), + #[cfg(not(feature = "withdrawals"))] + withdrawals: None, + }), } } } diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 2a19c4165..1f53f074d 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1577,7 +1577,7 @@ impl ExecutionLayer { timestamp: eip4844_block.timestamp, extra_data: eip4844_block.extra_data, base_fee_per_gas: eip4844_block.base_fee_per_gas, - excess_blobs: eip4844_block.excess_blobs, + excess_data_gas: eip4844_block.excess_data_gas, block_hash: eip4844_block.block_hash, transactions, #[cfg(feature = "withdrawals")] diff --git a/consensus/serde_utils/src/lib.rs b/consensus/serde_utils/src/lib.rs index 92b5966c9..75fd6009b 100644 --- a/consensus/serde_utils/src/lib.rs +++ b/consensus/serde_utils/src/lib.rs @@ -7,6 +7,7 @@ pub mod json_str; pub mod list_of_bytes_lists; pub mod quoted_u64_vec; pub mod u256_hex_be; +pub mod u256_hex_be_opt; pub mod u32_hex; pub mod u64_hex_be; pub mod u8_hex; diff --git a/consensus/serde_utils/src/u256_hex_be_opt.rs b/consensus/serde_utils/src/u256_hex_be_opt.rs new file mode 100644 index 000000000..8eadbf024 --- /dev/null +++ b/consensus/serde_utils/src/u256_hex_be_opt.rs @@ -0,0 +1,169 @@ +use ethereum_types::U256; + +use serde::de::Visitor; +use serde::{de, Deserializer, Serialize, Serializer}; +use std::fmt; +use std::str::FromStr; + +pub fn serialize(num: &Option, serializer: S) -> Result +where + S: Serializer, +{ + num.serialize(serializer) +} + +pub struct U256Visitor; + +impl<'de> Visitor<'de> for U256Visitor { + type Value = String; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter.write_str("a well formatted hex string") + } + + fn visit_str(self, value: &str) -> Result + where + E: de::Error, + { + if !value.starts_with("0x") { + return Err(de::Error::custom("must start with 0x")); + } + let stripped = &value[2..]; + if stripped.is_empty() { + Err(de::Error::custom(format!( + "quantity cannot be {:?}", + stripped + ))) + } else if stripped == "0" { + Ok(value.to_string()) + } else if stripped.starts_with('0') { + Err(de::Error::custom("cannot have leading zero")) + } else { + Ok(value.to_string()) + } + } +} + +pub fn deserialize<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let decoded = deserializer.deserialize_string(U256Visitor)?; + + Some( + U256::from_str(&decoded) + .map_err(|e| de::Error::custom(format!("Invalid U256 string: {}", e))), + ) + .transpose() +} + +#[cfg(test)] +mod test { + use ethereum_types::U256; + use serde::{Deserialize, Serialize}; + use serde_json; + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(transparent)] + struct Wrapper { + #[serde(with = "super")] + val: Option, + } + + #[test] + fn encoding() { + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(0.into()) + }) + .unwrap(), + "\"0x0\"" + ); + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(1.into()) + }) + .unwrap(), + "\"0x1\"" + ); + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(256.into()) + }) + .unwrap(), + "\"0x100\"" + ); + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(65.into()) + }) + .unwrap(), + "\"0x41\"" + ); + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(1024.into()) + }) + .unwrap(), + "\"0x400\"" + ); + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(U256::max_value() - 1) + }) + .unwrap(), + "\"0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe\"" + ); + assert_eq!( + &serde_json::to_string(&Wrapper { + val: Some(U256::max_value()) + }) + .unwrap(), + "\"0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\"" + ); + } + + #[test] + fn decoding() { + assert_eq!( + serde_json::from_str::("\"0x0\"").unwrap(), + Wrapper { + val: Some(0.into()) + }, + ); + assert_eq!( + serde_json::from_str::("\"0x41\"").unwrap(), + Wrapper { + val: Some(65.into()) + }, + ); + assert_eq!( + serde_json::from_str::("\"0x400\"").unwrap(), + Wrapper { + val: Some(1024.into()) + }, + ); + assert_eq!( + serde_json::from_str::( + "\"0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe\"" + ) + .unwrap(), + Wrapper { + val: Some(U256::max_value() - 1) + }, + ); + assert_eq!( + serde_json::from_str::( + "\"0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\"" + ) + .unwrap(), + Wrapper { + val: Some(U256::max_value()) + }, + ); + serde_json::from_str::("\"0x\"").unwrap_err(); + serde_json::from_str::("\"0x0400\"").unwrap_err(); + serde_json::from_str::("\"400\"").unwrap_err(); + serde_json::from_str::("\"ff\"").unwrap_err(); + } +} diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 6036973d5..fa6348bdc 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -74,9 +74,9 @@ pub struct ExecutionPayload { #[superstruct(getter(copy))] pub base_fee_per_gas: Uint256, #[superstruct(only(Eip4844))] - #[serde(with = "eth2_serde_utils::quoted_u64")] + #[serde(with = "eth2_serde_utils::quoted_u256")] #[superstruct(getter(copy))] - pub excess_blobs: u64, + pub excess_data_gas: Uint256, #[superstruct(getter(copy))] pub block_hash: ExecutionBlockHash, #[serde(with = "ssz_types::serde_utils::list_of_hex_var_list")] diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 6f6b5aa95..7c4d9af1a 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -68,9 +68,9 @@ pub struct ExecutionPayloadHeader { #[superstruct(getter(copy))] pub base_fee_per_gas: Uint256, #[superstruct(only(Eip4844))] - #[serde(with = "eth2_serde_utils::quoted_u64")] + #[serde(with = "eth2_serde_utils::quoted_u256")] #[superstruct(getter(copy))] - pub excess_blobs: u64, + pub excess_data_gas: Uint256, #[superstruct(getter(copy))] pub block_hash: ExecutionBlockHash, #[superstruct(getter(copy))] @@ -150,7 +150,7 @@ impl ExecutionPayloadHeaderCapella { extra_data: self.extra_data.clone(), base_fee_per_gas: self.base_fee_per_gas, // TODO: verify if this is correct - excess_blobs: 0, + excess_data_gas: Uint256::zero(), block_hash: self.block_hash, transactions_root: self.transactions_root, #[cfg(feature = "withdrawals")] @@ -216,7 +216,7 @@ impl From> for ExecutionPayloadHeaderEip4 timestamp: payload.timestamp, extra_data: payload.extra_data, base_fee_per_gas: payload.base_fee_per_gas, - excess_blobs: payload.excess_blobs, + excess_data_gas: payload.excess_data_gas, block_hash: payload.block_hash, transactions_root: payload.transactions.tree_hash_root(), #[cfg(feature = "withdrawals")] From 160b915695b13e76feadb33fd994ceb97463df8c Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 22 Nov 2022 17:30:35 -0500 Subject: [PATCH 2/2] remove coments --- beacon_node/execution_layer/src/engine_api/http.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 446623744..2b7728b98 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -857,7 +857,6 @@ impl HttpJsonRpc { ) -> Result { let supported_apis = self.get_cached_supported_apis().await?; if supported_apis.new_payload_v2 { - // FIXME: I haven't thought at all about how to handle 4844.. self.new_payload_v2(execution_payload).await } else if supported_apis.new_payload_v1 { self.new_payload_v1(execution_payload).await @@ -875,7 +874,6 @@ impl HttpJsonRpc { ) -> Result, Error> { let supported_apis = self.get_cached_supported_apis().await?; if supported_apis.get_payload_v2 { - // FIXME: I haven't thought at all about how to handle 4844.. self.get_payload_v2(fork_name, payload_id).await } else if supported_apis.new_payload_v1 { self.get_payload_v1(fork_name, payload_id).await @@ -893,7 +891,6 @@ impl HttpJsonRpc { ) -> Result { let supported_apis = self.get_cached_supported_apis().await?; if supported_apis.forkchoice_updated_v2 { - // FIXME: I haven't thought at all about how to handle 4844.. self.forkchoice_updated_v2(forkchoice_state, payload_attributes) .await } else if supported_apis.forkchoice_updated_v1 {