refactor(platform-wallet-storage)!: delete removed identities instead of tombstoning - #4496
Merged
Conversation
…flict fix Final-pass QA against 1df6169: re-attempts the original duplicate-entry repro (now unions, confirmed) plus three new adversarial shapes against ProviderKeyAccountConflict: a three-way duplicate, a reordered-but-equal node-key duplicate (confirmed not a conflict), and a same-index/ conflicting-value node-key collision within one store() call. The last one surfaces a real asymmetry: the account-level conflict check compares only the encoded (account_type, extended_public_key) payload, so two entries sharing that payload but disagreeing on a node key's bytes at the same index reach the child-table's ON CONFLICT ... DO UPDATE with no arbitration -- silent last-write-wins, unlike the fail-closed reasoning applied one level up. Reported as a finding, not fixed here (test documents current behavior and flags it for a design decision). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… index The account-level conflict check compared only the encoded (account_type, xpub) payload, so two entries agreeing on the account but carrying different bytes at the SAME key_index reached `ON CONFLICT ... DO UPDATE` and silently overwrote one with the other — no error, no diagnostic, on material nothing can re-derive. Found independently by Smythe (SEC-007) and Marvin (QA-005). The asymmetry was the real defect: fail closed on a contradictory xpub one level up, silently pick a winner one level down. A node key is fully determined by its account xpub and index — derivation is a pure function and `node_id` is hash160(public_key) — so two different values at one index mean one is wrong and the store cannot tell which. Same contradiction, same answer: refuse the flush (`ProviderNodeKeyConflict`), writing neither key. Checked in both directions: between entries within a flush, and against the row already stored — so a stale or corrupted re-registration cannot overwrite a good key that a later flush disagrees with. The SQL drops to `DO NOTHING`, which the checks make a no-op for identical bytes and which, if they were ever bypassed, still cannot destroy a derived key. Marvin's `marvin_adversarial_same_index_conflicting_node_key_value_is_silently_ overwritten` documented the behavior rather than asserting it; it is now `sec_007_same_index_conflicting_node_keys_are_rejected`, asserting rejection and that neither key lands. A second test covers the cross-flush case his in-batch probe could not reach. Both confirmed failing against 33d18ac. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Root-cause confirmation, blob-codec audit, chosen fix (AssetLockEntryWire mirroring IdentityKeyWire), migration/compat strategy, secondary AlreadyOpen-masking fix, and test plan. Design only — no implementation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gging Addresses the two error-handling-coverage audits of the secrets/ subtree (PR #3968). All 11 findings are LOW — diagnostic-precision, logging-level, and lint-hygiene polish; no behavioural/security defects were found. Security-lens (Smythe): - SEC-001: SecretString::default() mlock failure now logs at debug! with distinct wording (empty buffer, no secret at risk) so it no longer shares byte-identical text with the new() warn!; both sites stay greppable. - SEC-002: demote the documented-non-fatal parent-dir fsync-uncertain log from error! to warn! (degraded-but-recoverable), matching the policy that reserves error! for the propagated/fatal write failure. - SEC-003: add SecretStoreError::EntropyUnavailable; random_bytes (which backs nonce + salt draws, not just KDF) now reports it instead of the misleading KdfFailure. - SEC-004: thread the store's durability-uncertain counter through the initial-create write path so durability_uncertain_count()'s "0 == all writes confirmed durable" contract holds for create too. - SEC-005: switch the five vault_lock unsafe overrides from #[allow(unsafe_code)] to #[expect(unsafe_code, reason=...)] so a stale override self-reports (M-LINT-OVERRIDE-EXPECT). - nits: drop the redundant `let _ =` on the ?-propagating validated_label guards; drop-time sync now calls the non-logging do_write_vault_at so a drop-path failure logs once (with context) instead of twice. Coverage-lens (Marvin): - QA-001: all three keyring platform arms now debug!-log the discarded backend-init error before falling back to NoDefaultStore. - QA-002: create_parent_dir (both branches) and VaultLock::acquire's non-WouldBlock branch route through SecretStoreError::io_at so the known path rides in the error, per the crate's io_at policy. - QA-003: map_spi only collapses a rejected `user` (label) attribute to InvalidLabel; a rejected service (or any other attribute) maps to OsKeyring{Backend} instead of mislabelling the caller's label. - QA-004: add is_recoverable() + error_kind_str() to SecretStoreError, mirroring WalletStorageError's SQLite-side classification so both typed errors in the crate read as one family. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rain failures QA-004: SqlitePersister::open() — the crate's highest-stakes failure boundary (IntegrityCheckFailed, SchemaVersionUnsupported, Migration, AlreadyOpen) — emitted zero tracing on any failure path. Wrap the body in open_inner() and log every returned Err classified via error_kind_str(): tracing::error! for real failures, warn! for the benign in-process AlreadyOpen race. One exit point catches all paths, not just the four named ones. QA-003: delete_wallet_inner's post-commit drain used `if let Ok(Some(_late)) = take_for_flush(..)`, silently swallowing a possible Err(LockPoisoned) — the one spot in the file breaking its own convention. Match the Err arm and log it at tracing::error!, matching the three other LockPoisoned sites (Drop, handle_flush_error, restore_buffer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…wallet, preserve typed errors QA-001: register_wallet's store()/load_persisted() failure paths treated a transient SQLITE_BUSY/FlushRetryable identically to a permanent failure — logged, rolled back, aborted — with nothing consuming the crate's own is_transient() classification. Wire a bounded exponential-backoff retry (retry_transient helper: 4 attempts, 20→40→80ms capped at 200ms, async sleep). On a transient store() failure the persister preserves the buffered changeset, so retries re-drive the write via flush() — no re-merge, no double-count; the first attempt hands the changeset over, later attempts flush what the buffer kept. Fatal errors fail fast. The idempotent load_persisted() read is retried the same way. QA-002 + QA-005: the three persistence-adjacent register_wallet sites flattened every failure into WalletCreation(String), discarding retry classification and the #[source] chain. Route them through dedicated typed variants: store() → new PersisterStore(PersistenceError), load() → the pre-existing-but-dead PersisterLoad, initialize_from_persisted() → new PersisterRestore(Box<PlatformWalletError>). A caller can now structurally match the phase and recover is_transient() instead of parsing prose. Tests: transient-store-retried-and-succeeds, fatal-store-fails-fast (no retry), persistently-transient-store-exhausts-bounded-retries, transient-load-retried, fatal-load-surfaces-as-PersisterLoad, and a unit test pinning classification + structural matching + source chain on all three variants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ub overwrites Two blocking review findings on #4117: SqlitePersister::load() dropped persisted hardened platform-node keys on the floor (no production accessor existed), and a second store() for an already-persisted provider account silently overwrote its xpub with no fail-closed guard. Add SqlitePersister::list_provider_node_keys() to surface the persisted batch, and reject a persisted-xpub mismatch (plus a provider-label bypass through the plain ECDSA writer) with the existing ProviderKeyAccountConflict/ProviderKeyAccountEntryMismatch policy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
…ema readers Audit of PR #3968's sqlite/schema/ readers turned up 7 spots where a typed error was fumbled — wrong target names, discarded upstream errors, a silent row-drop, and a silent clamp. All plugged, TDD (repro tests RED first). - QA-001: 4 hand-rolled u32 casts (platform_addrs nonce/account_index/ address_index, identity_keys key_id) stamped SafeCastTarget::U64 on a u32 overflow — misdirecting operators. Route through safe_cast::i64_to_u32, which stamps U32. - QA-002: merge_contacts_and_keys silently dropped any identity_keys/contacts entry whose owner wasn't loaded, contradicting load_prekeyed's fail-hard doc. Now fallible + tombstone-aware: a known-tombstoned owner's orphans are skipped (one summary log per collection, not per entry); any other absent owner hard-errors via the new OrphanedIdentityEntry variant. Positive tombstone signal read from identities.tombstoned (load_tombstoned_ids). - QA-003: the production all_platform_payment_registrations reader (+ its per-wallet sibling) cross-checked account_type+index but not key_class — the very discriminator the widened PK protects. Select and cross-check key_class, mirroring load_state. - QA-004: 3 sites discarded dashcore::address::Error via map_err(|_| ...). Add AddressDecode { #[source] } + From impl; route all 3 through it. - QA-005: provider_key_account_registrations was dropped with zero runtime signal. Emit one tracing::warn when non-empty (deferred, #4113). - QA-006: enqueued_at_ms clamped to i64::MAX instead of erroring — route through safe_cast::u64_to_i64. - QA-008: Txid::from_slice error discarded despite an existing HashDecode variant; route through it via ?. QA-007 (test-helper-gated .expect()) confirmed not exploitable — no change. Shared-error-type edits (error.rs) and the two exhaustive/allowlist guard tests (sqlite_error_classification, sqlite_compile_time) updated as the necessary consequence of the new variants and reader SQL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t a hard MUST register_wallet's retry_transient retries a transient store() failure via a bare flush() (no re-supplied changeset), relying on the implementor having already buffered the changeset. SqlitePersister honors this; FFIPersister is safe only because it never returns Transient. Nothing in the trait doc stated this as a requirement, so a future implementor returning Transient without re-buffering would make flush() a no-op Ok(()) and register_wallet report a success that never persisted. State it explicitly on PlatformWalletPersistence::store: returning PersistenceErrorKind::Transient MUST mean the changeset is preserved for a subsequent bare flush(); an implementation that can't preserve it MUST classify Fatal/Constraint instead. Doc-only, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…re type (#4133) An `AssetLockEntry` carrying `proof: Some(AssetLockProof)` was written to `asset_locks.lifecycle_blob` but never read back: the shared blob codec routes the value through `bincode::serde`, whose deserializer cannot service the `deserialize_any` an internally-tagged serde enum requires. Every wallet holding such a row failed rehydration permanently. Introduce `AssetLockEntryWire`, mirroring the proven `IdentityKeyWire` pattern: carry `proof` as a natively bincode-pre-encoded `Option<Vec<u8>>` and ride fields 1-7 on the serde encoder unchanged, so a pre-fix `proof: None` row decodes byte-identically (no migration for those). `into_entry` rejects trailing bytes on the inner proof decode. Ship refinery migration V004 to delete pre-fix proof-bearing rows (`status IN ('is_locked','chain_locked')`) — unrecoverable by construction and already unreadable today, so no regression; they re-derive from Core on the next SPV sync. `max_supported_version` lifts 3 -> 4 automatically. Tests: Chain/Instant proof round-trip (repro), trailing-byte guard, None-row byte-identical compat pin, the V004 migration (pre-fix seed -> migrate -> clean load), a public-path blob round-trip coverage guard, and an advisory note in `blob.rs`. Updated schema-version pins (3 -> 4), golden migration fingerprints, and the pre-migration backup-name range for the added migration. Refs: #4133 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ped PersisterLoad (#4133) `PlatformWalletManager::new` spawns the wallet-event adapter holding an `Arc<persister>` clone before the fallible `load_from_persistor` runs. A dirty drop merely detached the join handle, so the still-running adapter kept the persister "open" and a same-process reconstruct hit `WalletStorageError::AlreadyOpen` — masking the real load error (the #4133 blob decode failure). - Add a `Drop` backstop that cancels the token and aborts the adapter on every drop path (covers the dirty-drop leak). - Release the adapter deterministically on the `load_from_persistor` error paths via the awaitable `shutdown()`, so a reconstruct on the same path is provably clean. - Replace the `WalletCreation(format!("...{}", e))` collapse with the typed `PlatformWalletError::PersisterLoad(#[from] PersistenceError)` variant, preserving the source chain, and log the cause with Debug, not Display. Deviation from the design's "preferred" option: the adapter spawn stays in `new()` rather than deferring to a post-registration `start()`. No manager-level `start()` exists (only per-sub-manager starts, invoked individually by the FFI), and deferring the wallet-event subscription past `new()` would risk missing broadcast events emitted before a late subscribe. The Drop backstop + shutdown-on-load-error achieve the same "no persister retention after a failed load" guarantee without that risk. Test: `failed_load_releases_persister_for_reconstruct` proves the persister's strong count returns to 1 after a failed load + teardown (a lingering adapter would keep it above 1). Refs: #4133 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dressDecode The QA-004 fix missed one of the three sites: `core_state::load_used_addresses` (the address-reuse-guard rehydration path, production-reachable via persister.rs) still discarded `dashcore::address::Error` through `map_err(|_| blob_decode(...))`. The prior commit's `replace_all` matched only the 12-space-indented `load_state` site, not this 8-space one, so an unparseable stored `core_utxos.script` surfaced a context-free `BlobDecode` here instead of the rich `AddressDecode`. Route it through `AddressDecode` via `?` like the other two sites, and add a repro test (bare OP_RETURN script → AddressDecode), confirmed RED against the old code first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…op convention doc-comment (#4133) Fold in audit feedback (RUST-005) and a scope change: - Exercise `AssetLockProof::Instant` with distinct, non-default field values (transaction version/lock_time + a non-zero output_index) in both the unit repro (`wire_round_trips_instant_proof`) and the coverage guard, so the round-trip proves field fidelity rather than `default() == default()`. Both proof variants are now fully-populated — the gap that let the original bug (every fixture used `proof: None`) slip through. - Drop the project-wide `deserialize_any` advisory doc-comment from `blob.rs`; the convention documentation is being handled in a separate doc-only PR to avoid conflicting edits. The wire-type mechanism comments on `AssetLockEntryWire` itself stay (ordinary code comments). Refs: #4133 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ease data to clean up (#4133) The `AssetLockEntryWire` fix is a pure application-level encoding change: same table, same columns, no DDL. A refinery migration was only ever needed to clean up ALREADY-WRITTEN broken rows from before the fix — a live-data concern. This crate is pre-release with no live data, so there is nothing to migrate: new writes use the fixed encoding going forward, and a stale pre-fix row in a local test store is wiped by recreating that store. Removes `migrations/V004__drop_undecodable_asset_locks.rs` and its test, and reverts the pins that only existed to accommodate it — `max_supported_version` stays 3, the golden migration fingerprints and the pre-migration backup-name range revert, and the `bincode` dev-dependency (added solely to seed the V004 test's old-format row) comes out. No schema/DDL change remains. The primary fix (`AssetLockEntryWire`), the secondary fix (Drop backstop + shutdown), the tertiary fix (`PersisterLoad` + Debug logging), and all round-trip / trailing-byte / None-compat / status-cross-check / secondary-regression tests are unchanged. Refs: #4133 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nition sites (#4133) Three internally-tagged dpp enums — AssetLockProof, IdentityPublicKey, DataContractConfig — derive native bincode Encode/Decode AND serde Serialize/Deserialize. Routed through the `bincode::serde` bridge they encode write-once and then never decode: resolving the `$type` tag needs `deserialize_any`, which bincode's non-self-describing serde deserializer rejects with AnyNotSupported. This class corrupted the wallet-storage blob codec (#4133) and, earlier, IdentityPublicKey (fixed via IdentityKeyWire). - Definition-site doc comments on all three enums spelling out the native-only-through-bincode rule and citing the prior incidents. - `bincode_serde_hazard` characterization tests in asset_lock_proof pinning the three-way contract: native bincode round-trips both variants, platform_value value-conversion round-trips both, and the `bincode::serde` bridge fails decode deterministically with AnyNotSupported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
11 findings fixed (Smythe SEC-001..005 + 2 nits, Marvin QA-001..004), independently re-verified against the final commit.
7 of 8 findings fixed (QA-007 confirmed non-exploitable, left alone), independently re-verified against the final commit including a follow-up round that caught a missed third AddressDecode call site.
…fixes (PR #3968) 5 findings fixed (2 HIGH: dead retry architecture wired, dead PersisterLoad variant now used; 2 MEDIUM logging gaps; 1 LOW catch-all split), plus a trait-doc hardening follow-up. Independently re-verified against the final commit including the FFI boundary.
…thub.com/dashpay/platform into feat/platform-wallet-storage-rehydration
…tics, non-empty InstantLock.inputs (#4133) Addresses Marvin's post-review findings on the #4133 storage/manager fix. QA-002 — the `Drop` backstop for the wallet-event adapter overclaimed: `JoinHandle::abort()` only *requests* cancellation, so the task and its `Arc<P>` clone are dropped by the runtime at the next poll, not synchronously inside `Drop::drop`. Reword the doc-comment to state the release is eventual (only the graceful `shutdown` path guarantees the reference is gone before it returns), and add `drop_backstop_eventually_releases_persister_without_shutdown` — a dirty drop (no `shutdown`) that polls the persister strong count down to 1, exercising the `abort` branch the graceful-path test never reaches. QA-003 — soften `failed_load_releases_persister_for_reconstruct`'s doc-comment: it is a manager-side proxy (strong-count reaches 1), not a full open→fail→reopen end-to-end proof; the dev-dep cycle precludes the concrete persister here, and the end-to-end path is covered by the storage crate's round-trip test. QA-004 — the Instant-proof round-trips used `InstantAssetLockProof:: default()`, whose nested `InstantLock.inputs` is empty, so the length-prefixed-vec encoding path every genuine IS-lock hits went untested. Populate `instant_lock.inputs` with real outpoints in `wire_round_trips_instant_proof`, the `sqlite_blob_roundtrip_coverage` test, and the rs-dpp `bincode_serde_hazard` characterization test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…let-storage-rehydration # Conflicts: # Cargo.lock # Cargo.toml
Fixes surfaced by merging origin/v4.1-dev into this PR's branch: - Cargo.toml/Cargo.lock: the two branches had independently diverged rust-dashcore pins (be6e776d = PR #851 UTXO-spend fix; 19690d31 = PR #893 + the new provider-key derivation API), neither a superset of the other. Re-pin to #851's branch tip (73dcf3d0), which was freshly merged forward with dev today and is a strict superset of both. - Two migrations both claimed V003: this PR's `V003__unified.rs` and v4.1-dev's DIP-13 `V003__invitations.rs`, causing a refinery_schema_history UNIQUE constraint violation on open. Renumber the newcomer to V004. - The V004 migration's FK and its test fixture referenced the retired `wallet_metadata` table name (a divergent-branch naming leftover); this branch's reconciled table is `wallets`. Caught by this crate's own sqlite_schema_pinning retired-name guard. - `versions.rs::touched_domains`'s deliberately-exhaustive destructure of `PlatformWalletChangeSet` caught the new `invitations` field at compile time (by design, the R8 forgotten-domain guard) — wired a proper `Domain::Invitations` variant rather than silencing it, since invitations data is genuinely persisted and needs its cache-invalidation version bumped like every other domain. - Updated the golden schema-freeze fingerprints and the hardcoded max-supported-version assertions (3 -> 4) that the new migration legitimately changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Content half of the previous rename-only commit. Fixes surfaced by merging origin/v4.1-dev into this PR's branch: - Cargo.toml/Cargo.lock: the two branches had independently diverged rust-dashcore pins (be6e776d = PR #851 UTXO-spend fix; 19690d31 = PR #893 + the new provider-key derivation API), neither a superset of the other. Re-pin to #851's branch tip (73dcf3d0), freshly merged forward with dev today and a strict superset of both. - Two migrations both claimed V003: this PR's V003__unified.rs and v4.1-dev's DIP-13 V003__invitations.rs (renamed to V004 in the prior commit), causing a refinery_schema_history UNIQUE constraint violation on open. - The V004 migration's FK and its test fixture referenced the retired wallet_metadata table name (a divergent-branch naming leftover); this branch's reconciled table is `wallets`. Caught by this crate's own sqlite_schema_pinning retired-name guard. - versions.rs::touched_domains's deliberately-exhaustive destructure of PlatformWalletChangeSet caught the new `invitations` field at compile time (by design, the R8 forgotten-domain guard) — wired a proper Domain::Invitations variant rather than silencing it, since invitations data is genuinely persisted and needs its cache-invalidation version bumped like every other domain. - Updated the golden schema-freeze fingerprints and the hardcoded max-supported-version assertions (3 -> 4) that the new migration legitimately changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ydration' into feat/platform-wallet-storage-rehydration
…tence trait grumpy-review follow-up on the prior commit. Bug 1's fix (list_provider_node_keys) was an inherent SqlitePersister method with no production caller — every real consumer goes through PlatformWalletPersistence, so the data was still practically unreachable. Move it to a default trait method (mirroring get_core_tx_record), wire a public WalletPersister::provider_node_keys() wrapper, and prove reachability via PlatformWallet::persister() with a new manager-layer integration test. Also: harden the provider parent-account upsert to ON CONFLICT DO NOTHING (it was DO UPDATE while sibling node-key rows were already DO NOTHING), replace the ad-hoc provider-type matches!() with an exhaustive match so a future AccountType variant can't silently bypass the ECDSA-writer guard, update stale/incomplete doc comments and error messages, and add regression tests for the zero-state and mixed-batch-atomicity cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
… V003-max assumptions The V001-capped fixture (tests/fixtures/populated_v001.db) bincode-encodes its blobs with whatever struct shapes existed when it was captured; the v4.1-dev merge changed several of those shapes (DIP-13 invitations, provider-key persistence, seed-binding, etc.), so the stale fixture failed to decode once migrated forward. Regenerated via the crate's own documented fixture-regeneration test helper. Also caught two more hardcoded assumptions that V004 (the invitations migration) invalidated, missed by the prior merge-fallout commit because they don't share exact text with the ones already fixed there: - tc_b_033's backup-filename check still looked for the old pre-migration marker string (a near-duplicate of the already-fixed tc_b_032 check, different exact text). - tc_b_030 asserted a fresh store lands at schema version 3; it now lands at 4. Verified against the project's real CI scope for the wallet crates (scoped clippy, plus nextest), rather than the broader default test runner, which spuriously hits an unrelated doctest build-graph flake already characterized as environmental earlier in this session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second grumpy-review follow-up on this PR. Three independent reviewers converged on the same gap: FFIPersister inherits the new provider_node_keys trait default (Ok(empty)) even though it genuinely forwards derived_platform_node_keys to the Swift host on store() — so an FFI-backed wallet silently reported "no keys" instead of surfacing that this backend can't read them back yet. Override the method to return an explicit error instead; a full Swift-side read-back callback is a separate, larger follow-up (out of scope here). Also: reworded ProviderKeyAccountEntryMismatch's Display message to drop internal "ECDSA writer" jargon, and renamed a test whose name implied transactional rollback when it actually exercises upfront batch validation (the assertions were already correct). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
…ydration' into pr4117-codex-fix # Conflicts: # packages/rs-platform-wallet-ffi/src/persistence.rs # packages/rs-platform-wallet-storage/src/sqlite/schema/versions.rs # packages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rs # packages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rs # packages/rs-platform-wallet-storage/tests/sqlite_v003_migration.rs # packages/rs-platform-wallet/src/changeset/mod.rs
…on_bump test Wraps a long use-statement that cargo fmt --check flagged, fixing the red macOS CI job on PR #3968.
…ss pool rows PR #4127 replaced the retired dedicated-batch mechanism for platform-node Ed25519 keys with the generic account_address_pools pipeline, but only wired it up for the FFI backend. The SQLite backend silently dropped AddressInfo::public_key on every store()/load() round trip, so ProviderPlatformKeys pools came back empty after a restart -- these keys are SLIP-10 hardened-only and cannot be re-derived from a watch-only xpub. Adds migration V005 (nullable public_key/key_type columns on core_address_pool), persists the typed key alongside each pool row, and restores AbsentHardened platform-node entries during load() by reconstructing their AddressInfo the same way populate_platform_node_pool does at registration time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cKDZc6zhCkstdqE1HcSuU
Wave 3's tail findings (docs, InstantSend restore at load, public-surface trim, secrets at-rest and open-path gaps, test-case ID cleanup) join wave 1, wave 2 and the account key mapper guard. The wave-3 branch had already merged 2cf9b46 forward, so this merge adds no textual change over its tip; the tree is byte-identical to the automatic merge of both parents. It is recorded as a merge commit rather than a fast-forward so fix/3968-round5 keeps a first-parent line of its own. Neither side's earlier verification described this tree: wave 2's run predates wave 3's commits and wave 3's predates the load-path rewrite and the account key mapper guard. The merged tree is verified on its own.
…mode, not merely that one is set Three defects in the read-back added with the erasing cascade, each one a way for the guarantee to be reported as held while not being held. The open-time check accepted any nonzero `secure_delete`. SQLite reports the mode numerically and the values are distinct — `0` off, `1` ON, `2` FAST — so "nonzero" would have accepted `ON` where `FAST` was requested and, worse, would have accepted a cascade's raise that was never restored, leaving every later write paying the full erase cost with nothing to say so. Both constants now carry the value they must read back as, and one `set_secure_delete` helper does set-then-confirm for the raise and the restore alike. The cascade runs inside an EXCLUSIVE transaction, and a pragma that failed to carry into that context would fail silently — a delete reporting success over pages it never scrubbed. `delete_wallet_inner` reads the mode back from inside its own transaction, at the point of use rather than at the point it was set. A test pins the SQLite behaviour that read-back relies on, so a change in it surfaces as a named failure instead of as unexplained residue. The residue test could also have degraded silently. Sized small enough, its rows sit inside a page that stays in use — the one case where FAST and ON are indistinguishable — and it would then have passed while proving nothing about whole pages released to the freelist, which is what deleting a wallet actually does. It now asserts the cascade freed at least one page, so a fixture that stops exercising the hard case fails instead of quietly agreeing with the implementation. Two scanner defects in the same test are closed with it. It asserts the marker is findable BEFORE the delete, because a scan that silently matches nothing reports a clean file exactly as a clean file does; the experiment that produced this fix first returned zero hits in all three modes because the scanner refused to read a binary file. And it scans the `-wal` alongside the `.db`: pages live there until a checkpoint, so the pre-delete scan would otherwise miss data plainly still present, and a post-delete scan would miss residue parked in a WAL that outlived the handle.
41a2b2b landed on fix/3968-round5-wave3 after the previous integration merge, so the branch carried a real hole rather than a polish: the open-time guard accepted any nonzero secure_delete, which would have accepted ON where FAST was requested and, worse, accepted a cascade's raise that was never restored, leaving every later write paying the full erase cost with nothing reporting it. The merge is textually trivial - 41a2b2b is a direct child of the commit whose tree this branch already carries - but the merged tree is verified on its own rather than inherited from either side.
…quests This PR rewrites .cargo/audit.toml, which is the only file security-audit-rust.yml's audit step reads, and adds a cryptographic dependency subtree. The workflow fires on workflow_dispatch and a nightly schedule only, so neither the rewrite nor the new subtree is audited by the pull request that introduces them; the first run lands after merge. The fix is a workflow change (a pull_request trigger scoped to the manifest and lockfile paths) plus getting the job green, both of which belong to the CI file rather than to this PR's diff. The deferral is recorded where the next reader of this file will meet it rather than left to memory. Comment only - the TOML parses to the same ignore list.
Preserve mainline migration V007 and renumber the rehydration migrations. Reconcile sweep placeholders and ownership tracking with transaction-based confirmation heights, including cleanup of swept height-only transactions. Validation: storage tests with all features (952 passed, 2 ignored), cargo fmt, and targeted Clippy with CI flags. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Inject the lock result through a private constructor helper so the refusal test exercises warning, zero-initialization, and buffer access on the same allocation. Keep the production constructor bound to the real mlock call. Remove the assumption that every host rejects a fixed low address. Validation: 12 guarded-buffer tests with all features, rustfmt, and storage Clippy with CI flags on Linux. Co-Authored-By: OpenAI Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…w-up PR Keep the insert_platform_node_pool_entry refactor and PlatformNodePoolError type in this PR (rs-platform-wallet-storage's rehydrate.rs depends on both to compile), but move the five new unit tests exercising them out of the storage PR's diff — none are needed for rs-platform-wallet-storage to build or pass its own tests. They land in a follow-up PR stacked on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Preserve the independently published platform-wallet test split alongside the verified storage migration fixes. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Move provider account and pool reconstruction with their errors and tests into SQLite storage. Restore wallet pool registration to its base version and shorten audit exception comments. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
reviewed
Sep 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The identity-delete implementation still leaves persisted pending contact-crypto rows behind, which contradicts the PR's complete-cleanup goal. Source-derived SQLite probes confirmed retention after both deletion and migration backfill with foreign-key enforcement enabled and disabled; additional verified issues concern migration coverage and documentation. These are non-consensus suggestions and nitpicks under the supplied severity policy, not blocking findings.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This changes destructive wallet-storage behavior and migrates existing databases, where incorrect cascade triggers, NULL-owner handling, backfill purges, or transaction ordering could permanently delete unintended identity data or break persistence and recovery. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort max); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort max); agentphase1-reviewer - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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/V016__identity_hard_delete.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V016__identity_hard_delete.rs:37-40: Delete pending contact-crypto rows with their owning identity
`pending_contact_crypto` belongs to the removed identity's in-memory DashPay state, but its only database foreign key references `wallets`. The production changeset writer persists this queue, while `remove_identity` drops the managed identity and emits only an identity removal, not queue clears. Neither this trigger nor the explicit tombstone backfill deletes those rows. Executing the source migration SQL confirmed that both paths leave one pending queue row after its identity is gone, with foreign keys either ON or OFF. This leaves owner/contact identifiers and encrypted payloads on disk contrary to the complete-cleanup contract. Delete by `owner_identity_id` in both cleanup paths and add regression coverage. The queue reader is currently test-gated, so this finding concerns persisted leftovers, not a demonstrated production rehydration of the old queue.
- [SUGGESTION] packages/rs-platform-wallet-storage/migrations/V016__identity_hard_delete.rs:17-20: Correct the claimed load behavior for orphaned ignored senders
The new documentation says leftover `contacts` and `ignored_senders` rows both produce `OrphanedIdentityEntry` and fail loading. Only contacts take that path. `load_state_with_ctx` loads ignored senders into a map, removes entries for identities it actually loads, and drops the remaining map without an error, recovery count, or log. `merge_contacts_and_keys` explicitly skips ignored/unignored entries. Correct this paragraph and the matching new error-behavior paragraph on `merge_contacts_and_keys` to distinguish contacts from ignored senders. The silent-drop behavior predates this PR; correcting the newly introduced documentation does not require expanding this change into reader hardening.
In `packages/rs-platform-wallet-storage/tests/sqlite_identity_hard_delete.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/tests/sqlite_identity_hard_delete.rs:650-651: Test pragma-independent backfill and DashPay dependent cleanup
V016 explicitly promises that its backfill works independently of the migrating connection's `foreign_keys` pragma, but this test enables enforcement unconditionally. Consequently, removing the explicit `token_balances` purge would remain undetected because its native cascade would perform the deletion. The migration fixture and normal-removal assertions also omit `dashpay_profiles` and `dashpay_payments_overlay`, despite both being part of the advertised cleanup contract. Parameterize the upgrade test over foreign keys ON/OFF, seed and check both DashPay tables, and retain live-identity controls. Include those tables in the normal-removal fixture and assertions as well.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [NITPICK] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:188-190: Update the scope-guard reference to the identities DELETE
This comment describes the key-removal guard as mirroring the `identities` tombstone, but this PR replaces that operation with the wallet-scoped physical DELETE in `identities::apply_removals`. The guard remains correct; update its reference so readers are directed to the current mechanism.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Three pre-existing clippy failures on the base branch — The cited fixture, manager, and feature-gated import cleanup is outside the identity-deletion change. These reported baseline lint issues are routine adjacent work, not in-scope findings or exceptional follow-ups.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Match the enabled type's derives and conditional serde support. Check public trait parity and serde round-tripping with shielded off and on. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Validate both owner readers while accepting current and legacy standard labels. Strict loading fails on invalid labels; Recovery isolates the wallet so its used-address guard is never silently discarded. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Replace manual wipe-on-drop code with a zeroizing boxed block slice. Keep the matrix length stable and verify its contents are wiped in place. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
…orage base Preserve current migration history, append identity hard delete as V018, and retain deterministic recovery loading and per-wallet degradation context. Co-authored-by: Codex <noreply@openai.com>
…nto hard delete Co-authored-by: Codex <noreply@openai.com>
Base automatically changed from
feat/platform-wallet-storage-rehydration
to
v4.2-dev
September 9, 2026 13:59
Replay buffered removal and re-addition in one transaction, retaining each identity incarnation's snapshot merge semantics and retry ordering. Sweep deferred contact-crypto rows on removal and migration, and correct orphan ignored-sender documentation. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Exercise V018 with foreign keys enabled and disabled, including DashPay profiles, payment overlays, and token metadata with live identity controls. Check DashPay cleanup on normal removal and update the key scope comment. Co-Authored-By: Codex <noreply@openai.com>
thepastaclaw
approved these changes
Sep 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The reviewed range correctly implements permanent identity removal, including ordered manual-flush segments, dependent-row cleanup, historical tombstone migration, and recovery-mode handling. The current head addresses all four prior findings with targeted documentation, migration, cleanup, and test changes; no additional in-scope correctness, security, or architectural defects were found.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This is a large storage and migration refactor that changes identity deletion semantics, cascades across keys, contacts, balances, and DashPay data, and introduces ordered transactional/retry behavior with potentially irreversible data-loss and recovery consequences. - Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Removing an identity now deletes its stored record, keys, contacts, balances, and DashPay data. Re-adding it starts with fresh identity-owned data, including when removal and re-addition are saved together.
User story
As a developer integrating this SDK (DET, iOS/Android), I want removing an identity to clean up its stored data, so removed identities do not leave keys, contacts, and metadata behind.
Scenario
Base flow
A host app removes a wallet-owned or out-of-wallet identity and may later add the same identity again.
Actual behavior
Removal only marks the stored identity as deleted, retaining its dependent data. Memory already drops the managed identity, so adding it again can bring old stored keys back on the next load.
Expected behavior
After successful persistence, removal deletes the identity and its dependent records. A later addition keeps only its new identity-owned data. A manually flushed removal/re-addition sequence preserves that order and commits atomically.
Detailed discussion
What was done
V018__identity_hard_delete.rsaddscascade_children_on_identity_delete, purges historical tombstones and their dependents, then dropsidentities.tombstoned. V001–V017 remain unchanged.identity_keys,contacts,ignored_senders, andpending_contact_cryptoby owner identity. SQLite's composite foreign key is dormant for NULL-scoped keys under MATCH SIMPLE; the other three tables have no identity foreign key. Existing cascades remove token balances, DashPay profiles and payment overlays, while existing triggers clean identity/token metadata and metadata attached to deleted contacts.idx_contacts_owner,idx_ignored_senders_owner, andidx_pending_contact_crypto_owner. Historical dependent-row purges are explicit so they also work with foreign-key enforcement disabled.PendingWriteskeeps removal/re-addition boundaries as ordered segments. Each segment applies identity upserts and child writes before identity removals; all segments commit in one transaction. The old incarnation is cleaned before a later segment recreates it. Identity normalization preserves unrelated identities' revision and snapshot merge semantics across boundaries, and slot admission uses the projected final identity state.MissingIdentityOwnerreplacesTombstonedIdentityOrphan. Recovery counts/logs skipped key/contact rows with missing owners; Strict returns an error.SCHEMA.mdand rustdoc describe the deletion and ordering contract.Scope and base
This PR targets
v4.2-devand builds on the storage/rehydration work from #3968. The reviewed diff is against basea4bb6a63103a6870c1ac8d6884eb1357f91a762a.Wallet-scoped DPNS marketplace state and identity-scan progress remain independent of identity removal. Existing documented retention of metadata whose parent never existed remains unchanged; this PR does not add general orphan-metadata garbage collection.
Testing
Verified at
9756bae1e701e39674d3ed5861724887d9c83cf0:platform-wallet-storage; Clippy used--all-targets --all-features --locked -- --no-deps -D warnings.e8904f844852d3530cd8f492f522c4f206844936also passed 169 targeted storage/identity-manager tests and formatting/Clippy for both wallet packages. The follow-up changes are confined to tests and a comment.Breaking changes
identities.tombstoned; direct SQL consumers of that column must update.Checklist
!and breaking changes are describedSCHEMA.md, rustdoc)🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes