feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration - #3968
Open
Claudius-Maginificent wants to merge 338 commits into
Open
feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration#3968Claudius-Maginificent wants to merge 338 commits into
Claudius-Maginificent wants to merge 338 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a Tier-2 secret-envelope format and hardens secret storage, while renaming the SQLite wallet root to ChangesSecrets
SQLite
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
June 29, 2026 13:44
52cdad9 to
83f7d4f
Compare
lklimek
force-pushed
the
feat/platform-wallet-storage-rehydration
branch
from
June 29, 2026 13:44
3d57f73 to
2f2a74a
Compare
5 tasks
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Claudius-Maginificent
marked this pull request as ready for review
June 29, 2026 16:00
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
June 29, 2026 16:00
Collaborator
|
🕓 Ready for review — 40 ahead in queue (commit add5926) |
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
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.
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.
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.
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.
Claudius-Maginificent
changed the base branch from
feat/platform-wallet-rehydration
to
v4.1-dev
July 1, 2026 09:34
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #3968 +/- ##
============================================
- Coverage 87.10% 86.04% -1.06%
============================================
Files 2756 2756
Lines 359634 364311 +4677
============================================
+ Hits 313247 313480 +233
- Misses 46387 50831 +4444
🚀 New features to boost your workflow:
|
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
… in SCHEMA.md The ER diagram, the Tables entry, and the FK-conventions section all documented identity_keys' primary key as (wallet_id, identity_id, key_id) — the wide key this PR narrowed to (identity_id, key_id) because it was the enabling condition for duplicate-row corruption (migrations/V001__initial.rs, current state). They also described wallet_id as a single-column NOT NULL FK, where it is actually a nullable, denormalised copy of identities.wallet_id enforced by a compound FK, (wallet_id, identity_id) -> identities(wallet_id, identity_id). Corrected all three sites against V001__initial.rs and added the NULL-scope dormant-FK / trigger-guard context from its inline comments, since the wrong nullability claim was hiding exactly the mechanism (SQLite MATCH SIMPLE going dormant on a NULL child-key column) the corruption fix depends on.
… triggers and migrations log Walked every migration file (V001-V015) and every src/sqlite/schema/ module against SCHEMA.md and found more drift than the two named tables: the gap disclaimer only ever tracked V003/V004/V008 and never grew to admit V011's dpns_name_states, V013's tracked_masternodes, or V015's identity_scan_states/identity_scan_failed_indices pair, so the document silently claimed completeness on tables it does not cover. The Migrations log skipped V010, V011, V013 and V014 outright. Also, two migrations the disclaimer implicitly claimed WERE already covered were not: V002's platform_addresses.as_of_height and V010's widening of asset_locks.status to recovered_from_chain were both missing from their diagrams and Tables entries. Added both directly rather than deferring them. Fixed: - Gap disclaimer (line 40): accurate, complete list of the nine still-undiagrammed tables, through V015. - Diagram 4 / Tables: platform_addresses.as_of_height, asset_locks.status's recovered_from_chain member. - Tables: identities' idx_identities_wallet_identity UNIQUE index (V001) — the parent key identity_keys' compound FK depends on. - Triggers table: the identity_keys_null_scope_requires_unowned_identity pair (V001, tightened by V014) — per V001's own comment, the sole guard on NULL-scoped identity keys once both FKs go dormant. - Migrations log: added V010, V011, V013, V014, V015 rows. Verified against migrations/V001..V015 and src/sqlite/schema/*.rs (asset_locks.rs's ASSET_LOCK_STATUS_LABELS/frozen-in-V010 test confirmed the exact status list). Scope note: the nine newly- acknowledged tables are still deferred to the Migrations log rather than given full diagrams/Tables entries, consistent with the document's existing convention for V003/V004/V008 — see report for a recommendation on generating this section instead of hand-maintaining it.
…RETS.md The secret-serde Cargo feature was deleted (pure rename to serde, default-off Deserialize semantics preserved — pinned by deserialize_absent_without_the_serde_feature_even_though_the_dep_is_on and its --all-features counterpart on fix/triage-code). Updated the two SECRETS.md mentions to match; verified the surrounding prose (both sites just name the feature gating an impl, no claim tied to the old name that the rename would invalidate). Also checked SECRETS.md for the other two features deleted in the same commit, secret-schemars and rehydration-apply (per fix/triage-code:Cargo.toml, JsonSchema for SecretString is now unconditional under secrets, and the rehydration-apply items are now unconditionally public) — neither name appears in this file, so no further change needed.
…ed-memory budget `locked_cost` denominates the crate's 64 KiB `RLIMIT_MEMLOCK` budget in 4 KiB pages, but memsec 0.7.0 rounds every guarded allocation to the page size the kernel reports at run time (`alloc/mod.rs:29`: `sysconf(_SC_PAGESIZE)` / `GetSystemInfo`). On a 16 KiB-page host (macOS/iOS arm64) the real reprotect peak is ~112 KiB and on a 64 KiB-page host (aarch64 RHEL/SLES) ~448 KiB. `mlock` then fails open with a warning and seed / xpriv material silently becomes swappable — the CWE-316 weakness this module exists to close. The `const _` budget assertion constrains the ceilings; it can say nothing about the host. Keep the 4096 constant and all compile-time budget maths, and check the host at store construction instead: `verify_host_page_size` returns the new typed `SecretStoreError::HostPageSizeExceedsBudget`, naming the host's page size, the assumed one, and the remedy. A typed error rather than a panic — this is a library and an oversized page is a property of the host, not a programming bug. Wired into `EncryptedFileStore::open_inner` (ahead of any filesystem work, so no vault file is left behind) and `SecretStore::os()`, since both arms hand out guarded `SecretBytes`. Smaller-than-assumed pages pass: they make `locked_cost` an over-estimate, leaving the budget conservative rather than overrun. `region` moves from dev-dependency to an optional dependency gated by `secrets` — already in `Cargo.lock`, so the graph is unchanged, and its page-size query stays safe on Windows where `libc` is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eleven features were too many to keep documented or combinatorially tested, and three of them earned nothing. `secret-serde` becomes the conventional `serde` feature. It gated the `SecretString: Deserialize` IMPL, never the dep — `secrets` compiles serde for the vault format regardless — so the rename changes the name and nothing else. Still default-off, still no `Serialize` under any combination, and a plain package test still asserts the impl's ABSENCE while `--all-features` asserts its presence. `secret-schemars` disappears: the `JsonSchema` impl renders a bare `"type": "string"` with no length policy and no value, so there was nothing for a consumer to opt out of. `schemars` moves into the `secrets` feature, the only one under which `SecretString` exists at all. `rehydration-apply` gated `sqlite::util::apply_persisted_core_state` and, transitively, `LoadCtx` — the type that function takes. Both are now unconditionally public, which is what the downstream consumer wanted the feature for. The two #3968 end-to-end regressions it also gated (`rehydration_routes_via_real_sql_resolver`, `rehydration_routes_used_addresses_to_owning_account`) plus `utxo_on_fresh_gap_limit_address_rehydrates_under_first_funds_account` lose their `cfg` and now run under the DEFAULT feature set instead of only under `--all-features`: coverage gained, not dropped. README's Cargo-features table documented 5 of 11. It now documents all 8 that survive, `serde` / `shielded` / `test-util` included. BREAKING CHANGE: the `secret-serde`, `secret-schemars` and `rehydration-apply` features are gone. Enable `serde` for the `SecretString: Deserialize` impl; the schemars impl and the rehydration apply/`LoadCtx` surface need no feature at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rom the reserve `regression_reports_max_from_usable_suffix_not_total_account_balance` failed on `--all-features`, on this branch and on its base. The failing line was the test's own shape-precondition guard, not a production assertion, and its message said what to do: re-seed the balances when the versioned reserve drops below the hardcoded leading balance. That is exactly what happened. `reserve()` is `2 x compute_minimum_shielded_fee(2 actions)`. Under the v9 event constants (proof verification 100_000_000, storage 344 bytes/action) it was 325_702_400; under v10 (40_000_000 and 550 bytes/action) it is 228_280_000. The fixture's 297_264_780 sits between the two, so the leading address stopped being sub-reserve dust and the "usable suffix" shape the test claims to build no longer existed. The planner is right either way: with a lower reserve that address genuinely is a viable input 0, so the whole balance genuinely is usable. Maximum spendable balance was never miscomputed. Derive the leading balance as `reserve() - 1` instead — the largest balance that must still be rejected by the strict `> reserve` viability test, so a tighter boundary than the magic number was, and one no future fee re-balance can invalidate. The guard goes with it: the precondition is now true by construction. CI never caught this. The wallet job filtered nextest with `not test(~shield)`, a substring match on the full test path, so the `shield_input_selection_tests` module was excluded as collateral by a filter aimed at the shielded-wallet suite. It took 47 pure-logic tests across the three wallet crates with it — input selection, FFI error codes and memo encoding, SQLite viewing-key rows — for a measured 0.089 s of runtime. Exclude by module path (`wallet::shielded::`) so the step skips what it means to skip, and pin `--no-tests fail` so a filter that stops selecting anything fails the step instead of passing green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd re-derive the locked-memory budget `verify_host_page_size` refused any host whose pages exceed ASSUMED_PAGE_SIZE, which was 4096. Apple Silicon and iOS use 16 KiB pages, so `SecretStore` could not be constructed there at all and every `secrets` test failed on the `macOS, ARM64` wallet runner — on a first-class target with a Swift SDK and an FFI layer in this repo. Raising the assumption changes zero bytes of actually-locked memory. memsec computes `mlock(ptr, page_round(CANARY_SIZE + size))` against `sysconf(_SC_PAGESIZE)` (memsec 0.7.0, src/alloc/mod.rs), so a 16 KiB host was already locking at 16 KiB granularity. Assuming 4096 never reduced the real cost; it made the accounting wrong. Every enlarged figure below was already true and merely mis-stated. Re-derived from the call paths, not scaled by a guessed factor. At a 16 KiB page every budgeted ceiling plus the 16-byte canary fits inside one page, so the budget collapses from a sum of sizes into a count of live secrets: the File-arm reprotect peak is seven pages (112 KiB) and a concurrent max-size read three more (48 KiB). MEMLOCK_BUDGET goes to 256 KiB, the smallest power of two holding 160 KiB with real headroom. MAX_SECRET_LEN stays 8176 and MAX_PASSPHRASE_LEN 4080 — both public API, both deliberately not widened. At 16 KiB, 16368 would have cost the same single page, but that slack is free only in the accounting: on the 4 KiB hosts that dominate deployment memsec rounds to the real page size, so 16368 costs four locked pages per secret instead of one. 8176 is already ~30x the largest legitimate secret. MAX_SECRET_LEN is now spelt as a literal, since deriving it from a page size is what tied a product ceiling to one host's idea of a page. The 64 KiB RLIMIT_MEMLOCK the old budget targeted described no supported host: systemd has defaulted DefaultLimitMEMLOCK to 8 MiB for years, and a default Docker container inherits it (both measured at 8388608 bytes on the development host). macOS enforcement could not be measured here and is deliberately not asserted; it cannot block this change, because a Darwin limit too small for 160 KiB would already be too small for the 112 KiB such a host really locks today. The const-assertion block keeps its compile-time guarantee, restated on invariants that survive the move: each of the envelope and passphrase ceilings fits a single guarded page, and the peak is seven of them. Refusal semantics are unchanged and now fire only above 16 KiB, for 64 KiB-page aarch64 RHEL/SLES builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… 16 KiB pages Brings SECRETS.md into line with the re-derived budget: MAX_SECRET_LEN described without the `2 * 4096 - 16` page arithmetic, the 64 KiB RLIMIT_MEMLOCK figure replaced by 256 KiB, and the per-secret page cost given per platform rather than as a bare 4 KiB. Documents two properties the file never stated. The budget assumes 16 KiB pages and refuses a larger-paged host at store construction with HostPageSizeExceedsBudget, so a consumer can tell a deliberate refusal from a bug. And nothing calls getrlimit: MEMLOCK_BUDGET is an arithmetic ceiling asserted at compile time, not a limit checked against the host, so a consumer under a restrictive limit gets the fail-open path and a warn rather than a refusal, and should check the limit at its own startup. Corrects the dependency audit scope. `region` was described as a dev-dependency only and not in the production dependency graph; it is an optional normal dependency enabled by the `secrets` feature, and verify_host_page_size calls region::page::size() on every store construction. An auditor following the old text would have excluded a crate that runs on the production path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6 tasks
This PR bundled ~1600 lines of work that the `rs-platform-wallet-storage`
crate does not need to build or integrate. A forward-dependency scan of the
crate found zero references to any of it: no `Persister*` variants, no
`retry_transient`, no `account_generation`, no `now_secs`, no
`record_or_persister`, no `PlatformWalletError` at all. It was reviewer load
with no linkage.
Every such file is reverted to its base-branch content here. What remains
outside the crate is exactly its build/integration surface:
- `changeset/changeset.rs`, `changeset/shielded_changeset_disabled.rs`,
`changeset/mod.rs`, `wallet/apply.rs` — the feature-unification fix
(`shielded` unconditional) plus `rebuild_provider_key_account`
- `changeset/traits.rs` — `delete_wallet` + `PersistenceError::UnsupportedOperation`
- `wallet/provider_key_at_index.rs` — `insert_platform_node_pool_entry`
and `PlatformNodePoolError`
- `error.rs` — `PlatformWalletError::PlatformNodePool` only
- `Cargo.lock`, root `Cargo.toml` argon2 profiles, `.cargo/audit.toml`,
`.github/workflows/tests-rs-wallet.yml`
The extracted work moves to follow-up PRs: typed persister errors with
bounded transient retry (incl. the #4133 persister leak and the #4365
retry gap), FFI persister result codes 49/50 with Swift mirrors, the FFI
provider-rebuild dedup, the contact-account filter-scan generation fix, and
a cosmetics batch. Two of them stack: the FFI codes need the typed errors,
and the provider-rebuild dedup needs this PR's `rebuild_provider_key_account`.
Straddling files (`error.rs`, `changeset/traits.rs`) were split by hunk, not
by file — only their required halves survive here.
<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…n PR The 16 MiB bincode-decode size gate on asset-lock proof bytes is unrelated to rs-platform-wallet-storage and has been split out to fix/platform-wallet-ffi-asset-lock-proof-size-gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 2, 2026
…the code SCHEMA.md gains the missing cascade_meta_data_versions_on_wallet_delete trigger, the third cascade path, the V014 trigger citation, the two undiagrammed V001 tables, and contacts.payment_channel_broken — whose absence also made the "four metadata columns" claim wrong in five places. The three write-only tables now say load() does not return them. SECRETS.md drops its drifting variant count in favour of pointing at error.rs, and notes replace_range can exceed the passphrase ceiling. README no longer understates the implemented trait surface. kv.rs stops implying orphan GC is coming. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…rait delete_wallet had no caller anywhere outside platform-wallet-storage, and the trait impl discarded DeleteWalletReport (including backup_path) and flattened WalletStorageError. It stays as an inherent method on SqlitePersister, which is what reports.rs and the removed TODO already claimed. PersistenceError:: UnsupportedOperation goes with it — the deleted default body was its only producer. The removed TODO also deferred `list_wallets`, which has not existed since 2026-04-02 and was never a method on this trait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…16 migration `Domain::WalletMetadata` and `Domain::AccountAddressPools` persisted the strings `wallet_metadata` and `account_address_pools` into `meta_data_versions.domain`. Neither names a table that exists on this schema baseline; both are on the retired-name list the schema-pinning test guards. The labels are the cache-invalidation key hosts read, so a bare rename would silently reset every affected domain's seq to 0. Rename the variants to `Wallets` / `CoreAddressPool` after their live tables, and add migration V016 to rewrite already-persisted rows. On collision the migration takes `MAX(seq)`, preserving the monotonic cache-invalidation invariant. `tc_b_041` now matches retired names as SQL identifiers (keyword-prefixed) rather than as bare substrings, so V016's own string literals do not trip it. A new `domain_labels_are_live_sql_names` test pins the invariant that no `Domain` label may name a retired table. Verified: 20/20 tests across sqlite_migration_execution, sqlite_schema_pinning and sqlite_version_bump (ledger key 2b23f236ca3e36a22ed5b0b15437031c); package clippy clean with `--no-deps`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…p the fictional CI claim Two documentation defects, both of which mislead the next reader into a wrong conclusion about what is enforced. **Trust model.** Nothing stated the crate's actual position on the wallet `.db`, so review after review re-litigated an attacker-authored-database threat model that is out of scope: an adversary who can write arbitrary rows already has same-privilege local code execution, and at that point the process memory, keyring entries and vault passphrase prompt are all equally reachable. Add a `Database trust model` section saying so, and reframe the read-path defenses (size caps, typed/BLOB cross-checks, structural co-ownership, the unconditional integrity and foreign-key pragmas) as what they are: bounds on the blast radius of a bug, a crash mid-write, or failing hardware — not security controls, and not to be cited as such. **Off-state build.** Three sites justified a design decision by pointing at a `--no-default-features --features sqlite,cli` CI build that exists in no workflow: two `Cargo.toml` comments and one `SECRETS.md` paragraph. The decisions themselves are right — `default-features = false` on the dev-dep self-reference IS load-bearing, and the standalone `zeroize` dev-dep IS needed — only the claimed enforcement was fictional. Reword all three to say the invocation is a local/manual check, and state plainly in `SECRETS.md` that an off-state regression therefore reaches `main` unnoticed. No code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…cret-store errors actionable Review findings across the secrets surface. **Ownership check (new).** `check_perms` validated the vault file's mode but never its owner, so a `0600` file belonging to another local user was accepted on the strength of its permission bits alone. It now also compares `meta.uid()` against the process's effective uid and refuses a mismatch with a new `InsecureOwnership` variant naming both uids. The check reuses the same fd the mode check already derives from, so it adds no metadata→read TOCTOU. **Keyless vaults.** `open_unprotected` / `file_unprotected` docs promised "obfuscation, not confidentiality" but said nothing about authenticity. Since the key derives from an empty passphrase under a public salt, anyone who can write the file can also forge a valid vault and inject a chosen secret. Both doc comments now say so. **Actionable errors.** `InsecurePermissions`, `InsecureParentDir` and the new `InsecureOwnership` carry their path and render a remedy (`chmod 600 <path>`, `chmod go-w`, change the owner) instead of a bare "vault file has insecure permissions". `InvalidLabel` now states the allowlist it enforces. **Zeroize on the oversize path.** An OS-keyring blob rejected for exceeding `MAX_SECRET_LEN + MAX_ENVELOPE_OVERHEAD` was dropped without wiping. It is now zeroized before the error returns. **Passphrase ceiling.** `SecretString::replace_range` warns once (lengths only, never content) when growth crosses `MAX_PASSPHRASE_LEN`, which the `SECRETS.md` contract names as the store ceiling. Kept as a warning rather than an error: `SecretString` is general-purpose and erroring would break legitimate large secrets. **Deliberately NOT done: clamping header KDF params to the shipped default.** An intermediate version of this change rejected any vault whose `m_kib`/`t` exceeded `KdfParams::default_target()`. That is a bricking hazard, not a hardening: per-vault params exist precisely so a vault may be hardened above the default, and the clamp would also make every existing vault unopenable if `ARGON2_DEFAULT_M_KIB` were ever lowered. It further contradicted a shipped test asserting the exact `ARGON2_MAX_*` ceilings are accepted. The real DoS control already exists and already gates before the allocator: `KdfParams::enforce_bounds` bounds the header to `ARGON2_MIN_M_KIB..=ARGON2_MAX_M_KIB` (19 MiB..=1 GiB) inside `derive_key`. `derive_and_verify` now documents why that band is deliberately wide, and two tests pin both directions: a header above the absolute ceiling is refused on the read path, and a vault hardened above the shipped target still opens. Verified: 241/241 secrets tests (ledger ded6ff467162a4660237be8459756ff5); package clippy clean with `--no-deps` (workspace clippy blocked by the pre-existing `rs-drive` unused-import warning). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…ale audit rationale Four review findings on the manifest and its supporting comments. Six further items in the same batch were already resolved by the earlier documentation pass on this branch and needed no change. **`shielded` feature wiring (DEP-004).** `shielded = ["platform-wallet/shielded"]` neither implied `sqlite` nor used the optional-dependency form, so `--features shielded` without `sqlite` produced an incoherent build: the optional `platform-wallet` dependency was force-enabled while the persister that consumes shielded state was not. Now `shielded = ["sqlite", "platform-wallet?/shielded"]` — `sqlite` already enables `dep:platform-wallet`, so the `?` form resolves through it. Proven, not assumed: `schema::shielded_viewing_keys::apply` reads `changeset.viewing_keys`, and the feature-off `ShieldedChangeSet` is a fieldless unit struct, so `clippy --no-default-features --features shielded` compiling at all establishes that `platform-wallet/shielded` is active (ledger f8df014ee76ec70970be552248c3bf3f). README's feature table updated to state the implication. **`dash-sdk` feature comment (DEP-003).** The comment claimed the feature set "mirrors sibling `rs-platform-wallet` so the resolver picks identical hashes". It does not mirror it. Storage declares only what it uses directly; `platform-wallet` is what contributes `dash-sdk/wallet` to Cargo's unified set. Comment corrected to describe that. **`.cargo/audit.toml` rationale (PROJ-004).** The bincode advisory suppression justified itself partly on an FFI asset-lock proof size gate that was trimmed out of this branch and now lives in #4585. Removed from the mitigation list and recorded as a residual risk instead, so the ignore is not resting on a control that is not present. **Feature-off serde guarantee (DOC-005).** The `shielded` field's comment asserted the wire format is byte-identical with the feature off. Reworded to state what is actually guaranteed, and pinned with a test proving the serialized object has no `shielded` key under `--features serde` without `shielded`. Not changed: `test-util` (DEP-002) is deliberately exposed for downstream test suites and already follows the conventional naming. Verified: platform-wallet-storage 823 passed, platform-wallet 939 passed; package clippy clean with `--no-deps` in default, `serde`, and `no-default-features + shielded` configurations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…the schema-history guard Four review findings on the connection, migration-history and provider-key paths. **URI filenames (SEC-008).** `open_conn`'s read-write arm used `Connection::open`, whose default flags include `SQLITE_OPEN_URI`, so a path could smuggle query parameters (`?mode=rwc`) that defeat the caller's intent — while the doc comment claimed URI parsing was "deliberately left off". Two changes: the read-write arm now passes explicit flags (`READ_WRITE | CREATE | NO_MUTEX`, i.e. rusqlite's default minus `URI`), and because the bundled SQLite can enable URI parsing globally regardless of open flags, any path beginning `file:` is rejected outright. A prefix check, not a substring one, so an ordinary path containing a colon is unaffected. **Schema-history timestamp guard (SEC-005).** The guard exists because refinery parses `applied_on` with `unwrap()` — a malformed value aborts the process. Validating with a bare `parse_from_rfc3339` accepted shapes refinery itself would then reject, leaving the panic reachable. The check is now restricted to refinery's canonical shape, with leap-second and Unicode-minus regressions. Established rather than assumed that this cannot reject a database refinery wrote: `refinery-core` stamps `OffsetDateTime::now_utc()` (`runner.rs`) and formats with `time::Rfc3339` (`traits/mod.rs`), and `time` emits a trailing `Z` for a UTC offset rather than `+00:00`. So every refinery-written value passes. The gate IS narrower than refinery's own reader, which accepts any RFC3339 value — a hand-written `+00:00` is now refused. That is the intended trade: a typed error in place of an unrecoverable panic. The comment at the site records the whole chain so the next reader does not have to re-derive it. **Provider-key pool atomicity (CALL-002).** `populate_platform_node_pool` converted each key's index inside the mutation loop, so an invalid index halfway through a batch left earlier keys already inserted. Every index is now validated before any mutation, with a test asserting the pool is empty after a mid-batch rejection. **Owner-mismatch coverage (RUST-012).** The fail-closed owner check had only pure-function coverage. Added a privilege-aware integration test; it guards itself out where no foreign uid is available, so it proves the path only on a host that can actually stage one. Not done here: CALL-001 (`insert_platform_node_pool_entry` returning `Ok(())` for an unmanaged account) needs a new error variant plus an exhaustive-match update in `sqlite/util/wallet.rs`, outside this change's file scope. Carried to the batch that owns that file. Verified: platform-wallet-storage 826 passed, platform-wallet 940 passed; targeted re-run after the comment correction 25/25 (ledger 6a6dafb7741d15418f7bf3758dddf809); clippy clean with `--no-deps` on both crates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…Recovery losing an identity The `LoadSite` taxonomy had fused conditions that must stay distinguishable, and one Recovery path discarded data outright. **Recovery lost an identity (the reason this is not just hygiene).** On two live `identities` rows claiming the same `identity_index`, `HashMap::insert` returned the displaced `ManagedIdentity`, which was read for its id and then dropped. A Recovery load therefore returned a wallet with that identity's balance, revision, DPNS names, contested names, contact profiles, ignored senders and pre-keyed public keys entirely absent — and unrecoverably so, since Recovery makes the persister read-only, so nothing dropped at load can ever be re-persisted. Nothing in the rows establishes which identity truly owns the slot, so the loser is now moved to `out_of_wallet_identities`, the same bucket a NULL-`identity_index` row lands in, where it picks up the existing prekeying pass. Only one identity can hold the derivation slot; neither has to be thrown away. **Fused sites split.** `LoadSite` keyed a never-fatal benign site and a Strict-fatal corruption site to the same value, which broke `last_load_degradation`'s own documented invariant that a non-empty snapshot under Strict contains only never-fatal sites. Undecodable address scripts (`UndecodableAddressScript`), gap-limit maintenance failure (`RehydrationMaintainGapLimit`), and the three-way registration split (`AccountRegistrationDrift`, `ProviderKeyRegistrationDrift`, `ProviderKeyCurveMismatch`) now count independently, so `by_site` can say which condition fired rather than only that something did. **`tolerate_at` honours the contract it was violating.** It routed Strict-fatal incidents through `note_degraded`, whose rustdoc says "never fatal, in either policy" and whose log line says the row was accepted as-is, while dropping `error_kind` and sharing one message string across six call sites. It now has its own log record, keeps `error_kind`, and takes a site-specific message. The contract was kept and the code brought to it, rather than the wording widened to legitimise the violation. **Coverage that can actually fail.** `LoadSite::RehydrationEnsureDerived` had no behavioural coverage and now has Strict and Recovery cases. The identity-collision test asserted `identities.len() == 1`, codifying the data loss as intended behaviour; it now asserts the displaced identity survives, and was RED-checked — reverting the source line fails it with `got keys []`. The asset-lock sticky-consumed tests gained a live positive control, and `SiteCoords.wallet_id` became optional so an unreadable id logs as unknown instead of borrowing the unowned sentinel. Also `insert_platform_node_pool_entry` no longer returns `Ok(())` for a wallet with no managed provider-platform account — it returns a typed `NoManagedAccount`. Its only caller is guarded by an `entries.is_empty()` early return, so this is reached only when persisted pool rows exist for an account the wallet lacks, which is a genuine inconsistency rather than the ordinary no-provider-account case. Comment corrections where the code had moved on: the sticky-consumed note no longer claims to defend a production double-spend path (`load_state` is test-gated), the collision comment no longer claims an ordered read solved the loss, and `LoadSite::IdentityIndexCollision` no longer documents the identity as dropped. Verified: 838/838 tests with `--all-features` (ledger 0f755d12ee06ed0d70a58bb70fb62cd1); clippy clean `--all-targets --all-features --no-deps`; merged with the six sibling branches, 1948/1948 all-features (ledger e21517ea70e606fae67f0865a20f8af9). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…cript and close the wildcard message arms Follow-up to 201eaf3, from a second adversarial review of that change. Three of the four items are consequences of that commit rather than pre-existing defects. **An inconsistency the previous commit created (R5).** Splitting `LoadSite::UndecodableAddressScript` out made an undecodable persisted script Recovery-tolerable in `core_pool::load_used_addresses_with_ctx` and `core_state::load_used_addresses_with_ctx` — but `restore_provider_platform_node_pool` still aborted on the identical condition. Before that split all three were fatal, which was at least coherent; fixing two of three made the crate tolerate a bad script in two readers and die on it in a third. It now routes through the same site. Two things made this worth fixing rather than deferring. The error `?`-propagates out of `load()`'s per-wallet loop, so one damaged wallet's platform-node pool aborted the load of *every* wallet in the file — the opposite of what Recovery exists for. And the data at stake is the most disposable in the crate: a re-derivable cache of pre-derived platform-node public keys, with no funds, no identity and no address-reuse guard behind it. **Wildcard message arms (R2).** `recovery_message()` and `degradation_message()` ended in `_ =>` arms while `as_str()` was exhaustive, so a new `LoadSite` silently inherited a generic message with no compile error — and three of the four variants added in 201eaf3 landed on exactly that arm. Both matches are now exhaustive with the generic text kept in an explicit or-pattern, so adding a site fails to compile until its author says which message it takes. For `degradation_message` that decision is load-bearing: it is where a Strict-fatal site would otherwise quietly acquire an "accepted as-is" wording. **A fixture that could stop testing anything (R4).** The identity-collision fixture skews `sqlite_stat1` so the planner prefers the non-covering index, which is what makes an unordered read return insertion order. Sensitivity therefore rests on planner behaviour, so a future SQLite that ignored the skew would make the test pass for a reason unrelated to the `ORDER BY` it protects — silently, looking exactly like success. The fixture now asserts the unordered read really does yield insertion order, and says plainly that production never runs `ANALYZE`, so the clause guards against a future planner rather than fixing a live bug. **A claim softened to match the evidence (R1).** The note on `maintain_gap_limit`'s error branch asserted it had "no reachable seed from a persisted row". A sibling fixture has since shown that class of fixture is buildable, so the claim is downgraded to "no *known* seed" and carries the specific untried construction lead, with an explicit instruction not to restore the flat "unreachable" wording on assumption. Known gap, stated rather than papered over: the newly tolerable provider-platform path has no fixture of its own. Strict behaviour is unchanged (`tolerate_at` still returns the error), so a regression could only affect Recovery, but this commit does not prove the Recovery branch. Verified: 838/838 with `--all-features` (ledger 8c8a4388d3f8b93d9e0ff9184c279a4d); clippy clean `--all-targets --all-features --no-deps`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gg89yBFhBWr4UQ8W3fP899
…-storage-rehydration
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Adds a durable, embeddable SQLite storage backend for Dash Platform wallet state, so identities, contacts, keys, and balances survive an app restart instead of being lost or re-derived from scratch.
User story
As a Dash Platform wallet developer, I want my wallet's Platform data (identities, contacts, keys, balances, core sync progress) to survive an app/node restart, so that users don't lose data or sit through a full re-sync every time the app starts.
Scenario
Base flow
A Dash Platform wallet app registers identities and contacts, tracks balances and asset locks, and derives Platform payment addresses as the user interacts with it.
Actual behavior
platform-walletdefines the persistence trait (PlatformWalletPersistence) and the manager-sideload_from_persistor()entry point, but ships no production storage backend. Restart the app and its Platform identities and contacts are gone until re-derived, the address-reuse guard resets, and core sync restarts from scratch — no on-disk durability, no backup/restore, no schema-migration path.Expected behavior
That state is durably persisted to a local SQLite database (one
.dbfile can hold many wallets), with online backup/restore and automatic schema migration. Restarting the app restores everything seedlessly and signing works immediately post-load — and no private-key material is ever written to the database (signing material stays in the OS keyring or an encrypted vault).Detailed discussion
Adds
rs-platform-wallet-storage— a self-contained, embeddable SQLite backend implementingPlatformWalletPersistence(Arc<dyn PlatformWalletPersistence>,Send + Sync, object-safe). One.dbfile holds many wallets.Persistence —
SqlitePersistersupports configurable journal/synchronous/flush modes, a retention policy, auto-backup, and online backup/restore.load()reconstructs each wallet external-signable with no seed required, then layers the persisted core-state projection (UTXOs, sync watermarks, chainlock, address-pool depth); prekeyed identity/contact joins mean signing works immediately post-load. Any row that fails to decode, or an out-of-rangewallet_id, fails the wholeload()call — no silent per-row skip.Load failure policy —
SqlitePersisterConfig::with_load_policyselectsLoadPolicy::Strict(default) orLoadPolicy::Recovery. UnderStrict, any inconsistency in persisted rows aborts the wholeload(), so a half-formed wallet is never handed to the caller.Recoveryis an opt-in rescue mode reproducing the previous best-effort behaviour: tolerable inconsistencies are logged and counted rather than returned, and the persister becomes read-only —store,flush,commit_writes,delete_wallet, the KV writers andprune_backupsall refuse withReadOnlyRecoveryMode, so a degraded projection can never be written back over good rows.backup_tostays available, since snapshotting is the first thing a rescuing user should do. Every lenient site funnels through a singleLoadCtx::toleratechoke point; per-site counts and adegradedflag are exposed viaSqlitePersister::last_load_degradation(), replaced perload()rather than accumulated. Open-time gates (PRAGMA integrity_check, schema-version and foreign-keys checks) stay unconditionally hard in both modes —open()runs migrations, and migrating a structurally corrupt file amplifies the damage.Two rehydration signals are counted as degraded but never fatal in either mode, because neither can distinguish corruption from a healthy wallet: a used address owned by a non-funding (provider) account has no funds account to route to, and a legitimately deep-and-sparse address is indistinguishable from a foreign one past the bounded-derivation cap. Making either fatal would refuse to open wallets that are perfectly sound. A separate, size-based guard (
MAX_REHYDRATION_GAP_REFILL) also bounds the work a single gap-limit refill can imply, so a corrupted or adversarial pool state can't force an unbounded address-generation loop.Two behaviour changes worth calling out: the
core_transactionssoft column-repairUPDATEthat previously ran during a read is deleted outright (a&selftrait read must not mutate the DB), and an oversize chain-lock blob now hard-errors inRecoverywhere it was previously swallowed asNone— fail-closed takes precedence over bug-for-bug compatibility.Schema (refinery migrations V001–V016, additive) — per-wallet tables keyed by
wallet_idwith native cascading foreign keys: accounts, identities and their keys (structurally enforced co-ownership), platform addresses,core_address_pool(per-index pool with reservation timestamps and pre-derived platform-node keys), asset locks (status includesrecovered_from_chainfor restore-scan reconstruction), DIP-13 invitations, shielded viewing keys (shieldedfeature), anddpns_name_statesfor the wallet-level DPNS marketplace. Full ER diagrams: SCHEMA.md.Database trust model — the wallet
.dbis trusted local state, not untrusted input. It lives in the host application's own private directory, owned by the same user at the same privilege level as the process reading it; an adversary who can write arbitrary rows into it already has same-privilege local code execution, at which point process memory, keyring entries and the vault passphrase prompt are all equally reachable. Threat models premised on an attacker-authored database are therefore out of scope for this crate — see theDatabase trust modelsection in README.md.The read path is nonetheless defensive, deliberately, against corruption rather than attack: layered size limits (16 MiB per-value cap, bounded bincode decode, 32 MiB connection backstop) keep a truncated or bit-rotted blob from becoming an unsurvivable allocation; typed columns are cross-checked against their decoded BLOB counterparts on every read, so a partially-applied write is caught rather than trusted; key/identity co-ownership is structurally enforced (compound FK plus a trigger fallback where SQLite's own FK check goes dormant on NULL columns); and
PRAGMA integrity_checkplusPRAGMA foreign_key_checkrun unconditionally at open under both load policies. Each of those bounds the blast radius of a bug, a crash mid-write, or failing hardware. None is a security control and none should be cited as one.Secrets —
SecretStore/EncryptedFileStore(Argon2id KDF + XChaCha20-Poly1305 AEAD,zeroized, overkeyring-core). No private-key material is ever written to the wallet.db.Outside the storage crate (historical — moved out, see scope note above) — this PR previously also carried:
platform-wallettyped persister errors + transient retry (now #4586), a contact-account filter-scan fix and broadcast-timeout fix (now #4587),platform-wallet-ffipersister result codes and the asset-lock proof size gate (now #4587 and #4585 respectively), and assorted Swift cleanup (now #4587). Norust-dashcorepin bump is actually part of this PR's diff — that line in an earlier revision of this description was inaccurate. The load-policy work in this update stayed entirely insiders-platform-wallet-storagethroughout.Cargo feature-unification fix —
cargo check --workspacewas broken pre-existing (acknowledged but unowned since this PR's original description):PlatformWalletChangeSet.shieldedwas#[cfg(feature = "shielded")]onplatform-wallet's own flag, but a downstream crate can't cfg on a dependency's feature —rs-unified-sdk-jnienablesshieldedby default, so any workspace-wide build unified the field into existence whileplatform-wallet-storage's exhaustivetouched_domainsdestructure (the R8 forgotten-domain guard) had it cfg'd away, producing E0027. Fixed by making the field exist unconditionally inplatform-wallet(an inert placeholder swapped in via cfg when the feature is off, byte-identical wire format either way), so every downstream destructure —touched_domainsandwallet/apply.rs::apply_changeset— names the field in every feature combination with no..anywhere; the exhaustive guard is fully intact. This unblocks downstream consumers (e.g. dash-evo-tool) from bumping their pinnedplatformcommit without inheriting the same break.Post-merge regression fixes — the
v4.2-devmerge silently reintroduced two things this branch had already fixed, both caught by a subsequent independent review-comment audit against live code rather than by the merge itself: a migration-version collision (this branch'sV006__pool_reserved_at.rsvs.v4.2-dev'sV006__tracked_masternodes.rsfrom #4465 — renumbered the latter toV013, refreshed the two pinned schema fingerprints insqlite_schema_pinning.rs), and 6 reintroducedINSERT INTO wallet_metadatatest-fixture statements indpns_name_states.rsreferencing a table this branch had already renamed towallets(from #4423, authored onv4.2-devbefore the rename existed there). Neither is a new defect in this PR's own work — both are artifacts of merging a moving base branch, now fixed and re-verified (fullplatform-wallet-storagesuite green, default features and--features shielded).Second
v4.2-devmerge-base pass — a follow-up merge (21 more upstream commits) surfaced a third instance of the same pattern, this time a hard compile error rather than a silent regression: upstream #4451 claimed FFI error discriminant42forErrorMasternodeWithdrawalUnconfirmedwhile this branch already held it asErrorPersisterTransient, producing a duplicate-discriminant compile failure inplatform-wallet-ffiinvisible toplatform-wallet-storage's own suite (that crate doesn't depend on FFI). The persister-code renumbering that resolved this (nowErrorPersisterFatal = 49/ErrorPersisterTransient = 50, not the48/49an earlier revision of this description claimed) shipped as part of the FFI work that has since moved to #4587 — this PR's own diff no longer touchesplatform-wallet-ffiat all.Review triage — an independent pass verifying every open review thread against current code (not against reply text claiming a fix) found 14 threads needing attention beyond a bot's "resolved" claim. Triaged and actioned:
V014, read-path cross-checks, transaction handling):identities::load_statenow fetches and cross-checksidentity_index, and separately detects a duplicate-slot collision that was previously silently dropping an identity on load (fails closed underLoadPolicy::Strict, counted/degraded underRecovery— never silent either way); orphanedidentity_keysrows are re-scoped when an identity'swallet_idpromotes from NULL;Recovery-modeget_tx_recordnow declines a txid-drifted record instead of returning the wrong one; the NULL-scope trigger now also rejects a key naming no identity at all, not just a wallet-owned one (V014, since editing the already-appliedV001wouldn't reach existing databases);identity_keys::load_statecross-checkspublic_key_hash;run_integrity_checknow also runsPRAGMA foreign_key_check.INTENTIONAL(...)at each site, no code change):route_to_funds_account'sowner: None => 0fallback, the OS-keyringreprotectnon-atomic get/set,restore_from's WAL/SHM cleanup,do_write_vault_at's fsync-failure tolerance, and the unconstrainedcore_address_pool.account_typecolumn.key_wallet::insert_walletrecomputeswallet_id(external crate,TODO(insert-wallet-id-recompute)left at the call site) — see also#3992.route_to_funds_accounttrade-off above (confirmed a mixed-store restore genuinely can reach theNone-owner path, mitigated by the fallback being counted/logged and the exposure window bounded to before-first-sync), and a Kotlin/Swift consumer-sideshutdown()-on-load-failure question (confirmed for this crate's own code; the broader claim lives in consumer repos this PR doesn't touch).Also fixed in the same pass:
#4441's doc comment claiming a wallet with a pre-existing duplicate index "already fails to load" — it did not; that was the false premise the identity-index fix above closes.Deferred — manifest authentication (a MAC binding the persisted manifest to its
wallet_id, #3992); orphaned wallet rows from a crash between wallet creation and first-account registration (rehydrate harmlessly, no eviction path yet); address-reservation release/sweep (reserved_atis persisted but nothing consumes it yet, #4188). Recovery mode has no human-facing surface yet: no FFI entry point constructs aSqlitePersister(platform-wallet-ffidoes not depend on the storage crate), and the maintenance CLI intentionally has no--recoveryflag because none of its subcommands callload(). Both gaps carryTODO(recovery-mode):markers insrc/sqlite/config.rs. Two rehydration derivation sites are kept fail-closed but have no direct fatal/tolerated test pair, since a key-derivation failure cannot be induced on a watch-only xpub path for a pool that already holds the address;NOTE(recovery-mode):marks each site inutil/wallet.rs.Stacked PR: #4496 (based on this branch) replaces identity soft-delete (
tombstonedflag) with a realDELETE, closing the orphaned-metadata gap the tombstone design left — see that PR for detail.Independent review round (2026-09-03) — a multi-agent review of this branch produced 74 findings, which were triaged by the maintainer and actioned below. Two of the fixes were themselves reverted or corrected after review, which is noted where it happened.
Persistence-trait scope.
delete_walletis gone fromPlatformWalletPersistence, along with thePersistenceError::UnsupportedOperationvariant that existed only to give it a default body. A staleTODOintraits.rs— written two months afterlist_walletshad already been deleted — had been cited as evidence of a planned cross-backend contract, and a method was added on the strength of it. The SQLite backend keepsdelete_walletas an inherent method with its full typed report; the trait no longer advertises an operation whose cross-backend semantics were never agreed.Persisted domain labels.
Domain::WalletMetadataandDomain::AccountAddressPoolswrote the stringswallet_metadataandaccount_address_poolsintometa_data_versions.domain. Neither names a table that exists on this schema baseline, and both were on the retired-name list the schema-pinning test guards. Because those labels are the cache-invalidation key hosts read, a bare rename would have silently reset every affected domain'sseqto zero — so migration V016 rewrites already-persisted rows, takingMAX(seq)on collision to preserve the monotonic invariant.Load-failure policy. The
LoadSitetaxonomy was over-fused: several distinct conditions shared one site key, including a never-fatal benign site and a Strict-fatal corruption site, which brokelast_load_degradation's own documented guarantee that a non-empty snapshot under Strict contains only never-fatal sites. Undecodable address scripts, gap-limit maintenance failures, ECDSA registration drift, provider typed-column drift and provider curve mismatch are now separate sites with separate counters.tolerate_atno longer routes Strict-fatal incidents throughnote_degraded, whose contract is "never fatal, in either policy"; it keeps its own log record with the error kind preserved. Both message-selecting matches are exhaustive rather than wildcard-terminated, so a new site cannot silently inherit a description that misstates it. An undecodable persisted script is now tolerable in all three readers that meet it, not two of three — the third aborted the load of every wallet in the file over what is a re-derivable cache of pre-derived public keys.The same pass closed a data-loss defect in Recovery mode: on two live
identitiesrows claiming oneidentity_index, the displaced identity — balance, revision, DPNS names, contact profiles, pre-keyed public keys — was read for its id and then discarded, so a Recovery load returned a wallet with an identity silently missing. Since nothing persisted establishes which row truly owns the slot, and Recovery makes the persister read-only so nothing dropped could ever be re-persisted, the displaced identity is now moved toout_of_wallet_identitiesinstead of dropped.Vault hardening. A pre-existing vault file's permissions were checked but its owner never was, so a
0600file belonging to another local user was accepted on its mode bits alone; ownership is now verified against the process's effective uid. Keyless vaults now document that they provide neither confidentiality nor authenticity — anyone who can write the file can forge one. Oversized OS-keyring blobs are zeroized before their error returns, and the permission errors name their path and print the command that fixes them.Deliberately not done: clamping a vault header's Argon2 parameters to the shipped default target. Per-vault parameters exist so a vault may be hardened above the default, and clamping reads to the current default would make every such vault — and every vault at all, were the default ever lowered — permanently unopenable. The DoS control already exists and already gates before the allocator:
enforce_boundsbounds the header to 19 MiB..=1 GiB insidederive_key. Two tests now pin both directions.Connection and migration hardening.
open_conn's read-write arm used flags that includeSQLITE_OPEN_URI, so a path could smuggle query parameters that defeat the caller's intent, while the doc comment claimed URI parsing was off. It now passes explicit flags and rejects any path beginningfile:, because the bundled SQLite can enable URI parsing regardless of open flags. Therefinery_schema_historyguard — which exists because refinery parsesapplied_onwithunwrap(), aborting the process on a malformed value — now validates refinery's canonical shape rather than any RFC3339 string, so the panic is no longer reachable through a value refinery itself would reject.Provider-key pool atomicity.
populate_platform_node_poolconverted each child index inside its mutation loop, so an invalid index halfway through a batch left earlier keys already inserted; every index is now validated before any mutation.insert_platform_node_pool_entryno longer returnsOk(())for an unmanaged account, which silently did nothing — it returns a typedNoManagedAccount.Documentation corrected against the code. A
Database trust modelsection now states the crate's actual position: the wallet.dbis trusted local state, an attacker-authored database is out of scope, and the read-path defenses exist against corruption, crashes mid-write and failing hardware — not against an adversary, and are not to be cited as security controls. Three sites that justified a design decision by citing a--no-default-features --features sqlite,cliCI build were reworded, because no workflow runs it; the arrangement they justify is still correct, only the claimed enforcement was fictional. Theshieldedfeature now impliessqliteand uses the optional-dependency form, fixing a combination that enabled the dependency without the persister that consumes it.Out of scope, by decision. Bumping SQLite past the
refinery-corerusqliteceiling is blocked upstream by refinery PR #445 and alinks = "sqlite3"single-version constraint; it is not attempted here. Two structural refactors — extractingload()'s phases and splittingextend_pools_for_restored_addresses— were reviewed and deliberately dropped from this PR's scope, withTODOmarkers left at both sites.Testing
cargo check --workspace --all-targetsclean, zero warnings,platform-wallet-storageconfirmed in the compilation set;cargo fmt --all -- --checkclean. The counts below are pre-trim and include coverage for work that has since moved to fix(platform-wallet): typed persister errors with caller-visible retry classification #4586/feat(platform-wallet-ffi): persister error codes, contact-account fix, and cleanup #4587/fix(platform-wallet-ffi): bound bincode decode size on asset-lock proof bytes #4585 — retained for history.cargo clippy/cargo test --all-featuresclean acrossplatform-wallet,platform-wallet-storage,platform-wallet-ffi: 1683 passed / 0 failed / 6 skipped.platform-wallet-storagestandalone, post load-policy work,--all-features: 743 passed / 0 failed / 2 ignored (up from 726 pre-load-policy — net new coverage forLoadPolicy::{Strict,Recovery}, write-blocking, and the degraded-load surface). Independently reviewed by an adversarial QA pass across relocated tests, every tolerate-site, write blocking (proven by an execution-based whole-DB fingerprint, not just assertions), policy-branch leakage, and the per-load degraded-count semantics: 7 findings, all low/informational, all fixed.extern "C"surface.identitieshas no uniqueness constraint on(wallet_id, identity_index), andload_from_persistor's failure path isn't recoverable by the shipped Swift reference caller.cargo check --workspaceproven 101 → 0 across the fix (forced a real online rebuild of thedash-networkFFI build script, not a cached no-op, to make the before/after honest).cargo clippy -p platform-wallet-storage --features platform-wallet/shielded -- -D warningsand-p platform-wallet -p platform-wallet-storage --all-targetsclean in every feature combination (shieldedon and off, both crates); the R8 domain-coverage test (tc_b_013_every_domain_maps_and_isolates) passes in both configs, confirming the guard survived.UNIQUE constraint failed: refinery_schema_history.version), fixed, then re-ran the fullplatform-wallet-storagesuite (default features and--features shielded) clean, including the schema-pinning (tc_b_040_*) and retired-table-name (tc_b_041_*) guards with refreshed golden fingerprints.platform-wallet-storage784 passed / 0 failed,platform-wallet783 passed / 0 failed, clippy clean (-D warnings) on both libs and every touched test target.Breaking changes
The PR title is deliberately left unmarked:
platform-wallet-storageis unreleased with zeroexternal Rust consumers, and every break below is either inside that crate or inside a trait
whose only implementor ships in this same repository. Listed explicitly so a future consumer
bisecting the history is not surprised.
3df58ddc12 refactor(platform-wallet-storage)!removes three public Cargo features:secret-serde,secret-schemars,rehydration-apply. Naming any of them in a dependencydeclaration stops resolving.
ef32f6fad0 fix(platform-wallet-storage)!renames twoDomainvariants —WalletMetadata→Wallets,AccountAddressPools→CoreAddressPool— so each label namesits live SQL table. Because those strings are the persisted cache-invalidation key, migration
V016 rewrites already-stored
meta_data_versions.domainrows, takingMAX(seq)oncollision so the monotonic invariant survives. A bare rename would have silently reset every
affected domain's
seqto 0.1479d147e9 refactor(platform-wallet)!dropsdelete_walletfromPlatformWalletPersistencealong with thePersistenceError::UnsupportedOperationvariantthat existed only to give it a default body. The SQLite backend keeps
delete_walletas aninherent method with its full typed report; the trait no longer advertises an operation no
cross-backend contract had been agreed for. Any out-of-tree implementor overriding
delete_walletnow has a dead method; any caller invoking it throughArc<dyn PlatformWalletPersistence>must reach for the concrete backend.LoadPolicy::Strictis the new defaultload()behaviour. Previously-tolerated rowinconsistencies now abort the whole load.
LoadPolicy::Recoveryreproduces the oldbest-effort behaviour, at the cost of the persister becoming read-only.
PersistenceErrorandPlatformWalletErrorboth gain variants and are deliberately not#[non_exhaustive]— consistent withPersistenceErrorKind, whose rationale is documented inREADME.md: a future
variant must force every consumer
matchto update explicitly rather than fall through awildcard arm.
Checklist
For repository code-owners and collaborators only
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes