Skip to content

feat(drive-abci)!: state sync via ABCI snapshots with reduced platform state (protocol v15) - #4648

Open
PastaPastaPasta wants to merge 36 commits into
v4.2-devfrom
feat/state-sync-v15
Open

feat(drive-abci)!: state sync via ABCI snapshots with reduced platform state (protocol v15)#4648
PastaPastaPasta wants to merge 36 commits into
v4.2-devfrom
feat/state-sync-v15

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

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 to run_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):

  • PlatformState persists only to grovedb aux, which chunk replication does not transfer. A new ReducedPlatformState (rs-dpp) — the non-re-derivable subset (header-fixed block info, protocol versions, quorum hashes + positions, faithful previous_fee_versions, proposed core height, superseded lock quorums) — is written to the Misc tree (b"reduced_saved_state") every block by a new run_block_proposal v1, 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_update moves 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_chunk on the gRPC (check-tx) app serve from drive.checkpoints, offering only checkpoints that contain the reduced state (activation-height filter as a key-presence probe). Served checkpoints are pinned via the existing Arc<Checkpoint> refcount (600 s inactivity TTL) so pruning cannot delete a snapshot mid-transfer. Checkpoint frequency/count become configurable via SNAPSHOTS_ENABLED / SNAPSHOTS_FREQUENCY_SECONDS / MAX_NUM_SNAPSHOTS / CHECKPOINTS_PATH (all default-off; stanzas added to the .env.* files).

Snapshot consuming:

  • offer_snapshot/apply_snapshot_chunk on 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 full verify_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 the info handler's panic-level consistency check.

QA fixes: an independent QA pass found and fixed three real bugs, cherry-picked here. (1) offer_snapshot wiped 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 in db_path is 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_update v2 emits EvidenceParams on 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?

FEE_VERSION2 (#4647): the pre-existing fee_version_number collision is documented on the constant and tracked in #4647; state sync adds a second number-only round trip of previous_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:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

🤖 Generated with Claude Code

PastaPastaPasta and others added 30 commits September 9, 2026 16:14
…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 (#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 (#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 #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 #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 (#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 #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-#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>
PastaPastaPasta and others added 4 commits September 9, 2026 16:14
…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 #2512 decision.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Since #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>
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Too many files!

This PR contains 108 files, which is 8 over the limit of 100.

To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch.

Upgrade to a paid plan to raise the limit.

This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: c8641c77-ed47-4176-b9c4-62ffec894988

📥 Commits

Reviewing files that changed from the base of the PR and between b84975e and 16cc49d.

📒 Files selected for processing (108)
  • packages/rs-dpp/src/lib.rs
  • packages/rs-dpp/src/reduced_platform_state/mod.rs
  • packages/rs-dpp/src/reduced_platform_state/v0/mod.rs
  • packages/rs-drive-abci/.env.local
  • packages/rs-drive-abci/.env.mainnet
  • packages/rs-drive-abci/.env.testnet
  • packages/rs-drive-abci/src/abci/app/check_tx.rs
  • packages/rs-drive-abci/src/abci/app/consensus.rs
  • packages/rs-drive-abci/src/abci/app/full.rs
  • packages/rs-drive-abci/src/abci/app/mod.rs
  • packages/rs-drive-abci/src/abci/config.rs
  • packages/rs-drive-abci/src/abci/error.rs
  • packages/rs-drive-abci/src/abci/handler/apply_snapshot_chunk.rs
  • packages/rs-drive-abci/src/abci/handler/list_snapshots.rs
  • packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs
  • packages/rs-drive-abci/src/abci/handler/mod.rs
  • packages/rs-drive-abci/src/abci/handler/offer_snapshot.rs
  • packages/rs-drive-abci/src/execution/engine/consensus_params_update/mod.rs
  • packages/rs-drive-abci/src/execution/engine/consensus_params_update/v2/mod.rs
  • packages/rs-drive-abci/src/execution/engine/initialization/init_chain/v0/mod.rs
  • packages/rs-drive-abci/src/execution/engine/run_block_proposal/mod.rs
  • packages/rs-drive-abci/src/execution/engine/run_block_proposal/v1/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/block_end/create_grovedb_checkpoint/v0/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/block_end/should_checkpoint/v0/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/block_end/update_checkpoints/v0/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/block_end/validator_set_update/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_core_info/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/state_sync/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/state_sync/reconstruct_platform_state/mod.rs
  • packages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/mod.rs
  • packages/rs-drive-abci/src/execution/storage/fetch_reduced_platform_state/v0/mod.rs
  • packages/rs-drive-abci/src/execution/storage/mod.rs
  • packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/mod.rs
  • packages/rs-drive-abci/src/execution/storage/store_reduced_platform_state/v0/mod.rs
  • packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs
  • packages/rs-drive-abci/src/platform_types/mod.rs
  • packages/rs-drive-abci/src/platform_types/platform/mod.rs
  • packages/rs-drive-abci/src/platform_types/platform_state/mod.rs
  • packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/mod.rs
  • packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/quorum_set.rs
  • packages/rs-drive-abci/src/platform_types/snapshot/mod.rs
  • packages/rs-drive-abci/src/query/document_query/v1/tests.rs
  • packages/rs-drive-abci/src/query/response_metadata/v0/mod.rs
  • packages/rs-drive-abci/src/query/service.rs
  • packages/rs-drive-abci/src/server.rs
  • packages/rs-drive-abci/src/test/helpers/fast_forward_to_block.rs
  • packages/rs-drive-abci/src/utils/mod.rs
  • packages/rs-drive-abci/src/utils/serialization.rs
  • packages/rs-drive-abci/tests/strategy_tests/test_cases/mod.rs
  • packages/rs-drive-abci/tests/strategy_tests/test_cases/process_proposal_collision_tests.rs
  • packages/rs-drive-abci/tests/strategy_tests/test_cases/state_sync_sentinel_tests.rs
  • packages/rs-drive-abci/tests/strategy_tests/test_cases/state_sync_tests.rs
  • packages/rs-drive/src/drive/mod.rs
  • packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/mod.rs
  • packages/rs-drive/src/drive/platform_state/fetch_reduced_platform_state_bytes/v0/mod.rs
  • packages/rs-drive/src/drive/platform_state/mod.rs
  • packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/mod.rs
  • packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs
  • packages/rs-drive/src/error/drive.rs
  • packages/rs-drive/src/open/load_current_checkpoints.rs
  • packages/rs-drive/src/open/mod.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v1.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v10.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v11.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v2.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v3.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v4.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v5.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v6.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v7.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v8.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v9.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_state_sync_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_state_sync_versions/v1.rs
  • packages/rs-platform-version/src/version/drive_abci_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_versions/mod.rs
  • packages/rs-platform-version/src/version/drive_versions/v1.rs
  • packages/rs-platform-version/src/version/drive_versions/v2.rs
  • packages/rs-platform-version/src/version/drive_versions/v3.rs
  • packages/rs-platform-version/src/version/drive_versions/v4.rs
  • packages/rs-platform-version/src/version/drive_versions/v5.rs
  • packages/rs-platform-version/src/version/drive_versions/v6.rs
  • packages/rs-platform-version/src/version/drive_versions/v7.rs
  • packages/rs-platform-version/src/version/drive_versions/v8.rs
  • packages/rs-platform-version/src/version/drive_versions/v9.rs
  • packages/rs-platform-version/src/version/fee/mod.rs
  • packages/rs-platform-version/src/version/fee/v2.rs
  • packages/rs-platform-version/src/version/mocks/v2_test.rs
  • packages/rs-platform-version/src/version/mocks/v3_test.rs
  • packages/rs-platform-version/src/version/mod.rs
  • packages/rs-platform-version/src/version/protocol_version.rs
  • packages/rs-platform-version/src/version/v1.rs
  • packages/rs-platform-version/src/version/v10.rs
  • packages/rs-platform-version/src/version/v11.rs
  • packages/rs-platform-version/src/version/v12.rs
  • packages/rs-platform-version/src/version/v13.rs
  • packages/rs-platform-version/src/version/v14.rs
  • packages/rs-platform-version/src/version/v15.rs
  • packages/rs-platform-version/src/version/v2.rs
  • packages/rs-platform-version/src/version/v3.rs
  • packages/rs-platform-version/src/version/v4.rs
  • packages/rs-platform-version/src/version/v5.rs
  • packages/rs-platform-version/src/version/v6.rs
  • packages/rs-platform-version/src/version/v7.rs
  • packages/rs-platform-version/src/version/v8.rs
  • packages/rs-platform-version/src/version/v9.rs

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

🕓 Queued for automated review — 13th in line, estimated start in ~2.3 h (commit 16cc49d)
Estimated review time once started: ~20 min (two-phase automated review; median of recent runs).

  • Request priority review — tick this box and the review moves to the front of the queue.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.70375% with 633 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.14%. Comparing base (b84975e) to head (64a8729).

Files with missing lines Patch % Lines
...vents/state_sync/reconstruct_platform_state/mod.rs 47.94% 139 Missing ⚠️
...rive-abci/src/abci/handler/apply_snapshot_chunk.rs 53.31% 134 Missing ⚠️
.../src/execution/engine/run_block_proposal/v1/mod.rs 63.19% 106 Missing ⚠️
...s/rs-drive-abci/src/abci/handler/offer_snapshot.rs 84.33% 26 Missing ⚠️
packages/rs-drive-abci/src/abci/app/full.rs 28.57% 25 Missing ⚠️
...drive-abci/src/abci/handler/load_snapshot_chunk.rs 84.72% 22 Missing ⚠️
...s/rs-drive-abci/src/abci/handler/list_snapshots.rs 80.95% 20 Missing ⚠️
...s/rs-drive-abci/src/platform_types/snapshot/mod.rs 90.33% 20 Missing ⚠️
packages/rs-drive-abci/src/abci/app/check_tx.rs 50.00% 18 Missing ⚠️
packages/rs-drive-abci/src/abci/app/consensus.rs 10.00% 18 Missing ⚠️
... and 19 more
Additional details and impacted files
@@             Coverage Diff              @@
##           v4.2-dev    #4648      +/-   ##
============================================
- Coverage     87.88%   87.14%   -0.74%     
============================================
  Files          2766     2815      +49     
  Lines        360586   365856    +5270     
============================================
+ Hits         316904   318842    +1938     
- Misses        43682    47014    +3332     
Components Coverage Δ
dpp 88.89% <91.93%> (-0.21%) ⬇️
drive 86.21% <92.19%> (-0.38%) ⬇️
drive-abci 88.51% <70.84%> (-1.43%) ⬇️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.92% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.78% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 2 only (queue backlog)

Verified the findings against the exact head and pinned GroveDB implementation. Three blocking issues remain: unbounded snapshot response generation also conflicts with the receive cap, ranked-index snapshots cannot be restored, and late restore failures can leave published state inconsistent with the database. Two additional suggestions address crash durability and blocking checkpoint cleanup; existing local regression binaries reproduced both interoperability failures, and three snapshot-handler tests passed.

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: critical by gpt-6-astra (effort low) — This large cross-crate change alters consensus-critical app-hash computation and protocol activation while introducing peer-supplied snapshot decoding, destructive database restoration, and validator/quorum state reconstruction, where errors could cause chain divergence, corrupted state, or denial of service.
  • Phase 1 reviewers: not run (skipped for throughput: 21 PRs queued, above the 10 limit)
  • Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

🔴 3 blocking | 🟡 2 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/abci/handler/load_snapshot_chunk.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs:94-102: Bound snapshot generation and coordinate batching with the receive cap
  The 64 KiB chunk-ID cap does not bound the work or response allocated by this call. In pinned GroveDB commit 6fc7e1e, fetch_chunk decodes global/local ID lists, generates every requested chunk—including duplicates—and accumulates their encoded contents before returning. Thousands of duplicate local IDs fit under this limit, allowing a peer to trigger large allocations repeatedly or concurrently whenever snapshot serving is enabled. A response-size check after fetch_chunk returns would be too late to prevent those allocations.

  The same missing generation budget breaks honest transfers: MultiStateSyncSession batches up to 32 local IDs and then 32 global IDs without accounting for payload bytes. The populated-tree regression produced a 25,212,164-byte honest response, which apply_snapshot_chunk rejects against its 16 MiB cap with RETRY and a sender rejection. Other peers generate the same oversized response, so refetching cannot recover.

  Validate decoded batch counts and duplicates before generation, enforce a cumulative output/work budget during generation, and coordinate target batching or pagination so legitimate responses fit the receive cap. Cover both adversarial descriptors and populated-tree round trips.

In `packages/rs-drive-abci/src/abci/handler/list_snapshots.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/list_snapshots.rs:57-66: Account for ranked-index trees before advertising snapshots as restorable
  The reduced-state key only establishes the activation boundary; it does not establish restorability. Protocol v15 retains v14's ranked-index support, and insert_contract_v0 creates indexed-tree elements for valid ranked contracts, including at registration for single-property ranked indexes. However, the pinned GroveDB MultiStateSyncSession::discover_new_subtrees_metadata returns NotSupported whenever it encounters an indexed-tree element, even when that tree is empty. apply_snapshot_chunk converts this permanent unsupported-state error into RETRY_SNAPSHOT and rejects the honest sender; the existing regression reproduces that result.

  Consequently, valid ranked contracts can make subsequent snapshots unusable while this handler continues advertising them. This is a separate limitation from the documented SumTree/#840 dependency. Add indexed-tree replication support before offering these snapshots, or exclude unsupported snapshots until that support is available. Handle permanent unsupported-state failures without blaming senders, and add a round trip containing an actual ranked contract.

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:203-209: Do not publish reconstructed state before the remaining restore checks succeed
  update_state_cache_v0 publishes the reconstructed PlatformState through self.state.store before the aux transaction is committed. That commit and the caller's subsequent root-hash read/check can still fail. Their rejection path calls wipe_drive_for_restore, which clears GroveDB and Drive caches but does not reset Platform.state. Even a fresh target can therefore retain the snapshot's nonzero app hash in memory after its database has been wiped.

  A subsequent Info request, including after Tenderdash reconnects without restarting Drive, then reaches the explicit app-hash-mismatch panic. The sentinel repairs this only on a Drive restart; returning REJECT_SNAPSHOT alone does not restore the promised self-consistent state. Keep reconstruction private until persistence and verification succeed, or reset the published state and committed-height guard when rejecting the restore. Add a late-failure test that calls Info before restarting the target.

In `packages/rs-drive-abci/src/platform_types/snapshot/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/platform_types/snapshot/mod.rs:52-60: Make the restore marker durable before wiping the database
  std::fs::write makes the sentinel visible but does not sync either its contents or its parent-directory entry. A host crash can therefore preserve some subsequent database deletions or restore writes while losing the sentinel. Startup then skips the recovery wipe and can encounter the database/platform-state mismatch that this marker is intended to prevent. This differs from terminating only the Drive process, where the operating system's page cache survives.

  Sync the marker and its directory entry before allowing the wipe. Also establish database durability before removing the marker after successful restoration or recovery, so marker removal cannot survive while the database operation it certifies is lost. The orderly restart tests do not establish this ordering because dropping Platform flushes GroveDB.

In `packages/rs-drive-abci/src/server.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/server.rs:44-49: Run serving-pin cleanup on the blocking pool
  release_expired_pins acquires a synchronous RwLock and removes entries through BTreeMap::retain. When an expired pin is the last owner of an already-pruned checkpoint, removing it runs Checkpoint::drop synchronously, including std::fs::remove_dir_all and subsequent database teardown. This timer therefore performs filesystem and database cleanup directly on a Tokio worker, potentially delaying unrelated gRPC work. The new snapshot request handlers already move comparable synchronous work to the blocking pool.

  Run the sweep in an awaited blocking task. Release removed checkpoint owners outside the pin-map lock as well, so directory deletion does not hold up other snapshot requests.

Comment thread packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs
Comment thread packages/rs-drive-abci/src/abci/handler/list_snapshots.rs
Comment thread packages/rs-drive-abci/src/platform_types/snapshot/mod.rs Outdated
Comment thread packages/rs-drive-abci/src/server.rs Outdated

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Phase 1 blocker gate

The latest commit fixes the previously identified restore durability, state publication, pin cleanup, and chunk-generation bounding issues. Two state-sync correctness defects remain: packed 32-byte global chunk IDs are rejected by the serving validator, and checkpoint discovery traverses unrelated GroveDB subtrees until its 4096-path budget is exhausted, causing valid snapshots to be omitted. The discovery routine also fails to enforce its documented full-page cutoff, allowing unsupported trees beyond the first page to be missed.

Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.

🔴 2 blocking | 🟡 1 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)

  • Triage: critical by gpt-6-astra (effort low) — This large, intricate diff changes consensus behavior and peer-facing ABCI snapshot deserialization plus storage restoration, notably in run_block_proposal, snapshot handlers, reduced-state persistence, and platform-state reconstruction.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 81% left, 5h 74% left
  • Fresh verifier: gpt-6-astra — verifier; agent astra-gate-verifier
  • Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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/handler/load_snapshot_chunk.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/load_snapshot_chunk.rs:30-42: Allow 32-byte global IDs in packed chunk-batch validation
  `validate_chunk_id_batch` explicitly accepts 32-byte IDs when unpacking a nested batch at line 19, but the subsequent loop rejects every ID shorter than 35 bytes. `apply_snapshot_chunk::split_chunk_ids` preserves 32-byte global IDs and packs them into follow-up requests, so a valid packed request containing such an ID reaches this loop and is rejected as malformed. Handle the 32-byte form before parsing the 35-byte follow-up descriptor.

In `packages/rs-drive-abci/src/abci/handler/list_snapshots.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/abci/handler/list_snapshots.rs:86-119: Do not silently skip unsupported trees beyond a full discovery page
  The comment says that a full 1024-element page must conservatively disqualify a checkpoint because an unsupported tree may occur beyond the returned page. The implementation never checks `elements.len() == DISCOVERY_PAGE_SIZE`; it traverses only the returned elements and can therefore advertise a checkpoint when an indexed tree is later in a truncated subtree. Add the full-page check and exclude that checkpoint, or continue discovery with pagination. The traversal also queries primary document leaf trees unnecessarily; bypassing those paths would reduce the I/O cost while preserving detection of indexed subtrees.
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/list_snapshots.rs:72-124: Account for ranked-index trees before advertising snapshots as restorable
  (existing thread: https://github.com/dashpay/platform/pull/4648#discussion_r3981308481)
  The discovery walk starts at the GroveDB root and recursively enqueues every subtree. It treats exceeding `DISCOVERY_MAX_PATHS` as evidence that an indexed tree exists, but normal platform state includes many unrelated subtrees under identities, pools, contracts, and other system data. Once more than 4096 paths are inspected, a healthy checkpoint is incorrectly marked as containing indexed trees and omitted from `list_snapshots`, disabling snapshot serving. Restrict discovery to the data-contract document hierarchy (where indexed trees can occur), or otherwise inspect only paths whose element types can contain ranked indexes instead of using the global database traversal budget.

Comment on lines +30 to +42
let mut seen = std::collections::HashSet::new();
for global_id in global_ids {
if !seen.insert(global_id.clone()) {
return Err(AbciError::StateSyncBadRequest(
"load_snapshot_chunk contains duplicate global chunk ids".to_string(),
));
}
if global_id.len() < 35 {
return Err(AbciError::StateSyncBadRequest(
"load_snapshot_chunk malformed global chunk id".to_string(),
));
}
let root_key_len = u16::from_be_bytes([global_id[32], global_id[33]]) as usize;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Allow 32-byte global IDs in packed chunk-batch validation

validate_chunk_id_batch explicitly accepts 32-byte IDs when unpacking a nested batch at line 19, but the subsequent loop rejects every ID shorter than 35 bytes. apply_snapshot_chunk::split_chunk_ids preserves 32-byte global IDs and packs them into follow-up requests, so a valid packed request containing such an ID reaches this loop and is rejected as malformed. Handle the 32-byte form before parsing the 35-byte follow-up descriptor.

Suggested change
let mut seen = std::collections::HashSet::new();
for global_id in global_ids {
if !seen.insert(global_id.clone()) {
return Err(AbciError::StateSyncBadRequest(
"load_snapshot_chunk contains duplicate global chunk ids".to_string(),
));
}
if global_id.len() < 35 {
return Err(AbciError::StateSyncBadRequest(
"load_snapshot_chunk malformed global chunk id".to_string(),
));
}
let root_key_len = u16::from_be_bytes([global_id[32], global_id[33]]) as usize;
if global_id.len() == 32 {
continue;
}
if global_id.len() < 35 {
return Err(AbciError::StateSyncBadRequest(
"load_snapshot_chunk malformed global chunk id".to_string(),
));
}

source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality, security-auditor)

Comment on lines +86 to +119
let mut query = Query::new();
query.insert_item(QueryItem::RangeFull(RangeFull));
// A checkpoint can contain millions of document records. Only the first
// bounded page is needed to find the tree elements that state sync cannot
// restore; if the page is full, conservatively do not advertise the
// checkpoint because the unsupported element may be beyond it.
let path_query = PathQuery::new(
path.clone(),
SizedQuery::new(query, Some(DISCOVERY_PAGE_SIZE), None),
);
let (elements, _) = checkpoint
.grove_db
.query_raw(
&path_query,
false,
true,
true,
QueryResultType::QueryKeyElementPairResultType,
None,
grove_version,
)
.value
.map_err(Error::from)?;
let elements = elements.to_key_elements();
for (key, element) in elements {
if element.is_indexed_tree() {
contains_indexed_tree = true;
break;
}
if element.is_any_tree() {
let mut child_path = path.clone();
child_path.push(key);
pending_paths.push(child_path);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Do not silently skip unsupported trees beyond a full discovery page

The comment says that a full 1024-element page must conservatively disqualify a checkpoint because an unsupported tree may occur beyond the returned page. The implementation never checks elements.len() == DISCOVERY_PAGE_SIZE; it traverses only the returned elements and can therefore advertise a checkpoint when an indexed tree is later in a truncated subtree. Add the full-page check and exclude that checkpoint, or continue discovery with pagination. The traversal also queries primary document leaf trees unnecessarily; bypassing those paths would reduce the I/O cost while preserving detection of indexed subtrees.

source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality, security-auditor)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants