feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) - #3992
Draft
Claudius-Maginificent wants to merge 424 commits into
Draft
feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up)#3992Claudius-Maginificent wants to merge 424 commits into
Claudius-Maginificent wants to merge 424 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
lklimek
marked this pull request as ready for review
July 3, 2026 14:48
lklimek
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 3, 2026 14:48
lklimek
previously approved these changes
Jul 3, 2026
lklimek
changed the title
Collaborator
|
✅ Final review complete — no blockers (commit 06e768b) |
thepastaclaw
reviewed
Jul 3, 2026
Collaborator
There was a problem hiding this comment.
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.
Comment on lines
+259
to
+275
| let mut stmt = conn.prepare( | ||
| "SELECT length(account_xpub_bytes), account_xpub_bytes, checksum \ | ||
| FROM account_registrations WHERE wallet_id = ?1", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| blob::check_size(row.get::<_, i64>(0)?)?; | ||
| let payload: Vec<u8> = row.get(1)?; | ||
| let stored: Option<Vec<u8>> = row.get(2)?; | ||
| let expected = account_registration_checksum(wallet_id, &payload); | ||
| match stored { | ||
| Some(c) if c.as_slice() == expected => {} | ||
| _ => return Err(WalletStorageError::ManifestIntegrityMismatch), | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 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']
Comment on lines
+285
to
+317
| pub fn backfill_missing_checksums(conn: &mut Connection) -> Result<usize, WalletStorageError> { | ||
| let tx = conn.transaction()?; | ||
| let pending: Vec<(i64, Vec<u8>, Vec<u8>)> = { | ||
| let mut stmt = tx.prepare( | ||
| "SELECT rowid, wallet_id, account_xpub_bytes \ | ||
| FROM account_registrations WHERE checksum IS NULL", | ||
| )?; | ||
| let mapped = stmt.query_map([], |row| { | ||
| let rowid: i64 = row.get(0)?; | ||
| let wid_bytes: Vec<u8> = row.get(1)?; | ||
| let payload: Vec<u8> = row.get(2)?; | ||
| Ok((rowid, wid_bytes, payload)) | ||
| })?; | ||
| mapped.collect::<Result<Vec<_>, _>>()? | ||
| }; | ||
| let mut filled = 0usize; | ||
| { | ||
| let mut upd = | ||
| tx.prepare_cached("UPDATE account_registrations SET checksum = ?1 WHERE rowid = ?2")?; | ||
| for (rowid, wid_bytes, payload) in pending { | ||
| let wallet_id = <[u8; 32]>::try_from(wid_bytes.as_slice()).map_err(|_| { | ||
| WalletStorageError::InvalidWalletIdLength { | ||
| actual: wid_bytes.len(), | ||
| } | ||
| })?; | ||
| let checksum = account_registration_checksum(&wallet_id, &payload); | ||
| upd.execute(params![&checksum[..], rowid])?; | ||
| filled += 1; | ||
| } | ||
| } | ||
| tx.commit()?; | ||
| Ok(filled) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 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']
Base automatically changed from
feat/3968-snapshot-redirect
to
feat/platform-wallet-storage-rehydration
July 6, 2026 13:47
thepastaclaw
reviewed
Jul 6, 2026
Collaborator
There was a problem hiding this comment.
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.
Comment on lines
+255
to
+275
| pub fn verify_manifest_checksums( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<(), WalletStorageError> { | ||
| let mut stmt = conn.prepare( | ||
| "SELECT length(account_xpub_bytes), account_xpub_bytes, checksum \ | ||
| FROM account_registrations WHERE wallet_id = ?1", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| blob::check_size(row.get::<_, i64>(0)?)?; | ||
| let payload: Vec<u8> = row.get(1)?; | ||
| let stored: Option<Vec<u8>> = row.get(2)?; | ||
| let expected = account_registration_checksum(wallet_id, &payload); | ||
| match stored { | ||
| Some(c) if c.as_slice() == expected => {} | ||
| _ => return Err(WalletStorageError::ManifestIntegrityMismatch), | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 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']
Collaborator
There was a problem hiding this comment.
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
suggested changes
Jul 6, 2026
Collaborator
There was a problem hiding this comment.
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_checksumsfail-hards on oversized blobs before the checksum mismatch can become a per-wallet skip. - STILL VALID / carried forward: prior-2
backfill_missing_checksumsstill materializesaccount_xpub_byteswithout the blob-size gate used by sibling readers. - STILL VALID / carried forward: prior-3 Swift
SkippedWalletOnLoad.reasonDescriptionstill lacks wire code 104 for manifest-integrity mismatch. - STILL VALID / carried forward: prior-4
all_platform_payment_registrationsstill 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
suggested changes
Jul 6, 2026
Collaborator
There was a problem hiding this comment.
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..69d85ddbdelta 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.
Comment on lines
+285
to
+317
| pub fn backfill_missing_checksums(conn: &mut Connection) -> Result<usize, WalletStorageError> { | ||
| let tx = conn.transaction()?; | ||
| let pending: Vec<(i64, Vec<u8>, Vec<u8>)> = { | ||
| let mut stmt = tx.prepare( | ||
| "SELECT rowid, wallet_id, account_xpub_bytes \ | ||
| FROM account_registrations WHERE checksum IS NULL", | ||
| )?; | ||
| let mapped = stmt.query_map([], |row| { | ||
| let rowid: i64 = row.get(0)?; | ||
| let wid_bytes: Vec<u8> = row.get(1)?; | ||
| let payload: Vec<u8> = row.get(2)?; | ||
| Ok((rowid, wid_bytes, payload)) | ||
| })?; | ||
| mapped.collect::<Result<Vec<_>, _>>()? | ||
| }; | ||
| let mut filled = 0usize; | ||
| { | ||
| let mut upd = | ||
| tx.prepare_cached("UPDATE account_registrations SET checksum = ?1 WHERE rowid = ?2")?; | ||
| for (rowid, wid_bytes, payload) in pending { | ||
| let wallet_id = <[u8; 32]>::try_from(wid_bytes.as_slice()).map_err(|_| { | ||
| WalletStorageError::InvalidWalletIdLength { | ||
| actual: wid_bytes.len(), | ||
| } | ||
| })?; | ||
| let checksum = account_registration_checksum(&wallet_id, &payload); | ||
| upd.execute(params![&checksum[..], rowid])?; | ||
| filled += 1; | ||
| } | ||
| } | ||
| tx.commit()?; | ||
| Ok(filled) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 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']
thepastaclaw
suggested changes
Jul 8, 2026
Collaborator
There was a problem hiding this comment.
Code Review
All five carried-forward prior findings are STILL VALID at 06e768b and remain in the review. The latest delta also introduces several in-scope Core wallet/Swift FFI regressions: removed public APIs despite the provided PR context claiming no breaking changes, account-index misuse in the example Core send path, an acknowledged UTXO double-selection race in the split builder API, and unsafe C ABI enum parameters.
Reviewed commit: 06e768bfe42184fccfb47e73d4b2396eec5fe420; prior reviewed commit: 69d85ddbd8502da2c01f85411b4fde7cfd3418bb.
Source: reviewers codex/gpt-5.5 general, codex/gpt-5.5 security-auditor, codex/gpt-5.5 rust-quality, codex/gpt-5.5 ffi-engineer; verifier codex/gpt-5.5; failed lanes claude/opus general, claude/opus security-auditor, claude/opus rust-quality, claude/opus ffi-engineer (Claude quota: out of extra usage; resets Jul 10, 8am America/Chicago). Specialist selector: heuristic fallback selected security-auditor, rust-quality, ffi-engineer after selector LLM timeout.
6 blocking | 4 suggestion(s)
Prior Findings Reconciliation
- STILL VALID / carried forward: prior-1
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441- Swift skip decoder still omits manifest-integrity code 104 - STILL VALID / carried forward: prior-2
packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:90-90- migration execution tests still expect V003 after V004 was added - STILL VALID / carried forward: prior-3
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:265-265- oversized manifest blobs still abort the whole load - STILL VALID / carried forward: prior-4
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-113- bulk platform-payment scan can still abort before checksum skip - STILL VALID / carried forward: prior-5
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:287-298- checksum backfill still materializes unchecked blobs
Carried-Forward Prior Findings
- BLOCKING
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:415-441: Carried-forward prior-1: Swift skip decoder still omits manifest-integrity code 104 - BLOCKING
packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:90-90: Carried-forward prior-2: migration execution tests still expect V003 after V004 was added - SUGGESTION
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:265-265: Carried-forward prior-3: oversized manifest blobs still abort the whole load - SUGGESTION
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-113: Carried-forward prior-4: bulk platform-payment scan can still abort before checksum skip - SUGGESTION
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:287-298: Carried-forward prior-5: checksum backfill still materializes unchecked blobs
New Findings In Latest Delta
- BLOCKING
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:101-109: Latest delta: public Swift Core wallet API was source-broken - BLOCKING
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282: Latest delta: Core sends can use a Platform Payment account index as the BIP44 account - BLOCKING
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348: Latest delta: split transaction builder can double-select the same UTXO - BLOCKING
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:55-102: Latest delta: C ABI accepts caller-controlled Rust enum values - SUGGESTION
packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:23-29: Latest delta: exported C broadcast ABI was replaced without a compatibility shim
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 prior-1: Swift skip decoder still omits manifest-integrity code 104
Rust defines and emits LOAD_SKIP_REASON_MANIFEST_INTEGRITY_MISMATCH = 104 for ManifestIntegrityMismatch, but Swift's SkippedWalletOnLoad documentation and reasonDescription switch still cover only 100, 101, 102, 103, 199, 200, and 300. A wallet skipped for this PR's new checksum failure therefore reaches Swift as unknown skip reason (104), losing the exact host-facing integrity signal the PR adds.
In `packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs:90-90: Carried-forward prior-2: migration execution tests still expect V003 after V004 was added
The current migration set embeds V004__manifest_checksum.rs, so SqlitePersister::open should migrate the fixture through schema version 4. This helper still asserts schema_version(conn) == 3, and the same test file still searches for pre-migration-1-to-3 backups, treats version 4 as a future schema, and asserts clean/idempotent opens reach V003. The suite is stale and no longer verifies the V004 migration path this PR adds.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:265-265: Carried-forward prior-3: oversized manifest blobs still abort the whole load
verify_manifest_checksums applies blob::check_size before comparing the checksum. An oversized corrupted account_xpub_bytes row returns BlobTooLarge, but SqlitePersister::load only maps ManifestIntegrityMismatch into ClientStartState.skipped; other verifier errors abort the whole load. That breaks the PR's stated per-wallet skip contract for one class of manifest tamper.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-113: Carried-forward prior-4: bulk platform-payment scan can still abort before checksum skip
all_platform_payment_registrations runs before the per-wallet checksum verifier and drops checksum mismatches later in the loop, but the blob-size gate returns BlobTooLarge first. A single oversized corrupted platform_payment row can therefore abort ClientStartState loading before the authoritative verifier records ManifestIntegrityMismatch for that wallet.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:287-298: Carried-forward prior-5: checksum backfill still materializes unchecked blobs
backfill_missing_checksums selects account_xpub_bytes directly and collects every NULL-checksum row into one Vec before any size validation. Other readers of this column select length(account_xpub_bytes) first and enforce blob::check_size before materializing the payload, so a corrupted pre-V004 store can force large allocations during open before load-time integrity classification can skip the wallet.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:101-109: Latest delta: public Swift Core wallet API was source-broken
The latest delta removes the public sendToAddresses(...) API and changes broadcastTransaction from accepting raw Data to accepting CoreTransaction. Existing Swift SDK callers that build a one-shot send or broadcast already-signed transaction bytes no longer compile, while the provided PR context still declares no breaking changes. Keep compatibility overloads around the new builder flow or explicitly mark and document this as a breaking API change.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282: Latest delta: Core sends can use a Platform Payment account index as the BIP44 account
For non-platformToPlatform flows this branch still derives senderAccountIndex from persisted Platform Payment address rows, and the comment says those flows ignore it. That is no longer true: coreToCore now passes senderAccountIndex into CoreTransactionBuilder.setFunding and buildSigned as the BIP44 Core account index. If the first funded Platform Payment account is index 1, a Core send tries to spend from BIP44 account #1 instead of the previous default BIP44 account #0, causing failures or spending from the wrong Core account.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348: Latest delta: split transaction builder can double-select the same UTXO
core_wallet_tx_builder_set_funding snapshots spendable UTXOs and stores them in the builder, but the actual reservation is only taken later during build_signed. The function comment acknowledges that two same-account builders can both pass set_funding before either reserves and then sign transactions spending the same UTXO. This is a regression from the removed single-call send path, and it is not safe to leave to external caller discipline on a public Swift/FFI API; serialize set_funding through build_signed per wallet/account or expose an atomic fund-and-sign entry point that preserves the wallet-side reservation invariant.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:55-102: Latest delta: C ABI accepts caller-controlled Rust enum values
CoreAccountTypeFFI and CoreSelectionStrategyFFI are #[repr(C)] Rust enums and are used directly as extern "C" parameters by the new builder, gap-limit, and broadcast entry points. A C caller can pass any integer; if it is not one of the declared discriminants, Rust has already received an invalid enum value, which is undefined behavior before the match can report an error. Keep the raw ABI as u32/c_uint and convert with TryFrom at the boundary, returning ErrorInvalidParameter for unknown values.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:23-29: Latest delta: exported C broadcast ABI was replaced without a compatibility shim
The previous core_wallet_broadcast_transaction symbol accepted raw transaction bytes plus length, and the crate exported core_wallet_send_to_addresses and core_wallet_free_tx_bytes. The current head keeps the broadcast symbol name but changes its call frame to *const FFICoreTransaction plus account metadata, and removes the old send/free symbols. In-tree Swift was updated, but any app or generated binding built against the previous header will fail to link or can call an incompatible symbol if headers and library are mixed. If this FFI surface is intended to be stable, keep deprecated wrappers or bump/version the ABI explicitly.
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| blob::check_size(row.get::<_, i64>(0)?)?; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior-3: oversized manifest blobs still abort the whole load
verify_manifest_checksums applies blob::check_size before comparing the checksum. An oversized corrupted account_xpub_bytes row returns BlobTooLarge, but SqlitePersister::load only maps ManifestIntegrityMismatch into ClientStartState.skipped; other verifier errors abort the whole load. That breaks the PR's stated per-wallet skip contract for one class of manifest tamper.
source: ['claude', 'codex']
Comment on lines
112
to
113
| }); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior-4: bulk platform-payment scan can still abort before checksum skip
all_platform_payment_registrations runs before the per-wallet checksum verifier and drops checksum mismatches later in the loop, but the blob-size gate returns BlobTooLarge first. A single oversized corrupted platform_payment row can therefore abort ClientStartState loading before the authoritative verifier records ManifestIntegrityMismatch for that wallet.
source: ['claude', 'codex']
Comment on lines
+287
to
+298
| let pending: Vec<(i64, Vec<u8>, Vec<u8>)> = { | ||
| let mut stmt = tx.prepare( | ||
| "SELECT rowid, wallet_id, account_xpub_bytes \ | ||
| FROM account_registrations WHERE checksum IS NULL", | ||
| )?; | ||
| let mapped = stmt.query_map([], |row| { | ||
| let rowid: i64 = row.get(0)?; | ||
| let wid_bytes: Vec<u8> = row.get(1)?; | ||
| let payload: Vec<u8> = row.get(2)?; | ||
| Ok((rowid, wid_bytes, payload)) | ||
| })?; | ||
| mapped.collect::<Result<Vec<_>, _>>()? |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Carried-forward prior-5: checksum backfill still materializes unchecked blobs
backfill_missing_checksums selects account_xpub_bytes directly and collects every NULL-checksum row into one Vec before any size validation. Other readers of this column select length(account_xpub_bytes) first and enforce blob::check_size before materializing the payload, so a corrupted pre-V004 store can force large allocations during open before load-time integrity classification can skip the wallet.
source: ['claude', 'codex']
…ey persistence Phase 1c spec for persisting provider_key_account_registrations (BLS operator-key / EdDSA platform-node-key accounts) instead of dropping them in versions.rs. Covers round-trip, discriminated BLS/EdDSA encoding, one-to-many platform-node keys, empty case, migration/schema-freeze correctness, fail-hard trust boundary, no-private-key-material invariant, and cross-backend parity with the FFI persister.
The persisted shape of a `ProviderKeyAccountEntry` minus its `derived_platform_node_keys`: an unbounded one-to-many belongs in its own rows, not inline in an account's payload. `account_type` stays on the blob so a backend can cross-check its typed column against the decoded payload. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`store()` dropped `provider_key_account_registrations` on the floor, so a reloaded seedless wallet came back without its BLS operator-key and EdDSA platform-node-key accounts. The EdDSA platform-node pool is hardened-only (SLIP-10, no public derivation), so those pre-derived keys were unrecoverable without the recovery phrase — exactly the loss the batch was captured to prevent. Both provider accounts now ride the existing `account_registrations` row (its `account_type` CHECK already admits both labels), encoded as a `ProviderKeyRegistrationBlob`; `account_type` is the decode discriminator, matching the convention the FFI backend already ships. V004 adds the one table that has nowhere else to live: `provider_platform_node_keys`, the one-to-many node-key batch, FK'd to its parent account row so it cannot outlive it. V001-V003 are byte-identical. Reader hardening: the ECDSA `SELECT` now excludes the provider rows (their blob is over a different curve and would hard-error the decode), and a provider row is rejected when its typed columns contradict the blob or the blob carries the wrong curve for its account type (`ProviderKeyAccountEntryMismatch`). Tests: `sqlite_provider_key_accounts.rs` (12 cases: round-trip, per-curve decode, cross-curve rejection, node-key order/completeness, empty case, idempotent re-persist, batch replacement, corrupt/oversize blob, cascade) and `sqlite_v004_migration.rs` (additivity, pre-V004 rows survive). Migration-version assertions now derive from the embedded set instead of a hardcoded 3, so they don't rot on the next migration. Closes: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n table Migration log stopped at V001; V002 (ADDR-09 height pin), V003 (#3968 unified migration), and V004 (#4113 provider key accounts) were never added. Also documents the new provider_platform_node_keys child table and corrects account_registrations' PK, which SCHEMA.md listed as 3 columns while V001 always declared 6 (key_class + DashPay pair). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independently ran cargo test/clippy on platform-wallet-storage, constructed and verified an adversarial duplicate-account-type changeset (silent node-key-batch data loss, QA-001), and audited testspec-4113.md coverage gaps (shrink-direction re-persist, node_id/key_index edge cases untested).
…keys `apply_provider_registrations` cleared a provider account's node-key rows before re-inserting the incoming batch. The pool is hardened-only (Ed25519/SLIP-10), so any key the store forgets is one no watch-only wallet can ever re-derive — and two live callers hand it a batch that is shorter than what is already persisted: - registration falls back to an empty batch when pre-derivation fails (`wallet_lifecycle::register_wallet` treats that as non-fatal); - `Merge` is append-only `.extend()`, so one flush can carry two entries for the same account, and the second one wins. Node keys are now upserted per `key_index` and never deleted: a shorter or empty batch says nothing about the missing indices rather than retracting them. Guarding the DELETE on a non-empty batch would have fixed only the first path — the merged-entry case passes a non-empty batch. Three regression tests, each confirmed failing against the previous writer: shrinking batch, empty batch, and two merged entries for one account. Also in this commit: - Extract `rebuild_provider_key_account` into `platform-wallet`; the SQLite and FFI restore paths were two verbatim copies of the same watch-only rebuild (same constructors, same inserters, differing only in error type). Both now call it and map the one error into their own. - Narrow `ProviderKeyRegistrationBlob`'s rustdoc: it is the SQLite payload shape, not a cross-backend wire contract. The FFI bincodes the bare key; what the backends share is the `account_type` discriminator, nothing more. - Trim the V004 header and two over-long doc comments to the internal cap. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two writers share `account_registrations`: one PK space, one blob column, discriminated only by `account_type`. The reader validated the curve/type pairing; the writer trusted its caller. A `ProviderOperatorKeys` entry carrying an EdDSA key — reachable through the public `store()` and a `pub` changeset field — would upsert onto the operator account's row with a payload the fail-hard reader then rejects, making the whole wallet unopenable on the next `load()`. The writer now enforces the same invariant, before any SQL runs, so a mis-paired entry is refused instead of stored as a landmine. Duplicate entries for one account in a single flush are now decided, not left to write order. `Merge` is append-only, so a re-emitted registration can ride the same flush as the original; identical entries reconcile (node keys union by index — nothing is lost). Two entries that disagree about the account's own extended public key are a contradiction no merge semantic can resolve: one is wrong and the store cannot tell which, so both are refused (`ProviderKeyAccountConflict`) rather than letting the last write win. Union, not rejection, is the answer for the node-key batches themselves: they are hardened-only (Ed25519/SLIP-10) and unrecoverable, every key in either batch is a legitimate key of the same account, and erroring would fail the whole flush — including the unrelated sub-changesets riding it — over an anomaly with a lossless reading. Tests (both guards confirmed failing against c1349e6 first): mis-paired curve rejected at write time with no row left behind; conflicting duplicates rejected with neither written. Plus two coverage tests, no bug behind either: provider rows + node keys + domain-seq bump all roll back together on a mid-flush failure (mirroring tc_b_012), and a registered account with an empty pre-derived batch round-trips as an account with zero keys, not as no account. Also documents the cross-backend discriminator parity at the match itself rather than only in a commit message. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…flict fix Final-pass QA against 1df6169: re-attempts the original duplicate-entry repro (now unions, confirmed) plus three new adversarial shapes against ProviderKeyAccountConflict: a three-way duplicate, a reordered-but-equal node-key duplicate (confirmed not a conflict), and a same-index/ conflicting-value node-key collision within one store() call. The last one surfaces a real asymmetry: the account-level conflict check compares only the encoded (account_type, extended_public_key) payload, so two entries sharing that payload but disagreeing on a node key's bytes at the same index reach the child-table's ON CONFLICT ... DO UPDATE with no arbitration -- silent last-write-wins, unlike the fail-closed reasoning applied one level up. Reported as a finding, not fixed here (test documents current behavior and flags it for a design decision). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… index The account-level conflict check compared only the encoded (account_type, xpub) payload, so two entries agreeing on the account but carrying different bytes at the SAME key_index reached `ON CONFLICT ... DO UPDATE` and silently overwrote one with the other — no error, no diagnostic, on material nothing can re-derive. Found independently by Smythe (SEC-007) and Marvin (QA-005). The asymmetry was the real defect: fail closed on a contradictory xpub one level up, silently pick a winner one level down. A node key is fully determined by its account xpub and index — derivation is a pure function and `node_id` is hash160(public_key) — so two different values at one index mean one is wrong and the store cannot tell which. Same contradiction, same answer: refuse the flush (`ProviderNodeKeyConflict`), writing neither key. Checked in both directions: between entries within a flush, and against the row already stored — so a stale or corrupted re-registration cannot overwrite a good key that a later flush disagrees with. The SQL drops to `DO NOTHING`, which the checks make a no-op for identical bytes and which, if they were ever bypassed, still cannot destroy a derived key. Marvin's `marvin_adversarial_same_index_conflicting_node_key_value_is_silently_ overwritten` documented the behavior rather than asserting it; it is now `sec_007_same_index_conflicting_node_keys_are_rejected`, asserting rejection and that neither key lands. A second test covers the cross-flush case his in-batch probe could not reach. Both confirmed failing against 33d18ac. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Root-cause confirmation, blob-codec audit, chosen fix (AssetLockEntryWire mirroring IdentityKeyWire), migration/compat strategy, secondary AlreadyOpen-masking fix, and test plan. Design only — no implementation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gging Addresses the two error-handling-coverage audits of the secrets/ subtree (PR #3968). All 11 findings are LOW — diagnostic-precision, logging-level, and lint-hygiene polish; no behavioural/security defects were found. Security-lens (Smythe): - SEC-001: SecretString::default() mlock failure now logs at debug! with distinct wording (empty buffer, no secret at risk) so it no longer shares byte-identical text with the new() warn!; both sites stay greppable. - SEC-002: demote the documented-non-fatal parent-dir fsync-uncertain log from error! to warn! (degraded-but-recoverable), matching the policy that reserves error! for the propagated/fatal write failure. - SEC-003: add SecretStoreError::EntropyUnavailable; random_bytes (which backs nonce + salt draws, not just KDF) now reports it instead of the misleading KdfFailure. - SEC-004: thread the store's durability-uncertain counter through the initial-create write path so durability_uncertain_count()'s "0 == all writes confirmed durable" contract holds for create too. - SEC-005: switch the five vault_lock unsafe overrides from #[allow(unsafe_code)] to #[expect(unsafe_code, reason=...)] so a stale override self-reports (M-LINT-OVERRIDE-EXPECT). - nits: drop the redundant `let _ =` on the ?-propagating validated_label guards; drop-time sync now calls the non-logging do_write_vault_at so a drop-path failure logs once (with context) instead of twice. Coverage-lens (Marvin): - QA-001: all three keyring platform arms now debug!-log the discarded backend-init error before falling back to NoDefaultStore. - QA-002: create_parent_dir (both branches) and VaultLock::acquire's non-WouldBlock branch route through SecretStoreError::io_at so the known path rides in the error, per the crate's io_at policy. - QA-003: map_spi only collapses a rejected `user` (label) attribute to InvalidLabel; a rejected service (or any other attribute) maps to OsKeyring{Backend} instead of mislabelling the caller's label. - QA-004: add is_recoverable() + error_kind_str() to SecretStoreError, mirroring WalletStorageError's SQLite-side classification so both typed errors in the crate read as one family. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rain failures QA-004: SqlitePersister::open() — the crate's highest-stakes failure boundary (IntegrityCheckFailed, SchemaVersionUnsupported, Migration, AlreadyOpen) — emitted zero tracing on any failure path. Wrap the body in open_inner() and log every returned Err classified via error_kind_str(): tracing::error! for real failures, warn! for the benign in-process AlreadyOpen race. One exit point catches all paths, not just the four named ones. QA-003: delete_wallet_inner's post-commit drain used `if let Ok(Some(_late)) = take_for_flush(..)`, silently swallowing a possible Err(LockPoisoned) — the one spot in the file breaking its own convention. Match the Err arm and log it at tracing::error!, matching the three other LockPoisoned sites (Drop, handle_flush_error, restore_buffer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…count key mappers
`account_key_class` and `account_dashpay_ids` fell through `_ =>` into a
sentinel, and both feed PRIMARY KEY columns. An upstream `AccountType` variant
this crate has not been taught about would therefore be handed another
variant's sentinel and collapse onto an existing key — no error, no warning,
one account overwriting another at the next write.
Measured rather than argued. Deleting the `PlatformPayment` arm while the
wildcard stood left the crate compiling and made
`distinct_key_class_accounts_do_not_collide` fail with `left: 1, right: 2` —
two accounts persisted, one row survived. With both matches exhaustive the
same deletion is `error[E0004]: non-exhaustive patterns:
`&AccountType::PlatformPayment { .. }` not covered`. The change converts a
silent account loss into a compile error, which is the one signal in this
codebase that cannot fail to appear.
Listing the sentinel-valued variants explicitly costs a dozen lines and
matches what `account_index`, `account_type_db_label` and the test helper's
own exhaustive match already do in this file.
`no_two_account_types_share_a_pk_tuple` adds the runtime half: the mappers
make an untaught variant a compile error, and this makes a variant that is
taught but mapped onto an existing key a test failure. It reaches every
variant through `all_account_type_variants`, whose exhaustive match means a
new one cannot arrive without a decision being taken.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hat actually apply SCHEMA.md's `dashpay_profiles` and `dashpay_payments_overlay` entries carried a sentence copied verbatim from `token_balances`, crediting `IdentitySyncManager` with rebuilding a "canonical token-balance copy". Neither table holds a token balance and neither is rebuilt by that manager: `DashPaySyncManager` drives `sync_profiles` and the payment reconciles. A reader chasing a profile that failed to rehydrate was pointed at the wrong module. SECRETS.md described the Tier-2 per-read Argon2 gate as clamping to `default_target()`. The gate reads `ARGON2_READ_MAX_M_KIB`/`ARGON2_READ_MAX_T`, and the decoupling is the point: a read gate keyed to a write-side tunable orphans already-enrolled secrets the day the tunable moves. The document was only accidentally true because the shipped default currently coincides with the ceiling, and would have become false in exactly the scenario the code was changed to survive. The same section understated the parent-directory check. It is not one directory and not one condition: every ancestor up to `/` is walked twice, over the lexical path and over its canonical target, and an ancestor is refused for an untrusted owner as well as for group/other write access. Sticky writable directories are accepted, which the previous wording denied. `sha2` moves out of the `sqlite` feature and into `__test-helpers`. Its only users are the two migration-fingerprint helpers, both gated `cfg(any(test, feature = "__test-helpers"))`, so every shipping iOS, Android and desktop build was compiling a hash crate no production path calls. `cargo machete` cannot see this: the symbol is referenced, just never outside a test build.
…ping the table The primary-key collision guard printed all sixteen mapped tuples on failure, leaving the reader to spot the duplicate among fifteen near-identical rows of zeros. It now reports only the keys claimed twice, and says what the collision costs: the second account written overwrites the first. Verified by forcing a collision — two variants onto one label — and reading the message it produced, rather than assuming the filter picked the right rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ad of waiting for sync `apply_persisted_core_state` decoded every `core_instant_locks` row into `CoreChangeSet::instant_locks_for_non_final_records` and then dropped it on the floor, so instant-locked funds came back merely confirmed after a restart and stayed that way until the next sync re-learned the lock. The comment guarding that gap claimed replay needed an upstream `key_wallet` change because `ManagedWalletInfo.instant_send_locks` is `pub(crate)` with no public setter. Only half of that was true. `WalletInfoInterface` — already in scope in this very function for `update_balance` — exposes `mark_instant_send_utxos(&Txid, &InstantLock)`, whose body inserts into that field and marks the matching UTXOs across every account. The tx-record half of the comment does hold and is kept: injecting a record needs the raw `dashcore::Transaction`, and this crate persists only the abstracted blob. The replay runs AFTER the UTXO restore, not before. `mark_instant_send_utxos` marks the UTXOs it can find, so the reverse order records the txid and marks nothing — a bug that would leave the lock set correct and every UTXO flag wrong. The test asserts both halves for exactly that reason. The rustdoc listed `is_instantlocked` among the flags deferred to the first post-load sync. It is now reconstructed, so it moves out of that list rather than being left as a claim the code contradicts.
… callers can reach Three unrelated shape defects, all in the same direction: the API the crate publishes is not the API it means. `apply_persisted_core_state` is documented as the crate's public rehydration entry point, and `sqlite/mod.rs` already explains that `LoadCtx` is unconditionally public because that function takes one. Two of its other parameters name `core_pool::OwningAccount`, which lives under a `schema` module that is `pub(crate)` in every build not enabling `__test-helpers` — a feature whose own manifest comment says downstream MUST NOT enable it. A default-features consumer could therefore neither name the type, construct the maps, nor call the function, and `--all-features` CI compiled a different, callable API than the one shipped. `OwningAccount` is re-exported alongside `LoadCtx` for the reason the file had already written down. Narrowing the function instead was rejected: it has no caller anywhere in `packages/` outside its own tests, so `pub(crate)` would have turned a reachability bug into dead code. `util/mod.rs` calls its contents "shared internal helpers" and then published `permissions` — and `backup` published its filename builders and the raw `run_to`, which bypasses the persister's registry, auto-backup and recovery-mode gates that `restore_from` was made `pub(crate)` to protect. Nothing outside the crate consumes any of them. `permissions` follows the `schema` / `migrations` pattern (crate-private, widened under `__test-helpers` for this crate's own mode-asserting tests); the four `backup` helpers have no test caller at all and are simply `pub(crate)`. `persistence_kind` was the one classification arm with a `_ =>` fallthrough, while `is_transient` and `error_kind_str` enumerate every variant. That fallthrough sent each future variant to `Fatal` with no compile-time prompt — the wrong answer for any caller-data fault, as the three hand-classified `Constraint` arms already sitting above it demonstrate. The wildcard is replaced by the explicit list. It is long; the length is the guarantee.
…n the secrets tree Deleting a wallet left it readable. SQLite moves freed pages to the freelist without clearing them, `secure_delete` was never set, and `Backup` copies pages including the freelist — so a deleted wallet's addresses, scripts, identity keys and contact data survived in the `.db` and rode into every snapshot taken afterwards. `apply_pragmas` now sets `FAST` with a read-back like `journal_mode`'s, and `delete_wallet` raises it to `ON` for the cascade and restores it after. The split is not cosmetic: measured on this schema, a delete under `FAST` still leaves 1876 legible copies of the seeded marker, because FAST zeroes freed content only within pages it is already rewriting and the cascade releases whole pages. `ON` leaves none. The new test seeds page-spanning rows precisely so it exercises the case FAST cannot cover, and carries a positive control — a surviving wallet whose marker the same scan must still find — so "absent" cannot be read off a broken scan. The open-path registry was claimed as the LAST step of `open_inner`, after migrations. That left two concurrent opens both computing their pending list from the same pre-migration history and both applying it, against the one file that is the wallet. The claim moves ahead of `precreate_secure` behind an RAII guard, which keeps the "no stale claim on failure" property the late ordering was chosen for. The key becomes the canonical PARENT joined with the file name, not `canonicalize(path)`: the claim now precedes the file's creation, and `canonicalize` cannot resolve a path that does not exist, so keying on the whole path would give two spellings of one new database two different keys. `restore_from` probes through the same function so both agree. Three security decisions rested on a vault owner being able to harden their Argon2 header above the shipped default. No public API produces such a header — `open`/`open_unprotected` pass `default_target()`, `open_mock` passes `floor_target()`, and `build_fresh_vault` is private with no setter on the handle. The `max_strength` ratchet defended a state nothing can construct, and is removed. Worse than the dead code was the invitation: the docs told owners a vault is "a local artefact they may harden at will", the format is legible JSON with a visible `kdf` block, and `kdf` is bound into both the derived key and the verify-token AAD — so an owner who took that advice got a permanently unopenable vault and a `WrongPassphrase` telling them they mistyped. The rustdoc now states what the wide read band actually buys (tolerance for a build whose default differed) and `format.rs` warns that the header is authenticated. `InsecureParentDir` walked every ancestor to `/` and then reported the vault's own parent, with a mode, and a `chmod go-w` aimed at "the offending ancestor" it never named. When the real cause was OWNERSHIP the mode shown was a perfectly ordinary `0755` and `chmod` could not have helped. The error carries the actual ancestor and a two-arm reason, so each message names one path and one command that works. The walk is Unix-only, so the POSIX remediation is always correct where it can appear. `MIN_PASSPHRASE_LEN` records its accepted one-way risk in place.
…repo will not contain Around thirty committed doc comments cited `TC-B-NNN`, `TC-PKA-NNN` and `TC-1..TC-10`, and one line named `docs/testspec-4113.md` outright. That file is untracked, and the repository guidelines forbid committing it: a spec written to drive a change is a working artifact, deleted when the work ships. So no reader after merge can resolve any of these — they are dangling references by construction, not by neglect. The IDs are stripped and the sentences keep the meaning, which they were already carrying: `tc_b_040_sql_fingerprint_pinned` says what it does without the prefix, and every module header now describes its own coverage instead of enumerating identifiers. Test names, not IDs, are the index. The `TC-0NN` series is deliberately left alone. It belongs to an older spec, predates this branch and is absent from its diff; rewriting ~40 occurrences across ten untouched files would bury this work in unrelated churn. Its referent is untracked too — no committed markdown in the repository contains any `TC-` identifier — so it is the same defect, and it wants its own change. `secrets_default_on_compiles.rs` claimed to prove that importing `EncryptedFileStore` from the crate root "without enabling any feature flag" is the assertion. It compiles under a dev-dependency that sets `default-features = false` and then lists `secrets` explicitly, and the file itself opens with `#![cfg(feature = "secrets")]` — so it proved that enabling `secrets` gives you `secrets`, and said so on a page that opened by contradicting itself. The doc now states what the file actually asserts (the public surface is reachable from the crate root, not only by a deep path) and names what would be needed to prove the default-on claim it cannot.
Wave 3's tail findings (docs, InstantSend restore at load, public-surface trim, secrets at-rest and open-path gaps, test-case ID cleanup) join wave 1, wave 2 and the account key mapper guard. The wave-3 branch had already merged 2cf9b46 forward, so this merge adds no textual change over its tip; the tree is byte-identical to the automatic merge of both parents. It is recorded as a merge commit rather than a fast-forward so fix/3968-round5 keeps a first-parent line of its own. Neither side's earlier verification described this tree: wave 2's run predates wave 3's commits and wave 3's predates the load-path rewrite and the account key mapper guard. The merged tree is verified on its own.
…mode, not merely that one is set Three defects in the read-back added with the erasing cascade, each one a way for the guarantee to be reported as held while not being held. The open-time check accepted any nonzero `secure_delete`. SQLite reports the mode numerically and the values are distinct — `0` off, `1` ON, `2` FAST — so "nonzero" would have accepted `ON` where `FAST` was requested and, worse, would have accepted a cascade's raise that was never restored, leaving every later write paying the full erase cost with nothing to say so. Both constants now carry the value they must read back as, and one `set_secure_delete` helper does set-then-confirm for the raise and the restore alike. The cascade runs inside an EXCLUSIVE transaction, and a pragma that failed to carry into that context would fail silently — a delete reporting success over pages it never scrubbed. `delete_wallet_inner` reads the mode back from inside its own transaction, at the point of use rather than at the point it was set. A test pins the SQLite behaviour that read-back relies on, so a change in it surfaces as a named failure instead of as unexplained residue. The residue test could also have degraded silently. Sized small enough, its rows sit inside a page that stays in use — the one case where FAST and ON are indistinguishable — and it would then have passed while proving nothing about whole pages released to the freelist, which is what deleting a wallet actually does. It now asserts the cascade freed at least one page, so a fixture that stops exercising the hard case fails instead of quietly agreeing with the implementation. Two scanner defects in the same test are closed with it. It asserts the marker is findable BEFORE the delete, because a scan that silently matches nothing reports a clean file exactly as a clean file does; the experiment that produced this fix first returned zero hits in all three modes because the scanner refused to read a binary file. And it scans the `-wal` alongside the `.db`: pages live there until a checkpoint, so the pre-delete scan would otherwise miss data plainly still present, and a post-delete scan would miss residue parked in a WAL that outlived the handle.
41a2b2b landed on fix/3968-round5-wave3 after the previous integration merge, so the branch carried a real hole rather than a polish: the open-time guard accepted any nonzero secure_delete, which would have accepted ON where FAST was requested and, worse, accepted a cascade's raise that was never restored, leaving every later write paying the full erase cost with nothing reporting it. The merge is textually trivial - 41a2b2b is a direct child of the commit whose tree this branch already carries - but the merged tree is verified on its own rather than inherited from either side.
…quests This PR rewrites .cargo/audit.toml, which is the only file security-audit-rust.yml's audit step reads, and adds a cryptographic dependency subtree. The workflow fires on workflow_dispatch and a nightly schedule only, so neither the rewrite nor the new subtree is audited by the pull request that introduces them; the first run lands after merge. The fix is a workflow change (a pull_request trigger scoped to the manifest and lockfile paths) plus getting the job green, both of which belong to the CI file rather than to this PR's diff. The deferral is recorded where the next reader of this file will meet it rather than left to memory. Comment only - the TOML parses to the same ignore list.
Preserve mainline migration V007 and renumber the rehydration migrations. Reconcile sweep placeholders and ownership tracking with transaction-based confirmation heights, including cleanup of swept height-only transactions. Validation: storage tests with all features (952 passed, 2 ignored), cargo fmt, and targeted Clippy with CI flags. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Inject the lock result through a private constructor helper so the refusal test exercises warning, zero-initialization, and buffer access on the same allocation. Keep the production constructor bound to the real mlock call. Remove the assumption that every host rejects a fixed low address. Validation: 12 guarded-buffer tests with all features, rustfmt, and storage Clippy with CI flags on Linux. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…w-up PR Keep the insert_platform_node_pool_entry refactor and PlatformNodePoolError type in this PR (rs-platform-wallet-storage's rehydrate.rs depends on both to compile), but move the five new unit tests exercising them out of the storage PR's diff — none are needed for rs-platform-wallet-storage to build or pass its own tests. They land in a follow-up PR stacked on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Preserve the independently published platform-wallet test split alongside the verified storage migration fixes. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Move provider account and pool reconstruction with their errors and tests into SQLite storage. Restore wallet pool registration to its base version and shorten audit exception comments. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Match the enabled type's derives and conditional serde support. Check public trait parity and serde round-tripping with shielded off and on. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Validate both owner readers while accepting current and legacy standard labels. Strict loading fails on invalid labels; Recovery isolates the wallet so its used-address guard is never silently discarded. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Replace manual wipe-on-drop code with a zeroizing boxed block slice. Keep the matrix length stable and verify its contents are wiped in place. Co-Authored-By: OpenAI Codex <noreply@openai.com>
…hecksum Integrate manifest integrity with strict and recovery loading, provider-key registrations, and atomic V018 migration backfill. Preserve current FFI load API and all previously published migration contents. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Co-authored-by: Codex <noreply@openai.com>
…er errors Merge PR #3968 after its update to the latest v4.2-dev. Manifest checksum failures retain their fatal classification in the typed persister API. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Base automatically changed from
feat/platform-wallet-storage-rehydration
to
v4.2-dev
September 9, 2026 13:59
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Detects corrupted wallet account manifests before rebuilding accounts; recovery keeps healthy wallets available.
Issue being fixed or feature implemented
Detect corrupted account manifests and rows copied to the wrong wallet before reconstruction. Strict loading fails with a typed error; recovery excludes the affected wallet and reports its cause while loading healthy wallets.
What was done?
Stacked on #3968.
SHA-256(wallet_id || account_xpub_bytes)checksums, backfilled atomically during migration. V001–V017 remain unchanged.ManifestIntegrityMismatch(fatal). Recovery reportsmanifest_integrity_mismatchthroughlast_load_degradation().wallets_degraded. Missing checksums remain detectable after reopening.How Has This Been Tested?
cargo test -p platform-wallet-storage --features __test-helpers --no-fail-fast— 970 passed, 0 failed, 2 ignored, including documentation tests.cargo fmt -p platform-wallet-storage -- --checkand storage Clippy (--all-targets --features __test-helpers --locked -- --no-deps -D warnings) passed.Breaking Changes
None to the base PR’s public API. The additive migration establishes checksums for existing manifests; it cannot identify corruption that predates that migration.
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent