refactor(l1): transform crate blockchain into struct #2047

JereSalo · 2025-02-21T17:26:50Z

Motivation

Description

Move methods like add_block to Blockchain struct.
I use Blockchain::default_with_store(store) in the case of tests. Default VM is REVM. The Store that I use for tests is the InMemory Engine Type.
Blockchain has attributes: VM and Store

Note: I don't know if I should test these changes in some other way rather than just running the tests that we have.

Closes #2018

github-actions · 2025-02-21T17:29:29Z

| File                                                                    | Lines | Diff |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ef_tests/blockchain/test_runner.rs  | 138   | +1   |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/ethrex.rs                    | 388   | -42  |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex_l2/src/commands/wallet.rs    | 617   | -1   |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/blockchain/blockchain.rs         | 280   | +68  |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/blockchain/payload.rs            | 569   | +8   |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/blockchain/smoke_test.rs         | 205   | +24  |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/l2/utils/prover/save_state.rs    | 438   | +1   |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/l2/utils/test_data_io.rs         | 89    | +1   |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/p2p/sync.rs           | 433   | +8   |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/networking/rpc/engine/payload.rs | 567   | +16  |
+-------------------------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/crates/vm/vm.rs                         | 129   | -6   |
+-------------------------------------------------------------------------+-------+------+

Total lines added: +127
Total lines removed: 49
Total lines changed: 176

mpaulucci · 2025-02-24T11:15:59Z

cmd/ef_tests/blockchain/test_runner.rs


        // Attempt to add the block as the head of the chain
-        let chain_result = add_block(block, &store);
+        let chain_result = Blockchain::default().add_block(block, &store);


The idea is that the blockchain will be stateful,

let blockchain = Blockchain:new(evm, store, ... etc);
for block in blocks {
blockchain.add_block(block)
}

Otherwise a blockchain struct wouldn't make much sense

Right! I haven't thought about that.

cmd/ethrex/ethrex.rs

mpaulucci · 2025-02-24T11:19:28Z

crates/blockchain/blockchain.rs


