feat: run pipeline only if missing range is large #3059

Rjected · 2023-06-08T05:49:02Z

Modifies the beacon engine to only run the pipeline if the missing range is larger than a set constant (currently 128)

should fix #2964

codecov · 2023-06-08T06:03:50Z

Codecov Report

Merging #3059 (2bf5b57) into main (0ffb9c5) will increase coverage by 0.05%.
The diff coverage is 85.38%.

@@            Coverage Diff             @@
##             main    #3059      +/-   ##
==========================================
+ Coverage   69.75%   69.80%   +0.05%     
==========================================
  Files         529      530       +1     
  Lines       71339    71451     +112     
==========================================
+ Hits        49763    49877     +114     
+ Misses      21576    21574       -2

Flag	Coverage Δ
integration-tests	`16.41% <0.00%> (-0.02%)`	⬇️
unit-tests	`64.84% <85.38%> (+0.05%)`	⬆️

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

Impacted Files	Coverage Δ
bin/reth/src/node/mod.rs	`10.74% <0.00%> (-0.02%)`	⬇️
crates/consensus/beacon/src/engine/mod.rs	`78.49% <85.77%> (-1.23%)`	⬇️

... and 19 files with indirect coverage changes

onbjerg · 2023-06-08T09:14:26Z

Is this correct? I.e. if I start the node and I am lacking like 90 blocks, will we be able to get these w/o the pipeline (is the tree p2p enabled yet?)

Rjected · 2023-06-08T17:22:12Z

Is this correct? I.e. if I start the node and I am lacking like 90 blocks, will we be able to get these w/o the pipeline (is the tree p2p enabled yet?)

yeah, this will currently queue downloads for the 90 block range one-by-one, and then execute them once they've all been downloaded

mattsse · 2023-06-08T17:56:36Z

(is the tree p2p enabled yet?)

yes we have this mode. when we receive a new FCU and we don't have the new head buffered yet, we will start downloading from this new head towards the local head and try to canonicalize the new head via tree this way.

Rjected · 2023-06-09T05:20:45Z

a test is timing out 🤔🤔🤔🤔🤔

mattsse

this likely needs a rebase because I think we renamed ShareableDB since

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

mattsse · 2023-06-13T18:55:17Z

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

+                        // get the latest valid finalized block or the tip (if we don't have the
+                        // finalized block set)
+                        let fork_ancestor_num = self.forkchoice_state_tracker.last_valid_finalized().and_then(|finalized| {
+                            // now we need to actually fetch these
+                            self.blockchain.block_number(finalized).unwrap_or_else(|err| {
+                                // TODO: store full headers (or BlockNumHash) in forkchoice
+                                // state tracker if valid, so we don't need to fetch at all
+                                error!(target: "consensus::engine", ?finalized, ?err, "Failed to get block number from validated finalized block hash");
+                               None
+                            })
+                        }).unwrap_or_else(|| self.blockchain.canonical_tip().number);
+


It's not immediately clear to me what we're checking here.
what exactly are we checking here?

this looks way too complex

this should be much easier imo

I thought the latest valid finalized block would be give a better heuristic for how long the new fork could be, but yeah it's too complex and using the head is much simpler, so switched to doing that

Rjected · 2023-06-14T05:24:23Z

need to fix another test post-rebase

mattsse · 2023-06-14T12:45:54Z

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

+                        // compare the missing parent with the canonical tip
+                        let canonical_tip_num = self.blockchain.canonical_tip().number;
+
+                        // if the number of missing blocks is greater than the max, run the
+                        // pipeline
+                        if missing_parent.number >= canonical_tip_num &&
+                            missing_parent.number - canonical_tip_num >


this is much easier to understand and should be sufficient

mattsse · 2023-06-14T12:46:48Z

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

+                            missing_parent.number - canonical_tip_num >
+                                self.pipeline_run_threshold
+                        {
+                            self.sync.set_pipeline_sync_target(missing_parent.hash)


this is a bit dangerous imo, because this is not necessarily part of the chain
so I think we should use the finalized block of the latest FCU here

switched to setting the finalized sync target here, but would like some input on the edge case where the latest finalized block is None:
https://github.com/paradigmxyz/reth/pull/3059/files#diff-6422221acf035d27429c58dd93438246d53b548efb00b9b3cfbd610c94f8c544R1059-R1064

yeah, we can ignore this else branch because I don't think this is even reachable since we only trigger pipeline on a FCU in the first placee

onbjerg · 2023-06-19T11:05:55Z

Is this still relevant with #2964 being merged? If it is, what is the path forward here?

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

mattsse

very nice!

tests look good,
I also checked several scenarios on an outdated sepolia node with restarts etc. works as expected.

one thing we can consider is lowering the threshold because 128 blocks is ~25min and executing a lot of blocks in the engine has some perf drawbacks (unresponsive to CL requests) which is not ideal but also not very problematic during sync

onbjerg

looks good to me too

Rjected added C-enhancement New feature or request A-consensus Related to the consensus engine labels Jun 8, 2023

Rjected force-pushed the dan/run-pipeline-if-range-is-long branch 2 times, most recently from d3c9cd4 to 1905dc1 Compare June 12, 2023 17:21

Rjected mentioned this pull request Jun 13, 2023

Support queuing multi-block downloads and executing ranges of blocks after a forkchoice update #3117

Closed

Rjected force-pushed the dan/run-pipeline-if-range-is-long branch from 696d893 to 2c28d8a Compare June 13, 2023 01:20

Rjected marked this pull request as ready for review June 13, 2023 01:20

Rjected requested review from rkrasiuk, mattsse and onbjerg as code owners June 13, 2023 01:20

mattsse requested changes Jun 13, 2023

View reviewed changes

Rjected force-pushed the dan/run-pipeline-if-range-is-long branch from dc9ef84 to 588cf3a Compare June 14, 2023 04:55

mattsse reviewed Jun 14, 2023

View reviewed changes

Rjected added 10 commits June 19, 2023 11:37

fix: only run pipeline if range is long

3ebe4fd

always download full blocks first

b27fab8

use correct traces for triggering block download

280437a

fix check_invalid and traces

b7c8478

add TestConsensusEngineBuilder and let tests disable bctree sync

6480964

compare to validated finalized hash instead of head

df91255

use canonical tip instead of fetching the head

cee15bc

remove latest valid finalized method

a9e5621

remove clone

b08ff58

cargo fmt

d20902d

Rjected added 4 commits June 19, 2023 11:49

improve pipeline_run_threshold comment

0b69e62

run pipeline to finalized head

a2a02c1

cargo fmt

aeb5dbc

remove else branch

361afbb

Rjected force-pushed the dan/run-pipeline-if-range-is-long branch from af1d4fa to 361afbb Compare June 19, 2023 15:51

re-add max block setting

9167fa3

Rjected requested a review from mattsse June 19, 2023 17:02

mattsse reviewed Jun 19, 2023

View reviewed changes

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

use finalized block as target and check if it's already canonical

2bf5b57

mattsse approved these changes Jun 19, 2023

View reviewed changes

onbjerg approved these changes Jun 19, 2023

View reviewed changes

Rjected added this pull request to the merge queue Jun 19, 2023

Merged via the queue into main with commit 255780d Jun 19, 2023

Rjected deleted the dan/run-pipeline-if-range-is-long branch June 19, 2023 18:54

mattsse mentioned this pull request Jun 19, 2023

Node is stuck in infinite 'try_insert_validated_block' loop (+ DB breakage) #3087

Closed

1 task

shekhirin mentioned this pull request Jun 23, 2023

feat(stages, storage): pruning configuration #3341

Merged

feat: run pipeline only if missing range is large #3059

feat: run pipeline only if missing range is large #3059

Uh oh!

Conversation

Rjected commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

onbjerg commented Jun 8, 2023

Uh oh!

Rjected commented Jun 8, 2023

Uh oh!

mattsse commented Jun 8, 2023

Uh oh!

Rjected commented Jun 9, 2023

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattsse Jun 13, 2023

Choose a reason for hiding this comment

Uh oh!

Rjected Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Rjected commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

mattsse Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Rjected Jun 19, 2023

Choose a reason for hiding this comment

Uh oh!

mattsse Jun 19, 2023 • 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!

onbjerg commented Jun 19, 2023

Uh oh!

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

onbjerg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rjected commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading

Rjected commented Jun 14, 2023 •

edited

Loading

mattsse Jun 19, 2023 •

edited

Loading