Repository navigation
feat(platform-wallet)!: detect inconsistent shielded commitment trees and add explicit rebuild - #5565
Conversation
…ify rebuild RED: a duplicated tree returns Ok instead of rejecting the sync (3 passed, 1 failed). Typed-error and rebuild contracts additionally fail compilation until their APIs exist. Fixtures are offline and deterministic. Co-Authored-By: OpenAI Codex <noreply@openai.com> Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
… and add explicit rebuild Require a response at least as fresh as the tree's recorded height bound before diagnosing excess leaves. Persist the bound before corresponding appends in the same SQLite database; skip stale or unknown provenance and never infer a height from another response's speculative count. Expose explicit tree-only rebuild while retaining notes, nullifiers and spent status. Use indexed nullifier lookups during receipt reconciliation. Keep spend-time classification deferred and add no FFI or automatic repair. BREAKING CHANGE: exhaustive matches on PlatformWalletError must handle ShieldedTreeInconsistent. Existing method signatures are unchanged. Validation: 224 offline shielded tests; one-block-behind regression fails without freshness comparison and passes with it. Clippy all-targets / all-features with --no-deps -D warnings, cargo fmt --all, git diff --check. Co-Authored-By: OpenAI Codex <noreply@openai.com> Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Final review complete — 1 blocking finding(s) (commit 7ff9831) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The tree consistency detection and preserving rebuild are generally coherent, but the advertised recovery path is unavailable to the FFI/mobile wallet consumers and the new typed error is flattened at that boundary. The implementation also has two non-blocking correctness and coverage gaps around proof-count freshness pairing and scan metadata after rebuild.
🔴 1 blocking | 🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to shielded-wallet persistence and concurrent synchronization, including a SQLite storage migration and recovery logic that can affect note, nullifier, and spend-state integrity. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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-platform-wallet/src/manager/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:1038-1051: Expose the preserving rebuild and typed inconsistency result through FFI
The only non-destructive recovery operation, `rebuild_shielded_tree`, is reachable only through the Rust manager/coordinator API. No corresponding entry point exists in `rs-platform-wallet-ffi` or the Swift/Kotlin wrappers. The FFI sync path maps `ShieldedTreeInconsistent` to the generic `ErrorWalletOperation` with a rendered string, so mobile hosts cannot reliably distinguish this terminal tree condition from a transient sync failure or invoke the required repair. The existing `shielded_clear` operation is not equivalent because it removes registrations, notes, spent status, and related wallet state. Add a thin FFI marshal entry point for the Rust rebuild method and a stable result-code mapping for `ShieldedTreeInconsistent`.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:418-420: Keep shielded count evidence paired with its proof height
`chain_leaf_count` is accumulated as the maximum of all emitted `batch.total_count` values, while `response_height` is accumulated independently. However, the SDK's `ShieldedChunkBatch.total_count` is itself a monotonic max across fetch completions, while `batch.block_height` belongs to the response that produced the emitted chunk. A stale or forked response with a larger count can therefore be carried into a batch emitted at a fresh height, causing the final gate to accept an excessive local tree because the stale larger count masks the fresh lower count. Preserve count and height as a pair, or only use count evidence from responses whose own proof height meets the recorded tree-height bound.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:151-155: Reset host-visible scan metadata during a tree rebuild
`rewind_scan_watermarks` resets only `last_synced_index`, but `last_scanned_index` remains at the pre-rebuild value and `balance_source` remains `ScannedThisSession`. Until the rescan completes, `local_balance_snapshot` can therefore report the old scan coverage and a successfully scanned session even though the commitment tree has been emptied. Reset the session metadata as part of the same rewind while retaining the note ledger as restored state.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1338-1354: Test background sync handling of ShieldedTreeInconsistent
The typed `try_sync` path is tested, but the background `sync()` path is not tested for this new failure. `sync()` catches `ShieldedTreeInconsistent`, converts it through `fail_all_wallets`, and exposes per-wallet `Err(String)` outcomes to the event layer. Add a test that drives `sync(false)` with an excessive local tree and fresh count evidence, then asserts the per-wallet error outcome and error count before rebuilding.
| pub async fn rebuild_shielded_tree(&self) -> Result<(), PlatformWalletError> { | ||
| let _clearing = self.registry.hold_clearing(WalletWorker::ShieldedSync); | ||
| let Some(_quiesced) = self.shielded_sync_manager.quiesce_held().await else { | ||
| return Err(PlatformWalletError::ShutdownIncomplete( | ||
| "shielded sync pass did not drain within the quiesce budget; rebuild aborted" | ||
| .to_string(), | ||
| )); | ||
| }; | ||
| let coordinator = self.shielded_coordinator().await.ok_or_else(|| { | ||
| PlatformWalletError::ShieldedStoreError( | ||
| "shielded rebuild requested but no coordinator is configured".to_string(), | ||
| ) | ||
| })?; | ||
| coordinator.rebuild_shielded_tree().await |
There was a problem hiding this comment.
🔴 Blocking: Expose the preserving rebuild and typed inconsistency result through FFI
The only non-destructive recovery operation, rebuild_shielded_tree, is reachable only through the Rust manager/coordinator API. No corresponding entry point exists in rs-platform-wallet-ffi or the Swift/Kotlin wrappers. The FFI sync path maps ShieldedTreeInconsistent to the generic ErrorWalletOperation with a rendered string, so mobile hosts cannot reliably distinguish this terminal tree condition from a transient sync failure or invoke the required repair. The existing shielded_clear operation is not equivalent because it removes registrations, notes, spent status, and related wallet state. Add a thin FFI marshal entry point for the Rust rebuild method and a stable result-code mapping for ShieldedTreeInconsistent.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)
| total_target = total_target.max(batch.total_count); | ||
| chain_leaf_count = Some(chain_leaf_count.unwrap_or(0).max(batch.total_count)); | ||
| response_height = response_height.max(batch.block_height); |
There was a problem hiding this comment.
🟡 Suggestion: Keep shielded count evidence paired with its proof height
chain_leaf_count is accumulated as the maximum of all emitted batch.total_count values, while response_height is accumulated independently. However, the SDK's ShieldedChunkBatch.total_count is itself a monotonic max across fetch completions, while batch.block_height belongs to the response that produced the emitted chunk. A stale or forked response with a larger count can therefore be carried into a batch emitted at a fresh height, causing the final gate to accept an excessive local tree because the stale larger count masks the fresh lower count. Preserve count and height as a pair, or only use count evidence from responses whose own proof height meets the recorded tree-height bound.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)
| /// Rewind all scan cursors without discarding receipts or spend guards. | ||
| pub(super) fn rewind_scan_watermarks(&mut self) { | ||
| for state in self.subwallets.values_mut() { | ||
| state.last_synced_index = 0; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Reset host-visible scan metadata during a tree rebuild
rewind_scan_watermarks resets only last_synced_index, but last_scanned_index remains at the pre-rebuild value and balance_source remains ScannedThisSession. Until the rescan completes, local_balance_snapshot can therefore report the old scan coverage and a successfully scanned session even though the commitment tree has been emptied. Reset the session metadata as part of the same rewind while retaining the note ledger as restored state.
| /// Rewind all scan cursors without discarding receipts or spend guards. | |
| pub(super) fn rewind_scan_watermarks(&mut self) { | |
| for state in self.subwallets.values_mut() { | |
| state.last_synced_index = 0; | |
| } | |
| for state in self.subwallets.values_mut() { | |
| state.last_synced_index = 0; | |
| state.last_scanned_index = None; | |
| state.balance_source = if state.notes.is_empty() { | |
| ShieldedBalanceSource::NoHistory | |
| } else { | |
| ShieldedBalanceSource::Restored | |
| }; | |
| } |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)
| /// [`PlatformEventManager::on_shielded_sync_completed`]: | ||
| /// crate::events::PlatformEventManager::on_shielded_sync_completed | ||
| pub async fn sync(&self, force: bool) -> ShieldedSyncPassSummary { | ||
| match self.try_sync(force).await { | ||
| Ok(summary) => summary, | ||
| Err(error) => { | ||
| let subwallets = self | ||
| .accounts | ||
| .read() | ||
| .await | ||
| .iter() | ||
| .map(|(id, views)| (*id, views.clone())) | ||
| .collect::<Vec<_>>(); | ||
| self.fail_all_wallets(&subwallets, &error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test background sync handling of ShieldedTreeInconsistent
The typed try_sync path is tested, but the background sync() path is not tested for this new failure. sync() catches ShieldedTreeInconsistent, converts it through fail_all_wallets, and exposes per-wallet Err(String) outcomes to the event layer. Add a test that drives sync(false) with an excessive local tree and fresh count evidence, then asserts the per-wallet error outcome and error count before rebuilding.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
Basic explanation
What this does: Detects excess local shielded-tree leaves and adds an explicit repair that retains wallet notes and spent status.
Value: Wallet applications can identify an inconsistent tree and request a full rescan instead of leaving spends permanently stuck.
Risks: No consensus changes. Repair requires a full pool download and trial decryption. The verdict requires a response at least as fresh as the local tree’s recorded height bound. Trees written before this change have no recorded height bound, so the check never fires for them: a store already corrupted by the #5564 bug (excess leaves) is not detected and needs an explicit
rebuild_shielded_tree(orclear_shielded) by the host. Equal-size content corruption remains outside this check.Issue being fixed or feature implemented
Stacked on #5564 (
ac9693ed7a), which prevents concurrent syncs from duplicating commitments. This follow-up detects excess leaves when sufficiently fresh evidence is available and provides explicit recovery for existing stores. Legacy stores without provenance conservatively skip the verdict.What was done?
ShieldedChunkBatch.total_count, independently of the progress denominator. ReturnShieldedTreeInconsistent { local_leaf_count, chain_leaf_count }only when an emitted response is at least as fresh as the tree’s height bound, before checkpointing or advancing scan cursors. Older responses are logged at debug level and do not rewind scan cursors.NetworkShieldedCoordinator::try_syncandShieldedSyncManager::sync_wallet; retain background summary formatting.rebuild_shielded_treeon the manager and coordinator. Reset only the tree and scan cursors; preserve notes, nullifiers, spent status, reservations, and registrations. No automatic rebuild and no FFI changes.ShieldedAnchorscontains only roots, without anchor heights or tree sizes. Missing roots cannot reliably distinguish a behind/mid-block wallet from corruption. Equal-size content corruption is outside this count check.How Has This Been Tested?
Offline, deterministic SDK mocks and temporary SQLite stores;
CARGO_BUILD_JOBS=4.cargo test --offline -p platform-wallet --all-features --lib wallet::shielded::: 224 passed. Covers concurrent syncs, healthy/behind/stale sources, provenance across SQLite reopen, speculative totals, typed detection, explicit rebuild, spent-note preservation, restored cursors, cooldown reset, and witness roots against mocked anchors.cargo clippy --offline -p platform-wallet --all-targets --all-features -- --no-deps -D warnings: passed.cargo fmt --allandgit diff --check.Breaking Changes
The public
PlatformWalletErrorenum is exhaustive: downstream exhaustive matches must handle the newShieldedTreeInconsistentvariant.sync_walletbehaviour changes: a wallet id that is not registered now skips the pass, and sync failures surface as their typed variants instead ofShieldedSyncFailed. Existing method signatures and FFI surfaces are unchanged.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
🤖 Generated with Claude Code
https://claude.ai/code/session_019H9THphMnuQyh9oPY13SoE