From 71e84e5cfa90b22d2f967fb5a7b9891d340e9eda Mon Sep 17 00:00:00 2001 From: Thomas Coratger Date: Fri, 2 Feb 2024 19:58:12 +0100 Subject: [PATCH 1/5] small refactoring in payload crate --- crates/payload/builder/src/database.rs | 31 +++++--------- crates/payload/builder/src/noop.rs | 6 ++- crates/payload/builder/src/optimism.rs | 59 ++++++++++++-------------- crates/payload/builder/src/payload.rs | 27 ++++++------ crates/payload/builder/src/service.rs | 15 +++---- 5 files changed, 59 insertions(+), 79 deletions(-) diff --git a/crates/payload/builder/src/database.rs b/crates/payload/builder/src/database.rs index 0205bad335d3..32f84c58a9b6 100644 --- a/crates/payload/builder/src/database.rs +++ b/crates/payload/builder/src/database.rs @@ -7,7 +7,6 @@ use reth_primitives::{ }, U256, }; - use std::{ cell::RefCell, collections::{hash_map::Entry, HashMap}, @@ -72,36 +71,27 @@ impl<'a, DB: DatabaseRef> Database for CachedReadsDbMut<'a, DB> { type Error = ::Error; fn basic(&mut self, address: Address) -> Result, Self::Error> { - let basic = match self.cached.accounts.entry(address) { + Ok(match self.cached.accounts.entry(address) { Entry::Occupied(entry) => entry.get().info.clone(), Entry::Vacant(entry) => { entry.insert(CachedAccount::new(self.db.basic_ref(address)?)).info.clone() } - }; - Ok(basic) + }) } fn code_by_hash(&mut self, code_hash: B256) -> Result { - let code = match self.cached.contracts.entry(code_hash) { + Ok(match self.cached.contracts.entry(code_hash) { Entry::Occupied(entry) => entry.get().clone(), Entry::Vacant(entry) => entry.insert(self.db.code_by_hash_ref(code_hash)?).clone(), - }; - Ok(code) + }) } fn storage(&mut self, address: Address, index: U256) -> Result { match self.cached.accounts.entry(address) { - Entry::Occupied(mut acc_entry) => { - let acc_entry = acc_entry.get_mut(); - match acc_entry.storage.entry(index) { - Entry::Occupied(entry) => Ok(*entry.get()), - Entry::Vacant(entry) => { - let slot = self.db.storage_ref(address, index)?; - entry.insert(slot); - Ok(slot) - } - } - } + Entry::Occupied(mut acc_entry) => match acc_entry.get_mut().storage.entry(index) { + Entry::Occupied(entry) => Ok(*entry.get()), + Entry::Vacant(entry) => Ok(*entry.insert(self.db.storage_ref(address, index)?)), + }, Entry::Vacant(acc_entry) => { // acc needs to be loaded for us to access slots. let info = self.db.basic_ref(address)?; @@ -120,11 +110,10 @@ impl<'a, DB: DatabaseRef> Database for CachedReadsDbMut<'a, DB> { } fn block_hash(&mut self, number: U256) -> Result { - let code = match self.cached.block_hashes.entry(number) { + Ok(match self.cached.block_hashes.entry(number) { Entry::Occupied(entry) => *entry.get(), Entry::Vacant(entry) => *entry.insert(self.db.block_hash_ref(number)?), - }; - Ok(code) + }) } } diff --git a/crates/payload/builder/src/noop.rs b/crates/payload/builder/src/noop.rs index f1479d1a37c2..db5d46c25e31 100644 --- a/crates/payload/builder/src/noop.rs +++ b/crates/payload/builder/src/noop.rs @@ -25,8 +25,10 @@ where /// Creates a new [NoopPayloadBuilderService]. pub fn new() -> (Self, PayloadBuilderHandle) { let (service_tx, command_rx) = mpsc::unbounded_channel(); - let handle = PayloadBuilderHandle::new(service_tx); - (Self { command_rx: UnboundedReceiverStream::new(command_rx) }, handle) + ( + Self { command_rx: UnboundedReceiverStream::new(command_rx) }, + PayloadBuilderHandle::new(service_tx), + ) } } diff --git a/crates/payload/builder/src/optimism.rs b/crates/payload/builder/src/optimism.rs index 6b8b272ace8e..b104d5d7c32c 100644 --- a/crates/payload/builder/src/optimism.rs +++ b/crates/payload/builder/src/optimism.rs @@ -26,40 +26,35 @@ impl PayloadBuilderAttributes for OptimismPayloadBuilderAttributes { /// /// Derives the unique [PayloadId] for the given parent and attributes fn try_new(parent: B256, attributes: OptimismPayloadAttributes) -> Result { - let (id, transactions) = { - let transactions: Vec<_> = attributes - .transactions - .as_deref() - .unwrap_or(&[]) - .iter() - .map(|tx| TransactionSigned::decode_enveloped(&mut tx.as_ref())) - .collect::>()?; - (payload_id_optimism(&parent, &attributes, &transactions), transactions) - }; - - let withdraw = attributes.payload_attributes.withdrawals.map( - |withdrawals: Vec| { - Withdrawals::new( - withdrawals - .into_iter() - .map(convert_standalone_withdraw_to_withdrawal) // Removed the parentheses here - .collect(), - ) - }, - ); - - let payload_attributes = EthPayloadBuilderAttributes { - id, - parent, - timestamp: attributes.payload_attributes.timestamp, - suggested_fee_recipient: attributes.payload_attributes.suggested_fee_recipient, - prev_randao: attributes.payload_attributes.prev_randao, - withdrawals: withdraw.unwrap_or_default(), - parent_beacon_block_root: attributes.payload_attributes.parent_beacon_block_root, - }; + let transactions: Vec<_> = attributes + .transactions + .as_deref() + .unwrap_or_default() + .iter() + .map(|tx| TransactionSigned::decode_enveloped(&mut tx.as_ref())) + .collect::>()?; Ok(Self { - payload_attributes, + payload_attributes: EthPayloadBuilderAttributes { + id: payload_id_optimism(&parent, &attributes, &transactions), + parent, + timestamp: attributes.payload_attributes.timestamp, + suggested_fee_recipient: attributes.payload_attributes.suggested_fee_recipient, + prev_randao: attributes.payload_attributes.prev_randao, + withdrawals: attributes + .payload_attributes + .withdrawals + .map(|withdrawals| { + Withdrawals::new( + withdrawals + .into_iter() + .map(convert_standalone_withdraw_to_withdrawal) // Removed the parentheses here + .collect(), + ) + }) + .unwrap_or_default(), + parent_beacon_block_root: attributes.payload_attributes.parent_beacon_block_root, + }, no_tx_pool: attributes.no_tx_pool.unwrap_or_default(), transactions, gas_limit: attributes.gas_limit, diff --git a/crates/payload/builder/src/payload.rs b/crates/payload/builder/src/payload.rs index 5aeabf684e82..9623754c460c 100644 --- a/crates/payload/builder/src/payload.rs +++ b/crates/payload/builder/src/payload.rs @@ -170,26 +170,23 @@ impl EthPayloadBuilderAttributes { /// /// Derives the unique [PayloadId] for the given parent and attributes pub fn new(parent: B256, attributes: PayloadAttributes) -> Self { - let id = payload_id(&parent, &attributes); - - let withdraw = attributes.withdrawals.map( - |withdrawals: Vec| { - Withdrawals::new( - withdrawals - .into_iter() - .map(convert_standalone_withdraw_to_withdrawal) // Removed the parentheses here - .collect(), - ) - }, - ); - Self { - id, + id: payload_id(&parent, &attributes), parent, timestamp: attributes.timestamp, suggested_fee_recipient: attributes.suggested_fee_recipient, prev_randao: attributes.prev_randao, - withdrawals: withdraw.unwrap_or_default(), + withdrawals: attributes + .withdrawals + .map(|withdrawals| { + Withdrawals::new( + withdrawals + .into_iter() + .map(convert_standalone_withdraw_to_withdrawal) + .collect(), + ) + }) + .unwrap_or_default(), parent_beacon_block_root: attributes.parent_beacon_block_root, } } diff --git a/crates/payload/builder/src/service.rs b/crates/payload/builder/src/service.rs index 4d3ae7298246..1a84d78ef3a3 100644 --- a/crates/payload/builder/src/service.rs +++ b/crates/payload/builder/src/service.rs @@ -288,17 +288,14 @@ where &self, id: PayloadId, ) -> Option::PayloadAttributes, PayloadBuilderError>> { - let attributes = self - .payload_jobs + self.payload_jobs .iter() .find(|(_, job_id)| *job_id == id) - .map(|(j, _)| j.payload_attributes()); - - if attributes.is_none() { - trace!(%id, "no matching payload job found to get attributes for"); - } - - attributes + .map(|(j, _)| j.payload_attributes()) + .or_else(|| { + trace!(%id, "no matching payload job found to get attributes for"); + None + }) } } From 1507c3d04656e04135b165d99890c31441f8c1c7 Mon Sep 17 00:00:00 2001 From: Thomas Coratger Date: Fri, 2 Feb 2024 21:00:05 +0100 Subject: [PATCH 2/5] revert --- crates/payload/builder/src/optimism.rs | 59 ++++++++++++++------------ 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/crates/payload/builder/src/optimism.rs b/crates/payload/builder/src/optimism.rs index b104d5d7c32c..30ea3e63ebf9 100644 --- a/crates/payload/builder/src/optimism.rs +++ b/crates/payload/builder/src/optimism.rs @@ -26,35 +26,40 @@ impl PayloadBuilderAttributes for OptimismPayloadBuilderAttributes { /// /// Derives the unique [PayloadId] for the given parent and attributes fn try_new(parent: B256, attributes: OptimismPayloadAttributes) -> Result { - let transactions: Vec<_> = attributes - .transactions - .as_deref() - .unwrap_or_default() - .iter() - .map(|tx| TransactionSigned::decode_enveloped(&mut tx.as_ref())) - .collect::>()?; + let (id, transactions) = { + let transactions: Vec<_> = attributes + .transactions + .as_deref() + .unwrap_or(&[]) + .iter() + .map(|tx| TransactionSigned::decode_enveloped(&mut tx.as_ref())) + .collect::>()?; + (payload_id_optimism(&parent, &attributes, &transactions), transactions) + }; + + let payload_attributes = EthPayloadBuilderAttributes { + id, + parent, + timestamp: attributes.payload_attributes.timestamp, + suggested_fee_recipient: attributes.payload_attributes.suggested_fee_recipient, + prev_randao: attributes.payload_attributes.prev_randao, + withdrawals: attributes + .payload_attributes + .withdrawals + .map(|withdrawals| { + Withdrawals::new( + withdrawals + .into_iter() + .map(convert_standalone_withdraw_to_withdrawal) + .collect(), + ) + }) + .unwrap_or_default(), + parent_beacon_block_root: attributes.payload_attributes.parent_beacon_block_root, + }; Ok(Self { - payload_attributes: EthPayloadBuilderAttributes { - id: payload_id_optimism(&parent, &attributes, &transactions), - parent, - timestamp: attributes.payload_attributes.timestamp, - suggested_fee_recipient: attributes.payload_attributes.suggested_fee_recipient, - prev_randao: attributes.payload_attributes.prev_randao, - withdrawals: attributes - .payload_attributes - .withdrawals - .map(|withdrawals| { - Withdrawals::new( - withdrawals - .into_iter() - .map(convert_standalone_withdraw_to_withdrawal) // Removed the parentheses here - .collect(), - ) - }) - .unwrap_or_default(), - parent_beacon_block_root: attributes.payload_attributes.parent_beacon_block_root, - }, + payload_attributes, no_tx_pool: attributes.no_tx_pool.unwrap_or_default(), transactions, gas_limit: attributes.gas_limit, From 96eeefd5e656fd0dd7f9d59f623379a72345e740 Mon Sep 17 00:00:00 2001 From: Thomas Coratger Date: Fri, 2 Feb 2024 21:02:11 +0100 Subject: [PATCH 3/5] revert --- crates/payload/builder/src/database.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/payload/builder/src/database.rs b/crates/payload/builder/src/database.rs index 32f84c58a9b6..5b5239fddcb1 100644 --- a/crates/payload/builder/src/database.rs +++ b/crates/payload/builder/src/database.rs @@ -71,19 +71,21 @@ impl<'a, DB: DatabaseRef> Database for CachedReadsDbMut<'a, DB> { type Error = ::Error; fn basic(&mut self, address: Address) -> Result, Self::Error> { - Ok(match self.cached.accounts.entry(address) { + let basic = match self.cached.accounts.entry(address) { Entry::Occupied(entry) => entry.get().info.clone(), Entry::Vacant(entry) => { entry.insert(CachedAccount::new(self.db.basic_ref(address)?)).info.clone() } - }) + }; + Ok(basic) } fn code_by_hash(&mut self, code_hash: B256) -> Result { - Ok(match self.cached.contracts.entry(code_hash) { + let code = match self.cached.contracts.entry(code_hash) { Entry::Occupied(entry) => entry.get().clone(), Entry::Vacant(entry) => entry.insert(self.db.code_by_hash_ref(code_hash)?).clone(), - }) + }; + Ok(code) } fn storage(&mut self, address: Address, index: U256) -> Result { @@ -110,10 +112,11 @@ impl<'a, DB: DatabaseRef> Database for CachedReadsDbMut<'a, DB> { } fn block_hash(&mut self, number: U256) -> Result { - Ok(match self.cached.block_hashes.entry(number) { + let code = match self.cached.block_hashes.entry(number) { Entry::Occupied(entry) => *entry.get(), Entry::Vacant(entry) => *entry.insert(self.db.block_hash_ref(number)?), - }) + }; + Ok(code) } } From 175c9f6741de34b6f6bcdc349291c271ed82704d Mon Sep 17 00:00:00 2001 From: Thomas Coratger Date: Fri, 2 Feb 2024 21:05:10 +0100 Subject: [PATCH 4/5] revert --- crates/payload/builder/src/optimism.rs | 19 +++++++------------ crates/payload/builder/src/service.rs | 15 +++++++++------ 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/crates/payload/builder/src/optimism.rs b/crates/payload/builder/src/optimism.rs index 30ea3e63ebf9..b03928fa0665 100644 --- a/crates/payload/builder/src/optimism.rs +++ b/crates/payload/builder/src/optimism.rs @@ -37,24 +37,19 @@ impl PayloadBuilderAttributes for OptimismPayloadBuilderAttributes { (payload_id_optimism(&parent, &attributes, &transactions), transactions) }; + let withdraw = attributes.payload_attributes.withdrawals.map(|withdrawals| { + Withdrawals::new( + withdrawals.into_iter().map(convert_standalone_withdraw_to_withdrawal).collect(), + ) + }); + let payload_attributes = EthPayloadBuilderAttributes { id, parent, timestamp: attributes.payload_attributes.timestamp, suggested_fee_recipient: attributes.payload_attributes.suggested_fee_recipient, prev_randao: attributes.payload_attributes.prev_randao, - withdrawals: attributes - .payload_attributes - .withdrawals - .map(|withdrawals| { - Withdrawals::new( - withdrawals - .into_iter() - .map(convert_standalone_withdraw_to_withdrawal) - .collect(), - ) - }) - .unwrap_or_default(), + withdrawals: withdraw.unwrap_or_default(), parent_beacon_block_root: attributes.payload_attributes.parent_beacon_block_root, }; diff --git a/crates/payload/builder/src/service.rs b/crates/payload/builder/src/service.rs index 1a84d78ef3a3..4d3ae7298246 100644 --- a/crates/payload/builder/src/service.rs +++ b/crates/payload/builder/src/service.rs @@ -288,14 +288,17 @@ where &self, id: PayloadId, ) -> Option::PayloadAttributes, PayloadBuilderError>> { - self.payload_jobs + let attributes = self + .payload_jobs .iter() .find(|(_, job_id)| *job_id == id) - .map(|(j, _)| j.payload_attributes()) - .or_else(|| { - trace!(%id, "no matching payload job found to get attributes for"); - None - }) + .map(|(j, _)| j.payload_attributes()); + + if attributes.is_none() { + trace!(%id, "no matching payload job found to get attributes for"); + } + + attributes } } From 974f9c49c1e84932574c6185106b29c39c36c91a Mon Sep 17 00:00:00 2001 From: Thomas Coratger Date: Fri, 2 Feb 2024 21:06:35 +0100 Subject: [PATCH 5/5] revert --- crates/payload/builder/src/payload.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/payload/builder/src/payload.rs b/crates/payload/builder/src/payload.rs index 9623754c460c..00b69ffb4eb8 100644 --- a/crates/payload/builder/src/payload.rs +++ b/crates/payload/builder/src/payload.rs @@ -170,23 +170,24 @@ impl EthPayloadBuilderAttributes { /// /// Derives the unique [PayloadId] for the given parent and attributes pub fn new(parent: B256, attributes: PayloadAttributes) -> Self { + let id = payload_id(&parent, &attributes); + + let withdraw = attributes.withdrawals.map(|withdrawals| { + Withdrawals::new( + withdrawals + .into_iter() + .map(convert_standalone_withdraw_to_withdrawal) // Removed the parentheses here + .collect(), + ) + }); + Self { - id: payload_id(&parent, &attributes), + id, parent, timestamp: attributes.timestamp, suggested_fee_recipient: attributes.suggested_fee_recipient, prev_randao: attributes.prev_randao, - withdrawals: attributes - .withdrawals - .map(|withdrawals| { - Withdrawals::new( - withdrawals - .into_iter() - .map(convert_standalone_withdraw_to_withdrawal) - .collect(), - ) - }) - .unwrap_or_default(), + withdrawals: withdraw.unwrap_or_default(), parent_beacon_block_root: attributes.parent_beacon_block_root, } }