-    Ok(())
+impl Blockchain {
+    pub fn new(evm: EVM) -> Self {


can we add store?

can be in a follow up.

Yes! I will try to implement it and see if I can make it work, so that we don't need to send store as parameter

Implemented and merged.

mpaulucci · 2025-02-24T11:20:03Z

crates/vm/vm.rs

 pub use execution_result::*;
 pub use revm::primitives::{Address as RevmAddress, SpecId, U256 as RevmU256};

-use std::sync::OnceLock;


mpaulucci · 2025-02-24T11:42:13Z

crates/blockchain/payload.rs

-    finalize_payload(&mut context)?;
-    Ok((context.blobs_bundle, context.block_value))
-}
+impl Blockchain {


why did we need to move this methods to Blockchain in the first iteration?

I moved them to Blockchain because these methods were using get_evm_backend_or_default() and I wanted them to use the Blockchain EVM that we previously set.

mpaulucci · 2025-02-24T11:42:58Z

crates/l2/utils/test_data_io.rs

    store.add_initial_state(genesis)?;
    for block in chain {
-        add_block(&block, &store)?;
+        Blockchain::default().add_block(&block, &store)?;


create blockchain above the adding of blocks.

Although it has changed a little bit I fixed that here. The difference with the current code is that now we also have Store inside of Blockchain and we don't need to send it as parameter in some Blockchain methods.

mpaulucci · 2025-02-24T13:31:56Z

cmd/ef_tests/blockchain/test_runner.rs

        let block: &CoreBlock = &block_fixture.block().unwrap().clone().into();
        let hash = block.hash();

+        let blockchain = Blockchain::default();


Once you move store, you won't be able to use default. Maybe the default should be on the evm.

Yeah, I was thinking on using something like default_with_store(store) so that we use the default VM and the provided store. Is there a better alternative to that?

mpaulucci · 2025-02-24T13:33:50Z

crates/blockchain/blockchain.rs

+                    block.header.number, hash, error
+                );
+            }
+            if store


Add a TODO comment stating that the forkchoice update should not be part of this function.

**Motivation**  **Description**   Closes #issue_number

mpaulucci

🎉

crates/blockchain/blockchain.rs

ilitteri · 2025-02-24T19:21:34Z

L2 Integration Test will probably fail until #1705 is merged

rodrigo-o

LGTM, just a small comment for something that we might need to check later.

rodrigo-o · 2025-02-24T19:35:50Z

crates/networking/rpc/engine/payload.rs

+    let add_block_result = {
+        let lock = context.syncer.try_lock();
+        if let Ok(syncer) = lock {
+            syncer.blockchain.add_block(block)
+        } else {
+            Err(ChainError::Custom(
+                "Error when trying to lock syncer".to_string(),
+            ))
+        }
+    };


I'm not sure if the new locks to the syncer might have some impact or if we want to maintain the state of the blockchain in it, but we can check this later if needed.

I agree. I created an issue just for not forgetting about it.
#2063

Co-authored-by: Ivan Litteri <[email protected]>

**Motivation**  **Description**  - Move methods like `add_block` to `Blockchain` struct. - I use `Blockchain::default_with_store(store)` in the case of tests. Default VM is REVM. The Store that I use for tests is the `InMemory` Engine Type. - Blockchain has attributes: VM and Store Note: I don't know if I should test these changes in some other way rather than just running the tests that we have.  Closes #2018 --------- Co-authored-by: Ivan Litteri <[email protected]>

**Motivation**  **Description**  - Move methods like `add_block` to `Blockchain` struct. - I use `Blockchain::default_with_store(store)` in the case of tests. Default VM is REVM. The Store that I use for tests is the `InMemory` Engine Type. - Blockchain has attributes: VM and Store Note: I don't know if I should test these changes in some other way rather than just running the tests that we have.  Closes lambdaclass#2018 --------- Co-authored-by: Ivan Litteri <[email protected]>

start implementing Blockchain struct

15704e5

JereSalo self-assigned this Feb 21, 2025

JereSalo changed the title ~~refactor(l1): transform blockchain into singleton struct~~ refactor(l1): transform crate blockchain into singleton struct Feb 21, 2025

JereSalo added 2 commits February 21, 2025 17:10

continue with refactor, replace add_block function for method

2d54b80

make progress in refactor, removed global variable

bc9265d

JereSalo changed the title ~~refactor(l1): transform crate blockchain into singleton struct~~ refactor(l1): transform crate blockchain into struct Feb 21, 2025

JereSalo and others added 5 commits February 21, 2025 17:46

undo changes i guess

cc69d39

remove tool versions

ac4c8a8

Delete .tool-versions~

cb902ce

undo delete...

b9638ae

nits

0224d5d

mpaulucci reviewed Feb 24, 2025

View reviewed changes

cmd/ethrex/ethrex.rs Show resolved Hide resolved

mpaulucci reviewed Feb 24, 2025

View reviewed changes

JereSalo added 2 commits February 24, 2025 09:58

move import blocks to blockchain struct

e3e1813

create blockchain before executing add_block()

f551bbc

mpaulucci reviewed Feb 24, 2025

View reviewed changes

JereSalo and others added 6 commits February 24, 2025 13:03

add todo comment forkchoice update

0d2f4c1

take store out of build_payload

708bc91

merge main

3c099ae

merge main

2ed9b56

fix tiny errors

0741574

JereSalo marked this pull request as ready for review February 24, 2025 18:35

JereSalo requested a review from a team as a code owner February 24, 2025 18:35

remove comment

9917653

mpaulucci approved these changes Feb 24, 2025

View reviewed changes

ilitteri reviewed Feb 24, 2025

View reviewed changes

crates/blockchain/blockchain.rs Outdated Show resolved Hide resolved

ilitteri approved these changes Feb 24, 2025

View reviewed changes

rodrigo-o approved these changes Feb 24, 2025

View reviewed changes

Update crates/blockchain/blockchain.rs

69f46cc

Co-authored-by: Ivan Litteri <[email protected]>

JereSalo enabled auto-merge February 24, 2025 21:36

JereSalo disabled auto-merge February 24, 2025 21:37

Merge branch 'main' into l1/refactor_blockchain_vms

40b88dc

JereSalo enabled auto-merge February 24, 2025 21:45

JereSalo added this pull request to the merge queue Feb 24, 2025

Merged via the queue into main with commit 71b70b1 Feb 24, 2025
23 checks passed

JereSalo deleted the l1/refactor_blockchain_vms branch February 24, 2025 22:26

refactor(l1): transform crate blockchain into struct #2047

refactor(l1): transform crate blockchain into struct #2047

Uh oh!

Conversation

JereSalo commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpaulucci Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JereSalo Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpaulucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilitteri commented Feb 24, 2025

Uh oh!

rodrigo-o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JereSalo commented Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 21, 2025 •

edited

Loading

mpaulucci Feb 24, 2025 •

edited

Loading

JereSalo Feb 24, 2025 •

edited

Loading