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 1 commit 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.
|
⛔ Final review complete — 1 blocking finding(s) (commit 302f7ef) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4570 +/- ##
============================================
- Coverage 87.68% 87.22% -0.46%
============================================
Files 2778 2786 +8
Lines 359190 361994 +2804
============================================
+ Hits 314960 315767 +807
- Misses 44230 46227 +1997
🚀 New features to boost your workflow:
|
|
Warning Review limit reachedNext included review available in 56 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 (3)
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']
| /// True when a block with this timestamp is old enough that the node producing | ||
| /// it is clearly replaying history rather than following the tip. | ||
| pub fn is_historical_block(block_time_ms: u64) -> bool { | ||
| let now_ms = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|since_epoch| since_epoch.as_millis() as u64) | ||
| .unwrap_or(0); | ||
| now_ms.saturating_sub(block_time_ms) > HISTORICAL_BLOCK_AGE_MS | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn now_ms() -> u64 { | ||
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .expect("system clock is before the unix epoch") | ||
| .as_millis() as u64 | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_from_a_year_ago_is_historical() { | ||
| assert!(is_historical_block( | ||
| now_ms() - 365 * 24 * 60 * 60 * 1000 | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_from_a_minute_ago_is_not_historical() { | ||
| assert!(!is_historical_block(now_ms() - 60 * 1000)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_at_the_threshold_is_not_yet_historical() { | ||
| assert!(!is_historical_block(now_ms() - HISTORICAL_BLOCK_AGE_MS)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn a_block_timestamped_in_the_future_is_not_historical() { | ||
| assert!(!is_historical_block(now_ms() + 60 * 1000)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
| /// True when a block with this timestamp is old enough that the node producing | |
| /// it is clearly replaying history rather than following the tip. | |
| pub fn is_historical_block(block_time_ms: u64) -> bool { | |
| let now_ms = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .map(|since_epoch| since_epoch.as_millis() as u64) | |
| .unwrap_or(0); | |
| now_ms.saturating_sub(block_time_ms) > HISTORICAL_BLOCK_AGE_MS | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| fn now_ms() -> u64 { | |
| std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .expect("system clock is before the unix epoch") | |
| .as_millis() as u64 | |
| } | |
| #[test] | |
| fn a_block_from_a_year_ago_is_historical() { | |
| assert!(is_historical_block( | |
| now_ms() - 365 * 24 * 60 * 60 * 1000 | |
| )); | |
| } | |
| #[test] | |
| fn a_block_from_a_minute_ago_is_not_historical() { | |
| assert!(!is_historical_block(now_ms() - 60 * 1000)); | |
| } | |
| #[test] | |
| fn a_block_at_the_threshold_is_not_yet_historical() { | |
| assert!(!is_historical_block(now_ms() - HISTORICAL_BLOCK_AGE_MS)); | |
| } | |
| #[test] | |
| fn a_block_timestamped_in_the_future_is_not_historical() { | |
| assert!(!is_historical_block(now_ms() + 60 * 1000)); | |
| } | |
| } | |
| /// True when a block with this timestamp is old enough that the node producing | |
| /// it is clearly replaying history rather than following the tip. | |
| pub fn is_historical_block(block_time_ms: u64) -> bool { | |
| let now_ms = std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .map(|since_epoch| since_epoch.as_millis() as u64) | |
| .unwrap_or(0); | |
| is_historical_block_at(block_time_ms, now_ms) | |
| } | |
| fn is_historical_block_at(block_time_ms: u64, now_ms: u64) -> bool { | |
| now_ms.saturating_sub(block_time_ms) > HISTORICAL_BLOCK_AGE_MS | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| const NOW_MS: u64 = 2_000_000_000_000; | |
| #[test] | |
| fn a_block_from_a_year_ago_is_historical() { | |
| assert!(is_historical_block_at( | |
| NOW_MS - 365 * 24 * 60 * 60 * 1000, | |
| NOW_MS, | |
| )); | |
| } | |
| #[test] | |
| fn a_block_from_a_minute_ago_is_not_historical() { | |
| assert!(!is_historical_block_at(NOW_MS - 60 * 1000, NOW_MS)); | |
| } | |
| #[test] | |
| fn a_block_at_the_threshold_is_not_yet_historical() { | |
| assert!(!is_historical_block_at( | |
| NOW_MS - HISTORICAL_BLOCK_AGE_MS, | |
| NOW_MS, | |
| )); | |
| } | |
| #[test] | |
| fn a_block_timestamped_in_the_future_is_not_historical() { | |
| assert!(!is_historical_block_at(NOW_MS + 60 * 1000, NOW_MS)); | |
| } | |
| } |
source: ['claude', 'codex']
| mod serialization; | ||
| mod spawn; | ||
|
|
||
| pub use replay::is_historical_block; |
There was a problem hiding this comment.
🟡 Suggestion: 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.
| pub use replay::is_historical_block; | |
| pub(crate) use replay::is_historical_block; |
source: ['claude']
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