feat(drive-abci): state sync via ABCI snapshots with reduced platform state (protocol v15) - #4520
feat(drive-abci): state sync via ABCI snapshots with reduced platform state (protocol v15)#4520PastaPastaPasta wants to merge 34 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
PastaPastaPasta
changed the title
|
QA: replay-equivalence + fault-injection workstream (fixes landed in What was proven:
Three bugs found and fixed (all in this PR now):
Documented, deliberately not fixed here: 🤖 Posted autonomously by Claude on behalf of pasta. |
|
Live end-to-end validation is green. With this PR's fixes plus the companion changes, a fresh node joining a running local network completes a genuine state sync — 14 passing / 0 failing across join, proof-verified state serving, all-validators churn mid-join, re-sync after a platform-data wipe, and correct block-sync fallback when no snapshots are served. Both previously-fatal live bugs (chunk verification failure; the 🤖 Posted autonomously by Claude on behalf of pasta. |
|
⛔ Final review complete — 5 blocking finding(s) (commit 7c36bf8) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
The state-sync implementation has nine in-scope blockers at the exact reviewed head: the pinned GroveDB revision rejects valid Platform snapshots, restoration uses the fresh node's obsolete GroveDB version table, reconstructed platform state omits or insufficiently validates consensus-relevant quorum data, and several snapshot lifecycle paths can prevent recovery or retention from working as configured. The v15 reconstruction behavior also modifies existing v0 consensus implementations in place, and the evidence-window constants still require the release decision already identified in the PR description.
Source: Claude reviewers sol-fallback-reviewer-general, sol-fallback-reviewer-security-auditor, and sol-fallback-reviewer-rust-quality (exact backend model IDs were not provided in the supplied evidence); Codex reviewer (exact backend model ID not provided; no findings); final verifier Claude (exact backend model ID not exposed to this runtime).
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-rust-quality-4875e827540b4af6af21944d736206cd(completed),codex-general-fbff72220b1d401080d9f21152a37fc8(failed),codex-general-11305c123b69465f8d9ef3d7b0146a09(failed),codex-security-auditor-1c2bab2976c140aea4c32b11dbc07654(failed),codex-security-auditor-3ec85a9a996d4e94b1789e03e3ac7cd0(failed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
🔴 9 blocking | 🟡 1 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/state_sync/reconstruct_platform_state/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_sync/reconstruct_platform_state/mod.rs:90-97: State sync drops the previous chain-lock and instant-lock quorum sets
Reconstruction creates both signature-verification quorum sets empty and invokes `update_core_info` with `platform_state = None`. Both quorum update branches interpret the absent prior state as initialization: `update_quorums_from_validator_set` calls `set_current_quorums`, while `update_quorums_from_quorum_list` never calls `set_previous_past_quorums`. The restored node therefore loses the previous quorum sets and their activation/change heights. `select_quorums` uses exactly that history because lock signatures target the quorum active at `SIGN_OFFSET` blocks before the lock height, so a snapshot restored shortly after a quorum-list change can reject a lock that a replayed node accepts. GroveDB root verification cannot detect this divergence because these quorum sets live only in platform state. Persist the required history in reduced state or deterministically reconstruct the relevant prior Core state, and test restoration inside the post-rotation verification window.
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_sync/reconstruct_platform_state/mod.rs:158-163: Reconstruction accepts a validator-set list that differs from the snapshot
`saved.quorum_positions` is the consensus-covered ordered list of validator-set hashes from the snapshot source, but reconstruction treats it only as a sorting hint. `sort_validator_sets_by_saved_positions` leaves missing saved hashes absent and places unexpected Core-provided hashes last with `usize::MAX`. Validator sets are held in platform state rather than GroveDB, so the final app-hash check cannot reveal this mismatch. Require an exact hash-set match between the reconstructed validator sets and `saved.quorum_positions` before sorting or publishing the restored state; otherwise reject the snapshot.
In `packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rs:49-53: Custom checkpoint directories are not reloaded after restart
Checkpoint creation now writes under the configured `CHECKPOINTS_PATH`, but startup remains hard-coded to `<db_path>/checkpoints`: `Drive::open` calls `load_current_checkpoints(db_path)`, and `Platform::open_with_client` loads each `platform_state.bin` from `config.db_path.join("checkpoints")`. After restarting with a custom path, the checkpoint registry is empty, retained snapshots are no longer advertised, and subsequent retention calculations cannot prune the old external directories. Thread the resolved checkpoint path through checkpoint discovery and platform-state loading, with restart coverage for a non-default path.
In `packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs:34-49: Oversized peer chunks abort the ABCI request instead of rejecting the sender
The caps correctly run before decoding, but an oversized chunk or chunk ID returns an application error. That becomes an ABCI exception rather than the recoverable `RETRY`/`RETRY_SNAPSHOT` response used for other malformed chunks, and it neither bans `request.sender` nor clears/restarts the already-wiped restore session. A selected peer can therefore make a recoverable transfer fault abort state sync and leave the restore sentinel active. Oversized chunk data should reject the sender and request a refetch; an intrinsically invalid chunk ID should restart or reject the snapshot without throwing an application exception.
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs:52-53: Snapshot chunks are verified with the fresh node's stale GroveDB version
The consumer selects `grove_version` from its current in-memory platform state. A production node without saved state initializes at protocol version 1, whose Drive table uses `GROVE_V1`, while snapshots are restorable only from protocol v15 and use Drive v9 with `GROVE_V4`. `start_snapshot_syncing`, `apply_chunk`, `commit_session`, `verify_grovedb`, and the final root lookup all receive the target's old table, whereas the source generates chunks with its current newer table. GroveDB replication, tree opening, root calculation, and restore operations are version-gated, so generation and verification are not using the same rules. Carry the snapshot's Platform protocol version in authenticated/validated snapshot metadata or otherwise determine it before restoration, retain it in `SnapshotFetchingSession`, and use its exact GroveDB table consistently on both serving and consuming paths.
In `packages/rs-drive-abci/src/platform_types/snapshot/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/platform_types/snapshot/mod.rs:202-225: Expired checkpoint pins can remain on disk indefinitely
The advertised inactivity TTL is enforced only when a later `pin_for_serving` call happens. An abandoned transfer with no subsequent requests retains its `Arc<Checkpoint>` indefinitely, preventing a pruned checkpoint directory from being deleted. In addition, `pinned_checkpoint` ignores the stored timestamp, so a request can clone an expired, already-pruned checkpoint before `pin_for_serving` performs cleanup and immediately refresh it. By periodically touching pinned heights, a peer can defeat `MAX_NUM_SNAPSHOTS` and retain an unbounded sequence of checkpoint directories. Expiration must be checked before returning a pin, and stale pins need periodic or otherwise autonomous cleanup rather than relying only on unrelated future chunk requests.
In `packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs:66-82: Peer-controlled snapshot height prevents fallback to an older snapshot
The light client authenticates `RequestOfferSnapshot.app_hash`, but the peer-supplied snapshot descriptor controls `snapshot.height`. If a high snapshot is accepted and its chunks later become unavailable, Tenderdash can offer another available snapshot at a lower height. This branch returns an ABCI exception instead of replacing the stale session, while the session is otherwise removed only after transfer completion. A peer can therefore advertise a higher snapshot, withhold chunks, and prevent fallback to an honest peer's older checkpoint. Treat every subsequent accepted-format offer as Tenderdash resetting the transfer and replace the current session regardless of the previous untrusted height.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/state_sync_tests.rs`:
- [BLOCKING] packages/rs-drive-abci/tests/strategy_tests/test_cases/state_sync_tests.rs:373-377: The pinned GroveDB revision cannot complete a real state sync
The exact reviewed head pins GroveDB revision `6c882c3`, and the only end-to-end successful state-sync test is ignored because that revision restores Platform SumTrees with latent corruption. This is confirmed by the active `state_sync_transfer_detects_sum_tree_restore_defect` test, which expects an otherwise normal snapshot to be rejected after strict verification, and by `tests/sum_tree_sync_probe.rs`. The rejection is correct, but it means the PR's primary feature cannot successfully restore a normal Platform snapshot at this head. Pin a GroveDB revision containing dashpay/grovedb#840 and enable the successful round-trip test before merging.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/v0/mod.rs:44-59: State-sync reconstruction changes an existing v0 implementation in place
This changes the live `update_masternode_list_v0` behavior so `is_init_chain` also means reconstruction from scratch, bypassing its same-height short circuit. The PR likewise changes `update_core_info_v0` to forward that flag into quorum reconstruction, and `reconstruct_platform_state` passes `true` even though it is not executing InitChain. Protocol v15 still dispatches both methods to version 0, so this alters existing protocol behavior in place and conflates two distinct modes behind one boolean. Preserve v0 behavior and add versioned core/masternode update implementations or a separately versioned reconstruction path selected only by the v15 method table.
In `packages/rs-drive-abci/src/execution/engine/consensus_params_update/v2/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/engine/consensus_params_update/v2/mod.rs:13-21: The block bound collapses the stated 20-day evidence window to one day
Tenderdash expires evidence when either age bound is exceeded. At the documented approximately six-second block interval, 15,000 blocks is about 25 hours, so the configured 20-day duration never provides a 20-day accountability window. This inconsistency is explicitly acknowledged in the source and PR description as requiring a decision before release. Resolve that policy before v15 activation: either increase the block bound to match the intended duration or document and approve the approximately one-day effective evidence window.
| chain_lock_validating_quorums: SignatureVerificationQuorumSet::new( | ||
| &self.config.chain_lock, | ||
| state_platform_version, | ||
| )?, | ||
| instant_lock_validating_quorums: SignatureVerificationQuorumSet::new( | ||
| &self.config.instant_lock, | ||
| state_platform_version, | ||
| )?, |
There was a problem hiding this comment.
🔴 Blocking: State sync drops the previous chain-lock and instant-lock quorum sets
Reconstruction creates both signature-verification quorum sets empty and invokes update_core_info with platform_state = None. Both quorum update branches interpret the absent prior state as initialization: update_quorums_from_validator_set calls set_current_quorums, while update_quorums_from_quorum_list never calls set_previous_past_quorums. The restored node therefore loses the previous quorum sets and their activation/change heights. select_quorums uses exactly that history because lock signatures target the quorum active at SIGN_OFFSET blocks before the lock height, so a snapshot restored shortly after a quorum-list change can reject a lock that a replayed node accepts. GroveDB root verification cannot detect this divergence because these quorum sets live only in platform state. Persist the required history in reduced state or deterministically reconstruct the relevant prior Core state, and test restoration inside the post-rotation verification window.
source: ['claude']
There was a problem hiding this comment.
Resolved in 9eb0869 — State sync drops the previous chain-lock and instant-lock quorum sets no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Core RPC returns quorums in an order that need not match the incremental | ||
| // order the source node maintained; restore the recorded order. | ||
| sort_validator_sets_by_saved_positions( | ||
| platform_state.validator_sets_mut(), | ||
| &saved.quorum_positions, | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Reconstruction accepts a validator-set list that differs from the snapshot
saved.quorum_positions is the consensus-covered ordered list of validator-set hashes from the snapshot source, but reconstruction treats it only as a sorting hint. sort_validator_sets_by_saved_positions leaves missing saved hashes absent and places unexpected Core-provided hashes last with usize::MAX. Validator sets are held in platform state rather than GroveDB, so the final app-hash check cannot reveal this mismatch. Require an exact hash-set match between the reconstructed validator sets and saved.quorum_positions before sorting or publishing the restored state; otherwise reject the snapshot.
source: ['claude']
There was a problem hiding this comment.
Resolved in 9eb0869 — Reconstruction accepts a validator-set list that differs from the snapshot no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Build the checkpoint path: <checkpoints_path>/<block_height> | ||
| // (defaults to db_path/checkpoints) | ||
| let checkpoint_path = state_sync_config | ||
| .resolved_checkpoints_path(&self.config.db_path) | ||
| .join(block_height.to_string()); |
There was a problem hiding this comment.
🔴 Blocking: Custom checkpoint directories are not reloaded after restart
Checkpoint creation now writes under the configured CHECKPOINTS_PATH, but startup remains hard-coded to <db_path>/checkpoints: Drive::open calls load_current_checkpoints(db_path), and Platform::open_with_client loads each platform_state.bin from config.db_path.join("checkpoints"). After restarting with a custom path, the checkpoint registry is empty, retained snapshots are no longer advertised, and subsequent retention calculations cannot prune the old external directories. Thread the resolved checkpoint path through checkpoint discovery and platform-state loading, with restart coverage for a non-default path.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — Custom checkpoint directories are not reloaded after restart no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Cap peer-supplied sizes before anything decodes them (issue #3773) | ||
| if request.chunk_id.len() > MAX_STATE_SYNC_CHUNK_ID_SIZE { | ||
| return Err(AbciError::StateSyncBadRequest(format!( | ||
| "apply_snapshot_chunk chunk id of {} bytes exceeds the {} byte limit", | ||
| request.chunk_id.len(), | ||
| MAX_STATE_SYNC_CHUNK_ID_SIZE | ||
| )) | ||
| .into()); | ||
| } | ||
| if request.chunk.len() > MAX_STATE_SYNC_CHUNK_SIZE { | ||
| return Err(AbciError::StateSyncBadRequest(format!( | ||
| "apply_snapshot_chunk chunk of {} bytes exceeds the {} byte limit", | ||
| request.chunk.len(), | ||
| MAX_STATE_SYNC_CHUNK_SIZE | ||
| )) | ||
| .into()); |
There was a problem hiding this comment.
🔴 Blocking: Oversized peer chunks abort the ABCI request instead of rejecting the sender
The caps correctly run before decoding, but an oversized chunk or chunk ID returns an application error. That becomes an ABCI exception rather than the recoverable RETRY/RETRY_SNAPSHOT response used for other malformed chunks, and it neither bans request.sender nor clears/restarts the already-wiped restore session. A selected peer can therefore make a recoverable transfer fault abort state sync and leave the restore sentinel active. Oversized chunk data should reject the sender and request a refetch; an intrinsically invalid chunk ID should restart or reject the snapshot without throwing an application exception.
source: ['claude']
There was a problem hiding this comment.
Resolved in 6fe8c7e — Oversized peer chunks abort the ABCI request instead of rejecting the sender no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// Pins a checkpoint that is being served (or refreshes the pin of one that already | ||
| /// is), and drops pins whose transfers have been inactive for longer than the TTL. | ||
| pub fn pin_for_serving(&self, height: u64, checkpoint: Arc<Checkpoint>) { | ||
| let now = Instant::now(); | ||
| let mut pins = self | ||
| .serving_pins | ||
| .write() | ||
| .expect("serving pins lock poisoned"); | ||
| pins.retain(|_, (_, last_served)| { | ||
| now.saturating_duration_since(*last_served) < SERVING_PIN_INACTIVITY_TTL | ||
| }); | ||
| pins.insert(height, (checkpoint, now)); | ||
| } | ||
|
|
||
| /// Returns a pinned checkpoint for the given height, if the pin is still held. | ||
| /// | ||
| /// Used to keep serving a snapshot whose checkpoint pruning has already dropped | ||
| /// from the registry. | ||
| pub fn pinned_checkpoint(&self, height: u64) -> Option<Arc<Checkpoint>> { | ||
| self.serving_pins | ||
| .read() | ||
| .expect("serving pins lock poisoned") | ||
| .get(&height) | ||
| .map(|(checkpoint, _)| Arc::clone(checkpoint)) |
There was a problem hiding this comment.
🔴 Blocking: Expired checkpoint pins can remain on disk indefinitely
The advertised inactivity TTL is enforced only when a later pin_for_serving call happens. An abandoned transfer with no subsequent requests retains its Arc<Checkpoint> indefinitely, preventing a pruned checkpoint directory from being deleted. In addition, pinned_checkpoint ignores the stored timestamp, so a request can clone an expired, already-pruned checkpoint before pin_for_serving performs cleanup and immediately refresh it. By periodically touching pinned heights, a peer can defeat MAX_NUM_SNAPSHOTS and retain an unbounded sequence of checkpoint directories. Expiration must be checked before returning a pin, and stale pins need periodic or otherwise autonomous cleanup rather than relying only on unrelated future chunk requests.
source: ['claude']
There was a problem hiding this comment.
Resolved in 6fe8c7e — Expired checkpoint pins can remain on disk indefinitely no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if let Some(session) = session_write_guard.as_ref() { | ||
| // An offer at the same height is a legitimate snapshot restart (Tenderdash's | ||
| // RETRY_SNAPSHOT flow) and replaces the session; only strictly older offers are | ||
| // rejected. | ||
| if offered_snapshot.height < session.snapshot.height { | ||
| return Err(AbciError::StateSyncBadRequest(format!( | ||
| "offer_snapshot already syncing snapshot at height {}, offered height {} is older", | ||
| session.snapshot.height, offered_snapshot.height | ||
| )) | ||
| .into()); | ||
| } | ||
| tracing::warn!( | ||
| current_height = session.snapshot.height, | ||
| offered_height = offered_snapshot.height, | ||
| "[state_sync] offer_snapshot replacing session in progress", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Peer-controlled snapshot height prevents fallback to an older snapshot
The light client authenticates RequestOfferSnapshot.app_hash, but the peer-supplied snapshot descriptor controls snapshot.height. If a high snapshot is accepted and its chunks later become unavailable, Tenderdash can offer another available snapshot at a lower height. This branch returns an ABCI exception instead of replacing the stale session, while the session is otherwise removed only after transfer completion. A peer can therefore advertise a higher snapshot, withhold chunks, and prevent fallback to an honest peer's older checkpoint. Treat every subsequent accepted-format offer as Tenderdash resetting the transfer and replace the current session regardless of the previous untrusted height.
source: ['claude']
There was a problem hiding this comment.
Resolved in 6fe8c7e — Peer-controlled snapshot height prevents fallback to an older snapshot no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let platform_version = app.platform().state.load().current_platform_version()?; | ||
| let grove_version = &platform_version.drive.grove_version; |
There was a problem hiding this comment.
🔴 Blocking: Snapshot chunks are verified with the fresh node's stale GroveDB version
The consumer selects grove_version from its current in-memory platform state. A production node without saved state initializes at protocol version 1, whose Drive table uses GROVE_V1, while snapshots are restorable only from protocol v15 and use Drive v9 with GROVE_V4. start_snapshot_syncing, apply_chunk, commit_session, verify_grovedb, and the final root lookup all receive the target's old table, whereas the source generates chunks with its current newer table. GroveDB replication, tree opening, root calculation, and restore operations are version-gated, so generation and verification are not using the same rules. Carry the snapshot's Platform protocol version in authenticated/validated snapshot metadata or otherwise determine it before restoration, retain it in SnapshotFetchingSession, and use its exact GroveDB table consistently on both serving and consuming paths.
source: ['claude']
There was a problem hiding this comment.
Resolved in 6fe8c7e — Snapshot chunks are verified with the fresh node's stale GroveDB version no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// REVIEW BEFORE RELEASE: at ~6s blocks, 15_000 blocks is roughly one day, while | ||
| /// [`V15_EVIDENCE_MAX_AGE_DURATION_SECONDS`] below is 20 days. Evidence expires when | ||
| /// EITHER bound is exceeded, so the effective window is the smaller (~1 day) — the two | ||
| /// values from #2512 look inconsistent and need to be confirmed before this ships. | ||
| const V15_EVIDENCE_MAX_AGE_NUM_BLOCKS: i64 = 15_000; | ||
|
|
||
| /// Maximum evidence age in time: 20 days, per issue #2512. See the review note on | ||
| /// [`V15_EVIDENCE_MAX_AGE_NUM_BLOCKS`]. | ||
| const V15_EVIDENCE_MAX_AGE_DURATION_SECONDS: i64 = 20 * 24 * 60 * 60; |
There was a problem hiding this comment.
🟡 Suggestion: The block bound collapses the stated 20-day evidence window to one day
Tenderdash expires evidence when either age bound is exceeded. At the documented approximately six-second block interval, 15,000 blocks is about 25 hours, so the configured 20-day duration never provides a 20-day accountability window. This inconsistency is explicitly acknowledged in the source and PR description as requiring a decision before release. Resolve that policy before v15 activation: either increase the block bound to match the intended duration or document and approve the approximately one-day effective evidence window.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — The block bound collapses the stated 20-day evidence window to one day no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
All nine findings triaged (plus two forwarded from the #4521/#4530 reviews that belonged to this branch); seven confirmed and fixed in Confirmed + fixed:
Rebutted:
The evidence-window suggestion is deliberately left as the flagged "REVIEW BEFORE RELEASE" decision in Gates: fmt/check clean; state-sync suite 11 passing (+1 new custom-path test); full strategy suite 107 passing; drive-abci lib 2,790; drive 3,491; dpp 4,107 — zero failures. 🤖 Posted autonomously by Claude on behalf of pasta. |
|
@coderabbitai review |
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Sol-only technical fallback
State sync remains not merge-ready: the production serving manager does not autonomously expire pins, the pinned GroveDB revision rejects normal Platform snapshots, two existing v0 methods were modified in place, and restored nodes cannot immediately serve valid DAPI queries or proofs. Seven prior defects are fixed; five blocking findings and one performance suggestion remain. Source: Claude reviewer lanes and Codex checkpoint (exact backend model IDs were not supplied); final verifier: Claude (exact backend model ID was not supplied).
Source: reviewer 1: gpt-5.6-sol (agent: sol-fallback-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: sol-fallback-reviewer, role: security-auditor); reviewer 3: gpt-5.6-sol (agent: sol-fallback-reviewer, role: rust-quality); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)
One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.
Review provenance
- Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
- GLM failure attempts:
codex-general-80dfcd456e9d49b6a28616afe5096c75(failed),codex-general-aa50961fcb2e491b83e0fa3620cfe5b7(failed),codex-rust-quality-352974b42d134185b8657b29dbecdbb4(failed),codex-rust-quality-67d1ba0c15fd4689a27b30e70aa451f6(failed),codex-security-auditor-f30aeefbe45044cba4f7f7e7a89c8701(failed),codex-security-auditor-4e45682b19cd4129bdaecc3f27d6de9a(failed) - Sol-only fallback reasons:
launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit,launch_transport_or_nonzero_exit - Sol-only fallback reviewers:
gpt-5.6-sol— general (completed); agentsol-fallback-reviewer,gpt-5.6-sol— security-auditor (completed); agentsol-fallback-reviewer,gpt-5.6-sol— rust-quality (completed); agentsol-fallback-reviewer - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier; agentsol-verifier - Additional Phase 2 pass: not run; the Sol-only fallback is final
🔴 3 blocking | 🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/abci/app/check_tx.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/app/check_tx.rs:53-58: Expired checkpoint pins can remain on disk indefinitely
Production snapshot requests are served by the `CheckTxAbciApplication` constructed in `server.rs`, and this application owns a private `SnapshotManager`. The once-per-block `release_expired_pins` call is attached to `FullAbciApplication::finalize_block`, but production uses `ConsensusAbciApplication` for consensus and never finalizes blocks through that separate manager. An abandoned transfer therefore leaves its checkpoint `Arc` alive until another serving request or process shutdown, allowing already-pruned full-state directories to outlive both advertised deadlines. Share the serving manager with a component receiving block or timer callbacks, or run an autonomous expiry task against the CheckTx manager itself.
- [SUGGESTION] packages/rs-drive-abci/src/abci/app/check_tx.rs:121-127: Snapshot chunk generation blocks Tokio worker threads
`load_snapshot_chunk` is an async tonic handler but invokes the synchronous handler directly on a Tokio worker. That path reads checkpoint metadata, traverses RocksDB/Merk trees, generates a replication chunk, and encodes its operations. Because requests are peer-controlled, concurrent snapshot consumers can occupy runtime workers and delay unrelated gRPC and DAPI traffic. Move snapshot database work to the blocking pool, following the adjacent `check_tx` pattern, with shared ownership of the serving `SnapshotManager`.
In `packages/rs-drive-abci/src/execution/platform_events/state_sync/reconstruct_platform_state/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_sync/reconstruct_platform_state/mod.rs:241-257: State sync leaves the query height guard at the fresh node's height
`update_state_cache` publishes the reconstructed state at the snapshot height, but reconstruction never updates `Platform::committed_block_height_guard`, which remains zero on a fresh node. `QueryService` refuses to execute while the state height differs from this guard. Its inner wait counter is also reinitialized on every iteration, so a query started in this state can wait forever rather than reaching the intended timeout; if no later block finalizes, all queries remain unserviceable. Store the reconstructed committed height in the guard only after GroveDB reconstruction, aux-state persistence, and final app-hash verification have succeeded.
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_sync/reconstruct_platform_state/mod.rs:132-145: Reconstruction persists zero-valued block proof metadata
The per-block reduced state deliberately stores no block ID or quorum signature, and reconstruction substitutes all-zero values before persisting the resulting `PlatformState` to aux storage. `response_proof_v0` copies those fields into every current-state DAPI proof, while `rs-drive-proof-verifier` incorporates the block ID into the signed vote and explicitly rejects an all-zero signature. After a restart, the query guard is initialized from this persisted state, so the node can serve proofs that cannot authenticate until a later block finalizes. Carry authenticated block proof metadata with the snapshot or retrieve and validate it from Tenderdash before publishing and persisting the restored state.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/state_sync_tests.rs`:
- [BLOCKING] packages/rs-drive-abci/tests/strategy_tests/test_cases/state_sync_tests.rs:375-379: The pinned GroveDB revision cannot complete a real state sync
(existing thread: https://github.com/dashpay/platform/pull/4520#discussion_r3890005023)
`Cargo.lock` still pins GroveDB revision `6c882c3ee7d2c331f1feda2eb4223add9a6f0e45`, and the successful two-instance state-sync test remains ignored because this revision does not faithfully restore Platform SumTrees. The active `sum_tree_sync_probe` passes by detecting the expected latent corruption, while the production restore path correctly rejects it during strict verification. Consequently, an ordinary Platform snapshot cannot complete the feature's primary restore path at this head. Pin a GroveDB revision containing the SumTree restore fix and enable the successful end-to-end test before merging.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/v0/mod.rs:44-59: State-sync reconstruction changes an existing v0 implementation in place
(existing thread: https://github.com/dashpay/platform/pull/4520#discussion_r3890005026)
This PR changes `update_masternode_list_v0` so `is_init_chain` bypasses its original same-height return, and changes `update_core_info_v0` to forward that flag into quorum reconstruction instead of the original hardcoded `false`. Reconstruction passes `true`, while every protocol version through v15 still dispatches both methods as version 0. Even if existing InitChain callers happen to start from fresh state, the deployed v0 semantics have changed for a valid argument combination. Preserve v0 behavior and select new versioned implementations, or introduce a separately versioned reconstruction operation for v15.
5134c84 to
68759ce
Compare
|
Rebased onto
Verified locally (unsandboxed): Threads resolved with their commits: grovedb pin, in-place v0 versioning, Still draft; fork CI does not run here. 🤖 Posted autonomously by Claude on behalf of pasta. |
68759ce to
f299bd7
Compare
|
Addendum: the v0/v1 method split from the previous push was itself a no-op (reconstruction runs on a state with no committed block info, so the short circuits it bypassed can never fire); it is replaced by 2d2cbbc, which just restores both v0 bodies with no version bump. Also folded in the reviewer's doc/comment cleanups (f299bd7). Re-verified: fmt, clippy, 🤖 Posted autonomously by Claude on behalf of pasta. |
…ate sync Adds a minimal, platform-versioned subset of the Platform state that will be written into the replicated GroveDB state (Misc tree) so state-synced nodes can reconstruct the full Platform state, which is otherwise only persisted to non-replicated aux storage. Unlike the earlier prototype, fee versions of previous epochs are persisted faithfully by version number, and unknown-at-store-time block fields (app hash, block id hash, signature) are Options instead of zero-filled placeholders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… tree Persists the reduced platform state under Misc/reduced_saved_state inside the replicated grovedb state (unlike the full platform state, which lives in non-replicated aux storage). fetch returns Ok(None) when the key is absent, so callers can distinguish pre-activation snapshots. Adds the DriveError::Snapshot variant and the platform_state method version fields for the new methods. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bing Adds PLATFORM_V15 (drive-abci method versions v11: run_block_proposal 1, consensus_params_update 2), the DriveAbciStateSyncVersions substructure carrying the grovedb state sync wire protocol version on every platform version, and the reduced-platform-state storage method version slots on DriveAbciPlatformStateStorageMethodVersions. Pure plumbing: no behavior changes outside version selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… before root hash v1 (gated on drive-abci method versions v11 / protocol v15) is a copy of v0 with validator_set_update moved above the root-hash computation and the reduced platform state written into the replicated state immediately before the root hash, so the stored state carries the post-rotation next validator set and is covered by the block's app hash. Adds the store/fetch_reduced_platform_state execution wrappers and the PlatformState::to_reduced_platform_state conversion (fee versions persisted faithfully by number). A test proves rotation outcomes are unchanged by the reorder: validator_set_update only mutates in-memory block state and reads neither the app hash nor grovedb. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to v15 transition_to_version_15 stores the reduced platform state built from the last committed platform state under Misc/reduced_saved_state during the v15 activation block, so the key exists in the replicated state from the fork block onward and every snapshot taken at or after activation is restorable. run_block_proposal v1 overwrites it later in the same block with the state of the block being processed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…stry Adds StateSyncAbciConfig (env contract: SNAPSHOTS_ENABLED, SNAPSHOTS_FREQUENCY_SECONDS, MAX_NUM_SNAPSHOTS, CHECKPOINTS_PATH) which, when enabled, overrides the platform-version-driven checkpoint frequency, retention and directory. list_snapshots and load_snapshot_chunk (on both the tenderdash socket app and the gRPC CheckTx app) serve snapshots directly from drive.checkpoints: only checkpoints containing the reduced platform state are offered (pre-v15 checkpoints are unrestorable), requested wire versions are validated against a single supported-set const, chunk ids are size-capped before decoding (dashpay#3773), and served checkpoints are pinned via the existing Arc refcount so pruning cannot delete them mid-transfer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…unk handlers Adds the StateSyncApplication trait and a snapshot fetching session (grovedb sync session plus the wire version taken from the offered snapshot) on the Consensus and Full ABCI apps. offer_snapshot validates the offered version against the single supported-set const (REJECT_FORMAT otherwise), wipes grovedb, and answers Accept on both the fresh-session and the replace-with-newer-height paths. apply_snapshot_chunk caps chunk and chunk-id sizes before any decode (dashpay#3773), answers RETRY with the failed chunk in refetch_chunks (banning the sender) instead of killing the session when grovedb rejects a chunk, and on completion commits the session, verifies grovedb, reconstructs the platform state (stub until the next commit) and checks the restored root hash against the snapshot app hash. The completion log fires once per transfer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fter snapshot restore reconstruct_platform_state reads the reduced platform state out of the restored grovedb, restores scalar fields and fee versions faithfully by version number, re-derives masternode lists, identities and quorums from Core via update_core_info with start_from_scratch=true (idempotent re-derivation, proven by the caller's root-hash equality check), restores the recorded validator set order, and advances the state to the snapshot block via update_state_cache so the info handler reports the snapshot height and app hash across restarts. update_core_info now passes is_init_chain through to update_quorum_info (its only effect is skipping the same-core-height short-circuit, required for init chain and reconstruction; the normal block path is unchanged), and update_masternode_list's early return is likewise guarded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… v15 activation The first block of protocol v15 additionally emits EvidenceParams (max age 15000 blocks / 20 days, max bytes 1 MiB) per issue dashpay#2512, in named constants. A review-flag comment notes that 15000 blocks (~1 day at 6s blocks) vs 20 days look inconsistent, since evidence expires at the earlier bound, and must be confirmed before release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A source chain runs past several checkpoints via the strategy harness with snapshot serving enabled, and a fresh target restores its newest snapshot through the real offer/load/apply chunk loop (modeled on grovedb's run_sync driver) with mocked Core RPC. Findings baked into the tests: grovedb wire v1 at the pinned rev cannot faithfully restore sum trees (root hash reproduces but recomputation diverges - latent corruption that the strict post-restore verify_grovedb correctly refuses), pinned by a minimal tripwire reproducer plus an active test asserting the refusal; the full happy-path test is ignored until the grovedb pin gains the fixed wire version. The reconstruction path itself is fully validated by an active test running it against the source's own grove: it is byte-idempotent (root hash unchanged by the masternode identity re-derivation) and reproduces the complete platform state including validator set order, masternode lists and fee versions, satisfying the info handler. A tampered chunk yields RETRY with a refetch and sender ban; since grovedb drops a chunk id from its pending set before processing, a refetch it can no longer honor yields RETRY_SNAPSHOT, and offer_snapshot now accepts same-height re-offers so Tenderdash snapshot restarts work. Pre-v15 snapshots are not offered and cannot be restored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…constructed state Review follow-up: reconstruct_platform_state now commits the update_core_info re-derivation before update_state_cache publishes the in-memory state, so a commit failure propagates without the info handler ever reporting a snapshot height grovedb never persisted. Aux writes (not part of the root hash) commit in their own transaction afterwards. Also documents that the RetrySnapshot string-match fallback is safe if grovedb's error wording changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… wipes grovedb *** THIS COMMIT CHANGES FEATURE CODE, NOT TESTS — please review it on its own. *** offer_snapshot calls drive.grove.wipe() and then restores a snapshot, but Drive's lazily-loaded in-memory caches were left pointing at the state that was just destroyed. The protocol version counter is the damaging one: ProtocolVersionsCache keeps a 'loaded' flag, so load_if_needed never re-reads the restored version counters, and the first block after the restore writes vote counts derived from the WIPED chain. The result is an immediate app hash fork against every other node. Reproduced by state_synced_and_replayed_nodes_stay_converged: with a node whose caches had been touched before the snapshot offer, the synced node and the replayed node disagreed on the app hash at the very first block after the sync, with the divergence isolated to the Versions tree (RootTree::Versions and Versions/0). The test passes with this fix. Reset the counter wholesale rather than calling clear_global_cache, so the loaded flag is cleared too and the cache reloads from the restored state. Also clear the data contract cache and the cached genesis time, for the same reason. system_data_contracts is deliberately left alone: those are compiled-in, version-keyed contracts that never come from grovedb. Reachability: Tenderdash normally offers a snapshot only at startup, before any block has been processed, so on today's code paths the caches are usually still empty and the fork is not reachable in production. This is a latent landmine rather than a live incident — but offer_snapshot performs a destructive wipe and must not leave derived state behind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…te sync restore *** THIS COMMIT CHANGES FEATURE CODE, NOT TESTS — please review it on its own. *** A restore destroys the database before it rebuilds it, and the rebuild is not atomic with the platform state that has to describe it. Two paths left a node holding a database its platform state knew nothing about, and the info handler panics on exactly that mismatch, so drive-abci crash-looped on the first ABCI call and restarting only reloaded the state causing it: a crash between commit_session and reconstruct_platform_state, and a snapshot that turns out to be unusable — which any peer can cause by offering a pre-v15 one. Restore sentinel. offer_snapshot writes a marker file BEFORE it wipes, so there is no window where the database is destroyed and nothing says so. Platform::open_with_client treats a surviving marker as an unfinished restore: wipe, drop the caches derived from what was wiped, come up empty, clear the marker. The marker is a plain file in db_path, NOT aux storage, because GroveDb::wipe() clears the aux column family too — a sentinel there would be destroyed by the very wipe it exists to survive. It is outside everything grovedb touches and can never affect the app hash. Rejection path. Every failure after commit_session now goes through reject_restored_snapshot: wipe back to a clean slate and answer REJECT_SNAPSHOT rather than returning an error, so Tenderdash discards this snapshot, tries the next, and falls back to block sync when it runs out. An ABCI exception there would abort state sync altogether. Detecting an unusable snapshot BEFORE the commit would be better, but grovedb keeps MultiStateSyncSession::transaction private, so the Misc tree cannot be probed before it lands; that is a follow-up for grovedb dashpay#840. Clear points. The marker is cleared when the node is provably self-consistent: after a completed restore, after startup recovery has wiped, and at the end of init_chain — the last of these is what stops an abandoned restore from making the next restart wipe a perfectly good block-synced chain. It is deliberately kept on the rejection path, because an empty database plus a stale in-memory platform state is not yet consistent. The wipe-and-clear-caches helper is now shared by the offer path and the recovery path so the two cannot drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-wedge guarantee Regression tests for the preceding fix. Deliberately free of any assertion that a restore SUCCEEDS, so they are green at BOTH grovedb pins: Dash Platform state always contains sum trees, so a full successful restore needs dashpay/grovedb#840, but a restore that FAILS exercises the same recovery path either way — at the unpatched revision the sum-tree defect supplies the failure for free. Covers: offer_snapshot records the sentinel before it wipes; a rejected offer records none, so a peer cannot make a healthy node wipe itself on the next restart just by offering a format it cannot speak; a restart mid-restore wipes, comes up empty and passes the info handshake instead of crash-looping; a NORMAL restart keeps its state, which is the regression that matters most if startup recovery ever fires unconditionally; init_chain clears a sentinel left by an abandoned restore, so the block-sync fallback's chain survives the next restart; and end to end, an unusable snapshot offered by a peer leaves the node empty, recoverable and able to sync. The shared chunk-loop driver now reports REJECT_SNAPSHOT as a SnapshotSyncOutcome::Rejected rather than treating it as an unexpected result code, and the two existing tests that relied on the old error-returning refusal assert the rejection plus the new wipe-back-to-clean behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ision No behaviour change — this only makes an existing landmine visible. FEE_VERSION2, which protocol versions 9 and later actually run with, declares fee_version_number 1, the same number FEE_VERSION1 declares, and is absent from FEE_VERSIONS. FeeVersion::get resolves numbers through that list, so FeeVersion::get(1) can only ever return FEE_VERSION1 — never FEE_VERSION2 — even though the two differ in data_contract_registration. That makes every number-only round trip of a fee version silently lossy, and there are two: PlatformStateForSavingV1 stores previous_fee_versions as (epoch index -> number), so a node that RESTARTS rehydrates previous epochs' fees as FEE_VERSION1; ReducedPlatformStateV0 does the same, so a node that STATE-SYNCS gets the substitution without even restarting. It is latent rather than a live fork only because previous_fee_versions is consulted solely to price storage refunds and the two constants have identical storage fees. It becomes a consensus fork the moment a future FeeVersion changes a storage or processing fee without taking a distinct number. Documents the rule — every FeeVersion constant must have a unique fee_version_number and be listed in FEE_VERSIONS at the index its number implies — and adds fee_version_numbers_are_unique_and_resolvable to enforce it. The test is #[ignore]d because it fails today; running it with --ignored reproduces the defect. Un-ignore it as part of giving FEE_VERSION2 its own number, which is protocol-visible and needs a migration rather than an in-place edit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng on sentinel cleanup *** THIS COMMIT CHANGES FEATURE CODE, NOT TESTS — please review it on its own. *** Three findings from an independent review of the preceding fix. 1. The wipe did not clear drive.checkpoints. That registry is populated by Drive::open and is what list_snapshots serves to peers — it is not a value cache that merely goes stale. Left in place across a wipe, a node that discarded a chain kept advertising snapshots of it, so a peer could state-sync from state this node no longer had. Now cleared, with the entries marked for deletion first so their directories are removed rather than leaking on disk. Regression test: a_wiped_node_stops_serving_snapshots_of_the_discarded_chain. 2. Clearing the sentinel at the two points where the node is ALREADY self-consistent — the end of a completed restore, and the end of init_chain — propagated I/O errors, so a failed remove_file turned a fully successful restore or a working genesis into a hard ABCI error. Now best-effort with a loud error log: the cost of not removing it is one unnecessary wipe-and-resync on a later restart, which is bounded and safe, unlike failing the operation. 3. commit_session's own failure still returned an ABCI exception rather than going through the recovery path. grovedb only makes the session durable once its internal root-hash check passes, so nothing is committed on that error — but the database is still WIPED from the offer, so the node must not be left as it is, and an exception stalls Tenderdash's snapshot ladder where REJECT_SNAPSHOT keeps it moving. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Maintainer ruling: the original state sync never shipped, so grovedb updates its replication protocol in place and stays at version 1 - there is no v2. The supported-set constant and the offered-snapshot validation remain so any future incompatible protocol change fails fast on both sides; comments now say exactly that instead of describing a version bump that will not happen. No behavior changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
InitChain passes PlatformVersion::first() as the original version so Tenderdash learns the real app version, but that fiction also makes a chain STARTING on protocol v15+ look like it just crossed to v15 - and consensus_params_update_v2 then emits the 15000-block evidence window meant for chains upgrading with pre-state-sync genesis documents (dashpay#2512), silently overriding the evidence params of the genesis document being initialized. At genesis the operator's genesis document is authoritative; strip the evidence section from the InitChain update so it stays in force. Mid-chain crossings to v15 keep the override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Checkpoint creation honors the operator-configured checkpoints directory, but startup was hard-coded to <db_path>/checkpoints in both Drive::open and the platform_state.bin load. A node running with a custom CHECKPOINTS_PATH therefore came back from a restart with an empty registry: it stopped advertising the snapshots it had retained, and the directories it had written could never be pruned. Thread the resolved path through Drive::open_with_checkpoints_path and Platform::open_with_client so creation and reload always agree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ced at The consuming node picked grove_version from its own in-memory platform state. A node that state syncs has no saved state, so it sits at the initial protocol version (Drive v1 / GROVE_V1) while snapshots are only restorable from v15 (Drive v9 / GROVE_V4). grovedb replication, tree opening, root hashing and restore are all version gated, so generation and verification ran under different rules. list_snapshots now stamps the checkpoint's own protocol version into the snapshot metadata and serves each checkpoint under that version; offer_snapshot decodes it, refuses anything that is not a known version >= v15, and pins it on the session so every grovedb call of the transfer uses the same table. The value is peer-supplied but untrusted-safe: a lie fails verification against the light-client-verified app hash and lands on REJECT_SNAPSHOT. Also in the snapshot lifecycle: any accepted-format offer now replaces the session in progress (refusing a lower height let a peer advertise a high snapshot, withhold its chunks and block Tenderdash's fallback to an honest older one); oversized chunks and chunk ids answer RETRY/RETRY_SNAPSHOT with the sender rejected instead of throwing an ABCI exception that would abort state sync on a wiped database; and serving pins gained an absolute lifetime, a count cap, expiry on read, a per-block sweep, and are only taken after a chunk was actually served. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… in state sync Reconstruction built both signature-verification quorum sets empty and called update_core_info with platform_state = None, so a restored node had no previous quorums at all. For chain locks that degrades safely (verify_chain_lock_locally returns Ok(None) when there is no history and defers to Core), but instant lock verification has no Core fallback by design: a node restored within SIGN_OFFSET core blocks of a quorum change would judge an InstantAssetLockProof against a different quorum than a node that replayed the chain, and reject a state transition the network accepted. That history cannot be re-derived from Core — get_quorum_listextended answers which quorums exist at a height, not when this node observed the set change — so ReducedPlatformStateV0 now carries the superseded quorums and their three core heights for both sets, and reconstruction reinstates them verbatim (previous_change_height included, which set_previous_past_quorums would have derived wrongly). The current sets are still re-derived from Core, where the answer is exact. Reconstruction also treated saved.quorum_positions as a sorting hint only, dropping saved hashes it did not see and appending unexpected Core ones. Validator sets live in the platform state, not grovedb, so the app-hash check cannot catch that: require an exact hash-set match and refuse the snapshot otherwise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…metadata Adds a restart test proving checkpoints written under a configured CHECKPOINTS_PATH are reloaded and still advertised, and asserts that a snapshot honestly declaring a pre-v15 protocol version is refused at the offer, before anything is wiped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…napshot serving Follow-ups from reviewing the state sync fixes: the pin count cap now tracks MAX_NUM_SNAPSHOTS plus slack instead of a fixed 8, and the absolute pin lifetime moves to six hours — the count cap is the real bound on how many checkpoint directories can be held back, so the lifetime only needs to be a backstop, and an hour risked cutting off an honest slow peer whose checkpoint was pruned mid-transfer. Also: a new Checkpoint::platform_version collapses the version resolution duplicated across list_snapshots and load_snapshot_chunk; Drive::open_with_checkpoints_path takes the directory directly instead of an Option; list_snapshots logs when it declines to advertise a checkpoint instead of skipping it silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… succeeds The query service refuses to execute while the published state's height differs from committed_block_height_guard. A fresh node's guard is 0 and only finalize_block ever stored to it, so a completed restore published the reconstructed state at the snapshot height while the guard stayed at 0, leaving every query unserviceable until the first post-restore block finalized. Store the height into the guard in apply_snapshot_chunk, strictly after grovedb reconstruction, aux persistence and the final app-hash check succeed; a rejected restore leaves the gate closed (covered by a new assertion in the sum-tree-defect test). Also move the query service's wait counter out of the inner loop: declared inside it, it was reset on every pass, so the intended 1-second budget never expired and a query hitting a state/guard mismatch would spin forever instead of restarting and eventually returning NotServiceable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etadata yet A state restored via state sync stores an all-zero block id hash and quorum signature for the snapshot block. That is structural, not an omission: the reduced platform state is written into grovedb immediately before the root hash is computed, and the block's commit signature signs that root hash, so the signature can never be part of the state it signs. response_proof_v0 copied those zeroes into every current-state proof, and rs-drive-proof-verifier (correctly) rejects an all-zero signature, so between a completed restore (or a restart from the persisted restored state) and the first finalized block the node served proofs no client could ever authenticate. Refuse to build such a proof instead: response_proof_v0 now returns a dedicated error when the state has a committed block but an all-zero signature, and the query service maps it to gRPC UNAVAILABLE so clients retry (or re-query without a proof) rather than report verification failures. The first block finalized after the restore stores real metadata, persists it, and reopens proof serving; queries without proofs are unaffected. Height 0 stays exempt (a chain with no committed block has no signature for anyone), and so do test chains that run with block signing disabled (feature-gated testing-config, not part of production builds) - they finalize every block unsigned and their proofs were never verifiable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… off the async workers Production snapshot serving runs in CheckTxAbciApplication (the gRPC app server.rs registers), whose SnapshotManager was private and whose process never finalizes blocks - the once-per-block release_expired_pins in FullAbciApplication::finalize_block only covers the all-in-one test application. An abandoned transfer therefore kept its checkpoint Arc alive, holding an already-pruned full-state directory on disk until another serving request or shutdown. The serving SnapshotManager is now shared (Arc) with a small sweep task server.rs spawns next to the gRPC server, which releases expired pins once a minute regardless of peer activity or blocks. list_snapshots and load_snapshot_chunk were also running their synchronous rocksdb/Merk work (checkpoint metadata reads, chunk generation and encoding) directly on Tokio async workers of a tonic handler. Requests are peer-controlled, so concurrent snapshot consumers could occupy the runtime and delay unrelated gRPC traffic. Both handlers now run on the blocking pool, following the adjacent check_tx pattern; they take the platform and snapshot manager directly (owned Arcs clone into the blocking closure), which also retires the now-unused SnapshotManagerApplication trait. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sum-tree tripwire grovedb invalidates its restore session on a failed chunk, so the target asks Tenderdash to restart the snapshot instead of refetching one chunk. The sum-tree probe is removed; the full two-instance round trip stays ignored until the grovedb pin carries dashpay/grovedb#840, which arrives with the GroveDB 6.0.0 bump in dashpay#4635.
…st_v0 to their v4.2-dev bodies The PR changed both v0 implementations in place so that is_init_chain also bypassed their same-core-height short circuits for state sync reconstruction. That edit was a no-op: reconstruct_platform_state builds its state with last_committed_block_info = None, so last_committed_core_height() is 0 and neither short circuit can fire for any real snapshot; is_init_chain = true already selects the from-scratch build in update_state_masternode_list_v0. Both files go back to their exact v4.2-dev bodies and no method version is bumped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… contestant vote proofs in their query direction The proof-metadata guard treats an all-zero block signature as a state restored via state sync and refuses proofs until the next block finalizes. The hand-built ExtendedBlockInfo fixtures in fast_forward_to_block, the masternode vote tests and the document query v1 tests all used an all-zero signature and started failing on that guard; they now share a TEST_BLOCK_SIGNATURE placeholder. Also: get_proved_contestant_votes verified every proof with an ascending query even when the request was descending; the re-pinned grovedb enforces that a layer proof is encoded in its walk direction's family, so the verifier now uses the same order_ascending as the request. offer_snapshot drops a duplicated 'db bound clippy flagged. Stale doc comments that described the old grovedb pin and the pre-dashpay#840 refetch ladder are cleaned up, and sync_snapshot's doc comment is moved back onto the function. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…m state run_block_proposal v1 wrote block_proposal.round into the reduced platform state, which sits in the Misc tree under the app hash. Tenderdash re-proposes a block that reached a prevote majority but did not commit with the same header at later rounds, and re-runs ProcessProposal for every round while requiring the returned app hash to equal the header's. With the round hashed in, every validator rejects the re-proposal and the chain halts at that height. The reduced block info now carries only header-fixed fields; the Option app hash, block id hash and signature (only ever Some in transition_to_version_15, whose write v1 overwrote in the same block) go with it, as does that transition. Reconstruction zero-fills them until the next finalized block, which the proof metadata guard already handles. ReducedPlatformState also moves to the platform serialization derive so it encodes big-endian like every other versioned platform type; a test pins the encoding and another pins that the app hash is independent of the round. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…napshot restore reconstruct_platform_state routed through update_core_info, which re-issued AddNewIdentity for every masternode; each hit the re-enable branch and rewrote every key of an identity the restored grovedb already held. Thousands of no-op writes on the consensus thread, with correctness resting on byte-idempotence that only the final root-hash compare could catch. The new rebuild_core_info_in_memory helper rebuilds the masternode lists and quorum sets from Core without touching grovedb, and reconstruction no longer opens a write transaction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… smaller Tenderdash treats evidence as expired only when both max_age_num_blocks and max_age_duration are exceeded, and backfill likewise stops only when both are satisfied. The note claimed the smaller bound wins; the larger one does, so the effective window is 20 days and the 15 000 block bound never binds. Values unchanged pending the dashpay#2512 decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Since dashpay#4570 checkpoints are only taken for blocks younger than ten minutes, so a source chain starting at the fixed 2023 genesis time never produced a snapshot and every state sync integration test failed on an empty checkpoint registry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
f299bd7 to
64a8729
Compare
2 participants
Issue being fixed or feature implemented
ABCI state sync for Drive: new nodes bootstrap from a peer snapshot instead of replaying the whole chain. Supersedes #4520 (same branch, re-homed to an org branch so Rust CI runs) and #2486 (re-implemented on v4.2-dev rather than rebased — the v2.0-era branch predates the checkpoint registry, the de-versioned
PlatformState, grovedb 5.x, and four consensus-relevant additions torun_block_proposal).Related: issues #2512 (evidence params / backfill window), #3773 (chunk size caps). Companion PRs: grovedb sum-tree restore fix (dashpay/grovedb#840 — the replication protocol is updated in place and stays at version 1), dashmate config plumbing (#4521), tenderdash polish (dashpay/tenderdash#1425).
What was done?
Reduced platform state (consensus change, gated at protocol v15):
PlatformStatepersists only to grovedb aux, which chunk replication does not transfer. A newReducedPlatformState(rs-dpp) — the non-re-derivable subset (header-fixed block info, protocol versions, quorum hashes + positions, faithfulprevious_fee_versions, proposed core height, superseded lock quorums) — is written to the Misc tree (b"reduced_saved_state") every block by a newrun_block_proposalv1, just before the root hash. Only header-fixed block fields are stored: the consensus round (and the not-yet-known app hash, block id hash and signature) stay out, because Tenderdash re-proposes the same header at later rounds and re-runs ProcessProposal for each, requiring the same app hash. A test pins that the app hash is round-independent. The state is encoded with the platform serialization derive (big-endian, like every other versioned platform type).validator_set_updatemoves above the root-hash computation so the reduced state captures rotated validator sets; safety is proven by a test asserting rotation outcomes are independent of the reorder (v2 rotation reads only last-committed state) plus a reviewed v0→v1 diff.Snapshot serving (reuses the existing checkpoint registry — no second snapshot mechanism):
list_snapshots/load_snapshot_chunkon the gRPC (check-tx) app serve fromdrive.checkpoints, offering only checkpoints that contain the reduced state (activation-height filter as a key-presence probe). Served checkpoints are pinned via the existingArc<Checkpoint>refcount (600 s inactivity TTL) so pruning cannot delete a snapshot mid-transfer. Checkpoint frequency/count become configurable viaSNAPSHOTS_ENABLED/SNAPSHOTS_FREQUENCY_SECONDS/MAX_NUM_SNAPSHOTS/CHECKPOINTS_PATH(all default-off; stanzas added to the.env.*files).Snapshot consuming:
offer_snapshot/apply_snapshot_chunkon the consensus/full apps: wipe +start_snapshot_syncing, chunk application with 16 MiB chunk / 64 KiB chunk-id caps enforced before any decode (When wiring ABCI state-sync, cap incoming snapshot-chunk message size before grovedb decode #3773), bad chunks answered with RETRY +refetch_chunks+ sender ban (RETRY_SNAPSHOT where grovedb cannot honor a refetch), the protocol version taken from the offered snapshot and validated against a single supported-versions const (exactly one version exists; the gate makes any future incompatible change fail fast on both sides), root-hash-verified commit, then fullverify_grovedb.reconstruct_platform_state: rebuilds the full platform state from the reduced state + Dash Core RPC. The masternode lists and quorum sets are rebuilt in memory only (rebuild_core_info_in_memory); the restored grovedb already holds every masternode identity, so nothing is written and a final drive-root vs snapshot-app-hash equality check guards the restore. The reconstructed state satisfies theinfohandler's panic-level consistency check.QA fixes: an independent QA pass found and fixed three real bugs, cherry-picked here. (1)
offer_snapshotwiped grovedb but left Drive's in-memory caches (data contracts, protocol version, genesis info) describing the discarded chain — a consensus hazard after restore; the wipe now clears them. (2) A crash or failure mid-restore could wedge a node permanently: a restore sentinel file indb_pathis written before the wipe and cleared only after a complete restore (or startup recovery /init_chain), startup recovery wipes a half-restored database back to a clean slate, and any post-commit failure (verification, reconstruction, root-hash mismatch) wipes and answers REJECT_SNAPSHOT so Tenderdash moves on instead of the node erroring out with unusable state. (3) The checkpoint registry is cleared on wipe, so a freshly wiped node stops advertising snapshots of the chain it just discarded.Evidence params (#2512):
consensus_params_updatev2 emitsEvidenceParamson the v15 boundary. Values are the ones proposed in #2512 and are flagged in constants — they need a decision before release. Tenderdash expires evidence (and stops backfill) only when BOTH bounds are exceeded, so the effective window is the larger one: 20 days, with 15,000 blocks (~1 day) never binding.How Has This Been Tested?
--test strategy_tests state_sync): platform-state reconstruction is idempotent and matches the source; pre-v15 snapshots are neither served nor restorable; seven sentinel/never-wedge tests cover the restore sentinel lifecycle (recorded before the wipe, not recorded on a rejected offer, cleared byinit_chain), restart-mid-restore recovery, normal restarts keeping their state, and an unusable snapshot leaving the node empty and able to sync; Unit tests across all new handlers, config deserialization, round-independence of the app hash, rotation-reorder independence, and 3 new consensus-params tests. Existing strategy smokes (run_chain_*) pass and now exerciserun_block_proposalv1 at latest. Fulldrive-abcilib suite: 2784 passed.v4.2-devrestores SumTrees with a matching root hash but divergent recomputation (latent corruption, caught by our strictverify_grovedb); feat(replication)!: complete V1 state sync and bounded-memory restore grovedb#840 fixes it. This PR does not bump grovedb — the bump lands with the GroveDB 6.0.0 adoption in build(platform)!: adopt GroveDB 6.0.0 and the published grovedb-bincode 2.1.0 #4635, whose pin includes chore(dashmate): update production dashcore versions for mainnet and testnet #840. The full two-instance round triprun_state_sync_between_two_platformsis therefore#[ignore]d until build(platform)!: adopt GroveDB 6.0.0 and the published grovedb-bincode 2.1.0 #4635 merges, at which point the attribute is dropped in a follow-up. Verified against chore(dashmate): update production dashcore versions for mainnet and testnet #840: the round trip passes there. No protocol version change is involved (grovedb's replication protocol stays at version 1).FEE_VERSION2 (#4647): the pre-existing
fee_version_numbercollision is documented on the constant and tracked in #4647; state sync adds a second number-only round trip ofprevious_fee_versions, latent for the same reason restarts are.Breaking Changes
None until protocol v15 activates (all consensus changes are version-gated). New optional env vars for snapshot serving, default off.
Checklist:
🤖 Generated with Claude Code