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: Sync improvements and propagation #1530

Closed
wants to merge 50 commits into from
Closed

feat: Sync improvements and propagation #1530

wants to merge 50 commits into from

Conversation

Frando
Copy link
Member

@Frando Frando commented Sep 26, 2023

Description

This PR implements sync propagation. NOTE: The main feature of this PR, propagating SyncReports to neighboring peers, is currently DISABLED through a constant which is set to false (SYNC_REPORTS_ENABLED in iroh/src/sync_engine/live.rs). The test for this feature sync_big is ignored. We will enable the feature once the test is not flakey anymore.

  • When a set-reconcilation sync completes, iroh-sync now returns a struct AuthorHeads which contains a map with an entry for each author for which we received entries in the sync operation, and the latest timestamp of all entries we received. So it is a kind of "cheapo vector clock", indicating "what's the latest entry we received in this sync op for each author"
  • This AuthorHeads struct is then sent as a gossip message * to neighbors only* as a SyncReport
  • When receiving a sync report, a node checks if the AuthorHeads have any news to offer (i.e. if it contains a timestamp for an author newer than the latest existing entry for this author in the local store). If so, the node starts a sync request to the node we received the report from

Through this, the sync operations should propagate, and through checking the AuthorHeads if there are actually news for us loops should be avoided.

Notes & open questions

My biggest question is if that propagation model of sending theAuthorHeads to neighbors will scale in practice.

There is a test included that passes for 20 nodes, but hangs for more nodes. Reason unknown.

Some notes:

  • The implementation of getting the local AuthorHeads is naive and not at all efficient atm, we will have to add a new table to the redb to store AuthorId -> LatestEntry
  • The LiveSync actor would really benefit from some refactoring to better track the various states involved
  • For big sync operations with many messages we might want to limit the AuthorHeads to some number of authors
  • We will want to debounce and merge sending sync reports over some interval
  • We might want to delay sending sync reports, or not send sync reports for very recent entries,to not "race" with gossip

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@Frando Frando changed the title [WIP] Sync propagation feat: Sync propagation Oct 6, 2023
@Frando Frando changed the title feat: Sync propagation feat: Sync improvements and propagation (disabled) Oct 9, 2023
@Frando Frando changed the title feat: Sync improvements and propagation (disabled) feat: Sync improvements and propagation Oct 9, 2023
// Table
// Key: ([u8; 32], [u8; 32]) # (NamespaceId, AuthorId)
// Value: (u64, Vec<u8>) # (Timestamp, Key)
const LATEST_TABLE: TableDefinition<LatestKey, LatestValue> = TableDefinition::new("latest-1");
Copy link
Contributor

Choose a reason for hiding this comment

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

this table will be empty on new versions, resulting in wrong results, we should either do a migration or error on an existing db

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I wrote a migration: 8865641

@Frando Frando mentioned this pull request Oct 11, 2023
3 tasks
@Frando
Copy link
Member Author

Frando commented Oct 11, 2023

Likely will close this in favor of #1612 and #1613..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants