From 3eb39ad6ab54419272fbd1dfb20b7aee8e0babd7 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 6 Sep 2023 12:31:29 +1000 Subject: [PATCH 1/3] Fix `parent_beacon_block_root` during prep/reorg --- beacon_node/beacon_chain/src/beacon_chain.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 45acab478a7..90aa4f3fab4 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -236,9 +236,12 @@ pub enum ProduceBlockVerification { pub struct PrePayloadAttributes { pub proposer_index: u64, pub prev_randao: Hash256, + /// The block number of the block being built upon (same block as fcU `headBlockHash`). + /// /// The parent block number is not part of the payload attributes sent to the EL, but *is* /// sent to builders via SSE. pub parent_block_number: u64, + /// The block root of the block being built upon (same block as fcU `headBlockHash`). pub parent_beacon_block_root: Hash256, } @@ -4111,10 +4114,10 @@ impl BeaconChain { let proposal_epoch = proposal_slot.epoch(T::EthSpec::slots_per_epoch()); let head_block_root = cached_head.head_block_root(); - let parent_beacon_block_root = cached_head.parent_block_root(); + let head_parent_block_root = cached_head.parent_block_root(); // The proposer head must be equal to the canonical head or its parent. - if proposer_head != head_block_root && proposer_head != parent_beacon_block_root { + if proposer_head != head_block_root && proposer_head != head_parent_block_root { warn!( self.log, "Unable to compute payload attributes"; @@ -4193,7 +4196,7 @@ impl BeaconChain { // Get the `prev_randao` and parent block number. let head_block_number = cached_head.head_block_number()?; - let (prev_randao, parent_block_number) = if proposer_head == parent_beacon_block_root { + let (prev_randao, parent_block_number) = if proposer_head == head_parent_block_root { ( cached_head.parent_random()?, head_block_number.saturating_sub(1), @@ -4206,7 +4209,7 @@ impl BeaconChain { proposer_index, prev_randao, parent_block_number, - parent_beacon_block_root, + parent_beacon_block_root: proposer_head, })) } From de1e89ae9e29c48d48291f0b01c58313b7899924 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 7 Sep 2023 13:32:29 +1000 Subject: [PATCH 2/3] Fix another bug and add tests --- beacon_node/beacon_chain/src/beacon_chain.rs | 9 +- .../beacon_chain/src/execution_payload.rs | 7 +- beacon_node/beacon_chain/src/test_utils.rs | 61 ++-- .../test_utils/execution_block_generator.rs | 267 ++++++++++-------- .../src/test_utils/mock_builder.rs | 34 ++- .../http_api/tests/interactive_tests.rs | 17 +- beacon_node/http_api/tests/tests.rs | 28 -- 7 files changed, 231 insertions(+), 192 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 90aa4f3fab4..8e1ae3de2ef 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4592,8 +4592,13 @@ impl BeaconChain { let prepare_payload_handle = match &state { BeaconState::Base(_) | BeaconState::Altair(_) => None, BeaconState::Merge(_) | BeaconState::Capella(_) | BeaconState::Deneb(_) => { - let prepare_payload_handle = - get_execution_payload(self.clone(), &state, proposer_index, builder_params)?; + let prepare_payload_handle = get_execution_payload( + self.clone(), + &state, + parent_root, + proposer_index, + builder_params, + )?; Some(prepare_payload_handle) } }; diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 7a095fce4de..33c97efd267 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -404,6 +404,7 @@ pub fn get_execution_payload< >( chain: Arc>, state: &BeaconState, + parent_block_root: Hash256, proposer_index: u64, builder_params: BuilderParams, ) -> Result, BlockProductionError> { @@ -426,10 +427,10 @@ pub fn get_execution_payload< &BeaconState::Base(_) | &BeaconState::Altair(_) => None, }; let parent_beacon_block_root = match state { - &BeaconState::Deneb(_) => Some(state.latest_block_header().canonical_root()), - &BeaconState::Merge(_) | &BeaconState::Capella(_) => None, + BeaconState::Deneb(_) => Some(parent_block_root), + BeaconState::Merge(_) | BeaconState::Capella(_) => None, // These shouldn't happen but they're here to make the pattern irrefutable - &BeaconState::Base(_) | &BeaconState::Altair(_) => None, + BeaconState::Base(_) | BeaconState::Altair(_) => None, }; // Spawn a task to obtain the execution payload from the EL via a series of async calls. The diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index dea0f619266..e7d459afe4f 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -890,32 +890,10 @@ where | SignedBeaconBlock::Altair(_) | SignedBeaconBlock::Merge(_) | SignedBeaconBlock::Capella(_) => (signed_block, None), - SignedBeaconBlock::Deneb(_) => { - if let Some(blobs) = maybe_blob_sidecars { - let signed_blobs: SignedSidecarList> = Vec::from(blobs) - .into_iter() - .map(|blob| { - blob.sign( - &self.validator_keypairs[proposer_index].sk, - &state.fork(), - state.genesis_validators_root(), - &self.spec, - ) - }) - .collect::>() - .into(); - let mut guard = self.blob_signature_cache.write(); - for blob in &signed_blobs { - guard.insert( - BlobSignatureKey::new(blob.message.block_root, blob.message.index), - blob.signature.clone(), - ); - } - (signed_block, Some(signed_blobs)) - } else { - (signed_block, None) - } - } + SignedBeaconBlock::Deneb(_) => ( + signed_block, + maybe_blob_sidecars.map(|blobs| self.sign_blobs(blobs, &state, proposer_index)), + ), }; (block_contents, state) @@ -1037,6 +1015,35 @@ where ) } + /// Sign blobs, and cache their signatures. + pub fn sign_blobs( + &self, + blobs: BlobSidecarList, + state: &BeaconState, + proposer_index: usize, + ) -> SignedSidecarList> { + let signed_blobs: SignedSidecarList> = Vec::from(blobs) + .into_iter() + .map(|blob| { + blob.sign( + &self.validator_keypairs[proposer_index].sk, + &state.fork(), + state.genesis_validators_root(), + &self.spec, + ) + }) + .collect::>() + .into(); + let mut guard = self.blob_signature_cache.write(); + for blob in &signed_blobs { + guard.insert( + BlobSignatureKey::new(blob.message.block_root, blob.message.index), + blob.signature.clone(), + ); + } + signed_blobs + } + /// Produces an "unaggregated" attestation for the given `slot` and `index` that attests to /// `beacon_block_root`. The provided `state` should match the `block.state_root` for the /// `block` identified by `beacon_block_root`. @@ -1940,7 +1947,7 @@ where ) .await? .try_into() - .unwrap(); + .expect("block blobs are available"); self.chain.recompute_head_at_current_slot().await; Ok(block_hash) } diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index 444f9353312..4f7189bfb75 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -17,9 +17,9 @@ use std::sync::Arc; use tree_hash::TreeHash; use tree_hash_derive::TreeHash; use types::{ - BlobSidecar, ChainSpec, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, - ExecutionPayloadDeneb, ExecutionPayloadHeader, ExecutionPayloadMerge, ForkName, Hash256, - Transactions, Uint256, + Address, BlobSidecar, ChainSpec, EthSpec, ExecutionBlockHash, ExecutionPayload, + ExecutionPayloadCapella, ExecutionPayloadDeneb, ExecutionPayloadHeader, ExecutionPayloadMerge, + ForkName, Hash256, Transactions, Uint256, }; use super::DEFAULT_TERMINAL_BLOCK; @@ -121,6 +121,10 @@ pub struct ExecutionBlockGenerator { pub pending_payloads: HashMap>, pub next_payload_id: u64, pub payload_ids: HashMap>, + /// Payload attributes should be uniquely determined by the head block hash, fee recipient and + /// timestamp (slot). + pub payload_attributes: + HashMap<(ExecutionBlockHash, Address, u64), (PayloadId, PayloadAttributes)>, /* * Post-merge fork triggers */ @@ -153,6 +157,7 @@ impl ExecutionBlockGenerator { pending_payloads: <_>::default(), next_payload_id: 0, payload_ids: <_>::default(), + payload_attributes: <_>::default(), shanghai_time, cancun_time, blobs_bundles: <_>::default(), @@ -449,20 +454,18 @@ impl ExecutionBlockGenerator { ) -> Result { // This is meant to cover starting post-merge transition at genesis. Useful for // testing Capella forks and later. + let head_block_hash = forkchoice_state.head_block_hash; if let Some(genesis_pow_block) = self.block_by_number(0) { - if genesis_pow_block.block_hash() == forkchoice_state.head_block_hash { - self.terminal_block_hash = forkchoice_state.head_block_hash; + if genesis_pow_block.block_hash() == head_block_hash { + self.terminal_block_hash = head_block_hash; } } - if let Some(payload) = self - .pending_payloads - .remove(&forkchoice_state.head_block_hash) - { + if let Some(payload) = self.pending_payloads.remove(&head_block_hash) { self.insert_block(Block::PoS(payload))?; } - let unknown_head_block_hash = !self.blocks.contains_key(&forkchoice_state.head_block_hash); + let unknown_head_block_hash = !self.blocks.contains_key(&head_block_hash); let unknown_safe_block_hash = forkchoice_state.safe_block_hash != ExecutionBlockHash::zero() && !self.blocks.contains_key(&forkchoice_state.safe_block_hash); @@ -495,114 +498,41 @@ impl ExecutionBlockGenerator { let parent = self .blocks - .get(&forkchoice_state.head_block_hash) - .ok_or_else(|| { - format!( - "unknown parent block {:?}", - forkchoice_state.head_block_hash - ) - })?; - - let id = payload_id_from_u64(self.next_payload_id); - self.next_payload_id += 1; - - let mut execution_payload = match &attributes { - PayloadAttributes::V1(pa) => ExecutionPayload::Merge(ExecutionPayloadMerge { - parent_hash: forkchoice_state.head_block_hash, - fee_recipient: pa.suggested_fee_recipient, - receipts_root: Hash256::repeat_byte(42), - state_root: Hash256::repeat_byte(43), - logs_bloom: vec![0; 256].into(), - prev_randao: pa.prev_randao, - block_number: parent.block_number() + 1, - gas_limit: GAS_LIMIT, - gas_used: GAS_USED, - timestamp: pa.timestamp, - extra_data: "block gen was here".as_bytes().to_vec().into(), - base_fee_per_gas: Uint256::one(), - block_hash: ExecutionBlockHash::zero(), - transactions: vec![].into(), - }), - PayloadAttributes::V2(pa) => match self.get_fork_at_timestamp(pa.timestamp) { - ForkName::Merge => ExecutionPayload::Merge(ExecutionPayloadMerge { - parent_hash: forkchoice_state.head_block_hash, - fee_recipient: pa.suggested_fee_recipient, - receipts_root: Hash256::repeat_byte(42), - state_root: Hash256::repeat_byte(43), - logs_bloom: vec![0; 256].into(), - prev_randao: pa.prev_randao, - block_number: parent.block_number() + 1, - gas_limit: GAS_LIMIT, - gas_used: GAS_USED, - timestamp: pa.timestamp, - extra_data: "block gen was here".as_bytes().to_vec().into(), - base_fee_per_gas: Uint256::one(), - block_hash: ExecutionBlockHash::zero(), - transactions: vec![].into(), - }), - ForkName::Capella => ExecutionPayload::Capella(ExecutionPayloadCapella { - parent_hash: forkchoice_state.head_block_hash, - fee_recipient: pa.suggested_fee_recipient, - receipts_root: Hash256::repeat_byte(42), - state_root: Hash256::repeat_byte(43), - logs_bloom: vec![0; 256].into(), - prev_randao: pa.prev_randao, - block_number: parent.block_number() + 1, - gas_limit: GAS_LIMIT, - gas_used: GAS_USED, - timestamp: pa.timestamp, - extra_data: "block gen was here".as_bytes().to_vec().into(), - base_fee_per_gas: Uint256::one(), - block_hash: ExecutionBlockHash::zero(), - transactions: vec![].into(), - withdrawals: pa.withdrawals.clone().into(), - }), - _ => unreachable!(), - }, - PayloadAttributes::V3(pa) => ExecutionPayload::Deneb(ExecutionPayloadDeneb { - parent_hash: forkchoice_state.head_block_hash, - fee_recipient: pa.suggested_fee_recipient, - receipts_root: Hash256::repeat_byte(42), - state_root: Hash256::repeat_byte(43), - logs_bloom: vec![0; 256].into(), - prev_randao: pa.prev_randao, - block_number: parent.block_number() + 1, - gas_limit: GAS_LIMIT, - gas_used: GAS_USED, - timestamp: pa.timestamp, - extra_data: "block gen was here".as_bytes().to_vec().into(), - base_fee_per_gas: Uint256::one(), - block_hash: ExecutionBlockHash::zero(), - transactions: vec![].into(), - withdrawals: pa.withdrawals.clone().into(), - blob_gas_used: 0, - excess_blob_gas: 0, - }), - }; - - match execution_payload.fork_name() { - ForkName::Base | ForkName::Altair | ForkName::Merge | ForkName::Capella => {} - ForkName::Deneb => { - // get random number between 0 and Max Blobs - let num_blobs = rand::random::() % (T::max_blobs_per_block() + 1); - let kzg = self.kzg.as_ref().ok_or("kzg not initialized")?; - let (bundle, transactions) = generate_random_blobs(num_blobs, kzg)?; - for tx in Vec::from(transactions) { - execution_payload - .transactions_mut() - .push(tx) - .map_err(|_| "transactions are full".to_string())?; - } - self.blobs_bundles.insert(id, bundle); - } + .get(&head_block_hash) + .cloned() + .ok_or_else(|| format!("unknown parent block {head_block_hash:?}"))?; + let fee_recipient = attributes.suggested_fee_recipient(); + + if let Some((existing_id, existing_payload_attributes)) = self + .payload_attributes + .get(&(head_block_hash, fee_recipient, attributes.timestamp())) + { + // Check uniqueness of payload attributes. + assert_eq!( + *existing_payload_attributes, attributes, + "inconsistent payload attributes" + ); + + // Return existing payload ID. + Some(*existing_id) + } else { + let id = payload_id_from_u64(self.next_payload_id); + self.next_payload_id += 1; + + let execution_payload = self.build_new_execution_payload( + head_block_hash, + &parent, + id, + &attributes, + )?; + self.payload_ids.insert(id, execution_payload); + self.payload_attributes.insert( + (head_block_hash, fee_recipient, attributes.timestamp()), + (id, attributes), + ); + + Some(id) } - - *execution_payload.block_hash_mut() = - ExecutionBlockHash::from_root(execution_payload.tree_hash_root()); - - self.payload_ids.insert(id, execution_payload); - - Some(id) } }; @@ -626,6 +556,109 @@ impl ExecutionBlockGenerator { payload_id: id.map(Into::into), }) } + + pub fn build_new_execution_payload( + &mut self, + head_block_hash: ExecutionBlockHash, + parent: &Block, + id: PayloadId, + attributes: &PayloadAttributes, + ) -> Result, String> { + let mut execution_payload = match attributes { + PayloadAttributes::V1(pa) => ExecutionPayload::Merge(ExecutionPayloadMerge { + parent_hash: head_block_hash, + fee_recipient: pa.suggested_fee_recipient, + receipts_root: Hash256::repeat_byte(42), + state_root: Hash256::repeat_byte(43), + logs_bloom: vec![0; 256].into(), + prev_randao: pa.prev_randao, + block_number: parent.block_number() + 1, + gas_limit: GAS_LIMIT, + gas_used: GAS_USED, + timestamp: pa.timestamp, + extra_data: "block gen was here".as_bytes().to_vec().into(), + base_fee_per_gas: Uint256::one(), + block_hash: ExecutionBlockHash::zero(), + transactions: vec![].into(), + }), + PayloadAttributes::V2(pa) => match self.get_fork_at_timestamp(pa.timestamp) { + ForkName::Merge => ExecutionPayload::Merge(ExecutionPayloadMerge { + parent_hash: head_block_hash, + fee_recipient: pa.suggested_fee_recipient, + receipts_root: Hash256::repeat_byte(42), + state_root: Hash256::repeat_byte(43), + logs_bloom: vec![0; 256].into(), + prev_randao: pa.prev_randao, + block_number: parent.block_number() + 1, + gas_limit: GAS_LIMIT, + gas_used: GAS_USED, + timestamp: pa.timestamp, + extra_data: "block gen was here".as_bytes().to_vec().into(), + base_fee_per_gas: Uint256::one(), + block_hash: ExecutionBlockHash::zero(), + transactions: vec![].into(), + }), + ForkName::Capella => ExecutionPayload::Capella(ExecutionPayloadCapella { + parent_hash: head_block_hash, + fee_recipient: pa.suggested_fee_recipient, + receipts_root: Hash256::repeat_byte(42), + state_root: Hash256::repeat_byte(43), + logs_bloom: vec![0; 256].into(), + prev_randao: pa.prev_randao, + block_number: parent.block_number() + 1, + gas_limit: GAS_LIMIT, + gas_used: GAS_USED, + timestamp: pa.timestamp, + extra_data: "block gen was here".as_bytes().to_vec().into(), + base_fee_per_gas: Uint256::one(), + block_hash: ExecutionBlockHash::zero(), + transactions: vec![].into(), + withdrawals: pa.withdrawals.clone().into(), + }), + _ => unreachable!(), + }, + PayloadAttributes::V3(pa) => ExecutionPayload::Deneb(ExecutionPayloadDeneb { + parent_hash: head_block_hash, + fee_recipient: pa.suggested_fee_recipient, + receipts_root: Hash256::repeat_byte(42), + state_root: Hash256::repeat_byte(43), + logs_bloom: vec![0; 256].into(), + prev_randao: pa.prev_randao, + block_number: parent.block_number() + 1, + gas_limit: GAS_LIMIT, + gas_used: GAS_USED, + timestamp: pa.timestamp, + extra_data: "block gen was here".as_bytes().to_vec().into(), + base_fee_per_gas: Uint256::one(), + block_hash: ExecutionBlockHash::zero(), + transactions: vec![].into(), + withdrawals: pa.withdrawals.clone().into(), + blob_gas_used: 0, + excess_blob_gas: 0, + }), + }; + + match execution_payload.fork_name() { + ForkName::Base | ForkName::Altair | ForkName::Merge | ForkName::Capella => {} + ForkName::Deneb => { + // get random number between 0 and Max Blobs + let num_blobs = rand::random::() % (T::max_blobs_per_block() + 1); + let kzg = self.kzg.as_ref().ok_or("kzg not initialized")?; + let (bundle, transactions) = generate_random_blobs(num_blobs, kzg)?; + for tx in Vec::from(transactions) { + execution_payload + .transactions_mut() + .push(tx) + .map_err(|_| "transactions are full".to_string())?; + } + self.blobs_bundles.insert(id, bundle); + } + } + + *execution_payload.block_hash_mut() = + ExecutionBlockHash::from_root(execution_payload.tree_hash_root()); + Ok(execution_payload) + } } pub fn generate_random_blobs( diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index ee6854799e2..18a0a90e625 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -402,22 +402,34 @@ impl mev_rs::BlindedBlockProvider for MockBuilder { let prev_randao = head_state .get_randao_mix(head_state.current_epoch()) .map_err(convert_err)?; - let parent_root = head_state.latest_block_header().parent_root; + let expected_withdrawals = match fork { + ForkName::Base | ForkName::Altair | ForkName::Merge => None, + ForkName::Capella | ForkName::Deneb => Some( + self.beacon_client + .get_expected_withdrawals(&StateId::Head) + .await + .unwrap() + .data, + ), + }; let payload_attributes = match fork { - ForkName::Merge => { - PayloadAttributes::new(timestamp, *prev_randao, fee_recipient, None, None) - } - // the withdrawals root is filled in by operations - ForkName::Capella => { - PayloadAttributes::new(timestamp, *prev_randao, fee_recipient, Some(vec![]), None) - } + // the withdrawals root is filled in by operations, but we supply the valid withdrawals + // first in order to avoid tripping the execution block generator's payload attributes + // check. + ForkName::Merge | ForkName::Capella => PayloadAttributes::new( + timestamp, + *prev_randao, + fee_recipient, + expected_withdrawals, + None, + ), ForkName::Deneb => PayloadAttributes::new( timestamp, *prev_randao, fee_recipient, - Some(vec![]), - Some(parent_root), + expected_withdrawals, + Some(head_block_root), ), ForkName::Base | ForkName::Altair => { return Err(MevError::InvalidFork); @@ -451,7 +463,7 @@ impl mev_rs::BlindedBlockProvider for MockBuilder { .map_err(convert_err)? .into(); - let header: ExecutionPayloadHeader = match payload { + let header = match payload { ExecutionPayload::Merge(payload) => ExecutionPayloadHeader::Merge((&payload).into()), ExecutionPayload::Capella(payload) => { ExecutionPayloadHeader::Capella((&payload).into()) diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index f27a4d9519d..17213d6f530 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -391,8 +391,8 @@ pub async fn proposer_boost_re_org_test( ) { assert!(head_slot > 0); - // Test using Capella so that we simulate conditions as similar to mainnet as possible. - let mut spec = ForkName::Capella.make_genesis_spec(E::default_spec()); + // Test using the latest fork so that we simulate conditions as similar to mainnet as possible. + let mut spec = ForkName::latest().make_genesis_spec(E::default_spec()); spec.terminal_total_difficulty = 1.into(); // Ensure there are enough validators to have `attesters_per_slot`. @@ -623,7 +623,7 @@ pub async fn proposer_boost_re_org_test( .await .unwrap() .data; - let unsigned_block_c = unsigned_block_contents_c.deconstruct().0; + let (unsigned_block_c, block_c_blobs) = unsigned_block_contents_c.deconstruct(); let block_c = harness.sign_beacon_block(unsigned_block_c, &state_b); if should_re_org { @@ -634,9 +634,13 @@ pub async fn proposer_boost_re_org_test( assert_eq!(block_c.parent_root(), block_b_root); } + // Sign blobs. + let block_c_signed_blobs = + block_c_blobs.map(|blobs| harness.sign_blobs(blobs, &state_b, proposer_index)); + // Applying block C should cause it to become head regardless (re-org or continuation). let block_root_c = harness - .process_block_result((block_c.clone(), None)) + .process_block_result((block_c.clone(), block_c_signed_blobs)) .await .unwrap() .into(); @@ -699,6 +703,11 @@ pub async fn proposer_boost_re_org_test( assert_ne!(expected_withdrawals, pre_advance_withdrawals); } + // Check that the `parent_beacon_block_root` of the payload attributes are correct. + if let Ok(parent_beacon_block_root) = payload_attribs.parent_beacon_block_root() { + assert_eq!(parent_beacon_block_root, block_c.parent_root()); + } + let lookahead = slot_clock .start_of(slot_c) .unwrap() diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index a6faeb656bb..f3daf237b49 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -3973,20 +3973,6 @@ impl ApiTester { ))); let slot = self.chain.slot().unwrap(); - let propose_state = self - .harness - .chain - .state_at_slot(slot, StateSkipConfig::WithoutStateRoots) - .unwrap(); - let withdrawals = get_expected_withdrawals(&propose_state, &self.chain.spec).unwrap(); - let withdrawals_root = withdrawals.tree_hash_root(); - // Set withdrawals root for builder - self.mock_builder - .as_ref() - .unwrap() - .builder - .add_operation(Operation::WithdrawalsRoot(withdrawals_root)); - let epoch = self.chain.epoch().unwrap(); let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; @@ -4024,20 +4010,6 @@ impl ApiTester { ))); let slot = self.chain.slot().unwrap(); - let propose_state = self - .harness - .chain - .state_at_slot(slot, StateSkipConfig::WithoutStateRoots) - .unwrap(); - let withdrawals = get_expected_withdrawals(&propose_state, &self.chain.spec).unwrap(); - let withdrawals_root = withdrawals.tree_hash_root(); - // Set withdrawals root for builder - self.mock_builder - .as_ref() - .unwrap() - .builder - .add_operation(Operation::WithdrawalsRoot(withdrawals_root)); - let epoch = self.chain.epoch().unwrap(); let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; From 6845e580c2fdcf99c2d78c3a7e241c752df7973c Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 7 Sep 2023 14:51:40 +1000 Subject: [PATCH 3/3] Remove overzealous payload attributes check Too hard. I give up. --- .../test_utils/execution_block_generator.rs | 52 +++++-------------- .../src/test_utils/mock_builder.rs | 5 +- 2 files changed, 15 insertions(+), 42 deletions(-) diff --git a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs index 4f7189bfb75..c839d5da6ca 100644 --- a/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs +++ b/beacon_node/execution_layer/src/test_utils/execution_block_generator.rs @@ -17,9 +17,9 @@ use std::sync::Arc; use tree_hash::TreeHash; use tree_hash_derive::TreeHash; use types::{ - Address, BlobSidecar, ChainSpec, EthSpec, ExecutionBlockHash, ExecutionPayload, - ExecutionPayloadCapella, ExecutionPayloadDeneb, ExecutionPayloadHeader, ExecutionPayloadMerge, - ForkName, Hash256, Transactions, Uint256, + BlobSidecar, ChainSpec, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, + ExecutionPayloadDeneb, ExecutionPayloadHeader, ExecutionPayloadMerge, ForkName, Hash256, + Transactions, Uint256, }; use super::DEFAULT_TERMINAL_BLOCK; @@ -121,10 +121,6 @@ pub struct ExecutionBlockGenerator { pub pending_payloads: HashMap>, pub next_payload_id: u64, pub payload_ids: HashMap>, - /// Payload attributes should be uniquely determined by the head block hash, fee recipient and - /// timestamp (slot). - pub payload_attributes: - HashMap<(ExecutionBlockHash, Address, u64), (PayloadId, PayloadAttributes)>, /* * Post-merge fork triggers */ @@ -157,7 +153,6 @@ impl ExecutionBlockGenerator { pending_payloads: <_>::default(), next_payload_id: 0, payload_ids: <_>::default(), - payload_attributes: <_>::default(), shanghai_time, cancun_time, blobs_bundles: <_>::default(), @@ -501,38 +496,15 @@ impl ExecutionBlockGenerator { .get(&head_block_hash) .cloned() .ok_or_else(|| format!("unknown parent block {head_block_hash:?}"))?; - let fee_recipient = attributes.suggested_fee_recipient(); - - if let Some((existing_id, existing_payload_attributes)) = self - .payload_attributes - .get(&(head_block_hash, fee_recipient, attributes.timestamp())) - { - // Check uniqueness of payload attributes. - assert_eq!( - *existing_payload_attributes, attributes, - "inconsistent payload attributes" - ); - - // Return existing payload ID. - Some(*existing_id) - } else { - let id = payload_id_from_u64(self.next_payload_id); - self.next_payload_id += 1; - - let execution_payload = self.build_new_execution_payload( - head_block_hash, - &parent, - id, - &attributes, - )?; - self.payload_ids.insert(id, execution_payload); - self.payload_attributes.insert( - (head_block_hash, fee_recipient, attributes.timestamp()), - (id, attributes), - ); - - Some(id) - } + + let id = payload_id_from_u64(self.next_payload_id); + self.next_payload_id += 1; + + let execution_payload = + self.build_new_execution_payload(head_block_hash, &parent, id, &attributes)?; + self.payload_ids.insert(id, execution_payload); + + Some(id) } }; diff --git a/beacon_node/execution_layer/src/test_utils/mock_builder.rs b/beacon_node/execution_layer/src/test_utils/mock_builder.rs index 18a0a90e625..ccf6fecf283 100644 --- a/beacon_node/execution_layer/src/test_utils/mock_builder.rs +++ b/beacon_node/execution_layer/src/test_utils/mock_builder.rs @@ -415,8 +415,9 @@ impl mev_rs::BlindedBlockProvider for MockBuilder { let payload_attributes = match fork { // the withdrawals root is filled in by operations, but we supply the valid withdrawals - // first in order to avoid tripping the execution block generator's payload attributes - // check. + // first to avoid polluting the execution block generator with invalid payload attributes + // NOTE: this was part of an effort to add payload attribute uniqueness checks, + // which was abandoned because it broke too many tests in subtle ways. ForkName::Merge | ForkName::Capella => PayloadAttributes::new( timestamp, *prev_randao,