test(platform-wallet): cover insert_platform_node_pool_entry and its error paths - #4628
Open
Claudius-Maginificent wants to merge 399 commits into
Open
test(platform-wallet): cover insert_platform_node_pool_entry and its error paths#4628Claudius-Maginificent wants to merge 399 commits into
Claudius-Maginificent wants to merge 399 commits into
Conversation
…n table Migration log stopped at V001; V002 (ADDR-09 height pin), V003 (#3968 unified migration), and V004 (#4113 provider key accounts) were never added. Also documents the new provider_platform_node_keys child table and corrects account_registrations' PK, which SCHEMA.md listed as 3 columns while V001 always declared 6 (key_class + DashPay pair). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independently ran cargo test/clippy on platform-wallet-storage, constructed and verified an adversarial duplicate-account-type changeset (silent node-key-batch data loss, QA-001), and audited testspec-4113.md coverage gaps (shrink-direction re-persist, node_id/key_index edge cases untested).
…keys `apply_provider_registrations` cleared a provider account's node-key rows before re-inserting the incoming batch. The pool is hardened-only (Ed25519/SLIP-10), so any key the store forgets is one no watch-only wallet can ever re-derive — and two live callers hand it a batch that is shorter than what is already persisted: - registration falls back to an empty batch when pre-derivation fails (`wallet_lifecycle::register_wallet` treats that as non-fatal); - `Merge` is append-only `.extend()`, so one flush can carry two entries for the same account, and the second one wins. Node keys are now upserted per `key_index` and never deleted: a shorter or empty batch says nothing about the missing indices rather than retracting them. Guarding the DELETE on a non-empty batch would have fixed only the first path — the merged-entry case passes a non-empty batch. Three regression tests, each confirmed failing against the previous writer: shrinking batch, empty batch, and two merged entries for one account. Also in this commit: - Extract `rebuild_provider_key_account` into `platform-wallet`; the SQLite and FFI restore paths were two verbatim copies of the same watch-only rebuild (same constructors, same inserters, differing only in error type). Both now call it and map the one error into their own. - Narrow `ProviderKeyRegistrationBlob`'s rustdoc: it is the SQLite payload shape, not a cross-backend wire contract. The FFI bincodes the bare key; what the backends share is the `account_type` discriminator, nothing more. - Trim the V004 header and two over-long doc comments to the internal cap. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two writers share `account_registrations`: one PK space, one blob column, discriminated only by `account_type`. The reader validated the curve/type pairing; the writer trusted its caller. A `ProviderOperatorKeys` entry carrying an EdDSA key — reachable through the public `store()` and a `pub` changeset field — would upsert onto the operator account's row with a payload the fail-hard reader then rejects, making the whole wallet unopenable on the next `load()`. The writer now enforces the same invariant, before any SQL runs, so a mis-paired entry is refused instead of stored as a landmine. Duplicate entries for one account in a single flush are now decided, not left to write order. `Merge` is append-only, so a re-emitted registration can ride the same flush as the original; identical entries reconcile (node keys union by index — nothing is lost). Two entries that disagree about the account's own extended public key are a contradiction no merge semantic can resolve: one is wrong and the store cannot tell which, so both are refused (`ProviderKeyAccountConflict`) rather than letting the last write win. Union, not rejection, is the answer for the node-key batches themselves: they are hardened-only (Ed25519/SLIP-10) and unrecoverable, every key in either batch is a legitimate key of the same account, and erroring would fail the whole flush — including the unrelated sub-changesets riding it — over an anomaly with a lossless reading. Tests (both guards confirmed failing against c1349e6 first): mis-paired curve rejected at write time with no row left behind; conflicting duplicates rejected with neither written. Plus two coverage tests, no bug behind either: provider rows + node keys + domain-seq bump all roll back together on a mid-flush failure (mirroring tc_b_012), and a registered account with an empty pre-derived batch round-trips as an account with zero keys, not as no account. Also documents the cross-backend discriminator parity at the match itself rather than only in a commit message. Refs: #4113 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…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>
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>
…error paths Adds unit tests for the platform-node pool entry refactor introduced in #3968: batch validation before mutation, restoring the used/unused bookkeeping on rehydration, rejecting an unmanaged account, and rejecting a missing AbsentHardened pool. Stacked on #3968 (feat/platform-wallet-storage-rehydration) — split out per review to keep that PR's diff outside rs-platform-wallet-storage minimal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
September 8, 2026 12:28
Contributor
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
Collaborator
|
🕓 Queued for automated review — 5th in line, estimated start in ~2.0 h (commit 041a9d3)
|
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>
Merge the latest storage branch and move private pool helper tests beside the helper. Keep wallet batch atomicity coverage using KeyDerivation and check missing hardened pools through storage's strict insertion path. Co-Authored-By: Codex <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
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)
Verified the exact-head delta c01d229..3cab3e5 and the surrounding implementations. The five added tests cover batch prevalidation, restored usage bookkeeping, usage downgrades, and missing-account/pool errors without changing production behavior; no actionable in-scope findings were identified. Targeted storage tests were independently attempted with Rust 1.92.0 but blocked during offline dependency resolution by the unavailable pinned rust-dashcore revision, so runtime validation remains unconfirmed.
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:
lowbygpt-6-astra(effort low) — The verified diff adds five well-contained unit tests for pool bookkeeping, batch validation, and error paths without modifying production wallet logic, cryptography, or storage behavior. - Phase 1 reviewers: not run (skipped for throughput: 30 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer
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>
Co-authored-by: Codex <noreply@openai.com>
… tests 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
The squash-merge of #3968 (a4bb6a6) has no shared ancestry with the unsquashed rehydration commits this branch already carried, so the add/add on sqlite/provider_accounts.rs conflicted. `theirs` is a strict subset of `ours` (180 additions, 0 deletions), so the branch side is kept whole; it contains the upstream file verbatim plus this PR's insert_platform_node_pool_entry test cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
llbartekll
approved these changes
Sep 9, 2026
llbartekll
left a comment
llbartekll
left a comment
Contributor
There was a problem hiding this comment.
Reviewed exact head 041a9d3. The diff is test-only and the assertions correctly cover batch prevalidation, restored/cleared used-index bookkeeping, and the missing-account/pool error paths. All five added tests pass locally, cargo fmt --check is clean, and the relevant CI checks are green. No blocking findings.
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.
Stacked on #3968 (base branch:
feat/platform-wallet-storage-rehydration) — do not merge before #3968.Why split out
#3968 needed the
insert_platform_node_pool_entryrefactor and the newPlatformNodePoolErrortype inprovider_key_at_index.rsbecausers-platform-wallet-storage'srehydrate.rscalls that function directly to restore persisted platform-node pool entries (including their used/unused bookkeeping) on load. That functional change stays in #3968.The 5 new unit tests covering that refactor are not needed for
rs-platform-wallet-storageto build or pass its own tests, so they're split into this follow-up PR to keep #3968's diff outside the storage crate minimal.What's here
populate_platform_node_pool_validates_batch_before_mutatinginsert_used_platform_node_pool_entry_restores_used_bookkeepinginsert_platform_node_pool_entry_rejects_unmanaged_accountinsert_platform_node_pool_entry_clears_stale_used_bookkeeping_on_downgradepopulate_platform_node_pool_rejects_missing_hardened_poolByte-identical to what was originally in #3968's diff for this file — pure test relocation, no behavior change.
🤖 Generated with Claude Code