feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration - #3968
Open
Claudius-Maginificent wants to merge 290 commits into
Open
feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration#3968Claudius-Maginificent wants to merge 290 commits into
Claudius-Maginificent wants to merge 290 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
|
⛔ Blockers found — Opus deferred (commit 4784de0) |
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.21% 86.69% -0.53%
============================================
Files 2729 2700 -29
Lines 347524 344997 -2527
============================================
- Hits 303110 299107 -4003
- Misses 44414 45890 +1476
🚀 New features to boost your workflow:
|
…ing; doc irreducible xpriv transient Reorder the extended_public_key test so each field-level assert (public_key, chain_code, depth, network) runs before the full-struct assert_eq!, which otherwise short-circuits and leaves the per-field metadata checks unreachable on a regression (i.e. vacuous). Proven non-vacuous: temporarily corrupting only `depth` in the impl fails the test at "depth must match", where a pubkey-only check would have passed. Also document the one irreducible transient in resolve_and_derive: key-wallet's ExtendedPrivKey::derive_priv returns by value (Copy), so its return slot holds an un-wiped copy until the same-line Zeroizing::new takes ownership — noted so the zeroization contract doesn't over-promise "zero copies ever". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…edPrivKey on Drop) Advances the 8 rust-dashcore workspace deps from rev f42498e0d04257e28b4e457c16629904a872ab61 to a8c57fe863c96ac9c7e33833549e7a4f75ac9b5e (PR #833 head). The new commit drops `Copy` from `ExtendedPrivKey` and gives it a `Drop` impl that zeroizes its secret material on scope exit — so the key wipes itself automatically instead of relying on caller-side wrappers, and non-`Copy` moves leave no stray bitwise duplicate behind. No workspace call sites relied on `ExtendedPrivKey: Copy`: the Copy-removal impact is absorbed upstream in `bip32::derive_priv` (clones `self` instead of `*self`), so `cargo check --workspace --all-targets` is clean with no source changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…roizing wrap `ExtendedPrivKey` now zeroizes on `Drop` (rust-dashcore rev a8c57fe863c96ac9c7e33833549e7a4f75ac9b5e), so wrapping the master and derived keys in `Zeroizing` inside `resolve_and_derive` only bought a harmless double-wipe. Unwrap both and let the type's own `Drop` do the work; `Zeroizing` stays on the plain byte buffers that have no `Drop` of their own (mnemonic buffer, BIP-39 seed, final 32-byte scalar). Rewrites the module `# Zeroization` block and the `resolve_and_derive` contract to the present three-mechanism reality (self-wiping key + Zeroizing buffers + non_secure_erase on SecretKey). The previously documented "irreducible transient" caveat is fully closed: a non-`Copy` move leaves no bitwise duplicate, so there is no un-wiped return slot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ashcore-dev # Conflicts: # Cargo.lock # Cargo.toml
…unseedable too RehydrationGapLimit and RehydrationEnsureDerived are the only two of ten LoadSite variants with no test reference anywhere; only ensure_derived carried the marker explaining why. Someone reading the gap-limit site found nothing and would reasonably assume it was covered. The reasoning is per-site, not shared boilerplate: this site is unreachable because the key_source that reaches it is the one whose xpub already derived the pool, so failing it needs a key source that derives some indices and not others. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ap refill `maintain_gap_limit`'s call site in the mark/refill fixpoint had no bound on how much generation a pool's high-water marks could imply, unlike the discovery scan feeding `ensure_derived` (capped at MAX_REHYDRATION_DERIVATION_INDEX). A pool carrying `highest_used` far past `highest_generated` implies generating that whole span; at ~2.1e9 that exhausts host memory. Compute the refill target before the call — mirroring the private formula in key-wallet rev 173ffac, saturating so an overflowing pool over-estimates and fails closed — and route an over-cap span through the existing tolerate/RehydrationDerivationFailed path: fatal under Strict, deferred with a short-window warning under Recovery, same LoadSite as a refill that fails outright. The check is pure arithmetic ahead of the call, so a rejected pool derives nothing. MAX_REHYDRATION_GAP_REFILL is 250_000, roughly a 100MB ceiling at ~500 bytes per generated address (an AddressInfo plus its three pool index entries, two of which clone the Address/ScriptBuf). Legitimate refills span one gap window — tens of addresses — so the cap sits four orders of magnitude above anything real. Note this is defense in depth, not a currently reachable path: `mark_used` is the only writer of `highest_used` today and can only name an already-generated index, so `highest_used <= highest_generated` holds across the load path and the implied span is bounded by `gap_limit`. The cap guards against that upstream invariant breaking — e.g. key-wallet gaining a persisted-pool-state constructor. The durable fix is a read-only `refill_target()` on `AddressPool` upstream, which would remove the duplicated formula. Three unit tests cover the guard: fatal under Strict, deferred-and-counted under Recovery with `highest_generated` provably unchanged, and a deep pool (used 50_000, generated 49_990) refilling normally — the last verifying the cap bounds the refill's span rather than the depth it starts from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…est with its doc A test added between an existing doc comment and the test it documented left `rehydration_routes_used_address_to_owning_account` — the #3968 regression guarding the CoinJoin address-reuse privacy leak — with no doc at all, and fused its rationale onto a neighbour that tests owner routing for a different reason entirely. A privacy-leak regression test that no longer explains why it exists is a test someone deletes during a future cleanup. Move the newer orphaned-owner test and its `first_external_address` helper below the older test so each doc sits on the test it describes. No test body changes; ordering and comments only.
…on derivation error
`RehydrationDerivationFailed` erased three unrelated failures into one
variant with a `stage: &'static str` discriminant, an `index: Option<u32>`
that only one stage ever fills, and a `cause: String` — while the module
doc one screen above promises variants carry the upstream error via
`#[source]`, "never a stringified copy". Two incompatible source types is
an argument for separate variants, not for stringifying both.
One variant per failure, each carrying only what it has:
- `RehydrationEnsureDerivedFailed { index }` — the slot has no upstream
error, only "the pool produced no address here", which is the variant.
- `RehydrationGapLimitRefillTooLarge { refill_target, generated, implied,
cap }` — typed numbers instead of a formatted sentence, so a caller can
compare them.
- `RehydrationGapLimitFailed { source }` — a real `#[source]` carrying the
upstream `key_wallet` error, so `Error::source()` walking reaches it.
Each `Display` now states the consequence a rescue operator needs — a
restored address that may be handed out again as a fresh receive address —
rather than naming a private Rust function. Both wildcard-free enum tables
walked the compiler through every new arm.
…reads Every recovery-mode error variant's rustdoc states both the consequence and the way out; none of that reached `Display`, which is what shows up in a log at three in the morning. A reader of the rendered string learned that a write was refused but not that repairing the database and reopening it strictly is the exit, and that an unowned identity holds a registration index but not that the index is the thing to clear. Add the actionable clause to `ReadOnlyRecoveryMode` (both the persister's and the KV store's), `UsedAddressOwnerConflict` and `UnownedIdentityHasRegistrationIndex`, and pin the two blocked-write messages with a test so the exit cannot be edited away.
`by_site` counts occurrences at nine of its ten sites; at `TombstonedIdentityOrphan` it counted tolerate-calls, because `route_by_owner` rightly decides once per collection after its walk instead of logging inside it. Five thousand leftover key rows therefore read as `1`, and a rescue operator reading the tally could not tell three lost keys from three lost collections — the doc said "row", the code meant "collection", and nothing said which. Keep the single log record, move the count: `LoadCtx::tolerate_many` takes the occurrence count the walk already tallied, so one record can carry many counts and every site means the same thing again. The shipped test could not tell the two apart (one key row plus one contact row is two either way); the new one leaves three key rows under a single owner.
…incident Splitting the failure from its coordinates stopped the duplicate logging but left both halves unusable alone: the record from `tolerate` names the site with no wallet, account or pool, and the site's follow-up names the pool with no `site` to join it back by. "skipped shielded viewing-key row located here" — located where? Correlating them relied on the two lines being adjacent, which a concurrent load or a `site=` filter breaks. Every follow-up record now carries `site`, so a filter on one field still returns both halves. `warn_deferred_pool` takes the site it is reporting rather than assuming its caller. The two `note_degraded` callers were worse: they pre-`format!`ed wallet_id, account type and counts into one opaque string, dropping structured `tracing` fields the code had before this PR. They now pass `SiteCoords`, so a single complete record carries the fields and the cause is prose again.
`tolerate` emitted `site=unowned_identity_registration_index` beside `error_kind=unowned_identity_has_registration_index` for the same incident, so grepping for either name missed half the evidence. The site takes the error's spelling; the other nine already agree with theirs.
…one owner `total == sum(by_site)` and `degraded == !by_site.is_empty()` are stated in the type's own field docs and were then computed by hand in two unrelated places — a third writer, or any change to what `degraded` means, drifts silently and nothing on the type notices. `LoadDegradation::merge` folds a tally and re-derives both fields through one private constructor. `LoadCtx::degradation` and the persister's snapshot merge now both go through it; the persister's copy shrinks to one call.
…n use `LoadCtx` was exported at the crate root while every function that accepts one is `pub(crate)` in a production build, so an embedder could construct one, ask it nothing useful, and never pass it anywhere. The crate already documents the treatment for exactly this: `schema` and `migrations` are `pub(crate)` in production and widened by `__test-helpers`. `load_ctx` follows them, plus `rehydration-apply` — that feature's `apply_persisted_core_state` is public and takes a `&LoadCtx`, so the type is public exactly when a caller that can pass one exists. `LoadSite` and `LoadDegradation` stay unconditional: `last_load_degradation()` returns them. Narrowing the module made two conveniences and one accessor visibly dead in a production build. `strict` / `recovery` are gated with the module; `policy()` had no caller anywhere in the crate or its tests and is gone.
…he real schema `LOAD_UNIMPLEMENTED_TABLES` hardcodes three physical table names a second time, hand-kept in lockstep with the two logical entries of `LOAD_UNIMPLEMENTED`, with nothing pinning either to the migrations. A table renamed in a migration would first show up as a failing probe on a user's database. Deriving the list from `schema::versions::Domain` would look tidier and be wrong: `Domain::as_str` is the `meta_data_versions` label, and its equality with a table name is a coincidence of these three (`Core` is `"core"`, whose table is `core_sync_state`). Assert against the migrated schema instead — verified red by renaming one entry. While here, say why the row count saturates instead of using `safe_cast`.
Three readers now carry a `LoadCtx` and each treats the resulting tally differently: `load()` replaces the snapshot, `load_unowned_identities` merges into it, and `get_core_tx_record` drops it — the only one of the three the rustdoc did not mention. In recovery mode that means drift the persister just tolerated and logged leaves `is_degraded()` false, which reads as a bug until you know it is a choice. It is a choice: one tally per transaction folded into a per-load snapshot would grow without bound and stop describing the load. Documented at `last_load_degradation` and at the call site.
…t TODO Both derivation sites are defensive branches with no reachable seed from a persisted row — the probe resolved the index from the same xpub the pool derives from, so no stored row produces the failure. `TODO` invites a future reader to resolve something that cannot be resolved; `NOTE` says what is meant and greps the same. The two markers in `config.rs` stay `TODO`: an FFI entry point and a human-facing recovery surface are both real work someone will do.
`highest_generated` is an index, so `None` means the pool holds no addresses — but the refill guard read it as index 0, i.e. one address already generated. It therefore under-counted the implied work by exactly one, in a guard whose stated contract is to over-estimate and fail closed, and reported `generated: 0` for a pool that has generated nothing. The correct shape is two functions below in `ensure_derived`, which starts at 0 for `None`. Counting addresses rather than indices removes the branch: a pool holds `highest_generated + 1` addresses, or none, and the refill owes `target + 1 - already_generated` either way. The arithmetic moves into `ImpliedRefill` so it can be tested without fabricating pool state a real load cannot produce — `highest_used <= highest_generated` holds on every load path, so the empty-pool case is unreachable through the pools themselves and was untestable in place. Verified red: the empty-pool case returned 19 against the old expression, 20 against this one. The error carries `already_generated` instead of the index, so its message stops naming an address that does not exist.
…ects `note_degraded` hard-coded a count of one while both its callers had already computed the real size of the damage and were passing it in for logging, where it was dropped before reaching the tally. Nine hundred unresolved addresses reported `total = 1` — the same defect the tombstoned site had, on the sites where the number is largest. The count comes from `SiteCoords::affected`, floored at one so an incident is never keyed with nothing behind it, and the two site docs say what they count. Every existing test at these sites seeds exactly one address and so cannot tell the two rules apart; the new one seeds two and was verified red at `Some(1)` against the hard-coded count.
…g tested Three claims in the shipped rustdoc had no test behind them: - `get_core_tx_record` tolerates drift without tallying it. Untested, that reads as a bug rather than the bounded-growth decision it is; now pinned both on a persister that never loaded and against the snapshot a load left. - "a database restored from a backup and reloaded clean reports clean". The existing test loads the same dirty database twice, which cannot tell replacement apart from "keep whichever was worse". This one repairs the row out-of-band between the two loads. - "a `load()` that returns `Err` leaves it empty". `BlobTooLarge` is the one inconsistency recovery mode still refuses, so a tolerable wallet ordered ahead of an oversize one proves the tally counted on the way there is discarded rather than half-reported.
thepastaclaw
suggested changes
Aug 20, 2026
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The persistence backend has strong typed-error and bounded-decoding foundations, and the prior C ABI and mutable-dependency defects are fixed. Six blocking correctness issues remain in manager lifecycle, identity/key rehydration, transaction lookup integrity, and historical address attribution; four additional storage and secret-handling suggestions remain valid. Source: Codex reviewer lanes codex-general, codex-security-auditor, and codex-rust-quality: gpt-5.6-sol; verifier: gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 6 blocking | 🟡 4 suggestion(s)
10 additional finding(s) omitted from inline comments because GitHub refused gh pr diff (PullRequest.diff too_large); included below as unmapped findings.
10 unmapped finding(s)
1. [blocking] [carried-forward: prior-manager-load-shutdown] Keep the retained manager operational after load failure
packages/rs-platform-wallet/src/manager/load.rs:42-43
The exhausted persister-retry path still calls self.shutdown().await, and the hydration rollback repeats that call at line 227. shutdown() is terminal: it stops SPV and the synchronization coordinators, cancels the event-adapter token, and consumes the adapter's sole join handle. Only manager construction spawns that adapter. The shipped Swift WalletManagerStore.activate catches the load exception and then caches and exposes the same manager, so later wallet operations use a valid-looking handle whose wallet events no longer reach persistence. Keep the manager operational after an API-level load failure, make every stopped component restartable, or invalidate the handle so callers must reconstruct it.
2. [blocking] Reject duplicate identity derivation indices during rehydration
packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-201
The schema permits multiple live identities to share the same non-NULL (wallet_id, identity_index). identities::load_state also omits the typed identity_index column from its query and inserts decoded identities into a map keyed by entry.identity_index without checking whether BTreeMap::insert replaced an existing value. A later row can therefore silently remove an identity from a successful load, or cause a subsequent key/contact merge to fail as orphaned. Add a partial unique index for live non-NULL wallet/index pairs, cross-check the typed index against the decoded entry, and reject an occupied map key.
3. [blocking] Do not return a different transaction from a txid lookup
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:574-578
get_tx_record is a point lookup for the supplied txid, but a decoded record containing another txid is only logged and best-effort repaired before the mismatching record is returned. The repair may collide or fail, and even a successful repair does not change the current call returning transaction B for a request for transaction A. Callers can consequently consume B's finality or chain-lock context while handling A. Return CoreTransactionEntryMismatch for a txid mismatch and keep any repair in a separate maintenance operation that cannot violate the lookup contract.
4. [blocking] Reject orphan keys in the unowned scope
packages/rs-platform-wallet-storage/migrations/V001__initial.rs:256-280
When identity_keys.wallet_id is NULL, SQLite MATCH SIMPLE disables both foreign keys. These INSERT and UPDATE triggers abort only when a matching wallet-owned identity exists. If no identity with NEW.identity_id exists, the EXISTS predicate is false and the orphan key is accepted. A key-only write under the all-zero scope can therefore report success but later make load_unowned_identities() fail with OrphanedIdentityEntry. Both triggers must positively require a matching parent with wallet_id IS NULL.
5. [blocking] Move persisted keys when promoting an unowned identity
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:40-47
This upsert promotes an identity by changing identities.wallet_id from NULL to the incoming wallet, but existing NULL-scoped identity_keys rows are not moved. Their compound foreign key remains dormant while the child scope is NULL, and there is no parent-update trigger or ON UPDATE action. A scalar-only identity flush can therefore promote the parent while leaving its keys behind: the wallet loader restores an identity without its signing-key metadata, while the unowned loader sees keys without a NULL-scoped parent and fails as orphaned. Move all NULL-scoped keys for the identity into the new wallet atomically with the parent promotion.
6. [blocking] Resolve ownerless historical addresses across every funding account
packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs:320-331
The loader deliberately unions address-pool state with historical core_utxos scripts, but a historical script without a corresponding pool row has owner = None. route_to_funds_account sends that case only to funding account zero, after which extend_pools_for_restored_addresses probes only account zero's derivation chains. If the address belongs to another standard, CoinJoin, or DashPay funding account, it remains unmarked in its real pool and can be handed out again after restart. This is a normal migrated or partial-store shape because V003 creates core_address_pool without backfilling historical rows. Probe every funding account for ownerless addresses, route an unambiguous match to its actual account, and reject ambiguity rather than silently selecting account zero.
7. [suggestion] Make OS-keyring reprotection atomic
packages/rs-platform-wallet-storage/src/secrets/store.rs:330-335
The File arm now performs reprotection under the encrypted store's lock, but the OS-keyring arm still calls get_secret and set_secret as independent operations. The source explicitly documents this residual. A concurrent set or delete of the same (service, label) between those calls can still be overwritten or resurrected from stale plaintext. The caller-serialization warning does not enforce the invariant for this shared &self API. Use backend CAS/versioning or shared per-slot synchronization where possible; otherwise expose a typed unsupported/non-atomic result rather than presenting this path as equivalent to the atomic File operation.
8. [suggestion] [carried-forward: prior-account-type-domain] Constrain core_address_pool.account_type to known labels
packages/rs-platform-wallet-storage/migrations/V003__unified.rs:43-55
core_address_pool.account_type still accepts arbitrary text. Both pool readers copy the raw value into OwningAccount; an unknown label cannot match an account reconstructed from the manifest, so route_to_funds_account assigns its UTXO or used-address state to the first funding account and emits only a warning. A corrupt or foreign-written row can consequently load successfully while balances, input selection, and address-reuse state are attributed to the wrong account. Apply the same known-label domain constraint used for account registrations or reject unknown labels in every pool reader.
9. [suggestion] [carried-forward: prior-restore-wal-cleanup-race] Do not unlink a fresh peer's post-restore WAL
packages/rs-platform-wallet-storage/src/sqlite/backup.rs:409-438
Restore releases the destination's SQLite exclusion, atomically exposes the replacement database, and only afterward removes the destination -wal and -shm paths. During that unlocked interval another process can open the replacement and create or commit through fresh sidecars. Path-based cleanup cannot distinguish those live post-swap files from stale pre-swap siblings and may unlink an active WAL/SHM pair; removing SHM can also split the lock domain between existing and later connections. Keep cleanup interlocked with new opens or remove only sidecars whose file identity proves they predate the swap.
10. [suggestion] Cross-check identity_keys.public_key_hash during load
packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:243-276
load_state now validates the decoded identity id, key id, inner public-key id, and wallet scope, but its query still omits the typed public_key_hash column. A row whose typed hash disagrees with the hash in public_key_blob therefore loads successfully despite the reader's stated typed-column/BLOB integrity contract. Select public_key_hash, require exactly 20 bytes, and reject a mismatch with IdentityKeyEntryMismatch before inserting the entry.
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:256-280: Reject orphan keys in the unowned scope
When `identity_keys.wallet_id` is NULL, SQLite MATCH SIMPLE disables both foreign keys. These INSERT and UPDATE triggers abort only when a matching wallet-owned identity exists. If no identity with `NEW.identity_id` exists, the `EXISTS` predicate is false and the orphan key is accepted. A key-only write under the all-zero scope can therefore report success but later make `load_unowned_identities()` fail with `OrphanedIdentityEntry`. Both triggers must positively require a matching parent with `wallet_id IS NULL`.
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-201: Reject duplicate identity derivation indices during rehydration
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3642723489)
The schema permits multiple live identities to share the same non-NULL `(wallet_id, identity_index)`. `identities::load_state` also omits the typed `identity_index` column from its query and inserts decoded identities into a map keyed by `entry.identity_index` without checking whether `BTreeMap::insert` replaced an existing value. A later row can therefore silently remove an identity from a successful load, or cause a subsequent key/contact merge to fail as orphaned. Add a partial unique index for live non-NULL wallet/index pairs, cross-check the typed index against the decoded entry, and reject an occupied map key.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:40-47: Move persisted keys when promoting an unowned identity
This upsert promotes an identity by changing `identities.wallet_id` from NULL to the incoming wallet, but existing NULL-scoped `identity_keys` rows are not moved. Their compound foreign key remains dormant while the child scope is NULL, and there is no parent-update trigger or `ON UPDATE` action. A scalar-only identity flush can therefore promote the parent while leaving its keys behind: the wallet loader restores an identity without its signing-key metadata, while the unowned loader sees keys without a NULL-scoped parent and fails as orphaned. Move all NULL-scoped keys for the identity into the new wallet atomically with the parent promotion.
In `packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs:320-331: Resolve ownerless historical addresses across every funding account
The loader deliberately unions address-pool state with historical `core_utxos` scripts, but a historical script without a corresponding pool row has `owner = None`. `route_to_funds_account` sends that case only to funding account zero, after which `extend_pools_for_restored_addresses` probes only account zero's derivation chains. If the address belongs to another standard, CoinJoin, or DashPay funding account, it remains unmarked in its real pool and can be handed out again after restart. This is a normal migrated or partial-store shape because V003 creates `core_address_pool` without backfilling historical rows. Probe every funding account for ownerless addresses, route an unambiguous match to its actual account, and reject ambiguity rather than silently selecting account zero.
In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/store.rs:330-335: Make OS-keyring reprotection atomic
The File arm now performs reprotection under the encrypted store's lock, but the OS-keyring arm still calls `get_secret` and `set_secret` as independent operations. The source explicitly documents this residual. A concurrent set or delete of the same `(service, label)` between those calls can still be overwritten or resurrected from stale plaintext. The caller-serialization warning does not enforce the invariant for this shared `&self` API. Use backend CAS/versioning or shared per-slot synchronization where possible; otherwise expose a typed unsupported/non-atomic result rather than presenting this path as equivalent to the atomic File operation.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:243-276: Cross-check identity_keys.public_key_hash during load
`load_state` now validates the decoded identity id, key id, inner public-key id, and wallet scope, but its query still omits the typed `public_key_hash` column. A row whose typed hash disagrees with the hash in `public_key_blob` therefore loads successfully despite the reader's stated typed-column/BLOB integrity contract. Select `public_key_hash`, require exactly 20 bytes, and reject a mismatch with `IdentityKeyEntryMismatch` before inserting the entry.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:42-43: [carried-forward: prior-manager-load-shutdown] Keep the retained manager operational after load failure
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3588618889)
The exhausted persister-retry path still calls `self.shutdown().await`, and the hydration rollback repeats that call at line 227. `shutdown()` is terminal: it stops SPV and the synchronization coordinators, cancels the event-adapter token, and consumes the adapter's sole join handle. Only manager construction spawns that adapter. The shipped Swift `WalletManagerStore.activate` catches the load exception and then caches and exposes the same manager, so later wallet operations use a valid-looking handle whose wallet events no longer reach persistence. Keep the manager operational after an API-level load failure, make every stopped component restartable, or invalidate the handle so callers must reconstruct it.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:574-578: Do not return a different transaction from a txid lookup
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3648065991)
`get_tx_record` is a point lookup for the supplied txid, but a decoded record containing another txid is only logged and best-effort repaired before the mismatching record is returned. The repair may collide or fail, and even a successful repair does not change the current call returning transaction B for a request for transaction A. Callers can consequently consume B's finality or chain-lock context while handling A. Return `CoreTransactionEntryMismatch` for a txid mismatch and keep any repair in a separate maintenance operation that cannot violate the lookup contract.
In `packages/rs-platform-wallet-storage/migrations/V003__unified.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V003__unified.rs:43-55: [carried-forward: prior-account-type-domain] Constrain core_address_pool.account_type to known labels
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3586330039)
`core_address_pool.account_type` still accepts arbitrary text. Both pool readers copy the raw value into `OwningAccount`; an unknown label cannot match an account reconstructed from the manifest, so `route_to_funds_account` assigns its UTXO or used-address state to the first funding account and emits only a warning. A corrupt or foreign-written row can consequently load successfully while balances, input selection, and address-reuse state are attributed to the wrong account. Apply the same known-label domain constraint used for account registrations or reject unknown labels in every pool reader.
In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:409-438: [carried-forward: prior-restore-wal-cleanup-race] Do not unlink a fresh peer's post-restore WAL
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3586330044)
Restore releases the destination's SQLite exclusion, atomically exposes the replacement database, and only afterward removes the destination `-wal` and `-shm` paths. During that unlocked interval another process can open the replacement and create or commit through fresh sidecars. Path-based cleanup cannot distinguish those live post-swap files from stale pre-swap siblings and may unlink an active WAL/SHM pair; removing SHM can also split the lock domain between existing and later connections. Keep cleanup interlocked with new opens or remove only sidecars whose file identity proves they predate the swap.
thepastaclaw
suggested changes
Aug 21, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The SQLite backend has strong bounded-decoding and recovery-mode coverage, but six blocking rehydration and lifecycle findings remain, including terminal manager shutdown, identity loss or key misplacement, incorrect recovery point reads, and an address-reuse gap. Four additional integrity and concurrency suggestions are included; two lower-ranked valid suggestions concerning foreign-key validation and vault durability were omitted because the 10-comment budget is fully consumed. Source: codex-general, codex-security-auditor, and codex-rust-quality reviewer lanes (exact backend model IDs were not supplied); Claude Agent SDK final verifier (exact backend model ID was not supplied). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 6 blocking | 🟡 2 suggestion(s)
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:256-281: Reject orphan keys in the unowned scope
When `identity_keys.wallet_id` is NULL, SQLite MATCH SIMPLE disables both foreign keys. The INSERT and UPDATE triggers abort only when a wallet-owned identity with the supplied ID exists. If no parent identity exists at all, their `EXISTS` predicates are false and the orphan key is accepted. Such a write can report success and later make unowned-identity reconstruction fail with an orphaned entry. Both triggers must positively require a matching `identities` row whose `wallet_id IS NULL`, thereby rejecting both wallet-owned and absent parents.
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-201: Reject duplicate identity derivation indices during rehydration
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3642723489)
The schema still permits multiple live identities with the same non-NULL `(wallet_id, identity_index)`. The loader also omits the typed `identity_index` column and inserts decoded identities into `wallet_identities` by `entry.identity_index` without checking the return from `BTreeMap::insert`. A later row can therefore silently replace an earlier identity while `load()` succeeds. Add a partial unique index for live non-NULL wallet/index pairs, select and cross-check the typed index against the decoded entry, and reject an occupied map key rather than replacing it.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:40-48: Move persisted keys when promoting an unowned identity
The identity upsert promotes a parent by changing `identities.wallet_id` from NULL to the incoming wallet, but it does not move existing NULL-scoped `identity_keys`. Because a NULL child scope disables the compound foreign key, those rows remain accepted after the parent moves. A scalar-only identity flush can therefore restore the wallet-owned identity without its signing-key metadata while leaving orphaned keys in the unowned scope. Move all NULL-scoped keys for the identity to the destination wallet atomically with the parent promotion.
In `packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs:322-399: Resolve ownerless historical addresses across every funding account
Historical addresses found only through `core_utxos` can have `owner = None`, and `route_to_funds_account` maps every such address directly to funding account zero. Only that account is then passed the address during pool extension. If the address belongs to another Standard, CoinJoin, or DashPay funding account, its actual pool remains unmarked and can issue the address again after restart. This is a valid migrated or partial-store shape because `core_address_pool` was not backfilled for all historical rows. Probe every funding account for ownerless addresses, route a unique match to the matching account, and reject or explicitly quarantine ambiguous matches instead of silently choosing account zero.
In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/store.rs:306-335: Make OS-keyring reprotection atomic
The File backend now performs reprotection under one store lock, but the OS-keyring branch still calls `get_secret` and `set_secret` as separate operations. A concurrent set or delete of the same `(service, label)` can interleave between them, after which reprotection overwrites the newer value or resurrects a deleted secret from stale plaintext. The documentation acknowledges the race, but caller serialization is not enforced by this shared `&self` API. Use backend CAS/versioning or shared per-slot synchronization; if the backend cannot provide that invariant, expose reprotection as unsupported or explicitly non-atomic through the result type.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:243-276: Cross-check identity_keys.public_key_hash during load
`load_state` verifies the decoded identity ID, key ID, inner public-key ID, and wallet scope, but the query still omits the typed `public_key_hash` column. A row whose typed hash disagrees with `IdentityKeyWire.public_key_hash` therefore passes the reader's typed-column/BLOB integrity boundary. Select the typed hash, require exactly 20 bytes before materializing it, compare it with `entry.public_key_hash`, and return `IdentityKeyEntryMismatch` on disagreement.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:42-149: [carried-forward: prior-manager-load-shutdown] Keep the retained manager operational after load failure
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3588618889)
Both the exhausted persister-load path and hydration rollback call `self.shutdown().await`. That operation is terminal: it stops SPV, seals the synchronization coordinators, cancels the wallet-event adapter, and consumes its join handle. The Swift activation path catches the error and retains the same manager, so subsequent operations can use a valid-looking handle whose events no longer reach persistence. The idempotence check also introduces a related lifecycle race: one concurrent load can insert the inner wallet and yield during asynchronous address initialization, while a second load sees the inner entry at line 148 and returns success before `self.wallets` is populated; if the first load then fails, it removes the entry after the second has already reported success. Keep the manager operational on API-level load errors and serialize the complete hydration transaction, or expose an explicit in-progress/completed state that concurrent callers await and invalidate the handle after terminal shutdown.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:577-580: Do not return a different transaction from a txid lookup
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3648065991)
Strict mode rejects a typed-column/BLOB mismatch, but recovery mode lets `ctx.tolerate(...)` succeed and then returns the decoded record. The regression test at lines 1282-1321 explicitly pins a lookup for typed transaction A returning blob transaction B. Read-only recovery prevents database writes, but it does not make it safe for payment or asset-lock logic to consume B's finality or chain-lock context while handling A. A point lookup must return the mismatch or omit the inconsistent row in both policies; it must never return `Some(record)` whose `record.txid` differs from the requested txid.
In `packages/rs-platform-wallet-storage/migrations/V003__unified.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V003__unified.rs:43-55: [carried-forward: prior-account-type-domain] Constrain core_address_pool.account_type to known labels
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3586330039)
`core_address_pool.account_type` remains unconstrained text, and both ownership readers copy it directly into `OwningAccount`. An unknown label cannot match any reconstructed funds account, so routing falls back to account zero and merely records degradation, even under strict policy. That can misattribute UTXOs, balances, coin selection, and address-reuse state. Apply the same `ACCOUNT_TYPE_LABELS` domain used by account registrations, or reject unknown labels in every pool reader before constructing `OwningAccount`.
In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:400-440: [carried-forward: prior-restore-wal-cleanup-race] Do not unlink a fresh peer's post-restore WAL
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3586330044)
Restore releases the destination's SQLite exclusion before exposing the replacement database, then removes the destination `-wal` and `-shm` paths after the swap. During that unlocked interval another process can open the replacement and create or commit through fresh sidecars. The path-based cleanup cannot distinguish those current files from stale pre-swap siblings and can unlink an active WAL/SHM pair or split SQLite's SHM lock domain between existing and later connections. Keep sidecar cleanup interlocked with new opens, or remove only sidecars whose file identity proves they predate the swap.
Comment on lines
+256
to
+281
| CREATE TRIGGER identity_keys_null_scope_requires_unowned_identity | ||
| BEFORE INSERT ON identity_keys | ||
| FOR EACH ROW WHEN NEW.wallet_id IS NULL | ||
| BEGIN | ||
| SELECT RAISE(ABORT, 'identity_keys.wallet_id is NULL but the identity is wallet-owned') | ||
| WHERE EXISTS ( | ||
| SELECT 1 FROM identities i | ||
| WHERE i.identity_id = NEW.identity_id AND i.wallet_id IS NOT NULL | ||
| ); | ||
| END; | ||
|
|
||
| -- Necessary twin, NOT a redundant copy — do not simplify away. The primary | ||
| -- key is (identity_id, key_id), so the writer's upsert resolves an | ||
| -- existing key to DO UPDATE, and an UPDATE never fires a BEFORE INSERT | ||
| -- trigger. Without this one the guard above is bypassed by the ordinary | ||
| -- re-save path, which is the path real writes take. | ||
| CREATE TRIGGER identity_keys_null_scope_requires_unowned_identity_on_update | ||
| BEFORE UPDATE ON identity_keys | ||
| FOR EACH ROW WHEN NEW.wallet_id IS NULL | ||
| BEGIN | ||
| SELECT RAISE(ABORT, 'identity_keys.wallet_id is NULL but the identity is wallet-owned') | ||
| WHERE EXISTS ( | ||
| SELECT 1 FROM identities i | ||
| WHERE i.identity_id = NEW.identity_id AND i.wallet_id IS NOT NULL | ||
| ); | ||
| END; |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Reject orphan keys in the unowned scope
When identity_keys.wallet_id is NULL, SQLite MATCH SIMPLE disables both foreign keys. The INSERT and UPDATE triggers abort only when a wallet-owned identity with the supplied ID exists. If no parent identity exists at all, their EXISTS predicates are false and the orphan key is accepted. Such a write can report success and later make unowned-identity reconstruction fail with an orphaned entry. Both triggers must positively require a matching identities row whose wallet_id IS NULL, thereby rejecting both wallet-owned and absent parents.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in this update — Reject orphan keys in the unowned scope no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Comment on lines
40
to
+48
| let mut stmt = tx.prepare_cached( | ||
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | ||
| "INSERT INTO identities (identity_id, wallet_id, identity_index, entry_blob, tombstoned) \ | ||
| VALUES (?1, ?2, ?3, ?4, 0) \ | ||
| ON CONFLICT(identity_id) DO UPDATE SET \ | ||
| wallet_id = COALESCE(identities.wallet_id, excluded.wallet_id), \ | ||
| wallet_index = excluded.wallet_index, \ | ||
| identity_index = excluded.identity_index, \ | ||
| entry_blob = excluded.entry_blob, \ | ||
| tombstoned = 0", | ||
| tombstoned = 0 \ | ||
| WHERE identities.wallet_id IS NULL OR identities.wallet_id IS excluded.wallet_id", |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Move persisted keys when promoting an unowned identity
The identity upsert promotes a parent by changing identities.wallet_id from NULL to the incoming wallet, but it does not move existing NULL-scoped identity_keys. Because a NULL child scope disables the compound foreign key, those rows remain accepted after the parent moves. A scalar-only identity flush can therefore restore the wallet-owned identity without its signing-key metadata while leaving orphaned keys in the unowned scope. Move all NULL-scoped keys for the identity to the destination wallet atomically with the parent promotion.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 72395da — Move persisted keys when promoting an unowned identity no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Comment on lines
+322
to
+399
| // The persisted pool used-state restores addresses whose funds were | ||
| // since spent — without it a previously-used address comes back marked | ||
| // unused and could be handed out again as a fresh receive address | ||
| // (address-reuse privacy leak). Each is routed to its owning funds | ||
| // account (same identity match as the UTXOs), so a used address on a | ||
| // non-first account is marked used on ITS OWN pool. A `None` owner, or | ||
| // one absent from this wallet, falls back to the first account. An | ||
| // empty map marks only the unspent-UTXO addresses. | ||
| for (addr, owner) in used_pool_addresses { | ||
| let target = | ||
| route_to_funds_account(&account_keys, owner.as_ref(), &mut orphaned_owners); | ||
| per_account_addrs[target].push(addr.clone()); | ||
| } | ||
|
|
||
| // Degraded in BOTH policies, never fatal. An owner absent from the | ||
| // funds accounts is not necessarily corruption: provider accounts are | ||
| // first-class in the schema yet sit on a non-secp256k1 curve, so they | ||
| // are not `ManagedCoreFundsAccount` and a used provider-owned address | ||
| // has no funds account to route to. Telling that apart needs an | ||
| // upstream `key-wallet` enumerator over every account kind; until then, | ||
| // failing here would brick a masternode-operator wallet. | ||
| if !orphaned_owners.is_empty() { | ||
| ctx.note_degraded( | ||
| LoadSite::OrphanedUtxoOwner, | ||
| SiteCoords { | ||
| wallet_id, | ||
| account_type: &orphaned_owners, | ||
| affected: orphaned_owners.len(), | ||
| }, | ||
| "restored UTXOs or used addresses were routed to the first funds account \ | ||
| because their own owning accounts are not funds accounts of this wallet; \ | ||
| the per-account view re-warms on the next sync", | ||
| ); | ||
| } | ||
|
|
||
| // Eager derivation covers only `0..gap_limit`; extend each chain to | ||
| // cover restored / used addresses at deeper indices. | ||
| for i in 0..funding.len() { | ||
| if !per_account_addrs[i].is_empty() { | ||
| extend_pools_for_restored_addresses( | ||
| funding[i], | ||
| manifest, | ||
| &per_account_addrs[i], | ||
| wallet_id, | ||
| ctx, | ||
| )?; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Recompute per-account + wallet balance from the restored set. | ||
| // After this, a non-zero persisted balance is non-zero here — a | ||
| // silent zero would be a hard FAIL of the rehydration contract. | ||
| wallet_info.update_balance(); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Resolve an owning account to its position among `account_keys`, or fall | ||
| /// back to the first funds account. A `None` owner (no attribution available) | ||
| /// falls back silently; an owner not present in `account_keys` (store drift) | ||
| /// falls back too but is recorded in `orphaned_owners` for a single post-loop | ||
| /// `tracing::warn!`. Shared by the UTXO and used-address routing loops so both | ||
| /// bucket funds and used-state by the exact same identity match. | ||
| fn route_to_funds_account( | ||
| account_keys: &[OwningAccount], | ||
| owner: Option<&OwningAccount>, | ||
| orphaned_owners: &mut Vec<String>, | ||
| ) -> usize { | ||
| match owner { | ||
| None => 0, | ||
| Some(owner) => account_keys | ||
| .iter() | ||
| .position(|k| k == owner) | ||
| .unwrap_or_else(|| { | ||
| orphaned_owners.push(format!("{}[{}]", owner.account_type, owner.account_index)); | ||
| 0 | ||
| }), | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Resolve ownerless historical addresses across every funding account
Historical addresses found only through core_utxos can have owner = None, and route_to_funds_account maps every such address directly to funding account zero. Only that account is then passed the address during pool extension. If the address belongs to another Standard, CoinJoin, or DashPay funding account, its actual pool remains unmarked and can issue the address again after restart. This is a valid migrated or partial-store shape because core_address_pool was not backfilled for all historical rows. Probe every funding account for ownerless addresses, route a unique match to the matching account, and reject or explicitly quarantine ambiguous matches instead of silently choosing account zero.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 9160fc4 — Resolve ownerless historical addresses across every funding account no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Comment on lines
+306
to
+335
| /// **Atomicity:** on the `File` arm the read → rewrap → write runs under | ||
| /// the store's single lock, so a concurrent `set`/`delete` can't interleave | ||
| /// and let this rewrite (built on the bytes read here) clobber a newer | ||
| /// value. The `Os` arm is a per-item keyring with no transaction, so its | ||
| /// read→write is NOT atomic — a documented residual; serialize reprotect | ||
| /// intent at the caller if a concurrent writer is possible there. | ||
| pub fn reprotect( | ||
| &self, | ||
| service: &WalletId, | ||
| label: &str, | ||
| current: Option<&SecretString>, | ||
| new: Option<&SecretString>, | ||
| ) -> Result<(), SecretStoreError> { | ||
| match self { | ||
| Self::File(s) => { | ||
| s.delete_bytes(service, label)?; | ||
| Ok(()) | ||
| let params = s.kdf_params(); | ||
| s.reprotect_bytes(service, label, |stored| { | ||
| let Some(stored) = stored else { | ||
| return Err(SecretStoreError::NoEntry); | ||
| }; | ||
| let secret = envelope::unwrap(service, label, current, stored.expose_secret())?; | ||
| envelope::wrap_with_params(service, label, new, secret.expose_secret(), params) | ||
| }) | ||
| } | ||
| Self::Os(_) => { | ||
| let Some(secret) = self.get_secret(service, label, current)? else { | ||
| return Err(SecretStoreError::NoEntry); | ||
| }; | ||
| self.set_secret(service, label, &secret, new) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Make OS-keyring reprotection atomic
The File backend now performs reprotection under one store lock, but the OS-keyring branch still calls get_secret and set_secret as separate operations. A concurrent set or delete of the same (service, label) can interleave between them, after which reprotection overwrites the newer value or resurrects a deleted secret from stale plaintext. The documentation acknowledges the race, but caller serialization is not enforced by this shared &self API. Use backend CAS/versioning or shared per-slot synchronization; if the backend cannot provide that invariant, expose reprotection as unsupported or explicitly non-atomic through the result type.
source: ['codex', 'coderabbit']
Collaborator
There was a problem hiding this comment.
Resolved in this update — Make OS-keyring reprotection atomic no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Comment on lines
+243
to
+276
| let mut stmt = conn.prepare( | ||
| "SELECT identity_id, key_id, length(public_key_blob), public_key_blob \ | ||
| FROM identity_keys WHERE wallet_id IS ?1", | ||
| )?; | ||
| let mut rows = stmt.query(params![wallet_id_param])?; | ||
| while let Some(row) = rows.next()? { | ||
| let identity_id_bytes: Vec<u8> = row.get(0)?; | ||
| let key_id: i64 = row.get(1)?; | ||
| blob::check_size(row.get::<_, i64>(2)?)?; | ||
| let payload: Vec<u8> = row.get(3)?; | ||
| let id32 = super::id32("identity_keys.identity_id", &identity_id_bytes)?; | ||
| let identity_id = Identifier::from(id32); | ||
| let key_id: KeyID = | ||
| crate::sqlite::util::safe_cast::i64_to_u32("identity_keys.key_id", key_id)?; | ||
| let entry = decode_entry(&payload)?; | ||
| // Cross-check the decoded blob against the typed columns it was | ||
| // selected by (mirrors `accounts`/`asset_locks` readers): a row whose | ||
| // blob names a different identity / key / wallet than its indexed | ||
| // columns is corruption, never silently mis-keyed into the map. | ||
| // `public_key.id()` is verified too — it becomes the DPP | ||
| // signing-selection map key via `add_public_key`, so a mismatch would | ||
| // file the key under a wrong KeyID rather than being caught here. | ||
| if entry.identity_id != identity_id | ||
| || entry.key_id != key_id | ||
| || entry.public_key.id() != key_id | ||
| { | ||
| return Err(WalletStorageError::IdentityKeyEntryMismatch); | ||
| } | ||
| if let Some(entry_wallet_id) = entry.wallet_id { | ||
| if entry_wallet_id != *wallet_id { | ||
| return Err(WalletStorageError::IdentityKeyEntryMismatch); | ||
| } | ||
| } | ||
| cs.upserts.insert((identity_id, key_id), entry); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Cross-check identity_keys.public_key_hash during load
load_state verifies the decoded identity ID, key ID, inner public-key ID, and wallet scope, but the query still omits the typed public_key_hash column. A row whose typed hash disagrees with IdentityKeyWire.public_key_hash therefore passes the reader's typed-column/BLOB integrity boundary. Select the typed hash, require exactly 20 bytes before materializing it, compare it with entry.public_key_hash, and return IdentityKeyEntryMismatch on disagreement.
source: ['codex', 'coderabbit']
…ndex) on write (#4441) Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…write `store()` merged the changeset, released the write connection, then called `flush_inner` as an independent operation. `flush_inner` is not special to the call that triggered it: an explicit `flush()`, a `commit_writes()`, or another thread's `store()` runs the same code, and whichever one reaches `take_for_flush` first owns everything it drained. If that bystander hit the flush-time backstop, `handle_flush_error`'s fatal branch dropped the changeset and returned `Err` to the bystander — leaving the `store()` that merged the now-destroyed entry to find an empty buffer and report `Ok(())`. Immediate mode promises durability on `Ok`, so that is a silent lost write, the same class as the probe/merge race this branch already closed, one window later. Under `FlushMode::Immediate`, `store()` now holds ONE connection guard across both the merge and the flush. Every path that drains the buffer (`flush_inner`, `delete_wallet`) already takes that same lock for its whole take/write/restore window, so no drain can interleave: the flush reports the fate of exactly what the call staged. `flush_inner` splits into itself plus `flush_locked`, which takes an already-held connection — `self.conn()` is not reentrant, so a `store` holding it could not otherwise call anything flush-shaped. The merge collapses to a single `store_checked` call taking the connection as an `Option` (`Manual` mode without identities needs none), which retires the `else` branch's `Buffer::store` from production. Tested through `store_flush_seam`, a test-only rendezvous in the same style as the crate's two error injectors: it parks a bystander exactly in the merge-to-flush window instead of racing for it, so the regression fails every run. The pre-existing 500-iteration stress loop drops to 64 — at 36ms per attempt it was 18.3s of CI, and the guarantee it shakes at is now pinned deterministically by its new sibling.
…s after the carve-out `delete_wallet_inner`'s pre-flush carve-out tolerates the one state that would otherwise make a wallet undeletable: pending identity writes that can never be persisted. It rolled the pre-flush back, warned, and — alone among the arms of that match — did not put the changeset back in `drained_slot`. Everything after that point can still fail: `run_auto_backup` (a persister configured without an auto-backup directory fails there every time), the cascade's `BEGIN EXCLUSIVE`, the `wallets` delete, the commit. On any of them `restore_buffer` runs against a slot that is already empty, so the wallet survives and its staged writes do not — and a changeset carries every sub-changeset staged for that wallet, not just the offending identity entry. Re-seat the changeset in the arm, like every other error arm does. The success path still drops it right after `tx.commit()`, which is the point at which discarding it is justified: the wallet it belongs to is gone.
…ngeset ends in
`check_index_conflicts` forgave a sitting on-disk occupant only when the
changeset tombstoned it, so it refused `{A -> 2, B -> 1}` against an
on-disk `A@1` even though the final state (`A@2`, `B@1`) is unique and
legal. A two-way swap failed for the same reason. The guard is a
uniqueness rule, not a freeze: an occupant the same changeset moves to
another index — or strips of its index entirely — has vacated the slot
by the time the changeset lands, exactly like a tombstoned one.
Skip such an occupant too. The `removed` check keeps its place ahead of
the new one, so nothing changes for a tombstone-and-reinsert. There is no
`(wallet_id, identity_index)` UNIQUE index on the table, so `apply`'s
row-at-a-time upserts pass through the transient double-claim a swap goes
through rather than trading a store-time rejection for a flush-time one.
Nothing in `rs-platform-wallet` reindexes an identity today —
`identity_index` is fixed at `ManagedIdentity::new` — so this is a latent
trap rather than a live bug: a write path that rejects a legal state is a
defect waiting for the first caller that needs it.
…one-shot Marvin's adversarial QA pass on the store()/flush_locked race fix (QA-001, low severity) found that set_store_flush_seam_for_test doesn't follow its two sibling injectors' one-shot convention (force_next_flush_to_fail / force_next_pre_flush_to_fail): the callback stays armed and fires on every subsequent store() call until overwritten, and the doc comment didn't say so. No production code path is affected (test/__test-helpers-gated only, confirmed absent from a release build's .rlib). Documents the behavior and points at release_at_store_seam's own one-shot latch for the common case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VLqkGNAz11EUZLLXqW9LfV
…ydration' into chore/3968-review-fixes
Collaborator
Author
|
Follow-up fixes from PR #4441's post-merge review, applied directly to this branch (
Full test suite green (777 passed, 0 failed), clippy/fmt clean. Review threads on #4441 replied to and resolved. |
Databases written before the producer reconstructed a spent output's script from its typed address carry permanently-poisoned `core_utxos` rows: `spent = 1 AND script = X''`. `core_state::load_used_addresses` decodes every stored script — spent and unspent — through a bare `Address::from_script` with no `LoadCtx` wiring, fatal under both Strict and Recovery, so a single such row rejects the load of the whole database file and every wallet in it. Nothing repairs it in place either: the spend path only flips `spent` on a row that already exists, and the load fails before SPV starts, so no resync can reach it. V012 deletes exactly those rows, once per database. The purge is balance-neutral — `load_state` and `list_unspent_utxos` select `spent = 0` only — and, contrary to the original analysis, costs the address-reuse guard nothing at all rather than one address: the guard is the only consumer of a spent row's script, and an empty script decodes to no address, so a poisoned row contributes no entry to the used-set today, only the failure. There is no privacy tradeoff to accept here. The predicate is exact and deliberately narrow. `script` is `NOT NULL`, so empty is its only degenerate value, and `spent = 1` is load-bearing: an unspent row is balance state and is left alone whatever its script holds. tc046 drives the V011 -> V012 upgrade path an existing install experiences: the used-set read fails with `AddressDecode` before the purge and recovers after it, the legitimate spent row beside the poisoned one survives byte-identical, and an unspent empty-script row on a second wallet is untouched. Both schema-freeze goldens are bumped deliberately for the added migration file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The doc comment claimed `script_pubkey` was filled with a default; it is reconstructed from `InputDetail`'s typed address. It also claimed the persister deletes by `outpoint` — no `DELETE FROM core_utxos` exists in the storage crate, which marks the row `spent = 1` or upserts it. Height and the confirmation flags genuinely are defaulted, and `core_utxos` has no column for either, so those defaults never become durable state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Landed the fix for TODO a7270ae8 (legacy empty-script
Also updates 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
suggested changes
Aug 21, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest writer-side identity-slot validation is useful, but the exact head still has seven blocking lifecycle and rehydration defects: failed loads can leave a retained manager shut down, malformed identity/key state can be silently lost or detached, recovery lookup can return the wrong transaction, ownerless historical addresses can bypass the reuse guard, and existing databases are accepted without foreign-key validation. Three additional in-scope integrity suggestions remain; one lower-priority CodeRabbit vault-durability suggestion is omitted because the 10-finding comment budget is full.
Source: Codex reviewers gpt-5.6-sol (general, security-auditor, rust-quality); final verifier: Claude Agent SDK (exact backend model ID not supplied). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 7 blocking
1 additional finding(s) omitted (not in diff).
9 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/backup.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:463-465: Check foreign-key integrity before accepting an existing database
`run_integrity_check` runs only `PRAGMA integrity_check`, which validates SQLite's structural consistency but does not report foreign-key violations. Turning `foreign_keys` on prevents new invalid statements; it does not retroactively validate rows created with enforcement disabled. An existing or staged restored database can therefore contain orphaned child rows and still pass the unconditional open-time gate. In the most direct case, deleting the wallet parent with enforcement disabled leaves child state that `load()` never enumerates, so Strict mode silently returns no wallet instead of rejecting the inconsistent store. Run `PRAGMA foreign_key_check` as part of the shared validation routine before migrations and before committing a staged restore.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:400-440: [carried-forward: prior-restore-wal-cleanup-race] Do not unlink a fresh peer's post-restore WAL
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3586330044)
Restore releases the destination's SQLite exclusion before atomically exposing the replacement database, then removes the destination `-wal` and `-shm` paths afterward. During that unlocked rename-to-cleanup interval, another process can open the replacement and create or commit through fresh sidecars. Path-based removal cannot distinguish those current files from stale pre-swap siblings, so it can unlink an active WAL/SHM pair or split SQLite's shared-memory lock domain between existing and later connections. Keep sidecar cleanup interlocked with new opens, or remove only sidecars whose file identity proves they predate the swap.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:33-43: [carried-forward: prior-manager-load-shutdown] Keep the retained manager operational after load failure
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3588618889)
Both an exhausted persister load and a later hydration failure call the terminal `shutdown()` path. That stops SPV, seals the synchronization coordinators, cancels the sole wallet-event adapter, and consumes its join handle. The shipped `WalletManagerStore.activate` catches the load error as non-fatal and caches this same manager, so subsequent wallet activity can continue through a valid-looking handle while UTXO, transaction, used-address, and sync-watermark events no longer reach persistence. The idempotence check is also incomplete: after one loader inserts into the inner wallet manager and yields during asynchronous address initialization, another loader can see that insertion and return success before `self.wallets` is populated; the first loader can then fail and roll the insertion back. Serialize the complete hydration transaction and keep reusable manager infrastructure operational after API-level errors, or terminally invalidate the handle and require reconstruction.
In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-192: Reject duplicate identity derivation indices during rehydration
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3642723489)
The new `check_index_conflicts` guard prevents cooperating writers from ending a changeset with duplicate live identity slots, but the untrusted-database boundary is still open. The schema permits multiple live rows with the same `(wallet_id, identity_index)`, while `identities::load_state` does not select or cross-check the typed `identity_index` column and ignores the value displaced by `BTreeMap::insert`. A pre-existing or tampered database can therefore make a later row silently replace an earlier identity even in Strict mode. Preserve legal same-transaction swaps if necessary, but validate the typed index during load and reject an already-occupied final map slot instead of normalizing the database by replacement.
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:256-281: Reject orphan keys in the unowned scope
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3828848280)
When `identity_keys.wallet_id` is NULL, SQLite MATCH SIMPLE disables both foreign keys. The INSERT and UPDATE triggers abort only when a wallet-owned identity with the supplied ID exists. If no parent identity exists at all, the `EXISTS` predicates are false and the orphan key is accepted, so a key-only write can succeed and later make unowned-identity reconstruction fail with `OrphanedIdentityEntry`. Both triggers must positively require a matching `identities` row with the same identity ID and `wallet_id IS NULL`, thereby rejecting both absent parents and wallet-owned parents.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:577-580: Do not return a different transaction from a txid lookup
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3648065991)
`get_tx_record` is a point lookup for the caller-supplied txid. Strict mode rejects a typed-column/blob mismatch, but Recovery mode lets `ctx.tolerate(...)` succeed and then returns the decoded record even when `record.txid` is different. Read-only recovery does not make it safe for payment reconciliation or asset-lock logic to consume transaction B's finality or chain-lock context while handling transaction A. Recovery may skip or quarantine the corrupt row, but this API must return `None` or the mismatch; it must never return `Some(record)` whose txid differs from the lookup key.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:46-54: Move persisted keys when promoting an unowned identity
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3828848286)
This upsert promotes an identity by changing its parent scope from NULL to the destination wallet, but it leaves existing NULL-scoped `identity_keys` rows unchanged. Their compound foreign key remains dormant because the child scope is NULL. A scalar-only identity changeset can therefore promote the parent while leaving all persisted public-key and derivation metadata behind: the wallet-owned loader restores an identity without those keys, while the unowned loader sees keys whose NULL-scoped parent disappeared. Move all NULL-scoped key rows for the identity to the destination wallet in the same transaction as the parent promotion.
In `packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs:322-399: Resolve ownerless historical addresses across every funding account
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3828848291)
Historical addresses discovered through `core_utxos` can legitimately have no covering `core_address_pool` row, especially because V003 does not backfill every historical script. Those entries arrive with `owner = None`, and `route_to_funds_account` always sends them to funding account zero. Pool extension then probes only that account's xpub. If the address belongs to another Standard, CoinJoin, or DashPay funding account, its actual pool remains unaware that it was used and can issue the address again after restart. Probe every reconstructed funding account for ownerless addresses, route a unique derivation match to the matching account, and reject or explicitly quarantine ambiguous matches instead of silently selecting the first account.
In `packages/rs-platform-wallet-storage/migrations/V003__unified.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V003__unified.rs:43-55: [carried-forward: prior-account-type-domain] Constrain core_address_pool.account_type to known labels
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3586330039)
`core_address_pool.account_type` remains unconstrained text, and both ownership readers copy it directly into `OwningAccount`. An unknown label cannot match any reconstructed funding account, so routing falls back to account zero and only records degradation, including under Strict policy. That can misattribute UTXOs, balances, coin selection, and address-reuse state. Apply the same `ACCOUNT_TYPE_LABELS` domain used by account registrations through a migration-safe table rebuild, or reject unknown labels in every pool reader before constructing `OwningAccount`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:243-276: Cross-check identity_keys.public_key_hash during load
(existing thread: https://github.com/dashpay/platform/pull/3968#discussion_r3828848302)
`load_state` verifies the decoded identity ID, key ID, inner public-key ID, and wallet scope, but its query still omits the typed `public_key_hash` column. A row whose typed hash disagrees with `IdentityKeyWire.public_key_hash` therefore passes the reader's typed-column/blob integrity boundary. Select `length(public_key_hash)` and `public_key_hash`, require exactly 20 bytes before materializing it, compare the typed value with `entry.public_key_hash`, and return `IdentityKeyEntryMismatch` on disagreement.
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
6 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–V011, 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.Trust boundary — the
.dbis untrusted input at load: layered size limits (16 MiB per-value cap, bounded bincode decode, 32 MiB connection backstop), typed columns cross-checked against decoded BLOBs on every read, and structurally enforced key/identity co-ownership (compound FK plus trigger fallback where SQLite's own FK check goes dormant on NULL columns).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 —
platform-wallet: pool-bookkeeping restore fix, address hand-out now reserves the address, spent-UTXO records now carry the real locking script instead of an empty one.platform-wallet-ffi: dedicated persister result codes (ErrorPersisterTransient = 42,ErrorPersisterFatal = 43, mirrored in Swift) and a 16 MiB size gate on asset-lock proof bytes.swift-sdk: consumer reconciled to the shippedplatform_wallet_manager_load_from_persistorcontract, plus aSendTransactionViewfunding-index fix.rust-dashcorepinned torev = "173ffac0fdc0c73dda0626cf385bbcfcf2437aeb"(v4.2-dev's revision, rootCargo.toml). The load-policy work in this update stayed entirely insiders-platform-wallet-storage— none of the above changed.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.Testing
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.Breaking changes
None. The storage crate is purely additive;
platform-walletandswift-sdktouches are bug fixes and comment renames with no public-signature changes.LoadPolicy::Strictis the new defaultload()behaviour, but the storage crate is unreleased with zero external Rust consumers today, so no shipped caller's behaviour changes.Checklist
For repository code-owners and collaborators only
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes