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

feat: reset broken sync #4955

Merged

Conversation

SWvheerden
Copy link
Collaborator

Description

If sync fails resets chain to the highest pow chain the node locally has the data to.

Motivation and Context

See: #4866

How Has This Been Tested?

Unit tests

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

I think the logic in add_block should then call this method as well, so that we don't have duplicate code

chain_strength_comparer: &dyn ChainStrengthComparer,
) -> Result<(), ChainStorageError> {
let metadata = db.fetch_chain_metadata()?;
// lets clear out all remaining headers that done have a matching block
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// lets clear out all remaining headers that done have a matching block
// lets clear out all remaining headers that don't have a matching block

@stringhandler stringhandler added the P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues label Nov 25, 2022
// height it will only trim the extra headers with no blocks
rewind_to_height(db, metadata.height_of_longest_chain())?;
let all_orphan_tips = db.fetch_all_orphan_chain_tips()?;
if all_orphan_tips.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this check should be before we rewind

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I specifically did not do this as I use the rewind code here to remove the "extra" headers.
I want to "main chain" to be "clean" after this run.

The intended case is this.

Header sync downloads 10 headers.
Block sync fails after 5.
That extra 5 headers will stay on top of the main chain till we remove them again. I want to remove them at this point regardless of whether the new tip is higher than our original one.

if all_orphan_tips.is_empty() {
// we have no orphan chain tips, we have trimmed remaining headers, we are on the best tip we have, so lets
// return ok
return Ok(BlockAddResult::OrphanBlock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably rename this to ChainReorgResult

Copy link
Collaborator

Choose a reason for hiding this comment

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

And OrphanBlock to change to NoChange

@stringhandler stringhandler merged commit 01e9e7e into tari-project:development Nov 28, 2022
@SWvheerden SWvheerden deleted the sw_reset_broken_sync branch December 7, 2022 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants