Skip to content

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

Merged
mpaulucci merged 4 commits into
mainfrom
standarize-levm-evm-block-import
Apr 21, 2025
Merged

chore(l1): standarize revm/levm behaviour when importing blocks.#2452
mpaulucci merged 4 commits into
mainfrom
standarize-levm-evm-block-import

Conversation

@mpaulucci

@mpaulucci mpaulucci commented Apr 11, 2025

Copy link
Copy Markdown
Collaborator

Description

  • standarize revm/levm behaviour when importing blocks
  • Remove fork choice when importing blocks.
  • Move block importing out of blockchain module

@mpaulucci mpaulucci requested a review from a team as a code owner April 11, 2025 18:07
@github-actions

github-actions Bot commented Apr 11, 2025

Copy link
Copy Markdown

Lines of code report

Total lines added: 22
Total lines removed: 52
Total lines changed: 74

Detailed view
+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs               | 324   | +22  |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 461   | -52  |
+----------------------------------------+-------+------+

Comment thread cmd/ethrex/cli.rs

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
Copy Markdown
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

Comment thread cmd/ethrex/cli.rs
info!("Importing blocks from chain file: {path}");
utils::read_chain_file(path)
};
blockchain.import_blocks(&blocks).await;

Copy link
Copy Markdown
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
Copy Markdown
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?

@mpaulucci mpaulucci enabled auto-merge April 21, 2025 15:09
@mpaulucci mpaulucci added this pull request to the merge queue Apr 21, 2025
@mpaulucci mpaulucci removed this pull request from the merge queue due to a manual request Apr 21, 2025
@mpaulucci mpaulucci added this pull request to the merge queue Apr 21, 2025
Merged via the queue into main with commit bc9aafd Apr 21, 2025
@mpaulucci mpaulucci deleted the standarize-levm-evm-block-import branch April 21, 2025 15:37
pedrobergamini pushed a commit to pedrobergamini/ethrex that referenced this pull request Aug 24, 2025
…bdaclass#2452)

**Description**
- standarize revm/levm behaviour when importing blocks
- Remove fork choice when importing blocks.
- Move block importing out of blockchain module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants