fix(sync): return control from `HeaderStage` back to pipeline #609

TechieBoy · 2022-12-26T07:49:40Z

We only write commit_threshold blocks after fetching them with the downloader.

Closes #607

TechieBoy · 2022-12-26T07:53:12Z

I think we can make this more efficient by only streaming head + commit_threshold blocks instead of head + fork choice tip, or by downloading from head onwards instead of downloading in reverse order from tip??

rkrasiuk · 2022-12-26T08:18:22Z

thanks for the PR @TechieBoy! unfortunately, we cannot download headers onwards, because of long-range attack (refer to the headers section of the book for more details). it's better to trust the CL and walk the canonical chain from the tip to prevent peers from advertising bad blocks to us. we are planning to add some onward logic for chains that don't have EL/CL separation and possibly for pre-merge headers

crates/stages/src/stages/headers.rs

codecov-commenter · 2022-12-26T10:07:59Z

Codecov Report

Merging #609 (7db0d8c) into main (5bb14ec) will increase coverage by 0.09%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##             main     #609      +/-   ##
==========================================
+ Coverage   72.22%   72.32%   +0.09%     
==========================================
  Files         255      255              
  Lines       25783    25792       +9     
==========================================
+ Hits        18623    18654      +31     
+ Misses       7160     7138      -22

Flag	Coverage Δ
unit-tests	`72.32% <96.22%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
crates/stages/src/stages/headers.rs	`97.20% <96.22%> (-0.46%)`	⬇️
crates/net/network/src/manager.rs	`46.24% <0.00%> (-0.25%)`	⬇️
crates/stages/src/stages/bodies.rs	`93.69% <0.00%> (+0.38%)`	⬆️
crates/transaction-pool/src/pool/txpool.rs	`58.73% <0.00%> (+0.52%)`	⬆️
crates/net/eth-wire/src/types/version.rs	`80.00% <0.00%> (+2.22%)`	⬆️
crates/transaction-pool/src/test_util/mock.rs	`60.37% <0.00%> (+6.66%)`	⬆️

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

TechieBoy · 2022-12-26T10:11:46Z

we cannot download headers onwards, because of long-range attack

Makes sense, but we do want to write the headers to db from head onwards, correct? Otherwise the stage_progress value in ExecOutput will stay the same until the entire stage is done. That is what I am doing here:

reth/crates/stages/src/stages/headers.rs

Lines 98 to 102 in 92f4bb5

    
           // Write a maximum of `commit_threshold` headers, starting from local head onwards 
        
           let write_progress = self 
        
               .write_headers::<DB>(tx, res, self.commit_threshold) 
        
               .await? 
        
               .unwrap_or_default();

Or should it write to db from tip downwards?

rkrasiuk · 2022-12-26T13:59:20Z

we do want to write the headers to db from head onwards

Since we are not appending anymore, it shouldn't matter really.

stage_progress value in ExecOutput will stay the same until the entire stage is done

that's the desired behavior. otherwise we wouldn't know the local "head"

crates/stages/src/stages/headers.rs

… blocks have been synced

TechieBoy · 2022-12-27T13:42:05Z

Take and write only commit_threshold headers from stream to db.
Use headers stored in local db + fork choice state to check if we haved finished the stage.

crates/stages/src/stages/headers.rs

rkrasiuk

lgtm, great job!

TechieBoy · 2022-12-27T15:28:29Z

lgtm, great job!

👍🏻 Thanks for being so patient and helpful and answering all my questions!

gakonst

Good from me - pending @onbjerg approval

gakonst · 2022-12-27T15:13:12Z

crates/stages/src/stages/headers.rs

-                    current_progress = current_progress.max(write_progress);
+        // The downloader returns the headers in descending order starting from the tip
+        // down to the local head (latest block in db)
+        let downloaded_headers: Result<Vec<SealedHeader>, DownloadError> = self


@Bjerg Ptal - looks good from my side

gakonst · 2022-12-27T16:21:36Z

crates/stages/src/stages/headers.rs

@@ -166,6 +169,19 @@ impl<D: HeaderDownloader, C: Consensus, H: HeadersClient, S: StatusUpdater>
        Ok(())
    }

+    async fn is_stage_done<DB: Database>(


@onbjerg PTAL

rkrasiuk · 2022-12-29T08:57:45Z

lgtm, merging

rkrasiuk added the A-staged-sync Related to staged sync (pipelines and stages) label Dec 26, 2022

rkrasiuk requested changes Dec 26, 2022

View reviewed changes

crates/stages/src/stages/headers.rs Outdated Show resolved Hide resolved

rkrasiuk reviewed Dec 26, 2022

View reviewed changes

crates/stages/src/stages/headers.rs Outdated Show resolved Hide resolved

gakonst requested changes Dec 26, 2022

View reviewed changes

crates/stages/src/stages/headers.rs Outdated Show resolved Hide resolved

crates/stages/src/stages/headers.rs Outdated Show resolved Hide resolved

TechieBoy force-pushed the header-commit branch 2 times, most recently from 26b2d68 to 76bb72c Compare December 27, 2022 13:33

TechieBoy added 3 commits December 27, 2022 19:05

Headerstage now gives control back to pipeline after commit_threshold…

ff8278d

… blocks have been synced

Updated tests

97366d0

Only take required number of headers from the stream

1c602d3

TechieBoy force-pushed the header-commit branch from 76bb72c to 1c602d3 Compare December 27, 2022 13:35

TechieBoy marked this pull request as ready for review December 27, 2022 13:36

TechieBoy requested a review from onbjerg as a code owner December 27, 2022 13:36

rkrasiuk requested changes Dec 27, 2022

View reviewed changes

crates/stages/src/stages/headers.rs Outdated Show resolved Hide resolved

crates/stages/src/stages/headers.rs Outdated Show resolved Hide resolved

Simplified stage complete check

7db0d8c

rkrasiuk approved these changes Dec 27, 2022

View reviewed changes

rkrasiuk changed the title ~~fix(sync): Headerstage now gives control back to pipeline after commit_threshold blocks have been synced~~ fix(sync): return control from HeaderStage back to pipeline Dec 27, 2022

gakonst approved these changes Dec 27, 2022

View reviewed changes

rkrasiuk merged commit 28669d4 into paradigmxyz:main Dec 29, 2022

TechieBoy deleted the header-commit branch December 29, 2022 10:57

rkrasiuk mentioned this pull request Dec 30, 2022

feat(sync): total difficulty stage #665

Merged

MegaRedHand mentioned this pull request Jan 12, 2023

Add test that ensures the committing happens correctly inside Headers Stage #418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sync): return control from `HeaderStage` back to pipeline #609

fix(sync): return control from `HeaderStage` back to pipeline #609

TechieBoy commented Dec 26, 2022

TechieBoy commented Dec 26, 2022

rkrasiuk commented Dec 26, 2022

codecov-commenter commented Dec 26, 2022 •

edited

Loading

TechieBoy commented Dec 26, 2022 •

edited

Loading

rkrasiuk commented Dec 26, 2022 •

edited

Loading

TechieBoy commented Dec 27, 2022 •

edited

Loading

rkrasiuk left a comment

TechieBoy commented Dec 27, 2022

gakonst left a comment

gakonst Dec 27, 2022

gakonst Dec 27, 2022

rkrasiuk commented Dec 29, 2022

fix(sync): return control from HeaderStage back to pipeline #609

fix(sync): return control from HeaderStage back to pipeline #609

Conversation

TechieBoy commented Dec 26, 2022

TechieBoy commented Dec 26, 2022

rkrasiuk commented Dec 26, 2022

codecov-commenter commented Dec 26, 2022 • edited Loading

Codecov Report

TechieBoy commented Dec 26, 2022 • edited Loading

rkrasiuk commented Dec 26, 2022 • edited Loading

TechieBoy commented Dec 27, 2022 • edited Loading

rkrasiuk left a comment

Choose a reason for hiding this comment

TechieBoy commented Dec 27, 2022

gakonst left a comment

Choose a reason for hiding this comment

gakonst Dec 27, 2022

Choose a reason for hiding this comment

gakonst Dec 27, 2022

Choose a reason for hiding this comment

rkrasiuk commented Dec 29, 2022

fix(sync): return control from `HeaderStage` back to pipeline #609

fix(sync): return control from `HeaderStage` back to pipeline #609

codecov-commenter commented Dec 26, 2022 •

edited

Loading

TechieBoy commented Dec 26, 2022 •

edited

Loading

rkrasiuk commented Dec 26, 2022 •

edited

Loading

TechieBoy commented Dec 27, 2022 •

edited

Loading