From a4a65740d70ae6746661591aec927fc54a0bce10 Mon Sep 17 00:00:00 2001 From: clabby Date: Sat, 19 Aug 2023 21:10:06 -0400 Subject: [PATCH 1/9] Start more fixes --- crates/payload/basic/src/optimism.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index ae1dc34c0eed..2ca9ea5c5b04 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -56,7 +56,7 @@ where // TODO(clabby): Add new error type let l1_block_info = (!attributes.transactions.is_empty()).then_some( - L1BlockInfo::try_from(&attributes.transactions[0].input()[..]) + L1BlockInfo::try_from(&attributes.transactions[0].input()[4..]) .map_err(|_| PayloadBuilderError::TransactionEcRecoverFailed)?, ); @@ -195,6 +195,7 @@ where } } + // TODO(clabby): This is for debugging. move back inline to `add_receipt` when done. let r = Receipt { tx_type: sequencer_tx.tx_type(), success: result.is_success(), @@ -226,6 +227,7 @@ where } while let Some(pool_tx) = best_txs.next() { + dbg!("POOL TX!!!!"); // ensure we still have capacity for this transaction if cumulative_gas_used + pool_tx.gas_limit() > block_gas_limit { // we can't fit this transaction into the block, so we need to mark it as invalid From f8f2c4d4a5ac0c38201f9f01bc58f3493aeb9eca Mon Sep 17 00:00:00 2001 From: clabby Date: Sat, 19 Aug 2023 21:21:30 -0400 Subject: [PATCH 2/9] test x --- crates/payload/basic/src/optimism.rs | 20 ++++++++++-- crates/revm/src/executor.rs | 47 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index 2ca9ea5c5b04..b900a5bb14df 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -99,8 +99,7 @@ where cfg.disable_block_gas_limit = true; }; - // Temporarily increase the sender's balance in the database if the deposit transaction - // mints eth. + // Increase the sender's balance in the database if the deposit transaction mints eth. if let Some(m) = sequencer_tx.mint() { let sender = db.load_account(sequencer_tx.signer())?.clone(); let mut sender_new = sender.clone(); @@ -117,6 +116,23 @@ where } } + if !sequencer_tx.is_deposit() { + if let Some(l1_cost) = l1_cost { + let sender = db.load_account(sequencer_tx.signer())?.clone(); + let mut sender_new = sender.clone(); + sender_new.info.balance -= U256::from(l1_cost); + + executor::decrement_account_balance( + &mut db, + &mut post_state, + parent_block.number + 1, + sequencer_tx.signer(), + U256::from(l1_cost), + )?; + db.insert_account_info(sequencer_tx.signer(), sender_new.info); + } + } + // Configure the environment for the block. let env = Env { cfg, diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index 5fa14d896e92..cb6f5f3a8aea 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -584,6 +584,53 @@ where Ok(()) } +/// Decrement the balance for the given account in the [PostState]. +/// +/// Returns an error if the database encountered an error while loading the account. +pub fn decrement_account_balance( + db: &mut CacheDB, + post_state: &mut PostState, + block_number: BlockNumber, + address: Address, + decrement: U256, +) -> Result<(), ::Error> +where + DB: DatabaseRef, +{ + let beneficiary = db.load_account(address)?; + let old = to_reth_acc(&beneficiary.info); + // Increment beneficiary balance by mutating db entry in place. + beneficiary.info.balance -= decrement; + let new = to_reth_acc(&beneficiary.info); + match beneficiary.account_state { + AccountState::NotExisting => { + // if account was not existing that means that storage is not + // present. + beneficiary.account_state = AccountState::StorageCleared; + + // if account was not present append `Created` changeset + post_state.create_account( + block_number, + address, + Account { nonce: 0, balance: new.balance, bytecode_hash: None }, + ) + } + + AccountState::StorageCleared | AccountState::Touched | AccountState::None => { + // If account is None that means that EVM didn't touch it. + // we are changing the state to Touched as account can have + // storage in db. + if beneficiary.account_state == AccountState::None { + beneficiary.account_state = AccountState::Touched; + } + // if account was present, append changed changeset. + post_state.change_account(block_number, address, old, new); + } + } + + Ok(()) +} + /// Commit change to the _run-time_ database [CacheDB], and update the given [PostState] with the /// changes made in the transaction, which can be persisted to the database. /// From f3463d19535ca14688e7a0fe9206c438334b696e Mon Sep 17 00:00:00 2001 From: clabby Date: Sat, 19 Aug 2023 21:36:56 -0400 Subject: [PATCH 3/9] wat do x x x x x x --- crates/payload/basic/src/optimism.rs | 23 +++++----- crates/revm/src/executor.rs | 63 +++++++++++++++++----------- crates/revm/src/optimism.rs | 9 ++-- 3 files changed, 53 insertions(+), 42 deletions(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index b900a5bb14df..3a1491d19b6d 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -114,9 +114,7 @@ where )?; db.insert_account_info(sequencer_tx.signer(), sender_new.info); } - } - - if !sequencer_tx.is_deposit() { + } else { if let Some(l1_cost) = l1_cost { let sender = db.load_account(sequencer_tx.signer())?.clone(); let mut sender_new = sender.clone(); @@ -188,7 +186,8 @@ where } else if sequencer_tx.is_deposit() && !sequencer_tx.is_system_transaction() { cumulative_gas_used += sequencer_tx.gas_limit(); } - if !(sequencer_tx.is_deposit() && is_regolith) { + + if !sequencer_tx.is_deposit() { // Route the l1 cost and base fee to the appropriate optimism vaults if let Some(l1_cost) = l1_cost { executor::increment_account_balance( @@ -199,15 +198,13 @@ where l1_cost, )? } - if !sequencer_tx.is_deposit() { - executor::increment_account_balance( - &mut db, - &mut post_state, - parent_block.number + 1, - executor::optimism::base_fee_recipient(), - U256::from(base_fee.saturating_mul(result.gas_used())), - )?; - } + executor::increment_account_balance( + &mut db, + &mut post_state, + parent_block.number + 1, + executor::optimism::base_fee_recipient(), + U256::from(base_fee.saturating_mul(result.gas_used())), + )?; } } diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index cb6f5f3a8aea..8e4d36946a89 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -174,6 +174,18 @@ where .map_err(|_| BlockExecutionError::ProviderError) } + /// Decrement the balance for the given account in the [PostState]. + fn decrement_account_balance( + &mut self, + block_number: BlockNumber, + address: Address, + decrement: U256, + post_state: &mut PostState, + ) -> Result<(), BlockExecutionError> { + decrement_account_balance(self.db(), post_state, block_number, address, decrement) + .map_err(|_| BlockExecutionError::ProviderError) + } + /// Runs a single transaction in the configured environment and proceeds /// to return the result and state diff (without applying it). /// @@ -284,17 +296,16 @@ where ) }); - let mut sender_account = db + let sender_account = db .load_account(sender) .map_err(|_| BlockExecutionError::ProviderError)? .clone(); - let old_sender_info = to_reth_acc(&sender_account.info); if let Some(l1_cost) = l1_cost { // Check if the sender balance can cover the L1 cost. // Deposits pay for their gas directly on L1 so they are exempt from the L2 // tx fee. - if !transaction.is_system_transaction() { + if !transaction.is_deposit() { if sender_account.info.balance.cmp(&l1_cost) == std::cmp::Ordering::Less { return Err(BlockExecutionError::InsufficientFundsForL1Cost { have: sender_account.info.balance.to::(), @@ -305,14 +316,15 @@ where // Safely take l1_cost from sender (the rest will be deducted by the // internal EVM execution and included in result.gas_used()) // TODO: need to handle calls with `disable_balance_check` flag set? - sender_account.info.balance -= l1_cost; + self.decrement_account_balance( + block.number, + sender, + l1_cost, + &mut post_state, + )?; } } - let new_sender_info = to_reth_acc(&sender_account.info); - post_state.change_account(block.number, sender, old_sender_info, new_sender_info); - db.insert_account_info(sender, sender_account.info); - // Execute transaction. let ResultAndState { result, state } = self.transact(transaction, sender)?; @@ -362,24 +374,25 @@ where &mut post_state, ); - if is_regolith || !transaction.is_deposit() { - cumulative_gas_used += result.gas_used() - } else if transaction.is_deposit() && !transaction.is_system_transaction() { - cumulative_gas_used += transaction.gas_limit(); - } - - // Skip coinbase payments in Regolith for deposit transactions - if self.chain_spec.optimism && !(transaction.is_deposit() && is_regolith) { - // Route the l1 cost and base fee to the appropriate optimism vaults - if let Some(l1_cost) = l1_cost { - self.increment_account_balance( - block.number, - optimism::l1_cost_recipient(), - l1_cost, - &mut post_state, - )? + // Pay out fees to Optimism vaults. + if self.chain_spec.optimism { + if is_regolith || !transaction.is_deposit() { + cumulative_gas_used += result.gas_used() + } else if transaction.is_deposit() && !transaction.is_system_transaction() { + cumulative_gas_used += transaction.gas_limit(); } if !transaction.is_deposit() { + // Route the l1 cost and base fee to the appropriate optimism vaults + if let Some(l1_cost) = l1_cost { + dbg!("L1 BLOCK INFO: ", l1_block_info.as_ref()); + dbg!("L1 COST: ", l1_cost); + self.increment_account_balance( + block.number, + optimism::l1_cost_recipient(), + l1_cost, + &mut post_state, + )? + } self.increment_account_balance( block.number, optimism::base_fee_recipient(), @@ -409,7 +422,7 @@ where logs, // Deposit nonce is only recorded after Regolith for deposit transactions. deposit_nonce: (is_regolith && transaction.is_deposit()) - .then_some(old_sender_info.nonce), + .then_some(sender_account.info.nonce), }, ); diff --git a/crates/revm/src/optimism.rs b/crates/revm/src/optimism.rs index 4b543b18e1ba..e40ef90e04de 100644 --- a/crates/revm/src/optimism.rs +++ b/crates/revm/src/optimism.rs @@ -21,6 +21,7 @@ const NON_ZERO_BYTE_COST: u64 = 16; /// uint64 _sequenceNumber, bytes32 _batcherHash, uint256 _l1FeeOverhead, uint256 _l1FeeScalar) /// /// For now, we only care about the fields necessary for L1 cost calculation. +#[derive(Debug)] pub struct L1BlockInfo { l1_base_fee: U256, l1_fee_overhead: U256, @@ -103,15 +104,15 @@ impl L1BlockInfo { acc + if *byte == 0x00 { ZERO_BYTE_COST } else { NON_ZERO_BYTE_COST } })); - if is_deposit || rollup_data_gas_cost == U256::ZERO { - return U256::ZERO - } - // Prior to regolith, an extra 68 non zero bytes were included in the rollup data costs. if !chain_spec.fork(reth_primitives::Hardfork::Regolith).active_at_timestamp(timestamp) { rollup_data_gas_cost += U256::from(NON_ZERO_BYTE_COST).mul(U256::from(68)); } + if is_deposit || rollup_data_gas_cost == U256::ZERO { + return U256::ZERO + } + rollup_data_gas_cost .saturating_add(self.l1_fee_overhead) .saturating_mul(self.l1_base_fee) From dd9712b1273ed218819554fb2d70987fc981e45e Mon Sep 17 00:00:00 2001 From: clabby Date: Sat, 19 Aug 2023 23:49:13 -0400 Subject: [PATCH 4/9] Not input, encoded enveloped :sweat_smile: --- crates/payload/basic/src/optimism.rs | 5 ++++- crates/revm/src/executor.rs | 9 ++++++--- crates/transaction-pool/src/validate/eth.rs | 9 ++++++--- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index 3a1491d19b6d..5fac860ec3a6 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -3,6 +3,7 @@ use super::*; use reth_primitives::Hardfork; use reth_revm::{executor, optimism::L1BlockInfo}; +use reth_rlp::BufMut; /// Constructs an Ethereum transaction payload from the transactions sent through the /// Payload attributes by the sequencer. If the `no_tx_pool` argument is passed in @@ -77,11 +78,13 @@ where .try_into_ecrecovered() .map_err(|_| PayloadBuilderError::TransactionEcRecoverFailed)?; + let mut encoded = BytesMut::default(); + sequencer_tx.encode_enveloped(&mut encoded); let l1_cost = l1_block_info.as_ref().map(|l1_block_info| { l1_block_info.calculate_tx_l1_cost( Arc::clone(&chain_spec), attributes.timestamp, - sequencer_tx.input(), + &reth_primitives::Bytes::from(&encoded[..]), sequencer_tx.is_deposit(), ) }); diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index 8e4d36946a89..c4abebf6c516 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -9,8 +9,8 @@ use crate::{ use reth_consensus_common::calc; use reth_interfaces::executor::{BlockExecutionError, BlockValidationError}; use reth_primitives::{ - Account, Address, Block, BlockNumber, Bloom, Bytecode, ChainSpec, Hardfork, Header, Receipt, - ReceiptWithBloom, TransactionSigned, Withdrawal, H256, U256, + bytes::BytesMut, Account, Address, Block, BlockNumber, Bloom, Bytecode, Bytes, ChainSpec, + Hardfork, Header, Receipt, ReceiptWithBloom, TransactionSigned, Withdrawal, H256, U256, }; use reth_provider::{BlockExecutor, PostState, StateProvider}; use revm::{ @@ -287,11 +287,14 @@ where let chain_spec = Arc::clone(&self.chain_spec); let db = self.db(); + + let mut encoded = BytesMut::default(); + transaction.encode_enveloped(&mut encoded); let l1_cost = l1_block_info.as_ref().map(|l1_block_info| { l1_block_info.calculate_tx_l1_cost( chain_spec, block.timestamp, - transaction.input(), + &Bytes::from(&encoded[..]), transaction.is_deposit(), ) }); diff --git a/crates/transaction-pool/src/validate/eth.rs b/crates/transaction-pool/src/validate/eth.rs index 2e203ec1dd15..5078ec7e4010 100644 --- a/crates/transaction-pool/src/validate/eth.rs +++ b/crates/transaction-pool/src/validate/eth.rs @@ -8,8 +8,9 @@ use crate::{ TransactionValidationOutcome, TransactionValidationTaskExecutor, TransactionValidator, }; use reth_primitives::{ - constants::ETHEREUM_BLOCK_GAS_LIMIT, ChainSpec, InvalidTransactionError, EIP1559_TX_TYPE_ID, - EIP2930_TX_TYPE_ID, EIP4844_TX_TYPE_ID, LEGACY_TX_TYPE_ID, + bytes::BytesMut, constants::ETHEREUM_BLOCK_GAS_LIMIT, Bytes, ChainSpec, + InvalidTransactionError, EIP1559_TX_TYPE_ID, EIP2930_TX_TYPE_ID, EIP4844_TX_TYPE_ID, + LEGACY_TX_TYPE_ID, }; use reth_provider::{AccountReader, BlockReaderIdExt, StateProviderFactory}; use reth_tasks::TaskSpawner; @@ -245,11 +246,13 @@ where } }; + let mut encoded = BytesMut::default(); + transaction.to_recovered_transaction().encode_enveloped(&mut encoded); let cost_addition = match L1BlockInfo::try_from(&block) { Ok(info) => info.calculate_tx_l1_cost( Arc::clone(&self.chain_spec), block.timestamp, - transaction.input(), + &Bytes::from(&encoded[..]), transaction.is_deposit(), ), Err(err) => { From 3b225dbf1cf95b709b5fb847852b2f6d97760dae Mon Sep 17 00:00:00 2001 From: clabby Date: Sat, 19 Aug 2023 23:59:58 -0400 Subject: [PATCH 5/9] Remove debug logs --- crates/payload/basic/src/lib.rs | 4 --- crates/payload/basic/src/optimism.rs | 51 ++++++++++++---------------- crates/revm/src/executor.rs | 5 --- 3 files changed, 21 insertions(+), 39 deletions(-) diff --git a/crates/payload/basic/src/lib.rs b/crates/payload/basic/src/lib.rs index 04ba7d8d09ed..3b545247e5d0 100644 --- a/crates/payload/basic/src/lib.rs +++ b/crates/payload/basic/src/lib.rs @@ -175,7 +175,6 @@ where #[cfg(feature = "optimism")] if config.attributes.no_tx_pool { - dbg!("No tx pool; Building"); let args = BuildArguments { client: self.client.clone(), pool: self.pool.clone(), @@ -186,7 +185,6 @@ where }; match self.builder.try_build(args)? { BuildOutcome::Better { payload, cached_reads } => { - dbg!("Got better payload", &payload); return Ok(BasicPayloadJob { config, client: self.client.clone(), @@ -491,8 +489,6 @@ where empty_payload = Some(rx); } - dbg!(&best_payload, &maybe_better, &empty_payload); - let fut = ResolveBestPayload { best_payload, maybe_better, empty_payload }; (fut, KeepPayloadJobAlive::No) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index 5fac860ec3a6..b99845f5b384 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -154,7 +154,6 @@ where // want to revisit this. match err { EVMError::Transaction(err) => { - dbg!("Transaction error", err); if matches!(err, InvalidTransaction::NonceTooLow { .. }) { // if the nonce is too low, we can skip this transaction trace!(?err, ?sequencer_tx, "skipping nonce too low transaction"); @@ -170,15 +169,12 @@ where continue } err => { - dbg!("EVM Error", &err); // this is an error that we should treat as fatal for this attempt return Err(PayloadBuilderError::EvmExecutionError(err)) } } } }; - dbg!("EXECUTED ", sequencer_tx.hash()); - // commit changes commit_state_changes(&mut db, &mut post_state, block_number, state, true); @@ -211,39 +207,34 @@ where } } - // TODO(clabby): This is for debugging. move back inline to `add_receipt` when done. - let r = Receipt { - tx_type: sequencer_tx.tx_type(), - success: result.is_success(), - cumulative_gas_used, - logs: result.logs().into_iter().map(into_reth_log).collect(), - deposit_nonce: if is_regolith && sequencer_tx.is_deposit() { - // Recovering the signer from the deposit transaction is only fetching - // the `from` address. Deposit transactions have no signature. - let from = sequencer_tx.signer(); - let account = db.load_account(from)?; - // The deposit nonce is the account's nonce - 1. The account's nonce - // was incremented during the execution of the deposit transaction - // above. - Some(account.info.nonce.saturating_sub(1)) - } else { - None - }, - }; - - dbg!(&r, result); - // Push transaction changeset and calculate header bloom filter for receipt. - post_state.add_receipt(block_number, r); - - dbg!("COMMITTED STATE CHANGES"); + post_state.add_receipt( + block_number, + Receipt { + tx_type: sequencer_tx.tx_type(), + success: result.is_success(), + cumulative_gas_used, + logs: result.logs().into_iter().map(into_reth_log).collect(), + deposit_nonce: if is_regolith && sequencer_tx.is_deposit() { + // Recovering the signer from the deposit transaction is only fetching + // the `from` address. Deposit transactions have no signature. + let from = sequencer_tx.signer(); + let account = db.load_account(from)?; + // The deposit nonce is the account's nonce - 1. The account's nonce + // was incremented during the execution of the deposit transaction + // above. + Some(account.info.nonce.saturating_sub(1)) + } else { + None + }, + }, + ); // append transaction to the list of executed transactions executed_txs.push(sequencer_tx.into_signed()); } while let Some(pool_tx) = best_txs.next() { - dbg!("POOL TX!!!!"); // ensure we still have capacity for this transaction if cumulative_gas_used + pool_tx.gas_limit() > block_gas_limit { // we can't fit this transaction into the block, so we need to mark it as invalid diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index c4abebf6c516..adcd5aa92ffb 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -332,7 +332,6 @@ where let ResultAndState { result, state } = self.transact(transaction, sender)?; if transaction.is_deposit() && !result.is_success() { - dbg!("FAILED DEPOSIT!!!!", &result); // If the Deposited transaction failed, the deposit must still be included. // In this case, we need to increment the sender nonce and disregard the // state changes. The transaction is also recorded as using all gas. @@ -387,8 +386,6 @@ where if !transaction.is_deposit() { // Route the l1 cost and base fee to the appropriate optimism vaults if let Some(l1_cost) = l1_cost { - dbg!("L1 BLOCK INFO: ", l1_block_info.as_ref()); - dbg!("L1 COST: ", l1_cost); self.increment_account_balance( block.number, optimism::l1_cost_recipient(), @@ -477,8 +474,6 @@ where } } - dbg!(&post_state); - Ok((post_state, cumulative_gas_used)) } From 0480cb6b5f51fb3b54b8ebcd9d787e99e6d5d5ce Mon Sep 17 00:00:00 2001 From: clabby Date: Sun, 20 Aug 2023 00:51:44 -0400 Subject: [PATCH 6/9] Clean up optimism payload builder --- crates/payload/basic/src/optimism.rs | 67 ++++++++++++-------- crates/payload/builder/src/error.rs | 5 ++ crates/revm/src/executor.rs | 94 +++++++++++++--------------- 3 files changed, 90 insertions(+), 76 deletions(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index b99845f5b384..d7c3a82d0fc9 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -3,7 +3,6 @@ use super::*; use reth_primitives::Hardfork; use reth_revm::{executor, optimism::L1BlockInfo}; -use reth_rlp::BufMut; /// Constructs an Ethereum transaction payload from the transactions sent through the /// Payload attributes by the sequencer. If the `no_tx_pool` argument is passed in @@ -55,10 +54,12 @@ where let is_regolith = chain_spec.is_fork_active_at_timestamp(Hardfork::Regolith, attributes.timestamp); - // TODO(clabby): Add new error type + // Parse the L1 block info from the first transaction in the payload attributes. This + // transaction should always be the L1 info tx. We skip the first 4 bytes of the calldata + // because the first 4 bytes are the function selector. let l1_block_info = (!attributes.transactions.is_empty()).then_some( L1BlockInfo::try_from(&attributes.transactions[0].input()[4..]) - .map_err(|_| PayloadBuilderError::TransactionEcRecoverFailed)?, + .map_err(|_| PayloadBuilderError::L1BlockInfoParseFailed)?, ); // Transactions sent via the payload attributes are force included at the top of the block, in @@ -78,6 +79,8 @@ where .try_into_ecrecovered() .map_err(|_| PayloadBuilderError::TransactionEcRecoverFailed)?; + // Compute the L1 cost of the transaction. This is the amount of ETH that it will cost to + // post the entire encoded typed transaction to L1. let mut encoded = BytesMut::default(); sequencer_tx.encode_enveloped(&mut encoded); let l1_cost = l1_block_info.as_ref().map(|l1_block_info| { @@ -91,6 +94,16 @@ where let mut cfg = initialized_cfg.clone(); + let sender = db.load_account(sequencer_tx.signer())?.clone(); + let mut sender_new = sender.clone(); + + // If the transaction is a deposit, we need to disable the base fee, balance check, and + // gas refund. We also need to disable the block gas limit if the Regolith hardfork is not + // active. In addition, we need to increase the sender's balance by the mint value of the + // deposit transaction if it is `Some(n)`. + // + // Otherwise, we need to decrement the sender's balance by the L1 cost of the transaction + // prior to execution. if sequencer_tx.is_deposit() { cfg.disable_base_fee = true; cfg.disable_balance_check = true; @@ -98,40 +111,35 @@ where // If the Regolith hardfork is active, we do not need to disable the block gas limit. // Otherwise, we allow for the block gas limit to be exceeded by deposit transactions. - if !is_regolith && sequencer_tx.is_deposit() { + if !is_regolith { cfg.disable_block_gas_limit = true; - }; + } // Increase the sender's balance in the database if the deposit transaction mints eth. if let Some(m) = sequencer_tx.mint() { - let sender = db.load_account(sequencer_tx.signer())?.clone(); - let mut sender_new = sender.clone(); - sender_new.info.balance += U256::from(m); + let m = U256::from(m); + sender_new.info.balance += m; executor::increment_account_balance( &mut db, &mut post_state, parent_block.number + 1, sequencer_tx.signer(), - U256::from(m), - )?; - db.insert_account_info(sequencer_tx.signer(), sender_new.info); - } - } else { - if let Some(l1_cost) = l1_cost { - let sender = db.load_account(sequencer_tx.signer())?.clone(); - let mut sender_new = sender.clone(); - sender_new.info.balance -= U256::from(l1_cost); - - executor::decrement_account_balance( - &mut db, - &mut post_state, - parent_block.number + 1, - sequencer_tx.signer(), - U256::from(l1_cost), + m, )?; db.insert_account_info(sequencer_tx.signer(), sender_new.info); } + } else if let Some(l1_cost) = l1_cost { + // Decrement the sender's balance by the L1 cost of the transaction prior to execution. + sender_new.info.balance -= l1_cost; + executor::decrement_account_balance( + &mut db, + &mut post_state, + parent_block.number + 1, + sequencer_tx.signer(), + l1_cost, + )?; + db.insert_account_info(sequencer_tx.signer(), sender_new.info); } // Configure the environment for the block. @@ -178,14 +186,23 @@ where // commit changes commit_state_changes(&mut db, &mut post_state, block_number, state, true); - // Skip coinbase payments in Regolith for deposit transactions if chain_spec.optimism { + // If either Regolith is active or the transaction is not a deposit, we report the + // actual gas used in execution to the cumulative gas. + // + // Otherwise, if we are pre-Regolith and the transaction is a non-system-tx deposit, + // we report the gas limit of the transaction to the cumulative gas. Pre-Regolith, + // system transactions are not included in the cumulative gas and their execution + // gas is ignored. Post regolith, system transactions are deprecated and no longer + // exist. if is_regolith || !sequencer_tx.is_deposit() { cumulative_gas_used += result.gas_used() } else if sequencer_tx.is_deposit() && !sequencer_tx.is_system_transaction() { cumulative_gas_used += sequencer_tx.gas_limit(); } + // If the transaction is not a deposit, we route the l1 cost and base fee to the + // appropriate optimism vaults. if !sequencer_tx.is_deposit() { // Route the l1 cost and base fee to the appropriate optimism vaults if let Some(l1_cost) = l1_cost { diff --git a/crates/payload/builder/src/error.rs b/crates/payload/builder/src/error.rs index d92d14eea073..1ce6e8a3ebd2 100644 --- a/crates/payload/builder/src/error.rs +++ b/crates/payload/builder/src/error.rs @@ -27,6 +27,11 @@ pub enum PayloadBuilderError { #[cfg(feature = "optimism")] #[error("failed to convert deposit transaction to TransactionSignedEcRecovered")] TransactionEcRecoverFailed, + /// Thrown when the L1 block info could not be parsed from the calldata of the + /// first transaction supplied in the payload attributes. + #[cfg(feature = "optimism")] + #[error("failed to parse L1 block info from L1 info tx calldata")] + L1BlockInfoParseFailed, } impl From for PayloadBuilderError { diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index adcd5aa92ffb..dda666717095 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -14,7 +14,7 @@ use reth_primitives::{ }; use reth_provider::{BlockExecutor, PostState, StateProvider}; use revm::{ - db::{AccountState, CacheDB, DatabaseRef}, + db::{AccountState, CacheDB, DatabaseRef, DbAccount}, primitives::{ hash_map::{self, Entry}, Account as RevmAccount, AccountInfo, ResultAndState, @@ -566,31 +566,7 @@ where // Increment beneficiary balance by mutating db entry in place. beneficiary.info.balance += increment; let new = to_reth_acc(&beneficiary.info); - match beneficiary.account_state { - AccountState::NotExisting => { - // if account was not existing that means that storage is not - // present. - beneficiary.account_state = AccountState::StorageCleared; - - // if account was not present append `Created` changeset - post_state.create_account( - block_number, - address, - Account { nonce: 0, balance: new.balance, bytecode_hash: None }, - ) - } - - AccountState::StorageCleared | AccountState::Touched | AccountState::None => { - // If account is None that means that EVM didn't touch it. - // we are changing the state to Touched as account can have - // storage in db. - if beneficiary.account_state == AccountState::None { - beneficiary.account_state = AccountState::Touched; - } - // if account was present, append changed changeset. - post_state.change_account(block_number, address, old, new); - } - } + update_account(beneficiary, post_state, address, old, new, block_number); Ok(()) } @@ -612,32 +588,9 @@ where let old = to_reth_acc(&beneficiary.info); // Increment beneficiary balance by mutating db entry in place. beneficiary.info.balance -= decrement; - let new = to_reth_acc(&beneficiary.info); - match beneficiary.account_state { - AccountState::NotExisting => { - // if account was not existing that means that storage is not - // present. - beneficiary.account_state = AccountState::StorageCleared; - // if account was not present append `Created` changeset - post_state.create_account( - block_number, - address, - Account { nonce: 0, balance: new.balance, bytecode_hash: None }, - ) - } - - AccountState::StorageCleared | AccountState::Touched | AccountState::None => { - // If account is None that means that EVM didn't touch it. - // we are changing the state to Touched as account can have - // storage in db. - if beneficiary.account_state == AccountState::None { - beneficiary.account_state = AccountState::Touched; - } - // if account was present, append changed changeset. - post_state.change_account(block_number, address, old, new); - } - } + let new = to_reth_acc(&beneficiary.info); + update_account(beneficiary, post_state, address, old, new, block_number); Ok(()) } @@ -812,6 +765,45 @@ pub fn verify_receipt<'a>( Ok(()) } +/// Updates an account in the passed post state and sets the [DbAccount]'s state +/// to [AccountState::Touched] if the account was not touched before or to +/// [AccountState::StorageCleared] if the account did not exist prior to the update. +#[inline] +fn update_account( + beneficiary: &mut DbAccount, + post_state: &mut PostState, + address: Address, + old: Account, + new: Account, + block_number: BlockNumber, +) { + match beneficiary.account_state { + AccountState::NotExisting => { + // if account was not existing that means that storage is not + // present. + beneficiary.account_state = AccountState::StorageCleared; + + // if account was not present append `Created` changeset + post_state.create_account( + block_number, + address, + Account { nonce: 0, balance: new.balance, bytecode_hash: None }, + ) + } + + AccountState::StorageCleared | AccountState::Touched | AccountState::None => { + // If account is None that means that EVM didn't touch it. + // we are changing the state to Touched as account can have + // storage in db. + if beneficiary.account_state == AccountState::None { + beneficiary.account_state = AccountState::Touched; + } + // if account was present, append changed changeset. + post_state.change_account(block_number, address, old, new); + } + } +} + /// Collect all balance changes at the end of the block. /// /// Balance changes might include the block reward, uncle rewards, withdrawals, or irregular From 0f6201d4f01089f56f190dd7383932048c5aae48 Mon Sep 17 00:00:00 2001 From: clabby Date: Sun, 20 Aug 2023 01:51:37 -0400 Subject: [PATCH 7/9] Add failed deposit receipts in optimism payload builder --- crates/payload/basic/src/optimism.rs | 31 +++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index d7c3a82d0fc9..2d0f1bfe8e44 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -155,11 +155,32 @@ where let ResultAndState { result, state } = match evm.transact() { Ok(res) => res, Err(err) => { - // TODO(clabby): This could be an issue - deposit transactions should always be - // included with a receipt regardless of if there was an error or not. The - // sequencer performs some basic validation on the transactions it sends prior - // to sending a fork choice update, so this shouldn't be an issue, but we may - // want to revisit this. + if sequencer_tx.is_deposit() { + post_state.add_receipt( + block_number, + Receipt { + tx_type: sequencer_tx.tx_type(), + success: false, + cumulative_gas_used, + logs: vec![], + deposit_nonce: if is_regolith && sequencer_tx.is_deposit() { + // Recovering the signer from the deposit transaction is only + // fetching the `from` address. + // Deposit transactions have no signature. + let from = sequencer_tx.signer(); + let account = db.load_account(from)?; + // The deposit nonce is the account's nonce - 1. The account's nonce + // was incremented during the execution of the deposit transaction + // above. + Some(account.info.nonce.saturating_sub(1)) + } else { + None + }, + }, + ); + continue + } + match err { EVMError::Transaction(err) => { if matches!(err, InvalidTransaction::NonceTooLow { .. }) { From c62e4cd0f5717aea8f0876b58c439921191e7057 Mon Sep 17 00:00:00 2001 From: clabby Date: Sun, 20 Aug 2023 04:38:07 -0400 Subject: [PATCH 8/9] dbg x :broom: --- crates/payload/basic/src/optimism.rs | 2 +- crates/revm/src/executor.rs | 2 +- crates/revm/src/optimism.rs | 4 ++-- crates/transaction-pool/src/validate/eth.rs | 7 +++---- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/crates/payload/basic/src/optimism.rs b/crates/payload/basic/src/optimism.rs index 2d0f1bfe8e44..cb786ee75360 100644 --- a/crates/payload/basic/src/optimism.rs +++ b/crates/payload/basic/src/optimism.rs @@ -87,7 +87,7 @@ where l1_block_info.calculate_tx_l1_cost( Arc::clone(&chain_spec), attributes.timestamp, - &reth_primitives::Bytes::from(&encoded[..]), + &encoded.freeze().into(), sequencer_tx.is_deposit(), ) }); diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index dda666717095..b9e56e84cb30 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -294,7 +294,7 @@ where l1_block_info.calculate_tx_l1_cost( chain_spec, block.timestamp, - &Bytes::from(&encoded[..]), + &encoded.freeze().into(), transaction.is_deposit(), ) }); diff --git a/crates/revm/src/optimism.rs b/crates/revm/src/optimism.rs index e40ef90e04de..ac6447f527e3 100644 --- a/crates/revm/src/optimism.rs +++ b/crates/revm/src/optimism.rs @@ -1,7 +1,7 @@ use std::{ops::Mul, str::FromStr, sync::Arc}; use reth_interfaces::executor; -use reth_primitives::{Address, Block, Bytes, ChainSpec, TransactionKind, U256}; +use reth_primitives::{Address, Block, Bytes, ChainSpec, Hardfork, TransactionKind, U256}; const L1_FEE_RECIPIENT: &str = "0x420000000000000000000000000000000000001A"; const BASE_FEE_RECIPIENT: &str = "0x4200000000000000000000000000000000000019"; @@ -105,7 +105,7 @@ impl L1BlockInfo { })); // Prior to regolith, an extra 68 non zero bytes were included in the rollup data costs. - if !chain_spec.fork(reth_primitives::Hardfork::Regolith).active_at_timestamp(timestamp) { + if !chain_spec.fork(Hardfork::Regolith).active_at_timestamp(timestamp) { rollup_data_gas_cost += U256::from(NON_ZERO_BYTE_COST).mul(U256::from(68)); } diff --git a/crates/transaction-pool/src/validate/eth.rs b/crates/transaction-pool/src/validate/eth.rs index 5078ec7e4010..966efb97c577 100644 --- a/crates/transaction-pool/src/validate/eth.rs +++ b/crates/transaction-pool/src/validate/eth.rs @@ -8,9 +8,8 @@ use crate::{ TransactionValidationOutcome, TransactionValidationTaskExecutor, TransactionValidator, }; use reth_primitives::{ - bytes::BytesMut, constants::ETHEREUM_BLOCK_GAS_LIMIT, Bytes, ChainSpec, - InvalidTransactionError, EIP1559_TX_TYPE_ID, EIP2930_TX_TYPE_ID, EIP4844_TX_TYPE_ID, - LEGACY_TX_TYPE_ID, + bytes::BytesMut, constants::ETHEREUM_BLOCK_GAS_LIMIT, ChainSpec, InvalidTransactionError, + EIP1559_TX_TYPE_ID, EIP2930_TX_TYPE_ID, EIP4844_TX_TYPE_ID, LEGACY_TX_TYPE_ID, }; use reth_provider::{AccountReader, BlockReaderIdExt, StateProviderFactory}; use reth_tasks::TaskSpawner; @@ -252,7 +251,7 @@ where Ok(info) => info.calculate_tx_l1_cost( Arc::clone(&self.chain_spec), block.timestamp, - &Bytes::from(&encoded[..]), + &encoded.freeze().into(), transaction.is_deposit(), ), Err(err) => { From 088e28f6d2f91ffbf12911d2611424993d632e0f Mon Sep 17 00:00:00 2001 From: clabby Date: Sun, 20 Aug 2023 11:28:55 -0400 Subject: [PATCH 9/9] lint --- crates/revm/src/executor.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index b9e56e84cb30..7b2386229a83 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -9,8 +9,8 @@ use crate::{ use reth_consensus_common::calc; use reth_interfaces::executor::{BlockExecutionError, BlockValidationError}; use reth_primitives::{ - bytes::BytesMut, Account, Address, Block, BlockNumber, Bloom, Bytecode, Bytes, ChainSpec, - Hardfork, Header, Receipt, ReceiptWithBloom, TransactionSigned, Withdrawal, H256, U256, + bytes::BytesMut, Account, Address, Block, BlockNumber, Bloom, Bytecode, ChainSpec, Hardfork, + Header, Receipt, ReceiptWithBloom, TransactionSigned, Withdrawal, H256, U256, }; use reth_provider::{BlockExecutor, PostState, StateProvider}; use revm::{