perf(drive-abci): don't create GroveDB checkpoints while replaying history - #4570
perf(drive-abci): don't create GroveDB checkpoints while replaying history#4570PastaPastaPasta wants to merge 2 commits into
Conversation
…story Protocol version 11 turns checkpoints on, with a policy of one every 600 seconds of chain time keeping the last 3. Mainnet blocks are about 2.5 minutes apart, so that is a checkpoint every four blocks — which at replay speed is roughly 37 a second, each one a RocksDB checkpoint over the whole database plus a copy of the platform state, and all but the last three deleted again immediately. Measured at 15.1 ms a block from height 318,704, against about 7 ms for everything else in a block put together. A finished mainnet sync had four checkpoint directories left on disk out of some 26,000 created. Checkpoints are restore points for a running node, so skip them for blocks more than ten minutes old and write the first real one on reaching the tip. They live outside the tree, so no app hash changes.
|
🕓 Queued for automated review — 33rd in line, estimated start in ~55 h (commit 9dfffa6)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4570 +/- ##
============================================
- Coverage 87.68% 85.59% -2.09%
============================================
Files 2778 2792 +14
Lines 359190 369992 +10802
============================================
+ Hits 314960 316705 +1745
- Misses 44230 53287 +9057
🚀 New features to boost your workflow:
|
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The checkpoint gate avoids replay-time I/O without changing authenticated state, but it confuses an old block timestamp with replay status. A fresh node that reaches a tip older than ten minutes can finish synchronization with no checkpoint, disabling checkpoint-backed address synchronization until another block arrives, so changes are required. The boundary test is also reproducibly flaky, and two smaller API and naming issues remain.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-5.6-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-5.6-sol (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed); agentphase1-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed); agentphase2-reviewer,gpt-5.6-sol— rust-quality (completed); agentphase2-reviewer
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs:62-63: A stale chain tip can leave a synchronized node with no checkpoint
Block age is not equivalent to replay status. If a fresh node catches up while the network tip is more than ten minutes old, such as during a consensus halt, this branch skips every checkpoint including the actual tip. No additional block-finalization callback runs when catch-up completes, so the promised first real checkpoint is never created until the network produces another block. This leaves a fully synchronized node unable to serve address full-tree synchronization: `prove_address_funds_trunk_query_v0` explicitly selects `GroveDBToUse::LatestCheckpoint`, whose GroveDB query returns `NoCheckpointsAvailable` when the registry is empty. Use an actual catch-up/tip signal or otherwise ensure completion creates a checkpoint instead of inferring synchronization state solely from the block timestamp.
- [NITPICK] packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs:151: Test name `test_first_block_should_always_checkpoint` no longer describes what it tests
The new age gate means a height-one historical block does not always checkpoint, as the companion test demonstrates. This test now verifies that a recent block with an empty checkpoint registry triggers a checkpoint, so its name should state those conditions rather than claim unconditional behavior.
In `packages/rs-drive-abci/src/utils/replay.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/utils/replay.rs:13-55: Exact-threshold test races against the system clock
The test computes its block timestamp from one `SystemTime::now()` call, while `is_historical_block` immediately samples the clock again. Crossing a millisecond boundary makes the measured age exceed `HISTORICAL_BLOCK_AGE_MS`, reversing the assertion. This was reproduced on the exact head, where the targeted test failed immediately. Extract the comparison into a helper with an injected current time so the boundary semantics are deterministic.
In `packages/rs-drive-abci/src/utils/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/utils/mod.rs:5: Keep the replay heuristic out of the crate's public API
`is_historical_block` has only one caller, the internal checkpoint implementation, but this public re-export exposes the ten-minute operational policy as `drive_abci::utils::is_historical_block` to downstream crates. That creates an unnecessary compatibility surface around an implementation detail. A crate-private re-export retains the required internal access.
| if crate::utils::is_historical_block(block_time) { | ||
| return Ok(None); |
There was a problem hiding this comment.
🔴 Blocking: A stale chain tip can leave a synchronized node with no checkpoint
Block age is not equivalent to replay status. If a fresh node catches up while the network tip is more than ten minutes old, such as during a consensus halt, this branch skips every checkpoint including the actual tip. No additional block-finalization callback runs when catch-up completes, so the promised first real checkpoint is never created until the network produces another block. This leaves a fully synchronized node unable to serve address full-tree synchronization: prove_address_funds_trunk_query_v0 explicitly selects GroveDBToUse::LatestCheckpoint, whose GroveDB query returns NoCheckpointsAvailable when the registry is empty. Use an actual catch-up/tip signal or otherwise ensure completion creates a checkpoint instead of inferring synchronization state solely from the block timestamp.
source: ['claude']
There was a problem hiding this comment.
Not changed in code. The limitation is documented in the replay.rs module doc on head 9dfffa61 (lines 8-11): block age is a proxy for catching up, and a node that finishes syncing during a network halt will treat the tip as historical and skip the checkpoint until the next block arrives. Whether that trade-off is acceptable needs maintainer acceptance, so this thread is left open.
🤖 Posted autonomously by Claude on behalf of pasta.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Review
Verdict: merge after changes. The idea is right and the measured win is large, but the branch as pushed fails cargo fmt --check, has a flaky unit test, and breaks an existing chain-simulation test that CI only runs after merge.
1. Correctness
Skipping checkpoints for blocks more than ten minutes old is safe for consensus: checkpoints live outside the tree and the app hash is unchanged, which the full replay confirms. A node interrupted mid-sync resumes without a checkpoint today as well.
Three problems I reproduced locally on 302f7eff:
cargo fmt --checkfails onutils/replay.rs. This is why "Rust workspace tests / Tests" is red: the job stops at the fmt step, so no Rust test has run in CI for this PR. (#4571 quietly carries the fmt fix, which is a sign the two branches got out of order.)a_block_at_the_threshold_is_not_yet_historicalis flaky. The test reads the clock, thenis_historical_blockreads it again; a millisecond tick in between flips the result. It failed on my first local run. Split the comparison into a pure function that takesnow_msand test that.run_chain_address_transitions_with_checkpoints_stop_and_restartnow fails (address_tests.rs:1007, "expected at least one checkpoint to be created"). The strategy harness starts every chain atGENESIS_TIME_MS(April 2023), so with this change every simulated block is historical and no checkpoint is ever taken. The PR run does not catch it because CI runsstrategy_testsin full only on push and nightly. Merging as is would turnv4.2-devred. The test needs a chain that starts near the wall clock.
One design limitation worth stating in the code and the PR body: block age is a proxy for "catching up". If the network tip itself is more than ten minutes old (a chain halt), a node that finishes syncing during the halt takes no checkpoint until the next block arrives, so address trunk queries return NoCheckpointsAvailable on that node until then. I think that is acceptable, since the whole network is degraded at that point, but it should be written down rather than discovered.
2. Clarity
The description is excellent: it names the activation height, the cadence, the I/O symptom, and the before/after numbers. The comments in should_checkpoint and replay.rs say why, not what.
3. Codebase standards
is_historical_block is re-exported pub from a pub mod utils, so a ten-minute internal policy becomes crate API. pub(crate) is enough (#4571 is the only other user and is in-crate). Otherwise idiomatic.
4. Importance and alternatives
35% of a full mainnet sync and a 60% per-block win in the affected range. The alternative is a real "am I caught up" signal from Tenderdash, which ABCI does not carry, so a timestamp heuristic is what is available. Keeping the predicate in one small pure function is the right shape.
5. Existing bot findings
- thepastaclaw "stale chain tip leaves no checkpoint": valid as a limitation, not a blocker. See above.
- thepastaclaw "threshold test races the clock": confirmed; will fix as suggested.
- thepastaclaw
pub(crate): agree.
I will push: fmt, the pure is_historical_block_at split with deterministic tests, pub(crate), the strategy test fix, and the limitation note. #4571 will need a rebase afterwards.
🤖 Posted autonomously by Claude on behalf of pasta.
…eplay skip The chain simulations start at a fixed 2023 timestamp, so with checkpoints skipped for historical blocks none of the seven checkpoint-enabled tests ever took one, and CI only runs them after merge. Give NetworkStrategy a start_time_ms and start those chains near the wall clock, at the same phase within the ten-minute interval so the asserted checkpoint heights are unchanged. Also: format replay.rs, test the age threshold against an injected clock instead of racing SystemTime, make is_historical_block crate-private, and write down the chain-halt limitation of the age heuristic.
|
Pushed 9dfffa6 on top of the original commit:
#4571 has been rebased onto this branch's new head. Ready for human review. 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
A node replaying mainnet history creates and destroys tens of thousands of GroveDB checkpoints.
Protocol version 11 (mainnet height 318,704) turns checkpoints on:
with
frequency_seconds: 600, num_checkpoints: 3. The interval is in chain time, and mainnet blocks are ~2.5 minutes apart, so that is a checkpoint every four blocks. At replay speed that is roughly 37 a second, each one a RocksDB checkpoint over the whole database — hard-linking every SST, flushing and copying the WAL — plus a copy of the platform state, and all but the last three deleted again immediately.Measured with per-block phase timing, replaying mainnet:
against roughly 7,000 µs for everything else in a block put together. A finished mainnet sync left four checkpoint directories on disk out of some 26,000 created:
The last three are four blocks apart, which is the cadence.
It shows up as an I/O stall rather than CPU: during the affected range drive-abci sat at 0.38 of one core with the system 47–73% idle and the disk at 2,000–4,000 tps.
What was done?
Skip checkpoint creation for blocks more than ten minutes old. Checkpoints are restore points for a running node; a node catching up has no use for restore points into blocks it is about to replace, and it writes its first real checkpoint on reaching the tip.
The age test is a new
utils::is_historical_block, with unit tests — the predicate is the testable part, since a historical replay makes every block historical by construction.Checkpoints live outside the tree, so no app hash changes.
How Has This Been Tested?
Same 3,000-block window from mainnet height 331,648, two local peers, back to back:
fb_checkpoint−60.8% per block. The 47.6 blocks/s reproduces what a full sync does over that range.
Full mainnet replay, genesis to 424,981, with this change on top of an otherwise identical build: 4,684.9 s → 3,032.9 s, 35% of the whole sync. Every committed app hash matched a reference sync across all 424,971 heights.
cargo test -p drive-abci --lib should_checkpoint utils::replay— 11 passed.One existing test needed updating:
test_first_block_should_always_checkpointbuilt its context withblock_time_ms = 1_000_000, which is 1970 and now reads as history. It uses a current timestamp, and a companion test covers the new behaviour for an old block.Breaking Changes
None for a node that is running. Once it reaches the tip it checkpoints exactly as it does today.
A node still catching up keeps no checkpoints, where before it kept three covering the last thirty minutes of chain time. Nothing during sync depends on them: a node that is interrupted picks up where it left off and carries on by itself, with no operator action, the same as it does today.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code