mimblewimble / grin

Minimal implementation of the Mimblewimble protocol.
https://grin.mw/
Apache License 2.0
5.04k stars 990 forks source link

track sync_head on header_sync sync status #3626

Closed antiochp closed 3 years ago

antiochp commented 3 years ago

The (somewhat) redundant sync_head MMR impl was removed and cleanup up in https://github.com/mimblewimble/grin/pull/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 -

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.

quentinlesceller commented 3 years ago

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

antiochp commented 3 years ago

Merging to master. Feel free to continue reviewing code.