feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up)

[Claudius-Maginificent]

Issue being fixed or feature implemented

account_registrations rows currently have no binding to the wallet_id they're supposed to belong to beyond "whichever row the query happened to read." A data bug, a migration error, or disk corruption could silently associate a manifest row with the wrong wallet, causing platform-wallet to derive addresses under the wrong key while believing it's still the original wallet — the manifest trust-boundary gap tracked as Risk-6/R12.5 in the Model-A adoption risk assessment, deferred out of #3986's scope for a focused follow-up.

What was done?

An unkeyed SHA-256 integrity checksum, deliberately not a keyed MAC — a local-write-access adversary can recompute any value stored in the same file, so keying with an in-file secret (e.g. the store-generation token) buys no real security. Honest framing: this closes the accidental-corruption / cross-wallet-row class, not an authentication guarantee.

• Additive V004 migration: ALTER TABLE account_registrations ADD COLUMN checksum BLOB. Numbered V004, not V003 — see "Migration renumbering" below. V001/V002/V003 stay byte-identical.
• Checksum: SHA-256(wallet_id ‖ account_xpub_bytes) — exactly these two fixed identity fields, nothing else. Deliberately excludes meta_store_generation/meta_data_versions.seq, both of which rotate on legitimate restore/migrate — including either would make every restore false-positive as tampered.
• Writer: computed and bound inside the existing single flush transaction (same atomicity as the rest of the schema). Write-once in practice — account_registrations is immutable after account creation; address-pool growth lives in the separate core_address_pool table.
• Backfill: an idempotent app-side pass in open() fills the checksum for every pre-existing row (SQLite has no SHA-256 builtin, so this can't be a pure-DDL default). By the time load() runs, every row is covered — no legacy grace period.
• Verify-at-load is a per-wallet skip, not a batch abort: the existing manifest read path is fail-hard on any mismatch; a checksum failure instead surfaces through the storage crate's first use of the already-present ClientStartState.skipped channel — CorruptKind::ManifestIntegrityMismatch (#[non_exhaustive]-safe), FFI wire code 104 (SnapshotIdentityMismatch already claimed 103 upstream). One tampered row never aborts the whole load.
• Bulk oracle hardened too: all_platform_payment_registrations (used before the per-wallet loop) now also verifies-and-drops on mismatch — without this, a tampered platform_payment row would abort the load before the per-wallet skip machinery ever ran.

Rebased onto #3986's final state (core_address_pool's widened PK, deterministic pool-script lookup, blob-size gates, restore-ordering fix) — no interaction between the two: this checksum touches only account_registrations; #3986's fixes touch core_address_pool. Verified independently.

Migration renumbering (post-#3986 merge)

#3986 landed as V002__unified.rs, colliding with #4019 (ADDR-09)'s already-merged V002__address_height_pin.rs — two migrations sharing one version number, breaking SqlitePersister::open() for every caller via a refinery_schema_history UNIQUE-constraint error. Fixed upstream on #3968 by renumbering the unified migration to V003 (also folding in a DashPay core_address_pool primary-key fix, T5). This PR's own checksum migration is renumbered V003→V004 to sequence after it. Final ordering: V001__initial → V002__address_height_pin → V003__unified → V004__manifest_checksum, verified to migrate a fresh store clean with zero collisions.

How Has This Been Tested?

• 11 test cases: checksum computed+stored on write; valid checksum loads normally; tampered blob → skip; wrong-wallet row (the Risk-6 core case) → skip; NULL checksum on a V004+ store → skip; combined-batch isolation (one tampered wallet doesn't sink a clean one); backfill exactness + idempotency; restore does not false-positive (proves the checksum path ignores the generation-rotation token); re-persist consistency; FFI mapping; fingerprint goldens re-pinned for the combined V001+V002(address_height_pin)+V003(unified, widened PK)+V004(manifest_checksum) schema.
• Full suite: platform-wallet-storage, platform-wallet (217+ lib tests), platform-wallet-ffi — 0 failures. cargo fmt --all clean; clippy --all-features --all-targets -- -D warnings clean. Post-merge: production build compiles; 7 migration/schema/pinning test targets re-verified passing (sqlite_v003_migration, sqlite_migrations, sqlite_schema_pinning, sqlite_manifest_integrity, sqlite_store_generation, sqlite_v003_isolation, sqlite_version_bump).

Breaking Changes

None. V001/V002/V003 untouched; V004 is additive with automatic backfill. No SkipReason/CorruptKind shape changes beyond the new additive variant.

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Attribution

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ef356bb1-cece-4235-ba67-daa9b65c7470

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

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/3968-manifest-integrity-checksum

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Review complete (commit 69d85dd)

[thepastaclaw]

Code Review

PR #3992 adds a well-scoped, additive SHA-256 integrity checksum over SHA-256(wallet_id ‖ account_xpub_bytes) to account_registrations, with an idempotent V003 backfill and per-wallet skip semantics on mismatch. Design and test coverage are solid (Risk-6 wrong-wallet-row, NULL-checksum skip, batch isolation, restore non-false-positive). Three in-scope suggestions worth addressing: (1) verify_manifest_checksums still calls blob::check_size before the checksum compare, so an oversized tampered blob returns BlobTooLarge and aborts the whole batch instead of falling into the per-wallet skip path the PR advertises; (2) backfill_missing_checksums does not size-gate account_xpub_bytes before materializing it in memory, unlike every other reader touching this column in this PR; (3) the Swift-side reasonDescription wire-code mirror was not extended for the new code 104.

Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] for general + security-auditor + rust-quality + ffi-engineer; verifier opus/opus. Failed lanes: codex/gpt-5.5[high] general, codex/gpt-5.5[high] security-auditor, codex/gpt-5.5[high] rust-quality, codex/gpt-5.5[high] ffi-engineer.

🟡 3 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:259-275: verify_manifest_checksums fails hard on oversize blob, breaking the per-wallet skip contract for exactly the tamper class it defends against
  `verify_manifest_checksums` calls `blob::check_size(row.get::<_, i64>(0)?)?` at line 265 *before* the SHA-256 comparison. If a tampered `account_xpub_bytes` also happens to exceed `BLOB_SIZE_LIMIT_BYTES`, this returns `WalletStorageError::BlobTooLarge` — which the caller in `persister.rs::load()` (line 952) routes through `Err(other) => return Err(PersistenceError::from(other))`, aborting the entire load batch. That contradicts the PR-headline invariant that a tampered / mis-bound row skips one wallet and never aborts a multi-wallet load, and it defeats it in exactly the corruption class this checksum exists to catch (in-place blob mutation, cross-wallet row copy). Because the writer's own size gate keeps this unreachable on legitimate rows, any hit here is by construction a manifest-integrity event. Either treat `BlobTooLarge` from this function as `ManifestIntegrityMismatch`, or drop the size gate here and let the checksum recompute be the sole verdict (a mismatched-length blob will fail the SHA-256 compare anyway). The pre-existing fail-hard in `load_state` predates this PR and is out of scope; this function is new and is where the skip contract lives.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: backfill_missing_checksums reads account_xpub_bytes without the blob-size gate used everywhere else in this PR
  `backfill_missing_checksums` runs on every `SqlitePersister::open()` and materializes `(rowid, wallet_id, account_xpub_bytes)` for every NULL-checksum row into a single `Vec` via `.collect()`. Unlike every other reader of this same column in this PR — `verify_manifest_checksums` (line 265: `blob::check_size(row.get::<_, i64>(0)?)?`), `load_state` (line 212), and `all_platform_payment_registrations` (lines 107-113) — this new function never calls `blob::check_size` or checks `length(account_xpub_bytes)` before reading. This matters because the backfill's own threat model is a legacy or corrupted store: a row with `checksum = NULL` and an oversized `account_xpub_bytes` blob (SQLite BLOBs can be ~2GB) would be read in full into memory on every `open()`, before any checksum verification runs. Exploitation requires an already-corrupted DB on disk (matching this PR's stated Risk-6 threat model), so it is defense-in-depth rather than remotely triggerable, but the inconsistency with the size gate that appears on every other reader of the same column in this same change is worth closing. Select `length(account_xpub_bytes)` alongside the payload and reject via `blob::check_size` before decoding, matching the pattern in the sibling functions.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:344-354: Swift-side wire-code mirror not updated for new LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH (104)
  This PR adds `LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH = 104` (rs-platform-wallet-ffi/src/manager.rs:193) and routes `CorruptKind::ManifestIntegrityMismatch` to it (manager.rs:249). The Swift binding's `SkippedWalletOnLoad.reasonDescription` (this file, lines 344-354) is the documented mirror of that wire contract — its own doc comment says the cases are matched by value against `rs-platform-wallet-ffi/src/manager.rs`. That switch was not extended with `case 104`, so any wallet skipped for the new manifest-integrity reason — the exact case this PR exists to surface — falls through to `default: return "unknown skip reason (104)"` instead of a meaningful description. The raw `reasonCode` is still delivered via `SkippedWalletOnLoad.reasonCode`, so no data is lost, but the SDK's own decoding helper silently degrades the PR's headline new signal. The struct-level doc comment at lines 329-334 also enumerates codes only up to 103 and should be updated in the same change.

[thepastaclaw]

🟡 Suggestion: verify_manifest_checksums fails hard on oversize blob, breaking the per-wallet skip contract for exactly the tamper class it defends against

verify_manifest_checksums calls blob::check_size(row.get::<_, i64>(0)?)? at line 265 before the SHA-256 comparison. If a tampered account_xpub_bytes also happens to exceed BLOB_SIZE_LIMIT_BYTES, this returns WalletStorageError::BlobTooLarge — which the caller in persister.rs::load() (line 952) routes through Err(other) => return Err(PersistenceError::from(other)), aborting the entire load batch. That contradicts the PR-headline invariant that a tampered / mis-bound row skips one wallet and never aborts a multi-wallet load, and it defeats it in exactly the corruption class this checksum exists to catch (in-place blob mutation, cross-wallet row copy). Because the writer's own size gate keeps this unreachable on legitimate rows, any hit here is by construction a manifest-integrity event. Either treat BlobTooLarge from this function as ManifestIntegrityMismatch, or drop the size gate here and let the checksum recompute be the sole verdict (a mismatched-length blob will fail the SHA-256 compare anyway). The pre-existing fail-hard in load_state predates this PR and is out of scope; this function is new and is where the skip contract lives.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: backfill_missing_checksums reads account_xpub_bytes without the blob-size gate used everywhere else in this PR

backfill_missing_checksums runs on every SqlitePersister::open() and materializes (rowid, wallet_id, account_xpub_bytes) for every NULL-checksum row into a single Vec via .collect(). Unlike every other reader of this same column in this PR — verify_manifest_checksums (line 265: blob::check_size(row.get::<_, i64>(0)?)?), load_state (line 212), and all_platform_payment_registrations (lines 107-113) — this new function never calls blob::check_size or checks length(account_xpub_bytes) before reading. This matters because the backfill's own threat model is a legacy or corrupted store: a row with checksum = NULL and an oversized account_xpub_bytes blob (SQLite BLOBs can be ~2GB) would be read in full into memory on every open(), before any checksum verification runs. Exploitation requires an already-corrupted DB on disk (matching this PR's stated Risk-6 threat model), so it is defense-in-depth rather than remotely triggerable, but the inconsistency with the size gate that appears on every other reader of the same column in this same change is worth closing. Select length(account_xpub_bytes) alongside the payload and reject via blob::check_size before decoding, matching the pattern in the sibling functions.

