fix(platform-wallet): dashpay M1 review findings — rotation, ignore durability, size cap, key selection#4018
Conversation
…bility, size cap, key selection Fixes the six medium+ findings from the multi-agent review of the DashPay M1 changes: 1. Pending-branch rotation supersede: `add_sent_contact_request`'s pending guard no-oped on `contains_key` regardless of `account_reference`, freezing the tracked reference at the first send — the next rotation re-derived an already-broadcast reference and collided forever on the contract's unique index. The pending branch now supersedes on a reference change, mirroring the established branch (persist-before-commit included). 2. Rotation/registration TOCTOU: the drain registers the external account and stamps the self-heal marker under SEPARATE guards, so a rotation sweep could advance `incoming_request` in between — the stamp then marked an account built from the rotated-away xpub as current, permanently silencing `external_account_needs_rebuild`. `note_external_account_registered` now takes the ciphertext the account was built from and skips the stamp on mismatch (the sweep's teardown + rebuild picks up the fresh request). Applies to both the drain and the accept path. 3. `ignore_sender` tombstone over-reach: it emitted `removed_incoming` unconditionally, and the SQLite writer's DELETE had no state filter — ignoring a sender with no pending incoming entry (e.g. an established contact) destroyed the established pair-row with the user's alias/note/hidden/accepted-accounts. Emission is now guarded on an actual removal (the same `removed.is_some()` discipline as the sibling removers), and both pending-tombstone DELETEs are state-filtered as defense in depth. 4. Un-ignore durability (SQLite backend): restore read `entry_blob.ignored_senders` — stale after an un-ignore (which only deletes the table row) and UNION-merged across buffered snapshots — so a restart resurrected the ignore, stickily. The loader now restores from the transactionally-maintained `ignored_senders` table, which is authoritative in both directions. 5. contactInfo `privateData` size cap: no layer capped alias/note, so an over-2048-byte encrypted payload failed broadcast permanently AFTER local metadata was durably persisted (no retry queue) — permanent local/chain divergence. `encode_private_data_bounded` enforces the schema cap (max plaintext 2031 bytes) and the publish path runs it BEFORE the local persist, reusing the bytes at the encrypt step. 6. Own-ECDH-key selection unified: the send path selected the first ENCRYPTION key WITHOUT filtering disabled keys (contactInfo did filter), so a disable-and-replace key rotation hard-failed every new outgoing request on pre-send validation despite an enabled replacement. Both paths now share `select_own_encryption_key` (enabled-only, mirroring `select_recipient_key_index`). Each fix carries a test that was red against the unfixed code: - add_sent_contact_request_rotation_supersedes_pending_reference - stamp_skips_when_registration_raced_a_rotation - ignore_sender_without_pending_incoming_emits_no_tombstone - pending_tombstones_do_not_destroy_established_rows (storage) - tc_p4_020_unignore_survives_restart_table_is_authoritative (storage) - private_data_over_cap_is_rejected_at_cap_is_accepted - own_key_selector_skips_disabled_first_encryption_key (+ sole-disabled) platform-wallet: 388 lib + 9 integration green; platform-wallet-storage full suite green; clippy clean on both; platform-wallet-ffi checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
✅ Review complete (commit ffdf73a) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus (failed); claude general opus (failed); claude rust-quality opus (failed); claude security-auditor opus (failed); codex ffi-engineer gpt-5.5; codex general gpt-5.5; codex rust-quality gpt-5.5; codex security-auditor gpt-5.5; verifier = codex gpt-5.5; specialists = security-auditor, rust-quality, ffi-engineer.
Verified PR head 29c7188 against the reported findings. The Rust cfg-gating issue is real and in scope because the PR adds a cfg(test) helper that depends on items still gated only by __test-helpers; the Swift embedded-NUL concern is real behavior but pre-existing and outside this PR's diff/required fix surface.
🔴 1 blocking
🤖 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/contacts.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:428-440: Align test-only helper cfg gates
`load_ignored_senders` is compiled whenever `cfg(test)` is set, but the names it uses are still compiled only with the `__test-helpers` feature: `Identifier`, `Connection`, `BTreeMap`, and `decode_pair_key`. In a unit-test build of this crate that enables `sqlite` without `__test-helpers`, Rust keeps this function but removes those dependencies before name resolution, so the test target cannot compile. Gate this function only on `__test-helpers`, or widen the dependent imports and helper to the same `#[cfg(any(test, feature = "__test-helpers"))]` condition.
| #[cfg(any(test, feature = "__test-helpers"))] | ||
| pub(crate) fn load_ignored_senders( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<BTreeMap<Identifier, std::collections::BTreeSet<Identifier>>, WalletStorageError> { | ||
| let mut stmt = | ||
| conn.prepare("SELECT owner_id, sender_id FROM ignored_senders WHERE wallet_id = ?1")?; | ||
| let mut map: BTreeMap<Identifier, std::collections::BTreeSet<Identifier>> = BTreeMap::new(); | ||
| let mut rows = stmt.query(params![wallet_id.as_slice()])?; | ||
| while let Some(row) = rows.next()? { | ||
| let owner: Vec<u8> = row.get(0)?; | ||
| let sender: Vec<u8> = row.get(1)?; | ||
| let (owner, sender) = decode_pair_key(&owner, &sender)?; |
There was a problem hiding this comment.
🔴 Blocking: Align test-only helper cfg gates
load_ignored_senders is compiled whenever cfg(test) is set, but the names it uses are still compiled only with the __test-helpers feature: Identifier, Connection, BTreeMap, and decode_pair_key. In a unit-test build of this crate that enables sqlite without __test-helpers, Rust keeps this function but removes those dependencies before name resolution, so the test target cannot compile. Gate this function only on __test-helpers, or widen the dependent imports and helper to the same #[cfg(any(test, feature = "__test-helpers"))] condition.
source: ['codex-general', 'codex-rust-quality']
There was a problem hiding this comment.
Resolved in ffdf73a — Align test-only helper cfg gates 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.
Review follow-up (#4018): load_ignored_senders was gated `any(test, feature = "__test-helpers")` — matching its caller `identities::load_state` — but the names it uses (Identifier, Connection, BTreeMap, decode_pair_key) were gated `feature = "__test-helpers"` only. A crate build with `cfg(test)` set but `__test-helpers` off would compile the function while its dependencies are cfg'd out → name-resolution failure. In practice this is unreachable: the crate's dev-dependency self-reference (Cargo.toml, `features = [..., "__test-helpers"]`) unifies `__test-helpers` into every `cfg(test)` build, so `test ⟹ __test-helpers` holds. But the gate mismatch is a latent trap (and static-analysis flags it), so widen the three imports and decode_pair_key to the same `any(test, feature = "__test-helpers")` condition as the function and its caller. Verified: `cargo clippy --tests` clean (no unused imports), lib-only `--no-default-features --features sqlite` (production-shaped, widened items absent) compiles, full test suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed in ffdf73a. The gate mismatch is real and I aligned it: Why not the other direction (narrow the function to One note on reachability: this crate's dev-dependency self-reference pins platform-wallet-storage = { path = ".", default-features = false, features = ["sqlite", "cli", "secrets", "kv", "__test-helpers"] }so Verified: |
QuantumExplorer
merged commit 0ee06e1
into
feat/dashpay-m1-sync-correctness
There was a problem hiding this comment.
Code Review
Source: reviewers = claude general opus; codex general gpt-5.5; claude security-auditor opus; codex security-auditor gpt-5.5; claude rust-quality opus; codex rust-quality gpt-5.5; claude ffi-engineer opus; codex ffi-engineer gpt-5.5; verifier = claude opus; specialists = security-auditor, rust-quality, ffi-engineer.
Prior reconciliation: prior-1 [blocking] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs "Align test-only helper cfg gates" is FIXED at ffdf73af. The Identifier, Connection, BTreeMap imports and decode_pair_key helper now use #[cfg(any(test, feature = "__test-helpers"))], matching load_ignored_senders and its caller identities::load_state.
Carried-forward prior findings: none.
New findings in latest delta (29c7188a..ffdf73af): none.
Cumulative current-head findings: none. All reviewer lanes converged on the prior blocker being fixed, and no new in-scope issues were verified in the latest delta or cumulative PR. Approving.
2 participants
Issue being fixed or feature implemented
Stacked on #3841 (base:
feat/dashpay-m1-sync-correctness). Fixes the six medium-severity-and-above findings from the multi-agent review of the DashPay M1 platform-wallet changes (review summary covers the earlier FFI batch; this PR addresses the wallet-core findings).What was done?
Pending-branch rotation supersede —
add_sent_contact_request's pending guard no-oped oncontains_keyregardless ofaccountReference, freezing the tracked reference at the first send; the next rotation re-derived an already-broadcast reference and collided forever on the contract's($ownerId, toUserId, accountReference)unique index. The pending branch now supersedes on a reference change, mirroring the established branch (persist-before-commit ordering included).Rotation/registration TOCTOU — the deferred-crypto drain registers the external account and stamps the rotation self-heal marker under separate guards; a rotation sweep landing in between made the stamp mark an account built from the rotated-away xpub as current, permanently silencing
external_account_needs_rebuild(sends would derive addresses the contact no longer watches).note_external_account_registerednow takes the ciphertext the account was built from and skips the stamp on mismatch; the sweep's teardown+rebuild then picks up the fresh request. Applies to both the drain and the accept path.ignore_sendertombstone over-reach — it emittedremoved_incomingunconditionally, and the SQLite writer's DELETE had no state filter, so ignoring a sender with no pending incoming entry (an established contact, or one that raced auto-establish) destroyed the established pair-row including the user's alias/note/hidden/accepted-accounts. Emission is now guarded on an actual removal (sameremoved.is_some()discipline as the sibling removers), and both pending-tombstone DELETEs are state-filtered as defense in depth.Un-ignore durability (SQLite backend) — restore read the identity
entry_blob'signored_senderssnapshot, which goes stale on un-ignore (only the table row is deleted; no fresh identity flush) and is UNION-merged across buffered snapshots — a restart resurrected the ignore, stickily. The loader now restores from the transactionally-maintainedignored_senderstable via a newload_ignored_sendersreader; the table is authoritative in both directions. (Latent untilWallet::from_persistedrehydration is wired; the FFI/Swift backend was already correct.)contactInfo
privateDatasize cap — nothing capped alias/note, so an over-2048-byte encrypted payload failed broadcast against the contract'smaxItemspermanently AFTER step 1 already durably persisted the local metadata (no retry queue) — permanent local/chain divergence. Newencode_private_data_boundedenforces the schema cap (max plaintext 2031 bytes = 2048 − IV − min PKCS7 pad) and the publish path runs it as step 0, before any local persist, reusing the encoded bytes at the encrypt step. Rejects rather than truncates (alias/note are user-visible verbatim).Own-ECDH-key selection unified — the send path selected the sender's first ENCRYPTION key without filtering disabled keys, while the contactInfo path filtered them: after a disable-and-replace key rotation, every new outgoing contact request hard-failed pre-send validation ("Sender key N is disabled") despite an enabled replacement, and the two surfaces disagreed about the ECDH root. Both paths now share
select_own_encryption_key(enabled-only, mirroringselect_recipient_key_index).How Has This Been Tested?
TDD: every fix carries a test that was red against the unfixed code:
add_sent_contact_request_rotation_supersedes_pending_reference(state)stamp_skips_when_registration_raced_a_rotation(network, mock-SDK harness)ignore_sender_without_pending_incoming_emits_no_tombstone(state)pending_tombstones_do_not_destroy_established_rows(storage)tc_p4_020_unignore_survives_restart_table_is_authoritative(storage, both directions)private_data_over_cap_is_rejected_at_cap_is_accepted(codec, incl. exact-boundary)own_key_selector_skips_disabled_first_encryption_key+own_key_selector_errors_when_only_encryption_key_is_disabledSuites:
platform-wallet388 lib + 9 integration green;platform-wallet-storagefull suite green;cargo clippy --all-targetsclean on both;cargo check -p platform-wallet-ffigreen.Breaking Changes
None.
note_external_account_registeredgained a parameter but ispub(crate);encode_private_datais unchanged (the bounded variant is additive); the SQLite schema is unchanged (reader added, DELETEs narrowed).Checklist:
🤖 Generated with Claude Code