Repository navigation
feat(platform-wallet)!: detect inconsistent shielded commitment trees and add explicit rebuild - #5565
Claudius-Maginificent wants to merge 13 commits into
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
 |
|
⛔ Final review complete — 2 blocking finding(s) (commit f666fe5) |
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.
…tree-consistency Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ielded tree rewind_scan_watermarks reset only last_synced_index, so the local balance snapshot kept reporting the pre-rebuild last_scanned_index and a ScannedThisSession source while the commitment tree was empty. Clear the scan coverage with the cursor and mark retained notes as Restored (NoHistory when the account has none) until the rescan completes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t shielded tree Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tree-consistency Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tch concurrency Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and restart sync after rebuild The background sync() path now reports an inconsistent commitment tree as WalletShieldedOutcome::TreeInconsistent instead of a stringified Err, so hosts detect it without matching message text. rebuild_shielded_tree restarts the background shielded sync loop it quiesced whenever the loop was running before, on every outcome, so the host no longer has to restart sync after the repair. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…le hosts - FFI: add ErrorShieldedTreeInconsistent = 67 (61-66 are claimed by open PRs #5395, #5487 and #5536), mapped by the blanket From and map_shielded_error; append error_code to ShieldedSyncWalletResultFFI so the sync-completed callback carries it per wallet; add the platform_wallet_manager_shielded_rebuild_tree export. - JNI: add shieldedRebuildTree; onShieldedSyncCompleted gains a trailing errorCode argument. - Kotlin: PlatformWalletManager.rebuildShieldedTree(), typed DashSdkError.PlatformWallet.ShieldedTreeInconsistent and WalletSyncEvent.ShieldedResult.error. - Swift: PlatformWalletManager.rebuildShieldedTree(), typed PlatformWalletError.shieldedTreeInconsistent and ShieldedWalletSyncResult.error. The appended repr(C) field changes the callback array layout: Swift and Kotlin must ship with the matching native library. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The Rust implementation correctly detects excess shielded-tree leaves, preserves the local ledger during rebuild, and now tests the typed background-sync failure path. A blocking mobile integration gap remains: the new terminal condition and preserving rebuild are not exposed through the FFI or mobile wrappers, leaving hosts with only a generic error and destructive clear operation. The restart-before-rescan metadata issue and a concurrent unregister attribution race also remain; the Kotlin build check is queued and policy reconciliation is still in progress, with no failing code check in the supplied CI snapshot.
🔴 1 blocking | 🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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 wallet shielded-tree persistence and recovery, including a SQLite storage migration and concurrency-sensitive logic that affects spend availability and preservation of spent state. - 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/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1298-1307: Reset host-visible scan metadata during a tree rebuild
`rebuild_shielded_tree` rewinds only the coordinator's in-memory scan state. It does not emit a host-persistence update, so a process restart before the next full scan can restore the old persisted watermark and scan metadata through `restore_balance_metadata` while the SQLite commitment tree is empty. The next sync can recover because its local watermark is capped by the tree size, but a pre-rescan balance snapshot can still report stale scan coverage and restored state. Persist an explicit rebuild/reset marker or otherwise invalidate persisted scan metadata when the tree is reset, and cover the restart-before-rescan sequence.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1341-1351: Preserve the pass snapshot when reporting sync errors
`try_sync` snapshots the registered subwallets before the network pass, but `sync` re-reads `self.accounts` after `try_sync` returns an error. Because `unregister_wallet` can run concurrently and the sync path does not hold the coordinator lifecycle mutex, the error summary can omit a wallet that participated in the failed pass or attribute the failure to a newly registered wallet with the same identifier. Preserve the original pass snapshot for the error-reporting path and use it when calling `fail_all_wallets`.
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
(existing thread: https://github.com/dashpay/platform/pull/5565#discussion_r4365720739)
The new `rebuild_shielded_tree` operation exists only on the Rust manager/coordinator. `platform_wallet_manager_shielded_sync_wallet` still routes `ShieldedTreeInconsistent` through `map_shielded_error`, which maps it to generic `ErrorWalletOperation` with a rendered message, and no FFI export or Swift/Kotlin wrapper invokes the preserving rebuild. Consequently mobile hosts cannot reliably distinguish this terminal condition from a transient sync failure or perform the non-destructive repair. `platform_wallet_manager_shielded_clear` is not equivalent because it removes registrations, notes, spent status, reservations, and related recovery state. The PR's Rust-only framing does not remove this responsibility because the advertised recovery flow is intended for wallet applications and there is no linked follow-up tracking the required FFI work.
| pub async fn rebuild_shielded_tree(&self) -> Result<(), PlatformWalletError> { | ||
| let _lifecycle = self.lifecycle.lock().await; | ||
| let mut store = self.store.write().await; | ||
| // Rewind even if the durable reset fails partway through. Rescanning | ||
| // an intact tree is harmless; skipping a partially reset tree is not. | ||
| store.rewind_scan_watermarks(); | ||
| if let Ok(mut caught_up) = self.last_caught_up_at.lock() { | ||
| *caught_up = None; | ||
| } | ||
| store.reset_commitment_tree()?; |
There was a problem hiding this comment.
🟡 Suggestion: Reset host-visible scan metadata during a tree rebuild
rebuild_shielded_tree rewinds only the coordinator's in-memory scan state. It does not emit a host-persistence update, so a process restart before the next full scan can restore the old persisted watermark and scan metadata through restore_balance_metadata while the SQLite commitment tree is empty. The next sync can recover because its local watermark is capped by the tree size, but a pre-rescan balance snapshot can still report stale scan coverage and restored state. Persist an explicit rebuild/reset marker or otherwise invalidate persisted scan metadata when the tree is reset, and cover the restart-before-rescan sequence.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
No code change. Host scan rows are advance-only by contract at every layer:
- Swift
persistShieldedSyncedIndiceswrites only when the value is>(PlatformWalletPersistenceHandler.swift:5895). - Kotlin
onPersistShieldedSyncedIndexreturns early when the existing value is>=(:2533). - The Rust changeset merge takes the max.
A rewind changeset would be silently dropped. Fixing it means a new reset callback across FFI, JNI, Swift and Kotlin, which is out of scope here.
Correctness does not depend on those rows. sync_notes_across caps every restored watermark at the tree size (sync.rs:238-240), so a restart after a rebuild still rescans from 0. Only the displayed scan coverage stays stale until the rescan passes it. Follow-up: a host reset signal for shielded sync rows, shared with Clear.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Still applies (re-reviewed at f666fe55): The Rust-side rewind is present, but the separate advance-only host rows still receive no reset or epoch. Restarting before rescanning therefore continues to expose stale host-visible scan coverage.
…d win rebuild_shielded_tree restarted background sync whenever it was running before, so a stop or Clear issued while the rebuild was in flight was silently undone. ShieldedSyncManager now counts stop requests (stop, quiesce, Clear) and restarts only if none arrived since the rebuild began; one lock spans that check and the start, so no stop slips between them. The internal quiesce used by rebuild cancels the loop without recording a stop request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The PR correctly adds typed inconsistency reporting and a preserving rebuild across the Rust, FFI, JNI, Swift, and Kotlin layers. Two blocking lifecycle/correctness defects remain: count-only detection can permanently miss an initially oversized but misaligned tree after chain growth, and failed or timed-out rebuilds can restart background sync against a closed or partially reset tree. Several non-blocking state and callback consistency issues also remain.
🔴 2 blocking | 🟡 4 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Suppress buffered Kotlin completion events after shielded-tree rebuild
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:263-267
The native callback bridge places shielded completion events into a buffered MutableSharedFlow with _syncEvents.tryEmit, but the Kotlin manager has no generation or invalidation boundary for rebuilds. A ShieldedTreeInconsistent event admitted before or during rebuildShieldedTree() can remain buffered and be delivered after the rebuild succeeds and background sync restarts, causing the host to display the repair state again or offer a redundant rebuild. Invalidate events admitted before the rebuild, or carry a native lifecycle generation through the callback and discard stale events.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: 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 that modifies persisted wallet storage and recovery semantics in packages/rs-platform-wallet/src/wallet/shielded/file_store.rs, while also changing cross-language ABI surfaces. - 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 final gate: an independent Phase-2 review ran after iterative findings were reconciled
- 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— ffi-engineer (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,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— ffi-engineer (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/wallet/shielded/sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:555-566: Count-only detection can miss a corrupted tree after chain growth
The inconsistency verdict only checks whether `local_leaf_count > chain_leaf_count`; it never validates that the commitments at the overlapping positions match. For example, a concurrent-append defect can leave the local tree as `[A, B, B]` while the chain is `[A, B]`. If the chain appends `C` before the next check, both counts become three, so this condition no longer fires even though position two is wrong. The append gate at lines 468-470 then treats every fetched position as already present, skips the real `C`, advances the watermark, and leaves the tree permanently misaligned. This is different from the documented same-size-corruption limitation because the corruption initially has an excess leaf; ordinary chain growth masks the count inequality before detection. Compare authenticated commitment/root evidence for the overlapping range, or persist a durable rebuild-required marker as soon as an excess tree is observed.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/mod.rs:1042-1053: Do not restart background sync after a failed tree rebuild
The restart decision depends only on whether the worker was running before the operation, so it restarts even when `rebuild_shielded_tree_quiesced()` returns an error. The reset rewinds in-memory cursors before deleting several SQLite tables, checkpointing the WAL, and reopening the commitment-tree wrapper. A delete, checkpoint, or reopen failure can therefore leave the on-disk tables partially reset while the live wrapper still holds its old frontier; restarting sync in that state can append or skip against inconsistent tree state. The same problem occurs on a quiesce timeout: the gate is intentionally latched closed, but this code restarts a worker whose `sync_now` calls will immediately bail, leaving `is_running()` true while admission remains blocked. Restart only after a successful rebuild, and leave the worker stopped after any reset or quiesce error.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:263-267: Suppress buffered Kotlin completion events after shielded-tree rebuild
The native callback bridge places shielded completion events into a buffered `MutableSharedFlow` with `_syncEvents.tryEmit`, but the Kotlin manager has no generation or invalidation boundary for rebuilds. A `ShieldedTreeInconsistent` event admitted before or during `rebuildShieldedTree()` can remain buffered and be delivered after the rebuild succeeds and background sync restarts, causing the host to display the repair state again or offer a redundant rebuild. Invalidate events admitted before the rebuild, or carry a native lifecycle generation through the callback and discard stale events.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:493-502: Coordinate Swift callback generations with the native rebuild restart
Rust restarts background sync inside `platform_wallet_manager_shielded_rebuild_tree`, before this Swift method increments `shieldedSyncGeneration`. A callback from the newly restarted pass can therefore snapshot the pre-rebuild generation on the FFI thread. Its MainActor task is queued while the synchronous rebuild call is still executing; after the native call returns, Swift bumps the generation and the legitimate post-rebuild callback is discarded as stale. Bracket the rebuild/restart lifecycle with an explicit callback-generation protocol, or defer the Rust restart until the host has invalidated callbacks.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1340-1352: Preserve the pass snapshot when reporting sync errors
(existing thread: https://github.com/dashpay/platform/pull/5565#discussion_r4403439232)
`try_sync` snapshots the registered subwallets before the awaited network pass, but `sync` reconstructs the wallet list from `self.accounts` only after the pass fails. A concurrent unregister can remove a wallet that participated in the failed pass, so it receives no failure outcome; a concurrent registration can add a wallet that did not participate, so it receives an error for a pass it never ran. Carry the pre-pass snapshot through the error path and use it for `fail_all_wallets`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1298-1307: Reset host-visible scan metadata during a tree rebuild
(existing thread: https://github.com/dashpay/platform/pull/5565#discussion_r4403439226)
`rewind_scan_watermarks` now resets the coordinator's in-memory cursors, but the host-persisted `ShieldedSyncStartState` remains unchanged. If the process exits after a successful rebuild and before the next full scan, the next bind restores the old watermark through the advance-only `restore_balance_metadata` path. The sync path caps that watermark to the empty tree and will eventually rescan safely, but host-visible balance metadata can still claim the old scan coverage before that rescan completes. Persist an explicit rebuild/reset marker or otherwise invalidate the host persistence as part of the rebuild, and cover a restart before the first rescan.
| if let Some(chain_leaf_count) = chain_leaf_count { | ||
| let local_leaf_count = store | ||
| .tree_size() | ||
| .map_err(|e| PlatformWalletError::ShieldedStoreError(e.to_string()))?; | ||
| if local_leaf_count > chain_leaf_count { | ||
| // The stream's maximum count includes the proof at response_height. | ||
| // Only that height (or newer) can contradict our tree's bound. | ||
| if tree_height.is_some_and(|height| response_height >= height) { | ||
| return Err(PlatformWalletError::ShieldedTreeInconsistent { | ||
| local_leaf_count, | ||
| chain_leaf_count, | ||
| }); |
There was a problem hiding this comment.
🔴 Blocking: Count-only detection can miss a corrupted tree after chain growth
The inconsistency verdict only checks whether local_leaf_count > chain_leaf_count; it never validates that the commitments at the overlapping positions match. For example, a concurrent-append defect can leave the local tree as [A, B, B] while the chain is [A, B]. If the chain appends C before the next check, both counts become three, so this condition no longer fires even though position two is wrong. The append gate at lines 468-470 then treats every fetched position as already present, skips the real C, advances the watermark, and leaves the tree permanently misaligned. This is different from the documented same-size-corruption limitation because the corruption initially has an excess leaf; ordinary chain growth masks the count inequality before detection. Compare authenticated commitment/root evidence for the overlapping range, or persist a durable rebuild-required marker as soon as an excess tree is observed.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
The mechanism is real, but it is the documented scope of this PR. The verdict is count-based, and validating the overlapping range needs root or anchor evidence. ShieldedAnchors has no heights or sizes today, so that classification is explicitly deferred (PR body, "Deferred").
Masking also needs the chain to grow by exactly the excess before the first fresh-evidence pass. #5564 removes the duplicate-append source that produced the excess. A durable "rebuild suspected" marker means schema work, so it goes into the same follow-up as root-based verification.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Still applies (re-reviewed at f666fe55): The count-only check and position-skipping gate are unchanged in substance, so ordinary chain growth can still make an initially detectable excess count-equal before the detector runs again. Deferring root evidence without a tracking reference does not prevent the permanent misalignment.
| public func rebuildShieldedTree() throws { | ||
| try ensureShieldedLifecycleAllowed("rebuildShieldedTree") | ||
| try ensureNoShieldedStopInFlight(before: "rebuilding the shielded tree") | ||
| try withShieldedLocalBalanceMutation { | ||
| try platform_wallet_manager_shielded_rebuild_tree(handle).check() | ||
| } | ||
| // Drop a trailing completion from the pass that found the tree | ||
| // inconsistent, so it cannot re-surface the error after the repair. | ||
| shieldedSyncGeneration.bump() | ||
| resetCurrentShieldedProgress() |
There was a problem hiding this comment.
🟡 Suggestion: Coordinate Swift callback generations with the native rebuild restart
Rust restarts background sync inside platform_wallet_manager_shielded_rebuild_tree, before this Swift method increments shieldedSyncGeneration. A callback from the newly restarted pass can therefore snapshot the pre-rebuild generation on the FFI thread. Its MainActor task is queued while the synchronous rebuild call is still executing; after the native call returns, Swift bumps the generation and the legitimate post-rebuild callback is discarded as stale. Bracket the rebuild/restart lifecycle with an explicit callback-generation protocol, or defer the Rust restart until the host has invalidated callbacks.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
There was a problem hiding this comment.
The window runs from the Rust start() to the Swift bump, which is microseconds. The first post-rebuild pass is a full rescan from zero, so no completion can land inside it. Only an instantly failing pass, or the first progress tick, could be dropped, and the next pass reports it again.
Closing the window would mean Rust not restarting and Swift/Kotlin restarting after the bump. That pushes call order back onto hosts, which this PR deliberately removed.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Still applies (re-reviewed at f666fe55): The queued callback still captures its generation before the synchronous rebuild returns, so the later Swift bump can discard a legitimate callback from the newly restarted pass. A full rescan reduces the probability but does not eliminate fast-failure or early-progress callbacks.
The tree reset ran its five DELETEs as separate auto-committed statements, and the rebuild rewound scan cursors before the reset. A failure partway through left a half-emptied tree under a stale live frontier, with cursors already at zero. The background sync restarted by `rebuild_shielded_tree` then ran on that half-reset state. The DELETEs and the replacement tree wrapper are now staged in one transaction. The wrapper opens on the staging connection, so it reads the uncommitted empty state, and the live tree is swapped only after COMMIT. Any earlier failure rolls back. The rebuild rewinds cursors only once the live tree is empty, so a failed reset leaves both untouched and the next pass reports the same inconsistency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
…e timeout A quiesce timeout latched the sync gate, and only a later successful drain cleared it. `rebuild_shielded_tree` still restarts background sync on that path, so the restarted loop reported `is_running()` while every pass bailed on the latched gate. The rebuild resets nothing when its quiesce times out, so it now drains with the new `DrainTimeout::Release` policy. That policy drops only its own hold and sets no latch. Passes resume once the wedged pass releases `is_syncing`. Stop, Clear and shutdown keep the latching drain, and a latch set by an earlier drain stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
… it ran for Splitting out `try_sync` made the error path of `sync()` re-read the account registry. A wallet registered or unregistered mid-pass then changed which wallets the failure was reported for. `sync()` and `try_sync()` now share `run_pass`, whose failure carries the pass's own subwallet snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
|
On the Kotlin 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The typed inconsistency signal, transactional tree replacement, pass-snapshot attribution, and mobile plumbing are implemented correctly. Two blocking lifecycle/correctness issues remain: count-only detection can be masked by subsequent chain growth, and a prior quiesce latch can cause the automatic restart to run without admitting any passes. Several non-blocking callback, persistence, durability, and async-threading issues also remain. CI checks covering policy/runner selection were still pending at the supplied snapshot.
🔴 2 blocking | 🟡 5 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Suppress buffered Kotlin completion events after shielded-tree rebuild
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:263-267
The Kotlin MutableSharedFlow has extraBufferCapacity = 256 and no generation or reset fence around rebuild or Clear. An active slow collector can receive a pre-rebuild ShieldedTreeInconsistent completion after the native tree has already been successfully reset and rescanning has begun. The flow's replay = 0 does not discard events already buffered for active collectors. This became actionable in this PR because code 67 is a terminal, repair-triggering result; add an owner-controlled generation/reset epoch or an equivalent ordered invalidation boundary.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, ffi-engineer, rust-quality)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, intricate cross-language change to wallet storage, synchronization, recovery logic, and FFI/ABI surfaces, but it does not itself modify consensus, funds movement, cryptography, key handling, peer-facing deserialization, or a storage migration. - 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) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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:1090-1093: Do not restart while an earlier quiesce latch remains
`quiesce_held_releasing_within` releases only its own hold on timeout; an older latch created by a previous timed-out `quiesce` remains set. If the current rebuild also times out while the pass is wedged, `was_running` can still be true and `restart_unless_stopped_since` starts a worker while the gate remains closed. That worker reports as running but every pass exits at the quiescing-gate check, and no later event clears the old latch. The rebuild must not restart until admission is known to be open, or it must clear an old latch only after a successful drain.
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:1090-1093: Do not synchronously reap the worker from an async rebuild future
`rebuild_shielded_tree_within` is async but directly calls `restart_unless_stopped_since`, which calls `ThreadRegistry::start_thread`. The registry documents that restart can synchronously spin for up to its reap backstop and must not be called directly from an async context; on a current-thread runtime it can also hit the registry's multi-thread assertion. Move the restart/reap operation to `spawn_blocking` with join-error propagation, or expose an async-safe registry restart API.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:263-267: Suppress buffered Kotlin completion events after shielded-tree rebuild
The Kotlin `MutableSharedFlow` has `extraBufferCapacity = 256` and no generation or reset fence around rebuild or Clear. An active slow collector can receive a pre-rebuild `ShieldedTreeInconsistent` completion after the native tree has already been successfully reset and rescanning has begun. The flow's `replay = 0` does not discard events already buffered for active collectors. This became actionable in this PR because code 67 is a terminal, repair-triggering result; add an owner-controlled generation/reset epoch or an equivalent ordered invalidation boundary.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1098-1121: Classify post-commit checkpoint failures separately from reset failures
`reset_commitment_tree` swaps in the empty live tree after `COMMIT`, then runs `PRAGMA wal_checkpoint(TRUNCATE)` and returns an error if that checkpoint fails. The coordinator consequently rewinds cursors and the manager restarts background sync even though the public rebuild operation reports failure. The logical tree is consistent, but the documented all-or-nothing contract says a failed reset leaves state untouched, and hosts cannot distinguish a committed reset with reduced crash durability from a rolled-back reset that should be retried. Return a distinct committed-but-not-checkpointed result or otherwise make the post-commit state explicit to callers.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:555-567: Count-only detection can miss a corrupted tree after chain growth
(existing thread: https://github.com/dashpay/platform/pull/5565#discussion_r4405452073)
The verdict still compares only `local_leaf_count > chain_leaf_count`; it does not compare authenticated commitments in the overlapping range or persist a rebuild-required marker. A local tree such as `[A, B, B]` versus chain `[A, B]` is initially detectable, but after the chain appends `C`, both counts become three and the check no longer fires. The append gate then skips position two because `global_pos < tree_size`, advances the watermark, and preserves the wrong commitment permanently. The documented absence of root/size evidence explains why the check is difficult, but the PR introduces the detection/rebuild path and this untracked limitation can defeat its stated recovery goal. Add authenticated overlap/root validation or persist a durable rebuild-required state as soon as excess leaves are observed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:493-502: Coordinate Swift callback generations with the native rebuild restart
(existing thread: https://github.com/dashpay/platform/pull/5565#discussion_r4405452122)
Rust restarts background sync before this Swift method increments `shieldedSyncGeneration`. The FFI trampoline captures the current generation before enqueueing its `Task { @MainActor ... }`; therefore a fast failure or early progress/completion callback from the restarted pass can capture the old generation, remain queued while the synchronous rebuild call is executing, and then be discarded after the method bumps the generation. The full rescan makes the window unlikely but does not guarantee that no callback occurs during it. Coordinate the lifecycle epoch with the Rust restart or invalidate the host generation before the restart can begin.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1300-1316: Reset host-visible scan metadata during a tree rebuild
(existing thread: https://github.com/dashpay/platform/pull/5565#discussion_r4403439226)
`rewind_scan_watermarks` now resets Rust-side cursors and balance provenance, but `rebuild_shielded_tree` emits no invalidation for the Swift and Kotlin persistence rows. Those rows merge watermarks monotonically, so a restart before rescanning can restore the old host-visible scan coverage while the native tree is empty; the native cap prevents unsafe skipping, but UI and persisted status remain falsely advanced until the rescan catches up. Add a durable reset epoch or explicit host reset callback shared with Clear. The PR description defers this work but provides no tracking reference, so it cannot be treated as a resolved follow-up.
| if was_running { | ||
| self.shielded_sync_manager | ||
| .restart_unless_stopped_since(stop_intents); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Do not restart while an earlier quiesce latch remains
quiesce_held_releasing_within releases only its own hold on timeout; an older latch created by a previous timed-out quiesce remains set. If the current rebuild also times out while the pass is wedged, was_running can still be true and restart_unless_stopped_since starts a worker while the gate remains closed. That worker reports as running but every pass exits at the quiescing-gate check, and no later event clears the old latch. The rebuild must not restart until admission is known to be open, or it must clear an old latch only after a successful drain.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
| // Durably flush the DELETEs into the main database file with a | ||
| // TRUNCATE checkpoint. A failure here leaves the reset committed | ||
| // and the live tree consistent with it; only crash durability is | ||
| // lost, and the error still surfaces. | ||
| // | ||
| // Without this, Clear is a no-op across a hard kill. The store | ||
| // runs `synchronous=NORMAL` in WAL mode (see `open_tuned_connection`), | ||
| // so a committed transaction lands in the `-wal` file but is NOT | ||
| // fsync'd until a checkpoint. Other connections (`pending_conn`) | ||
| // stay open on the same file, so SQLite's last-connection-close | ||
| // auto-checkpoint never fires — the emptied tables live only in the | ||
| // WAL. On Android the "Clear" button is routinely followed by a | ||
| // force-stop (non-graceful SIGKILL, no checkpoint), so the WAL | ||
| // frames are discarded and the next launch reopens the OLD full | ||
| // tree (the 771/771 "Clear did nothing" symptom). A TRUNCATE | ||
| // checkpoint rewrites the main db and resets the WAL, making the | ||
| // emptied state durable regardless of how the process later dies. | ||
| // The prior graceful `drop(store)` in the unit test masked this — | ||
| // a clean close checkpoints, a SIGKILL does not. | ||
| run( | ||
| "PRAGMA wal_checkpoint(TRUNCATE);", | ||
| "checkpoint after tree reset", | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Classify post-commit checkpoint failures separately from reset failures
reset_commitment_tree swaps in the empty live tree after COMMIT, then runs PRAGMA wal_checkpoint(TRUNCATE) and returns an error if that checkpoint fails. The coordinator consequently rewinds cursors and the manager restarts background sync even though the public rebuild operation reports failure. The logical tree is consistent, but the documented all-or-nothing contract says a failed reset leaves state untouched, and hosts cannot distinguish a committed reset with reduced crash durability from a rolled-back reset that should be retried. Return a distinct committed-but-not-checkpointed result or otherwise make the post-commit state explicit to callers.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, rust-quality, security-auditor)
| if was_running { | ||
| self.shielded_sync_manager | ||
| .restart_unless_stopped_since(stop_intents); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Do not synchronously reap the worker from an async rebuild future
rebuild_shielded_tree_within is async but directly calls restart_unless_stopped_since, which calls ThreadRegistry::start_thread. The registry documents that restart can synchronously spin for up to its reap backstop and must not be called directly from an async context; on a current-thread runtime it can also hit the registry's multi-thread assertion. Move the restart/reap operation to spawn_blocking with join-error propagation, or expose an async-safe registry restart API.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
Basic explanation
What this does: Detects excess local shielded-tree leaves and adds an explicit repair that keeps wallet notes and spent status. Both are exposed to mobile hosts through the FFI, JNI, Swift and Kotlin.
Value: Wallet apps can now detect an inconsistent tree from a typed result code, on both the explicit and the background sync paths, and repair it with one call. Before this, spends stayed stuck permanently, or the only way out was a destructive Clear.
Risks:
rebuildShieldedTree(or Clear) for it.ShieldedSyncWalletResultFFIC layout changes (see Breaking Changes).Issue being fixed or feature implemented
Stacked on #5564, which prevents concurrent syncs from duplicating commitments. This follow-up detects excess leaves when fresh enough evidence is available, and provides an explicit recovery for existing stores. Legacy stores without provenance conservatively skip the verdict.
User story
As a mobile wallet user whose shielded spends keep failing because the local commitment tree drifted from the chain, I want the app to tell me the tree needs repair and fix it in one tap, without losing my shielded notes or history.
Scenario
ErrorShieldedTreeInconsistent(67):ShieldedWalletSyncResult.error == .shieldedTreeInconsistent.WalletSyncEvent.ShieldedResult.error is DashSdkError.PlatformWallet.ShieldedTreeInconsistent.sync_walletreturns the same code.rebuildShieldedTree().What was done?
Detailed discussion
Detection (rs-platform-wallet)
ShieldedChunkBatch.total_count, independently of the progress denominator.ShieldedTreeInconsistent { local_leaf_count, chain_leaf_count }only when an emitted response is at least as fresh as the tree's height bound, and before checkpointing or advancing scan cursors. Older responses are logged at debug level and do not rewind scan cursors.Typed signal on both sync paths
NetworkShieldedCoordinator::try_syncandShieldedSyncManager::sync_walletkeep typed failures.sync()path reports the verdict as the newWalletShieldedOutcome::TreeInconsistent { local_leaf_count, chain_leaf_count }, not asErr(String). Hosts never match message text.Repair owns its call order
PlatformWalletManager::rebuild_shielded_treeresets only the tree and scan cursors. Notes, nullifiers, spent status, reservations and registrations survive.TreeInconsistentagain.DrainTimeout::Release). Passes resume once the wedged pass ends, andis_running()reports a loop that actually runs. Stop, Clear and shutdown keep the latching drain.stop,quiesce, or the FFI/Swift/Kotlin stop) or a Clear issued while the rebuild is in flight wins: the rebuild skips its restart.ShieldedSyncManagercounts stop requests under a lock that also covers the rebuild's "unchanged? then start" step, so no stop can slip between the check and the restart. Hosts need no guard of their own.FFI, JNI, Swift and Kotlin
These layers only marshal; every decision stays in Rust.
PlatformWalletFFIResultCode::ErrorShieldedTreeInconsistent = 67.From<PlatformWalletError>and bymap_shielded_error, which coversplatform_wallet_manager_shielded_sync_walletand the new rebuild export.ERROR_CODE_REGISTRY.md; the next allocatable code is 68.ShieldedSyncWalletResultFFI.error_code.Successfor ok and skipped wallets,ErrorShieldedTreeInconsistentfor the new outcome,ErrorWalletOperationfor any other failure.error_messagestill carries the text.platform_wallet_manager_shielded_rebuild_tree(handle) -> PlatformWalletFFIResult.ErrorShutdownIncomplete.map_shielded_error.rs-unified-sdk-jni):Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_shieldedRebuildTree.onShieldedSyncCompletedgains a trailingerrorCode: Int. Its descriptor becomes([BZZZIJIJLjava/lang/String;I)V.WalletManagerNative.shieldedRebuildTree(managerHandle).PlatformWalletManager.rebuildShieldedTree()(suspend, runs onDispatchers.IO).DashSdkError.PlatformWallet.ShieldedTreeInconsistent(code 67, not retryable).DashSdkError.fromPlatformWalletCode(code, message).WalletSyncEvent.ShieldedResult.errorCodeand its typederror: DashSdkError?.NativeWalletEventBridge.onShieldedSyncCompleted(..., errorCode).PlatformWalletManager.rebuildShieldedTree().PlatformWalletResultCode.errorShieldedTreeInconsistent = 67.PlatformWalletError.shieldedTreeInconsistent(String).ShieldedWalletSyncResult.errorCodeand its typederror: PlatformWalletError?.Deferred
ShieldedAnchorscontains only roots, without anchor heights or tree sizes. A missing root cannot reliably distinguish a wallet that is behind or mid-block from a corrupted one. The same evidence is needed to catch an excess that chain growth masks before the first fresh check.How Has This Been Tested?
All Rust tests run offline, against deterministic SDK mocks and temporary SQLite stores.
cargo nextest run -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --all-features: all passed (platform-wallet 1650, platform-wallet-ffi + JNI 562, including the integration suites).Coverage for the existing detection and repair work:
New tests:
tree_consistency_tests::should_rebuild_tree_preserving_notes_and_spends_then_match_recorded_anchor: backgroundsync()now reportsWalletShieldedOutcome::TreeInconsistent.manager::tests::should_restart_background_shielded_sync_after_rebuild_when_it_was_runningmanager::tests::should_leave_background_shielded_sync_stopped_after_rebuild_when_it_was_not_runningmanager::tests::should_restart_background_shielded_sync_when_rebuild_failsmanager::tests::should_not_restart_background_shielded_sync_stopped_during_rebuildmanager::tests::should_admit_passes_after_restart_when_rebuild_quiesce_times_outtree_consistency_tests::should_leave_tree_and_cursors_untouched_when_rebuild_reset_failstree_consistency_tests::should_attribute_a_failed_pass_to_the_wallets_it_snapshotted: a failed background pass is attributed to the wallets it snapshotted, even when one unregisters mid-pass.error::tests::should_map_shielded_tree_inconsistent_to_pinned_code_67shielded_sync::recovery_error_tests::should_preserve_recovery_errors_when_configuring_binding_or_syncing: now includes the inconsistent tree.event_handler::release_tests::should_report_typed_error_codes_through_the_shielded_sync_callbackshielded_sync::rebuild_tree_tests::should_reject_rebuild_on_an_invalid_handleshielded_sync::rebuild_tree_tests::should_rebuild_tree_keep_registration_and_restore_background_syncOther checks:
cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi -p rs-unified-sdk-ffi -p rs-unified-sdk-jni --all-features --all-targets -- --no-deps -D warnings: passed.cargo fmt --all: clean.Not built here; CI covers them:
DashSdkErrorTest.shouldMapAnInconsistentShieldedTreeToItsTypedErrorandWalletEventFanOutTest.shouldTypeAnInconsistentShieldedTreeFromItsResultCode.ErrorHandlingTests.testShouldPreserveShieldedTreeInconsistentFromFFIandtestShouldTypeShieldedSyncCallbackResultFromItsErrorCode.Breaking Changes
PlatformWalletErrorenum gainsShieldedTreeInconsistent.WalletShieldedOutcomeenum gainsTreeInconsistent.sync_walletbehaviour.ShieldedSyncFailed.#[repr(C)] ShieldedSyncWalletResultFFIgains a trailingerror_codefield. Hosts compiled against the old layout read the callback array with the wrong stride, so the Swift and Kotlin SDKs must ship together with this native library.onShieldedSyncCompletedsignature gains a trailingerrorCode: Int. CustomNativeWalletEventBridgesubclasses must add the parameter.Existing FFI function signatures are unchanged. The new FFI, JNI, Swift and Kotlin entry points are additive.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
https://claude.ai/code/session_01WunYYjpfLux1ByK7fGF7SG
🤖 Co-authored by Claudius the Magnificent AI Agent