_{source: ['claude']}

[thepastaclaw]

Code Review

Incremental review at 2020cf3 covering the 579b620→2020cf3 delta, which only renumbered the migration V003→V004 to sequence after the #3986 V002/V003 collision fix and did not touch the checksum verify/backfill functions, error routing, or Swift decoder. All three prior findings remain valid at head. Carried-forward prior findings: (1) verify_manifest_checksums fail-hards on oversize blob and breaks the advertised per-wallet skip contract; (2) backfill_missing_checksums lacks the size gate every other reader of account_xpub_bytes applies in this PR; (3) Swift SkippedWalletOnLoad.reasonDescription is missing case 104. New latest-delta findings: none introduced by the delta itself, though verification surfaced one additional in-scope gap in the same skip-contract system — all_platform_payment_registrations still returns BlobTooLarge before its belt-and-suspenders checksum-skip logic, so an oversized tampered platform_payment row aborts the load before verify_manifest_checksums can record the wallet as skipped.

Source: reviewers claude/opus general, codex/gpt-5.5 general, claude/opus security-auditor, codex/gpt-5.5 security-auditor, claude/opus rust-quality, codex/gpt-5.5 rust-quality, claude/opus ffi-engineer, codex/gpt-5.5 ffi-engineer; verifier claude/opus.

