feat: reth-scroll crates #33

greged93 · 2024-11-18T18:15:20Z

Overview

Introduces the scroll feature flag and Scroll Account changes. The following changes are implemented:

reth-scroll-* crates: introduces 4 Scroll related crates, which will accumulate as much changes as possible, in order to limit diff with upstream.
Account extending fields: code_size and poseidon_code_hash are added on the Ethereum account. These fields are needed for the computation of the state root.
ScrollStateProviderDatabase: a storage provider that implements the revm::Database trait and will thus be used during execution when the scroll feature flag is used. It caches all Scroll added fields for touched accounts during execution and can be used to build the ScrollBundleState from a revm::BundleState.

Notes

Some changes are marked as // TODO (scroll): .... These were added in order to make the current changes satisfy all clippy checks and will be updated in subsequent PR's related to the Account changes.
PR was rebased on 7f00db6 and only required some small fixes to the Cargo.lock file.

Builds towards #7

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

greged93 · 2024-11-19T14:30:19Z

.github/workflows/lint.yml

@@ -152,7 +152,7 @@ jobs:
        with:
          cache-on-failure: true
      - uses: taiki-e/install-action@cargo-udeps
-      - run: cargo udeps --workspace --lib --examples --tests --benches --all-features --locked
+      - run: cargo udeps --workspace --lib --examples --tests --benches --all-features --locked --exclude "reth-optimism-*" --exclude op-reth


because of the #![cfg(not(feature = "scroll"))] conditional compilation in the optimism crates, we exclude the udeps check on the optimism crates.

greged93 · 2024-11-19T14:32:19Z

crates/scroll/execution/src/context.rs

+#[cfg(not(feature = "scroll"))]
+type ExecutionContext = ();
+#[cfg(feature = "scroll")]
+type ExecutionContext = reth_scroll_primitives::ScrollPostExecutionContext;
+static DEFAULT_CONTEXT: LazyLock<ExecutionContext> = LazyLock::new(Default::default);


this allows CacheDB<DB> and StateProviderDatabase<DB> to always implement ContextFul, whatever the feature.

frisitano

Looks great, thanks! Left some minor comments inline. Once comments are answered / addressed we can merge.

frisitano · 2024-11-21T08:27:32Z

crates/optimism/payload/Cargo.toml

+]
+scroll = []


nit: new line at eof

you prefer to remove new lines at eof in manifests?

I'm indifferent but it seems to be the convention in reth from what I can tell so I think we should remain consistent.

frisitano · 2024-11-21T08:29:23Z

crates/primitives-traits/Cargo.toml

@@ -20,7 +20,9 @@ alloy-genesis.workspace = true
 alloy-primitives.workspace = true
 alloy-rlp.workspace = true

-revm-primitives = { workspace = true, features = ["serde"] }
+# revm-primitives scroll re-export
+revm-primitives = { package = "reth-scroll-revm", path = "../scroll/revm", features = ["serde"] }


nit: should we define this dependency at the workspace level?

do you mean update revm-primitives to point to reth-scroll-revm from the workspace mainfest?

I'm mean when we define revm-primitives in the workspace Cargo.toml we redefine it such that it points to reth-scroll-revm. Currently in workspace Cargo.toml we have:

revm-primitives = { version = "14.0.0", features = [ "std", ], default-features = false }

We can change it to:

revm-primitives = { package = "reth-scroll-revm", path = "../scroll/revm", features = ["std"], default-features = false }

Then in this crate we shouldn't have to change the dependency.

@frisitano I think this would work better in the follow up PR #39. The goal of the current PR was to introduce the Scroll crates with minimal propagation to the rest of the repo. Moving the dependency to workspace level would fit more 39's goal and limit the changes in this PR (which are done in 39 anyway).

Yep sounds good.

frisitano · 2024-11-21T10:09:38Z

crates/primitives/Cargo.toml

@@ -29,6 +28,9 @@ alloy-serde = { workspace = true, optional = true }
 alloy-eips = { workspace = true, features = ["serde"] }
 alloy-trie = { workspace = true, features = ["serde"] }

+# scroll
+revm-primitives = { package = "reth-scroll-revm", path = "../scroll/revm", features = ["serde"] }


Should we define this in the workspace?

frisitano · 2024-11-21T10:10:09Z

crates/primitives/Cargo.toml

@@ -67,7 +69,7 @@ reth-codecs = { workspace = true, features = ["test-utils"] }
 reth-primitives-traits = { workspace = true, features = ["arbitrary"] }
 reth-testing-utils.workspace = true
 reth-trie-common.workspace = true
-revm-primitives = { workspace = true, features = ["arbitrary"] }
+revm-primitives = { package = "reth-scroll-revm", path = "../scroll/revm", features = ["arbitrary"] }


should we define this in the workspace?

frisitano · 2024-11-21T10:28:30Z

crates/scroll/execution/src/context.rs

+impl<DB> WithContext for CacheDB<DB> {
+    type Context = ExecutionContext;
+
+    fn context(&self) -> &Self::Context {
+        &DEFAULT_CONTEXT
+    }
+}
+
+impl<DB> WithContext for StateProviderDatabase<DB> {
+    type Context = ExecutionContext;
+
+    fn context(&self) -> &Self::Context {
+        &DEFAULT_CONTEXT
+    }
+}


What are the implications of returning a DEFAULT_CONTEXT here. Is CacheDB ever used for building blocks and generating the state root?

This default implementation is only used in tests. To be sure this is never used in production code I could move it behind a test-utils feature, what do you think?

Yes I think putting it behind test-utils feature makes sense.

frisitano · 2024-11-21T10:30:00Z

crates/scroll/execution/src/context.rs

+/// Finalize the execution of the type and return the output
+pub trait FinalizeExecution<Output> {
+    /// Finalize the state and return the output.
+    fn finalize(&mut self) -> Output;
+}
+
+impl<DB: Database + ContextFul> FinalizeExecution<reth_scroll_revm::states::ScrollBundleState>
+    for State<DB>
+{
+    fn finalize(&mut self) -> reth_scroll_revm::states::ScrollBundleState {
+        let bundle = self.take_bundle();
+        (bundle, self.database.context()).into()
+    }
+}
+
+/// A type that returns additional execution context.
+pub trait ContextFul: WithContext<Context = ExecutionContext> {}
+impl<T> ContextFul for T where T: WithContext<Context = ExecutionContext> {}
+
+/// Types that can provide a context.
+#[auto_impl::auto_impl(&, &mut)]
+pub trait WithContext {
+    /// The context returned.
+    type Context;
+
+    /// Returns the context from the type.
+    fn context(&self) -> &Self::Context;
+}


Lets keep this as is for now but I'd like to see how this integrates with the BlockExecutor in the follow up PR. I wonder if we can simplify it a bit.

frisitano · 2024-11-21T10:58:39Z

crates/scroll/primitives/src/poseidon.rs

