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

add checkpoint low watermark for state sync #12231

Merged
merged 4 commits into from
Jun 3, 2023
Merged

Conversation

longbowlu
Copy link
Collaborator

@longbowlu longbowlu commented May 27, 2023

Description

This PR adds the checkpoint low watermark for state sync. This lets a node to know who to query checkpoint contents according to the watermarks, once checkpoint pruning is enabled.

  • It adds a new state sync method get_peer_latest_checkpoint_info. This call will eventually replace GetCheckpointSummaryRequest::Latest request and is called between every tick in get_latest_from_peer. The response include the highest synced checkpoint summary and the lowest available checkpoint content watermark.
    • To deal with the upgrade, we first query get_peer_latest_checkpoint_info, if it's 404 then we query get_checkpoint_summary and set the low watermark to 0 (if the node hasn't upgraded, they will have all the checkpoint content up to genesis).
  • this low watermark is then updated in PeerHeights. When a node queries peer for checkpoint content, it will only contact peers who has low-watermark lower than the requested checkpoint.
  • currently all nodes has low-watermark set to 0, once [pruner] initial version of checkpoints pruner #12067 lands, we will use the watermark set there.

Test Plan

Tests in state_sync/tests/rs


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Adds the checkpoint low watermark for state sync. This lets a node to know who to query checkpoint contents according to the watermarks, once checkpoint pruning is enabled.

@vercel
Copy link

vercel bot commented May 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
offline-signer-helper ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2023 1:08am
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Jun 3, 2023 1:08am
explorer-storybook ⬜️ Ignored (Inspect) Jun 3, 2023 1:08am
sui-wallet-kit ⬜️ Ignored (Inspect) Jun 3, 2023 1:08am
wallet-adapter ⬜️ Ignored (Inspect) Jun 3, 2023 1:08am

@longbowlu longbowlu marked this pull request as ready for review May 30, 2023 21:05
Copy link
Contributor

@aschran aschran left a comment

Choose a reason for hiding this comment

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

tyvm for the PR! mostly just style/naming nits from me

crates/sui-network/build.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/server.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/test_utils.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/mod.rs Outdated Show resolved Hide resolved
@@ -138,6 +141,9 @@ struct PeerStateSyncInfo {
on_same_chain_as_us: bool,
/// Highest checkpoint sequence number we know of for this Peer.
height: CheckpointSequenceNumber,
/// lowest available checkpoint watermark for this Peer.
/// This defaults to 0 for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

how does it work with defaulting to zero, once everyone has upgraded? will this ever cause us to request checkpoints from a peer that they might not have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Today we construct PeerStateSyncInfo only when for the first time we talk to this peer, by querying the genesis checkpoint summary. Later in every tick we poll peers to get their latest checkpoint availability (added in this PR). Theoretically it's possible to request a checkpoint before that info is updated.

I think we should call get_checkpoint_availability to init PeerStateSyncInfo once this call is rolled out so we always have the correct data. And before that happens we need to bear with the small possibility of what you described.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a todo for this? But I guess it's a non issue until peers start pruning tx, is that right?

crates/sui-network/src/state_sync/mod.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/mod.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/mod.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/mod.rs Outdated Show resolved Hide resolved
crates/sui-network/src/state_sync/mod.rs Outdated Show resolved Hide resolved
@longbowlu longbowlu force-pushed the low-checkpoint-watermark branch from f9ab0af to bfd0b61 Compare June 3, 2023 01:08
@longbowlu longbowlu merged commit 942b498 into main Jun 3, 2023
@longbowlu longbowlu deleted the low-checkpoint-watermark branch June 3, 2023 01:37
ebmifa added a commit that referenced this pull request Jun 15, 2023
## Description 
Release Notes PR checker and generator

## Test Plan
```
eugene@mac-studio ~/code/sui (eugene/create_release_notes_workflow) $ ./scripts/generate-release-notes.sh releases/sui-v1.3.0-release releases/sui-v1.2.0-release
#12359:
#12444:

Introduce new modules: `clob_v2` and `custodian_v2` and deprecate `clob` and `custodian`.
#12231:
Adds the checkpoint low watermark for state sync. This lets a node to know who to query checkpoint contents according to the watermarks, once checkpoint pruning is enabled.
#12321:
#12251:
#12092:

Added a version of stake withdraw function that returns the withdrawn SUI tokens, for better composability.
#12258:
#12171:
update anemo to MystenLabs/anemo@1bfa784 which includes the `alternate_server_name` change. Set primary server name as `sui-{chain-id}`.  Note that if node starts to run this version in testnet/mainnet before the update is officially rolled out, it's likely the node won't be able to state sync.
#12043:

- adds `sui::kiosk::default()` function for easy Kiosk setup
```
Will test the workflows once this PR lands
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