From fb30619192781a33b0c6e0f89e2f7fd12e1b3b14 Mon Sep 17 00:00:00 2001 From: 0xZerohero <133967403+0xZerohero@users.noreply.github.com> Date: Tue, 12 Mar 2024 18:45:49 +0200 Subject: [PATCH] fix(op-reth): non-deposit txs have depositReceiptVersion=1 (#6784) --- Cargo.lock | 1 - .../primitives/src/transaction/signature.rs | 2 +- crates/revm/Cargo.toml | 2 +- crates/revm/src/lib.rs | 4 + crates/revm/src/optimism/processor.rs | 204 +++++++++++++++++- crates/revm/src/processor.rs | 169 ++------------- crates/revm/src/test_utils.rs | 173 +++++++++++++++ 7 files changed, 402 insertions(+), 153 deletions(-) create mode 100644 crates/revm/src/test_utils.rs diff --git a/Cargo.lock b/Cargo.lock index c265450f1..b7eadb21e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6449,7 +6449,6 @@ dependencies = [ "reth-consensus-common", "reth-interfaces", "reth-node-api", - "reth-node-ethereum", "reth-primitives", "reth-provider", "reth-trie", diff --git a/crates/primitives/src/transaction/signature.rs b/crates/primitives/src/transaction/signature.rs index 4ab620a29..764321dd8 100644 --- a/crates/primitives/src/transaction/signature.rs +++ b/crates/primitives/src/transaction/signature.rs @@ -32,7 +32,7 @@ impl Signature { /// Returns the signature for the optimism deposit transactions, which don't include a /// signature. #[cfg(feature = "optimism")] - pub(crate) const fn optimism_deposit_tx_signature() -> Self { + pub const fn optimism_deposit_tx_signature() -> Self { Signature { r: U256::ZERO, s: U256::ZERO, odd_y_parity: false } } } diff --git a/crates/revm/Cargo.toml b/crates/revm/Cargo.toml index 04039a02d..74818a20a 100644 --- a/crates/revm/Cargo.toml +++ b/crates/revm/Cargo.toml @@ -28,12 +28,12 @@ tracing.workspace = true [dev-dependencies] reth-trie.workspace = true -reth-node-ethereum.workspace = true [features] optimism = [ "revm/optimism", "reth-primitives/optimism", + "reth-provider/optimism", "reth-consensus-common/optimism", "reth-interfaces/optimism", ] diff --git a/crates/revm/src/lib.rs b/crates/revm/src/lib.rs index d0e180fb7..a4ad94028 100644 --- a/crates/revm/src/lib.rs +++ b/crates/revm/src/lib.rs @@ -35,6 +35,10 @@ pub mod stack; #[cfg(feature = "optimism")] pub mod optimism; +/// Common test helpers +#[cfg(test)] +pub mod test_utils; + // Convenience re-exports. pub use revm::{self, *}; pub use revm_inspectors::*; diff --git a/crates/revm/src/optimism/processor.rs b/crates/revm/src/optimism/processor.rs index 4f43801a8..9a1a56726 100644 --- a/crates/revm/src/optimism/processor.rs +++ b/crates/revm/src/optimism/processor.rs @@ -179,10 +179,10 @@ where // receipt hashes should be computed when set. The state transition process ensures // this is only set for post-Canyon deposit transactions. #[cfg(feature = "optimism")] - deposit_receipt_version: self - .chain_spec() - .is_fork_active_at_timestamp(Hardfork::Canyon, block.timestamp) - .then_some(1), + deposit_receipt_version: (transaction.is_deposit() && + self.chain_spec() + .is_fork_active_at_timestamp(Hardfork::Canyon, block.timestamp)) + .then_some(1), }); } @@ -202,3 +202,199 @@ where Some(self.evm.context.evm.db.bundle_size_hint()) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + database::StateProviderDatabase, + test_utils::{StateProviderTest, TestEvmConfig}, + }; + use reth_primitives::{ + Account, Address, Block, ChainSpecBuilder, Header, Signature, StorageKey, StorageValue, + Transaction, TransactionKind, TransactionSigned, TxEip1559, BASE_MAINNET, + }; + use revm::L1_BLOCK_CONTRACT; + use std::{collections::HashMap, str::FromStr, sync::Arc}; + + fn create_op_state_provider() -> StateProviderTest { + let mut db = StateProviderTest::default(); + + let l1_block_contract_account = + Account { balance: U256::ZERO, bytecode_hash: None, nonce: 1 }; + + let mut l1_block_storage = HashMap::new(); + // base fee + l1_block_storage.insert(StorageKey::with_last_byte(1), StorageValue::from(1000000000)); + // l1 fee overhead + l1_block_storage.insert(StorageKey::with_last_byte(5), StorageValue::from(188)); + // l1 fee scalar + l1_block_storage.insert(StorageKey::with_last_byte(6), StorageValue::from(684000)); + // l1 free scalars post ecotone + l1_block_storage.insert( + StorageKey::with_last_byte(3), + StorageValue::from_str( + "0x0000000000000000000000000000000000001db0000d27300000000000000005", + ) + .unwrap(), + ); + + db.insert_account(L1_BLOCK_CONTRACT, l1_block_contract_account, None, l1_block_storage); + + db + } + + fn create_op_evm_processor<'a>( + chain_spec: Arc, + db: StateProviderTest, + ) -> EVMProcessor<'a, TestEvmConfig> { + let mut executor = EVMProcessor::new_with_db( + chain_spec, + StateProviderDatabase::new(db), + TestEvmConfig::default(), + ); + executor.evm.context.evm.db.load_cache_account(L1_BLOCK_CONTRACT).unwrap(); + executor + } + + #[test] + fn op_deposit_fields_pre_canyon() { + let header = Header { + timestamp: 1, + number: 1, + gas_limit: 1_000_000, + gas_used: 42_000, + ..Header::default() + }; + + let mut db = create_op_state_provider(); + + let addr = Address::ZERO; + let account = Account { balance: U256::MAX, ..Account::default() }; + db.insert_account(addr, account, None, HashMap::new()); + + let chain_spec = + Arc::new(ChainSpecBuilder::from(&*BASE_MAINNET).regolith_activated().build()); + + let tx = TransactionSigned::from_transaction_and_signature( + Transaction::Eip1559(TxEip1559 { + chain_id: chain_spec.chain.id(), + nonce: 0, + gas_limit: 21_000, + to: TransactionKind::Call(addr), + ..Default::default() + }), + Signature::default(), + ); + + let tx_deposit = TransactionSigned::from_transaction_and_signature( + Transaction::Deposit(reth_primitives::TxDeposit { + from: addr, + to: TransactionKind::Call(addr), + gas_limit: 21_000, + ..Default::default() + }), + Signature::default(), + ); + + let mut executor = create_op_evm_processor(chain_spec, db); + + // Attempt to execute a block with one deposit and one non-deposit transaction + executor + .execute( + &BlockWithSenders { + block: Block { + header: header.clone(), + body: vec![tx, tx_deposit], + ommers: vec![], + withdrawals: None, + }, + senders: vec![addr, addr], + }, + U256::ZERO, + ) + .unwrap(); + + let tx_receipt = executor.receipts[0][0].as_ref().unwrap(); + let deposit_receipt = executor.receipts[0][1].as_ref().unwrap(); + + // deposit_receipt_version is not present in pre canyon transactions + assert!(deposit_receipt.deposit_receipt_version.is_none()); + assert!(tx_receipt.deposit_receipt_version.is_none()); + + // deposit_nonce is present only in deposit transactions + assert!(deposit_receipt.deposit_nonce.is_some()); + assert!(tx_receipt.deposit_nonce.is_none()); + } + + #[test] + fn op_deposit_fields_post_canyon() { + // ensure_create2_deployer will fail if timestamp is set to less then 2 + let header = Header { + timestamp: 2, + number: 1, + gas_limit: 1_000_000, + gas_used: 42_000, + ..Header::default() + }; + + let mut db = create_op_state_provider(); + let addr = Address::ZERO; + let account = Account { balance: U256::MAX, ..Account::default() }; + + db.insert_account(addr, account, None, HashMap::new()); + + let chain_spec = + Arc::new(ChainSpecBuilder::from(&*BASE_MAINNET).canyon_activated().build()); + + let tx = TransactionSigned::from_transaction_and_signature( + Transaction::Eip1559(TxEip1559 { + chain_id: chain_spec.chain.id(), + nonce: 0, + gas_limit: 21_000, + to: TransactionKind::Call(addr), + ..Default::default() + }), + Signature::default(), + ); + + let tx_deposit = TransactionSigned::from_transaction_and_signature( + Transaction::Deposit(reth_primitives::TxDeposit { + from: addr, + to: TransactionKind::Call(addr), + gas_limit: 21_000, + ..Default::default() + }), + Signature::optimism_deposit_tx_signature(), + ); + + let mut executor = create_op_evm_processor(chain_spec, db); + + // attempt to execute an empty block with parent beacon block root, this should not fail + executor + .execute( + &BlockWithSenders { + block: Block { + header: header.clone(), + body: vec![tx, tx_deposit], + ommers: vec![], + withdrawals: None, + }, + senders: vec![addr, addr], + }, + U256::ZERO, + ) + .expect("Executing a block while canyon is active should not fail"); + + let tx_receipt = executor.receipts[0][0].as_ref().unwrap(); + let deposit_receipt = executor.receipts[0][1].as_ref().unwrap(); + + // deposit_receipt_version is set to 1 for post canyon deposit transations + assert_eq!(deposit_receipt.deposit_receipt_version, Some(1)); + assert!(tx_receipt.deposit_receipt_version.is_none()); + + // deposit_nonce is present only in deposit transactions + assert!(deposit_receipt.deposit_nonce.is_some()); + assert!(tx_receipt.deposit_nonce.is_none()); + } +} diff --git a/crates/revm/src/processor.rs b/crates/revm/src/processor.rs index 8283dff48..46bc9cdb3 100644 --- a/crates/revm/src/processor.rs +++ b/crates/revm/src/processor.rs @@ -579,114 +579,19 @@ pub fn compare_receipts_root_and_logs_bloom( #[cfg(test)] mod tests { use super::*; - use reth_interfaces::provider::ProviderResult; - use reth_node_ethereum::EthEvmConfig; + use crate::test_utils::{StateProviderTest, TestEvmConfig}; use reth_primitives::{ bytes, constants::{BEACON_ROOTS_ADDRESS, EIP1559_INITIAL_BASE_FEE, SYSTEM_ADDRESS}, - keccak256, - trie::AccountProof, - Account, Bytecode, Bytes, ChainSpecBuilder, ForkCondition, Signature, StorageKey, - Transaction, TransactionKind, TxEip1559, MAINNET, + keccak256, Account, Bytes, ChainSpecBuilder, ForkCondition, Signature, Transaction, + TransactionKind, TxEip1559, MAINNET, }; - #[cfg(feature = "optimism")] - use reth_provider::BundleStateWithReceipts; - use reth_provider::{AccountReader, BlockHashReader, StateRootProvider}; - use reth_trie::updates::TrieUpdates; use revm::{Database, TransitionState}; use std::collections::HashMap; static BEACON_ROOT_CONTRACT_CODE: Bytes = bytes!("3373fffffffffffffffffffffffffffffffffffffffe14604d57602036146024575f5ffd5b5f35801560495762001fff810690815414603c575f5ffd5b62001fff01545f5260205ff35b5f5ffd5b62001fff42064281555f359062001fff015500"); - #[derive(Debug, Default, Clone, Eq, PartialEq)] - struct StateProviderTest { - accounts: HashMap, Account)>, - contracts: HashMap, - block_hash: HashMap, - } - - impl StateProviderTest { - /// Insert account. - fn insert_account( - &mut self, - address: Address, - mut account: Account, - bytecode: Option, - storage: HashMap, - ) { - if let Some(bytecode) = bytecode { - let hash = keccak256(&bytecode); - account.bytecode_hash = Some(hash); - self.contracts.insert(hash, Bytecode::new_raw(bytecode)); - } - self.accounts.insert(address, (storage, account)); - } - } - - impl AccountReader for StateProviderTest { - fn basic_account(&self, address: Address) -> ProviderResult> { - Ok(self.accounts.get(&address).map(|(_, acc)| *acc)) - } - } - - impl BlockHashReader for StateProviderTest { - fn block_hash(&self, number: u64) -> ProviderResult> { - Ok(self.block_hash.get(&number).cloned()) - } - - fn canonical_hashes_range( - &self, - start: BlockNumber, - end: BlockNumber, - ) -> ProviderResult> { - let range = start..end; - Ok(self - .block_hash - .iter() - .filter_map(|(block, hash)| range.contains(block).then_some(*hash)) - .collect()) - } - } - - impl StateRootProvider for StateProviderTest { - fn state_root(&self, _bundle_state: &BundleStateWithReceipts) -> ProviderResult { - unimplemented!("state root computation is not supported") - } - - fn state_root_with_updates( - &self, - _bundle_state: &BundleStateWithReceipts, - ) -> ProviderResult<(B256, TrieUpdates)> { - unimplemented!("state root computation is not supported") - } - } - - impl StateProvider for StateProviderTest { - fn storage( - &self, - account: Address, - storage_key: StorageKey, - ) -> ProviderResult> { - Ok(self - .accounts - .get(&account) - .and_then(|(storage, _)| storage.get(&storage_key).cloned())) - } - - fn bytecode_by_hash(&self, code_hash: B256) -> ProviderResult> { - Ok(self.contracts.get(&code_hash).cloned()) - } - - fn proof(&self, _address: Address, _keys: &[B256]) -> ProviderResult { - unimplemented!("proof generation is not supported") - } - } - - #[test] - fn eip_4788_non_genesis_call() { - let mut header = - Header { timestamp: 1, number: 1, excess_blob_gas: Some(0), ..Header::default() }; - + fn create_state_provider_with_beacon_root_contract() -> StateProviderTest { let mut db = StateProviderTest::default(); let beacon_root_contract_account = Account { @@ -702,6 +607,16 @@ mod tests { HashMap::new(), ); + db + } + + #[test] + fn eip_4788_non_genesis_call() { + let mut header = + Header { timestamp: 1, number: 1, excess_blob_gas: Some(0), ..Header::default() }; + + let db = create_state_provider_with_beacon_root_contract(); + let chain_spec = Arc::new( ChainSpecBuilder::from(&*MAINNET) .shanghai_activated() @@ -713,7 +628,7 @@ mod tests { let mut executor = EVMProcessor::new_with_db( chain_spec, StateProviderDatabase::new(db), - EthEvmConfig::default(), + TestEvmConfig::default(), ); // attempt to execute a block without parent beacon block root, expect err @@ -805,7 +720,7 @@ mod tests { let mut executor = EVMProcessor::new_with_db( chain_spec, StateProviderDatabase::new(db), - EthEvmConfig::default(), + TestEvmConfig::default(), ); executor.init_env(&header, U256::ZERO); @@ -838,20 +753,8 @@ mod tests { fn eip_4788_empty_account_call() { // This test ensures that we do not increment the nonce of an empty SYSTEM_ADDRESS account // during the pre-block call - let mut db = StateProviderTest::default(); - let beacon_root_contract_account = Account { - balance: U256::ZERO, - bytecode_hash: Some(keccak256(BEACON_ROOT_CONTRACT_CODE.clone())), - nonce: 1, - }; - - db.insert_account( - BEACON_ROOTS_ADDRESS, - beacon_root_contract_account, - Some(BEACON_ROOT_CONTRACT_CODE.clone()), - HashMap::new(), - ); + let mut db = create_state_provider_with_beacon_root_contract(); // insert an empty SYSTEM_ADDRESS db.insert_account(SYSTEM_ADDRESS, Account::default(), None, HashMap::new()); @@ -866,7 +769,7 @@ mod tests { let mut executor = EVMProcessor::new_with_db( chain_spec, StateProviderDatabase::new(db), - EthEvmConfig::default(), + TestEvmConfig::default(), ); // construct the header for block one @@ -905,20 +808,7 @@ mod tests { #[test] fn eip_4788_genesis_call() { - let mut db = StateProviderTest::default(); - - let beacon_root_contract_account = Account { - balance: U256::ZERO, - bytecode_hash: Some(keccak256(BEACON_ROOT_CONTRACT_CODE.clone())), - nonce: 1, - }; - - db.insert_account( - BEACON_ROOTS_ADDRESS, - beacon_root_contract_account, - Some(BEACON_ROOT_CONTRACT_CODE.clone()), - HashMap::new(), - ); + let db = create_state_provider_with_beacon_root_contract(); // activate cancun at genesis let chain_spec = Arc::new( @@ -933,7 +823,7 @@ mod tests { let mut executor = EVMProcessor::new_with_db( chain_spec, StateProviderDatabase::new(db), - EthEvmConfig::default(), + TestEvmConfig::default(), ); executor.init_env(&header, U256::ZERO); @@ -1001,20 +891,7 @@ mod tests { ..Header::default() }; - let mut db = StateProviderTest::default(); - - let beacon_root_contract_account = Account { - balance: U256::ZERO, - bytecode_hash: Some(keccak256(BEACON_ROOT_CONTRACT_CODE.clone())), - nonce: 1, - }; - - db.insert_account( - BEACON_ROOTS_ADDRESS, - beacon_root_contract_account, - Some(BEACON_ROOT_CONTRACT_CODE.clone()), - HashMap::new(), - ); + let db = create_state_provider_with_beacon_root_contract(); let chain_spec = Arc::new( ChainSpecBuilder::from(&*MAINNET) @@ -1027,7 +904,7 @@ mod tests { let mut executor = EVMProcessor::new_with_db( chain_spec, StateProviderDatabase::new(db), - EthEvmConfig::default(), + TestEvmConfig::default(), ); executor.init_env(&header, U256::ZERO); @@ -1089,7 +966,7 @@ mod tests { let mut executor = EVMProcessor::new_with_db( chain_spec, StateProviderDatabase::new(db), - EthEvmConfig::default(), + TestEvmConfig::default(), ); // Create a test transaction that gonna fail diff --git a/crates/revm/src/test_utils.rs b/crates/revm/src/test_utils.rs new file mode 100644 index 000000000..879aa3531 --- /dev/null +++ b/crates/revm/src/test_utils.rs @@ -0,0 +1,173 @@ +use reth_interfaces::provider::ProviderResult; +use reth_node_api::{ConfigureEvm, ConfigureEvmEnv}; +use reth_primitives::{ + keccak256, revm::config::revm_spec, trie::AccountProof, Account, Address, BlockNumber, + Bytecode, Bytes, ChainSpec, Head, Header, StorageKey, Transaction, B256, U256, +}; + +#[cfg(not(feature = "optimism"))] +use reth_primitives::revm::env::fill_tx_env; +#[cfg(feature = "optimism")] +use { + reth_primitives::revm::env::fill_op_tx_env, + revm::{ + primitives::{HandlerCfg, SpecId}, + Database, Evm, EvmBuilder, + }, +}; + +use reth_provider::{ + AccountReader, BlockHashReader, BundleStateWithReceipts, StateProvider, StateRootProvider, +}; +use reth_trie::updates::TrieUpdates; +use revm::primitives::{AnalysisKind, CfgEnvWithHandlerCfg, TxEnv}; +use std::collections::HashMap; + +#[derive(Debug, Default, Clone, Eq, PartialEq)] +pub struct StateProviderTest { + accounts: HashMap, Account)>, + contracts: HashMap, + block_hash: HashMap, +} + +impl StateProviderTest { + /// Insert account. + pub fn insert_account( + &mut self, + address: Address, + mut account: Account, + bytecode: Option, + storage: HashMap, + ) { + if let Some(bytecode) = bytecode { + let hash = keccak256(&bytecode); + account.bytecode_hash = Some(hash); + self.contracts.insert(hash, Bytecode::new_raw(bytecode)); + } + self.accounts.insert(address, (storage, account)); + } +} + +impl AccountReader for StateProviderTest { + fn basic_account(&self, address: Address) -> ProviderResult> { + Ok(self.accounts.get(&address).map(|(_, acc)| *acc)) + } +} + +impl BlockHashReader for StateProviderTest { + fn block_hash(&self, number: u64) -> ProviderResult> { + Ok(self.block_hash.get(&number).cloned()) + } + + fn canonical_hashes_range( + &self, + start: BlockNumber, + end: BlockNumber, + ) -> ProviderResult> { + let range = start..end; + Ok(self + .block_hash + .iter() + .filter_map(|(block, hash)| range.contains(block).then_some(*hash)) + .collect()) + } +} + +impl StateRootProvider for StateProviderTest { + fn state_root(&self, _bundle_state: &BundleStateWithReceipts) -> ProviderResult { + unimplemented!("state root computation is not supported") + } + + fn state_root_with_updates( + &self, + _bundle_state: &BundleStateWithReceipts, + ) -> ProviderResult<(B256, TrieUpdates)> { + unimplemented!("state root computation is not supported") + } +} + +impl StateProvider for StateProviderTest { + fn storage( + &self, + account: Address, + storage_key: StorageKey, + ) -> ProviderResult> { + Ok(self.accounts.get(&account).and_then(|(storage, _)| storage.get(&storage_key).cloned())) + } + + fn bytecode_by_hash(&self, code_hash: B256) -> ProviderResult> { + Ok(self.contracts.get(&code_hash).cloned()) + } + + fn proof(&self, _address: Address, _keys: &[B256]) -> ProviderResult { + unimplemented!("proof generation is not supported") + } +} + +/// Test EVM configuration. +#[derive(Debug, Default, Clone, Copy)] +#[non_exhaustive] +pub struct TestEvmConfig; + +impl ConfigureEvmEnv for TestEvmConfig { + #[cfg(not(feature = "optimism"))] + type TxMeta = (); + #[cfg(feature = "optimism")] + type TxMeta = Bytes; + + #[allow(unused_variables)] + fn fill_tx_env(tx_env: &mut TxEnv, transaction: T, sender: Address, meta: Self::TxMeta) + where + T: AsRef, + { + #[cfg(not(feature = "optimism"))] + fill_tx_env(tx_env, transaction, sender); + #[cfg(feature = "optimism")] + fill_op_tx_env(tx_env, transaction, sender, meta); + } + + fn fill_cfg_env( + cfg_env: &mut CfgEnvWithHandlerCfg, + chain_spec: &ChainSpec, + header: &Header, + total_difficulty: U256, + ) { + let spec_id = revm_spec( + chain_spec, + Head { + number: header.number, + timestamp: header.timestamp, + difficulty: header.difficulty, + total_difficulty, + hash: Default::default(), + }, + ); + + cfg_env.chain_id = chain_spec.chain().id(); + cfg_env.perf_analyse_created_bytecodes = AnalysisKind::Analyse; + + cfg_env.handler_cfg.spec_id = spec_id; + #[cfg(feature = "optimism")] + { + cfg_env.handler_cfg.is_optimism = chain_spec.is_optimism(); + } + } +} + +impl ConfigureEvm for TestEvmConfig { + #[cfg(feature = "optimism")] + fn evm<'a, DB: Database + 'a>(&self, db: DB) -> Evm<'a, (), DB> { + let handler_cfg = HandlerCfg { spec_id: SpecId::LATEST, is_optimism: true }; + EvmBuilder::default().with_db(db).with_handler_cfg(handler_cfg).build() + } + + #[cfg(feature = "optimism")] + fn evm_with_inspector<'a, DB: Database + 'a, I>(&self, db: DB, inspector: I) -> Evm<'a, I, DB> { + let handler_cfg = HandlerCfg { spec_id: SpecId::LATEST, is_optimism: true }; + EvmBuilder::default() + .with_db(db) + .with_external_context(inspector) + .with_handler_cfg(handler_cfg) + .build() + } +}