chore: Last PR cleanup, nits (#353)

* chore: cleanup, tests and nits

* fmt

Cargo.lock

@@ -3214,6 +3214,7 @@ dependencies = [
  "hashbrown 0.13.1",
  "plain_hasher",
  "reth-consensus",
+ "reth-db",
  "reth-interfaces",
  "reth-primitives",
  "reth-rlp",

crates/executor/Cargo.toml

@@ -31,3 +31,7 @@ hash-db = "0.15"
 rlp = { version = "0.5", default-features = false }
 # replace with tiny-keccak (it is faster hasher)
 sha3 = { version = "0.10", default-features = false }
+
+
+[dev-dependencies]
+reth-db = { path = "../db", features = ["test-utils"]}

crates/executor/src/config.rs

@@ -2,6 +2,13 @@
 
 use reth_primitives::{BlockNumber, U256};
 
+/// Two ethereum worth of wei
+pub const WEI_2ETH: u128 = 2000000000000000000u128;
+/// Three ethereum worth of wei
+pub const WEI_3ETH: u128 = 3000000000000000000u128;
+/// Five ethereum worth of wei
+pub const WEI_5ETH: u128 = 5000000000000000000u128;
+
 /// Configuration for executor
 #[derive(Debug, Clone)]
 pub struct Config {
@@ -71,48 +78,48 @@ impl SpecUpgrades {
     }
 
     /// New homestead enabled spec
-    pub fn new_test_homestead() -> Self {
+    pub fn new_homestead_activated() -> Self {
         Self { homestead: 0, ..Self::new_ethereum() }
     }
 
     /// New tangerine enabled spec
-    pub fn new_test_tangerine_whistle() -> Self {
-        Self { tangerine_whistle: 0, ..Self::new_test_homestead() }
+    pub fn new_tangerine_whistle_activated() -> Self {
+        Self { tangerine_whistle: 0, ..Self::new_homestead_activated() }
     }
 
     /// New spurious_dragon enabled spec
-    pub fn new_test_spurious_dragon() -> Self {
-        Self { spurious_dragon: 0, ..Self::new_test_tangerine_whistle() }
+    pub fn new_spurious_dragon_activated() -> Self {
+        Self { spurious_dragon: 0, ..Self::new_tangerine_whistle_activated() }
     }
 
     /// New byzantium enabled spec
-    pub fn new_test_byzantium() -> Self {
-        Self { byzantium: 0, ..Self::new_test_spurious_dragon() }
+    pub fn new_byzantium_activated() -> Self {
+        Self { byzantium: 0, ..Self::new_spurious_dragon_activated() }
     }
 
     /// New petersburg enabled spec
-    pub fn new_test_petersburg() -> Self {
-        Self { petersburg: 0, ..Self::new_test_byzantium() }
+    pub fn new_petersburg_activated() -> Self {
+        Self { petersburg: 0, ..Self::new_byzantium_activated() }
     }
 
     /// New istanbul enabled spec
-    pub fn new_test_istanbul() -> Self {
-        Self { istanbul: 0, ..Self::new_test_petersburg() }
+    pub fn new_istanbul_activated() -> Self {
+        Self { istanbul: 0, ..Self::new_petersburg_activated() }
     }
 
     /// New berlin enabled spec
-    pub fn new_test_berlin() -> Self {
-        Self { berlin: 0, ..Self::new_test_istanbul() }
+    pub fn new_berlin_activated() -> Self {
+        Self { berlin: 0, ..Self::new_istanbul_activated() }
     }
 
     /// New london enabled spec
-    pub fn new_test_london() -> Self {
-        Self { london: 0, ..Self::new_test_berlin() }
+    pub fn new_london_activated() -> Self {
+        Self { london: 0, ..Self::new_berlin_activated() }
     }
 
     /// New paris enabled spec
-    pub fn new_test_paris() -> Self {
-        Self { paris: 0, ..Self::new_test_london() }
+    pub fn new_paris_activated() -> Self {
+        Self { paris: 0, ..Self::new_london_activated() }
     }
 
     /// return revm_spec from spec configuration.

crates/executor/src/executor.rs

@@ -1,4 +1,5 @@
 use crate::{
+    config::{WEI_2ETH, WEI_3ETH, WEI_5ETH},
     revm_wrap::{self, to_reth_acc, SubState},
     Config,
 };
@@ -108,7 +109,8 @@ pub struct AccountChangeSet {
 pub struct ExecutionResult {
     /// Transaction changeest contraining [Receipt], changed [Accounts][Account] and Storages.
     pub changeset: Vec<TransactionChangeSet>,
-    /// Block reward if present. It represent
+    /// Block reward if present. It represent changeset for block reward slot in
+    /// [tables::AccountChangeSet] .
     pub block_reward: Option<BTreeMap<Address, AccountInfoChangeSet>>,
 }
 
@@ -368,17 +370,20 @@ pub fn execute<DB: StateProvider>(
         return Err(Error::BlockGasUsed { got: cumulative_gas_used, expected: header.gas_used })
     }
 
-    // it is okay to unwrap the db. It is set at the start of the function.
-    let beneficiary =
-        evm.db.unwrap().basic(header.beneficiary).map_err(|_| Error::ProviderError)?;
+    // it is okay to unwrap the db.
+    let beneficiary = evm
+        .db
+        .expect("It is set at the start of the function")
+        .basic(header.beneficiary)
+        .map_err(|_| Error::ProviderError)?;
 
     // NOTE: Related to Ethereum reward change, for other network this is probably going to be moved
     // to config.
     let block_reward = match header.number {
         n if n >= config.spec_upgrades.paris => None,
-        n if n >= config.spec_upgrades.petersburg => Some(0x1bc16d674ec80000u128),
-        n if n >= config.spec_upgrades.byzantium => Some(0x29a2241af62c0000u128),
-        _ => Some(0x4563918244f40000u128),
+        n if n >= config.spec_upgrades.petersburg => Some(WEI_2ETH),
+        n if n >= config.spec_upgrades.byzantium => Some(WEI_3ETH),
+        _ => Some(WEI_5ETH),
     }
     .map(|reward| {
         // add block reward to beneficiary/miner
@@ -405,10 +410,17 @@ pub fn execute<DB: StateProvider>(
 #[cfg(test)]
 mod tests {
 
-    use std::collections::HashMap;
+    use std::{collections::HashMap, sync::Arc};
 
     use crate::{config::SpecUpgrades, revm_wrap::State};
-    use reth_interfaces::provider::{AccountProvider, StateProvider};
+    use reth_db::{
+        kv::{test_utils, Env, EnvKind},
+        mdbx::WriteMap,
+    };
+    use reth_interfaces::{
+        db::{Database, DbTx},
+        provider::{AccountProvider, StateProvider},
+    };
     use reth_primitives::{
         hex_literal::hex, keccak256, Account, Address, BlockLocked, Bytes, StorageKey, H160, H256,
         U256,
@@ -499,7 +511,7 @@ mod tests {
 
         let mut config = Config::new_ethereum();
         // make it berlin fork
-        config.spec_upgrades = SpecUpgrades::new_test_berlin();
+        config.spec_upgrades = SpecUpgrades::new_berlin_activated();
 
         let mut db = SubState::new(State::new(db));
         let transactions: Vec<TransactionSignedEcRecovered> =
@@ -566,4 +578,42 @@ mod tests {
             "Storage change from 0 to 2 on slot 1"
         );
     }
+
+    #[test]
+    fn apply_account_info_changeset() {
+        let db: Arc<Env<WriteMap>> = test_utils::create_test_db(EnvKind::RW);
+        let address = H160::zero();
+        let tx_num = 0;
+        let acc1 = Account { balance: 1.into(), nonce: 2, bytecode_hash: Some(H256::zero()) };
+        let acc2 = Account { balance: 3.into(), nonce: 4, bytecode_hash: Some(H256::zero()) };
+
+        let tx = db.tx_mut().unwrap();
+
+        // check Changed changeset
+        AccountInfoChangeSet::Changed { new: acc1, old: acc2 }
+            .apply_to_db(address, tx_num, &tx)
+            .unwrap();
+        assert_eq!(
+            tx.get::<tables::AccountChangeSet>(tx_num),
+            Ok(Some(AccountBeforeTx { address, info: Some(acc2) }))
+        );
+        assert_eq!(tx.get::<tables::PlainAccountState>(address), Ok(Some(acc1)));
+
+        AccountInfoChangeSet::Created { new: acc1 }.apply_to_db(address, tx_num, &tx).unwrap();
+        assert_eq!(
+            tx.get::<tables::AccountChangeSet>(tx_num),
+            Ok(Some(AccountBeforeTx { address, info: None }))
+        );
+        assert_eq!(tx.get::<tables::PlainAccountState>(address), Ok(Some(acc1)));
+
+        // delete old value, as it is dupsorted
+        tx.delete::<tables::AccountChangeSet>(tx_num, None).unwrap();
+
+        AccountInfoChangeSet::Destroyed { old: acc2 }.apply_to_db(address, tx_num, &tx).unwrap();
+        assert_eq!(tx.get::<tables::PlainAccountState>(address), Ok(None));
+        assert_eq!(
+            tx.get::<tables::AccountChangeSet>(tx_num),
+            Ok(Some(AccountBeforeTx { address, info: Some(acc2) }))
+        );
+    }
 }

crates/interfaces/src/consensus.rs

@@ -30,7 +30,7 @@ pub trait Consensus: Send + Sync {
     /// This flag is needed as reth change set is indexed of transaction granularity
     /// (change set is indexed per transaction) we are introducing one additional index for block
     /// reward This in essence would introduce gaps in [Transaction] table
-    /// More on it [here](https://github.com/foundry-rs/reth/issues/237)
+    /// More on it [here](https://github.com/paradigmxyz/reth/issues/237)
     fn has_block_reward(&self, block_num: BlockNumber) -> bool;
 }
 

crates/interfaces/src/provider/block.rs

@@ -154,7 +154,7 @@ pub fn insert_canonical_block<'a, TX: DbTxMut<'a> + DbTx<'a>>(
         for eth_tx in block.body.iter() {
             let rec_tx = eth_tx.clone().into_ecrecovered().unwrap();
             tx.put::<tables::TxSenders>(tx_number, rec_tx.signer())?;
-            tx.put::<tables::Transactions>(tx_number, rec_tx.as_ref().clone())?;
+            tx.put::<tables::Transactions>(tx_number, rec_tx.into())?;
             tx_number += 1;
         }
         tx.put::<tables::CumulativeTxCount>(

crates/primitives/src/transaction/mod.rs

@@ -539,6 +539,12 @@ pub struct TransactionSigned {
     pub transaction: Transaction,
 }
 
+impl From<TransactionSignedEcRecovered> for TransactionSigned {
+    fn from(recovered: TransactionSignedEcRecovered) -> Self {
+        recovered.signed_transaction
+    }
+}
+
 impl Encodable for TransactionSigned {
     fn encode(&self, out: &mut dyn bytes::BufMut) {
         self.encode_inner(out, true);

crates/stages/src/stages/execution.rs

@@ -157,7 +157,7 @@ impl<DB: Database> Stage<DB> for ExecutionStage {
                     .map(|(_, cumulative_tx_count)| {
                         let ret = if self.config.spec_upgrades.has_block_reward(ch_index.number()) {
                             // if there is block reward, cumulative tx count needs to remove block
-                            // reward index. It is okay ty subtract it, as
+                            // reward index. It is okay to subtract it, as
                             // block reward index is calculated in the block stage.
                             (last_tx_index, cumulative_tx_count - 1, Some(cumulative_tx_count - 1))
                         } else {
@@ -288,6 +288,7 @@ impl<DB: Database> Stage<DB> for ExecutionStage {
             }
 
             // If there is block reward we will add account changeset to db
+            // TODO add apply_block_reward_changeset to db tx fn which maybe takes an option.
             if let Some(block_reward_changeset) = results.block_reward {
                 // we are sure that block reward index is present.
                 let block_reward_index = block_reward_index.unwrap();
@@ -369,6 +370,7 @@ impl<DB: Database> Stage<DB> for ExecutionStage {
 
         // revert all changes to PlainState
         for (_, changeset) in account_changeset_batch.into_iter().rev() {
+            // TODO refactor in db fn called tx.aplly_account_changeset
             if let Some(account_info) = changeset.info {
                 db_tx.put::<tables::PlainAccountState>(changeset.address, account_info)?;
             } else {
@@ -470,7 +472,7 @@ mod tests {
 
         // execute
         let mut execution_stage = ExecutionStage::default();
-        execution_stage.config.spec_upgrades = SpecUpgrades::new_test_berlin();
+        execution_stage.config.spec_upgrades = SpecUpgrades::new_berlin_activated();
         let output = execution_stage.execute(&mut db, input).await.unwrap();
         db.commit().unwrap();
         assert_eq!(output, ExecOutput { stage_progress: 1, done: true, reached_tip: true });
@@ -550,7 +552,7 @@ mod tests {
         // execute
 
         let mut execution_stage = ExecutionStage::default();
-        execution_stage.config.spec_upgrades = SpecUpgrades::new_test_berlin();
+        execution_stage.config.spec_upgrades = SpecUpgrades::new_berlin_activated();
         let _ = execution_stage.execute(&mut db, input).await.unwrap();
         db.commit().unwrap();