Adds multi-commit checkpoint batching in Sui. #17955
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
aschran
force-pushed
the
aschran/ckpt-batch-0
branch
from
7731d23
to
6ced907
Compare
Adds `version_specific_data` to `CheckpointSummary` to keep track of which `RandomnessRound`s are present in a checkpoint. Batching is configurable by a minimum interval based on the commit timestamp.
aschran
force-pushed
the
aschran/ckpt-batch-0
branch
from
6ced907
to
198147b
Compare
aschran
force-pushed
the
aschran/ckpt-batch-0
branch
from
c6e029c
to
59129e8
Compare
aschran
force-pushed
the
aschran/ckpt-batch-0
branch
from
bbd70b8
to
ff5797a
Compare
aschran
requested review from
amnn,
wlmyng,
stefan-mysten,
emmazzz,
suiwombat and
mystenmark
as code owners
|
Semgrep found 1
Risk: Affected versions of vite are vulnerable to Improper Handling Of Case Sensitivity / Exposure Of Sensitive Information To An Unauthorized Actor / Improper Access Control. The vulnerability arises when the Vite development server's option, Manual Review Advice: A vulnerability from this advisory is reachable if you host vite's development server on Windows, and you rely on Fix: Upgrade this library to at least version 4.5.2 at sui/examples/trading/frontend/pnpm-lock.yaml:4700. Reference(s): GHSA-c24v-8rfc-w8vw, CVE-2023-34092, CVE-2024-23331 Ignore this finding from ssc-efa14576-9601-4ae6-939c-3da58aa25013. |
| None | Some(0) => None, | ||
| Some(1) => Some( | ||
| bcs::from_bytes(&self.version_specific_data).unwrap_or_else(|e| { | ||
| panic!( |
There was a problem hiding this comment.
since this is not on a CertifiedCheckpointSummary, we should probably return an error instead of panicking. Caller can panic if the summary was certified (and verified!)
| debug!( | ||
| checkpoint_commit_height = height, | ||
| "Making checkpoint at commit height" | ||
| ); | ||
| if let Err(e) = self.make_checkpoint(height, pending).await { | ||
| if let Err(e) = self | ||
| .make_checkpoint(std::mem::take(&mut grouped_pending_checkpoints)) |
There was a problem hiding this comment.
Yeah, it's a bit tricky to see through all the cases where grouped_pending_checkpoint still has leftovers. Maybe we should consider cleaning it up later.
| min_checkpoint_interval_ms: Option<u64>, | ||
|
|
||
| /// Version number to use for version_specific_data in `CheckpointSummary`. | ||
| checkpoint_summary_version_specific_data: Option<u64>, |
There was a problem hiding this comment.
Nit: would this be an enum in feature flag? (to get rid of all the unimplemented!()).
There was a problem hiding this comment.
I originally had it as a feature flag and @mystenmark asked me to use this style instead, other "version" settings in the protocol config also just use ints e.g. move_binary_format_version, gas_model_version, execution_version
| let version_specific_data = match protocol_config | ||
| .checkpoint_summary_version_specific_data_as_option() | ||
| { | ||
| None | Some(0) => Vec::new(), |
There was a problem hiding this comment.
So here (or somewhere), I think we also need to check that when checkpoint_summary_version_specific_data is < 0, min_checkpoint_interval_ms must be 0 . Otherwise, if checkpoint_summary_version_specific_data < 1, and min_checkpoint_interval_ms > 0, randomness rounds may be missed here.
Description
Adds
version_specific_datatoCheckpointSummaryto keep track of whichRandomnessRounds are present in a checkpoint.Batching is configurable by a minimum interval based on the commit timestamp.
Test plan
Unit/integration tests, manual testing in synthetic environment.
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.