From e3ccd8fd4abfa0863ddcad660d599450e18ba9a4 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 24 Nov 2022 15:14:06 +1100 Subject: [PATCH 1/3] Two Capella bugfixes (#3749) * Two Capella bugfixes * fix payload default check in fork choice * Revert "fix payload default check in fork choice" This reverts commit e56fefbd05811526af4499711045275db366aa09. Co-authored-by: realbigsean --- beacon_node/beacon_chain/src/beacon_chain.rs | 50 ++++++------- .../beacon_chain/src/execution_payload.rs | 2 +- .../src/per_block_processing.rs | 10 ++- .../types/src/execution_payload_header.rs | 4 +- consensus/types/src/payload.rs | 75 ++++++++++++++----- 5 files changed, 88 insertions(+), 53 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c243d50cb3..89ccd96b15 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4112,38 +4112,30 @@ impl BeaconChain { return Ok(()); } - #[cfg(feature = "withdrawals")] - let head_state = &self.canonical_head.cached_head().snapshot.beacon_state; #[cfg(feature = "withdrawals")] let withdrawals = match self.spec.fork_name_at_epoch(prepare_epoch) { - ForkName::Base | ForkName::Altair | ForkName::Merge => { - None - }, - ForkName::Capella | ForkName::Eip4844 => match &head_state { - &BeaconState::Capella(_) | &BeaconState::Eip4844(_) => { - // The head_state is already BeaconState::Capella or later - // FIXME(mark) - // Might implement caching here in the future.. - Some(get_expected_withdrawals(head_state, &self.spec)) - } - &BeaconState::Base(_) | &BeaconState::Altair(_) | &BeaconState::Merge(_) => { - // We are the Capella transition block proposer, need advanced state - let mut prepare_state = self - .state_at_slot(prepare_slot, StateSkipConfig::WithoutStateRoots) - .or_else(|e| { - error!(self.log, "Capella Transition Proposer"; "Error Advancing State: " => ?e); - Err(e) - })?; - // FIXME(mark) - // Might implement caching here in the future.. - Some(get_expected_withdrawals(&prepare_state, &self.spec)) - } - }, - }.transpose().or_else(|e| { - error!(self.log, "Error preparing beacon proposer"; "while calculating expected withdrawals" => ?e); + ForkName::Base | ForkName::Altair | ForkName::Merge => None, + ForkName::Capella | ForkName::Eip4844 => { + // We must use the advanced state because balances can change at epoch boundaries + // and balances affect withdrawals. + // FIXME(mark) + // Might implement caching here in the future.. + let prepare_state = self + .state_at_slot(prepare_slot, StateSkipConfig::WithoutStateRoots) + .or_else(|e| { + error!(self.log, "State advance for withdrawals failed"; "error" => ?e); + Err(e) + })?; + Some(get_expected_withdrawals(&prepare_state, &self.spec)) + } + } + .transpose() + .or_else(|e| { + error!(self.log, "Error preparing beacon proposer"; "error" => ?e); Err(e) - }).map(|withdrawals_opt| withdrawals_opt.map(|w| w.into())) - .map_err(Error::PrepareProposerFailed)?; + }) + .map(|withdrawals_opt| withdrawals_opt.map(|w| w.into())) + .map_err(Error::PrepareProposerFailed)?; let payload_attributes = PayloadAttributes::V2(PayloadAttributesV2 { timestamp: self diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index bf920a6dab..85aedc6592 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -310,7 +310,7 @@ pub fn validate_execution_payload_for_gossip( } }; - if is_merge_transition_complete || !execution_payload.is_default() { + if is_merge_transition_complete || !execution_payload.is_default_with_empty_roots() { let expected_timestamp = chain .slot_clock .start_of(block.slot()) diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 753a193987..d1c4cf12ac 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -428,9 +428,11 @@ pub fn process_execution_payload<'payload, T: EthSpec, Payload: AbstractExecPayl /// repeaetedly write code to treat these errors as false. /// https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#is_merge_transition_complete pub fn is_merge_transition_complete(state: &BeaconState) -> bool { + // We must check defaultness against the payload header with 0x0 roots, as that's what's meant + // by `ExecutionPayloadHeader()` in the spec. state .latest_execution_payload_header() - .map(|header| !header.is_default()) + .map(|header| !header.is_default_with_zero_roots()) .unwrap_or(false) } /// https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#is_merge_transition_block @@ -438,8 +440,12 @@ pub fn is_merge_transition_block>( state: &BeaconState, body: BeaconBlockBodyRef, ) -> bool { + // For execution payloads in blocks (which may be headers) we must check defaultness against + // the payload with `transactions_root` equal to the tree hash of the empty list. body.execution_payload() - .map(|payload| !is_merge_transition_complete(state) && !payload.is_default()) + .map(|payload| { + !is_merge_transition_complete(state) && !payload.is_default_with_empty_roots() + }) .unwrap_or(false) } /// https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#is_execution_enabled diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 6f6b5aa953..37547614de 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -103,9 +103,9 @@ impl ExecutionPayloadHeader { } impl<'a, T: EthSpec> ExecutionPayloadHeaderRef<'a, T> { - pub fn is_default(self) -> bool { + pub fn is_default_with_zero_roots(self) -> bool { map_execution_payload_header_ref!(&'a _, self, |inner, cons| { - let _ = cons(inner); + cons(inner); *inner == Default::default() }) } diff --git a/consensus/types/src/payload.rs b/consensus/types/src/payload.rs index 3081dd1cbe..2507a9f0eb 100644 --- a/consensus/types/src/payload.rs +++ b/consensus/types/src/payload.rs @@ -40,8 +40,11 @@ pub trait ExecPayload: Debug + Clone + PartialEq + Hash + TreeHash + #[cfg(feature = "withdrawals")] fn withdrawals_root(&self) -> Result; - /// Is this a default payload? (pre-merge) - fn is_default(&self) -> bool; + /// Is this a default payload with 0x0 roots for transactions and withdrawals? + fn is_default_with_zero_roots(&self) -> bool; + + /// Is this a default payload with the hash of the empty list for transactions and withdrawals? + fn is_default_with_empty_roots(&self) -> bool; } /// `ExecPayload` functionality the requires ownership. @@ -241,12 +244,17 @@ impl ExecPayload for FullPayload { } } - fn is_default<'a>(&'a self) -> bool { + fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_full_payload_ref!(&'a _, self.to_ref(), move |payload, cons| { cons(payload); payload.execution_payload == <_>::default() }) } + + fn is_default_with_empty_roots<'a>(&'a self) -> bool { + // For full payloads the empty/zero distinction does not exist. + self.is_default_with_zero_roots() + } } impl FullPayload { @@ -338,13 +346,17 @@ impl<'b, T: EthSpec> ExecPayload for FullPayloadRef<'b, T> { } } - // TODO: can this function be optimized? - fn is_default<'a>(&'a self) -> bool { + fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_full_payload_ref!(&'a _, self, move |payload, cons| { cons(payload); payload.execution_payload == <_>::default() }) } + + fn is_default_with_empty_roots(&self) -> bool { + // For full payloads the empty/zero distinction does not exist. + self.is_default_with_zero_roots() + } } impl AbstractExecPayload for FullPayload { @@ -505,11 +517,16 @@ impl ExecPayload for BlindedPayload { } } - fn is_default<'a>(&'a self) -> bool { - map_blinded_payload_ref!(&'a _, self.to_ref(), move |payload, cons| { - cons(payload); - payload.execution_payload_header == <_>::default() - }) + fn is_default_with_zero_roots<'a>(&'a self) -> bool { + self.to_ref().is_default_with_zero_roots() + } + + // For blinded payloads we must check "defaultness" against the default `ExecutionPayload` + // which has been blinded into an `ExecutionPayloadHeader`, NOT against the default + // `ExecutionPayloadHeader` which has a zeroed out `transactions_root`. The transactions root + // should be the root of the empty list. + fn is_default_with_empty_roots(&self) -> bool { + self.to_ref().is_default_with_empty_roots() } } @@ -591,24 +608,38 @@ impl<'b, T: EthSpec> ExecPayload for BlindedPayloadRef<'b, T> { } } - // TODO: can this function be optimized? - fn is_default<'a>(&'a self) -> bool { - map_blinded_payload_ref!(&'a _, self, move |payload, cons| { + fn is_default_with_zero_roots<'a>(&'a self) -> bool { + map_blinded_payload_ref!(&'b _, self, move |payload, cons| { cons(payload); payload.execution_payload_header == <_>::default() }) } + + fn is_default_with_empty_roots<'a>(&'a self) -> bool { + map_blinded_payload_ref!(&'b _, self, move |payload, cons| { + cons(payload); + payload.is_default_with_empty_roots() + }) + } } macro_rules! impl_exec_payload_common { - ($wrapper_type:ident, $wrapped_type_full:ident, $wrapped_header_type:ident, $wrapped_field:ident, $fork_variant:ident, $block_type_variant:ident, $f:block, $g:block) => { + ($wrapper_type:ident, + $wrapped_type:ident, + $wrapped_type_full:ident, + $wrapped_type_header:ident, + $wrapped_field:ident, + $fork_variant:ident, + $block_type_variant:ident, + $f:block, + $g:block) => { impl ExecPayload for $wrapper_type { fn block_type() -> BlockType { BlockType::$block_type_variant } fn to_execution_payload_header(&self) -> ExecutionPayloadHeader { - ExecutionPayloadHeader::$fork_variant($wrapped_header_type::from( + ExecutionPayloadHeader::$fork_variant($wrapped_type_header::from( self.$wrapped_field.clone(), )) } @@ -641,8 +672,12 @@ macro_rules! impl_exec_payload_common { self.$wrapped_field.gas_limit } - fn is_default(&self) -> bool { - self.$wrapped_field == $wrapped_type_full::default() + fn is_default_with_zero_roots(&self) -> bool { + self.$wrapped_field == $wrapped_type::default() + } + + fn is_default_with_empty_roots(&self) -> bool { + self.$wrapped_field == $wrapped_type::from($wrapped_type_full::default()) } fn transactions(&self) -> Option<&Transactions> { @@ -657,8 +692,8 @@ macro_rules! impl_exec_payload_common { } } - impl From<$wrapped_type_full> for $wrapper_type { - fn from($wrapped_field: $wrapped_type_full) -> Self { + impl From<$wrapped_type> for $wrapper_type { + fn from($wrapped_field: $wrapped_type) -> Self { Self { $wrapped_field } } } @@ -672,6 +707,7 @@ macro_rules! impl_exec_payload_for_fork { impl_exec_payload_common!( $wrapper_type_header, $wrapped_type_header, + $wrapped_type_full, $wrapped_type_header, execution_payload_header, $fork_variant, @@ -741,6 +777,7 @@ macro_rules! impl_exec_payload_for_fork { impl_exec_payload_common!( $wrapper_type_full, $wrapped_type_full, + $wrapped_type_full, $wrapped_type_header, execution_payload, $fork_variant, From 58b54f0a53093395e3d5854e0318423771796bd0 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 24 Nov 2022 00:41:35 -0500 Subject: [PATCH 2/3] Rename excess blobs and update 4844 json RPC serialization/deserialization (#3745) * rename excess blobs and fix json serialization/deserialization * remove coments --- beacon_node/execution_layer/src/engine_api.rs | 6 +- .../execution_layer/src/engine_api/http.rs | 3 - .../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 +- 8 files changed, 236 insertions(+), 89 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 128f23386f..b1a3cfa413 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/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 446623744e..2b7728b98d 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 { 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 4f372beda5..0e53a3b060 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 0cdce4f129..c90ed291d5 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1579,7 +1579,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 92b5966c9a..75fd6009b7 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 0000000000..8eadbf0243 --- /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 6036973d5e..fa6348bdce 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 37547614de..a9708153ca 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 788b337951b85c3db3564cba8cc9a2115d82eeaf Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 25 Nov 2022 07:09:26 +1100 Subject: [PATCH 3/3] Op pool and gossip for BLS to execution changes (#3726) --- beacon_node/Cargo.toml | 7 +- beacon_node/beacon_chain/Cargo.toml | 7 +- beacon_node/beacon_chain/src/beacon_chain.rs | 42 ++++++ beacon_node/beacon_chain/src/builder.rs | 2 + .../beacon_chain/src/canonical_head.rs | 8 +- beacon_node/beacon_chain/src/errors.rs | 7 +- .../beacon_chain/src/observed_operations.rs | 18 ++- beacon_node/http_api/Cargo.toml | 3 + beacon_node/http_api/src/lib.rs | 61 ++++++++- .../src/service/gossip_cache.rs | 13 ++ .../lighthouse_network/src/service/mod.rs | 1 + .../lighthouse_network/src/service/utils.rs | 1 + .../lighthouse_network/src/types/pubsub.rs | 22 ++- .../lighthouse_network/src/types/topics.rs | 34 ++--- .../network/src/beacon_processor/mod.rs | 62 ++++++++- .../beacon_processor/worker/gossip_methods.rs | 64 ++++++++- beacon_node/network/src/metrics.rs | 13 ++ beacon_node/network/src/router/mod.rs | 12 ++ beacon_node/network/src/router/processor.rs | 17 ++- beacon_node/operation_pool/Cargo.toml | 3 + beacon_node/operation_pool/src/lib.rs | 127 ++++++++++++++++-- beacon_node/operation_pool/src/persistence.rs | 3 +- .../block_signature_verifier.rs | 22 +++ .../state_processing/src/verify_operation.rs | 53 +++++++- .../types/src/bls_to_execution_change.rs | 2 - .../src/signed_bls_to_execution_change.rs | 2 - testing/ef_tests/Makefile | 2 +- 27 files changed, 539 insertions(+), 69 deletions(-) diff --git a/beacon_node/Cargo.toml b/beacon_node/Cargo.toml index 093f09949c..18973cb9d4 100644 --- a/beacon_node/Cargo.toml +++ b/beacon_node/Cargo.toml @@ -14,7 +14,12 @@ node_test_rig = { path = "../testing/node_test_rig" } [features] write_ssz_files = ["beacon_chain/write_ssz_files"] # Writes debugging .ssz files to /tmp during block processing. withdrawals = ["beacon_chain/withdrawals", "types/withdrawals", "store/withdrawals", "execution_layer/withdrawals"] -withdrawals-processing = ["beacon_chain/withdrawals-processing", "store/withdrawals-processing", "execution_layer/withdrawals-processing"] +withdrawals-processing = [ + "beacon_chain/withdrawals-processing", + "store/withdrawals-processing", + "execution_layer/withdrawals-processing", + "http_api/withdrawals-processing", +] [dependencies] eth2_config = { path = "../common/eth2_config" } diff --git a/beacon_node/beacon_chain/Cargo.toml b/beacon_node/beacon_chain/Cargo.toml index 39ff16c6b7..6d768476e6 100644 --- a/beacon_node/beacon_chain/Cargo.toml +++ b/beacon_node/beacon_chain/Cargo.toml @@ -11,7 +11,12 @@ write_ssz_files = [] # Writes debugging .ssz files to /tmp during block process participation_metrics = [] # Exposes validator participation metrics to Prometheus. fork_from_env = [] # Initialise the harness chain spec from the FORK_NAME env variable withdrawals = ["state_processing/withdrawals", "types/withdrawals", "store/withdrawals", "execution_layer/withdrawals"] -withdrawals-processing = ["state_processing/withdrawals-processing", "store/withdrawals-processing", "execution_layer/withdrawals-processing"] +withdrawals-processing = [ + "state_processing/withdrawals-processing", + "store/withdrawals-processing", + "execution_layer/withdrawals-processing", + "operation_pool/withdrawals-processing" +] [dev-dependencies] maplit = "1.0.2" diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 89ccd96b15..51aed941f1 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -341,6 +341,10 @@ pub struct BeaconChain { /// Maintains a record of which validators we've seen attester slashings for. pub(crate) observed_attester_slashings: Mutex, T::EthSpec>>, + /// Maintains a record of which validators we've seen BLS to execution changes for. + #[cfg(feature = "withdrawals-processing")] + pub(crate) observed_bls_to_execution_changes: + Mutex>, /// Provides information from the Ethereum 1 (PoW) chain. pub eth1_chain: Option>, /// Interfaces with the execution client. @@ -2181,6 +2185,42 @@ impl BeaconChain { } } + /// Verify a signed BLS to exection change before allowing it to propagate on the gossip network. + pub fn verify_bls_to_execution_change_for_gossip( + &self, + bls_to_execution_change: SignedBlsToExecutionChange, + ) -> Result, Error> { + #[cfg(feature = "withdrawals-processing")] + { + let wall_clock_state = self.wall_clock_state()?; + Ok(self + .observed_bls_to_execution_changes + .lock() + .verify_and_observe(bls_to_execution_change, &wall_clock_state, &self.spec)?) + } + + #[cfg(not(feature = "withdrawals-processing"))] + { + drop(bls_to_execution_change); + Ok(ObservationOutcome::AlreadyKnown) + } + } + + /// Import a BLS to execution change to the op pool. + pub fn import_bls_to_execution_change( + &self, + bls_to_execution_change: SigVerifiedOp, + ) { + if self.eth1_chain.is_some() { + #[cfg(feature = "withdrawals-processing")] + self.op_pool + .insert_bls_to_execution_change(bls_to_execution_change); + + #[cfg(not(feature = "withdrawals-processing"))] + drop(bls_to_execution_change); + } + } + /// Attempt to obtain sync committee duties from the head. pub fn sync_committee_duties_from_head( &self, @@ -3491,6 +3531,8 @@ impl BeaconChain { let eth1_data = eth1_chain.eth1_data_for_block_production(&state, &self.spec)?; let deposits = eth1_chain.deposits_for_block_inclusion(&state, ð1_data, &self.spec)?; + + #[cfg(feature = "withdrawals")] let bls_to_execution_changes = self .op_pool .get_bls_to_execution_changes(&state, &self.spec); diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 58bbb2b5c6..116a0c3980 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -780,6 +780,8 @@ where observed_voluntary_exits: <_>::default(), observed_proposer_slashings: <_>::default(), observed_attester_slashings: <_>::default(), + #[cfg(feature = "withdrawals-processing")] + observed_bls_to_execution_changes: <_>::default(), eth1_chain: self.eth1_chain, execution_layer: self.execution_layer, genesis_validators_root, diff --git a/beacon_node/beacon_chain/src/canonical_head.rs b/beacon_node/beacon_chain/src/canonical_head.rs index c9bd6db0e6..1aa8d8715f 100644 --- a/beacon_node/beacon_chain/src/canonical_head.rs +++ b/beacon_node/beacon_chain/src/canonical_head.rs @@ -907,8 +907,12 @@ impl BeaconChain { .execution_status .is_optimistic_or_invalid(); - self.op_pool - .prune_all(&new_snapshot.beacon_state, self.epoch()?); + self.op_pool.prune_all( + &new_snapshot.beacon_block, + &new_snapshot.beacon_state, + self.epoch()?, + &self.spec, + ); self.observed_block_producers.write().prune( new_view diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index e4d00d9ca6..60282426a5 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -17,8 +17,9 @@ use ssz_types::Error as SszTypesError; use state_processing::{ block_signature_verifier::Error as BlockSignatureVerifierError, per_block_processing::errors::{ - AttestationValidationError, AttesterSlashingValidationError, ExitValidationError, - ProposerSlashingValidationError, SyncCommitteeMessageValidationError, + AttestationValidationError, AttesterSlashingValidationError, + BlsExecutionChangeValidationError, ExitValidationError, ProposerSlashingValidationError, + SyncCommitteeMessageValidationError, }, signature_sets::Error as SignatureSetError, state_advance::Error as StateAdvanceError, @@ -70,6 +71,7 @@ pub enum BeaconChainError { ExitValidationError(ExitValidationError), ProposerSlashingValidationError(ProposerSlashingValidationError), AttesterSlashingValidationError(AttesterSlashingValidationError), + BlsExecutionChangeValidationError(BlsExecutionChangeValidationError), StateSkipTooLarge { start_slot: Slot, requested_slot: Slot, @@ -212,6 +214,7 @@ easy_from_to!(SyncCommitteeMessageValidationError, BeaconChainError); easy_from_to!(ExitValidationError, BeaconChainError); easy_from_to!(ProposerSlashingValidationError, BeaconChainError); easy_from_to!(AttesterSlashingValidationError, BeaconChainError); +easy_from_to!(BlsExecutionChangeValidationError, BeaconChainError); easy_from_to!(SszTypesError, BeaconChainError); easy_from_to!(OpPoolError, BeaconChainError); easy_from_to!(NaiveAggregationError, BeaconChainError); diff --git a/beacon_node/beacon_chain/src/observed_operations.rs b/beacon_node/beacon_chain/src/observed_operations.rs index 8d8272b67d..5781f9b5b1 100644 --- a/beacon_node/beacon_chain/src/observed_operations.rs +++ b/beacon_node/beacon_chain/src/observed_operations.rs @@ -1,5 +1,5 @@ use derivative::Derivative; -use smallvec::SmallVec; +use smallvec::{smallvec, SmallVec}; use ssz::{Decode, Encode}; use state_processing::{SigVerifiedOp, VerifyOperation}; use std::collections::HashSet; @@ -9,6 +9,9 @@ use types::{ SignedVoluntaryExit, Slot, }; +#[cfg(feature = "withdrawals-processing")] +use types::SignedBlsToExecutionChange; + /// Number of validator indices to store on the stack in `observed_validators`. pub const SMALL_VEC_SIZE: usize = 8; @@ -39,7 +42,7 @@ pub enum ObservationOutcome { AlreadyKnown, } -/// Trait for exits and slashings which can be observed using `ObservedOperations`. +/// Trait for operations which can be observed using `ObservedOperations`. pub trait ObservableOperation: VerifyOperation + Sized { /// The set of validator indices involved in this operation. /// @@ -49,13 +52,13 @@ pub trait ObservableOperation: VerifyOperation + Sized { impl ObservableOperation for SignedVoluntaryExit { fn observed_validators(&self) -> SmallVec<[u64; SMALL_VEC_SIZE]> { - std::iter::once(self.message.validator_index).collect() + smallvec![self.message.validator_index] } } impl ObservableOperation for ProposerSlashing { fn observed_validators(&self) -> SmallVec<[u64; SMALL_VEC_SIZE]> { - std::iter::once(self.signed_header_1.message.proposer_index).collect() + smallvec![self.signed_header_1.message.proposer_index] } } @@ -80,6 +83,13 @@ impl ObservableOperation for AttesterSlashing { } } +#[cfg(feature = "withdrawals-processing")] +impl ObservableOperation for SignedBlsToExecutionChange { + fn observed_validators(&self) -> SmallVec<[u64; SMALL_VEC_SIZE]> { + smallvec![self.message.validator_index] + } +} + impl, E: EthSpec> ObservedOperations { pub fn verify_and_observe( &mut self, diff --git a/beacon_node/http_api/Cargo.toml b/beacon_node/http_api/Cargo.toml index fedd66c540..cfd572083d 100644 --- a/beacon_node/http_api/Cargo.toml +++ b/beacon_node/http_api/Cargo.toml @@ -5,6 +5,9 @@ authors = ["Paul Hauner "] edition = "2021" autotests = false # using a single test binary compiles faster +[features] +withdrawals-processing = [] + [dependencies] warp = { version = "0.3.2", features = ["tls"] } serde = { version = "1.0.116", features = ["derive"] } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index a747430eee..e26bbe6b33 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -49,9 +49,9 @@ use types::{ Attestation, AttestationData, AttesterSlashing, BeaconStateError, BlindedPayload, CommitteeCache, ConfigAndPreset, Epoch, EthSpec, ForkName, FullPayload, ProposerPreparationData, ProposerSlashing, RelativeEpoch, SignedAggregateAndProof, - SignedBeaconBlock, SignedBlindedBeaconBlock, SignedContributionAndProof, - SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, SyncCommitteeMessage, - SyncContributionData, + SignedBeaconBlock, SignedBlindedBeaconBlock, SignedBlsToExecutionChange, + SignedContributionAndProof, SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, + SyncCommitteeMessage, SyncContributionData, }; use version::{ add_consensus_version_header, execution_optimistic_fork_versioned_response, @@ -1536,6 +1536,59 @@ pub fn serve( }, ); + // GET beacon/pool/bls_to_execution_changes + let get_beacon_pool_bls_to_execution_changes = beacon_pool_path + .clone() + .and(warp::path("bls_to_execution_changes")) + .and(warp::path::end()) + .and_then(|chain: Arc>| { + blocking_json_task(move || { + let address_changes = chain.op_pool.get_all_bls_to_execution_changes(); + Ok(api_types::GenericResponse::from(address_changes)) + }) + }); + + // POST beacon/pool/bls_to_execution_changes + let post_beacon_pool_bls_to_execution_changes = beacon_pool_path + .clone() + .and(warp::path("bls_to_execution_changes")) + .and(warp::path::end()) + .and(warp::body::json()) + .and(network_tx_filter.clone()) + .and_then( + |chain: Arc>, + address_change: SignedBlsToExecutionChange, + network_tx: UnboundedSender>| { + blocking_json_task(move || { + let outcome = chain + .verify_bls_to_execution_change_for_gossip(address_change) + .map_err(|e| { + warp_utils::reject::object_invalid(format!( + "gossip verification failed: {:?}", + e + )) + })?; + + if let ObservationOutcome::New(address_change) = outcome { + #[cfg(feature = "withdrawals-processing")] + { + publish_pubsub_message( + &network_tx, + PubsubMessage::BlsToExecutionChange(Box::new( + address_change.as_inner().clone(), + )), + )?; + } + drop(network_tx); + + chain.import_bls_to_execution_change(address_change); + } + + Ok(()) + }) + }, + ); + // GET beacon/deposit_snapshot let get_beacon_deposit_snapshot = eth_v1 .and(warp::path("beacon")) @@ -3170,6 +3223,7 @@ pub fn serve( .or(get_beacon_pool_attester_slashings.boxed()) .or(get_beacon_pool_proposer_slashings.boxed()) .or(get_beacon_pool_voluntary_exits.boxed()) + .or(get_beacon_pool_bls_to_execution_changes.boxed()) .or(get_beacon_deposit_snapshot.boxed()) .or(get_config_fork_schedule.boxed()) .or(get_config_spec.boxed()) @@ -3218,6 +3272,7 @@ pub fn serve( .or(post_beacon_pool_proposer_slashings.boxed()) .or(post_beacon_pool_voluntary_exits.boxed()) .or(post_beacon_pool_sync_committees.boxed()) + .or(post_beacon_pool_bls_to_execution_changes.boxed()) .or(post_validator_duties_attester.boxed()) .or(post_validator_duties_sync.boxed()) .or(post_validator_aggregate_and_proofs.boxed()) diff --git a/beacon_node/lighthouse_network/src/service/gossip_cache.rs b/beacon_node/lighthouse_network/src/service/gossip_cache.rs index 665e383f20..58816251b8 100644 --- a/beacon_node/lighthouse_network/src/service/gossip_cache.rs +++ b/beacon_node/lighthouse_network/src/service/gossip_cache.rs @@ -36,6 +36,8 @@ pub struct GossipCache { signed_contribution_and_proof: Option, /// Timeout for sync committee messages. sync_committee_message: Option, + /// Timeout for signed BLS to execution changes. + bls_to_execution_change: Option, } #[derive(Default)] @@ -59,6 +61,8 @@ pub struct GossipCacheBuilder { signed_contribution_and_proof: Option, /// Timeout for sync committee messages. sync_committee_message: Option, + /// Timeout for signed BLS to execution changes. + bls_to_execution_change: Option, } #[allow(dead_code)] @@ -117,6 +121,12 @@ impl GossipCacheBuilder { self } + /// Timeout for BLS to execution change messages. + pub fn bls_to_execution_change_timeout(mut self, timeout: Duration) -> Self { + self.bls_to_execution_change = Some(timeout); + self + } + pub fn build(self) -> GossipCache { let GossipCacheBuilder { default_timeout, @@ -129,6 +139,7 @@ impl GossipCacheBuilder { attester_slashing, signed_contribution_and_proof, sync_committee_message, + bls_to_execution_change, } = self; GossipCache { expirations: DelayQueue::default(), @@ -142,6 +153,7 @@ impl GossipCacheBuilder { attester_slashing: attester_slashing.or(default_timeout), signed_contribution_and_proof: signed_contribution_and_proof.or(default_timeout), sync_committee_message: sync_committee_message.or(default_timeout), + bls_to_execution_change: bls_to_execution_change.or(default_timeout), } } } @@ -165,6 +177,7 @@ impl GossipCache { GossipKind::AttesterSlashing => self.attester_slashing, GossipKind::SignedContributionAndProof => self.signed_contribution_and_proof, GossipKind::SyncCommitteeMessage(_) => self.sync_committee_message, + GossipKind::BlsToExecutionChange => self.bls_to_execution_change, }; let expire_timeout = match expire_timeout { Some(expire_timeout) => expire_timeout, diff --git a/beacon_node/lighthouse_network/src/service/mod.rs b/beacon_node/lighthouse_network/src/service/mod.rs index 5e770db2e9..65e805ca8b 100644 --- a/beacon_node/lighthouse_network/src/service/mod.rs +++ b/beacon_node/lighthouse_network/src/service/mod.rs @@ -199,6 +199,7 @@ impl Network { .attester_slashing_timeout(half_epoch * 2) // .signed_contribution_and_proof_timeout(timeout) // Do not retry // .sync_committee_message_timeout(timeout) // Do not retry + .bls_to_execution_change_timeout(half_epoch * 2) .build() }; diff --git a/beacon_node/lighthouse_network/src/service/utils.rs b/beacon_node/lighthouse_network/src/service/utils.rs index 8073ae7768..4e81138489 100644 --- a/beacon_node/lighthouse_network/src/service/utils.rs +++ b/beacon_node/lighthouse_network/src/service/utils.rs @@ -253,6 +253,7 @@ pub(crate) fn create_whitelist_filter( add(ProposerSlashing); add(AttesterSlashing); add(SignedContributionAndProof); + add(BlsToExecutionChange); for id in 0..attestation_subnet_count { add(Attestation(SubnetId::new(id))); } diff --git a/beacon_node/lighthouse_network/src/types/pubsub.rs b/beacon_node/lighthouse_network/src/types/pubsub.rs index 1b14c93c09..02f2bfff1d 100644 --- a/beacon_node/lighthouse_network/src/types/pubsub.rs +++ b/beacon_node/lighthouse_network/src/types/pubsub.rs @@ -15,7 +15,8 @@ use types::{ Attestation, AttesterSlashing, BlobsSidecar, EthSpec, ForkContext, ForkName, ProposerSlashing, SignedAggregateAndProof, SignedBeaconBlock, SignedBeaconBlockAltair, SignedBeaconBlockBase, SignedBeaconBlockCapella, SignedBeaconBlockEip4844, SignedBeaconBlockMerge, - SignedContributionAndProof, SignedVoluntaryExit, SubnetId, SyncCommitteeMessage, SyncSubnetId, + SignedBlsToExecutionChange, SignedContributionAndProof, SignedVoluntaryExit, SubnetId, + SyncCommitteeMessage, SyncSubnetId, }; /// TODO(pawan): move this to consensus/types? strictly not a consensus type @@ -48,6 +49,8 @@ pub enum PubsubMessage { SignedContributionAndProof(Box>), /// Gossipsub message providing notification of unaggregated sync committee signatures with its subnet id. SyncCommitteeMessage(Box<(SyncSubnetId, SyncCommitteeMessage)>), + /// Gossipsub message for BLS to execution change messages. + BlsToExecutionChange(Box), } // Implements the `DataTransform` trait of gossipsub to employ snappy compression @@ -133,6 +136,7 @@ impl PubsubMessage { PubsubMessage::AttesterSlashing(_) => GossipKind::AttesterSlashing, PubsubMessage::SignedContributionAndProof(_) => GossipKind::SignedContributionAndProof, PubsubMessage::SyncCommitteeMessage(data) => GossipKind::SyncCommitteeMessage(data.0), + PubsubMessage::BlsToExecutionChange(_) => GossipKind::BlsToExecutionChange, } } @@ -258,6 +262,14 @@ impl PubsubMessage { sync_committee, )))) } + GossipKind::BlsToExecutionChange => { + let bls_to_execution_change = + SignedBlsToExecutionChange::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?; + Ok(PubsubMessage::BlsToExecutionChange(Box::new( + bls_to_execution_change, + ))) + } } } } @@ -280,6 +292,7 @@ impl PubsubMessage { PubsubMessage::Attestation(data) => data.1.as_ssz_bytes(), PubsubMessage::SignedContributionAndProof(data) => data.as_ssz_bytes(), PubsubMessage::SyncCommitteeMessage(data) => data.1.as_ssz_bytes(), + PubsubMessage::BlsToExecutionChange(data) => data.as_ssz_bytes(), } } } @@ -320,6 +333,13 @@ impl std::fmt::Display for PubsubMessage { PubsubMessage::SyncCommitteeMessage(data) => { write!(f, "Sync committee message: subnet_id: {}", *data.0) } + PubsubMessage::BlsToExecutionChange(data) => { + write!( + f, + "Signed BLS to execution change: validator_index: {}, address: {:?}", + data.message.validator_index, data.message.to_execution_address + ) + } } } } diff --git a/beacon_node/lighthouse_network/src/types/topics.rs b/beacon_node/lighthouse_network/src/types/topics.rs index 8cecc2e682..5d020e132c 100644 --- a/beacon_node/lighthouse_network/src/types/topics.rs +++ b/beacon_node/lighthouse_network/src/types/topics.rs @@ -19,8 +19,9 @@ pub const PROPOSER_SLASHING_TOPIC: &str = "proposer_slashing"; pub const ATTESTER_SLASHING_TOPIC: &str = "attester_slashing"; pub const SIGNED_CONTRIBUTION_AND_PROOF_TOPIC: &str = "sync_committee_contribution_and_proof"; pub const SYNC_COMMITTEE_PREFIX_TOPIC: &str = "sync_committee_"; +pub const BLS_TO_EXECUTION_CHANGE_TOPIC: &str = "bls_to_execution_change"; -pub const CORE_TOPICS: [GossipKind; 7] = [ +pub const CORE_TOPICS: [GossipKind; 8] = [ GossipKind::BeaconBlock, GossipKind::BeaconBlocksAndBlobsSidecar, GossipKind::BeaconAggregateAndProof, @@ -28,6 +29,7 @@ pub const CORE_TOPICS: [GossipKind; 7] = [ GossipKind::ProposerSlashing, GossipKind::AttesterSlashing, GossipKind::SignedContributionAndProof, + GossipKind::BlsToExecutionChange, ]; /// A gossipsub topic which encapsulates the type of messages that should be sent and received over @@ -67,6 +69,8 @@ pub enum GossipKind { /// Topic for publishing unaggregated sync committee signatures on a particular subnet. #[strum(serialize = "sync_committee")] SyncCommitteeMessage(SyncSubnetId), + /// Topic for validator messages which change their withdrawal address. + BlsToExecutionChange, } impl std::fmt::Display for GossipKind { @@ -141,6 +145,7 @@ impl GossipTopic { VOLUNTARY_EXIT_TOPIC => GossipKind::VoluntaryExit, PROPOSER_SLASHING_TOPIC => GossipKind::ProposerSlashing, ATTESTER_SLASHING_TOPIC => GossipKind::AttesterSlashing, + BLS_TO_EXECUTION_CHANGE_TOPIC => GossipKind::BlsToExecutionChange, topic => match committee_topic_index(topic) { Some(subnet) => match subnet { Subnet::Attestation(s) => GossipKind::Attestation(s), @@ -177,30 +182,8 @@ impl From for Topic { impl From for String { fn from(topic: GossipTopic) -> String { - let encoding = match topic.encoding { - GossipEncoding::SSZSnappy => SSZ_SNAPPY_ENCODING_POSTFIX, - }; - - let kind = match topic.kind { - GossipKind::BeaconBlock => BEACON_BLOCK_TOPIC.into(), - GossipKind::BeaconBlocksAndBlobsSidecar => BEACON_BLOCK_AND_BLOBS_SIDECAR_TOPIC.into(), - GossipKind::BeaconAggregateAndProof => BEACON_AGGREGATE_AND_PROOF_TOPIC.into(), - GossipKind::VoluntaryExit => VOLUNTARY_EXIT_TOPIC.into(), - GossipKind::ProposerSlashing => PROPOSER_SLASHING_TOPIC.into(), - GossipKind::AttesterSlashing => ATTESTER_SLASHING_TOPIC.into(), - GossipKind::Attestation(index) => format!("{}{}", BEACON_ATTESTATION_PREFIX, *index,), - GossipKind::SignedContributionAndProof => SIGNED_CONTRIBUTION_AND_PROOF_TOPIC.into(), - GossipKind::SyncCommitteeMessage(index) => { - format!("{}{}", SYNC_COMMITTEE_PREFIX_TOPIC, *index) - } - }; - format!( - "/{}/{}/{}/{}", - TOPIC_PREFIX, - hex::encode(topic.fork_digest), - kind, - encoding - ) + // Use the `Display` implementation below. + topic.to_string() } } @@ -222,6 +205,7 @@ impl std::fmt::Display for GossipTopic { GossipKind::SyncCommitteeMessage(index) => { format!("{}{}", SYNC_COMMITTEE_PREFIX_TOPIC, *index) } + GossipKind::BlsToExecutionChange => BLS_TO_EXECUTION_CHANGE_TOPIC.into(), }; write!( f, diff --git a/beacon_node/network/src/beacon_processor/mod.rs b/beacon_node/network/src/beacon_processor/mod.rs index dd28b15c0c..ba71f0d95d 100644 --- a/beacon_node/network/src/beacon_processor/mod.rs +++ b/beacon_node/network/src/beacon_processor/mod.rs @@ -64,8 +64,8 @@ use task_executor::TaskExecutor; use tokio::sync::mpsc; use types::{ Attestation, AttesterSlashing, Hash256, ProposerSlashing, SignedAggregateAndProof, - SignedBeaconBlock, SignedContributionAndProof, SignedVoluntaryExit, SubnetId, - SyncCommitteeMessage, SyncSubnetId, + SignedBeaconBlock, SignedBlsToExecutionChange, SignedContributionAndProof, SignedVoluntaryExit, + SubnetId, SyncCommitteeMessage, SyncSubnetId, }; use work_reprocessing_queue::{ spawn_reprocess_scheduler, QueuedAggregate, QueuedRpcBlock, QueuedUnaggregate, ReadyWork, @@ -163,6 +163,12 @@ const MAX_BLOBS_BY_RANGE_QUEUE_LEN: usize = 1_024; /// will be stored before we start dropping them. const MAX_BLOCKS_BY_ROOTS_QUEUE_LEN: usize = 1_024; +/// Maximum number of `SignedBlsToExecutionChange` messages to queue before dropping them. +/// +/// This value is set high to accommodate the large spike that is expected immediately after Capella +/// is activated. +const MAX_BLS_TO_EXECUTION_CHANGE_QUEUE_LEN: usize = 16_384; + /// The name of the manager tokio task. const MANAGER_TASK_NAME: &str = "beacon_processor_manager"; @@ -206,6 +212,7 @@ pub const BLOCKS_BY_ROOTS_REQUEST: &str = "blocks_by_roots_request"; pub const BLOBS_BY_RANGE_REQUEST: &str = "blobs_by_range_request"; pub const UNKNOWN_BLOCK_ATTESTATION: &str = "unknown_block_attestation"; pub const UNKNOWN_BLOCK_AGGREGATE: &str = "unknown_block_aggregate"; +pub const GOSSIP_BLS_TO_EXECUTION_CHANGE: &str = "gossip_bls_to_execution_change"; /// A simple first-in-first-out queue with a maximum length. struct FifoQueue { @@ -515,6 +522,22 @@ impl WorkEvent { } } + /// Create a new `Work` event for some BLS to execution change. + pub fn gossip_bls_to_execution_change( + message_id: MessageId, + peer_id: PeerId, + bls_to_execution_change: Box, + ) -> Self { + Self { + drop_during_sync: false, + work: Work::GossipBlsToExecutionChange { + message_id, + peer_id, + bls_to_execution_change, + }, + } + } + /// Create a new `Work` event for some block, where the result from computation (if any) is /// sent to the other side of `result_tx`. pub fn rpc_beacon_block( @@ -789,6 +812,11 @@ pub enum Work { request_id: PeerRequestId, request: BlobsByRangeRequest, }, + GossipBlsToExecutionChange { + message_id: MessageId, + peer_id: PeerId, + bls_to_execution_change: Box, + }, } impl Work { @@ -815,6 +843,7 @@ impl Work { Work::BlobsByRangeRequest { .. } => BLOBS_BY_RANGE_REQUEST, Work::UnknownBlockAttestation { .. } => UNKNOWN_BLOCK_ATTESTATION, Work::UnknownBlockAggregate { .. } => UNKNOWN_BLOCK_AGGREGATE, + Work::GossipBlsToExecutionChange { .. } => GOSSIP_BLS_TO_EXECUTION_CHANGE, } } } @@ -960,6 +989,9 @@ impl BeaconProcessor { let mut bbroots_queue = FifoQueue::new(MAX_BLOCKS_BY_ROOTS_QUEUE_LEN); let mut blbrange_queue = FifoQueue::new(MAX_BLOBS_BY_RANGE_QUEUE_LEN); + let mut gossip_bls_to_execution_change_queue = + FifoQueue::new(MAX_BLS_TO_EXECUTION_CHANGE_QUEUE_LEN); + // Channels for sending work to the re-process scheduler (`work_reprocessing_tx`) and to // receive them back once they are ready (`ready_work_rx`). let (ready_work_tx, ready_work_rx) = mpsc::channel(MAX_SCHEDULED_WORK_QUEUE_LEN); @@ -1194,9 +1226,12 @@ impl BeaconProcessor { self.spawn_worker(item, toolbox); } else if let Some(item) = gossip_proposer_slashing_queue.pop() { self.spawn_worker(item, toolbox); - // Check exits last since our validators don't get rewards from them. + // Check exits and address changes late since our validators don't get + // rewards from them. } else if let Some(item) = gossip_voluntary_exit_queue.pop() { self.spawn_worker(item, toolbox); + } else if let Some(item) = gossip_bls_to_execution_change_queue.pop() { + self.spawn_worker(item, toolbox); // Handle backfill sync chain segments. } else if let Some(item) = backfill_chain_segment.pop() { self.spawn_worker(item, toolbox); @@ -1313,6 +1348,9 @@ impl BeaconProcessor { Work::UnknownBlockAggregate { .. } => { unknown_block_aggregate_queue.push(work) } + Work::GossipBlsToExecutionChange { .. } => { + gossip_bls_to_execution_change_queue.push(work, work_id, &self.log) + } } } } @@ -1365,6 +1403,10 @@ impl BeaconProcessor { &metrics::BEACON_PROCESSOR_ATTESTER_SLASHING_QUEUE_TOTAL, gossip_attester_slashing_queue.len() as i64, ); + metrics::set_gauge( + &metrics::BEACON_PROCESSOR_BLS_TO_EXECUTION_CHANGE_QUEUE_TOTAL, + gossip_bls_to_execution_change_queue.len() as i64, + ); if aggregate_queue.is_full() && aggregate_debounce.elapsed() { error!( @@ -1623,6 +1665,20 @@ impl BeaconProcessor { seen_timestamp, ) }), + /* + * BLS to execution change verification. + */ + Work::GossipBlsToExecutionChange { + message_id, + peer_id, + bls_to_execution_change, + } => task_spawner.spawn_blocking(move || { + worker.process_gossip_bls_to_execution_change( + message_id, + peer_id, + *bls_to_execution_change, + ) + }), /* * Verification for beacon blocks received during syncing via RPC. */ diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 37cc1903d3..59f157e21b 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -23,8 +23,9 @@ use store::hot_cold_store::HotColdDBError; use tokio::sync::mpsc; use types::{ Attestation, AttesterSlashing, BlobsSidecar, EthSpec, Hash256, IndexedAttestation, - ProposerSlashing, SignedAggregateAndProof, SignedBeaconBlock, SignedContributionAndProof, - SignedVoluntaryExit, Slot, SubnetId, SyncCommitteeMessage, SyncSubnetId, + ProposerSlashing, SignedAggregateAndProof, SignedBeaconBlock, SignedBlsToExecutionChange, + SignedContributionAndProof, SignedVoluntaryExit, Slot, SubnetId, SyncCommitteeMessage, + SyncSubnetId, }; use super::{ @@ -1192,6 +1193,65 @@ impl Worker { metrics::inc_counter(&metrics::BEACON_PROCESSOR_ATTESTER_SLASHING_IMPORTED_TOTAL); } + pub fn process_gossip_bls_to_execution_change( + self, + message_id: MessageId, + peer_id: PeerId, + bls_to_execution_change: SignedBlsToExecutionChange, + ) { + let validator_index = bls_to_execution_change.message.validator_index; + let address = bls_to_execution_change.message.to_execution_address; + + let change = match self + .chain + .verify_bls_to_execution_change_for_gossip(bls_to_execution_change) + { + Ok(ObservationOutcome::New(change)) => change, + Ok(ObservationOutcome::AlreadyKnown) => { + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); + debug!( + self.log, + "Dropping BLS to execution change"; + "validator_index" => validator_index, + "peer" => %peer_id + ); + return; + } + Err(e) => { + debug!( + self.log, + "Dropping invalid BLS to execution change"; + "validator_index" => validator_index, + "peer" => %peer_id, + "error" => ?e + ); + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); + // We penalize the peer slightly to prevent overuse of invalids. + self.gossip_penalize_peer( + peer_id, + PeerAction::HighToleranceError, + "invalid_bls_to_execution_change", + ); + return; + } + }; + + metrics::inc_counter(&metrics::BEACON_PROCESSOR_BLS_TO_EXECUTION_CHANGE_VERIFIED_TOTAL); + + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Accept); + + self.chain.import_bls_to_execution_change(change); + + debug!( + self.log, + "Successfully imported BLS to execution change"; + "validator_index" => validator_index, + "address" => ?address, + ); + + metrics::inc_counter(&metrics::BEACON_PROCESSOR_BLS_TO_EXECUTION_CHANGE_IMPORTED_TOTAL); + } + /// Process the sync committee signature received from the gossip network and: /// /// - If it passes gossip propagation criteria, tell the network thread to forward it. diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index 94de2988c8..f23ab46a6f 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -143,6 +143,19 @@ lazy_static! { "beacon_processor_attester_slashing_imported_total", "Total number of attester slashings imported to the op pool." ); + // Gossip BLS to execution changes. + pub static ref BEACON_PROCESSOR_BLS_TO_EXECUTION_CHANGE_QUEUE_TOTAL: Result = try_create_int_gauge( + "beacon_processor_bls_to_execution_change_queue_total", + "Count of address changes from gossip waiting to be verified." + ); + pub static ref BEACON_PROCESSOR_BLS_TO_EXECUTION_CHANGE_VERIFIED_TOTAL: Result = try_create_int_counter( + "beacon_processor_bls_to_execution_change_verified_total", + "Total number of address changes verified for propagation." + ); + pub static ref BEACON_PROCESSOR_BLS_TO_EXECUTION_CHANGE_IMPORTED_TOTAL: Result = try_create_int_counter( + "beacon_processor_bls_to_execution_change_imported_total", + "Total number of address changes imported to the op pool." + ); // Rpc blocks. pub static ref BEACON_PROCESSOR_RPC_BLOCK_QUEUE_TOTAL: Result = try_create_int_gauge( "beacon_processor_rpc_block_queue_total", diff --git a/beacon_node/network/src/router/mod.rs b/beacon_node/network/src/router/mod.rs index cb90813b26..75986ff3f2 100644 --- a/beacon_node/network/src/router/mod.rs +++ b/beacon_node/network/src/router/mod.rs @@ -291,6 +291,18 @@ impl Router { sync_committtee_msg.0, ); } + PubsubMessage::BlsToExecutionChange(bls_to_execution_change) => { + trace!( + self.log, + "Received BLS to execution change"; + "peer_id" => %peer_id + ); + self.processor.on_bls_to_execution_change_gossip( + id, + peer_id, + bls_to_execution_change, + ); + } } } } diff --git a/beacon_node/network/src/router/processor.rs b/beacon_node/network/src/router/processor.rs index dadaf60c1e..b8bcab8476 100644 --- a/beacon_node/network/src/router/processor.rs +++ b/beacon_node/network/src/router/processor.rs @@ -19,8 +19,8 @@ use store::SyncCommitteeMessage; use tokio::sync::mpsc; use types::{ Attestation, AttesterSlashing, BlobsSidecar, EthSpec, ProposerSlashing, - SignedAggregateAndProof, SignedBeaconBlock, SignedContributionAndProof, SignedVoluntaryExit, - SubnetId, SyncSubnetId, + SignedAggregateAndProof, SignedBeaconBlock, SignedBlsToExecutionChange, + SignedContributionAndProof, SignedVoluntaryExit, SubnetId, SyncSubnetId, }; /// Processes validated messages from the network. It relays necessary data to the syncing thread @@ -411,6 +411,19 @@ impl Processor { )) } + pub fn on_bls_to_execution_change_gossip( + &mut self, + message_id: MessageId, + peer_id: PeerId, + bls_to_execution_change: Box, + ) { + self.send_beacon_processor_work(BeaconWorkEvent::gossip_bls_to_execution_change( + message_id, + peer_id, + bls_to_execution_change, + )) + } + fn send_beacon_processor_work(&mut self, work: BeaconWorkEvent) { self.beacon_processor_send .try_send(work) diff --git a/beacon_node/operation_pool/Cargo.toml b/beacon_node/operation_pool/Cargo.toml index 8483233589..d752354437 100644 --- a/beacon_node/operation_pool/Cargo.toml +++ b/beacon_node/operation_pool/Cargo.toml @@ -4,6 +4,9 @@ version = "0.2.0" authors = ["Michael Sproul "] edition = "2021" +[features] +withdrawals-processing = [] + [dependencies] derivative = "2.1.1" itertools = "0.10.0" diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index ba0567277b..159454b9e9 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -30,10 +30,10 @@ use std::collections::{hash_map::Entry, HashMap, HashSet}; use std::marker::PhantomData; use std::ptr; use types::{ - sync_aggregate::Error as SyncAggregateError, typenum::Unsigned, Attestation, AttestationData, - AttesterSlashing, BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, ProposerSlashing, - SignedBlsToExecutionChange, SignedVoluntaryExit, Slot, SyncAggregate, - SyncCommitteeContribution, Validator, + sync_aggregate::Error as SyncAggregateError, typenum::Unsigned, AbstractExecPayload, + Attestation, AttestationData, AttesterSlashing, BeaconState, BeaconStateError, ChainSpec, + Epoch, EthSpec, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange, + SignedVoluntaryExit, Slot, SyncAggregate, SyncCommitteeContribution, Validator, }; type SyncContributions = RwLock>>>; @@ -51,6 +51,7 @@ pub struct OperationPool { /// Map from exiting validator to their exit data. voluntary_exits: RwLock>>, /// Map from credential changing validator to their execution change data. + #[cfg(feature = "withdrawals-processing")] bls_to_execution_changes: RwLock>>, /// Reward cache for accelerating attestation packing. reward_cache: RwLock, @@ -432,7 +433,7 @@ impl OperationPool { pub fn prune_proposer_slashings(&self, head_state: &BeaconState) { prune_validator_hash_map( &mut self.proposer_slashings.write(), - |validator| validator.exit_epoch <= head_state.finalized_checkpoint().epoch, + |_, validator| validator.exit_epoch <= head_state.finalized_checkpoint().epoch, head_state, ); } @@ -507,28 +508,115 @@ impl OperationPool { // // We choose simplicity over the gain of pruning more exits since they are small and // should not be seen frequently. - |validator| validator.exit_epoch <= head_state.finalized_checkpoint().epoch, + |_, validator| validator.exit_epoch <= head_state.finalized_checkpoint().epoch, head_state, ); } + /// Insert a BLS to execution change into the pool. + pub fn insert_bls_to_execution_change( + &self, + verified_change: SigVerifiedOp, + ) { + #[cfg(feature = "withdrawals-processing")] + { + self.bls_to_execution_changes.write().insert( + verified_change.as_inner().message.validator_index, + verified_change, + ); + } + #[cfg(not(feature = "withdrawals-processing"))] + { + drop(verified_change); + } + } + /// Get a list of execution changes for inclusion in a block. + /// + /// They're in random `HashMap` order, which isn't exactly fair, but isn't unfair either. pub fn get_bls_to_execution_changes( &self, state: &BeaconState, spec: &ChainSpec, ) -> Vec { - // FIXME: actually implement this - return vec![]; + #[cfg(feature = "withdrawals-processing")] + { + filter_limit_operations( + self.bls_to_execution_changes.read().values(), + |address_change| { + address_change.signature_is_still_valid(&state.fork()) + && state + .get_validator( + address_change.as_inner().message.validator_index as usize, + ) + .map_or(false, |validator| { + !validator.has_eth1_withdrawal_credential(spec) + }) + }, + |address_change| address_change.as_inner().clone(), + T::MaxBlsToExecutionChanges::to_usize(), + ) + } + + #[cfg(not(feature = "withdrawals-processing"))] + { + drop((state, spec)); + vec![] + } + } + + /// Prune BLS to execution changes that have been applied to the state more than 1 block ago. + /// + /// The block check is necessary to avoid pruning too eagerly and losing the ability to include + /// address changes during re-orgs. This is isn't *perfect* so some address changes could + /// still get stuck if there are gnarly re-orgs and the changes can't be widely republished + /// due to the gossip duplicate rules. + pub fn prune_bls_to_execution_changes>( + &self, + head_block: &SignedBeaconBlock, + head_state: &BeaconState, + spec: &ChainSpec, + ) { + #[cfg(feature = "withdrawals-processing")] + { + prune_validator_hash_map( + &mut self.bls_to_execution_changes.write(), + |validator_index, validator| { + validator.has_eth1_withdrawal_credential(spec) + && head_block + .message() + .body() + .bls_to_execution_changes() + .map_or(true, |recent_changes| { + !recent_changes + .iter() + .any(|c| c.message.validator_index == validator_index) + }) + }, + head_state, + ); + } + + #[cfg(not(feature = "withdrawals-processing"))] + { + drop((head_block, head_state, spec)); + } } /// Prune all types of transactions given the latest head state and head fork. - pub fn prune_all(&self, head_state: &BeaconState, current_epoch: Epoch) { + pub fn prune_all>( + &self, + head_block: &SignedBeaconBlock, + head_state: &BeaconState, + current_epoch: Epoch, + spec: &ChainSpec, + ) { self.prune_attestations(current_epoch); self.prune_sync_contributions(head_state.slot()); self.prune_proposer_slashings(head_state); self.prune_attester_slashings(head_state); self.prune_voluntary_exits(head_state); + self.prune_bls_to_execution_changes(head_block, head_state, spec); } /// Total number of voluntary exits in the pool. @@ -594,6 +682,23 @@ impl OperationPool { .map(|(_, exit)| exit.as_inner().clone()) .collect() } + + /// Returns all known `SignedBlsToExecutionChange` objects. + /// + /// This method may return objects that are invalid for block inclusion. + pub fn get_all_bls_to_execution_changes(&self) -> Vec { + #[cfg(feature = "withdrawals-processing")] + { + self.bls_to_execution_changes + .read() + .iter() + .map(|(_, address_change)| address_change.as_inner().clone()) + .collect() + } + + #[cfg(not(feature = "withdrawals-processing"))] + vec![] + } } /// Filter up to a maximum number of operations out of an iterator. @@ -627,7 +732,7 @@ fn prune_validator_hash_map( prune_if: F, head_state: &BeaconState, ) where - F: Fn(&Validator) -> bool, + F: Fn(u64, &Validator) -> bool, T: VerifyOperation, { map.retain(|&validator_index, op| { @@ -635,7 +740,7 @@ fn prune_validator_hash_map( && head_state .validators() .get(validator_index as usize) - .map_or(true, |validator| !prune_if(validator)) + .map_or(true, |validator| !prune_if(validator_index, validator)) }); } diff --git a/beacon_node/operation_pool/src/persistence.rs b/beacon_node/operation_pool/src/persistence.rs index 92c5bd92f6..184b967dbe 100644 --- a/beacon_node/operation_pool/src/persistence.rs +++ b/beacon_node/operation_pool/src/persistence.rs @@ -142,7 +142,8 @@ impl PersistedOperationPool { attester_slashings, proposer_slashings, voluntary_exits, - // FIXME: IMPLEMENT THIS + // FIXME(capella): implement schema migration for address changes in op pool + #[cfg(feature = "withdrawals-processing")] bls_to_execution_changes: Default::default(), reward_cache: Default::default(), _phantom: Default::default(), diff --git a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs index a919f5f5ea..a8d0acc555 100644 --- a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs +++ b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs @@ -158,6 +158,8 @@ where // Deposits are not included because they can legally have invalid signatures. self.include_exits(block)?; self.include_sync_aggregate(block)?; + #[cfg(feature = "withdrawals")] + self.include_bls_to_execution_changes(block)?; Ok(()) } @@ -339,6 +341,26 @@ where Ok(()) } + /// Include the signature of the block's BLS to execution changes for verification. + #[cfg(feature = "withdrawals")] + pub fn include_bls_to_execution_changes>( + &mut self, + block: &'a SignedBeaconBlock, + ) -> Result<()> { + // FIXME(capella): to improve performance we might want to decompress the withdrawal pubkeys + // in parallel. + if let Ok(bls_to_execution_changes) = block.message().body().bls_to_execution_changes() { + for bls_to_execution_change in bls_to_execution_changes { + self.sets.push(bls_execution_change_signature_set( + self.state, + bls_to_execution_change, + self.spec, + )?); + } + } + Ok(()) + } + /// Verify all the signatures that have been included in `self`, returning `true` if and only if /// all the signatures are valid. /// diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index 80dee28f62..e2e434417e 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -15,6 +15,14 @@ use types::{ SignedVoluntaryExit, }; +#[cfg(feature = "withdrawals-processing")] +use { + crate::per_block_processing::{ + errors::BlsExecutionChangeValidationError, verify_bls_to_execution_change, + }, + types::SignedBlsToExecutionChange, +}; + const MAX_FORKS_VERIFIED_AGAINST: usize = 2; /// Wrapper around an operation type that acts as proof that its signature has been checked. @@ -65,7 +73,7 @@ where fn new(op: T, state: &BeaconState) -> Self { let verified_against = VerifiedAgainst { fork_versions: op - .verification_epochs() + .verification_epochs(state.current_epoch()) .into_iter() .map(|epoch| state.fork().get_fork_version(epoch)) .collect(), @@ -87,8 +95,13 @@ where } pub fn signature_is_still_valid(&self, current_fork: &Fork) -> bool { + // Pass the fork's epoch as the effective current epoch. If the message is a current-epoch + // style message like `SignedBlsToExecutionChange` then `get_fork_version` will return the + // current fork version and we'll check it matches the fork version the message was checked + // against. + let effective_current_epoch = current_fork.epoch; self.as_inner() - .verification_epochs() + .verification_epochs(effective_current_epoch) .into_iter() .zip(self.verified_against.fork_versions.iter()) .all(|(epoch, verified_fork_version)| { @@ -118,7 +131,13 @@ pub trait VerifyOperation: Encode + Decode + Sized { /// Return the epochs at which parts of this message were verified. /// /// These need to map 1-to-1 to the `SigVerifiedOp::verified_against` for this type. - fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]>; + /// + /// If the message contains no inherent epoch it should return the `current_epoch` that is + /// passed in, as that's the epoch at which it was verified. + fn verification_epochs( + &self, + current_epoch: Epoch, + ) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]>; } impl VerifyOperation for SignedVoluntaryExit { @@ -134,7 +153,7 @@ impl VerifyOperation for SignedVoluntaryExit { } #[allow(clippy::integer_arithmetic)] - fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { + fn verification_epochs(&self, _: Epoch) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { smallvec![self.message.epoch] } } @@ -152,7 +171,7 @@ impl VerifyOperation for AttesterSlashing { } #[allow(clippy::integer_arithmetic)] - fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { + fn verification_epochs(&self, _: Epoch) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { smallvec![ self.attestation_1.data.target.epoch, self.attestation_2.data.target.epoch @@ -173,7 +192,7 @@ impl VerifyOperation for ProposerSlashing { } #[allow(clippy::integer_arithmetic)] - fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { + fn verification_epochs(&self, _: Epoch) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { // Only need a single epoch because the slots of the two headers must be equal. smallvec![self .signed_header_1 @@ -182,3 +201,25 @@ impl VerifyOperation for ProposerSlashing { .epoch(E::slots_per_epoch())] } } + +#[cfg(feature = "withdrawals-processing")] +impl VerifyOperation for SignedBlsToExecutionChange { + type Error = BlsExecutionChangeValidationError; + + fn validate( + self, + state: &BeaconState, + spec: &ChainSpec, + ) -> Result, Self::Error> { + verify_bls_to_execution_change(state, &self, VerifySignatures::True, spec)?; + Ok(SigVerifiedOp::new(self, state)) + } + + #[allow(clippy::integer_arithmetic)] + fn verification_epochs( + &self, + current_epoch: Epoch, + ) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { + smallvec![current_epoch] + } +} diff --git a/consensus/types/src/bls_to_execution_change.rs b/consensus/types/src/bls_to_execution_change.rs index ca8e0ecf70..fa15a0132b 100644 --- a/consensus/types/src/bls_to_execution_change.rs +++ b/consensus/types/src/bls_to_execution_change.rs @@ -7,8 +7,6 @@ use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; /// A deposit to potentially become a beacon chain validator. -/// -/// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] #[derive( Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, diff --git a/consensus/types/src/signed_bls_to_execution_change.rs b/consensus/types/src/signed_bls_to_execution_change.rs index fc636bb82d..d7cce693b8 100644 --- a/consensus/types/src/signed_bls_to_execution_change.rs +++ b/consensus/types/src/signed_bls_to_execution_change.rs @@ -7,8 +7,6 @@ use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; /// A deposit to potentially become a beacon chain validator. -/// -/// Spec v0.12.1 #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] #[derive( Debug, PartialEq, Hash, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, TestRandom, diff --git a/testing/ef_tests/Makefile b/testing/ef_tests/Makefile index 5dd22de8d6..717ff13c97 100644 --- a/testing/ef_tests/Makefile +++ b/testing/ef_tests/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := f5c7cf78 +TESTS_TAG := v1.3.0-alpha.1 TESTS = general minimal mainnet TARBALLS = $(patsubst %,%-$(TESTS_TAG).tar.gz,$(TESTS))