feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692)#3968
Conversation
📝 WalkthroughWalkthroughThis PR introduces a Tier-2 bincode secret-envelope wire format (scheme-0 plaintext / scheme-1 Argon2id+AEAD) under ChangesSecret Store: Tier-2 Envelope Wire Format and EncryptedFileStore Hardening
SQLite Wallets Rename, Keyless Rehydration, and Persister Hardening
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
lklimek
changed the title
…3968, independent on v3.1-dev] Storage-crate half of the rehydration work, rebuilt to stand alone on v3.1-dev: SqlitePersister::load() wiring + per-area readers (accounts, core_state, identities, asset_locks, contacts, identity_keys) that reconstruct the keyless ClientWalletStartState. Independence on v3.1-dev required two deliberate stubs — the reshaped ClientWalletStartState drops wallet/wallet_info, breaking two base consumers; both are resolved by #3692 in the dash-evo-tool integration: - manager/load.rs: whole-body todo!("keyless rehydration lands in #3692") - ffi/persistence.rs: tail-only todo!("seeded FFI restore path lands in #3692") — keeps the 8 builder helpers live (no dead_code under -D warnings) and minimizes the #3692 merge conflict Cross-crate manager-apply e2e tests in sqlite_core_state_reader.rs are gated behind a new off-by-default `rehydration-apply` feature (enabled in the integrated stack); storage-level load_state assertions run standalone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
52cdad9 to
83f7d4f
Compare
lklimek
force-pushed
the
feat/platform-wallet-storage-rehydration
branch
from
3d57f73 to
2f2a74a
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
🔍 Review in progress — actively reviewing now (commit 4624cec) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR adds the storage-side keyless load readers, but it also replaces two externally reachable restore paths with unconditional panics. The new rehydration readers are mostly wired, but several fail-hard corruption checks are missing where typed SQLite columns can disagree with decoded blobs.
🔴 2 blocking | 🟡 6 suggestion(s)
Findings not posted inline (2)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key —load_state()selectsidentity_idbut discards it, then decodesentry_bloband routes the restored identity usingentry.id. The writer rejectsIdentityEntryvalues whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader shou... - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys — The contacts reader keys pending rows from(owner_id, contact_id)but stores the decodedContactRequestwithout checking its sender and recipient IDs. During apply, sent requests are inserted underentry.request.recipient_idand incoming requests underentry.request.sender_id, so a row wh...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:13-15: Public manager restore API now panics
`load_from_persistor()` is a public restore entry point returning `Result<(), PlatformWalletError>`, but this PR replaces the previous implementation with `todo!()`. The exported C ABI function `platform_wallet_manager_load_from_persistor` calls this method directly, and the Swift `loadFromPersistor()` wrapper calls that exported function, so any app invoking persisted wallet restore aborts instead of receiving a typed error. If this branch intentionally defers keyless manager rehydration to #3692, the public API still needs to fail closed with an error rather than panic across the FFI boundary.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3389-3390: FFI persister load panics after receiving restore rows
`FFIPersister::load()` calls `build_wallet_start_state()` for every wallet returned by the Swift `on_load_wallet_list_fn` callback, and this function now reaches an unconditional `todo!()` after partially reconstructing the entry. This path is externally reachable through restore and shielded binding flows that call `persister.load()`. A panic here can unwind toward `extern "C"` callers and abort the process instead of returning the existing `PersistenceError`/`PlatformWalletFFIResult` failure path.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
`load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
`load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:168-169: Identity-key reader does not verify decoded entries match row columns
`load_state()` reconstructs `(identity_id, key_id)` from the SQL row, decodes `public_key_blob`, and inserts the decoded entry without checking that the blob carries the same identity, key id, wallet id, or public-key hash. The apply path later ignores the changeset map key and routes by fields from the decoded `IdentityKeyEntry`, so a semantically inconsistent row can attach a public key to the wrong identity or carry a hash that disagrees with the indexed column. Mirror the writer-side consistency checks on read before inserting into the changeset.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:316-325: Oversized BLOB rows are materialized before the size cap runs
The new load readers fetch BLOB columns directly into `Vec<u8>` and only then call `blob::decode()`, whose 16 MiB cap runs after rusqlite has already allocated and copied the value. A restored or locally modified SQLite DB can therefore store a huge `record_blob` or other `*_blob` value that passes SQLite integrity checks and forces large process allocations on startup before returning `BlobTooLarge`. Use a shared bounded read helper or select `length(blob_column)` first, as the KV path already does, before materializing BLOB contents.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs (1)
165-180: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCount
identity_keysbywallet_idnow that the table is wallet-scoped.
identity_keysmoved onto(wallet_id, identity_id, key_id), but this smoke test still routes it through thevia_identitypath. That means the assertion would still pass if the row were written with the wrongwallet_idas long asidentity_idmatched, so the new schema contract is not actually being exercised here.Suggested fix
let via_identity = [ - "identity_keys", "token_balances", "dashpay_profiles", "dashpay_payments_overlay", ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs` around lines 165 - 180, The smoke test still treats identity_keys as identity-scoped, but the schema now scopes it by wallet_id. Update the test logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query path instead of the via_identity branch, while keeping the other tables that still depend on identities routed through identity_id. Use the existing via_identity handling in the loop over cases to locate and adjust the count_sql selection.packages/rs-platform-wallet-storage/SCHEMA.md (1)
507-513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe soft-cascade note overstates cleanup for identity-scoped metadata.
meta_identityandmeta_tokendo not carrywallet_id, so a wallet delete only reaches them through existingidentitiesrows. If metadata was written before anidentitiesrow ever existed, that cleanup path never fires; the orphan-metadata section above already documents exactly that case.Suggested wording
-`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that -brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`, -`meta_platform_address`) by `wallet_id`, and the FK cascade through -`identities` fires a per-identity trigger that brooms `meta_identity` + -`meta_token` by `identity_id`. Both legs key on the id alone, so a wallet -delete cleans its metadata transitively whether or not the typed parent -was ever written and regardless of any contact's lifecycle state. +`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that +brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`, +`meta_platform_address`) by `wallet_id`, and the FK cascade through +existing `identities` rows fires a per-identity trigger that brooms +`meta_identity` + `meta_token` by `identity_id`. That means wallet-scoped +metadata is cleaned regardless of typed-parent existence, while +identity-scoped metadata still requires an `identities` row to exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/SCHEMA.md` around lines 507 - 513, The soft-cascade description in SCHEMA.md overstates what a wallet delete cleans up for identity-scoped metadata. Update the note near the wallet/identity trigger flow to say that `wallets` deletion only reaches `meta_identity` and `meta_token` through existing `identities` rows and that orphan metadata written before an `identities` row exists is not covered; align the wording with the existing orphan-metadata section and reference the `wallets` trigger and the `identities` FK cascade path.packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs (1)
27-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed on corrupted platform-payment registration rows.
This helper trusts the typed
account_indexcolumn but never verifies that the decodedAccountRegistrationEntryis actually aPlatformPaymententry for that same index.all_platform_payment_registrations()feedsplatform_addrs::load_all(), so a tampered row will currently rehydrate under the typed index with the blob's xpub instead of trippingAccountRegistrationEntryMismatch.Suggested fix
fn decode_platform_payment_row( account_index: i64, xpub_bytes: &[u8], ) -> Result<PlatformPaymentRegistration, WalletStorageError> { let account_index = crate::sqlite::util::safe_cast::i64_to_u32( "account_registrations.account_index", account_index, )?; let entry: AccountRegistrationEntry = blob::decode(xpub_bytes)?; + if account_type_db_label(&entry.account_type) != "platform_payment" + || account_index(&entry.account_type) != account_index + { + return Err(WalletStorageError::AccountRegistrationEntryMismatch); + } Ok((account_index, entry.account_xpub)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs` around lines 27 - 36, `decode_platform_payment_row` currently decodes the blob and returns the typed `account_index` without checking that the `AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update this helper to validate the decoded `AccountRegistrationEntry` matches the expected `PlatformPayment` variant and index, and return `AccountRegistrationEntryMismatch` if it does not. Keep the existing `safe_cast::i64_to_u32` conversion, but make `all_platform_payment_registrations()` fail closed by rejecting any corrupted or mismatched row instead of rehydrating it.packages/rs-platform-wallet-storage/src/sqlite/backup.rs (2)
243-263: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not delete WAL/SHM before the replacement is guaranteed.
If sibling removal succeeds and
tmp.persist(dest_db_path)then fails, the original main DB remains but its WAL/SHM may be gone, losing committed WAL-mode state. The restore path needs a rollback-safe swap strategy or a SQLite-native restore that does not destructively unlink siblings before the main replacement succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 243 - 263, The restore flow in `backup.rs` removes `-wal`/`-shm` siblings before `tmp.persist(dest_db_path)`, which can leave the original DB intact but its WAL-mode state lost if persist fails. Change the `restore` logic to use a rollback-safe replacement strategy: do not unlink siblings until the destination swap is guaranteed, or replace the whole SQLite set atomically via a SQLite-native restore path. Keep the fix localized around the sibling cleanup and `tmp.persist` sequence so the operation remains all-or-nothing.
361-374: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply
keep_last_nas a floor, not a ceiling.With both
keep_last_nandmax_ageset, line 373 still requirespass_count, so backups beyond the newest N are deleted even when they are withinmax_age. That contradicts the new floor semantics.Proposed fix
- let pass_count = match policy.keep_last_n { - Some(n) => idx < n, - None => true, - }; let pass_age = match policy.max_age { Some(max) => now.duration_since(ts).map(|d| d <= max).unwrap_or(true), - None => true, + None => policy.keep_last_n.is_none(), }; - if within_floor || (pass_count && pass_age) { + if within_floor || pass_age {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 361 - 374, In backup pruning logic in the `retain_backups` flow, `keep_last_n` is still being treated like a ceiling because the deletion condition requires `pass_count` even when `max_age` is also set. Update the condition around `within_floor`, `pass_count`, and `pass_age` so that the newest N backups are always kept as a floor and any backup within the age limit is also retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in this block.packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)
143-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate typed identity columns against the blob during load.
load_stateignores the selectedidentity_id, so a corrupted row whose typed column andentry_blob.iddiverge is silently rehydrated under the blob value. Also reject a blobwallet_idthat disagrees with the scoped wallet.Proposed fix
- let _identity_id: Vec<u8> = row.get(0)?; + let identity_id: Vec<u8> = row.get(0)?; let payload: Vec<u8> = row.get(1)?; let tombstoned: i64 = row.get(2)?; if tombstoned != 0 { continue; } + let typed_id = <[u8; 32]>::try_from(identity_id.as_slice()) + .map_err(|_| WalletStorageError::blob_decode("identities.identity_id is not 32 bytes"))?; let entry: IdentityEntry = blob::decode(&payload)?; + if entry.id.as_bytes() != &typed_id { + return Err(WalletStorageError::IdentityEntryIdMismatch); + } + if let Some(entry_wallet_id) = entry.wallet_id { + if entry_wallet_id != *wallet_id { + return Err(WalletStorageError::WalletIdMismatch { + expected: *wallet_id, + found: entry_wallet_id, + }); + } + } let managed = managed_identity_from_entry(&entry, wallet_id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs` around lines 143 - 150, The load path in load_state is trusting the blob too much and currently ignores the selected identity_id, so mismatched typed columns can be silently rehydrated under the blob value. Update the row handling in load_state to validate that the typed identity_id matches entry_blob.id before decoding into IdentityEntry, and also verify the blob wallet_id matches the wallet_id scope passed into managed_identity_from_entry. If either check fails, reject the row instead of continuing.packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)
299-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the open-path registry before restore.
restore_from_innercan replacedest_db_pathwhile a liveSqlitePersisterin this process still owns the same DB. Check the registry up front and returnAlreadyOpen; otherwise the live handle/buffer can diverge from the restored file.Proposed fix outline
+ let registered_path = dest_db_path + .canonicalize() + .unwrap_or_else(|_| dest_db_path.to_path_buf()); + if open_path_registry() + .lock() + .unwrap_or_else(|p| p.into_inner()) + .contains(®istered_path) + { + return Err(WalletStorageError::AlreadyOpen { + path: registered_path, + }); + } + if !skip_backup && dest_db_path.exists() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines 299 - 326, restore_from_inner currently restores the database without checking whether the destination path is already owned by a live SqlitePersister, which can leave an in-memory handle out of sync with the replaced file. Add an upfront registry lookup in restore_from_inner for dest_db_path and return WalletStorageError::AlreadyOpen when the path is already registered, before any backup or restore work begins. Keep the change localized around restore_from_inner and the open-path registry used by SqlitePersister so existing live handles are protected from restore-time replacement.
🧹 Nitpick comments (4)
packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs (1)
91-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
synced_heightas well aslast_processed_height.This test writes both fields, but only validates one of them. If
load()stops wiringsynced_height, the round-trip still passes.Suggested assertion
assert_eq!(slice.core_state.new_utxos.len(), 1); assert_eq!(slice.core_state.new_utxos[0].value(), 777_000); + assert_eq!(slice.core_state.synced_height, Some(50)); assert_eq!(slice.core_state.last_processed_height, Some(50));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs` around lines 91 - 127, The round-trip test in `sqlite_load_wiring.rs` only verifies `last_processed_height` from `state.wallets.get(&w).core_state` even though `synced_height` is also written into `CoreChangeSet`; update the existing load assertions to check both fields after `p2.load()` so `load()` wiring regressions for `synced_height` are caught alongside `last_processed_height`.packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs (1)
93-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the overlay stays out of the rehydrated identity.
This currently proves only that
load()still returns the wallet's core state. If a regression starts mergingdashpay_profilesinto the loaded identity payload, this test still passes. Please also assert that the seeded identity is present afterload()and that its DashPay profile remains absent for the overlay-only write case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs` around lines 93 - 108, The current test around persister.load() only verifies wallet.core_state, so it can miss regressions where dashpay_profiles gets merged into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to also inspect the loaded identity payload for the seeded wallet after load() and assert that the identity is still present while its DashPay profile remains absent in this overlay-only write scenario. Use the existing persister.load(), wallets.get(&w), and any identity fields already available in the loaded state to make the check explicit.packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs (1)
67-72: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert that the failed pre-flush left nothing durable.
Restoring the buffer is only half of the contract here. If
apply_changeset_to_txever leaks thewalletsinsert before thecore_sync_statefailure, this test still passes and leaves duplicate-on-retry state behind.Suggested assertion block
assert!( persister.buffer_has_changeset_for_test(&w), "buffered changeset must be restored after a real pre-flush apply failure" ); + + let conn = persister.lock_conn_for_test(); + let wallets_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM wallets WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); + let core_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(wallets_rows, 0, "failed pre-flush must not durably create the wallet row"); + assert_eq!(core_rows, 0, "failed pre-flush must not durably create child rows");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs` around lines 67 - 72, The test currently only verifies the buffered changeset is restored, but it should also verify that a failed pre-flush did not persist any durable state. In sqlite_delete_real_apply_failure.rs, extend the existing scenario around the failed delete so it checks the database/transaction state after the apply failure and confirms no `wallets` insert or other durable side effects remain from `apply_changeset_to_tx`. Keep the existing `persister.buffer_has_changeset_for_test(&w)` assertion, and add a second assertion in the same test that validates the storage is clean after the failure so retry does not see duplicate-on-retry state.packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)
813-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the query-budget documentation.
load()currently performs multiple reader calls inside thefor wallet_id in wallet_idsloop, so the query count grows with wallet count. Reword this to avoid promising constant query budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines 813 - 814, Update the query-budget comment in the load path so it no longer claims constant cost with wallet count; the current load() flow iterates over wallet_ids and performs multiple reader calls per wallet, so reword the documentation to describe that it has per-wallet read/query work rather than a fixed query budget. Keep the note near the wallet_ids loop/load() implementation and make sure the wording matches the actual behavior of the reader calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 3389-3390: The temporary restore stub in the persistence restore
flow should not panic via todo!(); replace it with a recoverable typed error so
callers receive a PersistenceError instead of crashing. Update the restore-path
branch that currently ignores identity_manager and unused_asset_locks to return
an appropriate PersistenceError variant (or equivalent error conversion) from
the same function/method, keeping the signature consistent and preserving the
existing error handling path.
In `@packages/rs-platform-wallet-storage/README.md`:
- Around line 165-168: The README wording around the manager-side rehydration
flow is too strong for this PR because the manager/FFI load path is still
stubbed. Update the description near the watch-only rebuild note to clearly mark
the manager-side `load_from_persistor`/`Wallet::new_watch_only` application as
pending or follow-up work, and keep the current text scoped to the storage-side
behavior only.
In `@packages/rs-platform-wallet-storage/src/kv.rs`:
- Around line 62-65: The key-length validation in validate_key currently assumes
Rust chars().count() matches SQLite length() for all strings, but embedded NULs
break that equivalence. Update the key precheck to explicitly reject keys
containing \0 before comparing length, or adjust the validation/comment so it no
longer claims the same key set; keep the logic aligned with the SQL CHECK
constraint in kv.rs.
In `@packages/rs-platform-wallet-storage/src/secrets/error.rs`:
- Around line 3-5: The file-level non-leakage docs in error.rs are too broad for
the current Io behavior: they claim variants never carry a stringified source,
but Io::fmt/rendering still exposes the underlying source text. Update the docs
to carve out the Io exception, or change Io’s display implementation/tests so it
no longer includes the source string, keeping the wording aligned with the
actual Error and Io rendering behavior.
- Around line 88-91: The UnsupportedEnvelopeVersion error currently truncates
the envelope version to u8, so update the error variant in error.rs to store the
full u32 version value instead. Then adjust the envelope parsing call site that
constructs UnsupportedEnvelopeVersion to pass the original Envelope.version
without narrowing, keeping the reported version accurate in the error message.
In `@packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- Around line 21-22: The docs for the nested BTreeMap format currently imply
duplicate (wallet_id, label) pairs are prevented entirely, but the read path
still accepts duplicate JSON keys and serde collapses them. Update the
documentation near the format description to state that uniqueness is only
guaranteed by serialization, or change the deserialization logic in the file
format/parser code to explicitly reject duplicate keys, and make the behavior
match the tests and the intended API.
In `@packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- Around line 628-654: The post-persist Unix handling in the vault write path is
swallowing parent-directory fsync failures and returning success, which makes
`put`/`delete`/`rekey` report a durable commit when only the rename succeeded.
Update the flow around the `persist()`/`sync_all()` block to surface a distinct
“committed but not durable” result or otherwise keep the in-memory commit behind
the durability boundary, and make sure the caller can tell when
`fs::File::open(parent)` or `sync_all()` fails instead of only logging via
`tracing::warn!`.
In `@packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- Around line 255-266: The reprotect method in SecretStore currently does a
non-atomic read-then-write using get_secret followed by set_secret, which can
overwrite concurrent updates with stale plaintext. Update reprotect to use an
atomic backend-specific reprotect/CAS path, or add a version check so the write
only succeeds if the entry has not changed since get_secret; reference
SecretStore::reprotect, get_secret, and set_secret when wiring the fix.
In `@packages/rs-platform-wallet-storage/src/secrets/wire/envelope.rs`:
- Around line 136-141: The scheme-0 plaintext path in the envelope handling
still leaves temporary Vec<u8> buffers unwiped, including the
Unprotected(plaintext.to_vec()) branch and the ExpectedProtectedButUnsealed arm.
Update the envelope logic in the encode/decode flow around the Envelope and
Payload handling to use zeroizing storage for these plaintext temporaries or
explicitly wipe them before drop, while keeping SecretBytes::new only for the
final encoded blob.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 179-199: `persist`/`open` currently treats `has_schema_history()`
as the only brand-new-vs-existing check, so a pre-existing non-wallet SQLite
file with no `refinery_schema_history` can still be migrated. Add an explicit
guard in the `had_schema_history` decision path to reject existing SQLite files
that lack wallet schema history, using the same `conn`/`has_schema_history` flow
and returning a typed wallet storage error before any backup, integrity check,
or `migrations::run()` work begins.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 143-154: The sync-state write path in core_state should treat
last_applied_chain_lock monotonically, not as a blind overwrite. Update the
CoreChangeSet-to-DB flow around upsert_sync_state so the stored chain-lock is
max-merged with the existing row (using the same chain-lock height comparison
logic as the height watermarks) before persisting. Apply this behavior wherever
last_applied_chain_lock is written in the affected core_state update functions
so the persisted chain-lock cannot regress.
- Around line 40-41: The `decode_from_slice` handling in
`last_applied_chain_lock` is too permissive because it accepts a valid prefix
and ignores any appended data. Update this decoding path in `core_state.rs` to
mirror the other blob decoders: after calling `bincode::decode_from_slice` for
`ChainLock`, verify the returned consumed length matches `bytes.len()` and treat
any mismatch as corruption by returning `None` instead of loading the state.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 151-169: Mirror the writer-side validation in load_state by
checking that each decoded public_key_blob matches the row’s typed columns
before inserting into cs.upserts. After decode_entry(&payload), verify the
entry’s identity_id, key_id, wallet_id, and public_key_hash against the values
from the identity_keys query, and return a WalletStorageError if any mismatch is
found. Keep the checks local to load_state and use the existing decode_entry,
Identifier::from, and KeyID::try_from flow so inconsistent rows are rejected
instead of loaded silently.
In `@packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs`:
- Around line 46-82: The sqlite_accounts_reader test is too weak because both
AccountRegistrationEntry fixtures use the same xpub and the assertions only
check set membership, so row reordering or xpub/row mixups can still pass.
Update the test to use distinct xpub fixtures for each entry and assert the
loaded manifest in the expected order, using the accounts::load_state result and
the existing AccountType variants to verify each row maps to the correct xpub.
In `@packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs`:
- Line 33: The doc comment on the wallet start state field still references the
old wallet_metadata table. Update the comment in client_wallet_start_state.rs to
point to the renamed wallets table instead, keeping the wording aligned with the
field’s source of truth and using the existing comment near the network field to
locate it.
In `@packages/rs-platform-wallet/src/manager/load.rs`:
- Around line 8-14: The public rehydration entry point
PlatformWalletManager::load_from_persistor currently panics via todo!, which
turns a caller error into a runtime abort. Replace the todo! with a recoverable
Result path by returning an explicit PlatformWalletError for the unsupported
stub state, or otherwise gate/remove this API until keyless rehydration in
PlatformWalletManager is implemented. Ensure callers receive an error instead of
a panic.
---
Outside diff comments:
In `@packages/rs-platform-wallet-storage/SCHEMA.md`:
- Around line 507-513: The soft-cascade description in SCHEMA.md overstates what
a wallet delete cleans up for identity-scoped metadata. Update the note near the
wallet/identity trigger flow to say that `wallets` deletion only reaches
`meta_identity` and `meta_token` through existing `identities` rows and that
orphan metadata written before an `identities` row exists is not covered; align
the wording with the existing orphan-metadata section and reference the
`wallets` trigger and the `identities` FK cascade path.
In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- Around line 243-263: The restore flow in `backup.rs` removes `-wal`/`-shm`
siblings before `tmp.persist(dest_db_path)`, which can leave the original DB
intact but its WAL-mode state lost if persist fails. Change the `restore` logic
to use a rollback-safe replacement strategy: do not unlink siblings until the
destination swap is guaranteed, or replace the whole SQLite set atomically via a
SQLite-native restore path. Keep the fix localized around the sibling cleanup
and `tmp.persist` sequence so the operation remains all-or-nothing.
- Around line 361-374: In backup pruning logic in the `retain_backups` flow,
`keep_last_n` is still being treated like a ceiling because the deletion
condition requires `pass_count` even when `max_age` is also set. Update the
condition around `within_floor`, `pass_count`, and `pass_age` so that the newest
N backups are always kept as a floor and any backup within the age limit is also
retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in
this block.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 299-326: restore_from_inner currently restores the database
without checking whether the destination path is already owned by a live
SqlitePersister, which can leave an in-memory handle out of sync with the
replaced file. Add an upfront registry lookup in restore_from_inner for
dest_db_path and return WalletStorageError::AlreadyOpen when the path is already
registered, before any backup or restore work begins. Keep the change localized
around restore_from_inner and the open-path registry used by SqlitePersister so
existing live handles are protected from restore-time replacement.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- Around line 27-36: `decode_platform_payment_row` currently decodes the blob
and returns the typed `account_index` without checking that the
`AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update
this helper to validate the decoded `AccountRegistrationEntry` matches the
expected `PlatformPayment` variant and index, and return
`AccountRegistrationEntryMismatch` if it does not. Keep the existing
`safe_cast::i64_to_u32` conversion, but make
`all_platform_payment_registrations()` fail closed by rejecting any corrupted or
mismatched row instead of rehydrating it.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 143-150: The load path in load_state is trusting the blob too much
and currently ignores the selected identity_id, so mismatched typed columns can
be silently rehydrated under the blob value. Update the row handling in
load_state to validate that the typed identity_id matches entry_blob.id before
decoding into IdentityEntry, and also verify the blob wallet_id matches the
wallet_id scope passed into managed_identity_from_entry. If either check fails,
reject the row instead of continuing.
In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs`:
- Around line 165-180: The smoke test still treats identity_keys as
identity-scoped, but the schema now scopes it by wallet_id. Update the test
logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query
path instead of the via_identity branch, while keeping the other tables that
still depend on identities routed through identity_id. Use the existing
via_identity handling in the loop over cases to locate and adjust the count_sql
selection.
---
Nitpick comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 813-814: Update the query-budget comment in the load path so it no
longer claims constant cost with wallet count; the current load() flow iterates
over wallet_ids and performs multiple reader calls per wallet, so reword the
documentation to describe that it has per-wallet read/query work rather than a
fixed query budget. Keep the note near the wallet_ids loop/load() implementation
and make sure the wording matches the actual behavior of the reader calls.
In
`@packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs`:
- Around line 93-108: The current test around persister.load() only verifies
wallet.core_state, so it can miss regressions where dashpay_profiles gets merged
into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to
also inspect the loaded identity payload for the seeded wallet after load() and
assert that the identity is still present while its DashPay profile remains
absent in this overlay-only write scenario. Use the existing persister.load(),
wallets.get(&w), and any identity fields already available in the loaded state
to make the check explicit.
In
`@packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs`:
- Around line 67-72: The test currently only verifies the buffered changeset is
restored, but it should also verify that a failed pre-flush did not persist any
durable state. In sqlite_delete_real_apply_failure.rs, extend the existing
scenario around the failed delete so it checks the database/transaction state
after the apply failure and confirms no `wallets` insert or other durable side
effects remain from `apply_changeset_to_tx`. Keep the existing
`persister.buffer_has_changeset_for_test(&w)` assertion, and add a second
assertion in the same test that validates the storage is clean after the failure
so retry does not see duplicate-on-retry state.
In `@packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs`:
- Around line 91-127: The round-trip test in `sqlite_load_wiring.rs` only
verifies `last_processed_height` from `state.wallets.get(&w).core_state` even
though `synced_height` is also written into `CoreChangeSet`; update the existing
load assertions to check both fields after `p2.load()` so `load()` wiring
regressions for `synced_height` are caught alongside `last_processed_height`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edc85543-e83f-4a54-88ef-17800859c720
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-storage/.cargo/audit.tomlpackages/rs-platform-wallet-storage/Cargo.tomlpackages/rs-platform-wallet-storage/README.mdpackages/rs-platform-wallet-storage/SCHEMA.mdpackages/rs-platform-wallet-storage/SECRETS.mdpackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rspackages/rs-platform-wallet-storage/src/kv.rspackages/rs-platform-wallet-storage/src/lib.rspackages/rs-platform-wallet-storage/src/secrets/error.rspackages/rs-platform-wallet-storage/src/secrets/file/crypto.rspackages/rs-platform-wallet-storage/src/secrets/file/format.rspackages/rs-platform-wallet-storage/src/secrets/file/mod.rspackages/rs-platform-wallet-storage/src/secrets/keyring.rspackages/rs-platform-wallet-storage/src/secrets/mod.rspackages/rs-platform-wallet-storage/src/secrets/secret.rspackages/rs-platform-wallet-storage/src/secrets/store.rspackages/rs-platform-wallet-storage/src/secrets/wire/aad.rspackages/rs-platform-wallet-storage/src/secrets/wire/config.rspackages/rs-platform-wallet-storage/src/secrets/wire/envelope.rspackages/rs-platform-wallet-storage/src/secrets/wire/kdf.rspackages/rs-platform-wallet-storage/src/secrets/wire/mod.rspackages/rs-platform-wallet-storage/src/sqlite/backup.rspackages/rs-platform-wallet-storage/src/sqlite/config.rspackages/rs-platform-wallet-storage/src/sqlite/conn.rspackages/rs-platform-wallet-storage/src/sqlite/error.rspackages/rs-platform-wallet-storage/src/sqlite/kv.rspackages/rs-platform-wallet-storage/src/sqlite/migrations.rspackages/rs-platform-wallet-storage/src/sqlite/persister.rspackages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet-storage/src/sqlite/schema/blob.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rspackages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rspackages/rs-platform-wallet-storage/src/sqlite/schema/mod.rspackages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rspackages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rspackages/rs-platform-wallet-storage/src/sqlite/schema/wallets.rspackages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rspackages/rs-platform-wallet-storage/tests/common/mod.rspackages/rs-platform-wallet-storage/tests/secrets_api.rspackages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rspackages/rs-platform-wallet-storage/tests/secrets_scan.rspackages/rs-platform-wallet-storage/tests/sqlite_account_zero_attribution.rspackages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_asset_locks_filter.rspackages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rspackages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rspackages/rs-platform-wallet-storage/tests/sqlite_commit_writes_lock_poison_shortcircuit.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rspackages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_partial_commit_window.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rspackages/rs-platform-wallet-storage/tests/sqlite_error_classification.rspackages/rs-platform-wallet-storage/tests/sqlite_fk_changeset_ordering.rspackages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rspackages/rs-platform-wallet-storage/tests/sqlite_identity_keys_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet-storage/tests/sqlite_money_column_overflow_on_read.rspackages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rspackages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rspackages/rs-platform-wallet-storage/tests/sqlite_second_open_guard.rspackages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rspackages/rs-platform-wallet-storage/tests/sqlite_wallet_db_identity.rspackages/rs-platform-wallet/src/changeset/client_wallet_start_state.rspackages/rs-platform-wallet/src/manager/load.rs
…riminators to stop distinct-variant collapse (#3968 review) account_registrations keyed on (wallet_id, account_type, account_index) only. PlatformPayment key classes and DashPay (user, friend) identity pairs share that key across genuinely distinct accounts, so the ON CONFLICT DO UPDATE silently overwrote one with another — a restored wallet lost accounts (data loss). Chose option (a) widen-PK over fail-loud: a wallet legitimately holds multiple DashpayReceivingFunds accounts (one per contact) at the same index, so failing the collision would reject valid multi-contact wallets. Add key_class, user_identity_id, friend_identity_id as NOT NULL columns with sentinel defaults (0 / zeroblob) so non-discriminated variants still dedup on re-persist, and widen the PK to include them. The reader cross-checks every typed PK column against the decoded blob and orders deterministically. V001 edited in place (on-disk format unshipped). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…open database (#3968 review) restore_from's doc promised the destination must not be open in this process, but restore_from_inner never consulted the open-path registry. Restoring over a live persister's file would leave that handle's connection and write buffer silently diverged from the freshly restored bytes. Canonicalize dest_db_path the same way open() registers it and return WalletStorageError::AlreadyOpen when the path is held by a live persister. The guard runs before the pre-restore auto-backup, so both restore_from and restore_from_skip_backup are covered. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…gainst typed columns on read (#3968 review) identity_keys::load_state inserted each decoded entry under the typed (identity_id, key_id) columns with no cross-check of the blob's own identity_id / key_id / wallet_id — unlike its accounts and asset_locks siblings. A row whose blob disagrees with its indexed columns (corruption that passes PRAGMA integrity_check) would be silently mis-keyed into the upsert map. Assert the decoded ids and wallet scope equal the typed columns after decode_entry, returning the existing IdentityKeyEntryMismatch otherwise. Mirrors the writer-side guard and the sibling readers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…e owning wallet (#3968 review) identities apply used ON CONFLICT(identity_id) DO UPDATE. Ownership was already guarded (wallet_id COALESCE + wallet-scoped tombstone), but entry_blob / identity_index overwrite and the tombstoned=0 reset fired unconditionally across wallet scopes — a wallet-B flush could clobber wallet-A's blob/index and resurrect a tombstoned identity. Add 'WHERE identities.wallet_id IS NULL OR identities.wallet_id IS excluded.wallet_id' to the DO UPDATE so the overwrite fires only for an unowned row (orphan -> parented promotion) or the owning wallet. A cross-wallet write becomes a no-op (SQLite skips a false-WHERE upsert without erroring). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…txos to prevent address reuse (#3968 review) #3968's ClientWalletStartState was missing the used_core_addresses field that #3692 added, so its struct diverged AND the native/SQLite path could not restore address used-ness — a used-then-emptied address would be handed back as a fresh receive address (address reuse). Add the field (type Vec<key_wallet::Address>, matching #3692 byte-for-byte) and populate it in load(). The in-band pool snapshot was retired (account_index hardcoded 0), so used addresses are derived from the full core_utxos set (spent + unspent): every address that ever held a UTXO is used, mapped script_pubkey -> Address network-aware. A focused test persists a spent (zero-balance) UTXO and asserts its address comes back in used_core_addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…ad of panicking (#3968 review) load_from_persistor() and the FFI build_wallet_start_state() were `todo!()` stubs whose keyless-load path lands in #3692. Both run beneath an extern "C" boundary, where an unwind is undefined behaviour — a panic there is worse than an error. Return PlatformWalletError::WalletCreation / PersistenceError::backend respectively. The integration branch still overwrites both bodies with #3692's real implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…ast_n floor (#3968 review) C1: restore_from cleared the destination's WAL/SHM siblings BEFORE the atomic persist swap, so a failed persist (disk full, EXDEV, perms) left the live DB without its WAL-committed state. Reorder: persist FIRST (the rename is the commit point — a failed restore now leaves the old DB untouched), then unlink the now-stale siblings so a leftover -wal can't shadow the restored DB. M4: prune's keep_last_n acted as a ceiling when combined with max_age — files beyond the N newest were evicted even within the age window. Make it a true floor: keep a file if it satisfies EITHER policy (the union), removing only files failing both. Adds a regression test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…rrect load query-budget doc (#3968 review) M3: a pre-existing non-wallet SQLite file (schema objects but no refinery_schema_history) was treated as brand-new and migrated in place, grafting wallet tables onto a foreign schema. open() now detects a non-empty DB without refinery history and rejects it via the application_id gate (NotAWalletDb) instead of migrating. N4: the load() 'Query budget' doc claimed a constant query count, but the keyless per-wallet payload is a fan-out (O(wallets) reads). Corrected the doc to describe the actual behaviour. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…across schema modules (#3968 review) H4: last_applied_chain_lock was blind-overwritten while heights max-merged, so an out-of-order lower-height chain lock regressed the finalized checkpoint. Monotonic-max-merge the chain lock by block_height too. H5: identities::load_state trusted the decoded blob's identity_id over the typed column. M1: contacts::load_state decoded ContactRequests without checking sender/recipient vs the typed owner/contact columns. M2: decode_platform_payment_row decoded the blob without validating its account_type/index. All now apply the cross-check pattern (reject on mismatch) like the sibling readers. M5: decode_chain_lock_soft ignored trailing bytes after a valid ChainLock; now asserts consumed == len. L1: core_transactions reader capped record_blob via length() BEFORE materializing, so a tampered oversize blob can't OOM the process. L3: accounts-reader roundtrip test now uses distinct xpubs and asserts the deterministic manifest order. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
L2: validate_key relied on chars().count() == SQLite length(), but SQLite length() stops at the first NUL, so a NUL-bearing key broke the invariant (and SQLite string comparisons). Reject embedded NUL explicitly with a new KvError::KeyContainsNul variant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…/rebuild notes, add scope assertion (#3968 review) N1: drop the stale '(from wallet_metadata)' ref on ClientWalletStartState.network to re-sync with #3692. L4: README now marks the manager-side rebuild (load_from_persistor) as pending/stubbed on this build, not live. N2: SCHEMA.md corrected — identity-scoped meta cleanup is conditional on the identities row existing and being wallet-linked, not unconditional. N3: sqlite_migrations smoke test now also asserts identity_keys is countable by its direct wallet_id column. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…cheme-0 zeroize, version width (#3968 review) H1: SecretStore::reprotect did a non-atomic get→set; a concurrent set/delete between them could let a transform built on stale bytes clobber a newer value. The File arm now runs the read→rewrap→write under the store's single lock (new EncryptedFileStore::reprotect_bytes). The Os arm is a per-item keyring with no transaction, so its non-atomicity is now documented as a residual. H3: the scheme-0 (unprotected) wrap left a cleartext plaintext copy in the envelope's Vec unzeroized; it's now wiped after encoding (the returned SecretBytes is the only retained copy). L6: UnsupportedEnvelopeVersion stored the version as u8, truncating Envelope.version (u32) — now u32 so a >255 version isn't aliased in diagnostics (with a regression test). L5: narrowed the error-module non-leakage doc — the Io variant intentionally carries its non-secret OS source; every other variant is source-free. L7: documented the vault's read-side duplicate-JSON-key collapse (serde_json last-wins), benign because every entry is AEAD-bound to (wallet_id, label). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…ning read-before-cap / unbounded-bincode gaps (#3968 review) * blob.rs: make bounded_config() pub(crate); add check_size(i64) and check_fixed_width(i64, usize, &'static str) helpers * Remove chain_lock_config() — replaced everywhere with blob::bounded_config() * DRY all inline 4-6 line gate blocks across accounts, asset_locks, identities, contacts, identity_keys → single check_size/check_fixed_width call * core_state::load_state — restructure query_row→prepare+query for last_applied_chain_lock; gate length() before materializing Vec * core_state::get_tx_record — same restructure so check_size can early-return * platform_addrs::list_per_wallet / all_address_rows — add length(address) pre-read; check_fixed_width(len, 20) guards fixed-width column * identity_keys::from_entry / into_entry — switch from unbounded standard() to bounded_config() for inner public_key_bincode encode/decode * Tests: 4 new blob-gate integration tests (oversize chain-lock, wrong-width address, oversize address, crafted inner pk_bincode) * sqlite_compile_time: update allow-list substrings for modified SQL strings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… typed gate on unspent-UTXO reader (#3968 review) * Cargo.toml: enable rusqlite `limits` feature (required for set_limit / Limit API) * conn.rs: define SQLITE_MAX_BLOB_BYTES = 2 × SIZE_LIMIT_BYTES (32 MiB); call conn.set_limit(SQLITE_LIMIT_LENGTH, SQLITE_MAX_BLOB_BYTES) in open_conn for every connection (both ReadWrite and ReadOnly). Global backstop caps all ungated columns (script, outpoint, wallet_id, txid, identity_id, etc.) without requiring per-column length() pre-reads. Per-column typed gates still fire first at 16 MiB; this floor covers the remaining ~11 sibling columns Marvin's audit flagged. * core_state.rs load_state UTXO reader: restructure query_map closure → prepare + query + while let Some(row) so BlobTooLarge can be returned early; add length(outpoint) (col 0) and length(script) (col 3) to the SELECT; blob::check_size gate each before materializing Vec. * Tests: 2 new blob-gate integration tests: - connection_has_sqlite_limit_length_set: verifies SQLITE_LIMIT_LENGTH = 32 MiB - blob_gate_core_utxos_load_state_rejects_oversize_script: oversize script → BlobTooLarge * sqlite_compile_time.rs: add allow-list entry for new length()-prefixed UTXO SELECT <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…YTES fits i32 (#3968 review) Guards against a future increase to SIZE_LIMIT_BYTES silently wrapping the `as i32` cast in SQLITE_MAX_BLOB_BYTES and turning the global blob backstop into a no-op. The assert fires at compile time with a descriptive message, so widening SIZE_LIMIT_BYTES beyond ~1 GiB is a build error rather than a silent security regression. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: all three prior storage-reader findings are fixed at 53b7d26. New latest-delta findings: none validated in scope; the only new agent finding is a real FFI ownership concern, but the same callback/free behavior exists on the PR base and was not introduced or worsened by this PR.
Bring the PR branch up to date with the new base v4.1-dev (retarget from previous base). Clean merge, no conflicts. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Claudius-Maginificent
changed the base branch from
feat/platform-wallet-rehydration
to
v4.1-dev
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3968 +/- ##
============================================
+ Coverage 87.18% 87.27% +0.09%
============================================
Files 2632 2635 +3
Lines 327563 330271 +2708
============================================
+ Hits 285592 288260 +2668
- Misses 41971 42011 +40
🚀 New features to boost your workflow:
|
…' into fix/3968-merge-3692 # Conflicts: # packages/rs-platform-wallet-ffi/src/persistence.rs # packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs # packages/rs-platform-wallet/src/manager/load.rs
…in topology fix)
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This cumulative pass confirms the three prior findings on the storage BLOB-decode discipline (chain-lock cap, platform-address fixed-width, nested identity-key bincode) remain FIXED at 12979b5 — the delta since 53b7d26 does not touch rs-platform-wallet-storage. One new blocking finding emerged: load_used_addresses (added in this PR at commit 6365be7 as the rehydration address-reuse guard) reads core_utxos.script into a Vec without the length()+blob::check_size gate that every sibling reader in the same file applies, re-opening exactly the class of hazard the earlier rehydration-reader hardening closed. Remaining findings are lower-severity: a doc/impl mismatch in load_and_apply_persisted (only re-hydrates platform addresses despite the doc implying full replay for late-registered accounts), a wallet_id/bucket migration gap on the update branch of apply_identity_entry, a handful of ungated/partially-gated BLOB reads on adjacent readers, an FFI helper missing the isize::MAX guard its sibling has, and a test-fixture coverage gap for the new rehydration path.
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 3 nitpick(s)
Findings not posted inline (10)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate — Every other BLOB read in this file gates the column with aSELECT length(col), ...+blob::check_size()pre-read before materializing the Vec, per the file's own stated discipline (comment at lines 299-302: "Pre-read length() gates ... before materializing the Vec so tampered oversize values... - [SUGGESTION]
packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay — The doc comment states this is "the recommended entry point for startup hydration after late-registered accounts (e.g. DashPay contact accounts thatbootstrap_dashpay_contact_accountsadds) have landed" and that "a second call after account bootstrap picks up the rest without regressing anyth... - [SUGGESTION]
packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch — On the existing-identity branch (lines 48-69), scalar fields are updated in place butexisting.wallet_idis never reassigned and no bucket move betweenout_of_wallet_identitiesandwallet_identities[wallet_id](withlocation_indexupdate) is performed — unlike the fresh-insert path (lines... - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate —load_unconsumedgateslifecycle_blobwithblob::check_size(row.get::<_, i64>(2)?)?(line 197) before reading it into a Vec, butop_bytes(theoutpointcolumn, line 195) is read into a Vec with no length check — the same gap exists in the siblingload_statereader a few lines above (li... - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate — In thecore_instant_locksblock ofload_state,islock_blobis gated withblob::check_size(row.get::<_, i64>(1)?)?(line 372) before materialization, buttxid_bytes(column 0) is read directly into a Vec (line 371) with no length gate beforedashcore::Txid::from_slice(&txid_bytes)is c... - [SUGGESTION]
packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has —decode_cmx_array(lines 2180-2190+) explicitly guardslenagainstisize::MAXbefore callingfrom_raw_parts, perfrom_raw_parts's documented safety requirement.slice_from_rawcallsslice::from_raw_parts(ptr, len)directly with only a null/zero check, no upper-bound check onlen. `... - [SUGGESTION]
packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures — Fixtures in this file buildClientWalletStartStatevalues but leaveidentity_manager,contacts,identity_keys, andunused_asset_locksat their default/empty values throughout. Given this PR wiresIdentityManager::apply_contacts_and_keysand asset-lock flattening into `load_from_persis... - [NITPICK]
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size — Every other reader in this PR routeslength(<col>)through the sharedblob::check_sizehelper (e.g. identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). This one reader open-codes the same logic (usize::try_from(...).unwrap_or(usize::MAX)+ inlineBlobTooLargeconstruction).... - [NITPICK]
packages/rs-platform-wallet/src/manager/load.rs:153-157: Flattening unused_asset_locks silently drops attribution on outpoint collisions —tracked_asset_locks.extend(account_locks)overwrites the outer key when the inner outpoint collides across accounts. In practice outpoints are globally unique andTrackedAssetLock.account_indexis already denormalized inside the value, so no data is actually lost — but the collision is silent... - [NITPICK]
packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper —KeyID::try_from(key_id)(i64 → KeyID) is done inline with a manually constructedWalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... }rather than going through thecrate::sqlite::util::safe_casthelpers used elsewhere in this file and sibling files. TheU64label...
🤖 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/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate
Every other BLOB read in this file gates the column with a `SELECT length(col), ...` + `blob::check_size()` pre-read before materializing the Vec, per the file's own stated discipline (comment at lines 299-302: "Pre-read length() gates ... before materializing the Vec so tampered oversize values are caught before heap allocation"). `load_used_addresses` — added by this PR (commit 6365be79) as the rehydration address-reuse guard — breaks that pattern: the prepared SQL is `SELECT DISTINCT script FROM core_utxos ...`, executed via `query_map`, with `row.get::<_, Vec<u8>>(0)` called directly with zero size check. A tampered/corrupted SQLite file with an oversized `script` blob forces an unbounded heap allocation per row before any validation runs — the same hazard class as the three previously-fixed findings in this exact file. Since this reader is invoked from `persister.rs:948` on every rehydration load, it re-opens a hole the earlier hardening commits (`pre-read BLOB size-gate on rehydration readers`) explicitly closed on the sibling readers.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate
In the `core_instant_locks` block of `load_state`, `islock_blob` is gated with `blob::check_size(row.get::<_, i64>(1)?)?` (line 372) before materialization, but `txid_bytes` (column 0) is read directly into a Vec (line 371) with no length gate before `dashcore::Txid::from_slice(&txid_bytes)` is called. `Txid` is a fixed 32-byte hash, so a tampered oversized `txid` column would still force an unbounded allocation before `from_slice` rejects the length. Add a `length(txid)` or `blob::check_fixed_width` gate before materializing `txid_bytes`, consistent with the fixed-width pattern used for other 32-byte identifiers in this PR (platform_addrs.rs).
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay
The doc comment states this is "the recommended entry point for startup hydration *after* late-registered accounts (e.g. DashPay contact accounts that `bootstrap_dashpay_contact_accounts` adds) have landed" and that "a second call after account bootstrap picks up the rest without regressing anything." The implementation destructures `ClientStartState` and immediately discards `wallets: _` (which carries `core_state`, `identity_manager`, `contacts`, `identity_keys`, `unused_asset_locks` per `ClientWalletStartState`), only re-applying `platform_addresses`. If the intent is that late-registered accounts' core/identity state should be re-hydrated on this second call, that never happens — only platform-address state is reloaded. `ClientStartState`/`ClientWalletStartState` are new types introduced by this PR, so the doc/impl divergence originates here.
In `packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch
On the existing-identity branch (lines 48-69), scalar fields are updated in place but `existing.wallet_id` is never reassigned and no bucket move between `out_of_wallet_identities` and `wallet_identities[wallet_id]` (with `location_index` update) is performed — unlike the fresh-insert path (lines 84-140), which correctly sets `wallet_id` and calls `location_index_insert`. `IdentityChangeSet::merge` (changeset.rs:465) does set `existing.wallet_id = entry.wallet_id` on merge, and `managed.wallet_id` is mutated on already-managed identities in discovery.rs:275, registration.rs:296, and loading.rs:251. An identity discovered out-of-wallet and later associated with a wallet can end up with a changeset `wallet_id` that never gets replayed into the correct bucket on restart, leaving it stuck in `out_of_wallet_identities` after a reload even though its live in-memory state (before persistence) had it correctly bucketed. Either narrow the doc comment to explicitly scope out wallet_id/bucket migration on this branch, or add the migration logic mirroring the fresh-insert path.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate
`load_unconsumed` gates `lifecycle_blob` with `blob::check_size(row.get::<_, i64>(2)?)?` (line 197) before reading it into a Vec, but `op_bytes` (the `outpoint` column, line 195) is read into a Vec with no length check — the same gap exists in the sibling `load_state` reader a few lines above (line 167). Outpoints are fixed-width (36 bytes), so this is a smaller allocation-size hazard than the blocking finding above, but it breaks the file's stated pre-read length-gate discipline and a tampered row could still force a larger-than-expected allocation before `decode_row` rejects it. Same class of fix as the blocking finding: add a `length(outpoint)` gate (or a fixed-width check).
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has
`decode_cmx_array` (lines 2180-2190+) explicitly guards `len` against `isize::MAX` before calling `from_raw_parts`, per `from_raw_parts`'s documented safety requirement. `slice_from_raw` calls `slice::from_raw_parts(ptr, len)` directly with only a null/zero check, no upper-bound check on `len`. `len` is host-supplied (Swift-side), so this is lower risk than a fully adversarial-DB path, but it's an inconsistency with the hardening applied to the sibling helper in the same file. Add the same `isize::MAX` bound check.
In `packages/rs-platform-wallet/tests/rehydration_load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures
Fixtures in this file build `ClientWalletStartState` values but leave `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` at their default/empty values throughout. Given this PR wires `IdentityManager::apply_contacts_and_keys` and asset-lock flattening into `load_from_persistor` (load.rs:167-177, 154-157), there's no integration-level coverage confirming identities, contacts, keys, or asset locks actually survive a full `load_from_persistor` round trip — only the lower-level unit tests in rehydrate.rs and apply.rs cover these paths in isolation. Add at least one integration fixture that populates these fields and asserts they're correctly present after `load_from_persistor`.
Note: GitHub refused the PR diff for inline mapping (PullRequest.diff too_large), so this review is posted body-only while preserving the verified findings.
There was a problem hiding this comment.
Code Review
Cumulative pass at fa6a031. Carried-forward prior findings: all 10 findings from 12979b5 are STILL VALID by direct re-inspection and are included below; none of the files carrying them were touched by the latest delta. New latest-delta finding: one nitpick in rehydrate.rs where a doc comment still describes CoinJoin as single-pool even though this delta's own test asserts CoinJoin now carries both External and Internal pools.
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 4 nitpick(s)
Verified findings (11)
These findings are body-only because GitHub refuses the PR diff (PullRequest.diff too_large). Prior findings marked STILL VALID were re-checked against fa6a031c and intentionally carried forward.
- [BLOCKING]
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate — Verified STILL VALID at fa6a031 — file untouched by this delta. Every other BLOB reader in this same file pre-readslength(col)and callsblob::check_size()before materializing the Vec (see the utxo/record/islock/chain_lock readers at lines 305–316, 355, 372, 396), and the file's own comment at lines 299–302 states this discipline explicitly.load_used_addressesbreaks it:SELECT DISTINCT script FROM core_utxos ...executed viaquery_mapcallsrow.get::<_, Vec<u8>>(0)directly with zero size check. Since this reader is invoked frompersister.rson every rehydration load, a tampered/corrupted SQLite file with an oversizedscriptblob forces an unbounded heap allocation per row before any validation runs — re-opening the exact hazard class the sibling readers were previously hardened against. - [SUGGESTION]
packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay — Verified STILL VALID at fa6a031 — file untouched by this delta. The doc comment (lines 1037–1044) advertises this as the entry point for hydration "after late-registered accounts ... have landed" where "a second call after account bootstrap picks up the rest without regressing anything." The implementation destructuresClientStartStateand immediately discardswallets: _at line 1050 — the field that carriescore_state,identity_manager,contacts,identity_keys, andunused_asset_locks— only re-applyingplatform_addresses. Either narrow the doc comment to platform-address re-hydration only, or extend the function to also apply the reloadedwalletsstate for the currentwallet_id. - [SUGGESTION]
packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch — Verified STILL VALID at fa6a031 — file untouched by this delta. The existing-identity branch (lines 48–69) updates scalar fields in place but never reassignsexisting.wallet_idand never moves the identity betweenout_of_wallet_identitiesandwallet_identities[wallet_id](withlocation_indexupdate), unlike the fresh-insert path at lines 84–140.IdentityChangeSet::mergedoes setexisting.wallet_id = entry.wallet_idon merge, andmanaged.wallet_idis mutated on already-managed identities in discovery.rs / registration.rs / loading.rs. An identity discovered out-of-wallet and later associated with a wallet can therefore end up with a changesetwallet_idthat never gets replayed into the correct bucket on restart. - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate — Verified STILL VALID at fa6a031 — file untouched by this delta.lifecycle_blobis gated withblob::check_size(row.get::<_, i64>(2)?)?at line 197, butop_bytes(the outpoint column, line 195) is materialized into a Vec with no length check. The siblingload_statereader (line 167) has the same gap. Outpoints are fixed-width (36 bytes), so the allocation-size hazard is smaller than the blocking finding above, but it still breaks the file's stated pre-read length-gate discipline. Add alength(outpoint)gate or a fixed-width check before materializing. - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate — Verified STILL VALID at fa6a031 — file untouched by this delta.islock_blobis gated withblob::check_sizeat line 372, buttxid_bytes(column 0, line 371) is read directly into a Vec with no length gate beforedashcore::Txid::from_slice(&txid_bytes)is called.Txidis a fixed 32-byte hash, so a tampered oversizedtxidcolumn would still force an unbounded allocation beforefrom_slicerejects the length. Add alength(txid)or fixed-width gate consistent with the pattern used elsewhere in the PR. - [SUGGESTION]
packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has — Verified STILL VALID at fa6a031 — file untouched by this delta.slice_from_rawcallsslice::from_raw_parts(ptr, len)with only a null/zero check. The siblingdecode_cmx_arrayin the same file explicitly guardslenagainstisize::MAXperfrom_raw_parts's documented safety requirement.lenis host-supplied (Swift-side over FFI), so exploitability requires a malicious/buggy host rather than adversarial DB, but the asymmetry with the sibling helper at the same FFI boundary should be closed. - [SUGGESTION]
packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures — Verified STILL VALID at fa6a031 — file untouched by this delta. EveryClientWalletStartStatefixture (lines 73–76, 133–136, 284–287, 351–354) leavesidentity_manager,contacts,identity_keys, andunused_asset_locksatDefault::default(). Given this PR wiresIdentityManager::apply_contacts_and_keysand asset-lock flattening intoload_from_persistor(load.rs:167–177, 154–157), there's no integration-level coverage confirming identities, contacts, keys, or asset locks survive a fullload_from_persistorround trip. Add at least one integration fixture that populates these fields and asserts they're correctly present afterload_from_persistor. - [NITPICK]
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size — Verified STILL VALID at fa6a031 — file untouched by this delta. This reader open-codesusize::try_from(...).unwrap_or(usize::MAX)+ inlineBlobTooLargeconstruction, while every other reader in the PR routes throughblob::check_size(identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). Functionally equivalent today, but any future tweak to the size-gate semantics has to touch two paths. Route through the shared helper for consistency. - [NITPICK]
packages/rs-platform-wallet/src/manager/load.rs:153-157: Flattening unused_asset_locks silently drops attribution on outpoint collisions — Verified STILL VALID at fa6a031 — file untouched by this delta.tracked_asset_locks.extend(account_locks)overwrites the outer key on outpoint collisions across accounts. Outpoints are globally unique in practice andTrackedAssetLock.account_indexis denormalized inside the value, so no real data loss — but the collision is silent. Adebug_assert!on duplicate insertion would keep the invariant honest without changing release behavior. - [NITPICK]
packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper — Verified STILL VALID at fa6a031 — file untouched by this delta.KeyID::try_from(key_id)(i64 → KeyID) is done inline with a manually constructedWalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... }rather than going through thecrate::sqlite::util::safe_casthelpers used elsewhere in this file and sibling files. TheU64label is also questionable ifKeyIDis narrower than u64. Cosmetic/consistency issue only —KeyID::try_fromitself still bounds-checks. - [NITPICK]
packages/rs-platform-wallet/src/manager/rehydrate.rs:301-308: extend_pools_for_restored_addresses doc contradicts this delta's own CoinJoin test on pool topology — New in this delta. The rust-dashcore 0.45 bump changed CoinJoin's account topology: the delta's own test at rehydrate.rs:912–917 states "CoinJoin accounts carry both an External and an Internal pool (mirroringStandard)" and the fixture at line 962 / 1032 unwraps anExternalpool from a CoinJoin account. But the production doc comment onextend_pools_for_restored_addresses(line 302) still reads "CoinJoin topology (single External pool)". The function's logic itself is topology-agnostic (fully positional, fail-closed onpool_typemismatch), so there's no functional bug — but the doc is now stale and can mislead a future reader about CoinJoin's actual pool count.
🤖 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.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate
Verified STILL VALID at fa6a031c — file untouched by this delta. Every other BLOB reader in this same file pre-reads `length(col)` and calls `blob::check_size()` before materializing the Vec (see the utxo/record/islock/chain_lock readers at lines 305–316, 355, 372, 396), and the file's own comment at lines 299–302 states this discipline explicitly. `load_used_addresses` breaks it: `SELECT DISTINCT script FROM core_utxos ...` executed via `query_map` calls `row.get::<_, Vec<u8>>(0)` directly with zero size check. Since this reader is invoked from `persister.rs` on every rehydration load, a tampered/corrupted SQLite file with an oversized `script` blob forces an unbounded heap allocation per row before any validation runs — re-opening the exact hazard class the sibling readers were previously hardened against.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded core/identity state despite doc claiming full late-account replay
Verified STILL VALID at fa6a031c — file untouched by this delta. The doc comment (lines 1037–1044) advertises this as the entry point for hydration "*after* late-registered accounts ... have landed" where "a second call after account bootstrap picks up the rest without regressing anything." The implementation destructures `ClientStartState` and immediately discards `wallets: _` at line 1050 — the field that carries `core_state`, `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` — only re-applying `platform_addresses`. Either narrow the doc comment to platform-address re-hydration only, or extend the function to also apply the reloaded `wallets` state for the current `wallet_id`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch
Verified STILL VALID at fa6a031c — file untouched by this delta. The existing-identity branch (lines 48–69) updates scalar fields in place but never reassigns `existing.wallet_id` and never moves the identity between `out_of_wallet_identities` and `wallet_identities[wallet_id]` (with `location_index` update), unlike the fresh-insert path at lines 84–140. `IdentityChangeSet::merge` does set `existing.wallet_id = entry.wallet_id` on merge, and `managed.wallet_id` is mutated on already-managed identities in discovery.rs / registration.rs / loading.rs. An identity discovered out-of-wallet and later associated with a wallet can therefore end up with a changeset `wallet_id` that never gets replayed into the correct bucket on restart.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate
Verified STILL VALID at fa6a031c — file untouched by this delta. `lifecycle_blob` is gated with `blob::check_size(row.get::<_, i64>(2)?)?` at line 197, but `op_bytes` (the outpoint column, line 195) is materialized into a Vec with no length check. The sibling `load_state` reader (line 167) has the same gap. Outpoints are fixed-width (36 bytes), so the allocation-size hazard is smaller than the blocking finding above, but it still breaks the file's stated pre-read length-gate discipline. Add a `length(outpoint)` gate or a fixed-width check before materializing.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate
Verified STILL VALID at fa6a031c — file untouched by this delta. `islock_blob` is gated with `blob::check_size` at line 372, but `txid_bytes` (column 0, line 371) is read directly into a Vec with no length gate before `dashcore::Txid::from_slice(&txid_bytes)` is called. `Txid` is a fixed 32-byte hash, so a tampered oversized `txid` column would still force an unbounded allocation before `from_slice` rejects the length. Add a `length(txid)` or fixed-width gate consistent with the pattern used elsewhere in the PR.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:4019-4025: slice_from_raw lacks the isize::MAX overflow guard its sibling helper has
Verified STILL VALID at fa6a031c — file untouched by this delta. `slice_from_raw` calls `slice::from_raw_parts(ptr, len)` with only a null/zero check. The sibling `decode_cmx_array` in the same file explicitly guards `len` against `isize::MAX` per `from_raw_parts`'s documented safety requirement. `len` is host-supplied (Swift-side over FFI), so exploitability requires a malicious/buggy host rather than adversarial DB, but the asymmetry with the sibling helper at the same FFI boundary should be closed.
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-356: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures
Verified STILL VALID at fa6a031c — file untouched by this delta. Every `ClientWalletStartState` fixture (lines 73–76, 133–136, 284–287, 351–354) leaves `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` at `Default::default()`. Given this PR wires `IdentityManager::apply_contacts_and_keys` and asset-lock flattening into `load_from_persistor` (load.rs:167–177, 154–157), there's no integration-level coverage confirming identities, contacts, keys, or asset locks survive a full `load_from_persistor` round trip. Add at least one integration fixture that populates these fields and asserts they're correctly present after `load_from_persistor`.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size
Verified STILL VALID at fa6a031c — file untouched by this delta. This reader open-codes `usize::try_from(...).unwrap_or(usize::MAX)` + inline `BlobTooLarge` construction, while every other reader in the PR routes through `blob::check_size` (identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). Functionally equivalent today, but any future tweak to the size-gate semantics has to touch two paths. Route through the shared helper for consistency.
- [NITPICK] packages/rs-platform-wallet/src/manager/load.rs:153-157: Flattening unused_asset_locks silently drops attribution on outpoint collisions
Verified STILL VALID at fa6a031c — file untouched by this delta. `tracked_asset_locks.extend(account_locks)` overwrites the outer key on outpoint collisions across accounts. Outpoints are globally unique in practice and `TrackedAssetLock.account_index` is denormalized inside the value, so no real data loss — but the collision is silent. A `debug_assert!` on duplicate insertion would keep the invariant honest without changing release behavior.
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper
Verified STILL VALID at fa6a031c — file untouched by this delta. `KeyID::try_from(key_id)` (i64 → KeyID) is done inline with a manually constructed `WalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... }` rather than going through the `crate::sqlite::util::safe_cast` helpers used elsewhere in this file and sibling files. The `U64` label is also questionable if `KeyID` is narrower than u64. Cosmetic/consistency issue only — `KeyID::try_from` itself still bounds-checks.
- [NITPICK] packages/rs-platform-wallet/src/manager/rehydrate.rs:301-308: extend_pools_for_restored_addresses doc contradicts this delta's own CoinJoin test on pool topology
New in this delta. The rust-dashcore 0.45 bump changed CoinJoin's account topology: the delta's own test at rehydrate.rs:912–917 states "CoinJoin accounts carry both an External and an Internal pool (mirroring `Standard`)" and the fixture at line 962 / 1032 unwraps an `External` pool from a CoinJoin account. But the production doc comment on `extend_pools_for_restored_addresses` (line 302) still reads "CoinJoin topology (single External pool)". The function's logic itself is topology-agnostic (fully positional, fail-closed on `pool_type` mismatch), so there's no functional bug — but the doc is now stale and can mislead a future reader about CoinJoin's actual pool count.
Note: Normal review_poster dry-run failed because GitHub refused the PR diff (PullRequest.diff too_large), so this is posted as a top-level exact-SHA review body.
Reviewed commit: fa6a031
lklimek
changed the base branch from
v4.1-dev
to
feat/platform-wallet-rehydration
…llet-storage-rehydration
There was a problem hiding this comment.
Code Review
Cumulative + incremental review at 0205123 reconciling all 11 prior findings from fa6a031. The latest delta (fa6a031→02051232) adds the wallet-info snapshot passthrough, snapshot mismatch handling, a mark/refill fixpoint correction, typed persister-load propagation, and Swift-side LoadOutcome handling. Carried-forward prior findings: all 11 prior findings are STILL_VALID at the current head and are included below; none were fixed, outdated, or intentionally deferred. New findings in the latest delta: one suggestion remains for snapshot_accounts_match_manifest, which compares account-type sets without comparing xpub material and can vacuously match all-excluded manifests. The existing blocking load_used_addresses BLOB-size gate issue still requires changes before merge.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 4 nitpick(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/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439: load_used_addresses reads core_utxos.script with no BLOB size gate
Verified STILL_VALID at head 020512323c — file untouched by the fa6a031c→02051232 delta. `load_used_addresses` is the only reader in this file that materializes a BLOB column without a prior `length()` + `blob::check_size` gate: `SELECT DISTINCT script FROM core_utxos ...` is fed to `query_map` and each row's `script` is read as `row.get::<_, Vec<u8>>(0)` directly (line 429). Every sibling reader in the same file follows the discipline documented at lines 299-302 — `load_state` for outpoint/script/record_blob/islock_blob/last_applied_chain_lock, `get_tx_record` at line 459 — all pre-read `length()` and pass through `blob::check_size` before allocating the Vec. `load_used_addresses` breaks the discipline: an oversized `script` blob in a tampered/corrupted store forces an unbounded heap allocation before rejection. Invoked by `persister.rs` on every rehydration load, so a malicious backup restore or hostile local file-system peer can force OOM at next launch. Fix is a mechanical port of the `load_state` pattern.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380: core_instant_locks.txid materialized with no length/fixed-width gate
Verified STILL_VALID at head — reader unchanged. `islock_blob` is properly gated at line 372 (`blob::check_size(row.get::<_, i64>(1)?)`), but `txid_bytes` at line 371 is read directly as `Vec<u8>` before any length check. `dashcore::Txid` is a fixed 32-byte hash, so an oversized `txid` column forces an unbounded allocation before `Txid::from_slice` rejects the length. Consistent with the file's stated discipline, pre-read `length(txid)` and pass through `blob::check_size` (or an `expect_fixed_size(32)` helper), matching the treatment of `islock_blob` on the same row.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1028-1064: load_and_apply_persisted discards reloaded wallet state despite doc claiming late-account replay
Verified STILL_VALID at head. The doc (lines 1037-1044) advertises this as the entry point for hydration *after* late-registered accounts (DashPay contact accounts from `bootstrap_dashpay_contact_accounts`) have landed, stating a second call after account bootstrap 'picks up the rest'. But the body destructures `ClientStartState { mut platform_addresses, wallets: _, skipped: _, .. }` at line 1050 and only re-applies `platform_addresses`. Everything under `wallets[self.wallet_id]` — `core_state` (UTXOs, sync watermarks), `identity_manager`, `contacts`, `identity_keys`, `unused_asset_locks`, and the new `core_wallet_info` snapshot — is discarded. Any state associated with an account only present after bootstrap (e.g. a DashPay contact account's UTXOs or identity keys) is silently not re-applied. With the delta adding a `core_wallet_info` snapshot, the divergence is bigger, not smaller. Either narrow the doc to platform-address re-hydration only, or extend the function to also apply the reloaded `wallets` state for `self.wallet_id`.
In `packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:42-69: apply_identity_entry never migrates wallet_id/bucket on the update branch
Verified STILL_VALID at head. On the existing-identity branch (lines 47-69) the code updates scalar fields in place through `existing`, but `existing.wallet_id` is never reassigned and no bucket move between `out_of_wallet_identities` and `wallet_identities[wallet_id]` is performed. The fresh-insert branch (lines 84-140) does place identities into the bucket named by `entry.wallet_id`. Since `IdentityChangeSet::merge` sets `wallet_id` on the entry when an out-of-wallet identity is promoted to wallet-owned, the next persister round-trip arrives with `wallet_id = Some(new_wallet_id)` and this update branch silently drops the association: after restart the identity stays in `out_of_wallet_identities` even though the persisted entry names its wallet. Fix: if `existing.wallet_id != entry.wallet_id`, remove the current bucket occupant and re-insert via the fresh-insert path to maintain the location-index invariant. In-scope because identity rehydration is one of the readers this PR wires up.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204: load_unconsumed materializes outpoint before any size gate
Verified STILL_VALID at head. `lifecycle_blob` is gated with `blob::check_size` at line 197, but `op_bytes` (the outpoint column, line 195) is read into `Vec<u8>` with no length gate. Outpoints are fixed-width (36 bytes), so a tampered oversized `outpoint` value can force an unbounded allocation before `blob::decode_outpoint` rejects it. Same discipline the file already applies to `lifecycle_blob` should apply here: add `length(outpoint)` to the SELECT and pass through `blob::check_size` before `row.get`. Same pattern in sibling `list_active` (lines 210-219).
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:3990-3996: slice_from_raw lacks the isize::MAX overflow guard its sibling decode_cmx_array has
Verified STILL_VALID at head — helper shifted from 4019-4025 to 3990-3996 by unrelated edits above it, but body is byte-identical. `slice::from_raw_parts(ptr, len)` is called with only a `ptr.is_null() || len == 0` guard, no upper bound. Per `slice::from_raw_parts`' safety contract, `len * size_of::<T>() <= isize::MAX` must hold — violating it is UB. Sibling `decode_cmx_array` in the same file (line 2180) explicitly guards `len > isize::MAX as usize` before `from_raw_parts` and documents it. `len` here is host-supplied (Swift passes it across FFI, e.g. via `rec.tx_bytes_len` at line 4042), so exploitability requires a malicious/buggy host, but the asymmetry with the sibling helper at the same FFI boundary is a genuine hole rather than intentional narrowing. Mirror the `decode_cmx_array` filter.
In `packages/rs-platform-wallet/tests/rehydration_load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/rehydration_load.rs:57-403: Rehydration integration tests never exercise identity_manager/contacts/identity_keys/unused_asset_locks fixtures
Verified STILL_VALID at head. The delta adds ~420 lines of new snapshot/error-path tests (`rt_snapshot_preserves_attribution_and_pools`, `rt_snapshot_wallet_id_mismatch_is_skipped`, `rt_snapshot_account_set_mismatch_is_skipped`, `rt_snapshot_mismatch_skip_coexists_with_healthy_load`, `rt_persister_load_transient_error_is_typed_and_retryable`, `rt_persister_load_permanent_error_is_typed_and_not_retryable`) but every fixture — including the new ones — still leaves `identity_manager`, `contacts`, `identity_keys`, and `unused_asset_locks` at `Default::default()`. The four per-area readers this PR wires up (identities, contacts, identity_keys, asset_locks unconsumed-only) have no end-to-end coverage through `load_from_persistor`. A regression that miswires `apply_contacts_and_keys` (load.rs:234) or the identity-manager rebuild would pass every existing test. Add at least one integration test that populates non-default values and asserts they round-trip through `load_from_persistor`.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:338-366: snapshot_accounts_match_manifest checks account-type set but never compares xpub material or asserts a non-empty comparable set
New in this delta. `snapshot_accounts_match_manifest` compares `manifest_types == snapshot_types` where both sides are `BTreeSet<AccountType>` — `AccountType` carries variant/index but no key material. The check validates wallet_id, network, and 'same set of account slots' but never validates that the carried `ManagedWalletInfo` snapshot's account xpubs equal the persisted `account_manifest`'s xpubs. Combined with the deliberate exclusion of `PlatformPayment`/`ProviderOperatorKeys`/`ProviderPlatformKeys`, this creates two residual gaps: (a) a tampered store that substitutes a different xpub for an account of the same type/index would pass all three structural gates while the addresses/balances shown to the user come from attacker-chosen keys (this is the same threat class the SnapshotIdentityMismatch mechanism exists to defend against); (b) if a manifest happens to contain only non-comparable types (`build_watch_only_wallet` only rejects a fully empty manifest, not an all-excluded one), `manifest_types` becomes an empty `BTreeSet` and the check degenerates to `{} == {}` — vacuous match. The PR docs explicitly track cryptographic manifest authentication as a follow-up; at minimum, adding a `debug_assert!` or explicit precondition that the comparable set is non-empty (or documenting why an all-excluded manifest cannot reach this function) closes the vacuous-match case without waiting for the larger MAC design.
Carried-forward prior nitpicks (4)
These prior findings are STILL_VALID at the current head and are included explicitly because this is a cumulative review.
- [NITPICK]
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size — Verified STILL_VALID at head 0205123 — file untouched by this delta. This reader open-codesusize::try_from(...).unwrap_or(usize::MAX)+ inlineBlobTooLargeconstruction, while every other reader in the PR routes throughblob::check_size(identity_keys.rs:159, accounts.rs:179, core_state.rs:311/315/355). Functionally equivalent today, but any future tweak to the size-gate semantics has to touch two paths. Route through the shared helper for consistency. - [NITPICK]
packages/rs-platform-wallet/src/manager/load.rs:215-219: Flattening unused_asset_locks silently drops attribution on outpoint collisions — Verified STILL_VALID at head — line-shifted from 153-157 to 215-219 by the snapshot-path insertion, but the code is unchanged:let mut tracked_asset_locks = BTreeMap::new(); for (_account_index, account_locks) in unused_asset_locks { tracked_asset_locks.extend(account_locks); }.extendon aBTreeMapsilently overwrites on key collision. Outpoints are globally unique in practice (TXID+vout collision requires SHA-256 preimages), so no real data loss is expected, but the code would silently prefer the last iteration's account's lifecycle blob if two accounts ever named the same outpoint (e.g. from a corrupted persister row set). Adebug_assert!(tracked_asset_locks.insert(outpoint, lock).is_none())-style guard would surface this in tests/debug builds without changing release behavior. - [NITPICK]
packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:165-169: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper — Verified STILL_VALID at head 0205123 — file untouched by this delta.KeyID::try_from(key_id)(i64 → KeyID) is done inline with a manually constructedWalletStorageError::IntegerOverflow { ..., target: SafeCastTarget::U64, ... }rather than going through thecrate::sqlite::util::safe_casthelpers used elsewhere in this file and sibling files. TheU64label is also questionable ifKeyIDis narrower than u64. Cosmetic/consistency issue only —KeyID::try_fromitself still bounds-checks. - [NITPICK]
packages/rs-platform-wallet/src/manager/rehydrate.rs:304-311: extend_pools_for_restored_addresses doc still says CoinJoin has 'single External pool' while its own test asserts dual-pool topology — Verified STILL_VALID at head (opus-security-auditor incorrectly claimed FIXED — direct read of the file at line 305 shows the doc still reads 'CoinJoin topology (single External pool)'). This function was directly rewritten in this delta (the mark↔refill fixpoint loop) but the stale doc block above it was not corrected. The PR's own test suite (rehydration_coinjoin_single_pool_deep_index, lines ~929-934) documents 'CoinJoin accounts carry both an External and an Internal pool (mirroring Standard)'. The function's logic is topology-agnostic (fully positional, fail-closed onpool_typemismatch), so no functional bug — but the doc is now stale and can mislead a future reader about the account's actual pool count. Trivial doc-only fix.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
| pub fn load_used_addresses( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| network: dashcore::Network, | ||
| ) -> Result<Vec<dashcore::Address>, WalletStorageError> { | ||
| let mut stmt = conn | ||
| .prepare("SELECT DISTINCT script FROM core_utxos WHERE wallet_id = ?1 ORDER BY script")?; | ||
| let rows = stmt.query_map(params![wallet_id.as_slice()], |row| { | ||
| row.get::<_, Vec<u8>>(0) | ||
| })?; | ||
| let mut out = Vec::new(); | ||
| for r in rows { | ||
| let script = dashcore::ScriptBuf::from_bytes(r?); | ||
| let address = dashcore::Address::from_script(&script, network) | ||
| .map_err(|_| WalletStorageError::blob_decode("core_utxos.script not an address"))?; | ||
| out.push(address); | ||
| } | ||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: load_used_addresses reads core_utxos.script with no BLOB size gate
Verified STILL_VALID at head 0205123 — file untouched by the fa6a031→02051232 delta. load_used_addresses is the only reader in this file that materializes a BLOB column without a prior length() + blob::check_size gate: SELECT DISTINCT script FROM core_utxos ... is fed to query_map and each row's script is read as row.get::<_, Vec<u8>>(0) directly (line 429). Every sibling reader in the same file follows the discipline documented at lines 299-302 — load_state for outpoint/script/record_blob/islock_blob/last_applied_chain_lock, get_tx_record at line 459 — all pre-read length() and pass through blob::check_size before allocating the Vec. load_used_addresses breaks the discipline: an oversized script blob in a tampered/corrupted store forces an unbounded heap allocation before rejection. Invoked by persister.rs on every rehydration load, so a malicious backup restore or hostile local file-system peer can force OOM at next launch. Fix is a mechanical port of the load_state pattern.
| pub fn load_used_addresses( | |
| conn: &Connection, | |
| wallet_id: &WalletId, | |
| network: dashcore::Network, | |
| ) -> Result<Vec<dashcore::Address>, WalletStorageError> { | |
| let mut stmt = conn | |
| .prepare("SELECT DISTINCT script FROM core_utxos WHERE wallet_id = ?1 ORDER BY script")?; | |
| let rows = stmt.query_map(params![wallet_id.as_slice()], |row| { | |
| row.get::<_, Vec<u8>>(0) | |
| })?; | |
| let mut out = Vec::new(); | |
| for r in rows { | |
| let script = dashcore::ScriptBuf::from_bytes(r?); | |
| let address = dashcore::Address::from_script(&script, network) | |
| .map_err(|_| WalletStorageError::blob_decode("core_utxos.script not an address"))?; | |
| out.push(address); | |
| } | |
| Ok(out) | |
| } | |
| pub fn load_used_addresses( | |
| conn: &Connection, | |
| wallet_id: &WalletId, | |
| network: dashcore::Network, | |
| ) -> Result<Vec<dashcore::Address>, WalletStorageError> { | |
| let mut stmt = conn.prepare( | |
| "SELECT DISTINCT length(script), script FROM core_utxos WHERE wallet_id = ?1 ORDER BY script", | |
| )?; | |
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | |
| let mut out = Vec::new(); | |
| while let Some(row) = rows.next()? { | |
| blob::check_size(row.get::<_, i64>(0)?)?; | |
| let script_bytes: Vec<u8> = row.get(1)?; | |
| let script = dashcore::ScriptBuf::from_bytes(script_bytes); | |
| let address = dashcore::Address::from_script(&script, network) | |
| .map_err(|_| WalletStorageError::blob_decode("core_utxos.script not an address"))?; | |
| out.push(address); | |
| } | |
| Ok(out) | |
| } |
source: ['claude']
| pub fn load_unconsumed( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<AssetLocksByAccount, WalletStorageError> { | ||
| let mut stmt = conn.prepare( | ||
| "SELECT outpoint, account_index, length(lifecycle_blob), lifecycle_blob, status \ | ||
| FROM asset_locks WHERE wallet_id = ?1 AND status NOT IN ('consumed')", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| let mut out: AssetLocksByAccount = BTreeMap::new(); | ||
| for r in rows { | ||
| let (op_bytes, account_index, blob_bytes) = r?; | ||
| let (acct, outpoint, tracked) = decode_row(&op_bytes, account_index, &blob_bytes)?; | ||
| while let Some(row) = rows.next()? { | ||
| let op_bytes: Vec<u8> = row.get(0)?; | ||
| let account_index: i64 = row.get(1)?; | ||
| blob::check_size(row.get::<_, i64>(2)?)?; | ||
| let blob_bytes: Vec<u8> = row.get(3)?; | ||
| let status: String = row.get(4)?; | ||
| let (acct, outpoint, tracked) = decode_row(&op_bytes, account_index, &blob_bytes, &status)?; | ||
| out.entry(acct).or_default().insert(outpoint, tracked); | ||
| } | ||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: load_unconsumed materializes outpoint before any size gate
Verified STILL_VALID at head. lifecycle_blob is gated with blob::check_size at line 197, but op_bytes (the outpoint column, line 195) is read into Vec<u8> with no length gate. Outpoints are fixed-width (36 bytes), so a tampered oversized outpoint value can force an unbounded allocation before blob::decode_outpoint rejects it. Same discipline the file already applies to lifecycle_blob should apply here: add length(outpoint) to the SELECT and pass through blob::check_size before row.get. Same pattern in sibling list_active (lines 210-219).
source: ['claude']
| { | ||
| // Same pre-read length gate as `record_blob` above. | ||
| let mut stmt = conn.prepare( | ||
| "SELECT txid, length(islock_blob), islock_blob \ | ||
| FROM core_instant_locks WHERE wallet_id = ?1", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| use dashcore::hashes::Hash; | ||
| let txid_bytes: Vec<u8> = row.get(0)?; | ||
| blob::check_size(row.get::<_, i64>(1)?)?; | ||
| let blob_bytes: Vec<u8> = row.get(2)?; | ||
| let txid = dashcore::Txid::from_slice(&txid_bytes) | ||
| .map_err(|_| WalletStorageError::blob_decode("core_instant_locks.txid"))?; | ||
| let islock: dashcore::ephemerealdata::instant_lock::InstantLock = | ||
| blob::decode(&blob_bytes)?; | ||
| cs.instant_locks_for_non_final_records.insert(txid, islock); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: core_instant_locks.txid materialized with no length/fixed-width gate
Verified STILL_VALID at head — reader unchanged. islock_blob is properly gated at line 372 (blob::check_size(row.get::<_, i64>(1)?)), but txid_bytes at line 371 is read directly as Vec<u8> before any length check. dashcore::Txid is a fixed 32-byte hash, so an oversized txid column forces an unbounded allocation before Txid::from_slice rejects the length. Consistent with the file's stated discipline, pre-read length(txid) and pass through blob::check_size (or an expect_fixed_size(32) helper), matching the treatment of islock_blob on the same row.
source: ['claude']
…and_keys Domain-side routing that folds persisted PUBLIC keys + contact state onto the already-built managed identities so `Identity.public_keys` and the contact maps are populated at load time (pre-keyed shape), across BOTH the wallet and out-of-wallet buckets. Orphan entries (owner not loaded) are logged and skipped; `removed_*` are ignored (rehydration feed is insert-only); no `Network` needed. Reuse the existing DPP `IdentityGettersV0::add_public_key` accessor for the key insert and refactor the replay path `apply_identity_key_entry` onto it, dropping the clone-the-whole-map idiom (one routing primitive, shared). Unit tests: key routed into wallet + out-of-wallet identities, contacts to the correct maps, no cross-identity leakage, orphan skip, empty no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add `schema::identities::load_prekeyed`, an orchestrator that reads the identities, then folds this wallet's persisted identity keys and contacts onto them via `merge_contacts_and_keys` — each ManagedIdentity leaves the loader already carrying its own public keys + contact state, so Platform signing works immediately post-load with no key sync. Rewire `persister.rs::load()` onto it and leave `ClientWalletStartState.contacts` / `.identity_keys` at `Default` (empty), matching the FFI persister: the manager's `apply_contacts_and_keys(empty, empty)` becomes a harmless no-op, so this PR is correct and mergeable in either order relative to #3692's field removal. Also set the required `core_wallet_info: None` (SQLite reconstructs core state from typed rows). Schema test: load_prekeyed populates keys across both identity buckets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dIdentity Rewrite sqlite_contacts_keys_rehydration.rs to the pre-keyed shape, asserting on `ManagedIdentity.identity.public_keys()` / `.established_contacts` / `.sent_contact_requests` / `.incoming_contact_requests` after store→drop→reopen→load, per the #3968 test spec TC-1..TC-10: - TC-1 keys populate public_keys bit-exact, no sync - TC-2 (load-bearing) freshly-loaded AUTH/CRITICAL key is selectable via the exact `get_first_public_key_matching` predicate the signing path uses - TC-3 established contact restores; TC-4 sent/incoming directionality - TC-5/6 no cross-identity key/contact leakage (same KeyID, two identities) - TC-7 zero keys, TC-8 out-of-wallet bucket, TC-9 tombstoned-orphan rows don't crash the join, TC-10 cross-wallet scoping preserved Re-point sqlite_load_reconstruction.rs::tc043 onto the managed identity (the field it asserted is now empty by design). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the fabricated "keys before contacts" ordering rationale — the key and contact maps are independent (established-contact routing never reads public_keys), so the ordering carries no invariant. Trim the block under the public-API rustdoc line cap. Docs only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…entity_keys reader identity_keys::load_state now verifies the decoded blob's inner `IdentityPublicKey.id()` against the typed `key_id` column. That inner id becomes the DPP signing-selection map key via `add_public_key`, so a locally-tampered row could otherwise file a key under a mismatched KeyID. Hard-errors as `IdentityKeyEntryMismatch`, matching the reader's existing column-vs-blob cross-checks for identity_id / key_id / wallet_id. +1 rejection test. (Smythe LOW S1 review follow-up.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sync the prekeyed identity_keys + contacts join (#3968) onto #3692's final post-v4.1-dev state (incl. rust-dashcore 647fa98). Reconciliations: - Assemble the now-required core_wallet_info in persister::load(): build_watch_only_wallet + ManagedWalletInfo::from_wallet + apply_persisted_core_state, revived as a shared pub platform_wallet::rehydrate module and adapted to the post-#3841 sealed DashPay contact model. Metadata-only wallets (empty manifest) rebuild as empty watch-only wallets rather than being dropped. - Reconcile the wallet-table rename: FK refs wallet_metadata -> wallets in V001 for pending_contact_crypto + ignored_senders; module wallet_meta -> wallets. - Restore #3968's length(address) pre-read gate on all_address_rows that the merge dropped when adopting #3692's as_of_height column; re-add blob::check_fixed_width. - Contact routing rewritten onto the sealed ManagedIdentity accessors / apply_* mutators; identity_keys + contacts fields removed from ClientWalletStartState. - Defer instant-send-lock set and tx-record rehydration with TODOs (no public key_wallet setter / injection API; both re-warm on next sync, no regression vs. the prior loader). - Per-account UTXO attribution left approximated to account 0 with a TODO(#3986) marker for the real core_address_pool attribution. - Update integration tests for the sealed-contact API, removed fields, and #3692's load_all raw-count contract. Known-red (out of scope, tracked separately): 3 asset-lock coin-selection tests fail with "No UTXOs available for selection" — confirmed rust-dashcore 647fa98 upstream regression, not introduced by this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The old comment claimed a metadata-only wallet "still appears with a zero balance rather than being dropped." It does not: manager/load.rs re-checks the empty manifest and skips the wallet as MissingManifest one layer up (proved by rt_corrupt_row_skipped_and_other_loads). The persister-layer placeholder exists only so one unregistered wallet's empty manifest doesn't abort load() for every other wallet via `?` propagation. Doc-only; no logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at 04b93e4 after prior SHA 0205123. Prior-6 (slice_from_raw isize::MAX guard) is FIXED with a regression test. The remaining eleven prior findings are STILL VALID at current head and are carried forward (some line-shifted; prior-7 is narrowed to the still-untested unused_asset_locks fixture path). Two new findings were verified in the latest delta: contacts::load_state now materializes three request BLOBs without a pre-allocation size gate, and newly added typed FFI restore helpers bypass the typed slice overflow guard discipline. Canonical count after enforcing carry-forward policy: 2 blocking, 7 suggestions, 4 nitpicks.
Reviewed commit: 04b93e49 (04b93e491632a3515d492ed6a36e4a59e41ee625); prior reviewed commit: 02051232.
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 | 7 suggestion(s) | 4 nitpick(s)
Prior Finding Reconciliation
- STILL VALID / carried forward: prior-1
load_used_addressesscript BLOB gate. - STILL VALID / carried forward: prior-2
load_and_apply_persisteddiscards reloaded wallet state. - STILL VALID / carried forward: prior-3
apply_identity_entrydoes not migrate wallet buckets on update. - STILL VALID / carried forward: prior-4
asset_locks::load_unconsumedoutpoint materialization before gate. - STILL VALID / carried forward: prior-5
core_instant_locks.txidmaterialization before fixed-width gate. - FIXED: prior-6
slice_from_rawmissingisize::MAXguard now has the guard and regression test; a new typed-slice variant of the same issue is listed under latest-delta findings. - STILL VALID / carried forward, narrowed: prior-7 rehydration tests now cover identity manager/key selection, but still do not exercise non-default
unused_asset_locksround-trip. - STILL VALID / carried forward: prior-8 snapshot manifest comparison still checks account slot types, not xpub bytes.
- STILL VALID / carried forward: prior-9 open-coded blob-size gate in
all_platform_payment_registrations. - STILL VALID / carried forward: prior-10 unused asset-lock flattening silently overwrites on outpoint collisions.
- STILL VALID / carried forward: prior-11 manual
key_idcast error label/helper drift. - STILL VALID / carried forward: prior-12 stale CoinJoin topology doc.
New Findings In Latest Delta
BLOCKING: contacts::load_state materializes three request BLOBs with no size gate
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:293-338
New in the delta's unified contacts table reader. The SELECT at lines 293–297 pulls outgoing_request, incoming_request, and accepted_accounts as raw columns, and lines 303–304 (Sent/Received branches) and 334 (Established branch) read them into Option<Vec<u8>> via row.get() with no length() pre-read and no blob::check_size call. blob::decode (line 336) only enforces the 16 MiB bincode cap after the Vec has already been allocated. This reader is consumed by identities::load_prekeyed on every normal rehydration, so a corrupted or hostile SQLite store (e.g. malicious backup restore) can force unbounded heap allocations on wallet startup and DoS the wallet UI before decode can reject the row. This breaks the file-wide discipline every sibling reader (core_state::load_state, asset_locks::load_unconsumed for lifecycle_blob, identity_keys::load_all for public_key_blob, etc.) enforces. Pre-read length(outgoing_request), length(incoming_request), and length(accepted_accounts) in the SELECT and gate each with blob::check_size before the row.get::<_, Option<Vec<u8>>>() call.
Source attribution: codex
SUGGESTION: New restore_dashpay_* / build_identity_public_keys helpers bypass the isize::MAX guard prior-6 just installed on slice_from_raw
packages/rs-platform-wallet-ffi/src/persistence.rs:3840-4221
The delta lands prior-6's fix on slice_from_raw (guard at 4436–4442 plus slice_from_raw_rejects_overflowing_len regression test at 4638), but introduces five new call sites that hand a Swift-supplied count straight to slice::from_raw_parts on *const T for repr(C) struct T, guarded only against null/zero:
restore_dashpay_paymentsat 3844 overPaymentRestoreEntryFFIrestore_dashpay_ignoredat 3868 over[u8; 32]restore_contact_profilesat 3960 overContactProfileRestoreEntryFFIrestore_dashpay_contactsat 4064 overContactRequestFFI(plus two byte/u32inner slices at 4111 and 4118)build_identity_public_keysat 4221 overIdentityKeyRestoreFFI
slice::from_raw_parts::<T> requires count * size_of::<T>() <= isize::MAX; a _count in the range (isize::MAX/size_of::<T>() .. isize::MAX] satisfies prior-6's byte-slice guard but is UB for typed arrays. This is exactly the fail-closed convention prior-6 established, and the new call sites this PR adds are the right place to extend it before the pattern spreads further. Fix: generalize slice_from_raw to a typed helper (slice_from_raw_typed<T>) that checks count.checked_mul(mem::size_of::<T>()).is_some_and(|n| n <= isize::MAX as usize) and route all five sites through it.
Source attribution: claude, codex
Carried-Forward Prior Findings
BLOCKING: load_used_addresses reads core_utxos.script with no BLOB size gate
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:421-439
Verified at 04b93e4. load_used_addresses is the only reader in this file that materializes a Vec<u8> without gating on length(col) + blob::check_size first — the pattern every neighbor (load_state, get_tx_record, core_instant_locks block) enforces per the file's own discipline comment. SELECT DISTINCT script FROM core_utxos … is executed and row.get::<_, Vec<u8>>(0) at line 429 allocates unconditionally. Called on every rehydration load (address-reuse guard reconstruction), so a tampered/corrupted store with an oversized script column forces an unbounded heap allocation per row before ScriptBuf/Address::from_script can reject it. Carried forward from prior-1 — flagged by all four specialist reviewers.
Source attribution: claude, codex
pub fn load_used_addresses(
conn: &Connection,
wallet_id: &WalletId,
network: dashcore::Network,
) -> Result<Vec<dashcore::Address>, WalletStorageError> {
let mut stmt = conn.prepare(
"SELECT DISTINCT length(script), script FROM core_utxos WHERE wallet_id = ?1 ORDER BY script",
)?;
let mut rows = stmt.query(params![wallet_id.as_slice()])?;
let mut out = Vec::new();
while let Some(row) = rows.next()? {
blob::check_size(row.get::<_, i64>(0)?)?;
let script_bytes: Vec<u8> = row.get(1)?;
let script = dashcore::ScriptBuf::from_bytes(script_bytes);
let address = dashcore::Address::from_script(&script, network)
.map_err(|_| WalletStorageError::blob_decode("core_utxos.script not an address"))?;
out.push(address);
}
Ok(out)
}
SUGGESTION: load_unconsumed materializes outpoint before any size gate
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204
Verified at 04b93e4 (file untouched by delta). The SELECT reads outpoint, account_index, length(lifecycle_blob), lifecycle_blob, status. lifecycle_blob IS gated with blob::check_size at line 197, but op_bytes (column 0, line 195) is read into Vec<u8> unconditionally. Outpoints are fixed-width 36 bytes, so a tampered oversized value has smaller blast radius than lifecycle blobs but still forces an allocation before decode_row's blob::decode_outpoint can reject it. Same gap exists in the sibling list_active reader. Delta added blob::check_fixed_width(len, expected, col); apply it here. Carried forward from prior-4 — flagged by all four reviewers.
Source attribution: claude, codex
SUGGESTION: core_instant_locks.txid materialized with no length/fixed-width gate
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:362-380
Verified at 04b93e4. In load_state's instant-lock block islock_blob is properly gated at line 372, but txid_bytes at line 371 is read directly as Vec<u8> before any length check. dashcore::Txid is a fixed 32-byte hash — an oversized txid column forces an unbounded allocation before Txid::from_slice (line 374) rejects it. Use the new blob::check_fixed_width(row.get::<_, i64>(?)?, 32, "core_instant_locks.txid") helper by adding length(txid) to the SELECT, matching the treatment islock_blob on the same row already gets. Carried forward from prior-5 — flagged by all four reviewers.
Source attribution: claude, codex
SUGGESTION: load_and_apply_persisted discards reloaded wallet state despite doc claiming late-account replay
packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1192-1211
Verified at 04b93e4 (line-shifted from 1028–1064). The doc (1179–1191) advertises this as the entry point for hydration after late-registered accounts (e.g. DashPay contact accounts from bootstrap_dashpay_contact_accounts) have landed and promises 'a second call after account bootstrap picks up the rest without regressing anything'. But the body destructures ClientStartState { mut platform_addresses, wallets: _, skipped: _, .. } and only re-applies platform_addresses.remove(&self.wallet_id). wallets: _ now discards core_wallet_info (UTXOs, per-account pools, balances), identity_manager (public keys, contacts, payments, contact profiles, ignored senders), and unused_asset_locks. Any state associated with an account only present after bootstrap silently fails to re-hydrate. Either narrow the doc to platform-address-only replay, or apply the reloaded wallets slice for self.wallet_id too. Carried forward from prior-2.
Source attribution: claude, codex
SUGGESTION: apply_identity_entry never migrates wallet_id / bucket on the update branch
packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:47-78
Verified at 04b93e4. The existing-identity branch (47–78) updates scalar fields (balance, revision, status, dashpay_profile, dpns_names, dashpay_payments, contact_profiles, ignored_senders) in place through existing but never reassigns existing.wallet_id and never moves the identity between out_of_wallet_identities and wallet_identities[wallet_id] — unlike the fresh-insert branch (93+). If an identity is first observed out-of-wallet and later gets a wallet association via apply_identity_entry with wallet_id = Some(new_wallet_id), it stays stuck in out_of_wallet_identities and the location_index_insert invariant breaks. The load path bypasses this via load_prekeyed + merge_contacts_and_keys, so today's exposure is limited to the runtime changeset-replay path — but that path exists (apply_contacts_and_keys etc.) and this is the generic replay hook. Fix: when existing.wallet_id != entry.wallet_id, remove the current bucket occupant and re-insert on the fresh-insert path. Carried forward from prior-3.
Source attribution: claude, codex
SUGGESTION: snapshot_accounts_match_manifest compares AccountType set but never verifies xpub bytes
packages/rs-platform-wallet/src/manager/load.rs:332-360
Verified at 04b93e4. snapshot_accounts_match_manifest compares manifest_types == snapshot_types as BTreeSet<AccountType> (with ProviderOperator/ProviderPlatform/PlatformPayment excluded via comparable). AccountType derives Ord/Eq over variant/index only — no key material. The check validates wallet_id + network + 'same slot set' but never checks that snapshot account xpubs match the manifest xpubs used to build the signing-capable watch-only wallet. A tampered snapshot with matching slots but substituted xpubs would pass. The doc now correctly labels this as a self-consistency check (not authenticity — that's tracked in #3992), which scopes the concern accurately, but the residual gap — accepting persister-provided data as consistent when it isn't — is worth closing by hashing the manifest's xpub set into the comparison. Carried forward from prior-8.
Source attribution: claude, codex
SUGGESTION: Rehydration integration tests still leave unused_asset_locks defaulted
packages/rs-platform-wallet/tests/rehydration_load.rs:76-82
The delta added end-to-end coverage of identity_manager restoration via rt_signing_key_selectable_immediately_after_load at 961–1005 — that portion of prior-7 is fixed. But every other ClientWalletStartState fixture in the file (including the shared slice() helper at 76 and the rollback_tests helper at load.rs:385) still passes unused_asset_locks: Default::default(). load_from_persistor's tracked_asset_locks.extend flattening (load.rs:198–202) has no manager-level round-trip coverage — a regression that miswires it would pass every test in this file. Add one integration test that populates non-default unused_asset_locks and asserts they round-trip onto the registered PlatformWalletManager. Carried forward from prior-7 (partially resolved).
Source attribution: claude, codex
NITPICK: extend_pools_for_restored_addresses doc still says CoinJoin has 'single External pool' while its own test asserts dual-pool topology
packages/rs-platform-wallet/src/manager/rehydrate.rs:327-329
Verified at 04b93e4 (line-shifted from 304–307). Doc still says Tested with … CoinJoin topology (single External pool). The delta's own regression test rehydration_coinjoin_single_pool_deep_index explicitly documents CoinJoin as dual-pool ('CoinJoin accounts carry both an External and an Internal pool (mirroring Standard)'). Function logic is topology-agnostic, so no behavioural bug — but stale doc that could mislead future audits. Carried forward from prior-12.
Source attribution: claude, codex
/// Tested with Standard BIP44 topology (External + Internal pools) and
/// CoinJoin topology (External + Internal pools, mirroring Standard). The
/// per-chain probe loop has no topology-specific branches, so the
/// non-hardened single-pool type (`Absent`) follows the same code path
/// with a different relative derivation path. `AbsentHardened` pools
/// cannot be derived from a public xpub at all —
NITPICK: all_platform_payment_registrations open-codes the blob-size gate instead of using blob::check_size
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:87-95
Carried forward from prior-9 and verified STILL VALID at 04b93e4. all_platform_payment_registrations still open-codes usize::try_from(...).unwrap_or(usize::MAX) plus inline BlobTooLarge construction at lines 87-95 instead of routing through blob::check_size, while neighboring readers use the shared helper. Functionally equivalent today, but it leaves this reader on a separate size-gate implementation path for future changes.
Source attribution: claude, prior-review
NITPICK: Flattening unused_asset_locks silently drops attribution on outpoint collisions
packages/rs-platform-wallet/src/manager/load.rs:198-202
Carried forward from prior-10 and verified STILL VALID at 04b93e4. The flattening code still builds tracked_asset_locks with tracked_asset_locks.extend(account_locks) at lines 198-202. BTreeMap::extend silently overwrites on key collision; outpoints should be globally unique, so this is defensive rather than expected data loss, but a corrupted persister row set with the same outpoint attributed to two accounts would silently keep the later lifecycle blob. A debug assertion around insert would surface that inconsistency in tests/debug builds.
Source attribution: claude, prior-review
NITPICK: key_id cast error uses SafeCastTarget::U64 label but doesn't use the safe_cast helper
packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:166-170
Carried forward from prior-11 and verified STILL VALID at 04b93e4. KeyID::try_from(key_id) is still mapped manually into WalletStorageError::IntegerOverflow { target: SafeCastTarget::U64, ... } at lines 166-170 instead of using the local safe-cast helper pattern. The conversion itself remains bounds-checked, so this is a consistency/error-label nit only.
Source attribution: claude, prior-review
Resolved prior findings
- slice_from_raw lacks the isize::MAX overflow guard its sibling helper has — FIXED at persistence.rs:4436–4442.
slice_from_rawnow checksptr.is_null() || len == 0 || len > isize::MAX as usizeand returns&[]in that case. Regression testslice_from_raw_rejects_overflowing_lenat line 4638 locks in the guard. Note: the discipline was not extended to five newly-added typed-slice restore sites — carried forward as a new finding above.
Dropped / out-of-scope candidates
- [codex-security NEW] contacts::load_state no longer verifies request parties against row keys — Hallucination.
git log -S check_request_partiesreturned zero matches across the entire repo history — the function this finding claims was 'removed' has never existed. The severity claim of fund-misdirection is unsupported. - [codex-ffi] existing mnemonic-resolver C symbol changed ABI (dash_sdk_sign_with_mnemonic_resolver_and_path) — Not a real ABI break in this monorepo. The Swift SDK bindings are built from the same commit as the Rust FFI (cbindgen regenerates the header per build); there is no scenario where a caller built against the previous header calls the new symbol. The in-tree Swift caller (KeychainSigner) was updated in the same delta. If maintainers add a stable-ABI contract, revisit.
- [codex-ffi] DashPay exported C functions reuse old symbol names with new parameters — Same reasoning as above — Swift bindings (ManagedPlatformWallet) were updated in the same delta and are co-built. No stable-ABI contract is claimed by this PR.
- [codex-ffi] ProtocolVersionUpgradeState WASM shape drops JS-visible properties — Out of scope for this PR (persistence readers + seedless load() wiring). If the WASM API removal is unintentional, it belongs in a separate WASM SDK review.
- [claude-security out-of-scope] AES-256-CBC without authentication in rs-platform-encryption — DIP-15 spec-level decision (cross-client interop with dashj/kotlin); cannot be changed in one client. Not a defect of this PR.
GitHub refused the PR diff API for this large PR during dry-run (PullRequest.diff too_large), so these verified findings are posted in the top-level review body instead of inline comments.
…on (#3986) Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Head 1a332ad fixes prior-1 (core_utxos.script now has a MAX(length) precheck), but introduces two new blocking issues in the latest delta. (1) The new V002__unified.rs migration collides with the pre-existing V002__address_height_pin.rs: at runtime refinery applies the first version-2 row then hits UNIQUE constraint failed: refinery_schema_history.version — verified by running cargo test -p platform-wallet-storage --lib v001_creates_dashpay_sync_schema, which panics with exactly that error and leaves 25 lib tests failing. (2) Four new/updated test files (fixture_gen.rs, sqlite_pool_reader.rs, sqlite_migration_execution.rs, sqlite_version_bump.rs) fail to compile because they still reference ClientWalletStartState::used_core_addresses (removed on the #3692 merge) and construct IdentityEntry / AddressFunds without the newly-required fields — cargo check -p platform-wallet-storage --tests produces 17 errors. Together these contradict the PR body's 509 passed / 0 failed claim. Carried forward: prior-2 (contacts blob gate) remains blocking; prior-3..prior-9 remain valid suggestions; prior-10..prior-13 nitpicks dropped for budget. Prior-1 is resolved.
Reviewed commit: 1a332ad4 (1a332ad4f25fe0d160564a9b2be59d4a3ad04fcc); prior reviewed commit: 04b93e49.
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
3 blocking | 7 suggestion(s) | 0 nitpick(s)
Prior Finding Reconciliation
- FIXED: prior-1 load_used_addresses script BLOB gate is now enforced with a MAX(length(script)) precheck plus blob::check_size before materializing scripts.
- STILL VALID / carried forward as blocking: prior-2 contacts::load_state still materializes outgoing_request, incoming_request, and accepted_accounts before any length gate.
- STILL VALID / carried forward as suggestion: prior-3 restore_dashpay_* / build_identity_public_keys still bypass typed-slice overflow guard discipline.
- STILL VALID / carried forward as suggestion: prior-4 asset_locks::load_unconsumed still materializes outpoint before any size gate.
- STILL VALID / carried forward as suggestion: prior-5 core_instant_locks still materializes txid before a fixed-width gate.
- STILL VALID / carried forward as suggestion: prior-6 load_and_apply_persisted still discards reloaded wallet state despite the late-account replay doc.
- STILL VALID / carried forward as suggestion: prior-7 apply_identity_entry update branch still does not migrate wallet_id / bucket placement.
- STILL VALID / carried forward as suggestion: prior-8 snapshot_accounts_match_manifest still compares account types without verifying xpub bytes.
- STILL VALID / carried forward as suggestion: prior-9 rehydration tests still leave unused_asset_locks defaulted, leaving asset-lock flattening untested.
- STILL VALID / budget-dropped nitpick: prior-10 extend_pools_for_restored_addresses doc still says CoinJoin has a single External pool.
- STILL VALID / budget-dropped nitpick: prior-11 all_platform_payment_registrations still open-codes the blob-size gate rather than using blob::check_size.
- STILL VALID / budget-dropped nitpick: prior-12 flattening unused_asset_locks still silently overwrites attribution on outpoint collisions.
- STILL VALID / budget-dropped nitpick: prior-13 identity_keys key_id cast error still uses the SafeCastTarget::U64 label without the local safe-cast helper pattern.
New Findings In Latest Delta
BLOCKING: V002__unified.rs collides with existing V002__address_height_pin.rs — refinery panics on UNIQUE constraint at first migration
packages/rs-platform-wallet-storage/migrations/V002__unified.rs:1-80
Both migrations/V002__unified.rs (new here) and migrations/V002__address_height_pin.rs exist and are picked up by refinery::embed_migrations!("./migrations") in src/sqlite/migrations.rs:13. Refinery accepts both at proc-macro time but at runtime attempts to insert two rows into refinery_schema_history with version = 2, violating that table's PRIMARY KEY. Verified by running cargo test -p platform-wallet-storage --lib v001_creates_dashpay_sync_schema, which panics at migrations.rs:280 with SqliteFailure(..., "UNIQUE constraint failed: refinery_schema_history.version") after applying V001 (name=initial) and V002 (name=unified) and then attempting to insert V002 (name=address_height_pin). Blast radius: every fresh open of a V001 store — including all downstream lib tests — fails during migration; 25 of 243 lib tests are failing at head purely because of this. Fold the address_height_pin DDL into V002__unified.rs, or bump unified to V003__ and update max_supported_version() / forward-version tests accordingly.
Source attribution: codex.
BLOCKING: Four new/updated test files do not compile after merge with #3692 (missing fields / removed used_core_addresses / non-exhaustive Domain match)
packages/rs-platform-wallet-storage/tests/sqlite_pool_reader.rs:1-335
cargo check -p platform-wallet-storage --tests at 1a332ad fails with 17 errors across four test files, contradicting the PR body's 509 passed / 0 failed claim — none of these suites (which include the PR's own regression coverage for the new core_pool reader and D0 schema-freeze) actually build or run.
tests/sqlite_pool_reader.rs: 9 ×E0609: no field 'used_core_addresses' on type 'ClientWalletStartState'at lines 95, 138, 145, 166, 208, 209, 252, 257, 332 plus 5 follow-onE0282type-annotation errors. Field was removed on the #3692 merge; the new pool-reader tests (tc_b_020,tc_b_023,tc_b_025,pre_pool_store_yields_utxo_derived_used_set,tc_b_026,mixed_store_unions_utxo_and_pool_used_sets) still read from it.tests/sqlite_migration_execution.rs: sameused_core_addresseserrors at lines 147 and 167 —tc_b_035_populated_fixture_migrates_and_readsandtc_b_036_empty_wallet_through_migrationdo not run.tests/fixture_gen.rs:E0063: missing fields 'contact_profiles' and 'ignored_senders' in initializer of 'IdentityEntry'at line 144. Blocks thefixture_genbinary and therefore prevents regeneratingpopulated_v001.db; the always-runpopulated_v001_fixture_is_present_and_openableguard is not exercised.tests/sqlite_version_bump.rs:E0063: missing field 'as_of_height' in initializer of 'AddressFunds'at line 115;E0063: missing fields 'contact_profiles' and 'ignored_senders' in initializer of 'IdentityEntry'at line 183. Additionally,single_domain_changesetmatchesDomainwith 12 arms (stops atDomain::AccountAddressPools) whileDomainhas 13 variants includingDomain::PendingContactCrypto(seeDomain::ALLin versions.rs). Once the E0063 errors are fixed,tc_b_013_every_domain_maps_and_isolatesand the other tests that iterateDomain::ALLwill still fail on non-exhaustive match.
Each test file needs its constructors updated with the new required fields, used_core_addresses reads replaced with the actual API ClientWalletStartState exposes post-merge (or a direct pool-table query), and the Domain match extended to cover PendingContactCrypto.
Source attribution: claude.
Carried-Forward Prior Findings Still Valid At Current Head
BLOCKING: contacts::load_state materializes outgoing_request/incoming_request/accepted_accounts with no length gate
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:293-338
Carried forward from prior-2; verified STILL VALID at 1a332ad. load_state selects outgoing_request, incoming_request, and accepted_accounts and reads them into Option<Vec<u8>> at lines 303, 304, and 334 without any length() pre-read or blob::check_size call. blob::decode only enforces the 16 MiB bincode cap after the Vec is already allocated, so a corrupted or hostile SQLite store forces unbounded heap allocation on every wallet rehydration path that goes through identities::load_prekeyed. This delta explicitly installed the MAX(length(col)) pre-read gate in core_state::load_used_addresses and the new core_pool::load_used_addresses; the same protection should be applied to the three BLOB columns in this reader (add SELECT MAX(length(outgoing_request)), MAX(length(incoming_request)), MAX(length(accepted_accounts)) and blob::check_size before the row loop).
Source attribution: claude, codex.
SUGGESTION: load_and_apply_persisted destructures `wallets: _` and silently discards reloaded wallet state despite the doc promising late-account replay
packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1192-1211
Carried forward from prior-6; STILL VALID at 1a332ad. The doc above load_and_apply_persisted advertises the function as the hydration entry point after late-registered accounts (e.g. DashPay contact accounts from bootstrap_dashpay_contact_accounts) have landed and promises that 'a second call after account bootstrap picks up the rest without regressing anything.' The body destructures ClientStartState { mut platform_addresses, wallets: _, skipped: _, .. } and only re-applies platform_addresses.remove(&self.wallet_id). wallets: _ discards core_wallet_info, identity_manager, and unused_asset_locks for this wallet, so any state associated with an account only present after bootstrap silently fails to re-hydrate — the documented second-call contract is not honored.
Source attribution: claude, codex.
SUGGESTION: restore_dashpay_* / build_identity_public_keys pass Swift-supplied count to typed slice::from_raw_parts without isize::MAX/size_of:: guard
packages/rs-platform-wallet-ffi/src/persistence.rs:3840-4235
Carried forward from prior-3; STILL VALID at 1a332ad. The byte-slice helper slice_from_raw (persistence.rs:4436) only accepts *const u8 and checks len <= isize::MAX. Five typed restore paths call slice::from_raw_parts::<T>(ptr, count) on repr(C) structs guarded only against null/zero: restore_dashpay_payments (line 3844, PaymentRestoreEntryFFI), restore_dashpay_ignored (3868, [u8; 32]), restore_contact_profiles (~3960, ContactProfileRestoreEntryFFI), restore_dashpay_contacts (~4064, ContactRequestFFI; plus inner slices at ~4111 and ~4118), and build_identity_public_keys (~4221, IdentityKeyRestoreFFI; plus per-row data/data_len at ~4235). slice::from_raw_parts::<T> requires count * size_of::<T>() <= isize::MAX; a Swift-supplied count in (isize::MAX/size_of::<T>() .. isize::MAX] satisfies the byte-slice guard but is instant UB for typed arrays. Introduce a typed helper (slice_from_raw_typed<T> that checks count.checked_mul(mem::size_of::<T>()).is_some_and(|n| n <= isize::MAX as usize)) and route these five sites through it; inner [u8]/[u32] slices can go through the existing byte helper.
Source attribution: claude, codex.
SUGGESTION: core_instant_locks reader materializes txid before any fixed-width gate
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:372-390
Carried forward from prior-5; STILL VALID at 1a332ad. The instant-lock block reads txid_bytes: Vec<u8> = row.get(0)? at line 381 without a length gate before Txid::from_slice at line 384; only islock_blob at line 382 uses blob::check_size. dashcore::Txid is fixed 32 bytes, so this delta's newly-added blob::check_fixed_width helper is the right shape — add length(txid) to the SELECT and gate with blob::check_fixed_width(len, 32, "core_instant_locks.txid") before the Vec allocation, matching the delta's treatment of core_utxos.script at core_state.rs:686-700.
Source attribution: claude, codex.
SUGGESTION: load_unconsumed materializes outpoint before any size gate
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:184-204
Carried forward from prior-4; STILL VALID at 1a332ad. load_unconsumed reads op_bytes: Vec<u8> = row.get(0)? at line 195 with no pre-length gate; only lifecycle_blob at line 197 uses blob::check_size. Outpoints are fixed-width 36 bytes, so the delta's newly-added blob::check_fixed_width helper is the right shape — add length(outpoint) to the SELECT and gate with blob::check_fixed_width(len, 36, "asset_locks.outpoint") before the Vec allocation. Same gap applies to list_active at lines 211+.
Source attribution: claude, codex.
SUGGESTION: apply_identity_entry update branch never migrates wallet_id or bucket placement
packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:41-78
Carried forward from prior-7; STILL VALID at 1a332ad. The existing-identity branch (lines 47-78) updates scalar fields (balance, revision, status, dashpay_profile, dpns_names, contested_dpns_names, dashpay_payments, contact_profiles, ignored_senders) but never reassigns existing.wallet_id and never moves the identity between out_of_wallet_identities and wallet_identities[wallet_id] — unlike the fresh-insert branch. If an identity is first observed out-of-wallet and later gets a wallet association via apply_identity_entry(entry.wallet_id = Some(w)), it stays stuck in out_of_wallet_identities and the location_index_insert invariant breaks. Today's exposure is limited to the runtime changeset-replay path (the load path routes through load_prekeyed + merge_contacts_and_keys), but the update branch will silently miswire whenever replay is exercised.
Source attribution: claude, codex.
SUGGESTION: snapshot_accounts_match_manifest compares AccountType set but never verifies xpub bytes
packages/rs-platform-wallet/src/manager/load.rs:332-360
Carried forward from prior-8; STILL VALID at 1a332ad. snapshot_accounts_match_manifest compares manifest_types == snapshot_types as BTreeSet<AccountType> (with ProviderOperator/ProviderPlatform/PlatformPayment filtered via comparable). AccountType derives Ord/Eq over variant/index only — no key material. A tampered snapshot with matching account slots but substituted xpubs passes this self-consistency check. Even scoped as self-consistency (not authenticity, which #3992 tracks), the residual gap is worth closing here by including a hash of each account's xpub bytes in the compared set.
Source attribution: claude, codex.
SUGGESTION: Rehydration integration tests leave unused_asset_locks defaulted — asset-lock flattening has no round-trip coverage
packages/rs-platform-wallet/tests/rehydration_load.rs:70-88
Carried forward from prior-9; STILL VALID at 1a332ad. The slice() helper still constructs ClientWalletStartState { ..., unused_asset_locks: Default::default(), ... }, and load_from_persistor's asset-lock flattening at manager/load.rs:198-202 has no manager-level round-trip test — a regression that miswires the flatten (e.g., silently dropping an account bucket) would pass every rehydration_load test. Add one integration that seeds non-default unused_asset_locks and asserts they round-trip onto the registered manager.
Source attribution: claude, codex.
Budget-Dropped Still-Valid Prior Nitpicks
The verifier kept these in dropped_findings due the public comment budget, but they were explicitly re-validated as still valid at 1a332ad4: prior-10, prior-11, prior-12, and prior-13 as listed above.
…ion-number collision T5: core_address_pool's primary key omitted the DashPay (user_identity_id, friend_identity_id) pair, so two contacts on one wallet collapsed onto the same PK and silently overwrote each other's pool rows once dashpay_account_registration_changeset started emitting AccountAddressPoolEntry for DashPay contacts. Widen the PK to mirror account_registrations (V001), thread the identity pair through apply_pools/UPSERT_POOL_SQL and the account_index_for_script tie-break, and add a regression test covering two DashPay contacts on one wallet. While adding a passing regression test, found every SqlitePersister::open()/ migrations::run() call was failing with "UNIQUE constraint failed: refinery_schema_history.version": V002__address_height_pin.rs (PR #4019, already merged) and V002__unified.rs (PR #3986) both independently claimed migration version 2. Renumber the unified migration to V003 (rename file + test files, bump every version-2 reference to 3, recompute the schema-freeze golden fingerprints). This was silently failing 25/243 --lib unit tests and all 8 sqlite_core_pool_writer tests; all now pass (243/243, 21/21 targeted). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3 participants
Why this PR exists
load_from_persistor()needs a persister that can actually reconstruct a wallet's identities, contacts, and identity keys from disk. Without it, the SQLite-backed native persister returns an empty start-state and every restart re-derives from scratch.ClientWalletStartState, byte-identical). Implements the storage-side reconstruction: prekeyed identity/contact joins, and thecore_wallet_info(ManagedWalletInfo) assembly that feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692's field-removal work made mandatory.What was done (
rs-platform-wallet-storage+rs-platform-wallet)IdentityManagerStartState::merge_contacts_and_keysroutes persisted keys and contacts onto managed identities in-memory at load time (joining theidentities/identity_keys/contactstables), rather than replaying a separate changeset later — matching what the FFI/iOS path already does viabuild_wallet_identity_bucket. Legacycontacts/identity_keyschangeset fields are emitted empty regardless of merge order, so this lands correctly whether feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 or feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692) #3968 merges first.core_wallet_infoassembly: feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 made this field a requiredBox<ManagedWalletInfo>(dropping the old skeleton+replay fallback). Revived feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692's deletedapply_persisted_core_state/extend_pools_for_restored_addressesas a sharedpub platform_wallet::rehydratemodule (DRY over a third copy), adapted to the post-feat: complete dashpay in platform wallet and swift example app #3841 sealed contact model. A wallet with an empty account manifest (never fully registered) is represented as a placeholder empty wallet at the persister layer; the manager still treats it asMissingManifestand skips it one layer up, exactly as before — this exists only so one unregistered wallet doesn't abort loading every other wallet in the store.core_address_poolsnapshot rows —load()now unions real pool data withcore_state-derived used-addresses instead of hardcoding account 0, andapply_changeset_to_txwritesaccount_address_poolsback to the pool table. Replaces the account-0 approximation this PR originally shipped with.wallet_metadatatable-name reference in two FK constraints (should bewallets) that broke inserts; a dropped blob-length pre-read gate onplatform_addresses.address(restored, combined with feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692's newas_of_height).core_address_pool's primary key omitted the DashPay(user_identity_id, friend_identity_id)pair, so two contacts on one wallet collapsed onto the same PK and silently overwrote each other's pool rows. Widened the PK to mirroraccount_registrations(V001); added a regression test covering two DashPay contacts on one wallet.V002__address_height_pin.rs) and feat(platform-wallet): snapshot readers + unified V002 schema migration #3986 (V002__unified.rs) each independently claimed schema version 2 — everySqlitePersister::open()was failing with arefinery_schema_historyUNIQUE-constraint error as a result (25/243--libtests, all 8sqlite_core_pool_writertests). Renumbered the unified migration to V003 and recomputed the schema-freeze golden fingerprints.key_walletAPI that doesn't exist yet; both re-warm on next sync).wallet_id) is intentionally not in this PR — tracked separately in feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) #3992.Known follow-up needed (not fixed in this PR)
sqlite_pool_reader.rs,sqlite_migration_execution.rs): both assume aused_core_addressesfield onClientWalletStartStatethat was never shipped (the real design folds used-address data intocore_wallet_info). TODO-commented in each file; unrelated to this PR's changes, needs a rewrite against the actual struct shape.Testing
cargo fmt,cargo clippyclean onplatform-walletandplatform-wallet-storage(excluding the two known-broken files above, which don't compile).cargo test -p platform-wallet-storage --lib: 243 passed / 0 failed (was 218/243 before the migration-collision fix). Targeted integration suites directly exercising the pool schema and migration shape (sqlite_core_pool_writer,sqlite_v003_migration,sqlite_schema_pinning,sqlite_v003_isolation,sqlite_store_generation): 21/21 passed, including the new T5 regression test. Independently re-verified across several rounds: the original 5-commit implementation (test execution, security audit, consistency/dedup review), the merge onto #3692's final tip, and this pass verifying the #3986 merge + T5 fix — no MEDIUM+ findings survived any round.Breaking changes
None beyond what #3692 already declares — this PR builds directly on that shape.
Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent