From 4fb3626546d68c79036ed2c5a8a26f81b37cacc0 Mon Sep 17 00:00:00 2001 From: rakita Date: Thu, 8 Dec 2022 11:16:01 +0100 Subject: [PATCH] chore: Last PR cleanup, nits (#353) * chore: cleanup, tests and nits * fmt --- Cargo.lock | 1 + crates/executor/Cargo.toml | 4 ++ crates/executor/src/config.rs | 41 ++++++++------ crates/executor/src/executor.rs | 70 ++++++++++++++++++++---- crates/interfaces/src/consensus.rs | 2 +- crates/interfaces/src/provider/block.rs | 2 +- crates/primitives/src/transaction/mod.rs | 6 ++ crates/stages/src/stages/execution.rs | 8 ++- 8 files changed, 102 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca7b8dfa01..f40477d660 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3214,6 +3214,7 @@ dependencies = [ "hashbrown 0.13.1", "plain_hasher", "reth-consensus", + "reth-db", "reth-interfaces", "reth-primitives", "reth-rlp", diff --git a/crates/executor/Cargo.toml b/crates/executor/Cargo.toml index a2882b3ced..c2009cc53a 100644 --- a/crates/executor/Cargo.toml +++ b/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"]} \ No newline at end of file diff --git a/crates/executor/src/config.rs b/crates/executor/src/config.rs index cfc30772aa..3e4724100b 100644 --- a/crates/executor/src/config.rs +++ b/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. diff --git a/crates/executor/src/executor.rs b/crates/executor/src/executor.rs index 5a590cecc1..af209cf82e 100644 --- a/crates/executor/src/executor.rs +++ b/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, - /// Block reward if present. It represent + /// Block reward if present. It represent changeset for block reward slot in + /// [tables::AccountChangeSet] . pub block_reward: Option>, } @@ -368,17 +370,20 @@ pub fn execute( 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( #[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 = @@ -566,4 +578,42 @@ mod tests { "Storage change from 0 to 2 on slot 1" ); } + + #[test] + fn apply_account_info_changeset() { + let db: Arc> = 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::(tx_num), + Ok(Some(AccountBeforeTx { address, info: Some(acc2) })) + ); + assert_eq!(tx.get::(address), Ok(Some(acc1))); + + AccountInfoChangeSet::Created { new: acc1 }.apply_to_db(address, tx_num, &tx).unwrap(); + assert_eq!( + tx.get::(tx_num), + Ok(Some(AccountBeforeTx { address, info: None })) + ); + assert_eq!(tx.get::(address), Ok(Some(acc1))); + + // delete old value, as it is dupsorted + tx.delete::(tx_num, None).unwrap(); + + AccountInfoChangeSet::Destroyed { old: acc2 }.apply_to_db(address, tx_num, &tx).unwrap(); + assert_eq!(tx.get::(address), Ok(None)); + assert_eq!( + tx.get::(tx_num), + Ok(Some(AccountBeforeTx { address, info: Some(acc2) })) + ); + } } diff --git a/crates/interfaces/src/consensus.rs b/crates/interfaces/src/consensus.rs index 967d35d1c2..fc69731808 100644 --- a/crates/interfaces/src/consensus.rs +++ b/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; } diff --git a/crates/interfaces/src/provider/block.rs b/crates/interfaces/src/provider/block.rs index 1d90209c14..a2a23f561c 100644 --- a/crates/interfaces/src/provider/block.rs +++ b/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::(tx_number, rec_tx.signer())?; - tx.put::(tx_number, rec_tx.as_ref().clone())?; + tx.put::(tx_number, rec_tx.into())?; tx_number += 1; } tx.put::( diff --git a/crates/primitives/src/transaction/mod.rs b/crates/primitives/src/transaction/mod.rs index a28d95de4b..b220c2e10c 100644 --- a/crates/primitives/src/transaction/mod.rs +++ b/crates/primitives/src/transaction/mod.rs @@ -539,6 +539,12 @@ pub struct TransactionSigned { pub transaction: Transaction, } +impl From 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); diff --git a/crates/stages/src/stages/execution.rs b/crates/stages/src/stages/execution.rs index f6eb24fa40..5bd8d120d0 100644 --- a/crates/stages/src/stages/execution.rs +++ b/crates/stages/src/stages/execution.rs @@ -157,7 +157,7 @@ impl Stage 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 Stage 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 Stage 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::(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();