feat(consensus): beacon consensus engine by rkrasiuk · Pull Request #1845 · paradigmxyz/reth

rkrasiuk · 2023-03-19T15:52:36Z

Replacement for #1662
Closes #1528

This PR introduces the Beacon Consensus Engine.

The Beacon Consensus Engine is responsible for initiating the historical sync and driving the real-time sync by consuming messages from the Engine API. The consensus engine will start syncing as soon as it receives first ForkchoiceUpdated message from the CL (or manually set forkchoice through --debug.tip cli argument).

codecov-commenter · 2023-03-19T20:31:50Z

Codecov Report

Merging #1845 (043a0c4) into main (6afdd33) will increase coverage by 11.70%.
The diff coverage is 86.01%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##             main    #1845       +/-   ##
===========================================
+ Coverage   61.41%   73.12%   +11.70%     
===========================================
  Files         418      423        +5     
  Lines       51340    51803      +463     
===========================================
+ Hits        31532    37882     +6350     
+ Misses      19808    13921     -5887

Flag	Coverage Δ
integration-tests	`19.41% <0.00%> (+3.69%)`	⬆️
unit-tests	`67.49% <86.01%> (+10.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bin/reth/src/args/rpc_server_args.rs	`54.05% <0.00%> (+30.42%)`	⬆️
bin/reth/src/node/mod.rs	`6.13% <0.00%> (+2.39%)`	⬆️
crates/consensus/beacon/src/engine/message.rs	`0.00% <0.00%> (ø)`
crates/consensus/beacon/src/lib.rs	`100.00% <ø> (ø)`
crates/interfaces/src/executor.rs	`100.00% <ø> (ø)`
crates/rpc/rpc-builder/src/auth.rs	`0.00% <0.00%> (ø)`
crates/rpc/rpc-engine-api/src/error.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/rpc/rpc-engine-api/src/message.rs	`0.00% <ø> (ø)`
crates/rpc/rpc/src/engine.rs	`0.00% <0.00%> (ø)`
crates/storage/provider/src/test_utils/blocks.rs	`100.00% <ø> (ø)`
... and 10 more

... and 150 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

onbjerg

Looks good to me overall, very simple. Some minor nits and a question

crates/consensus/beacon/src/engine/error.rs

crates/consensus/beacon/src/engine/message.rs

crates/consensus/beacon/src/engine/mod.rs

onbjerg · 2023-03-20T16:57:19Z

crates/consensus/beacon/src/engine/mod.rs

+/// While the pipeline is syncing, the controller will keep processing messages from the receiver
+/// and forwarding them to the blockchain tree.


Does this mean that all new payloads etc. are sent to the blockchain tree while we sync? My worry here is that if you start sync from block 0 to e.g. 16m on mainnet that blockchain tree will grow unbounded in that period since the state in the tree is never flushed. However, this assumes that CL will send us new payloads if we are not synced yet, which might not be true.

Yeah I would consider dropping messages as long as we're still syncing - beyond the first msg

I would just buffer the last N.
This is needed when the pipeline is close to the tip and the transition to the tree needs to be made, buffering blocks would make the transition better.

@onbjerg it doesn't forward if the pipeline is syncing

onbjerg · 2023-03-20T16:59:46Z

crates/stages/src/pipeline/mod.rs

        }
    }

+    /// Consume the pipeline and run it. Return the pipeline and its result as a future.


I would add more docs here and on run that line out the distinction between the two and when to use one over the other (for crate consumers)

crates/consensus/beacon/src/engine/mod.rs

gakonst

Took a close look. I really like this. What is the integration point with Engine API going to look like, and how/where are we going to instantiate this?

Other thoughts:

Maybe we can clone a handle to the txpool in this to support block building
We might want a Pending / InMempool state provider separately

crates/consensus/beacon/src/engine/mod.rs

gakonst · 2023-03-20T23:22:56Z

crates/consensus/beacon/src/engine/mod.rs

+/// While the pipeline is syncing, the controller will keep processing messages from the receiver
+/// and forwarding them to the blockchain tree.


Yeah I would consider dropping messages as long as we're still syncing - beyond the first msg

crates/consensus/beacon/src/engine/mod.rs

gakonst · 2023-03-20T23:33:06Z

crates/consensus/beacon/src/engine/mod.rs

+                            // Get next pipeline state.
+                            this.next_pipeline_state(pipeline, tip)


If the current pipeline tip is the same as tip should this set the state to Idle?

crates/consensus/beacon/src/engine/mod.rs

crates/stages/src/pipeline/mod.rs

gakonst

Generally LGTM. Clean. I would add some more tests for the happy cases, e.g. newPayload -> newPayload -> FCU

crates/executor/src/blockchain_tree/mod.rs

bin/reth/src/node/mod.rs

crates/consensus/beacon/src/engine/mod.rs

crates/executor/src/blockchain_tree/mod.rs

rakita

lgtm, did you maybe try it on testnet?

mattsse

looks pretty good, could follow easily.

left some nits, suggestions.

bin/reth/src/node/mod.rs

crates/consensus/beacon/src/engine/message.rs

bin/reth/src/node/mod.rs

crates/consensus/beacon/src/engine/mod.rs

mattsse · 2023-03-22T12:38:19Z

crates/consensus/beacon/src/engine/mod.rs

+{
+    type Output = Result<(), BeaconEngineError>;
+
+    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {


this logic seems ok:

drain all messages
poll pipeline
repeat if pipeline not pending

crates/consensus/beacon/src/engine/mod.rs

gakonst

Alright. Full send, given our existing testing is solid and it looks like running CL <> EL correctly makes the EL get driven by the CL's headers. We can iterate as we run this live etc.

gakonst · 2023-03-23T17:48:49Z

bin/reth/src/node/mod.rs

+            None => {
+                let warn_msg = "No tip specified. \
+                reth cannot communicate with consensus clients, \
+                so a tip must manually be provided for the online stages with --debug.tip <HASH>.";
+                warn!(target: "reth::cli", warn_msg);
+                None


We should probably remove this warning if a CL is detected

crates/executor/src/blockchain_tree/mod.rs

feat(sync): beacon consensus engine

628986a

rkrasiuk added A-staged-sync Related to staged sync (pipelines and stages) A-engine Related to the engine implementation labels Mar 19, 2023

add docs and rm unused import

f6bd0bc

gakonst mentioned this pull request Mar 20, 2023

wip:feat: add auto-seal consensus impl #1817

Closed

onbjerg reviewed Mar 20, 2023

View reviewed changes

gakonst reviewed Mar 20, 2023

View reviewed changes

crates/consensus/beacon/src/engine/mod.rs Outdated Show resolved Hide resolved

gakonst reviewed Mar 20, 2023

View reviewed changes

integration with engine api

d2f0aed

mattsse requested changes Mar 21, 2023

View reviewed changes

mattsse mentioned this pull request Mar 21, 2023

feat: Add autoseal consensus and downloaders #1880

Merged

add engine handler tests

52690c7

gakonst mentioned this pull request Mar 21, 2023

eth_coinbase impl #1896

Closed

rkrasiuk added 3 commits March 21, 2023 23:09

feat(cli): init consensus engine

1184f34

metrics update

3f60a38

address PR comments

afa0d9d

rkrasiuk marked this pull request as ready for review March 21, 2023 22:01

rkrasiuk requested review from Rjected and joshieDo as code owners March 21, 2023 22:01

rkrasiuk added 2 commits March 22, 2023 00:02

clippy

509d1b6

clippy

e5f71d9

gakonst approved these changes Mar 22, 2023

View reviewed changes

rkrasiuk added 5 commits March 22, 2023 08:49

add hash to pre merge error

6f8ca47

add valid fcu test

503b991

add new_payload_before_forkchoice test

c857fcb

clippy

c1fdc6f

clean up error types

2d15f59

rakita reviewed Mar 22, 2023

View reviewed changes

crates/executor/src/blockchain_tree/mod.rs Show resolved Hide resolved

rakita approved these changes Mar 22, 2023

View reviewed changes

mattsse requested changes Mar 22, 2023

View reviewed changes

rkrasiuk added 4 commits March 22, 2023 16:09

add max block parameter to the engine

775d1a8

Merge branch 'main' into rkrasiuk/sync-controller

5556a53

add docs, address PR comments

80ca8ae

more expressive message type

08e5ed8

rkrasiuk requested a review from mattsse March 22, 2023 15:35

rkrasiuk mentioned this pull request Mar 23, 2023

feat(consensus): start with safe block hash #1932

Merged

consensus engine: start with safe block hash (#1932)

043a0c4

gakonst approved these changes Mar 23, 2023

View reviewed changes

gakonst merged commit ce40bea into main Mar 23, 2023

gakonst deleted the rkrasiuk/sync-controller branch March 23, 2023 18:18

rkrasiuk added A-consensus Related to the consensus engine C-enhancement New feature or request and removed A-staged-sync Related to staged sync (pipelines and stages) labels Mar 24, 2023

rkrasiuk changed the title ~~feat(sync): beacon consensus engine~~ feat(consensus): beacon consensus engine Mar 24, 2023

rakita restored the rkrasiuk/sync-controller branch March 27, 2023 11:29

rakita deleted the rkrasiuk/sync-controller branch March 27, 2023 11:44

		/// While the pipeline is syncing, the controller will keep processing messages from the receiver
		/// and forwarding them to the blockchain tree.

		// Get next pipeline state.
		this.next_pipeline_state(pipeline, tip)

Conversation

rkrasiuk commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

onbjerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rakita left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gakonst left a comment

Choose a reason for hiding this comment

Uh oh!

rkrasiuk commented Mar 19, 2023 •

edited

Loading

codecov-commenter commented Mar 19, 2023 •

edited

Loading