From bf65ed45c5c7e0ea4a8ea5228cb813fa5b9a3cac Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 9 Jan 2025 14:58:09 +0100 Subject: [PATCH] chore!: make senders fields private (#13752) --- crates/chain-state/src/in_memory.rs | 4 +- crates/chain-state/src/test_utils.rs | 5 +- crates/engine/tree/src/download.rs | 12 ++--- crates/engine/tree/src/tree/mod.rs | 19 ++++--- crates/evm/execution-types/src/chain.rs | 4 +- crates/exex/exex/src/backfill/job.rs | 2 +- crates/optimism/evm/src/execute.rs | 16 +++--- crates/primitives/src/block.rs | 54 +++++++++++++++++-- .../rpc-eth-api/src/helpers/pending_block.rs | 2 +- crates/rpc/rpc-types-compat/src/block.rs | 2 +- crates/rpc/rpc/src/validation.rs | 2 +- .../src/providers/blockchain_provider.rs | 8 +-- .../src/providers/database/provider.rs | 2 +- .../storage/provider/src/test_utils/blocks.rs | 25 +++++++-- .../transaction-pool/src/blobstore/tracker.rs | 16 +++--- 15 files changed, 117 insertions(+), 56 deletions(-) diff --git a/crates/chain-state/src/in_memory.rs b/crates/chain-state/src/in_memory.rs index d0aafbd57e2b..fac148d4a716 100644 --- a/crates/chain-state/src/in_memory.rs +++ b/crates/chain-state/src/in_memory.rs @@ -648,7 +648,7 @@ impl BlockState { pub fn sealed_block_with_senders(&self) -> SealedBlockWithSenders { let block = self.block.block().clone(); let senders = self.block.senders().clone(); - SealedBlockWithSenders { block, senders } + SealedBlockWithSenders::new_unchecked(block, senders) } /// Returns the hash of executed block that determines the state. @@ -840,7 +840,7 @@ impl ExecutedBlock { /// /// Note: this clones the block and senders. pub fn sealed_block_with_senders(&self) -> SealedBlockWithSenders { - SealedBlockWithSenders { block: (*self.block).clone(), senders: (*self.senders).clone() } + SealedBlockWithSenders::new_unchecked((*self.block).clone(), (*self.senders).clone()) } /// Returns a reference to the block's execution outcome diff --git a/crates/chain-state/src/test_utils.rs b/crates/chain-state/src/test_utils.rs index 2ce18ea6d74c..a0ddeb8d0fe2 100644 --- a/crates/chain-state/src/test_utils.rs +++ b/crates/chain-state/src/test_utils.rs @@ -207,9 +207,10 @@ impl TestBlockBuilder { ) -> ExecutedBlock { let block_with_senders = self.generate_random_block(block_number, parent_hash); + let (block, senders) = block_with_senders.split(); ExecutedBlock::new( - Arc::new(block_with_senders.block.clone()), - Arc::new(block_with_senders.senders), + Arc::new(block), + Arc::new(senders), Arc::new(ExecutionOutcome::new( BundleState::default(), receipts, diff --git a/crates/engine/tree/src/download.rs b/crates/engine/tree/src/download.rs index 26c5b405de06..1359843c0a35 100644 --- a/crates/engine/tree/src/download.rs +++ b/crates/engine/tree/src/download.rs @@ -233,10 +233,9 @@ where .into_iter() .map(|b| { let senders = b.senders().unwrap_or_default(); - OrderedSealedBlockWithSenders(SealedBlockWithSenders { - block: b, - senders, - }) + OrderedSealedBlockWithSenders(SealedBlockWithSenders::new_unchecked( + b, senders, + )) }) .map(Reverse), ); @@ -290,14 +289,13 @@ impl Ord for OrderedSealedBlockWithSenders { impl From> for OrderedSealedBlockWithSenders { fn from(block: SealedBlockFor) -> Self { let senders = block.senders().unwrap_or_default(); - Self(SealedBlockWithSenders { block, senders }) + Self(SealedBlockWithSenders::new_unchecked(block, senders)) } } impl From> for SealedBlockWithSenders { fn from(value: OrderedSealedBlockWithSenders) -> Self { - let senders = value.0.senders; - Self { block: value.0.block, senders } + value.0 } } diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index 03b9f0ab50b6..1103c569f34c 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -1597,10 +1597,11 @@ where return Ok(None) }; - let SealedBlockWithSenders { block, senders } = self + let (block, senders) = self .provider .sealed_block_with_senders(hash.into(), TransactionVariant::WithHash)? - .ok_or_else(|| ProviderError::HeaderNotFound(hash.into()))?; + .ok_or_else(|| ProviderError::HeaderNotFound(hash.into()))? + .split(); let execution_output = self .provider .get_state(block.number())? @@ -2452,7 +2453,7 @@ where let executed: ExecutedBlock = ExecutedBlock { block: sealed_block.clone(), - senders: Arc::new(block.senders), + senders: Arc::new(block.senders().to_vec()), execution_output: Arc::new(ExecutionOutcome::from((output, block_number))), hashed_state: Arc::new(hashed_state), trie: Arc::new(trie_output), @@ -3002,9 +3003,11 @@ mod tests { self.persist_blocks( blocks .into_iter() - .map(|b| SealedBlockWithSenders { - block: (*b.block).clone(), - senders: b.senders.to_vec(), + .map(|b| { + SealedBlockWithSenders::new_unchecked( + (*b.block).clone(), + b.senders().clone(), + ) }) .collect(), ); @@ -3710,7 +3713,7 @@ mod tests { for block in &chain_a { test_harness.tree.state.tree_state.insert_executed(ExecutedBlock { block: Arc::new(block.block.clone()), - senders: Arc::new(block.senders.clone()), + senders: Arc::new(block.senders().to_vec()), execution_output: Arc::new(ExecutionOutcome::default()), hashed_state: Arc::new(HashedPostState::default()), trie: Arc::new(TrieUpdates::default()), @@ -3721,7 +3724,7 @@ mod tests { for block in &chain_b { test_harness.tree.state.tree_state.insert_executed(ExecutedBlock { block: Arc::new(block.block.clone()), - senders: Arc::new(block.senders.clone()), + senders: Arc::new(block.senders().to_vec()), execution_output: Arc::new(ExecutionOutcome::default()), hashed_state: Arc::new(HashedPostState::default()), trie: Arc::new(TrieUpdates::default()), diff --git a/crates/evm/execution-types/src/chain.rs b/crates/evm/execution-types/src/chain.rs index 43b5269bef3b..e5f7270bfef7 100644 --- a/crates/evm/execution-types/src/chain.rs +++ b/crates/evm/execution-types/src/chain.rs @@ -784,13 +784,13 @@ mod tests { let block1_hash = B256::new([15; 32]); block1.set_block_number(1); block1.set_hash(block1_hash); - block1.senders.push(Address::new([4; 20])); + block1.push_sender(Address::new([4; 20])); let mut block2: SealedBlockWithSenders = Default::default(); let block2_hash = B256::new([16; 32]); block2.set_block_number(2); block2.set_hash(block2_hash); - block2.senders.push(Address::new([4; 20])); + block2.push_sender(Address::new([4; 20])); let mut block_state_extended = execution_outcome1; block_state_extended.extend(execution_outcome2); diff --git a/crates/exex/exex/src/backfill/job.rs b/crates/exex/exex/src/backfill/job.rs index 3bb0e04ec25f..126a2562f708 100644 --- a/crates/exex/exex/src/backfill/job.rs +++ b/crates/exex/exex/src/backfill/job.rs @@ -107,7 +107,7 @@ where let execute_start = Instant::now(); // Unseal the block for execution - let (block, senders) = block.into_components(); + let (block, senders) = block.split(); let (header, body) = block.split(); let (unsealed_header, hash) = header.split(); let block = P::Block::new(unsealed_header, body).with_senders_unchecked(senders); diff --git a/crates/optimism/evm/src/execute.rs b/crates/optimism/evm/src/execute.rs index 402f0ab16f7a..ac5d75062656 100644 --- a/crates/optimism/evm/src/execute.rs +++ b/crates/optimism/evm/src/execute.rs @@ -420,13 +420,13 @@ mod tests { // Attempt to execute a block with one deposit and one non-deposit transaction executor - .execute_and_verify_one(&BlockWithSenders { - block: Block { + .execute_and_verify_one(&BlockWithSenders::new_unchecked( + Block { header, body: BlockBody { transactions: vec![tx, tx_deposit], ..Default::default() }, }, - senders: vec![addr, addr], - }) + vec![addr, addr], + )) .unwrap(); let receipts = executor.receipts(); @@ -496,13 +496,13 @@ mod tests { // attempt to execute an empty block with parent beacon block root, this should not fail executor - .execute_and_verify_one(&BlockWithSenders { - block: Block { + .execute_and_verify_one(&BlockWithSenders::new_unchecked( + Block { header, body: BlockBody { transactions: vec![tx, tx_deposit], ..Default::default() }, }, - senders: vec![addr, addr], - }) + vec![addr, addr], + )) .expect("Executing a block while canyon is active should not fail"); let receipts = executor.receipts(); diff --git a/crates/primitives/src/block.rs b/crates/primitives/src/block.rs index fd0dc0cee40a..8c6c8a870d61 100644 --- a/crates/primitives/src/block.rs +++ b/crates/primitives/src/block.rs @@ -91,7 +91,7 @@ pub struct BlockWithSenders { #[deref_mut] pub block: B, /// List of senders that match the transactions in the block - pub senders: Vec
, + senders: Vec
, } impl BlockWithSenders { @@ -105,6 +105,16 @@ impl BlockWithSenders { (block.body().transactions().len() == senders.len()).then_some(Self { block, senders }) } + /// Returns all senders of the transactions in the block. + pub fn senders(&self) -> &[Address] { + &self.senders + } + + /// Returns an iterator over all senders in the block. + pub fn senders_iter(&self) -> impl Iterator { + self.senders.iter() + } + /// Seal the block with a known hash. /// /// WARNING: This method does not perform validation whether the hash is correct. @@ -122,7 +132,7 @@ impl BlockWithSenders { /// Split Structure to its components #[inline] - pub fn into_components(self) -> (B, Vec
) { + pub fn split(self) -> (B, Vec
) { (self.block, self.senders) } @@ -483,7 +493,7 @@ pub struct SealedBlockWithSenders { #[serde(bound = "SealedBlock: Serialize + serde::de::DeserializeOwned")] pub block: SealedBlock, /// List of senders that match transactions from block. - pub senders: Vec
, + senders: Vec
, } impl Default for SealedBlockWithSenders { @@ -493,6 +503,14 @@ impl Default for SealedBlockWithSenders { } impl SealedBlockWithSenders { + /// New sealed block with sender + pub const fn new_unchecked( + block: SealedBlock, + senders: Vec
, + ) -> Self { + Self { block, senders } + } + /// New sealed block with sender. Return none if len of tx and senders does not match pub fn new(block: SealedBlock, senders: Vec
) -> Option { (block.body.transactions().len() == senders.len()).then_some(Self { block, senders }) @@ -500,16 +518,26 @@ impl SealedBlockWithSenders { } impl SealedBlockWithSenders { + /// Returns all senders of the transactions in the block. + pub fn senders(&self) -> &[Address] { + &self.senders + } + + /// Returns an iterator over all senders in the block. + pub fn senders_iter(&self) -> impl Iterator { + self.senders.iter() + } + /// Split Structure to its components #[inline] - pub fn into_components(self) -> (SealedBlock, Vec
) { + pub fn split(self) -> (SealedBlock, Vec
) { (self.block, self.senders) } /// Returns the unsealed [`BlockWithSenders`] #[inline] pub fn unseal(self) -> BlockWithSenders { - let (block, senders) = self.into_components(); + let (block, senders) = self.split(); let (header, body) = block.split(); let header = header.unseal(); BlockWithSenders::new_unchecked(B::new(header, body), senders) @@ -555,6 +583,22 @@ impl SealedBlockWithSenders { } } +#[cfg(any(test, feature = "test-utils"))] +impl SealedBlockWithSenders +where + B: reth_primitives_traits::Block, +{ + /// Returns a mutable reference to the recovered senders. + pub fn senders_mut(&mut self) -> &mut Vec
{ + &mut self.senders + } + + /// Appends the sender to the list of senders. + pub fn push_sender(&mut self, sender: Address) { + self.senders.push(sender); + } +} + #[cfg(any(test, feature = "arbitrary"))] impl<'a, B> arbitrary::Arbitrary<'a> for SealedBlockWithSenders where diff --git a/crates/rpc/rpc-eth-api/src/helpers/pending_block.rs b/crates/rpc/rpc-eth-api/src/helpers/pending_block.rs index fd69422da68b..9b52b94a4db8 100644 --- a/crates/rpc/rpc-eth-api/src/helpers/pending_block.rs +++ b/crates/rpc/rpc-eth-api/src/helpers/pending_block.rs @@ -426,6 +426,6 @@ pub trait LoadPendingBlock: results, ); - Ok((SealedBlockWithSenders { block: block.seal_slow(), senders }, receipts)) + Ok((SealedBlockWithSenders::new_unchecked(block.seal_slow(), senders), receipts)) } } diff --git a/crates/rpc/rpc-types-compat/src/block.rs b/crates/rpc/rpc-types-compat/src/block.rs index 5eac699e12a5..ed97c7f5b40b 100644 --- a/crates/rpc/rpc-types-compat/src/block.rs +++ b/crates/rpc/rpc-types-compat/src/block.rs @@ -80,7 +80,7 @@ where // `from_block_with_transactions`, however we need to compute the length before let block_length = block.block.length(); let transactions = block.block.body().transactions().to_vec(); - let transactions_with_senders = transactions.into_iter().zip(block.senders); + let transactions_with_senders = transactions.into_iter().zip(block.senders_iter().copied()); let transactions = transactions_with_senders .enumerate() .map(|(idx, (tx, sender))| { diff --git a/crates/rpc/rpc/src/validation.rs b/crates/rpc/rpc/src/validation.rs index 1c40004f8bf3..d2faf0dd52e9 100644 --- a/crates/rpc/rpc/src/validation.rs +++ b/crates/rpc/rpc/src/validation.rs @@ -115,7 +115,7 @@ where if self.disallow.contains(&message.proposer_fee_recipient) { return Err(ValidationApiError::Blacklist(message.proposer_fee_recipient)) } - for (sender, tx) in block.senders.iter().zip(block.transactions()) { + for (sender, tx) in block.senders_iter().zip(block.transactions()) { if self.disallow.contains(sender) { return Err(ValidationApiError::Blacklist(*sender)) } diff --git a/crates/storage/provider/src/providers/blockchain_provider.rs b/crates/storage/provider/src/providers/blockchain_provider.rs index 1c19e8260b8d..046d000c7211 100644 --- a/crates/storage/provider/src/providers/blockchain_provider.rs +++ b/crates/storage/provider/src/providers/blockchain_provider.rs @@ -1211,10 +1211,10 @@ mod tests { assert_eq!( provider.pending_block_with_senders()?, - Some(reth_primitives::SealedBlockWithSenders { - block: block.clone(), - senders: block.senders().unwrap() - }) + Some(reth_primitives::SealedBlockWithSenders::new_unchecked( + block.clone(), + block.senders().unwrap() + )) ); assert_eq!(provider.pending_block_and_receipts()?, Some((block, vec![]))); diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs index 8f7dbbc2177b..34713d108ba2 100644 --- a/crates/storage/provider/src/providers/database/provider.rs +++ b/crates/storage/provider/src/providers/database/provider.rs @@ -2800,7 +2800,7 @@ impl BlockWrite // Ensures we have all the senders for the block's transactions. for (transaction, sender) in - block.block.body().transactions().iter().zip(block.senders.iter()) + block.block.body().transactions().iter().zip(block.senders_iter()) { let hash = transaction.tx_hash(); diff --git a/crates/storage/provider/src/test_utils/blocks.rs b/crates/storage/provider/src/test_utils/blocks.rs index 9924375ecb99..44773402450d 100644 --- a/crates/storage/provider/src/test_utils/blocks.rs +++ b/crates/storage/provider/src/test_utils/blocks.rs @@ -240,7 +240,10 @@ fn block1(number: BlockNumber) -> (SealedBlockWithSenders, ExecutionOutcome) { header.parent_hash = B256::ZERO; let block = SealedBlock::new(SealedHeader::seal(header), body); - (SealedBlockWithSenders { block, senders: vec![Address::new([0x30; 20])] }, execution_outcome) + ( + SealedBlockWithSenders::new_unchecked(block, vec![Address::new([0x30; 20])]), + execution_outcome, + ) } /// Block two that points to block 1 @@ -304,7 +307,10 @@ fn block2( header.parent_hash = parent_hash; let block = SealedBlock::new(SealedHeader::seal(header), body); - (SealedBlockWithSenders { block, senders: vec![Address::new([0x31; 20])] }, execution_outcome) + ( + SealedBlockWithSenders::new_unchecked(block, vec![Address::new([0x31; 20])]), + execution_outcome, + ) } /// Block three that points to block 2 @@ -368,7 +374,10 @@ fn block3( header.parent_hash = parent_hash; let block = SealedBlock::new(SealedHeader::seal(header), body); - (SealedBlockWithSenders { block, senders: vec![Address::new([0x31; 20])] }, execution_outcome) + ( + SealedBlockWithSenders::new_unchecked(block, vec![Address::new([0x31; 20])]), + execution_outcome, + ) } /// Block four that points to block 3 @@ -457,7 +466,10 @@ fn block4( header.parent_hash = parent_hash; let block = SealedBlock::new(SealedHeader::seal(header), body); - (SealedBlockWithSenders { block, senders: vec![Address::new([0x31; 20])] }, execution_outcome) + ( + SealedBlockWithSenders::new_unchecked(block, vec![Address::new([0x31; 20])]), + execution_outcome, + ) } /// Block five that points to block 4 @@ -543,5 +555,8 @@ fn block5( header.parent_hash = parent_hash; let block = SealedBlock::new(SealedHeader::seal(header), body); - (SealedBlockWithSenders { block, senders: vec![Address::new([0x31; 20])] }, execution_outcome) + ( + SealedBlockWithSenders::new_unchecked(block, vec![Address::new([0x31; 20])]), + execution_outcome, + ) } diff --git a/crates/transaction-pool/src/blobstore/tracker.rs b/crates/transaction-pool/src/blobstore/tracker.rs index 88c8faa7872a..de51b87c825f 100644 --- a/crates/transaction-pool/src/blobstore/tracker.rs +++ b/crates/transaction-pool/src/blobstore/tracker.rs @@ -127,8 +127,8 @@ mod tests { let tx3_hash = B256::random(); // Non-EIP-4844 transaction // Creating a first block with EIP-4844 transactions - let block1 = SealedBlockWithSenders { - block: SealedBlock::new( + let block1 = SealedBlockWithSenders::new_unchecked( + SealedBlock::new( SealedHeader::new(Header { number: 10, ..Default::default() }, B256::random()), BlockBody { transactions: vec![ @@ -152,13 +152,13 @@ mod tests { ..Default::default() }, ), - ..Default::default() - }; + Default::default(), + ); // Creating a second block with EIP-1559 and EIP-2930 transactions // Note: This block does not contain any EIP-4844 transactions - let block2 = SealedBlockWithSenders { - block: SealedBlock::new( + let block2 = SealedBlockWithSenders::new_unchecked( + SealedBlock::new( SealedHeader::new(Header { number: 11, ..Default::default() }, B256::random()), BlockBody { transactions: vec![ @@ -176,8 +176,8 @@ mod tests { ..Default::default() }, ), - ..Default::default() - }; + Default::default(), + ); // Extract blocks from the chain let chain: Chain = Chain::new(vec![block1, block2], Default::default(), None);