🟡 4 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441: Carried-forward prior-3: Swift wire-code mirror still missing case 104 for LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH
  STILL_VALID at 2020cf329a. Rust defines LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH = 104 in rs-platform-wallet-ffi/src/manager.rs:195, routes CorruptKind::ManifestIntegrityMismatch to it at :260, and pins the value as ABI-stable in load_skip_reason_wire_values_are_stable. The Swift mirror at PlatformWalletManager.swift:430-441 was extended in this PR for `case 300: "already registered"` but the 104 case was never added, so a wallet skipped for the exact new manifest-integrity reason this PR ships falls through to `default: "unknown skip reason (104)"`. The struct-level doc comment at lines 415-420 also still enumerates only 100/101/102/103/199/200/300 and needs 104 in the same edit. The raw reasonCode is still delivered on SkippedWalletOnLoad.reasonCode so no data crosses the boundary corrupt, but the SDK's own decoding helper silently degrades the PR's headline new signal.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:255-275: Carried-forward prior-1: verify_manifest_checksums fail-hards on oversize blob, breaking the per-wallet skip contract
  STILL_VALID at 2020cf329a; unchanged by the V003→V004 rename delta. Line 265 calls `blob::check_size(row.get::<_, i64>(0)?)?` before the SHA-256 recompute. If a tampered account_xpub_bytes exceeds BLOB_SIZE_LIMIT_BYTES, this returns WalletStorageError::BlobTooLarge; persister.rs:952 (`Err(other) => return Err(PersistenceError::from(other))`) routes any non-ManifestIntegrityMismatch error as a hard batch abort, contradicting the explicit invariant documented three lines above at persister.rs:938 (`Manifest integrity is a per-wallet SKIP, not a batch abort`). Because apply_registrations enforces the size limit on write, any oversized blob reaching this function is by construction a manifest-integrity event — precisely the tamper class (in-place blob mutation, cross-wallet row copy) the checksum exists to catch. Either translate BlobTooLarge from this function into ManifestIntegrityMismatch, or drop the pre-compare size gate here and let the SHA-256 recompute be the sole verdict (a mismatched-length blob fails the equality compare anyway).
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: Carried-forward prior-2: backfill_missing_checksums reads account_xpub_bytes without the blob-size gate used everywhere else in this PR
  STILL_VALID at 2020cf329a; unchanged by the delta. backfill_missing_checksums runs on every SqlitePersister::open() and materializes (rowid, wallet_id, account_xpub_bytes) for every NULL-checksum row into a single Vec via .collect(). Unlike every other reader of the same column in this PR — verify_manifest_checksums (line 265), load_state (line 212), all_platform_payment_registrations (lines 107-113) — this new function never selects length(account_xpub_bytes) and never calls blob::check_size before materialization. A pre-V004 row with checksum=NULL and an oversized account_xpub_bytes (SQLite BLOBs can reach ~2GB) would be read in full into memory on every open() before any checksum verification runs, and all such rows are held simultaneously in the pending Vec. Exploitation requires an already-corrupted DB on disk (the PR's stated Risk-6 threat model), so this is defense-in-depth rather than remotely triggerable, but the inconsistency with the size gate every sibling reader applies is worth closing. Select length(account_xpub_bytes) alongside the payload and reject via blob::check_size before decoding.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:104-134: Bulk platform-payment oracle still aborts load on an oversized tampered row before per-wallet skip runs
  all_platform_payment_registrations is called from platform_addrs::load_all at the top of persister.rs::load(), before the per-wallet verify_manifest_checksums loop. The function already added a belt-and-suspenders checksum-skip at lines 121-129 (`match &stored_checksum { Some(c) if c.as_slice() == expected => {} _ => continue }`) — comment on lines 121-124 explicitly notes this bulk oracle 'never fail-hard decodes a tampered / mis-bound blob'. But that skip logic sits *after* the size gate at lines 107-113, which returns BlobTooLarge for an oversized blob before the checksum comparison ever runs. So a corrupted platform_payment row with an oversized blob still aborts the entire ClientStartState load before the authoritative per-wallet verifier records ManifestIntegrityMismatch in skipped, defeating the same skip-not-batch-abort contract the function's own comment claims to uphold. Either move the size gate below the checksum skip (so a checksum-mismatched row is `continue`d before the size check), or convert BlobTooLarge here into the same continue path — mirroring the fix suggested for verify_manifest_checksums.

[thepastaclaw]

🟡 Suggestion: Carried-forward prior-1: verify_manifest_checksums fail-hards on oversize blob, breaking the per-wallet skip contract

STILL_VALID at 2020cf3; unchanged by the V003→V004 rename delta. Line 265 calls blob::check_size(row.get::<_, i64>(0)?)? before the SHA-256 recompute. If a tampered account_xpub_bytes exceeds BLOB_SIZE_LIMIT_BYTES, this returns WalletStorageError::BlobTooLarge; persister.rs:952 (Err(other) => return Err(PersistenceError::from(other))) routes any non-ManifestIntegrityMismatch error as a hard batch abort, contradicting the explicit invariant documented three lines above at persister.rs:938 (Manifest integrity is a per-wallet SKIP, not a batch abort). Because apply_registrations enforces the size limit on write, any oversized blob reaching this function is by construction a manifest-integrity event — precisely the tamper class (in-place blob mutation, cross-wallet row copy) the checksum exists to catch. Either translate BlobTooLarge from this function into ManifestIntegrityMismatch, or drop the pre-compare size gate here and let the SHA-256 recompute be the sole verdict (a mismatched-length blob fails the equality compare anyway).

_{source: ['claude', 'codex']}

[thepastaclaw]

Correction: the automated reconciliation reply above was wrong for commit 5914e23f. This finding is still valid at that SHA and is carried forward in the exact-SHA review: verify_manifest_checksums still size-checks before converting checksum mismatch into a per-wallet skip. The thread should remain open.

[thepastaclaw]

Code Review

Cumulative incremental review of PR #3992 at commit 5914e23f. The latest delta from 2020cf32 rewrites migration/pool-reader tests plus a small persister TODO; the production code touched by the previous findings was not changed. All four prior findings from the 2020cf32 review are still valid and are carried forward. One new latest-delta blocker was verified in sqlite_migration_execution.rs, where the rewritten test suite still pins schema version 3 even though this PR now embeds V004.

Reviewed commit: 5914e23f (5914e23fcbce4f055295fcf9d7f5be374818c1ff); prior reviewed commit: 2020cf32.

Source: reviewers claude general opus, codex general gpt-5.5, claude security-auditor opus, codex security-auditor gpt-5.5, claude rust-quality opus, codex rust-quality gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.

2 blocking | 3 suggestion(s)

Prior Findings Reconciliation

• STILL VALID / carried forward: prior-1 verify_manifest_checksums fail-hards on oversized blobs before the checksum mismatch can become a per-wallet skip.
• STILL VALID / carried forward: prior-2 backfill_missing_checksums still materializes account_xpub_bytes without the blob-size gate used by sibling readers.
• STILL VALID / carried forward: prior-3 Swift SkippedWalletOnLoad.reasonDescription still lacks wire code 104 for manifest-integrity mismatch.
• STILL VALID / carried forward: prior-4 all_platform_payment_registrations still size-gates before its checksum-skip path, so an oversized tampered platform-payment row aborts load early.

Carried-Forward Prior Findings

• BLOCKING packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441: Carried-forward (STILL VALID): Swift SkippedWalletOnLoad decoder omits case 104 — the exact wire code this PR ships
• SUGGESTION packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:255-275: Carried-forward (STILL VALID): verify_manifest_checksums fail-hards on oversized blob, breaking the per-wallet skip contract
• SUGGESTION packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:93-135: Carried-forward (STILL VALID): bulk platform-payment oracle still aborts load on oversized tampered row before per-wallet skip runs
• SUGGESTION packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: Carried-forward (STILL VALID): backfill_missing_checksums reads account_xpub_bytes without the blob-size gate every sibling reader applies

New Findings In Latest Delta

• BLOCKING packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:90-370: New latest-delta: migration execution test suite pins schema version 3 but this PR embeds V004

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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441: Carried-forward (STILL VALID): Swift SkippedWalletOnLoad decoder omits case 104 — the exact wire code this PR ships
  Rust defines LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH = 104 in rs-platform-wallet-ffi/src/manager.rs:195, routes CorruptKind::ManifestIntegrityMismatch to it at :260, and pins the value as ABI-stable in the wire-values test at :613. The Swift mirror at PlatformWalletManager.swift:430-441 still switches over only 100/101/102/103/199/200/300, so a wallet skipped for this PR's headline new signal renders as `unknown skip reason (104)`. The struct-level doc comment at 415-420 also enumerates the same set without 104 and needs the same edit. Raw reasonCode still crosses the boundary intact, so this is a diagnostic-degradation not a data-corruption bug, but the SDK's own decoding helper silently loses the PR's shipping signal — verified at head 5914e23f (file untouched by the latest delta).

In `packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:90-370: New latest-delta: migration execution test suite pins schema version 3 but this PR embeds V004
  sqlite_migration_execution.rs was rewritten in the latest delta but still assumes the migration graph tops out at V003. Concretely: line 90 asserts `schema_version(conn) == 3` after `SqlitePersister::open`, lines 223 and 268 look for backup filenames `pre-migration-1-to-3-*`, line 308 asserts `max_supported == 3`, line 370 asserts `clean_snapshot[0] == 3` post-migration, and line 297/307 treat version 4 as a forged forward version. But this PR embeds `migrations/V004__manifest_checksum.rs`, and `sqlite_v003_migration.rs::max_supported_version_is_four` asserts `mig::max_supported_version() == 4`. `SqlitePersister::open` runs all pending migrations, so a V001 fixture ends at schema version 4, not 3, and the pre-migration backup name embeds the target version. Every one of these assertions will fail against the current binary, and the forward-version rejection test (tc_b_034) no longer tests a forward version — it inserts the actual current max as if it were the future. The migration/backup regression suite this file exists to be no longer describes the crate's own migration graph and no longer validates the V004 path it should be protecting.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:255-275: Carried-forward (STILL VALID): verify_manifest_checksums fail-hards on oversized blob, breaking the per-wallet skip contract
  Line 265 calls `blob::check_size(row.get::<_, i64>(0)?)?` before the SHA-256 recompute. If a tampered `account_xpub_bytes` exceeds `BLOB_SIZE_LIMIT_BYTES`, this returns `WalletStorageError::BlobTooLarge`; the caller in persister.rs::load only maps `ManifestIntegrityMismatch` to a per-wallet skip and treats any other error as a whole-load abort. That contradicts the invariant the docblock three lines above states (`manifest integrity is a per-wallet SKIP, not a batch abort`). Because `apply_registrations` enforces the same size limit at write time, an oversized blob reaching this verifier is by construction the manifest-integrity tamper class (in-place blob mutation or cross-wallet row copy) the checksum exists to catch. Either translate `BlobTooLarge` from this function into `ManifestIntegrityMismatch`, or drop the pre-compare size gate here and let the SHA-256 recompute (which is constant-memory over the stream and fails immediately for a mismatched length) be the sole verdict. Verified STILL VALID at head 5914e23f (accounts.rs untouched by the latest delta).

- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:93-135: Carried-forward (STILL VALID): bulk platform-payment oracle still aborts load on oversized tampered row before per-wallet skip runs
  `all_platform_payment_registrations` is called from `platform_addrs::load_all` before the per-wallet `verify_manifest_checksums` loop. The belt-and-suspenders checksum-skip added at lines 121-129 (`match &stored_checksum { Some(c) if c.as_slice() == expected => {} _ => continue }`) and the comment immediately above it (`this bulk oracle scan never fail-hard decodes a tampered / mis-bound blob`) claim per-wallet-skip semantics, but the size gate at lines 107-113 sits ahead of that skip and returns `BlobTooLarge` for an oversized blob before the checksum comparison runs. A corrupted `platform_payment` row with an oversized blob therefore aborts the entire `ClientStartState` load before the authoritative per-wallet verifier records `ManifestIntegrityMismatch` in skipped, defeating the same skip-not-batch-abort contract the function's own comment asserts. Either move the size gate below the checksum-skip so a checksum-mismatched row is `continue`d before the size check, or convert `BlobTooLarge` here into the same `continue` path. Verified STILL VALID at head 5914e23f (function untouched by the latest delta).

- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: Carried-forward (STILL VALID): backfill_missing_checksums reads account_xpub_bytes without the blob-size gate every sibling reader applies
  `backfill_missing_checksums` runs on every `SqlitePersister::open()` and materializes `(rowid, wallet_id, account_xpub_bytes)` for every NULL-checksum row into a single `Vec` via `.collect::<Result<Vec<_>, _>>()?`. Unlike every other reader of the same column in this PR — `verify_manifest_checksums` (line 265), `load_state` (line 212), and `all_platform_payment_registrations` (lines 107-113) — this function never selects `length(account_xpub_bytes)` and never calls `blob::check_size` before materialization. A pre-V004 row with `checksum = NULL` and an oversized `account_xpub_bytes` (SQLite BLOBs reach ~2GB) is read in full on every open before any checksum verification runs, and all such rows are held simultaneously in the pending Vec. Exploitation requires an already-corrupted DB on disk (this PR's stated Risk-6 threat model), so this is defense-in-depth rather than remotely triggerable, but the inconsistency with the size discipline every sibling reader applies is worth closing. Select `length(account_xpub_bytes)` alongside the payload and reject via `blob::check_size` before decoding. Verified STILL VALID at head 5914e23f (function untouched by the latest delta).

[thepastaclaw]

Code Review

Cumulative review at head 69d85ddb (69d85ddbd8502da2c01f85411b4fde7cfd3418bb). The latest delta from 5914e23f is Swift SDK integration-test infrastructure plus a DASH_KEYCHAIN_SERVICE test hook/env override merged from #3712; it does not touch the manifest-checksum Rust storage, FFI skip-code, Swift skip decoder, or migration execution test assumptions. All five prior findings from the 5914e23f review are STILL VALID and carried forward. No new latest-delta findings were verified.

Source: reviewers claude/opus general, codex/gpt-5.5 general, claude/opus security-auditor, codex/gpt-5.5 security-auditor, claude/opus rust-quality, codex/gpt-5.5 rust-quality, claude/opus ffi-engineer, codex/gpt-5.5 ffi-engineer; verifier claude/opus.

2 blocking | 3 suggestion(s)

Prior Findings Reconciliation

• STILL VALID / carried forward: prior-1 packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441 - Swift SkippedWalletOnLoad decoder omits case 104 — the exact wire code this PR ships
• STILL VALID / carried forward: prior-2 packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:88-370 - migration_execution suite pins schema V003 while this PR embeds V004
• STILL VALID / carried forward: prior-3 packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:255-275 - verify_manifest_checksums fail-hards on oversized blob, breaking the per-wallet skip contract
• STILL VALID / carried forward: prior-4 packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:93-135 - bulk platform-payment oracle aborts load on oversized tampered row before per-wallet skip runs
• STILL VALID / carried forward: prior-5 packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317 - backfill_missing_checksums reads account_xpub_bytes without the blob-size gate every sibling reader applies

Carried-Forward Prior Findings

• BLOCKING packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441: Carried-forward (STILL_VALID prior-1): Swift SkippedWalletOnLoad decoder omits case 104 — the exact wire code this PR ships
• BLOCKING packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:88-370: Carried-forward (STILL_VALID prior-2): migration_execution suite pins schema V003 while this PR embeds V004
• SUGGESTION packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:255-275: Carried-forward (STILL_VALID prior-3): verify_manifest_checksums fail-hards on oversized blob, breaking the per-wallet skip contract
• SUGGESTION packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:93-135: Carried-forward (STILL_VALID prior-4): bulk platform-payment oracle aborts load on oversized tampered row before per-wallet skip runs
• SUGGESTION packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: Carried-forward (STILL_VALID prior-5): backfill_missing_checksums reads account_xpub_bytes without the blob-size gate every sibling reader applies

New Findings In Latest Delta

• None. The 5914e23f..69d85ddb delta did not introduce a separate in-scope defect after verification.

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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441: Carried-forward (STILL_VALID prior-1): Swift SkippedWalletOnLoad decoder omits case 104 — the exact wire code this PR ships
  Rust defines `LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH = 104` in `rs-platform-wallet-ffi/src/manager.rs` and maps `CorruptKind::ManifestIntegrityMismatch` to it, but Swift's `SkippedWalletOnLoad.reasonDescription` (line 431-439) still switches only over 100/101/102/103/199/200/300, and the struct-level doc at 415-420 enumerates the same short list. A wallet skipped for the exact tamper class this PR is built to surface therefore renders as `unknown skip reason (104)`. The raw `reasonCode` still crosses the ABI intact, but the SDK's own decoder silently degrades the headline user-facing signal. Verified untouched by the 5914e23f→69d85ddb delta.

In `packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:88-370: Carried-forward (STILL_VALID prior-2): migration_execution suite pins schema V003 while this PR embeds V004
  `migrations/` now contains V001..V004 (V004__manifest_checksum.rs is embedded), so `refinery::embed_migrations!` yields `max_supported_version() == 4` and `SqlitePersister::open` migrates a V001 fixture to schema version 4. But `sqlite_migration_execution.rs` still hard-codes V003 throughout: line 90 asserts `schema_version(conn) == 3, "must be migrated to V003"`; lines 223 and 268 look for `pre-migration-1-to-3-` backups; line 308 asserts `max_supported == 3`; lines 370/392/439 assert clean/first-open migrates to V003; and `tc_b_034_forward_version_rejected_at_new_max` forges version=4 to test the forward-version gate — but 4 is now the legitimate max and the gate no longer trips. The PR description's passing-tests list omits `sqlite_migration_execution`, consistent with this target being broken. Verified untouched by the 5914e23f→69d85ddb delta.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:255-275: Carried-forward (STILL_VALID prior-3): verify_manifest_checksums fail-hards on oversized blob, breaking the per-wallet skip contract
  `verify_manifest_checksums` calls `blob::check_size(row.get::<_, i64>(0)?)?` at line 265 before the SHA-256 recompute. An oversized `account_xpub_bytes` row returns `WalletStorageError::BlobTooLarge`, which `SqlitePersister::load` routes through the generic error arm as a whole-load abort — only `ManifestIntegrityMismatch` gets translated to `ClientStartState.skipped` (wire code 104 across FFI). This contradicts the invariant the docblock four lines above states ("manifest integrity is a per-wallet SKIP, not a batch abort") and denies the Swift boundary the chance to observe reason 104 for the exact tamper class the checksum defends against. Because `apply_registrations` enforces the same size limit on write, any oversized blob reaching this verifier is by construction the manifest-integrity tamper class. Fix: translate `BlobTooLarge` from this function into `ManifestIntegrityMismatch`, or drop the pre-compare size gate here — the SHA-256 compare fails on any length divergence regardless.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:93-135: Carried-forward (STILL_VALID prior-4): bulk platform-payment oracle aborts load on oversized tampered row before per-wallet skip runs
  `all_platform_payment_registrations` is called from `platform_addrs::load_all` at the top of `SqlitePersister::load` before the per-wallet `verify_manifest_checksums` loop. The belt-and-suspenders checksum-skip at lines 121-129 (`_ => continue`) and its comment claim per-wallet-skip semantics — "this bulk oracle scan never fail-hard decodes a tampered / mis-bound blob" — but the size gate at lines 108-113 sits ahead of that skip and returns `BlobTooLarge` for an oversized blob before the checksum compare runs. A corrupted `platform_payment` row with oversized blob therefore aborts the entire `ClientStartState` load — and the Swift `on_load_wallet_list_fn` callback — before the authoritative per-wallet verifier records `ManifestIntegrityMismatch` (wire 104). Fix: move the size gate below the checksum skip, or convert `BlobTooLarge` here to the same `continue` path.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:285-317: Carried-forward (STILL_VALID prior-5): backfill_missing_checksums reads account_xpub_bytes without the blob-size gate every sibling reader applies
  `backfill_missing_checksums` runs on every `SqlitePersister::open()` on a V004+ store. Lines 287-299 select `(rowid, wallet_id, account_xpub_bytes)` for every NULL-checksum row and collect into `Vec<(i64, Vec<u8>, Vec<u8>)>` via `.collect::<Result<Vec<_>, _>>()?` — no `length(account_xpub_bytes)` column, no `blob::check_size` gate before materialization. Every other reader added in this PR — `verify_manifest_checksums` (line 260), `load_state` (line 200/212), `all_platform_payment_registrations` (lines 97/107-113) — selects `length(...)` first and enforces `BLOB_SIZE_LIMIT_BYTES` before decoding. A pre-V004 or corrupted DB with `checksum = NULL` and a maliciously grown blob (SQLite BLOBs reach ~2 GB) is materialized in full on every open before any verify runs, with all such rows held simultaneously in the pending Vec. Exploitation presupposes on-disk write access (this PR's Risk-6 threat model), so this is defense-in-depth rather than remotely triggerable, but the inconsistency with the size discipline every sibling reader applies is worth closing. Fix: select `length(account_xpub_bytes)` alongside the payload and reject via `blob::check_size` before decoding.

[thepastaclaw]

🟡 Suggestion: Carried-forward (STILL_VALID prior-5): backfill_missing_checksums reads account_xpub_bytes without the blob-size gate every sibling reader applies

backfill_missing_checksums runs on every SqlitePersister::open() on a V004+ store. Lines 287-299 select (rowid, wallet_id, account_xpub_bytes) for every NULL-checksum row and collect into Vec<(i64, Vec<u8>, Vec<u8>)> via .collect::<Result<Vec<_>, _>>()? — no length(account_xpub_bytes) column, no blob::check_size gate before materialization. Every other reader added in this PR — verify_manifest_checksums (line 260), load_state (line 200/212), all_platform_payment_registrations (lines 97/107-113) — selects length(...) first and enforces BLOB_SIZE_LIMIT_BYTES before decoding. A pre-V004 or corrupted DB with checksum = NULL and a maliciously grown blob (SQLite BLOBs reach ~2 GB) is materialized in full on every open before any verify runs, with all such rows held simultaneously in the pending Vec. Exploitation presupposes on-disk write access (this PR's Risk-6 threat model), so this is defense-in-depth rather than remotely triggerable, but the inconsistency with the size discipline every sibling reader applies is worth closing. Fix: select length(account_xpub_bytes) alongside the payload and reject via blob::check_size before decoding.

_{source: ['claude', 'codex']}