+    b256!("2098f5fb9e239eab3ceac3f27b81e481dc3124d55ffed523a839ee8446b64864");
+
+/// Poseidon code hash
+pub fn poseidon(code: &[u8]) -> B256 {


Should we rename this to hash_code? I think poseidon is a bit ambiguous and doesn't represent that it's associated with hashing code.

I think that's fair, maybe poseidon_hash_code?

That works but I would still prefer hash_code, the reason being that we already have Poseidon in the import scope use scroll_primitives::poseidon::hash_code however I'm not strongly opinionated.

frisitano · 2024-11-21T11:12:48Z

crates/scroll/primitives/Cargo.toml

+reth-codecs.workspace = true
+
+# scroll
+poseidon-bn254 = { git = "https://github.com/scroll-tech/poseidon-bn254", branch = "master", features = ["bn254"] }


I think we should create a tag or publish this to crates.io instead of using a master branch dependency. I think we should move the dependency to workspace Cargo.toml and add a TODO which states that we should change this dep to point to the tag / crate onceds it's published. ref: scroll-tech/poseidon-bn254#6

agreed, I left a comment regarding this in the deny.toml. Will move to the workspace

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

Thegaram · 2024-11-22T14:43:28Z

Cargo.toml

@@ -470,6 +478,10 @@ alloy-transport-http = { version = "0.6.4", features = [
 alloy-transport-ipc = { version = "0.6.4", default-features = false }
 alloy-transport-ws = { version = "0.6.4", default-features = false }

+# scroll
+# TODO (scroll): point to crates.io/tag once the crate is published/ a tag is created.
+poseidon-bn254 = { git = "https://github.com/scroll-tech/poseidon-bn254", branch = "master", features = ["bn254"] }


Should we use a commit for now (instead of a branch)? So that things don't break unexpectedly.

that's fine with me, I was pointing it to master because this is what is currently done on scroll-tech/revm. I can check what commit is used in scroll-tech/revm in the lock file and use that one for now.

Thegaram · 2024-11-22T14:55:01Z

crates/scroll/revm/src/lib.rs

+pub mod states;
+
+#[cfg(feature = "optimism")]
+pub use revm::primitives::OptimismFields;


Is this left here intentionally?

yes, unfortunately because the reth-scroll-revm crate is used as a re-export for revm and revm-primitives, which allows us to shadow specific types (BundleState, BundleAccount,...) with the Scroll types when the scroll feature is active. This means we also need to re-export some of the Optimism types.

If you prefer I can modify the imports in the rest of the codebase (from revm_primitives:: OptimismFields to revm::primitives::OptimismFields) but I think just having this line is easier whenever we need to merge upstream.

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

greged93 added 4 commits November 18, 2024 18:56

add all the scroll crates

cdf519f

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

Scroll Account extension

c1df905

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

add Default::default to make tests compile

2a1a0fa

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

fix rebasing

64ac891

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

greged93 force-pushed the feat/reth-scroll branch 2 times, most recently from cdd8185 to 6c120a1 Compare November 19, 2024 10:16

fix lints + feature check

a667eae

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

greged93 force-pushed the feat/reth-scroll branch from 6c120a1 to a667eae Compare November 19, 2024 10:25

greged93 added 2 commits November 19, 2024 11:44

don't compile optimism crates with scroll feature

8a08dfb

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

fix optimism lints

71cafb7

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

greged93 commented Nov 19, 2024

View reviewed changes

greged93 mentioned this pull request Nov 20, 2024

feat: scroll account #39

Open

frisitano previously approved these changes Nov 21, 2024

View reviewed changes

greged93 added 2 commits November 22, 2024 09:12

update poseidon to hash_code and import to workspace

d6156e3

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

gate WithContext implementation behind test-utils

a8e5772

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

greged93 dismissed frisitano’s stale review via a8e5772 November 22, 2024 08:14

greged93 changed the title ~~feat: Reth Scroll~~ feat: reth-scroll crates Nov 22, 2024

Thegaram reviewed Nov 22, 2024

View reviewed changes

pin poseidon-bn254

4cd293c

Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reth-scroll crates #33

feat: reth-scroll crates #33

greged93 commented Nov 18, 2024 •

edited

Loading

greged93 Nov 19, 2024

greged93 Nov 19, 2024 •

edited

Loading

frisitano left a comment

frisitano Nov 21, 2024

greged93 Nov 21, 2024

frisitano Nov 21, 2024

frisitano Nov 21, 2024

greged93 Nov 21, 2024

frisitano Nov 21, 2024

greged93 Nov 22, 2024

frisitano Nov 22, 2024

frisitano Nov 21, 2024

frisitano Nov 21, 2024

frisitano Nov 21, 2024

greged93 Nov 21, 2024

frisitano Nov 21, 2024

frisitano Nov 21, 2024

frisitano Nov 21, 2024

greged93 Nov 21, 2024

frisitano Nov 21, 2024

frisitano Nov 21, 2024

greged93 Nov 21, 2024

Thegaram Nov 22, 2024

greged93 Nov 22, 2024

Thegaram Nov 22, 2024

greged93 Nov 22, 2024

feat: reth-scroll crates #33

Are you sure you want to change the base?

feat: reth-scroll crates #33

Conversation

greged93 commented Nov 18, 2024 • edited Loading

Overview

Notes

Choose a reason for hiding this comment

greged93 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

frisitano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greged93 commented Nov 18, 2024 •

edited

Loading

greged93 Nov 19, 2024 •

edited

Loading