perf(engine): only recover senders once (#20118)

Author: klkvr

crates/chain-state/src/test_utils.rs

@@ -92,7 +92,7 @@ impl<N: NodePrimitives> TestBlockBuilder<N> {
         &mut self,
         number: BlockNumber,
         parent_hash: B256,
-    ) -> RecoveredBlock<reth_ethereum_primitives::Block> {
+    ) -> SealedBlock<reth_ethereum_primitives::Block> {
         let mut rng = rand::rng();
 
         let mock_tx = |nonce: u64| -> Recovered<_> {
@@ -167,17 +167,14 @@ impl<N: NodePrimitives> TestBlockBuilder<N> {
             ..Default::default()
         };
 
-        let block = SealedBlock::from_sealed_parts(
+        SealedBlock::from_sealed_parts(
             SealedHeader::seal_slow(header),
             BlockBody {
                 transactions: transactions.into_iter().map(|tx| tx.into_inner()).collect(),
                 ommers: Vec::new(),
                 withdrawals: Some(vec![].into()),
             },
-        );
-
-        RecoveredBlock::try_recover_sealed_with_senders(block, vec![self.signer; num_txs as usize])
-            .unwrap()
+        )
     }
 
     /// Creates a fork chain with the given base block.
@@ -191,7 +188,9 @@ impl<N: NodePrimitives> TestBlockBuilder<N> {
 
         for _ in 0..length {
             let block = self.generate_random_block(parent.number + 1, parent.hash());
-            parent = block.clone_sealed_block();
+            parent = block.clone();
+            let senders = vec![self.signer; block.body().transactions.len()];
+            let block = block.with_senders(senders);
             fork.push(block);
         }
 
@@ -205,9 +204,8 @@ impl<N: NodePrimitives> TestBlockBuilder<N> {
         receipts: Vec<Vec<Receipt>>,
         parent_hash: B256,
     ) -> ExecutedBlock {
-        let block_with_senders = self.generate_random_block(block_number, parent_hash);
-
-        let (block, senders) = block_with_senders.split_sealed();
+        let block = self.generate_random_block(block_number, parent_hash);
+        let senders = vec![self.signer; block.body().transactions.len()];
         let trie_data = ComputedTrieData::default();
         ExecutedBlock::new(
             Arc::new(RecoveredBlock::new_sealed(block, senders)),

crates/engine/tree/src/download.rs

@@ -9,7 +9,7 @@ use reth_network_p2p::{
     full_block::{FetchFullBlockFuture, FetchFullBlockRangeFuture, FullBlockClient},
     BlockClient,
 };
-use reth_primitives_traits::{Block, RecoveredBlock, SealedBlock};
+use reth_primitives_traits::{Block, SealedBlock};
 use std::{
     cmp::{Ordering, Reverse},
     collections::{binary_heap::PeekMut, BinaryHeap, HashSet, VecDeque},
@@ -44,7 +44,7 @@ pub enum DownloadAction {
 #[derive(Debug)]
 pub enum DownloadOutcome<B: Block> {
     /// Downloaded blocks.
-    Blocks(Vec<RecoveredBlock<B>>),
+    Blocks(Vec<SealedBlock<B>>),
     /// New download started.
     NewDownloadStarted {
         /// How many blocks are pending in this download.
@@ -68,7 +68,7 @@ where
     inflight_block_range_requests: Vec<FetchFullBlockRangeFuture<Client>>,
     /// Buffered blocks from downloads - this is a min-heap of blocks, using the block number for
     /// ordering. This means the blocks will be popped from the heap with ascending block numbers.
-    set_buffered_blocks: BinaryHeap<Reverse<OrderedRecoveredBlock<B>>>,
+    set_buffered_blocks: BinaryHeap<Reverse<OrderedSealedBlock<B>>>,
     /// Engine download metrics.
     metrics: BlockDownloaderMetrics,
     /// Pending events to be emitted.
@@ -226,15 +226,8 @@ where
             let mut request = self.inflight_block_range_requests.swap_remove(idx);
             if let Poll::Ready(blocks) = request.poll_unpin(cx) {
                 trace!(target: "engine::download", len=?blocks.len(), first=?blocks.first().map(|b| b.num_hash()), last=?blocks.last().map(|b| b.num_hash()), "Received full block range, buffering");
-                self.set_buffered_blocks.extend(
-                    blocks
-                        .into_iter()
-                        .map(|b| {
-                            let senders = b.senders().unwrap_or_default();
-                            OrderedRecoveredBlock(RecoveredBlock::new_sealed(b, senders))
-                        })
-                        .map(Reverse),
-                );
+                self.set_buffered_blocks
+                    .extend(blocks.into_iter().map(OrderedSealedBlock).map(Reverse));
             } else {
                 // still pending
                 self.inflight_block_range_requests.push(request);
@@ -248,8 +241,7 @@ where
         }
 
         // drain all unique element of the block buffer if there are any
-        let mut downloaded_blocks: Vec<RecoveredBlock<B>> =
-            Vec::with_capacity(self.set_buffered_blocks.len());
+        let mut downloaded_blocks = Vec::with_capacity(self.set_buffered_blocks.len());
         while let Some(block) = self.set_buffered_blocks.pop() {
             // peek ahead and pop duplicates
             while let Some(peek) = self.set_buffered_blocks.peek_mut() {
@@ -265,32 +257,31 @@ where
     }
 }
 
-/// A wrapper type around [`RecoveredBlock`] that implements the [Ord]
+/// A wrapper type around [`SealedBlock`] that implements the [Ord]
 /// trait by block number.
 #[derive(Debug, Clone, PartialEq, Eq)]
-struct OrderedRecoveredBlock<B: Block>(RecoveredBlock<B>);
+struct OrderedSealedBlock<B: Block>(SealedBlock<B>);
 
-impl<B: Block> PartialOrd for OrderedRecoveredBlock<B> {
+impl<B: Block> PartialOrd for OrderedSealedBlock<B> {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
         Some(self.cmp(other))
     }
 }
 
-impl<B: Block> Ord for OrderedRecoveredBlock<B> {
+impl<B: Block> Ord for OrderedSealedBlock<B> {
     fn cmp(&self, other: &Self) -> Ordering {
         self.0.number().cmp(&other.0.number())
     }
 }
 
-impl<B: Block> From<SealedBlock<B>> for OrderedRecoveredBlock<B> {
+impl<B: Block> From<SealedBlock<B>> for OrderedSealedBlock<B> {
     fn from(block: SealedBlock<B>) -> Self {
-        let senders = block.senders().unwrap_or_default();
-        Self(RecoveredBlock::new_sealed(block, senders))
+        Self(block)
     }
 }
 
-impl<B: Block> From<OrderedRecoveredBlock<B>> for RecoveredBlock<B> {
-    fn from(value: OrderedRecoveredBlock<B>) -> Self {
+impl<B: Block> From<OrderedSealedBlock<B>> for SealedBlock<B> {
+    fn from(value: OrderedSealedBlock<B>) -> Self {
         value.0
     }
 }

crates/engine/tree/src/engine.rs

@@ -11,7 +11,7 @@ use reth_chain_state::ExecutedBlock;
 use reth_engine_primitives::{BeaconEngineMessage, ConsensusEngineEvent};
 use reth_ethereum_primitives::EthPrimitives;
 use reth_payload_primitives::PayloadTypes;
-use reth_primitives_traits::{Block, NodePrimitives, RecoveredBlock};
+use reth_primitives_traits::{Block, NodePrimitives, SealedBlock};
 use std::{
     collections::HashSet,
     fmt::Display,
@@ -307,7 +307,7 @@ pub enum FromEngine<Req, B: Block> {
     /// Request from the engine.
     Request(Req),
     /// Downloaded blocks from the network.
-    DownloadedBlocks(Vec<RecoveredBlock<B>>),
+    DownloadedBlocks(Vec<SealedBlock<B>>),
 }
 
 impl<Req: Display, B: Block> Display for FromEngine<Req, B> {

crates/engine/tree/src/tree/block_buffer.rs

@@ -1,7 +1,7 @@
 use crate::tree::metrics::BlockBufferMetrics;
 use alloy_consensus::BlockHeader;
 use alloy_primitives::{BlockHash, BlockNumber};
-use reth_primitives_traits::{Block, RecoveredBlock};
+use reth_primitives_traits::{Block, SealedBlock};
 use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};
 
 /// Contains the tree of pending blocks that cannot be executed due to missing parent.
@@ -18,7 +18,7 @@ use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};
 #[derive(Debug)]
 pub struct BlockBuffer<B: Block> {
     /// All blocks in the buffer stored by their block hash.
-    pub(crate) blocks: HashMap<BlockHash, RecoveredBlock<B>>,
+    pub(crate) blocks: HashMap<BlockHash, SealedBlock<B>>,
     /// Map of any parent block hash (even the ones not currently in the buffer)
     /// to the buffered children.
     /// Allows connecting buffered blocks by parent.
@@ -49,12 +49,12 @@ impl<B: Block> BlockBuffer<B> {
     }
 
     /// Return reference to the requested block.
-    pub fn block(&self, hash: &BlockHash) -> Option<&RecoveredBlock<B>> {
+    pub fn block(&self, hash: &BlockHash) -> Option<&SealedBlock<B>> {
         self.blocks.get(hash)
     }
 
     /// Return a reference to the lowest ancestor of the given block in the buffer.
-    pub fn lowest_ancestor(&self, hash: &BlockHash) -> Option<&RecoveredBlock<B>> {
+    pub fn lowest_ancestor(&self, hash: &BlockHash) -> Option<&SealedBlock<B>> {
         let mut current_block = self.blocks.get(hash)?;
         while let Some(parent) = self.blocks.get(&current_block.parent_hash()) {
             current_block = parent;
@@ -63,7 +63,7 @@ impl<B: Block> BlockBuffer<B> {
     }
 
     /// Insert a correct block inside the buffer.
-    pub fn insert_block(&mut self, block: RecoveredBlock<B>) {
+    pub fn insert_block(&mut self, block: SealedBlock<B>) {
         let hash = block.hash();
 
         self.parent_to_child.entry(block.parent_hash()).or_default().insert(hash);
@@ -87,10 +87,7 @@ impl<B: Block> BlockBuffer<B> {
     ///
     /// Note: that order of returned blocks is important and the blocks with lower block number
     /// in the chain will come first so that they can be executed in the correct order.
-    pub fn remove_block_with_children(
-        &mut self,
-        parent_hash: &BlockHash,
-    ) -> Vec<RecoveredBlock<B>> {
+    pub fn remove_block_with_children(&mut self, parent_hash: &BlockHash) -> Vec<SealedBlock<B>> {
         let removed = self
             .remove_block(parent_hash)
             .into_iter()
@@ -149,7 +146,7 @@ impl<B: Block> BlockBuffer<B> {
     /// This method will only remove the block if it's present inside `self.blocks`.
     /// The block might be missing from other collections, the method will only ensure that it has
     /// been removed.
-    fn remove_block(&mut self, hash: &BlockHash) -> Option<RecoveredBlock<B>> {
+    fn remove_block(&mut self, hash: &BlockHash) -> Option<SealedBlock<B>> {
         let block = self.blocks.remove(hash)?;
         self.remove_from_earliest_blocks(block.number(), hash);
         self.remove_from_parent(block.parent_hash(), hash);
@@ -158,7 +155,7 @@ impl<B: Block> BlockBuffer<B> {
     }
 
     /// Remove all children and their descendants for the given blocks and return them.
-    fn remove_children(&mut self, parent_hashes: Vec<BlockHash>) -> Vec<RecoveredBlock<B>> {
+    fn remove_children(&mut self, parent_hashes: Vec<BlockHash>) -> Vec<SealedBlock<B>> {
         // remove all parent child connection and all the child children blocks that are connected
         // to the discarded parent blocks.
         let mut remove_parent_children = parent_hashes;
@@ -184,7 +181,6 @@ mod tests {
     use super::*;
     use alloy_eips::BlockNumHash;
     use alloy_primitives::BlockHash;
-    use reth_primitives_traits::RecoveredBlock;
     use reth_testing_utils::generators::{self, random_block, BlockParams, Rng};
     use std::collections::HashMap;
 
@@ -193,10 +189,8 @@ mod tests {
         rng: &mut R,
         number: u64,
         parent: BlockHash,
-    ) -> RecoveredBlock<reth_ethereum_primitives::Block> {
-        let block =
-            random_block(rng, number, BlockParams { parent: Some(parent), ..Default::default() });
-        block.try_recover().unwrap()
+    ) -> SealedBlock<reth_ethereum_primitives::Block> {
+        random_block(rng, number, BlockParams { parent: Some(parent), ..Default::default() })
     }
 
     /// Assert that all buffer collections have the same data length.
@@ -216,7 +210,7 @@ mod tests {
     /// Assert that the block was removed from all buffer collections.
     fn assert_block_removal<B: Block>(
         buffer: &BlockBuffer<B>,
-        block: &RecoveredBlock<reth_ethereum_primitives::Block>,
+        block: &SealedBlock<reth_ethereum_primitives::Block>,
     ) {
         assert!(!buffer.blocks.contains_key(&block.hash()));
         assert!(buffer

crates/engine/tree/src/tree/metrics.rs

@@ -17,6 +17,7 @@ use reth_metrics::{
 use reth_primitives_traits::SignedTransaction;
 use reth_trie::updates::TrieUpdates;
 use revm::database::{states::bundle_state::BundleRetention, State};
+use revm_primitives::Address;
 use std::time::Instant;
 use tracing::{debug_span, trace};
 
@@ -64,7 +65,7 @@ impl EngineApiMetrics {
         executor: E,
         transactions: impl Iterator<Item = Result<impl ExecutableTx<E>, BlockExecutionError>>,
         state_hook: Box<dyn OnStateHook>,
-    ) -> Result<BlockExecutionOutput<E::Receipt>, BlockExecutionError>
+    ) -> Result<(BlockExecutionOutput<E::Receipt>, Vec<Address>), BlockExecutionError>
     where
         DB: alloy_evm::Database,
         E: BlockExecutor<Evm: Evm<DB: BorrowMut<State<DB>>>, Transaction: SignedTransaction>,
@@ -74,6 +75,7 @@ impl EngineApiMetrics {
         // be accessible.
         let wrapper = MeteredStateHook { metrics: self.executor.clone(), inner_hook: state_hook };
 
+        let mut senders = Vec::new();
         let mut executor = executor.with_state_hook(Some(Box::new(wrapper)));
 
         let f = || {
@@ -84,6 +86,7 @@ impl EngineApiMetrics {
                     debug_span!(target: "engine::tree", "execute tx", tx_hash=?tx.tx().tx_hash());
                 let enter = span.entered();
                 trace!(target: "engine::tree", "Executing transaction");
+                senders.push(*tx.signer());
                 let gas_used = executor.execute_transaction(tx)?;
 
                 // record the tx gas used
@@ -113,7 +116,7 @@ impl EngineApiMetrics {
         self.executor.storage_slots_updated_histogram.record(storage_slots as f64);
         self.executor.bytecodes_updated_histogram.record(bytecodes as f64);
 
-        Ok(output)
+        Ok((output, senders))
     }
 }