perf(drive-abci): stop rewriting the whole platform state every block - #4571
perf(drive-abci): stop rewriting the whole platform state every block#4571PastaPastaPasta wants to merge 5 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.
|
Warning Review limit reachedNext included review available in 51 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: Advanced Run ID: 📒 Files selected for processing (32)
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 |
|
✅ Final review complete — no blockers (commit 8e3a555) · triage: critical · Phase 2 only (queue backlog) |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Review
Verdict: merge after changes, and after #4570 lands (this is stacked on it and its base will move).
1. Correctness
I audited the dirty flag. Every mutation of the six heavy fields goes through a PlatformStateV0Methods setter or _mut accessor; there is no direct field access outside the module, and the only PlatformState { .. } constructors start dirty. set_current_protocol_version_in_consensus marking dirty is the important subtle case, because the protocol version selects the on-disk structure. All readers go through fetch_platform_state, which applies the small record only when its height is at least the full record's height; both are written in the same transaction, so they cannot disagree. The Option<u64> comparison handles the pre-first-block case (None >= None) correctly.
The two "do not borrow mutably when nothing moved" changes are equivalent to the old code. The read-only quorum comparison checks equal length plus every current quorum present with the same index, which means the sets are identical. The empty-diff early return in update_state_masternode_list skips only no-op operations.
Local verification on a319483d: cargo test -p drive-abci --lib passes, and the four stop_and_restart strategy tests pass (they run with store_platform_state: true and the 2023 genesis time, so they do exercise the skip-and-merge path).
The downgrade hazard is real and well described. Confining the skip to historical blocks is the right mitigation.
2. Clarity
Clear. The three-part "What was done" maps onto the diff. The comment blocks in store_platform_state and recent.rs explain the invariant a future reader needs: both records in one transaction, full record rewritten every block at the tip.
3. Codebase standards
Two deviations:
- The two new Drive methods are not versioned. Every other Drive method, including
store_platform_state_bytesright next to them, has amod.rsdispatcher over aFeatureVersioninrs-platform-versionand av0/implementation.store_platform_state_recent_bytesandfetch_platform_state_recent_bytesare plainimpl Drivefunctions inplatform_state/mod.rs. Please follow the convention. - The bincode config for the small record is duplicated in
store_platform_stateandfetch_platform_state. GivePlatformStateRecentserialize_to_bytes/deserializemethods and keep the config in one place.
Also: TryFrom<&PlatformState> duplicates TryFrom<PlatformState> field by field. If the owned conversion has no production caller left, delegate it to the borrowed one and drop the duplicate.
4. Importance and alternatives
1.3 ms of a ~7 ms block and 545 GB of write amplification over a sync. Splitting the record into a stable full part and a small per-block part, without changing the full record's format, is the least disruptive design; splitting the on-disk format itself would break older readers. No simpler approach avoids the per-block write, since block info changes every block.
5. Missing test
There is no focused test of the merge path: store with the dirty flag set, mutate block info, store again with the flag clear on a historical block, fetch, and assert the block info is new and the heavy fields intact. The restart strategy tests cover it indirectly but only run on push and nightly.
I will push: rebase onto the fixed #4570, versioned Drive methods, PlatformStateRecent (de)serialization helpers, the dedupe, and a merge test.
🤖 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.
The saved platform state is 1.28 MB on mainnet, almost all of it masternode lists, validator sets and quorum sets, and it was serialized and written to GroveDB aux storage on every block. Serialization also cloned the entire state first, so a block paid two full copies of some 4,000 masternodes. Serialization now builds the saving form from a borrowed state, and a test asserts the bytes are identical to the owned path. The heavy fields carry a dirty flag set by the accessors that can change them, and while replaying history the full record is rewritten only when it is set, with a small companion record holding the per-block fields written every block; both land in the block's transaction, and a database without the companion record reads exactly as before. Once at the tip the full record is written every block again, so an up-to-date node always has a complete record on disk. The two Core-driven update paths now decide read-only whether anything actually moved before taking a mutable borrow, because Core reports the same masternodes and quorums on most blocks and the borrow alone would force the rewrite.
a319483 to
82121d2
Compare
…ge and test its merge The two Drive methods for the per-block record now follow the crate's versioned dispatch like store_platform_state_bytes next to them, with feature versions in rs-platform-version. PlatformStateRecent owns its bincode config through serialize_to_bytes and deserialize, so store and fetch no longer repeat it. The owned PlatformStateForSavingV1 conversion delegates to the borrowed one instead of duplicating it field by field. A test stores a historical block that changed a heavy field, then one that did not, and reloads from disk: block info comes from the small record, the masternode from the full one.
|
Rebased onto the new head of #4570 (9dfffa6) and pushed one commit on top of the original:
One thing to be aware of: the Tests workflow only triggers for PRs whose base is Ready for human review once #4570 is in; this PR should be merged after it. 🤖 Posted autonomously by Claude on behalf of pasta. |
82121d2 to
af95f25
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4571 +/- ##
============================================
- Coverage 87.68% 84.46% -3.22%
============================================
Files 2778 2800 +22
Lines 359190 376737 +17547
============================================
+ Hits 314960 318221 +3261
- Misses 44230 58516 +14286
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied Phase-2 findings against exact head 8e3a555. Three non-blocking suggestions remain: two new regression tests have coverage blind spots, and the new dirty-tracking invariant is not enforced by field visibility. Source inspection and git diff --check completed successfully; the reviewers' reported test runs were not independently rerun.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This changes consensus-relevant platform-state persistence, dirty-field tracking, serialization and replay checkpoint behavior, where missed updates or inconsistent record recovery could cause state divergence, failed restarts or downgrade incompatibility. - Phase 1 reviewers: not run (skipped for throughput: 27 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🟡 3 suggestion(s)
🤖 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`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs:206-208: Enable checkpoints in the historical-block regression test
The default test configuration sets disable_checkpoints to true, and the crate's dev-dependency enables testing-config. This fixture therefore returns Ok(None) at the initial configuration guard, before reaching the historical-block predicate. Removing the replay-skip check would leave this test passing. Explicitly enable checkpoints, as the neighboring first-block test does, so this assertion exercises the new behavior.
In `packages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/block_end/update_state_cache/v0/mod.rs:290-293: Exercise a clean-to-dirty transition in the persistence test
The newly constructed platform state already has heavy_fields_dirty = true, and set_genesis_state does not clear it. The assertion after full_masternode_list_mut() therefore does not verify that this accessor marks the state dirty: removing its dirty-bit assignment would still allow block 7 to write in full and block 8 to exercise the small-record merge. Add a third historical block that mutates the heavy collection after block 8 has left the state clean, then verify that the full record advances and the mutation survives reload. This preserves the existing merge coverage while also testing the dirty transition that prevents lost updates.
In `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:67-75: Encapsulate the fields that participate in dirty tracking
Historical-block persistence now skips the full record when heavy_fields_dirty is false, but the heavy collections, current protocol version, and dirty flag remain public. Directly mutating a collection can therefore bypass the tracked accessors and leave the state eligible to skip a required write; before this PR, the unconditional full write persisted such mutations. The inspected production mutation paths use the tracked accessors, so this is an API-invariant concern rather than a confirmed lost update in a current caller. Make the tracked fields and flag private, retain read and tracked-mutation accessors, and provide a narrowly scoped method for the persistence layer to mark the state clean.
| let platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_genesis_state(); |
There was a problem hiding this comment.
🟡 Suggestion: Enable checkpoints in the historical-block regression test
The default test configuration sets disable_checkpoints to true, and the crate's dev-dependency enables testing-config. This fixture therefore returns Ok(None) at the initial configuration guard, before reaching the historical-block predicate. Removing the replay-skip check would leave this test passing. Explicitly enable checkpoints, as the neighboring first-block test does, so this assertion exercises the new behavior.
| let platform = TestPlatformBuilder::new() | |
| .build_with_mock_rpc() | |
| .set_genesis_state(); | |
| let platform = TestPlatformBuilder::new() | |
| .with_config(crate::config::PlatformConfig { | |
| testing_configs: crate::config::PlatformTestConfig { | |
| disable_checkpoints: false, | |
| ..Default::default() | |
| }, | |
| ..Default::default() | |
| }) | |
| .build_with_mock_rpc() | |
| .set_genesis_state(); |
source: ['claude']
| block_platform_state | ||
| .full_masternode_list_mut() | ||
| .insert(pro_tx_hash, masternode); | ||
| assert!(block_platform_state.heavy_fields_dirty); |
There was a problem hiding this comment.
🟡 Suggestion: Exercise a clean-to-dirty transition in the persistence test
The newly constructed platform state already has heavy_fields_dirty = true, and set_genesis_state does not clear it. The assertion after full_masternode_list_mut() therefore does not verify that this accessor marks the state dirty: removing its dirty-bit assignment would still allow block 7 to write in full and block 8 to exercise the small-record merge. Add a third historical block that mutates the heavy collection after block 8 has left the state clean, then verify that the full record advances and the mutation survives reload. This preserves the existing merge coverage while also testing the dirty transition that prevents lost updates.
source: ['claude']
| pub previous_fee_versions: CachedEpochIndexFeeVersions, | ||
|
|
||
| /// True when a field carried only by the full saved record has changed since | ||
| /// the state was last written in full. The masternode lists, validator sets | ||
| /// and quorum sets are over a megabyte on mainnet and change on a minority of | ||
| /// blocks, so the full record is rewritten only when this is set; every block | ||
| /// still writes the small record holding the block info and quorum hashes. | ||
| /// Not part of the saved record: a state read back from disk starts dirty. | ||
| pub heavy_fields_dirty: bool, |
There was a problem hiding this comment.
🟡 Suggestion: Encapsulate the fields that participate in dirty tracking
Historical-block persistence now skips the full record when heavy_fields_dirty is false, but the heavy collections, current protocol version, and dirty flag remain public. Directly mutating a collection can therefore bypass the tracked accessors and leave the state eligible to skip a required write; before this PR, the unconditional full write persisted such mutations. The inspected production mutation paths use the tracked accessors, so this is an API-invariant concern rather than a confirmed lost update in a current caller. Make the tracked fields and flag private, retain read and tracked-mutation accessors, and provide a narrowly scoped method for the persistence layer to mark the state clean.
source: ['claude']
2 participants
Issue being fixed or feature implemented
The saved platform state is written to GroveDB aux storage on every block, and on mainnet it is 1.28 MB — almost all of it masternode lists, validator sets and the chain-lock and instant-lock quorum sets.
PlatformSerializable::serialize_to_bytesalso didself.clone()before converting to the saving form, so a block paid two full deep copies of some 4,000 masternodes plus the validator sets, then serialized 1.28 MB, then wrote it.Measured replaying mainnet, per block: 1.08 ms serializing, 0.23 ms for the extra clone, and 1.28 MB into the block's transaction — 545 GB of writes over a full sync.
Stacked on #4570, which introduces the
utils::is_historical_blockpredicate this uses.What was done?
Three things.
Serialize from a borrowed state. A
TryFrom<&PlatformState>for the saving form clones each field once instead of cloning the whole state first. A test asserts the bytes are byte-identical to the owned path.Rewrite the full record only when it changed. The heavy fields carry a dirty flag, set by the accessors that can change them. While replaying history the full record is written only when the flag is set; a small companion record under a second aux key carries the per-block fields — block info, quorum hashes, protocol versions — every block. Both are written in the block's transaction, so a reader never sees them disagree, and a database without the companion record reads exactly as before.
Once the node is at the tip the full record is written every block again, so an up-to-date node always has a complete record on disk and an older drive-abci can still read it. Skipping is confined to a node that is catching up, where the remedy for any format trouble is the resync it is already doing.
Don't take a mutable borrow when nothing moved. Core reports the same quorums on most blocks and an empty masternode diff often, but
update_quorum_infoandupdate_state_masternode_listreached for_mut()accessors regardless — and the borrow alone marks the state dirty. Both now decide read-only whether anything actually changed first.How Has This Been Tested?
Full mainnet replay, genesis to 424,981. Every committed app hash matched a reference sync across all 424,971 heights, and the final app hash matched exactly.
Crash recovery specifically: two mid-sync restarts, at heights 40,200 and 80,188. Both resumed at the right height from the companion record and continued with no app-hash mismatch — which requires the heavy fields to be correct too, since they feed the app hash through masternode identity updates and rewards.
cargo test -p drive-abci --lib— 2,776 passed, including the new byte-identity test.Breaking Changes
None for normal operation, an interrupted sync included. A node that stops mid-sync restarts and carries on by itself with no operator action: the companion record restores the height, and the heavy fields are unchanged since the full record was written. Two mid-sync restarts are part of the testing above.
The one case that needs care is a deliberate downgrade. A node interrupted mid-initial-sync leaves a full record that lags the database, and an older drive-abci reading that database panics in the
Infohandler, which compares the saved state's app hash against GroveDB's root hash — confirmed by running a pre-change binary against a database this one wrote. That is why the skip is confined to historical blocks: a node that has caught up has a complete record on disk and can always be rolled back. A node caught mid-sync would need to finish syncing on the new build, or resync on the old one.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code