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 state prior to full sync should be correct #523

Merged
merged 11 commits into from
Mar 27, 2024

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Mar 18, 2024

Before starting sync of data, the sync state should accurately show the data that needs to be synced. There were bugs in the code that meant the "data to sync" was not accurate until the user had already started to sync the data. This PR fixes things so that after doing an "initial" sync, the sync state should accurately show how much data there is to be synced before the user starts syncing.

This PR fixes several things, because each fix revealed other bugs:

  • Updates RemoteBitfield with changes from upstream to fix findLast and a few other things (data was not being added to the Quickbit index when it was inserted from a UInt32Array (as opposed to being set bit-by-bit).
  • Fix CoreSyncState to correctly calculate want wanted have for cores that have not yet been replicated, based on preHave messages. We don't get the core length without replication, so we estimate it from the preHave messages. The length will be incorrect if the connected peer is missing blocks at the end of their core (because we use the last set bit in the bitfield for the length), but this is ok, it just means that missing might be inaccurate before a core is actually replicated.
  • Fix to ignore remote states for blocked peers - without this, because a blocked peer potentially has data which will not sync, then sync state never considers sync to be complete for a blocked peer.
  • Fix to test to ignore missing when comparing states of sync with a blocked peer, because different peers connected to a blocked peer can have different impressions of what is missing, based on whether they have tried to sync with them before blocking them.

(Note: I don't really like how this has been fixed by passing around peer sync controllers and making deriveState more complex because it relies on a more complicated state.... but all I can think of for now, so hopefully "good enough").

@gmaclennan gmaclennan self-assigned this Mar 18, 2024
Co-authored-by: me@evanhahn.com <me@evanhahn.com>
@gmaclennan gmaclennan marked this pull request as ready for review March 25, 2024 16:56
@gmaclennan gmaclennan marked this pull request as draft March 25, 2024 16:56
@gmaclennan gmaclennan marked this pull request as ready for review March 25, 2024 17:38
Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

LGTM other than a few small nitpicks.

src/core-manager/index.js Outdated Show resolved Hide resolved
tests/remote-bitfield.js Outdated Show resolved Hide resolved
test-e2e/sync.js Outdated Show resolved Hide resolved
src/sync/core-sync-state.js Show resolved Hide resolved
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.

2 participants