Skip to content

Conversation

rkrasiuk
Copy link
Member

Addresses first part of #607 (comment)

Writing total difficulty into the database is now extracted into a separate stage. Previously, it was done as a one-off write upon Headers stage completion.

Open to discussing whether this stage should be before or after Bodies.

@rkrasiuk rkrasiuk added C-enhancement New feature or request A-staged-sync Related to staged sync (pipelines and stages) labels Dec 30, 2022
@rkrasiuk rkrasiuk requested a review from onbjerg as a code owner December 30, 2022 14:04
Comment on lines -41 to -43
/// NOTE: This stage commits the header changes to the database (everything except the changes to
/// [`HeaderTD`][reth_interfaces::db::tables::HeaderTD] table). The stage does not return the
/// control flow to the pipeline in order to preserve the context of the chain tip.
Copy link
Member Author

Choose a reason for hiding this comment

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

this comment has been obsolete after #609

@codecov-commenter
Copy link

Codecov Report

Merging #665 (8e84270) into main (f5ae970) will increase coverage by 1.15%.
The diff coverage is 91.33%.

@@            Coverage Diff             @@
##             main     #665      +/-   ##
==========================================
+ Coverage   71.99%   73.15%   +1.15%     
==========================================
  Files         260      261       +1     
  Lines       26264    26760     +496     
==========================================
+ Hits        18909    19575     +666     
+ Misses       7355     7185     -170     
Flag Coverage Δ
unit-tests 73.15% <91.33%> (+1.15%) ⬆️

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

Impacted Files Coverage Δ
bin/reth/src/config.rs 0.00% <0.00%> (ø)
bin/reth/src/node/mod.rs 0.00% <0.00%> (ø)
crates/stages/src/test_utils/test_db.rs 64.91% <ø> (-1.19%) ⬇️
crates/stages/src/stages/total_difficulty.rs 96.58% <96.58%> (ø)
crates/stages/src/stages/headers.rs 97.03% <100.00%> (-0.21%) ⬇️
crates/net/discv4/src/lib.rs 50.86% <0.00%> (-0.23%) ⬇️
crates/interfaces/src/test_utils/generators.rs 100.00% <0.00%> (ø)
crates/common/rlp/src/decode.rs 90.69% <0.00%> (+0.19%) ⬆️
crates/transaction-pool/src/pool/txpool.rs 58.21% <0.00%> (+0.51%) ⬆️
crates/primitives/src/hex_bytes.rs 94.38% <0.00%> (+1.53%) ⬆️
... and 8 more

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.

ACK from me.

Before, we would only write total difficulty after headers stage is done inside the is_stage_done function. That works, but it's cleaner to move it to a separate stage, especially given that headers operate in reverse.

cc @onbjerg

Comment on lines +40 to +49
let stage_progress = input.stage_progress.unwrap_or_default();
let previous_stage_progress = input.previous_stage_progress();

let start_block = stage_progress + 1;
let end_block = previous_stage_progress.min(start_block + self.commit_threshold);

if start_block > end_block {
info!(target: "sync::stages::total_difficulty", stage_progress, "Target block already reached");
return Ok(ExecOutput { stage_progress, done: true })
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to make this an associated function on the Stage trait, we do this on every stage and should be something the developer of a stage doesn't need to think about.

Comment on lines +77 to +79
let done = end_block >= previous_stage_progress;
info!(target: "sync::stages::total_difficulty", stage_progress = end_block, done, "Sync iteration finished");
Ok(ExecOutput { done, stage_progress: end_block })
Copy link
Member

Choose a reason for hiding this comment

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

same as above for the start/end stuff.

@gakonst gakonst merged commit cb6ddfc into main Jan 1, 2023
@gakonst gakonst deleted the rkrasiuk/sync-td-stage branch January 1, 2023 17:03
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) C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants