Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sync): commit headers upon threshold #406

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Conversation

rkrasiuk
Copy link
Member

@rkrasiuk rkrasiuk commented Dec 13, 2022

Commit the transaction with header-related entries upon reaching the commit_threshold. The control flow is not returned to the pipeline (which would be responsible for committing under ordinary circumstances), because the Headers stage needs to preserve context while downloading from the tip.

The control flow is returned to the pipeline for committing newHeaderTD entries

@rkrasiuk rkrasiuk added the A-staged-sync Related to staged sync (pipelines and stages) label Dec 13, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #406 (5a5ee10) into main (2534aa8) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   74.13%   74.30%   +0.16%     
==========================================
  Files         236      234       -2     
  Lines       22231    22253      +22     
==========================================
+ Hits        16481    16535      +54     
+ Misses       5750     5718      -32     
Impacted Files Coverage Δ
crates/stages/src/stages/headers.rs 97.35% <100.00%> (+<0.01%) ⬆️
crates/net/network/src/session/active.rs 83.22% <0.00%> (-0.64%) ⬇️
crates/net/network/src/peers/manager.rs 61.41% <0.00%> (-0.60%) ⬇️
crates/net/eth-wire/src/types/status.rs 73.75% <0.00%> (ø)
crates/net/p2p/src/anchor.rs
crates/net/p2p/src/lib.rs
crates/net/network/src/manager.rs 45.32% <0.00%> (+0.12%) ⬆️
crates/net/eth-wire/src/capability.rs 61.79% <0.00%> (+1.12%) ⬆️
crates/stages/src/stages/senders.rs 91.86% <0.00%> (+2.62%) ⬆️
crates/primitives/src/hex_bytes.rs 92.85% <0.00%> (+33.37%) ⬆️

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

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will track the test in a separate issue

@@ -97,6 +101,7 @@ impl<DB: Database, D: HeaderDownloader, C: Consensus, H: HeadersClient> Stage<DB
self.validate_header_response(&res)?;
let write_progress =
self.write_headers::<DB>(db, res).await?.unwrap_or_default();
db.commit()?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could write a test that blocks the stream per-batch, letting us check that the commit actually happened while the stage is running

@gakonst gakonst merged commit c6d38f0 into main Dec 13, 2022
@gakonst gakonst deleted the rkrasiuk/sync-headers-commit branch December 13, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-staged-sync Related to staged sync (pipelines and stages)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants