feat(sync): headers commit threshold #296

rkrasiuk · 2022-11-30T09:33:19Z

Modify the headers stage to download the block headers in batches. Add a commit_threshold parameter for aggregated commits

blocked by #310

codecov · 2022-11-30T10:38:49Z

Codecov Report

Merging #296 (3a4eb06) into main (c7ec451) will increase coverage by 0.00%.
The diff coverage is 95.00%.

@@           Coverage Diff            @@
##             main     #296    +/-   ##
========================================
  Coverage   73.73%   73.74%            
========================================
  Files         229      230     +1     
  Lines       21081    21393   +312     
========================================
+ Hits        15544    15776   +232     
- Misses       5537     5617    +80

Impacted Files	Coverage Δ
crates/interfaces/src/p2p/headers/downloader.rs	`57.14% <ø> (ø)`
crates/interfaces/src/test_utils/headers.rs	`76.77% <90.00%> (-7.92%)`	⬇️
crates/net/headers-downloaders/src/linear.rs	`86.97% <95.23%> (+10.08%)`	⬆️
crates/primitives/src/header.rs	`93.85% <100.00%> (+0.07%)`	⬆️
crates/stages/src/stages/headers.rs	`97.34% <100.00%> (+0.74%)`	⬆️
crates/codecs/src/lib.rs	`94.22% <0.00%> (-5.41%)`	⬇️
crates/net/network/src/network.rs	`53.24% <0.00%> (-4.50%)`	⬇️
crates/net/discv4/src/lib.rs	`66.13% <0.00%> (-2.46%)`	⬇️
crates/net/discv4/src/proto.rs	`90.69% <0.00%> (-2.33%)`	⬇️
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mattsse

the stream looks alright,
since we need to day a lot of checks and conditionally return it makes sense to solve this with macros here

crates/net/headers-downloaders/src/linear.rs

gakonst

nice work. some nits, requests for more docs, and questions aroudn if we had bugs before

gakonst · 2022-12-02T12:25:45Z

crates/interfaces/src/test_utils/headers.rs

                let mut headers = resp.0.into_iter().skip(1).map(|h| h.seal()).collect::<Vec<_>>();
                headers.sort_unstable_by_key(|h| h.number);
-                Poll::Ready(Ok(headers))
+                Poll::Ready(Ok(headers.into_iter().rev().collect()))


why rev here? why was it not before?

initially, we returned it in reverse. then it was changed during a recent refactor. that made sense for the Future implementation alone, but it doesn't for the Stream which returns reverse batches. i changed it back to keep it consistent

cc @mattsse

crates/net/headers-downloaders/src/linear.rs

gakonst · 2022-12-02T12:44:39Z

crates/stages/src/stages/headers.rs

        while let Some(header) = headers_iter.next() {
-            ensure_parent(header, headers_iter.peek().unwrap_or(&&head))
-                .map_err(|err| StageError::Download(err.to_string()))?;
+            if let Some(parent) = headers_iter.peek() {


what if there's no parent? should this error?

this means that the parent will come in the next batch. best we can do is to validate upon receiving that batch

ok lets add that comment then

crates/net/headers-downloaders/src/linear.rs

gakonst · 2022-12-02T12:49:32Z

crates/net/headers-downloaders/src/linear.rs

+                }
+            }
+
+            if !this.done && this.buffered.len() > 1 {


what if we're not done and we have only 1 buffered?

or does having only 1 buffered item mean we're done? let's document that

the done flag is set to true only when the local head (latest db header at the beginning of execution) is reached. we need to keep at least one header to form the next request until we are done

crates/net/headers-downloaders/src/linear.rs

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

…oundry-rs/reth into rkrasiuk/sync-headers-commit-threshold

mattsse · 2022-12-02T14:21:12Z

crates/interfaces/src/test_utils/headers.rs

+        let empty = SealedHeader::default();
+        if let Err(error) = this.consensus.validate_header(&empty, &empty) {
+            this.done = true;
+            return Poll::Ready(Some(Err(DownloadError::HeaderValidation {


why are we validating the default header against the default header?

we're polling consensus stub merely to see whether it will produce an error. it is set on the TestConsensus struct and it will either succeed or fail regardless of input parameters

crates/interfaces/src/test_utils/headers.rs

gakonst

lgtm - merging to get the ball rolling but pls address comments in followup

gakonst · 2022-12-06T06:22:50Z

crates/interfaces/src/test_utils/headers.rs

                let mut headers = resp.0.into_iter().skip(1).map(|h| h.seal()).collect::<Vec<_>>();
                headers.sort_unstable_by_key(|h| h.number);
-                Poll::Ready(Ok(headers))
+                Poll::Ready(Ok(headers.into_iter().rev().collect()))


cc @mattsse

gakonst · 2022-12-06T06:23:32Z

crates/stages/src/stages/headers.rs

        }

        Ok(latest)
    }
+
+    /// Iterate over inserted headers and write td entries
+    fn write_td<DB: Database>(


sure what about a 3rd party client that wants to overwrite the difficulty table for any reason?

gakonst · 2022-12-06T06:24:41Z

crates/stages/src/stages/headers.rs

        while let Some(header) = headers_iter.next() {
-            ensure_parent(header, headers_iter.peek().unwrap_or(&&head))
-                .map_err(|err| StageError::Download(err.to_string()))?;
+            if let Some(parent) = headers_iter.peek() {


ok lets add that comment then

gakonst · 2022-12-06T06:26:57Z

crates/stages/src/stages/headers.rs

        }

        Ok(latest)
    }
+
+    /// Iterate over inserted headers and write td entries
+    fn write_td<DB: Database>(


i also think it looks weird given it doesn't need &self either

* headers stream init * fix tests * return header if available regardless of control flow * proper stream termination & docs * upd headers stage to consume stream * adjust response validation for stream * use cursor.insert for headers * wrap poll_next in a loop to bypass poking waker * fix typo * fix last td lookup * Apply suggestions from code review Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * misc * remove waker ref * dedup response handling logic * clippy * add docs to poll Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

headers stream init

fe434d7

rkrasiuk added C-enhancement New feature or request A-staged-sync Related to staged sync (pipelines and stages) labels Nov 30, 2022

rkrasiuk marked this pull request as draft November 30, 2022 09:33

fix tests

c8f047e

return header if available regardless of control flow

fed2b3b

mattsse requested changes Nov 30, 2022

View reviewed changes

crates/net/headers-downloaders/src/linear.rs Outdated Show resolved Hide resolved

crates/net/headers-downloaders/src/linear.rs Outdated Show resolved Hide resolved

crates/net/headers-downloaders/src/linear.rs Outdated Show resolved Hide resolved

rkrasiuk added 6 commits November 30, 2022 13:42

proper stream termination & docs

c348f97

upd headers stage to consume stream

fbf7cd8

adjust response validation for stream

69b13ae

Merge branch 'main' into rkrasiuk/sync-headers-commit-threshold

f696fa8

Merge branch 'main' into rkrasiuk/sync-headers-commit-threshold

a099132

use cursor.insert for headers

2348cc0

rkrasiuk marked this pull request as ready for review December 2, 2022 07:24

rkrasiuk requested review from mattsse, gakonst and onbjerg December 2, 2022 07:25

rkrasiuk added 2 commits December 2, 2022 12:57

wrap poll_next in a loop to bypass poking waker

c3185a2

fix typo

4ae0d7e

gakonst requested changes Dec 2, 2022

View reviewed changes

rkrasiuk and others added 3 commits December 2, 2022 16:14

fix last td lookup

08923a3

Apply suggestions from code review

d603859

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>

Merge branch 'rkrasiuk/sync-headers-commit-threshold' of github.com:f…

a187572

…oundry-rs/reth into rkrasiuk/sync-headers-commit-threshold

mattsse requested changes Dec 2, 2022

View reviewed changes

rkrasiuk added 5 commits December 2, 2022 16:31

misc

04ba32e

remove waker ref

998fdfc

dedup response handling logic

a2e0b8c

clippy

b7a556f

add docs to poll

3a4eb06

gakonst approved these changes Dec 6, 2022

View reviewed changes

gakonst merged commit 74d776d into main Dec 6, 2022

gakonst deleted the rkrasiuk/sync-headers-commit-threshold branch December 6, 2022 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sync): headers commit threshold #296

feat(sync): headers commit threshold #296

rkrasiuk commented Nov 30, 2022 •

edited

Loading

codecov bot commented Nov 30, 2022 •

edited

Loading

mattsse left a comment

gakonst left a comment

gakonst Dec 2, 2022

rkrasiuk Dec 2, 2022

gakonst Dec 6, 2022

gakonst Dec 2, 2022

rkrasiuk Dec 2, 2022

gakonst Dec 6, 2022

gakonst Dec 2, 2022

rkrasiuk Dec 2, 2022 •

edited

Loading

mattsse Dec 2, 2022

rkrasiuk Dec 2, 2022 •

edited

Loading

gakonst left a comment

gakonst Dec 6, 2022

gakonst Dec 6, 2022

gakonst Dec 6, 2022

gakonst Dec 6, 2022

feat(sync): headers commit threshold #296

feat(sync): headers commit threshold #296

Conversation

rkrasiuk commented Nov 30, 2022 • edited Loading

codecov bot commented Nov 30, 2022 • edited Loading

Codecov Report

mattsse left a comment

Choose a reason for hiding this comment

gakonst 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

rkrasiuk Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkrasiuk Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

gakonst 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

rkrasiuk commented Nov 30, 2022 •

edited

Loading

codecov bot commented Nov 30, 2022 •

edited

Loading

rkrasiuk Dec 2, 2022 •

edited

Loading

rkrasiuk Dec 2, 2022 •

edited

Loading