Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(trin-execution): execute multiple blocks in memory + able to execute to merge #1337

Merged
merged 11 commits into from
Sep 8, 2024
Merged

feat(trin-execution): execute multiple blocks in memory + able to execute to merge #1337

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b615207
feat(trin-execution): execute multiple blocks in memory + execute to …
KolbyML Jul 3, 2024
d1789a4
fix: some concerns
KolbyML Sep 2, 2024
e775929
fix: fix pr concerns
KolbyML Sep 2, 2024
0e05cef
fix: pr concerns
KolbyML Sep 2, 2024
d65163b
fix: pr concerns
KolbyML Sep 5, 2024
7892d5a
fix: resolve some more PR concerns
KolbyML Sep 5, 2024
6517bad
refactor: clean things up so they are more concise
KolbyML Sep 6, 2024
c9f10fe
fix: resolve PR concerns
KolbyML Sep 6, 2024
fe96d57
fix: add comment to issue
KolbyML Sep 7, 2024
da83d48
fix: resolve PR concerns
KolbyML Sep 7, 2024
b792d4b
fix: pr concerns
KolbyML Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions portal-bridge/src/bridge/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl StateBridge {
cache_contract_storage_changes: true,
block_to_trace: BlockToTrace::None,
};
let mut state =
let mut trin_execution =
TrinExecution::new(Some(temp_directory.path().to_path_buf()), state_config).await?;
for block_number in 0..=last_block {
info!("Gossipping state for block at height: {block_number}");
Expand All @@ -117,9 +117,9 @@ impl StateBridge {
let RootWithTrieDiff {
root: root_hash,
trie_diff: changed_nodes,
} = state.process_block(block_number).await?;
} = trin_execution.process_next_block().await?;

let block = state
let block = trin_execution
.era_manager
.lock()
.await
Expand Down Expand Up @@ -160,7 +160,7 @@ impl StateBridge {
// if the code_hash is empty then then don't try to gossip the contract bytecode
if account.code_hash != keccak256([]) {
// gossip contract bytecode
let code = state.database.code_by_hash(account.code_hash)?;
let code = trin_execution.database.code_by_hash(account.code_hash)?;
self.gossip_contract_bytecode(
address_hash,
&account_proof,
Expand All @@ -172,7 +172,8 @@ impl StateBridge {
}

// gossip contract storage
let storage_changed_nodes = state.database.get_storage_trie_diff(address_hash);
let storage_changed_nodes =
trin_execution.database.get_storage_trie_diff(address_hash);

let storage_walk_diff =
TrieWalker::new(account.storage_root, storage_changed_nodes);
Expand All @@ -191,7 +192,7 @@ impl StateBridge {

// flush the database cache
// This is used for gossiping storage trie diffs
state.database.storage_cache.clear();
trin_execution.database.storage_cache.clear();
}
Ok(())
}
Expand Down
141 changes: 96 additions & 45 deletions trin-execution/src/evm/block_executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
use std::{
collections::HashMap,
fs::{self, File},
path::PathBuf,
io::BufReader,
path::{Path, PathBuf},
};

use anyhow::ensure;
use anyhow::{bail, ensure};
use eth_trie::{RootWithTrieDiff, Trie};
use ethportal_api::types::execution::transaction::Transaction;
use ethportal_api::{
types::{
execution::transaction::Transaction,
state_trie::account_state::AccountState as AccountStateInfo,
},
Header,
};
use revm::{
db::{states::bundle_state::BundleRetention, State},
inspectors::TracerEip3155,
DatabaseCommit, Evm,
};
use revm_primitives::{keccak256, Account, Address, Env, ResultAndState, SpecId, B256, U256};
use revm_primitives::{keccak256, Address, Env, ResultAndState, SpecId, B256, U256};
use serde::{Deserialize, Serialize};
use tracing::info;

use crate::{
Expand All @@ -24,22 +32,36 @@ use crate::{
TRANSACTION_PROCESSING_TIMES,
},
spec_id::{get_spec_block_number, get_spec_id},
storage::evm_db::EvmDB,
storage::{account::Account as RocksAccount, evm_db::EvmDB},
transaction::TxEnvModifier,
types::block_to_trace::BlockToTrace,
};

use super::blocking::execute_transaction_with_external_context;

const BLOCKHASH_SERVE_WINDOW: u64 = 256;
const GENESIS_STATE_FILE: &str = "trin-execution/resources/genesis/mainnet.json";
const TEST_GENESIS_STATE_FILE: &str = "resources/genesis/mainnet.json";

#[derive(Debug, Serialize, Deserialize)]
struct AllocBalance {
balance: U256,
}

#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
struct GenesisConfig {
alloc: HashMap<Address, AllocBalance>,
state_root: B256,
}

/// BlockExecutor is a struct that is responsible for executing blocks or a block in memory.
/// The use case is
/// - initialize the BlockExecutor with a database
/// - execute blocks
/// - commit the changes and retrieve the result
pub struct BlockExecutor<'a> {
pub evm: Evm<'a, (), State<EvmDB>>,
evm: Evm<'a, (), State<EvmDB>>,
cumulative_gas_used: u64,
block_to_trace: BlockToTrace,
node_data_directory: PathBuf,
Expand All @@ -65,44 +87,42 @@ impl<'a> BlockExecutor<'a> {
}
}

fn set_evm_environment_from_block(&mut self, block: &ProcessedBlock) {
fn set_evm_environment_from_block(&mut self, header: &Header) {
let timer: prometheus_exporter::prometheus::HistogramTimer =
start_timer_vec(&BLOCK_PROCESSING_TIMES, &["initialize_evm"]);
if get_spec_id(block.header.number).is_enabled_in(SpecId::SPURIOUS_DRAGON) {
if get_spec_id(header.number).is_enabled_in(SpecId::SPURIOUS_DRAGON) {
self.evm.db_mut().set_state_clear_flag(true);
} else {
self.evm.db_mut().set_state_clear_flag(false);
};

// initialize evm environment
let mut env = Env::default();
env.block.number = U256::from(block.header.number);
env.block.coinbase = block.header.author;
env.block.timestamp = U256::from(block.header.timestamp);
if get_spec_id(block.header.number).is_enabled_in(SpecId::MERGE) {
env.block.number = U256::from(header.number);
env.block.coinbase = header.author;
env.block.timestamp = U256::from(header.timestamp);
if get_spec_id(header.number).is_enabled_in(SpecId::MERGE) {
env.block.difficulty = U256::ZERO;
env.block.prevrandao = block.header.mix_hash;
env.block.prevrandao = header.mix_hash;
} else {
env.block.difficulty = block.header.difficulty;
env.block.difficulty = header.difficulty;
env.block.prevrandao = None;
}
env.block.basefee = block.header.base_fee_per_gas.unwrap_or_default();
env.block.gas_limit = block.header.gas_limit;
env.block.basefee = header.base_fee_per_gas.unwrap_or_default();
env.block.gas_limit = header.gas_limit;

// EIP-4844 excess blob gas of this block, introduced in Cancun
if let Some(excess_blob_gas) = block.header.excess_blob_gas {
if let Some(excess_blob_gas) = header.excess_blob_gas {
env.block
.set_blob_excess_gas_and_price(u64::from_be_bytes(excess_blob_gas.to_be_bytes()));
}

self.evm.context.evm.env = Box::new(env);
self.evm
.handler
.modify_spec_id(get_spec_id(block.header.number));
self.evm.handler.modify_spec_id(get_spec_id(header.number));
stop_timer(timer);
}

pub fn set_transaction_evm_context(&mut self, tx: &TransactionsWithSender) {
fn set_transaction_evm_context(&mut self, tx: &TransactionsWithSender) {
let timer = start_timer_vec(&TRANSACTION_PROCESSING_TIMES, &["modify_tx"]);

let block_number = self.block_number();
Expand All @@ -118,24 +138,14 @@ impl<'a> BlockExecutor<'a> {
stop_timer(timer);
}

pub fn commit(&mut self, evm_state: HashMap<Address, Account>) {
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["commit_state"]);
self.evm.db_mut().commit(evm_state);
stop_timer(timer);
}

pub fn transact(&mut self) -> anyhow::Result<ResultAndState> {
Ok(self.evm.transact()?)
}

pub fn increment_balances(
fn increment_balances(
&mut self,
balances: impl IntoIterator<Item = (Address, u128)>,
) -> anyhow::Result<()> {
Ok(self.evm.db_mut().increment_balances(balances)?)
}

pub fn process_dao_fork(&mut self) -> anyhow::Result<()> {
fn process_dao_fork(&mut self) -> anyhow::Result<()> {
// drain balances from DAO hardfork accounts
let drained_balances = self.evm.db_mut().drain_balances(DAO_HARDKFORK_ACCOUNTS)?;
let drained_balance_sum: u128 = drained_balances.iter().sum();
Expand All @@ -146,7 +156,7 @@ impl<'a> BlockExecutor<'a> {
Ok(())
}

pub fn commit_bundle(&mut self) -> anyhow::Result<RootWithTrieDiff> {
pub fn commit_bundle(mut self) -> anyhow::Result<RootWithTrieDiff> {
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["commit_bundle"]);
self.evm
.db_mut()
Expand Down Expand Up @@ -175,11 +185,50 @@ impl<'a> BlockExecutor<'a> {
})
}

fn process_genesis(&mut self, header: &Header) -> anyhow::Result<()> {
ensure!(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: since this is internal function, I don't think it's necessary to do this check here (since the check is already done right before the call). Up to you.

header.number == 0,
"Trying to initialize genesis but received block {}",
header.number,
);
let genesis_file = if Path::new(GENESIS_STATE_FILE).is_file() {
File::open(GENESIS_STATE_FILE)?
} else if Path::new(TEST_GENESIS_STATE_FILE).is_file() {
File::open(TEST_GENESIS_STATE_FILE)?
} else {
bail!("Genesis file not found")
};
let genesis: GenesisConfig = serde_json::from_reader(BufReader::new(genesis_file))?;

for (address, alloc_balance) in genesis.alloc {
let address_hash = keccak256(address);
let mut account = RocksAccount::default();
account.balance += alloc_balance.balance;
self.evm.db().database.trie.lock().insert(
address_hash.as_ref(),
&alloy_rlp::encode(AccountStateInfo::from(&account)),
)?;
self.evm
.db()
.database
.db
.put(address_hash, alloy_rlp::encode(account))?;
}

Ok(())
}

pub fn execute_block(&mut self, block: &ProcessedBlock) -> anyhow::Result<()> {
info!("State EVM processing block {}", block.header.number);

let execute_block_timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["execute_block"]);
self.set_evm_environment_from_block(block);

if block.header.number == 0 {
self.process_genesis(&block.header)?;
return Ok(());
}

self.set_evm_environment_from_block(&block.header);

// execute transactions
let cumulative_transaction_timer =
Expand All @@ -189,7 +238,9 @@ impl<'a> BlockExecutor<'a> {
let transaction_timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["transaction"]);
let evm_result = self.execute_transaction(transaction)?;
block_gas_used += evm_result.result.gas_used();
self.commit(evm_result.state);
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["commit_state"]);
self.evm.db_mut().commit(evm_result.state);
stop_timer(timer);
stop_timer(transaction_timer);
}
stop_timer(cumulative_transaction_timer);
Expand All @@ -203,15 +254,15 @@ impl<'a> BlockExecutor<'a> {
self.cumulative_gas_used += block_gas_used;

// update beneficiary
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["update_beneficiary"]);
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["beneficiary_timer"]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I meant the variable name, not the timer name :)

let _ = self.increment_balances(get_block_reward(block));

// check if dao fork, if it is drain accounts and transfer it to beneficiary
if block.header.number == get_spec_block_number(SpecId::DAO_FORK) {
self.process_dao_fork()?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't really know, but I want to make sure we are doing it correctly. Should this be done at the start or at the end of the DAO_FORK block, or does it even make a difference?

My feeling is that it doesn't make a difference, but that logically it should be done at the start of the block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done after the block is executed.

  • block rewards
  • beacon withdrawals
  • dao fork draining

are all post block executed state changes.

The order you do this does matter, as balance differences can affect block execution

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just talking about DAO_FORK changes (I know that rewards and withdrawals must go at the end).

Since we are talking about one specific block, I can imagine that order in that specific block might not matter (regardless of which one is consider correct).

My feeling is that all fork related changes (both to state and evm execution) happen at the start of the block (before transactions are executed), therefore my question.
For example, what would happen if one of the malicious DAO wallets created transaction to transfer eth to "clean" wallet?! Current implementation wouldn't prevent that, but if this was executed at the start of the block, it would.

Ultimately, it doesn't matter (if we end up with the correct state), but logically it makes sense that dao_fork logic happens at the start of the block (at least for me).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, what would happen if one of the malicious DAO wallets created transaction to transfer eth to "clean" wallet?! Current implementation wouldn't prevent that, but if this was executed at the start of the block, it would.

The dao fork happened 8 years ago. The code to drain all the accounts was most likely a lot more complex then this to track if a transfer was to happen from the origin address.

We don't have to prevent anything so I don't really understand where this hypothetical is coming from. After the fork happened we would no longer need the complex code to properly prevent a transfer, we would only need to drain the accounts and transfer the funds at the right block.

I don't think discussing an 8 year old hypothetical is productive. This time should be spent working on shipping latest state data on the State network

My feeling is that all fork related changes (both to state and evm execution) happen at the start of the block (before transactions are executed), therefore my question.

My feeling is all the balance_increment code should be together, because that is what this is.

Copy link
Member Author

@KolbyML KolbyML Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is that all fork related changes (both to state and evm execution) happen at the start of the block (before transactions are executed)

The

  • change in block reward are fork related changes (certain forks changed what the block reward number was)
  • withdrawals are fork related changes (these were activated on shanghai)
  • the dao fork draining and moving the funds are fork related change

So I do not see why there is a distinction for the dao fork, when all 3 of those are relate to forks.

The commonality is they all use increment_balances()

Copy link
Collaborator

@morph-dev morph-dev Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My meaning of "fork related changes" are the changes either to the evm (or evm state) that happen only once, at the fork block. Some forks enable/disable/modify certain opt-codes, modify gas calculations, allow certain behaviors (withdrawals), etc. In my mental model, these changes happen at the start of the fork block (and only then).

Dao fork is not much different in that sense, at least to me (because it happens only once).

The other two (block rewards, withdrawals) happen at the end of the block, which is correct because it's according to spec. They are fork related because certain forks enable/modify them, but once they are active, they happen on every block afterwards. For example, we can say that withdrawals are enabled at the start of the shanghai fork, but updating balances due to withdrawal happen at the end of the block.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I agree with you that there is no point in discussing this further (as it's just one block that happened 8 years ago).

Feel free to leave it as it is (I just wanted to clarify my mental model).

}

self.manage_block_hash_serve_window(block)?;
self.manage_block_hash_serve_window(&block.header)?;

stop_timer(timer);
stop_timer(execute_block_timer);
Expand Down Expand Up @@ -244,27 +295,27 @@ impl<'a> BlockExecutor<'a> {
&mut self.evm.context.evm.inner.db,
)?
} else {
self.transact()?
self.evm.transact()?
};
stop_timer(timer);

Ok(result)
}

/// insert block hash into database and remove old one
fn manage_block_hash_serve_window(&self, block: &ProcessedBlock) -> anyhow::Result<()> {
fn manage_block_hash_serve_window(&self, header: &Header) -> anyhow::Result<()> {
let timer = start_timer_vec(&BLOCK_PROCESSING_TIMES, &["insert_blockhash"]);
self.evm.db().database.db.put(
keccak256(B256::from(U256::from(block.header.number))),
block.header.hash(),
keccak256(B256::from(U256::from(header.number))),
header.hash(),
)?;
if block.header.number >= BLOCKHASH_SERVE_WINDOW {
if header.number >= BLOCKHASH_SERVE_WINDOW {
self.evm
.db()
.database
.db
.delete(keccak256(B256::from(U256::from(
block.header.number - BLOCKHASH_SERVE_WINDOW,
header.number - BLOCKHASH_SERVE_WINDOW,
))))?;
}
stop_timer(timer);
Expand All @@ -279,7 +330,7 @@ impl<'a> BlockExecutor<'a> {
self.evm.db().bundle_size_hint()
}

pub fn block_number(&self) -> u64 {
fn block_number(&self) -> u64 {
self.evm.context.evm.env.block.number.to::<u64>()
}
}
Loading
Loading