Skip to content

chore(l1): standarize revm/levm behaviour when importing blocks. #2452

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

Merged
merged 4 commits into from
Apr 21, 2025
Merged

chore(l1): standarize revm/levm behaviour when importing blocks. #2452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5aa796b
chore(l1): Standarize revm/levm behaviour when importing blocks.
mpaulucci Apr 11, 2025
0078124
Remove import function from blockchain.
mpaulucci Apr 11, 2025
24bcb3b
Merge branch 'main' of github.com:lambdaclass/lambda_ethereum_rust in…
mpaulucci Apr 21, 2025
dd6d650
Fix linting.
mpaulucci Apr 21, 2025
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
34 changes: 31 additions & 3 deletions cmd/ethrex/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::{
};

use clap::{ArgAction, Parser as ClapParser, Subcommand as ClapSubcommand};
use ethrex_blockchain::fork_choice::apply_fork_choice;
use ethrex_p2p::{sync::SyncMode, types::Node};
use ethrex_vm::EvmEngine;
use tracing::{info, warn, Level};
Expand Down Expand Up @@ -55,7 +56,7 @@ pub struct Options {
)]
pub datadir: String,
#[arg(
long = "force",
long = "force",
help = "Force remove the database",
long_help = "Delete the database without confirmation.",
action = clap::ArgAction::SetTrue,
Expand Down Expand Up @@ -298,7 +299,7 @@ pub async fn import_blocks(path: &str, data_dir: &str, network: &str, evm: EvmEn

let store = init_store(&data_dir, network).await;

let blockchain = init_blockchain(evm, store);
let blockchain = init_blockchain(evm, store.clone());

let path_metadata = metadata(path).expect("Failed to read path");
let blocks = if path_metadata.is_dir() {
Expand All @@ -317,5 +318,32 @@ pub async fn import_blocks(path: &str, data_dir: &str, network: &str, evm: EvmEn
info!("Importing blocks from chain file: {path}");
utils::read_chain_file(path)
};
blockchain.import_blocks(&blocks).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the method instead of refactoring it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see the point of having such a function. It is ambiguous with add_block(s). What is the point of having a function that adds blocks + calls forkchoice instead of calling the two function separately?


let size = blocks.len();

for block in &blocks {
let hash = block.hash();

info!(
"Adding block {} with hash {:#x}.",
block.header.number, hash
);

if let Err(error) = blockchain.add_block(block).await {
warn!(
"Failed to add block {} with hash {:#x}: {}.",
block.header.number, hash, error
);
return;
}
}

if let Some(last_block) = blocks.last() {
let hash = last_block.hash();
if let Err(error) = apply_fork_choice(&store, hash, hash, hash).await {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

applying forkchoice should set all the blocks as canonical, you don't need to do it explicitly

warn!("Failed to apply fork choice: {}", error);
}
}

info!("Added {size} blocks to blockchain");
}
57 changes: 1 addition & 56 deletions crates/blockchain/blockchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ use std::{ops::Div, time::Instant};
use ethrex_storage::error::StoreError;
use ethrex_storage::{AccountUpdate, Store};
use ethrex_vm::{BlockExecutionResult, Evm, EvmEngine};
use fork_choice::apply_fork_choice;
use tracing::{error, info, warn};
use tracing::info;

//TODO: Implement a struct Chain or BlockChain to encapsulate
//functionality and canonical chain state and config
Expand Down Expand Up @@ -295,60 +294,6 @@ impl Blockchain {
Ok(())
}

//TODO: Forkchoice Update shouldn't be part of this function
pub async fn import_blocks(&self, blocks: &[Block]) {
let size = blocks.len();
for block in blocks {
let hash = block.hash();
info!(
"Adding block {} with hash {:#x}.",
block.header.number, hash
);
if let Err(error) = self.add_block(block).await {
warn!(
"Failed to add block {} with hash {:#x}: {}.",
block.header.number, hash, error
);
}
if self
.storage
.update_latest_block_number(block.header.number)
.await
.is_err()
{
error!("Fatal: added block {} but could not update the block number -- aborting block import", block.header.number);
break;
};
if self
.storage
.set_canonical_block(block.header.number, hash)
.await
.is_err()
{
error!(
"Fatal: added block {} but could not set it as canonical -- aborting block import",
block.header.number
);
break;
};
}
if let Some(last_block) = blocks.last() {
let hash = last_block.hash();
match self.evm_engine {
EvmEngine::LEVM => {
// We are allowing this not to unwrap so that tests can run even if block execution results in the wrong root hash with LEVM.
let _ = apply_fork_choice(&self.storage, hash, hash, hash).await;
}
EvmEngine::REVM => {
apply_fork_choice(&self.storage, hash, hash, hash)
.await
.unwrap();
}
}
}
info!("Added {size} blocks to blockchain");
}

/// Add a blob transaction and its blobs bundle to the mempool checking that the transaction is valid
#[cfg(feature = "c-kzg")]
pub async fn add_blob_transaction_to_pool(
Expand Down
Loading