feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692)#3968
Open
Claudius-Maginificent wants to merge 24 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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 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
3 tasks
…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
2 tasks
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Collaborator
|
✅ Review complete (commit 0205123) |
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
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}
Contributor
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>
…ity via a pollable counter (#3968 review, H2) The vault's atomic write fsyncs the parent dir to make the persist() rename durable across power loss. A dir-fsync failure happens AFTER the data is committed + visible, so it stays intentionally non-fatal — propagating it would force put/delete/rekey to roll back in-memory state that already matches disk, diverging the live handle. Previously it was swallowed at warn! + Ok, which the review flagged as silent. Option B: keep the Result contract unchanged (still Ok — the write succeeded) but stop swallowing the signal. Elevate the log warn! -> error!, and bump a per-store durability_uncertain AtomicU64 exposed via EncryptedFileStore::durability_uncertain_count() (and SecretStore pass-through, Some on File / None on Os). A caller that cares about hard durability polls it rather than seeing a spurious error. The increment-on-failure path is environment-specific (can't force a dir-fsync failure portably) so it isn't unit-tested; a test guards the accessor + the no-spurious-bump invariant on a confirmed write. 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>
Collaborator
Author
Review-fix summary (pushed in
|
thepastaclaw
approved these changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: prior-1 through prior-5 are fixed, while the prior oversized-BLOB allocation issue remains valid in the broader rehydration load surface even though core_transactions.record_blob was hardened. New latest-delta finding: the new chain-lock monotonic merge helper accepts a valid-prefix/trailing-garbage blob that the load path later rejects. No actionable CodeRabbit findings were provided.
🟡 3 suggestion(s)
Findings not posted inline (1)
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/accounts.rs:82-87: Rehydration BLOB readers still allocate before the size cap — The priorcore_transactions.record_blobinstance was fixed with alength(record_blob)check, but the productionSqlitePersister::load()path still materializes other PR-added rehydration BLOB columns beforeblob::decode()can enforce the 16 MiB cap.platform_addrs::load_all()reaches `a...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:82-87: Rehydration BLOB readers still allocate before the size cap
The prior `core_transactions.record_blob` instance was fixed with a `length(record_blob)` check, but the production `SqlitePersister::load()` path still materializes other PR-added rehydration BLOB columns before `blob::decode()` can enforce the 16 MiB cap. `platform_addrs::load_all()` reaches `all_platform_payment_registrations()` first, where `account_xpub_bytes` is read with `row.get::<_, Vec<u8>>(2)`, and the per-wallet load loop has the same pattern for `asset_locks.lifecycle_blob`, `core_instant_locks.islock_blob`, `identities.entry_blob`, `identity_keys.public_key_blob`, and contact request/metadata blobs. A restored or locally replaced SQLite DB can therefore pass schema and integrity checks while forcing startup to allocate/copy an attacker-sized cell before returning `BlobTooLarge`; apply the same pre-materialization `length(blob_column)` gate, preferably through a shared bounded-read helper, to every decoded load BLOB.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:82-87: Rehydration BLOB readers still allocate before the size cap
The prior `core_transactions.record_blob` instance was fixed with a `length(record_blob)` check, but the production `SqlitePersister::load()` path still materializes other PR-added rehydration BLOB columns before `blob::decode()` can enforce the 16 MiB cap. `platform_addrs::load_all()` reaches `all_platform_payment_registrations()` first, where `account_xpub_bytes` is read with `row.get::<_, Vec<u8>>(2)`, and the per-wallet load loop has the same pattern for `asset_locks.lifecycle_blob`, `core_instant_locks.islock_blob`, `identities.entry_blob`, `identity_keys.public_key_blob`, and contact request/metadata blobs. A restored or locally replaced SQLite DB can therefore pass schema and integrity checks while forcing startup to allocate/copy an attacker-sized cell before returning `BlobTooLarge`; apply the same pre-materialization `length(blob_column)` gate, preferably through a shared bounded-read helper, to every decoded load BLOB.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:65-69: Chain-lock merge can preserve bytes that load rejects
`decode_chain_lock_soft()` rejects a valid `ChainLock` prefix with trailing bytes and leaves `last_applied_chain_lock` as `None`, but `chain_lock_height()` ignores the consumed byte count. During `upsert_sync_state()`, an existing corrupt `last_applied_chain_lock` with a higher decoded prefix height can beat a later valid lower-height chain lock and remain stored, contradicting the recovery comment that the next ChainLock event repopulates the column. Make the merge helper use the same full-consumption rule as the load decoder so corrupt existing bytes lose to the next valid update.
…erge-height helper (#3968 review) `chain_lock_height` decoded the `last_applied_chain_lock` blob without requiring `consumed == bytes.len()`, unlike the load-side `decode_chain_lock_soft`. A corrupt stored blob (valid ChainLock prefix + trailing bytes) therefore yielded a height and could out-rank a later valid lower-height update in the monotonic-max merge, staying stuck — while the load path already rejects it (→ None). Match the load decoder so a corrupt blob loses to the next valid update. Adds a unit test. <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
approved these changes
Jun 30, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The latest delta fixes the prior chain-lock merge issue by making chain_lock_height() require full bincode consumption and adding a regression test; I found no new latest-delta findings. One carried-forward, in-scope suggestion remains: the PR's seedless load() rehydration path still has decoded SQLite BLOB readers that allocate the cell before the shared size cap can reject oversized data.
🟡 2 suggestion(s)
Findings not posted inline (1)
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/accounts.rs:82-87: Rehydration BLOB readers still allocate before the size cap —SqlitePersister::load()callsplatform_addrs::load_all(), which reachesall_platform_payment_registrations()and materializesaccount_xpub_byteswithrow.get::<_, Vec<u8>>(2)beforeblob::decode()can enforceBLOB_SIZE_LIMIT_BYTES. The latest delta hardened `core_transactions.record...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:82-87: Rehydration BLOB readers still allocate before the size cap
`SqlitePersister::load()` calls `platform_addrs::load_all()`, which reaches `all_platform_payment_registrations()` and materializes `account_xpub_bytes` with `row.get::<_, Vec<u8>>(2)` before `blob::decode()` can enforce `BLOB_SIZE_LIMIT_BYTES`. The latest delta hardened `core_transactions.record_blob` with a pre-read `length(record_blob)` check, but this PR's load path still has the same read-before-cap pattern here and in other decoded rehydration BLOBs such as `core_instant_locks.islock_blob`, `asset_locks.lifecycle_blob`, `identities.entry_blob`, `identity_keys.public_key_blob`, and contact request/account blobs. A restored or locally replaced SQLite wallet DB can pass schema/integrity checks while forcing startup to allocate and copy an attacker-sized cell before returning `BlobTooLarge`; apply the same pre-materialization `length(blob_column)` gate consistently to decoded load-time BLOB columns.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:82-87: Rehydration BLOB readers still allocate before the size cap
`SqlitePersister::load()` calls `platform_addrs::load_all()`, which reaches `all_platform_payment_registrations()` and materializes `account_xpub_bytes` with `row.get::<_, Vec<u8>>(2)` before `blob::decode()` can enforce `BLOB_SIZE_LIMIT_BYTES`. The latest delta hardened `core_transactions.record_blob` with a pre-read `length(record_blob)` check, but this PR's load path still has the same read-before-cap pattern here and in other decoded rehydration BLOBs such as `core_instant_locks.islock_blob`, `asset_locks.lifecycle_blob`, `identities.entry_blob`, `identity_keys.public_key_blob`, and contact request/account blobs. A restored or locally replaced SQLite wallet DB can pass schema/integrity checks while forcing startup to allocate and copy an attacker-sized cell before returning `BlobTooLarge`; apply the same pre-materialization `length(blob_column)` gate consistently to decoded load-time BLOB columns.
…readers (#3968 review) Add `length(<blob_col>)` gate — checked BEFORE materialising the `Vec<u8>` — to every load-path BLOB reader that was missing it, mirroring the existing pattern already applied to `core_transactions.record_blob` in core_state.rs. Readers gated: - core_state.rs : core_instant_locks.islock_blob - accounts.rs : account_xpub_bytes (load_state, list_platform_payment_registrations, all_platform_payment_registrations) - asset_locks.rs : lifecycle_blob (load_unconsumed, load_state, list_active) - identities.rs : entry_blob (load_state, fetch) - contacts.rs : outgoing_request / incoming_request / accepted_accounts (load_state) - identity_keys.rs : public_key_blob (load_state) Each reader's SELECT now leads with `length(<col>)` (O(1) from the row header) and returns `WalletStorageError::BlobTooLarge` before calling `row.get::<_, Vec<u8>>()`, so a tampered/corrupt oversize BLOB in a local wallet DB cannot force a multi-GiB allocation that `blob::decode`'s post-hoc cap only catches after the Vec is built. Compile-time allow-list in sqlite_compile_time.rs updated to match the new SQL strings; two regression tests added (accounts xpub_bytes + identity_keys public_key_blob). <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>
Collaborator
Author
|
The pre-read BLOB size-gate suggestion is addressed in Two regression tests ( 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
approved these changes
Jun 30, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The prior prior-blob-read-before-cap finding is STILL VALID in part: the latest commit added pre-materialization length(...) gates to the originally cited account reader and the other named bincode-serde readers, but core_sync_state.last_applied_chain_lock remains a decoded load-time BLOB that is materialized before its bounded decoder runs. I also verified two new in-scope hardening gaps in the same seedless load() surface: fixed-width platform-address BLOBs are read before length validation, and the nested identity public-key bincode decode bypasses the bounded config.
🟡 4 suggestion(s)
Findings not posted inline (1)
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/platform_addrs.rs:324-333: Platform address BLOBs allocate before fixed-width validation —platform_addrs::load_all()scansplatform_addressesbefore the per-wallet reconstruction loop and materializesaddresswithrow.get::<_, Vec<u8>>(3)beforedecode_address_row()checks that the cell is exactly 20 bytes. The schema does not enforcelength(address) = 20, so a tampered re...
🤖 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`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:401-423: Chain-lock load still materializes the BLOB before applying the cap
`SqlitePersister::load()` reaches `core_state::load_state()`, which reads `core_sync_state.last_applied_chain_lock` with `row.get::<_, Option<Vec<u8>>>(2)` before `decode_chain_lock_soft()` can enforce `BLOB_SIZE_LIMIT_BYTES` through the bounded bincode config. A restored or locally replaced wallet DB can therefore pass schema and integrity checks while forcing startup to allocate and copy an oversized chain-lock cell before the soft decoder drops it. This is the remaining load-time decoded-BLOB case from the prior read-before-cap finding class, so add the same pre-materialization `length(last_applied_chain_lock)` gate used by the other readers.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs:324-333: Platform address BLOBs allocate before fixed-width validation
`platform_addrs::load_all()` scans `platform_addresses` before the per-wallet reconstruction loop and materializes `address` with `row.get::<_, Vec<u8>>(3)` before `decode_address_row()` checks that the cell is exactly 20 bytes. The schema does not enforce `length(address) = 20`, so a tampered restored DB can attach a very large `address` BLOB to an otherwise valid wallet row and force startup to allocate it before load fails. Select `length(address)` first and reject anything other than 20 bytes before reading the BLOB; doing the same for fixed-width `wallet_id` columns would keep this reader consistent with the new BLOB gates.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs:324-333: Platform address BLOBs allocate before fixed-width validation
`platform_addrs::load_all()` scans `platform_addresses` before the per-wallet reconstruction loop and materializes `address` with `row.get::<_, Vec<u8>>(3)` before `decode_address_row()` checks that the cell is exactly 20 bytes. The schema does not enforce `length(address) = 20`, so a tampered restored DB can attach a very large `address` BLOB to an otherwise valid wallet row and force startup to allocate it before load fails. Select `length(address)` first and reject anything other than 20 bytes before reading the BLOB; doing the same for fixed-width `wallet_id` columns would keep this reader consistent with the new BLOB gates.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:39-54: Nested public-key bincode decode bypasses the size limit
`public_key_blob` is now size-gated before materialization, and the outer `IdentityKeyWire` is decoded through `blob::decode()`, but the nested `public_key_bincode` field is decoded with unbounded `bincode::config::standard()`. `IdentityPublicKeyV0` contains `BinaryData(Vec<u8>)`, and bincode's native `Vec<u8>` decoder allocates from the decoded inner length when no limit is configured, so a small outer blob can still carry an oversized inner length prefix that drives allocation before decode failure. Use the same bounded config for this native bincode layer so the persisted identity-key payload is capped end to end.
…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
approved these changes
Jun 30, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
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
suggested changes
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
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.
thepastaclaw
suggested changes
Jul 2, 2026
Collaborator
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
6 tasks
6 tasks
lklimek
changed the base branch from
v4.1-dev
to
feat/platform-wallet-rehydration
…llet-storage-rehydration
thepastaclaw
suggested changes
Jul 3, 2026
Collaborator
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
Comment on lines
+421
to
+439
| 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) | ||
| } |
Collaborator
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.
Suggested change
| 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']
Comment on lines
+184
to
+204
| 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) | ||
| } |
Collaborator
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']
Comment on lines
+362
to
+380
| { | ||
| // 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); | ||
| } | ||
| } |
Collaborator
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']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
load_from_persistor()has nothing to read from — the persister'sload()returns an empty start-state. This PR is the storage-crate implementation that reconstructs persisted rows into the keylessClientWalletStartState.v3.1-dev. It carries only the storage work plus the single shared type (ClientWalletStartState, byte-identical to feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692). Because reshaping that struct removeswallet/wallet_info, two base consumers are stubbed withtodo!()(manager/load.rswhole-body,ffi/persistence.rstail-only) — both resolved by feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 in thedash-evo-toolintegration. The cross-crate manager-apply e2e tests are gated behind an off-by-defaultrehydration-applyfeature (enabled in the integrated stack).What was done (
rs-platform-wallet-storage)SqlitePersister::load()wiring + per-area readers (accounts, core_state, identities, asset_locks unconsumed-only, contacts, identity_keys).last_applied_chain_lockcolumn.NeedsPassword, size caps, fd-lock pin).Testing
cargo fmt --all --check;cargo clippy --workspace --all-targets -- -D warnings;cargo test -p platform-wallet-storage(222 passed; the gated cross-crate e2e runs in the integrated stack and passes there).Breaking changes
None against
v3.1-dev.Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Documentation