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

track sync_head on header_sync sync status #3626

Merged
merged 4 commits into from
Apr 6, 2021

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Mar 29, 2021

The (somewhat) redundant sync_head MMR impl was removed and cleanup up in #3556.
This never worked as intended as it did not allow header_head and sync_head to diverge successfully beyond a single batch (512) of headers. We also did not require sync_head to be persisted in the db across node restarts (sync_head would be reset during header sync).

This PR moves the tracking of sync_head into the HeaderSync sync status that we already track.
This allows a sync_head to diverge from current header_head in a scenario where multiple batches of headers are required to successfully sync the fork.

We now track a sync_head on a header fork even if the cumulative difficulty on a given batch of headers is (temporarily) lower than the current header_head.

The issue that this fixes is as follows -

  • local node sees a new header fork based on a peer advertising greater cumulative difficulty (total work).
  • we rewind from header_head (diff X) and process a batch of 512 headers on the fork
  • this results in diff Y on the fork
    • Y < X so header_head is not updated
  • we now fail to sync additional headers on the fork as "locator" is based on header_head
    • we keep syncing the initial 512 headers on the fork and do not make further progress along the fork

The changes in this PR ensure we now correctly track sync_head which in the scenario above would be the last_header of the initial batch of headers on the new fork.
This allows the "locator" to be constructed correctly and progress to be made along the fork.

Resolves #3615


We could have implemented this storing the sync_head in the db but this PR attempts to take advantage of the existing sync_status tracking. We only really need to track sync_head when we are in HeaderSync state for an extended period of time.

We can likely extend this in two ways in future work -

  1. rework the sync state to allow us to track the sync_peer and the various data for stalling etc. during HeaderSync so we can consolidate all the data being tracked during the sync process (currently spread all over header_sync.rs)
  2. track head and associated data in a similar way during BodySync (mainly for consistency)

The work for (1) is a little convoluted as sync state is defined in chain crate currently and has no knowledge of peers etc. We need to spend a bit of time thinking through the best way of approaching this without making the code really awkward.

But it would open up a relatively clean way of tracking a single peer during header_sync. i.e. We keep asking the same peer for subsequent headers until we either sync with that peer successfully or the peer disconnects.


This PR also adds an additional conditional check during check_run in header_sync -

			// Quick check - nothing to sync if we are caught up with the peer.
			if peer_diff <= sync_head.total_difficulty {
				return Ok(false);
			}

This prevents our local node from flapping between BodySync and HeaderSync toward the end of IBD.

@antiochp antiochp marked this pull request as ready for review March 29, 2021 19:12
@antiochp antiochp added this to the 5.1.0 milestone Mar 30, 2021
@antiochp antiochp force-pushed the sync_head_tracking branch from 45e682a to e7a84b4 Compare March 30, 2021 08:13
@antiochp antiochp force-pushed the sync_head_tracking branch from e7a84b4 to 2a1d22b Compare April 1, 2021 14:12
@quentinlesceller
Copy link
Member

Sync from scratch: done no issues. Will have a look at the code later.

@antiochp
Copy link
Member Author

antiochp commented Apr 6, 2021

Merging to master.
Feel free to continue reviewing code.

@antiochp antiochp merged commit 34413c1 into mimblewimble:master Apr 6, 2021
@antiochp antiochp deleted the sync_head_tracking branch April 6, 2021 10:16
@antiochp antiochp mentioned this pull request May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

header_sync during reorg in excess of 512 headers (problematic behavior)
3 participants