fix: Use OptimismBeaconConsensus in the OptimismNode #8487

BrianBland · 2024-05-29T18:20:54Z

Adds Consensus as a new Node Component, with default implementations for the EthereumNode and OptimismNode.
EthereumConsensusBuilder builds either EthBeaconConsensus or AutoSealConsensus depending on whether or not dev mode is enabled

mattsse

thanks, I wanted to do this for a while now,
need to check if we can make this work without features

gakonst · 2024-05-29T20:32:03Z

crates/node/builder/src/launch/mod.rs

+            #[cfg(feature = "optimism")]
+            if ctx.chain_spec().is_optimism() {
+                Arc::new(OptimismBeaconConsensus::new(ctx.chain_spec()))
+            } else {
+                Arc::new(EthBeaconConsensus::new(ctx.chain_spec()))
+            }


could this be part of node components so that each node builder implementer defines its own consensus interface implementation?

good point, I believe it should be, yes, because it's stateful

Sounds good, I'll push up another change soon with Consensus added as a new Component

This ended up becoming a much larger change, but hopefully this is directionally aligned with your intent

crates/node/builder/src/launch/mod.rs

BrianBland · 2024-06-01T01:13:06Z

crates/node/builder/src/launch/mod.rs

+            #[cfg(feature = "optimism")]
+            if ctx.chain_spec().is_optimism() {
+                Arc::new(OptimismBeaconConsensus::new(ctx.chain_spec()))
+            } else {
+                Arc::new(EthBeaconConsensus::new(ctx.chain_spec()))
+            }


This ended up becoming a much larger change, but hopefully this is directionally aligned with your intent

mattsse

ah now I get this,

this basically promotes the Consensus impl to a component.

imo this is reasonable and we should do this.

but we should keep using Arc<dyn>

separately, if you have any feedback re builder api/complexity, please let me know

mattsse · 2024-06-10T22:44:48Z

crates/blockchain-tree/src/blockchain_tree.rs

    /// The state of the tree
    ///
    /// Tracks all the chains, the block indices, and the block buffer.
    state: TreeState,
    /// External components (the database, consensus engine etc.)
-    externals: TreeExternals<DB, E>,
+    externals: TreeExternals<DB, C, E>,


the Consensus type is super invasive and would introduce generics everywhere, I'd like to keep using Arc<dyn Consensus>

Fair enough - I wasn't sure whether the use of dyn was actually preferred or only used as a placeholder for a future generic. Agreed that this additional type parameter increases the developer overhead anywhere that this struct is utilized.

developer overhead anywhere that this struct is utilized.

yeah we initial had this as a generic but pivoted to dyn because trait is object safe and the functions aren't called frequently.

we could likely use dyn for a few more generics but some aren't object safe, although we could likely make them, like Database

mattsse

this is great, tysm.

one nit re features

eventually, we will push the engine impls there as well.

mattsse · 2024-06-11T09:50:52Z

crates/node/builder/Cargo.toml

+[features]
+optimism = [
+    "reth-primitives/optimism",
+    "reth-rpc/optimism",
+    "reth-provider/optimism",
+    "reth-beacon-consensus/optimism",
+    "reth-blockchain-tree/optimism",
+    "reth-node-core/optimism",
+]


I don't think this is required?

we want to keep this crate feature free and instead push all things down to the node crates like you did with consensus

Good catch - this was left over from my original implementation

mattsse · 2024-06-11T09:53:24Z

crates/optimism/node/src/node.rs

+        Ok(OptimismBeaconConsensus::new(ctx.chain_spec()))
+    }


This is great,

we can now remove all the optimism features from eth beacon consensus in a followup: #8739

mattsse

lgtm, tysm!

BrianBland force-pushed the node-optimism-consensus branch from 09e4576 to a8ad8eb Compare May 29, 2024 18:28

mattsse reviewed May 29, 2024

View reviewed changes

mattsse self-assigned this May 29, 2024

mattsse added the A-op-reth Related to Optimism and op-reth label May 29, 2024

BrianBland force-pushed the node-optimism-consensus branch from c7df02d to 06e26fd Compare May 29, 2024 20:06

BrianBland marked this pull request as ready for review May 29, 2024 20:25

BrianBland requested review from Rjected, onbjerg and gakonst as code owners May 29, 2024 20:25

gakonst reviewed May 29, 2024

View reviewed changes

emhane mentioned this pull request May 30, 2024

Remove optimism feature #7649

Open

BrianBland force-pushed the node-optimism-consensus branch from 06e26fd to 86efc50 Compare June 1, 2024 00:57

BrianBland requested review from rkrasiuk, shekhirin and rakita as code owners June 1, 2024 00:57

BrianBland force-pushed the node-optimism-consensus branch from 4aabc65 to 9b23f6d Compare June 1, 2024 01:11

BrianBland commented Jun 1, 2024

View reviewed changes

BrianBland force-pushed the node-optimism-consensus branch from 9b23f6d to 1de97a6 Compare June 1, 2024 01:16

BrianBland marked this pull request as draft June 1, 2024 01:22

BrianBland force-pushed the node-optimism-consensus branch 9 times, most recently from 447a370 to 7ddfc8c Compare June 5, 2024 00:24

BrianBland marked this pull request as ready for review June 5, 2024 02:24

BrianBland force-pushed the node-optimism-consensus branch from f24a3af to d665768 Compare June 5, 2024 20:09

BrianBland changed the title ~~fix: Node uses optimism beacon consensus for op chains~~ fix: Use OptimismBeaconConsensus for the OptimismNode Jun 6, 2024

BrianBland changed the title ~~fix: Use OptimismBeaconConsensus for the OptimismNode~~ fix: Use OptimismBeaconConsensus in the OptimismNode Jun 6, 2024

BrianBland added 2 commits June 10, 2024 14:37

fix: Node uses optimism beacon consensus for op chains

1a6c250

Fix usage of optimism feature

7eacdea

BrianBland force-pushed the node-optimism-consensus branch from d665768 to ce4fff7 Compare June 10, 2024 22:21

BrianBland requested review from fgimenez and emhane as code owners June 10, 2024 22:21

mattsse requested changes Jun 10, 2024

View reviewed changes

BrianBland force-pushed the node-optimism-consensus branch 2 times, most recently from 5609b54 to c1125f6 Compare June 10, 2024 23:39

Add consensus as a node component

a2b2422

BrianBland force-pushed the node-optimism-consensus branch from c1125f6 to a2b2422 Compare June 10, 2024 23:47

Fix import

9eb95ff

BrianBland requested a review from mattsse June 11, 2024 00:25

mattsse mentioned this pull request Jun 11, 2024

Remove optimism feature from reth-ethereum-consensus #8739

Closed

mattsse requested changes Jun 11, 2024

View reviewed changes

Remove unnecessary features

5513a07

BrianBland force-pushed the node-optimism-consensus branch from 27a9196 to 5513a07 Compare June 11, 2024 16:55

BrianBland requested a review from mattsse June 11, 2024 17:25

mattsse approved these changes Jun 11, 2024

View reviewed changes

mattsse added this pull request to the merge queue Jun 11, 2024

Merged via the queue into paradigmxyz:main with commit 5eecc4f Jun 11, 2024
49 of 60 checks passed

BrianBland deleted the node-optimism-consensus branch June 11, 2024 18:14

emhane pushed a commit that referenced this pull request Jun 13, 2024

fix: Use OptimismBeaconConsensus in the OptimismNode (#8487)

c4f1d7f

emhane mentioned this pull request Sep 22, 2024

Remove optimism feature from consensus #11109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use OptimismBeaconConsensus in the OptimismNode #8487

fix: Use OptimismBeaconConsensus in the OptimismNode #8487

BrianBland commented May 29, 2024 •

edited

Loading

mattsse left a comment

gakonst May 29, 2024 •

edited

Loading

mattsse May 29, 2024

BrianBland May 29, 2024

BrianBland Jun 1, 2024 •

edited

Loading

BrianBland Jun 1, 2024 •

edited

Loading

mattsse left a comment

mattsse Jun 10, 2024

BrianBland Jun 10, 2024 •

edited

Loading

mattsse Jun 10, 2024

mattsse left a comment

mattsse Jun 11, 2024

BrianBland Jun 11, 2024

mattsse Jun 11, 2024

mattsse left a comment

fix: Use OptimismBeaconConsensus in the OptimismNode #8487

fix: Use OptimismBeaconConsensus in the OptimismNode #8487

Conversation

BrianBland commented May 29, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

gakonst May 29, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse May 29, 2024

Choose a reason for hiding this comment

BrianBland May 29, 2024

Choose a reason for hiding this comment

BrianBland Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

BrianBland Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jun 10, 2024

Choose a reason for hiding this comment

BrianBland Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

mattsse Jun 10, 2024

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jun 11, 2024

Choose a reason for hiding this comment

BrianBland Jun 11, 2024

Choose a reason for hiding this comment

mattsse Jun 11, 2024

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

BrianBland commented May 29, 2024 •

edited

Loading

gakonst May 29, 2024 •

edited

Loading

BrianBland Jun 1, 2024 •

edited

Loading

BrianBland Jun 1, 2024 •

edited

Loading

BrianBland Jun 10, 2024 •

edited

Loading