feat: complete dashpay in platform wallet and swift example app#3841
Conversation
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds DashPay SPEC/research docs and implements DIP-15 compact-xpub handling, tightened key-purpose validation, rejected-request tombstones and payment-channel-broken tracking, SDK writer seam, recurring DashPay sync manager, incoming-payment recording/reconciliation, FFI extensions (payments/sync/persistence/seed attach), SwiftData models, and SwiftExampleApp UI and tests. ChangesDashPay Spec & Research
Crypto & SDK
Validation, State & Storage
FFI & Persistence
SDK writer seam & wallet integration
Contact flow refactor
Payments, reconciliation & event bridge
Recurring sync manager
Swift SDK and Example App
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 5b7c317) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (1)
806-875:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe accept-adopt check is only local, not platform-aware.
already_reciprocatedis derived from localsent_contact_requests/established_contacts, but the sync code above explicitly allows "received loaded, sent fetch failed" by logging and continuing. In that state the reciprocal already exists on Platform whilealready_reciprocatedis still false here, so this path retries the same(ownerId, toUserId, accountReference)write and gets the unique-index rejection instead of adopting. This needs a platform check here, or a duplicate-send fallback that switches to the adopt path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs` around lines 806 - 875, The local-only already_reciprocated check (variable already_reciprocated) can be stale; change the flow so before attempting send_contact_request_with_external_signer you either (A) perform a platform check for an existing reciprocal contact request/relationship (use whatever network client/query you have for checking platform contact requests for (ownerId,toUserId,accountReference)) and set already_reciprocated accordingly, or (B) keep the existing local check but add a duplicate-send fallback: catch the unique-index conflict/error returned by send_contact_request_with_external_signer and, on that specific error, log that the reciprocal exists on Platform and run the adopt path (call register_contact_account(&our_identity_id, &sender_id, 0) and treat as success). Reference already_reciprocated, send_contact_request_with_external_signer, and register_contact_account when implementing either fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dashpay/research/01-dip-spec.md`:
- Line 131: Several fenced code blocks use plain ``` without a language tag;
update each triple-backtick fence in the document (e.g., the blocks currently
shown as ``` at the indicated locations) to include an explicit language token
(for non-code or prose use `text`, or a specific language like `json`, `bash`,
`markdown` where applicable) so the markdown linter passes; search for all
occurrences of ``` (including the ones noted around 131, 194, 245, 289, 418,
455) and replace them with ```text or the appropriate language identifier.
In `@docs/dashpay/research/02-rust-dashcore-keywallet.md`:
- Line 232: The markdown contains fenced code blocks without language tags;
update the offending triple-backtick fences to include the appropriate language
identifier (e.g., ```rust, ```bash, or ```text) for the code snippets so
markdownlint passes and syntax highlighting works—locate the plain ``` fences in
the document (the blocks referenced in the review) and replace them with
language-tagged fences.
In `@docs/dashpay/research/05-swift-app.md`:
- Line 47: The fenced code block currently uses a bare triple-backtick fence
(```); add a language tag (e.g., ```swift or ```text) immediately after the
opening backticks to satisfy markdownlint and enable proper syntax highlighting
for that block.
In `@docs/dashpay/research/06-interop-desk-check.md`:
- Line 366: The fenced code block uses plain ``` without a language tag; update
the opening fence to include an appropriate language identifier (for example
`http`, `text`, or `bash`) so markdownlint is satisfied and readability
improves—locate the triple-backtick fenced block in the document and add the
language tag immediately after the opening ``` fence.
- Line 24: The table row contains an extra leading column ("2") so it has four
columns while the table header defines three; remove the extra column in the row
that contains "2" (the row with "ECDH shared-key derivation" and the
libsecp256k1 SHA256 expression) so the row matches the 3-column header layout,
keeping the description "ECDH shared-key derivation" and the verdict "**PASS** —
all three stacks compute libsecp256k1-style `SHA256((y[31]&0x1|0x2) ‖ x)`" as
the remaining columns.
In `@docs/dashpay/SPEC.md`:
- Line 111: Several fenced code blocks in SPEC.md are missing language
identifiers; update each triple-backtick fence (``` ) at the noted examples so
they include an appropriate language tag (e.g., change ``` to ```text, ```rust,
or ```swift as appropriate) to satisfy markdownlint and enable correct syntax
highlighting; search for the bare ``` occurrences (including the ones referenced
near the examples) and replace them with language-tagged fences, ensuring
opening and closing fences remain paired.
In `@packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- Around line 221-223: The background loop cleanup currently unconditionally
sets this.background_cancel to None (in the block near start()), which can
overwrite a newer token if stop() and start() race; change the logic so the
background thread only clears background_cancel if the stored cancel token it
captured at spawn time still matches the current token in this.background_cancel
(i.e., capture the Arc/ID of the cancel handle when spawning and
compare-before-clearing); apply the same compare-and-clear pattern in the stop()
/ thread-exit cleanup (references: this.background_cancel, start(), stop()) so a
late-exiting old loop cannot null out a replacement token.
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 103-118: The sender/recipient key selection currently using
sender_identity.public_keys().iter().find(...) (checking Purpose::ENCRYPTION and
KeyType::ECDSA_SECP256K1) can pick a disabled/rotated key; update the logic to
only consider active/enabled keys (e.g., filter by .enabled() or reuse the
existing enabled-key selection utility used for signing) so
sender_encryption_key and recipient_key_index (the call to
select_recipient_key_index should be updated similarly or replaced) always
reference the current active ENCRYPTION/DECRYPTION ECDSA_SECP256K1 key; ensure
you still call .map(...).ok_or_else(...) and preserve error type
PlatformWalletError::InvalidIdentityData when no active key is found.
- Around line 516-556: collect_account_build_candidates currently skips contacts
when info.core_wallet.accounts.dashpay_external_accounts.contains_key(&key) is
true, which prevents retries if register_contact_account previously failed after
inserting an external entry; remove that gating so contacts with an
incoming_request (incoming.encrypted_public_key and key indices) are always
returned as AccountBuildCandidate (unless payment_channel_broken) to allow
build_contact_accounts -> register_contact_account to retry; specifically, in
collect_account_build_candidates remove or change the has_external
check/continue and rely on contact.incoming_request and payment_channel_broken
to decide inclusion (keep AccountBuildCandidate fields: contact_id,
encrypted_public_key, our_decryption_key_index, contact_encryption_key_index).
- Around line 452-509: parse_contact_request_doc currently only extracts
required fields and drops optional fields encryptedAccountLabel and
autoAcceptProof, causing restores to lose these values; update
parse_contact_request_doc (and thus parse_sent_contact_request_doc which calls
it) to also read props.get("encryptedAccountLabel").and_then(|v: &Value|
v.as_str()).map(|s| s.to_owned()) and props.get("autoAcceptProof").and_then(|v:
&Value| v.as_bytes()).cloned() (or appropriate conversions) and pass them into
ContactRequest::new (or the appropriate constructor/factory) so the
ContactRequest created preserves encryptedAccountLabel and autoAcceptProof
during ingest/reconcile. Ensure the match arm pattern includes these Option
values and the fallback logging remains unchanged.
In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- Around line 116-130: The code removes an incoming request from
self.incoming_contact_requests but the returned ContactChangeSet only records
cs.rejected, so on replay the incoming entry isn't removed; update the change
set returned by the function to also include the incoming-removal for (owner_id,
*sender_id, account_reference) (i.e., add the corresponding removal entry to the
ContactChangeSet alongside cs.rejected) so that replay will delete the
incoming_contact_requests entry when applying the rejection tombstone.
---
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- Around line 806-875: The local-only already_reciprocated check (variable
already_reciprocated) can be stale; change the flow so before attempting
send_contact_request_with_external_signer you either (A) perform a platform
check for an existing reciprocal contact request/relationship (use whatever
network client/query you have for checking platform contact requests for
(ownerId,toUserId,accountReference)) and set already_reciprocated accordingly,
or (B) keep the existing local check but add a duplicate-send fallback: catch
the unique-index conflict/error returned by
send_contact_request_with_external_signer and, on that specific error, log that
the reciprocal exists on Platform and run the adopt path (call
register_contact_account(&our_identity_id, &sender_id, 0) and treat as success).
Reference already_reciprocated, send_contact_request_with_external_signer, and
register_contact_account when implementing either fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 596c3a94-3c49-4cc0-869e-b392a37c181e
📒 Files selected for processing (38)
docs/dashpay/SPEC.mddocs/dashpay/research/01-dip-spec.mddocs/dashpay/research/02-rust-dashcore-keywallet.mddocs/dashpay/research/03-rs-platform-wallet.mddocs/dashpay/research/04-sdk-and-contract.mddocs/dashpay/research/05-swift-app.mddocs/dashpay/research/06-interop-desk-check.mdpackages/rs-platform-encryption/Cargo.tomlpackages/rs-platform-encryption/src/lib.rspackages/rs-platform-wallet-ffi/src/established_contact.rspackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/manager/accessors.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/validation.rspackages/rs-platform-wallet/src/wallet/identity/network/account_labels.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/contacts.rspackages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/profile.rspackages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-sdk-ffi/src/dashpay/contact_request.rspackages/rs-sdk/src/platform/dashpay/contact_request.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3841 +/- ##
============================================
+ Coverage 87.19% 87.21% +0.02%
============================================
Files 2636 2642 +6
Lines 327822 328272 +450
============================================
+ Hits 285852 286315 +463
+ Misses 41970 41957 -13
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
M1 of the DashPay completion plan: the SDK entropy / DIP-15 compact xpub / key-purpose interop fixes are correct, but six in-scope correctness issues block merge. The most concerning is editing V001 in-place (violates the documented append-only migration policy and bricks DB rehydration for the v4.0.0-beta.4 cohort). Additional blockers: the reject path emits an incomplete ChangeSet (no removed_incoming); the new rejected_contact_requests table is written but never read; transient identity fetches in register_external_contact_account are misclassified as permanent and brick the channel; validation.purpose_mismatch is set even when a hard error is also present, masking permanent failures as retryable; and the sync sweep skips superseding requests from established contacts, making the documented payment_channel_broken recovery path unreachable.
🔴 6 blocking | 🟡 2 suggestion(s)
2 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/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:186-213: V001 migration edited in-place violates append-only policy and breaks upgrade from v4.0.0-beta.4
This PR adds `contacts.payment_channel_broken` and a new `rejected_contact_requests` table by editing V001 directly. V001 (without these additions) was already shipped in `v4.0.0-beta.4` (commit da9d3fe84e / schema confirmed via `git show`), and `packages/rs-platform-wallet-storage/README.md:106` explicitly states migrations are append-only and applied by refinery on every `open`. refinery checksums each migration in `refinery_schema_history`; against an existing v4.0.0-beta.4 DB it will either abort with a divergent-checksum error or silently skip V001 (already applied) — in which case neither the new table nor the new column is ever created, and the first runtime write in `contacts.rs:240` (`INSERT INTO rejected_contact_requests …`) or `contacts.rs:194-212` (`payment_channel_broken` column) fails at the SQLite layer. `tc029_migration_fingerprint_stable` does not catch this because it only checks self-stability, not a pinned hash. Add `V002__dashpay_reject_and_broken_channel.rs` doing `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` and `CREATE TABLE rejected_contact_requests (…)`; the loader at `contacts.rs::load_state` already tolerates NULL `payment_channel_broken`, so a default-less ALTER is compatible.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: `record_rejected_contact_request` removes incoming in memory but does not emit `removed_incoming`
The function calls `self.incoming_contact_requests.remove(sender_id)` and returns a `ContactChangeSet` populated only with `cs.rejected`. The unified-`contacts`-table writer at `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193` only `DELETE`s when `cs.removed_incoming` is non-empty, so the previously persisted state='received' row (with the `incoming_request` blob) stays in SQLite. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the unified contacts reader rebuckets that row as an incoming request, `apply_changeset` re-inserts it into `incoming_contact_requests`, and the FFI surfaces the explicitly-rejected request back to the UI. The persisted delta is also internally inconsistent with the in-memory mutation — a delta-persistence invariant violation. The in-memory `rejected_tombstone_round_trips_and_respects_account_reference` test does not catch this because it round-trips via `apply_changeset`, not the SQLite reader. Fix by also inserting a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: `rejected_contact_requests` is written but never read — tombstones lost across restart
The PR adds a writer (`contacts.rs:240`), a migration row (`V001__initial.rs:203`), and an `apply_changeset` branch that restores `ManagedIdentity.rejected_contact_requests` from `cs.rejected`. But `managed_identity_from_entry` hard-codes `rejected_contact_requests: Default::default()` (line 214), and grep confirms no `load_state` reader for the new table. Once `persister.load()` (TODO at `sqlite/persister.rs:909`) is wired up, the in-memory tombstone map is always empty after restart even though SQLite holds the rows. `is_request_rejected` then returns `false`, the sweep's tombstone-skip in `network/contact_requests.rs:396-404` does not fire, and the recurring DashPay loop (G12) resurrects every rejected request on the first sweep — exactly the M1 failure mode the SPEC.md cites as the reason G5 must land with G12. Add a per-wallet `load_state` for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-728: Transient identity fetch failures inside account-build are marked as permanent
`build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. But `register_external_contact_account` (`network/contacts.rs:400-407`) performs another `Identity::fetch` for the same contact and wraps the DAPI/network error as `PlatformWalletError::InvalidIdentityData`. A transient DAPI hiccup after validation therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter at line 530, and recovery only fires if a superseding contactRequest happens to arrive — contradicting the policy in the docstring at lines 573-578 ("Transient (identity fetch / network): logged, left for the next sweep to retry. The broken flag stays clear."). The fix is to perform the contact-identity fetch and treat its failure as transient *before* calling `register_external_contact_account` (mirroring the existing fetch at lines 631-655) and to scope the permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-392: Superseding requests from established contacts are skipped — `payment_channel_broken` recovery is unreachable
The received-side ingest drops every doc whose sender is already in `established_contacts` before consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken = false` when a fresh request flows in (see `types/dashpay/established_contact.rs:84-104`), and `collect_account_build_candidates` documents the recovery contract at lines 528-529 ("never retry a permanently-broken channel — wait for a superseding request (which clears the flag on re-establish)"). But there is no path that reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` similarly returns early for established contacts. Net effect: once `payment_channel_broken` is set, it stays set forever. Either (a) detect a superseding incoming request (new `accountReference` for the same sender) and route it through the reestablish path, or (b) change the broken-channel policy so the next sweep can retry under controlled conditions.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:5733-5749: `select_recipient_key_index` returns disabled keys
The send-side recipient-key selector iterates `recipient_identity.public_keys()` and returns the first key whose purpose is DECRYPTION (then ENCRYPTION) and whose type is ECDSA_SECP256K1, with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on disabled keys, so if a preferred DECRYPTION key has been rotated/disabled this selector returns it anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Add `&& k.disabled_at().is_none()` to both branches so selection is consistent with validation.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:50-62: `purpose_mismatch` is set even when a non-purpose hard error is also present
The docstring at lines 19-29 contracts `purpose_mismatch` as `true` *only* when the sole reason for invalidity is a key-purpose mismatch — it is what tells `build_contact_accounts` at `network/contact_requests.rs:689` to treat the failure as a non-permanent skip instead of marking the channel permanently broken. The implementation does not preserve that invariant: `add_purpose_error` unconditionally sets `purpose_mismatch = true`, and `add_error` never clears it. A request whose key has both a wrong key type (hard, permanent error) and a wrong purpose ends up with `is_valid = false, purpose_mismatch = true`, and the caller skips + retries forever instead of marking broken. The mobile testnet census makes this rare in practice today, but the classifier is the load-bearing primitive the recovery policy is built on — fix `add_error` to clear the flag, and fix `add_purpose_error` to only set it if no prior hard error was recorded.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-227: `stop()` followed quickly by `start()` can let the old thread null out the new cancel token
`stop()` takes the current `background_cancel`, sets it to `None`, then cancels the old token. The spawned thread exits its loop on cancellation and at lines 221-223 re-acquires the guard and writes `*guard = None`. If `start()` runs between `stop()` and the old thread's cleanup block, the old thread's final clear will overwrite the new token just installed by `start()` — leaving `background_cancel` empty while a fresh sync thread is still running, so a subsequent `stop()`/`quiesce()` will be a no-op against that running thread. The normal shutdown path (`quiesce` waits for in-flight passes) does not hit this, but bare `stop()`/`start()` races can. Fix by capturing the token at spawn time and only clearing the guard if it still holds that same token (`if matches!(*guard, Some(t) if Arc::ptr_eq(...)) { *guard = None; }`).
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift (1)
421-465: ⚡ Quick winAlso assert the payment rows roll back in this atomicity test.
The doc comment says a mid-round
persistDashpayPaymentswrite must ride the open changeset and roll back with it, but the test only checksPersistentDashpayContactRequest. If payment persistence starts auto-saving again, this still passes. Add aPersistentDashpayPaymentfetch before and afterendChangeset(..., success: false)so the regression is pinned end-to-end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift` around lines 421 - 465, In testPaymentRefreshDoesNotCommitAnOpenChangesetRound, add assertions that verify the payment row staged by persistDashpayPayments is not visible mid-round and is rolled back after endChangeset(..., success: false); specifically, call the existing payment-fetch helper (or add/rename a fetch function for PersistentDashpayPayment rows) to assert count == 0 immediately after the mid-round persist and again after handler.endChangeset(..., success: false), mirroring the contact-row assertions so the test verifies payment atomicity as well as contact atomicity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 1919-1925: persistDashpayPayments is swallowing failures from
backgroundContext.save() via try?, which can silently drop payment-history
updates; change the save to propagate or log errors instead of ignoring them:
replace the try? backgroundContext.save() with a throwing or do/catch path
inside persistDashpayPayments that captures the thrown error from
backgroundContext.save(), records telemetry/logging (or rethrows to the caller)
with context (e.g., include which payment batch or wallet ID), and preserve the
existing inChangeset check (if !self.inChangeset) so the save still only runs
when appropriate; update callers or function signature as needed to handle
propagated errors or ensure telemetry is emitted in the catch.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 35-40: The optimisticSentIds and ownProfile state are
identity-scoped but currently persist across identity switches; update the
activeIdentity handling (the Task that observes activeIdentity) to reset
identity-scoped UI state at the start of the task: clear optimisticSentIds and
set ownProfile to nil (or otherwise remove cached profile) before loading;
alternatively refactor optimisticSentIds and ownProfile to be keyed by owner
identity (e.g., a dictionary keyed by activeIdentity.id) and read/write via that
key, and ensure loadOwnProfileFromCache() does not retain the previous profile
on read failure for the new identity but returns nil so the UI doesn’t show the
old identity’s data.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift`:
- Around line 1235-1243: The avatar downloader currently accepts any parseable
URL, allowing non-HTTPS schemes; update fetchAvatarBytes to explicitly validate
the URL scheme and reject anything not exactly "https" before creating the
URLRequest, returning an error (or nil) for non-https inputs; locate
fetchAvatarBytes (and the analogous implementation referenced around lines
1390-1410) and add a guard that checks url.scheme?.lowercased() == "https" and
fails early with a clear error to prevent http or other schemes from being
fetched.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swift`:
- Around line 92-97: Replace the immediate existence check on the toolbar
refresh button with a timed wait to avoid flakes: locate the `refresh` query
using `Identifier.refreshButton` in DashPayTabUITests (variable `refresh`) and
change the assertion to call `refresh.waitForExistence(timeout: ...)` instead of
checking `refresh.exists`, keeping the same failure message; choose a reasonable
timeout (e.g., 1–5s) consistent with other tests.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift`:
- Around line 421-465: In testPaymentRefreshDoesNotCommitAnOpenChangesetRound,
add assertions that verify the payment row staged by persistDashpayPayments is
not visible mid-round and is rolled back after endChangeset(..., success:
false); specifically, call the existing payment-fetch helper (or add/rename a
fetch function for PersistentDashpayPayment rows) to assert count == 0
immediately after the mid-round persist and again after
handler.endChangeset(..., success: false), mirroring the contact-row assertions
so the test verifies payment atomicity as well as contact atomicity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c0b4a7c-c449-41c7-bd16-7979ff30c777
📒 Files selected for processing (42)
docs/dashpay/SPEC.mdpackages/rs-platform-wallet-ffi/src/contact_persistence.rspackages/rs-platform-wallet-ffi/src/dashpay_payment.rspackages/rs-platform-wallet-ffi/src/dashpay_sync.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/manager/attach_seed.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/manager/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/contacts.rspackages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-sdk-ffi/src/sdk.rspackages/rs-sdk/src/platform/dashpay/contact_request_queries.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayContactRequest.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentDashpayPayment.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/DashPayPayment.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedIdentity.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerDashPaySync.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/SwiftExampleAppApp.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactRequestsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/ContactsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayContactMeta.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayProfileView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/SendDashPayPaymentSheet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppUITests/DashPayTabUITests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FriendsView.swift
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/AddContactView.swift
- docs/dashpay/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
- packages/rs-platform-wallet/src/manager/mod.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift (1)
23-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the persisted active-identity key by network.
dashpay.activeIdentityIdis shared across every network, so selecting an identity on testnet/devnet overwrites the remembered choice for mainnet too. When the user switches back,activeIdentityfalls back to the first eligible identity instead of restoring the last selection on that network.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift` around lines 23 - 26, The persisted AppStorage key stored in DashPayTabView (`@AppStorage("dashpay.activeIdentityId") private var storedIdentityId`) is global across networks; change it to be network-scoped by deriving the key from the current network identifier (e.g., include network.rawValue or chainId) so each network has its own stored key. Update DashPayTabView to compute the AppStorage key at runtime (or use a computed property / wrapper that returns "dashpay.activeIdentityId.\(networkId)") using the view’s network/environment value and ensure storedIdentityId is read/written through that network-scoped key so switching networks preserves separate selections.
♻️ Duplicate comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift (1)
169-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset identity-scoped state before loading the next identity.
optimisticSentIdsandownProfilestill survive an identity switch, andloadOwnProfileFromCache()explicitly keeps the previous profile on a read failure. That can render identity A's pending-request overlay or profile header under identity B until the cache catches up. Clear those fields at the start of the.task(id:)block, and don't retain the previousownProfilein the failure path for the new identity.Also applies to: 420-433
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift` around lines 169 - 177, The task block keyed by .task(id: activeIdentity?.identityId) is not resetting identity-scoped state: clear optimisticSentIds and ownProfile immediately at the top of that task before calling loadOwnProfileFromCache() and walletManager.dashPaySyncNow(); and update loadOwnProfileFromCache() so that on a cache read failure for the new identity it does not retain the previous ownProfile (set ownProfile to nil or replace with an empty/default value) instead of keeping the old profile. Ensure the reset refers to the existing properties optimisticSentIds and ownProfile and the loadOwnProfileFromCache() function so the UI doesn't show the previous identity while the new identity's cache is loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swift`:
- Around line 35-42: The visible-empty-state logic is still checking the raw
accounts collection instead of the filtered/sorted list, causing the UI to hide
the "No Accounts" state when only accountType 12/13 are present; update the
empty-state checks to use orderedAccounts.isEmpty (or introduce a
visibleAccounts computed collection that filters out accountType 12/13 and reuse
it everywhere) and replace any usages of accounts.isEmpty / !accounts.isEmpty in
AccountListView with checks against that filtered collection so the UI matches
the displayed list (keep AccountListView.sortKey as the sorting helper).
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 23-26: The persisted AppStorage key stored in DashPayTabView
(`@AppStorage("dashpay.activeIdentityId") private var storedIdentityId`) is
global across networks; change it to be network-scoped by deriving the key from
the current network identifier (e.g., include network.rawValue or chainId) so
each network has its own stored key. Update DashPayTabView to compute the
AppStorage key at runtime (or use a computed property / wrapper that returns
"dashpay.activeIdentityId.\(networkId)") using the view’s network/environment
value and ensure storedIdentityId is read/written through that network-scoped
key so switching networks preserves separate selections.
---
Duplicate comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swift`:
- Around line 169-177: The task block keyed by .task(id:
activeIdentity?.identityId) is not resetting identity-scoped state: clear
optimisticSentIds and ownProfile immediately at the top of that task before
calling loadOwnProfileFromCache() and walletManager.dashPaySyncNow(); and update
loadOwnProfileFromCache() so that on a cache read failure for the new identity
it does not retain the previous ownProfile (set ownProfile to nil or replace
with an empty/default value) instead of keeping the old profile. Ensure the
reset refers to the existing properties optimisticSentIds and ownProfile and the
loadOwnProfileFromCache() function so the UI doesn't show the previous identity
while the new identity's cache is loaded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cf1916b-35bc-47ca-bb9d-48b3d9493945
📒 Files selected for processing (4)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/DashPayTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
💤 Files with no reviewable changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/IdentityDetailView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All 8 prior findings against 9f770b8 remain STILL VALID at a51606d — verified directly against the worktree (V001 unchanged, record_rejected_contact_request still omits removed_incoming, no reader for rejected_contact_requests, transient identity fetch still permanently breaks channels, purpose_mismatch still sticky, established-contact ingest still skips superseding requests, dashpay_sync cleanup still clobbers cancel token unconditionally, select_recipient_key_index still ignores disabled_at). The M2 delta also introduced one new blocker: Swift wallet deletion does not pre-delete the newly added PersistentDashpayPayment children whose owner inverse is non-optional, mirroring the contact-request pattern that the surrounding comment explicitly calls out as fatal. One FFI suggestion is worth flagging: the new DashpayPaymentFFI derives Copy despite owning two *mut c_char allocations reclaimed by dashpay_payment_array_free. Overflow: 1 valid suggestion dropped (register_external_contact_account persist outside write lock — conf 0.55).
🔴 2 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests is written but never read — tombstones lost across restart
Verified at HEAD: managed_identity_from_entry still hard-codes rejected_contact_requests: Default::default() at line 214. The writer at contacts.rs:240 (INSERT INTO rejected_contact_requests) and migration row at V001:203 exist, but there is no load_state reader for the new table and apply_changeset only handles live deltas — restored state is always empty. The recurring DashPay loop's tombstone-skip at network/contact_requests.rs:396-404 never fires after a restart, so a rejected contact request is resurrected on the first sweep. Add a per-wallet load_state for rejected_contact_requests and route its output into the ContactChangeSet.rejected synthesized during rehydration.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:2980-2996: Wallet deletion omits new DashPay payment children before deleting identities — same fatal pattern as contact requests
Verified: PersistentDashpayPayment.owner is declared as non-optional (`public var owner: PersistentIdentity`), and the new cascade relationship was added on PersistentIdentity.dashpayPayments in the M2 delta. The PHASE 1 pre-delete loop at lines 2986-2996 iterates dpnsNames, dashpayProfile, and contactRequests — but not dashpayPayments. The surrounding comment (lines 2962-2978) explicitly states this phase exists because SwiftData fatals during save() when it must null out a non-optional inverse on a child processed in the same delete batch. dashpayPayments has the exact same shape as contactRequests, so a wallet with persisted DashPay payments can crash or fail to wipe cleanly when deleted. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing pre-deletion.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:245-261: select_recipient_key_index returns disabled keys
Verified at HEAD lines 245-261: the selector iterates recipient_identity.public_keys() and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no disabled_at check. validate_contact_request in crypto/validation.rs does gate on disabled keys, so a recipient with a disabled preferred DECRYPTION key gets returned anyway and the broadcast fails downstream with an opaque error instead of falling through to a usable ENCRYPTION key on the same identity. Beyond reliability, on the send-side this also means the wallet could encrypt the DIP-15 compact xpub to a revoked key whose private half may be compromised. Add `&& k.disabled_at().is_none()` to both branches so selection matches validation.
In `packages/rs-platform-wallet-ffi/src/dashpay_payment.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/dashpay_payment.rs:89-108: DashpayPaymentFFI derives Clone, Copy despite owning *mut c_char strings reclaimed by dashpay_payment_array_free
Verified at HEAD lines 89-108: DashpayPaymentFFI carries two heap-owned C strings (txid, memo — produced via CString::into_raw in cstring_or_null and reclaimed via CString::from_raw in dashpay_payment_array_free), yet the struct is `#[derive(Debug, Clone, Copy)]`. With Copy the compiler will silently shallow-duplicate the struct on any by-value rebinding inside this crate, and a subsequent free walk on the array (or a stray from_raw on the duplicate) would double-free the txid/memo allocations across the FFI boundary. Today's call sites are sound — the struct is built once, moved into Vec → Box<[T]> → Box::into_raw, and reclaimed exactly once — but the Copy derive removes the borrow-checker guardrail that normally prevents this class of bug at refactor time. Sibling FFI types in this crate that own heap pointers (ContactRequestFFI, WalletChangeSetFFI) deliberately omit Copy for exactly this reason. Drop Copy (and Clone if unneeded) on this struct. The cross-boundary contract is unchanged — Swift consumes by raw pointer.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-verified all 10 prior findings against worktree HEAD aabc21e; every one is STILL VALID — none of the three Swift example-app commits or the v3.1-dev merge touched the dashpay-correctness Rust/Swift hotspots. Carrying forward 7 blockers (append-only V001 violation, rejected-request persistence asymmetry, missing tombstone reader, transient-as-permanent channel breakage, sticky purpose_mismatch, unreachable broken-channel recovery, SwiftData wallet-deletion miss on the new payments cascade) and 3 suggestions (sync stop/start cleanup race, send-side disabled-key selection, Copy on FFI-owned C-strings). REQUEST_CHANGES.
🔴 7 blocking
3 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-131: record_rejected_contact_request drops incoming in memory but never persists the deletion
Verified at HEAD lines 109-131: line 116 calls `self.incoming_contact_requests.remove(sender_id)` but the returned `ContactChangeSet` (lines 127-129) only populates `cs.rejected`. The unified contacts-table writer in `rs-platform-wallet-storage/src/sqlite/schema/contacts.rs` only DELETEs incoming rows when `cs.removed_incoming` is non-empty, so the persisted `state='received'` row with its stale `incoming_request` blob stays in SQLite. Once `persister.load()` is wired up, the contacts reader re-buckets that row as an incoming request and `apply_changeset` re-inserts it — the explicitly-rejected request reappears in the UI. The in-memory mutation and the persisted delta are internally inconsistent. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id: *sender_id }` into `cs.removed_incoming` alongside `cs.rejected`.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
Verified at HEAD: `managed_identity_from_entry` still hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer at `contacts.rs:240` (`INSERT INTO rejected_contact_requests`) and the migration row at `V001:203` exist, but no `load_state` reader for the new table exists, and `apply_changeset` only handles live deltas — restored state is always empty. The recurring DashPay loop's tombstone-skip at `network/contact_requests.rs:396-404` therefore never fires after a restart, so a rejected contact request is resurrected on the first sweep and surfaces to the user again. Security framing: the on-platform document is immutable, so the local tombstone is the ONLY thing that suppresses a spammer's repeated contact request — a wipe-on-restart defeats the user's explicit reject. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the `ContactChangeSet.rejected` synthesized during rehydration.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:711-729: Transient DAPI failures inside register_external_contact_account are classified as permanent
Verified at HEAD lines 711-729: `build_contact_accounts` treats ANY error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. `register_external_contact_account` performs a fresh `Identity::fetch` internally and wraps DAPI/network failures as `PlatformWalletError::InvalidIdentityData`. A single transient DAPI hiccup therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery only fires if a superseding contactRequest arrives — but the established-contact ingest skip at line 389 makes that path unreachable. Combined, a transient network event bricks a channel forever, and a malicious or unreliable DAPI endpoint becomes a persistent availability attack against payments to a specific contact. Fix by either passing the pre-fetched identity into registration or scoping permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type mismatch).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:383-404: Established-contact ingest skip makes payment_channel_broken recovery unreachable
Verified at HEAD lines 383-404: the received-side ingest drops every doc whose sender is already in `established_contacts` (line 389) BEFORE consulting `accountReference`. `EstablishedContact::reestablish_preserving_metadata` exists precisely to clear `payment_channel_broken` when a fresh request flows in, and `collect_account_build_candidates` documents the recovery contract ("never retry a permanently-broken channel — wait for a superseding request which clears the flag on re-establish"). But no path reaches `reestablish_preserving_metadata` for an already-established sender from the sync sweep — `add_incoming_contact_request` is only called for new senders here, and the send-side guard at `state/managed_identity/contact_requests.rs:46-48` also returns early for established contacts. Once `payment_channel_broken` is set, it stays set forever. Either detect a new `accountReference` for the same sender and route through the reestablish path, or change the broken-channel policy to permit controlled retry.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:245-261: select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised
Verified at HEAD lines 245-261: the selector iterates `recipient_identity.public_keys()` and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no `disabled_at` check. `validate_contact_request` in `crypto/validation.rs` does gate on `disabled_at`, so the asymmetry creates both a reliability bug (an opaque downstream broadcast failure instead of falling through to a usable key) AND a real confidentiality exposure: on send, the wallet encrypts the 69-byte DIP-15 compact xpub (fingerprint‖chaincode‖pubkey — combined with `accountReference` lets the holder derive every receiving address on that account) to a key the recipient has explicitly revoked. Identity-key revocation is the on-platform mechanism for declaring "the private half of this key may be compromised". Add `&& k.disabled_at().is_none()` to both branches so selection matches validation. (Promoted from suggestion to blocking on the security-auditor confidentiality analysis.)
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3018-3035: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
Verified at HEAD: PHASE 1 (lines 3024-3034) iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional (`PersistentDashpayPayment.swift:98`), and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3018-3023) explicitly states this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 (`save()` after `delete(identity)`), aborting before the wallet row is removed. The user's belief that they wiped DashPay data is wrong, and plaintext memo + counterparty id + amount + txid rows remain on disk. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing pre-deletions.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:192-235: DashPaySyncManager thread cleanup unconditionally clears the cancel token — stop/start race enables use-after-free across FFI
Verified at HEAD lines 192-235: the spawned thread's cleanup at lines 230-232 writes `*guard = None` on loop exit regardless of which token the slot currently holds. If `stop()` cancels the old token and `start()` installs a fresh token before the old thread reaches cleanup, the old thread clears the NEW token — `background_cancel` is empty while a fresh sync thread is still running. `stop()`/`quiesce()` then become a no-op against that running thread. In this PR's Swift integration the persister and DashPay event callbacks close over an UnsafePointer<Context> allocated on the Swift side; calling `dashpay_sync_manager_destroy` (or the wallet manager's drop) after the visible token was cleared frees that context while the surviving thread continues invoking the callbacks against the freed pointer — a concrete use-after-free crossing the C ABI, reachable through normal start/stop/destroy controls (toggling tabs, login/logout) and widened by attacker-influenced network timing. Capture the spawned token and only clear the slot if it still holds the same token (`Arc::ptr_eq`), mirroring `ShieldedSyncManager`'s generation guard. (Upgraded from suggestion to blocking based on the security-auditor and codex-security cross-checks of the destroy/UAF path.)
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reconciliation against HEAD 440ffca: prior finding #6 (established-contact ingest skip) is FIXED by the new rotation path. Nine prior findings remain STILL VALID and the new G3 delta introduces two additional blockers — the send-side rotation-version lookup ignores established_contacts (forcing version=0 collisions after auto-establishment) and the receive-side rotation handler replays immutable historical requests as fresh rotations on every sweep, churning the external account. Total: 10 in-scope blockers. Overflow: 1 suggestion (DashpayPaymentFFI Copy) dropped due to budget.
🔴 5 blocking
2 additional finding(s) omitted (not in diff).
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:165-201: Send-side rotation version lookup ignores established_contacts — re-send after auto-establishment reuses version=0 and collides on the unique index
The new G3 rotation logic computes `previous_version` only from `managed.sent_contact_requests.get(recipient_identity_id)` (line 171). But establishment (`add_incoming_contact_request` line 175 and `apply_established_contact` line 372 in state/managed_identity/contact_requests.rs) explicitly removes the entry from `sent_contact_requests` and parks the prior outgoing request on `EstablishedContact.outgoing_request`. Once the reciprocal arrives and a sweep auto-establishes the pair, the next `send_contact_request` to that recipient sees `previous_version = None` and falls back to `version = 0`. With deterministic xpub/ECDH for the same (sender, recipient) and unchanged `account_index`, the PRF reproduces the same masked `account_reference` as the original sent request. The contract's unique index `($ownerId, toUserId, accountReference)` rejects the broadcast — the exact failure mode G3 was added to prevent. Fall back to `established_contacts[recipient].outgoing_request` (taking the max of both versions if both are present).
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:441-478: Historical contactRequest documents replay as fresh rotations every sync sweep
The rotation guard at line 451 only compares the incoming reference against the currently tracked reference (incoming map or established contact). contactRequest documents are immutable, and `fetch_received_contact_requests(identity_id, None)` (line 370) is unfiltered, so every sweep returns both the original v=0 and any rotated v=N documents. Within a single sweep, ingesting v=0 against an already-tracked v=N flips the established contact back to v=0 and queues a teardown (lines 472-478, then 517-528), and the next document in the same iteration flips it forward again. Across sweeps the same churn replays — the external account is torn down and rebuilt on every cycle, generating wasted DAPI traffic. Worse, if the freshest document falls outside the eventual paginated window (post-M3 growth), the contact can regress to the stale xpub. Compare by `created_at`/version monotonicity, not bare reference inequality: only apply rotation when the incoming request strictly supersedes the tracked one.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:292-308: select_recipient_key_index returns disabled (revoked) keys — DIP-15 compact xpub encrypted to a key whose private half may be compromised
Verified at HEAD lines 292-308: the selector iterates `recipient_identity.public_keys()` and returns the first DECRYPTION (then ENCRYPTION) ECDSA_SECP256K1 key with no `disabled_at` check. `validate_contact_request` does gate on `disabled_at`, so the send/receive interop rules are asymmetric. On send, the 69-byte DIP-15 compact xpub (fingerprint‖chaincode‖pubkey — combined with `accountReference` lets the holder derive every receiving address on that account) is encrypted to a key the recipient has explicitly revoked. Identity-key revocation is the on-platform mechanism for declaring 'this key's private half may be compromised'. Add `&& k.disabled_at().is_none()` to both branches so selection matches validation.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
`managed_identity_from_entry` still hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer at `sqlite/schema/contacts.rs:240` (INSERT INTO rejected_contact_requests) and the V001 row exist, but there is no `load_state` reader for the new table, and `apply_changeset` only handles live deltas — restored state is always empty after restart. The recurring DashPay loop's tombstone-skip at `network/contact_requests.rs:457` therefore never fires after restart, so a rejected request is resurrected on the first sweep. Because the on-platform document is immutable, wiping the tombstone on restart defeats the user's explicit reject. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the rehydration changeset / ManagedIdentity field.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3024-3034: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3008-3023) explicitly documents that this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 `save()`, aborting before the wallet row is removed. The user believes their data was wiped; plaintext memo + counterparty id + amount + txid rows remain on disk. Pre-delete `identity.dashpayPayments` alongside the other children.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (1)
109-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlso emit
removed_incomingwith the rejection tombstone.Line 116 removes the incoming request from in-memory state, but the returned
ContactChangeSetonly carriesrejected. The reject path inpackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspersists this delta as-is, so restore/replay has no persisted signal to delete the old incoming request and can resurrect the rejected request after reload. Add the matchingReceivedContactRequestKeytocs.removed_incomingwhen the map entry is removed.Proposed fix
pub fn record_rejected_contact_request( &mut self, sender_id: &Identifier, account_reference: u32, document_id: Option<Identifier>, ) -> ContactChangeSet { let owner_id = self.id(); - self.incoming_contact_requests.remove(sender_id); + let removed_incoming = self.incoming_contact_requests.remove(sender_id).is_some(); let tombstone = RejectedContactRequest { owner_id, sender_id: *sender_id, account_reference, document_id, }; self.rejected_contact_requests .insert((*sender_id, account_reference), tombstone.clone()); let mut cs = ContactChangeSet::default(); + if removed_incoming { + cs.removed_incoming.insert(ReceivedContactRequestKey { + owner_id, + sender_id: *sender_id, + }); + } cs.rejected .insert((owner_id, *sender_id, account_reference), tombstone); cs }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs` around lines 109 - 130, In record_rejected_contact_request, after removing the entry from incoming_contact_requests you must also record that removal in the returned ContactChangeSet so the delta persists deletion; build a ReceivedContactRequestKey (owner_id, *sender_id, account_reference) and insert it into cs.removed_incoming before returning. This ensures the rejection tombstone is added to cs.rejected and the corresponding incoming entry is emitted via cs.removed_incoming for proper replay; locate record_rejected_contact_request, incoming_contact_requests, ContactChangeSet and Removed/removed_incoming to implement the insertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dashpay/research/07-contactinfo-conventions.md`:
- Around line 25-28: The fenced code block showing the derivation paths lacks a
language tag; update the triple-backtick fence surrounding the lines starting
with "encToUserId key:" and "privateData key:" to include a plain text specifier
(e.g., ```text or ```plaintext) so the block is lint-compliant and renders
correctly.
In `@packages/rs-platform-wallet-ffi/src/contact_info.rs`:
- Around line 51-56: The code currently converts signer_handle to usize and back
to a pointer which can lose pointer provenance; instead capture a typed pointer
to VTableSigner before the worker hop and then unsafely dereference it inside
the closure. Concretely, in the block that calls
PLATFORM_WALLET_STORAGE.with_item and block_on_worker, replace the
signer_addr/usize roundtrip with a typed pointer (e.g., signer_ptr of type
*const VTableSigner derived from signer_handle) and inside the async closure
obtain the signer with unsafe { &*signer_ptr } when creating the &VTableSigner
used by the worker; leave block_on_worker and wallet access unchanged. Ensure
the symbol names referenced are signer_handle, signer_ptr, VTableSigner,
PLATFORM_WALLET_STORAGE.with_item, and block_on_worker.
---
Duplicate comments:
In
`@packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- Around line 109-130: In record_rejected_contact_request, after removing the
entry from incoming_contact_requests you must also record that removal in the
returned ContactChangeSet so the delta persists deletion; build a
ReceivedContactRequestKey (owner_id, *sender_id, account_reference) and insert
it into cs.removed_incoming before returning. This ensures the rejection
tombstone is added to cs.rejected and the corresponding incoming entry is
emitted via cs.removed_incoming for proper replay; locate
record_rejected_contact_request, incoming_contact_requests, ContactChangeSet and
Removed/removed_incoming to implement the insertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b106d317-7b25-48d6-884b-90c7c13f2b57
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
docs/dashpay/SPEC.mddocs/dashpay/research/07-contactinfo-conventions.mdpackages/rs-platform-encryption/src/lib.rspackages/rs-platform-wallet-ffi/src/contact_info.rspackages/rs-platform-wallet-ffi/src/dashpay_profile.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/crypto/contact_info.rspackages/rs-platform-wallet/src/wallet/identity/crypto/dip14.rspackages/rs-platform-wallet/src/wallet/identity/crypto/mod.rspackages/rs-platform-wallet/src/wallet/identity/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_info.rspackages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rspackages/rs-platform-wallet/src/wallet/identity/network/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rspackages/rs-platform-wallet/tests/contact_workflow_tests.rs
✅ Files skipped from review due to trivial changes (2)
- packages/rs-platform-wallet/Cargo.toml
- docs/dashpay/SPEC.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-platform-wallet-ffi/src/lib.rs
- packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
- packages/rs-platform-wallet/src/lib.rs
- packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All 10 prior findings against 440ffca are STILL VALID at HEAD 3f707c2 — verified directly in the worktree. The new contactInfo delta (commits d78bf31 + 3f707c2) does not touch any of the prior hotspots, so the prior blockers carry forward unchanged. Two additional issues surfaced in the new contactInfo code (pagination cap of 100 docs driving both sync and derivation_index allocation; ENCRYPTION key selection without disabled_at check on the owner's own keys at contact_info.rs:313-320). The pagination issue is folded into the budget as it overlaps with derivation-index correctness; the contact_info disabled_at gap is folded into finding #10 since it is the same root-cause pattern. Budget filled with 10 blockers; 2 suggestion-level findings (DashpayPaymentFFI Copy/Clone, contactInfo pagination cap) dropped due to budget but recorded in out_of_scope_findings.
🔴 2 blocking
2 additional finding(s) omitted (not in diff).
8 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:206-222: rejected_contact_requests tombstones are written but never restored on rehydration
Verified at HEAD: `managed_identity_from_entry` hard-codes `rejected_contact_requests: Default::default()` at line 214. The writer (INSERT INTO rejected_contact_requests) and the V001 row exist, but no `load_state` reader for the new table exists, and `apply_changeset` only handles live deltas — restored state is always empty after restart. The recurring DashPay loop's tombstone check at `network/contact_requests.rs:457` therefore never fires after a restart, so a rejected request is resurrected on the first sweep. Add a per-wallet `load_state` reader for `rejected_contact_requests` and route its output into the rehydrated `ManagedIdentity.rejected_contact_requests`.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3024-3034: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
Verified at HEAD lines 3024-3034: PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment (lines 3005-3023) explicitly documents that this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 `save()`, aborting before the wallet row is removed. The user believes their data was wiped; plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression for users handing off or disposing devices.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified the checked-out worktree at 8037a47. All 10 prior blocking findings remain valid and in scope for this PR; no prior finding was resolved. CodeRabbit supplied no inline findings, and one lower-priority in-scope suggestion was omitted because the 10 blocking issues fill the comment budget.
🔴 10 blocking
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/src/wallet/identity/network/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:167-180: Send-side rotation ignores established contacts
The resend path still derives `previous_version` only from `managed.sent_contact_requests`. Once reciprocal sync establishes the contact, that sent request is moved to `EstablishedContact.outgoing_request`, so a later send to the same recipient falls back to version 0. With the same sender, recipient, xpub, and account index, that can reproduce the original masked `accountReference` and collide with the DashPay contract unique index instead of creating a broadcastable rotation. The new load path also restores established contacts without repopulating `sent_contact_requests`, so a resend after relaunch hits the same state immediately.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:441-478: Historical requests replay as rotations
The receive-side rotation guard still compares only the incoming `accountReference` against the currently tracked reference. Because `fetch_received_contact_requests(identity_id, None)` is unfiltered and contactRequest documents are immutable, older v0 documents can be seen after a newer rotated document has advanced local state. Reprocessing the stale document as a rotation can replace the tracked request, tear down the external account, and then flip forward again on a later document or sweep; rotation acceptance needs a monotonic freshness check such as created time or decoded version, not bare reference inequality.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:806-826: Transient account-registration failures become permanent
`build_contact_accounts` still treats every `register_external_contact_account` error as permanent and marks `payment_channel_broken`. That callee performs additional network-backed identity/decryption work, so a transient DAPI or fetch failure can be converted into durable local broken-channel state. Future sweeps then skip the contact until the other party sends a superseding request; only genuinely non-recoverable decrypt, decode, missing-key, or key-type failures should take the permanent-broken path.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:292-308: Disabled encryption keys are still selectable
`select_recipient_key_index` still chooses the first DECRYPTION or ENCRYPTION ECDSA_SECP256K1 key without checking `disabled_at`, while receive-side validation rejects disabled keys. A sender can therefore encrypt the DIP-15 compact xpub to a key the recipient has revoked on-chain, which is the signal that the private half may be compromised. The same missing disabled-key filter remains in the contactInfo root encryption key selector at `contact_info.rs:313-320`, allowing owner-private alias/note/hidden metadata to be published under a revoked encryption key id.
In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:178-212: V001 was edited in place
`contacts.payment_channel_broken` and the `rejected_contact_requests` table are still added directly to V001, and there is no V002 migration. V001 already shipped, so existing v4.0.0-beta.4 databases will either fail refinery checksum validation or treat the old V001 as already applied and never create the new column/table. Runtime writes to the new broken-channel flag or rejected-request tombstone table then fail on upgraded wallets; these schema additions need an append-only migration.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:109-130: Reject does not persist the incoming-row deletion
`record_rejected_contact_request` removes the incoming request from memory, but the returned `ContactChangeSet` only populates `cs.rejected`. The storage writer inserts rejected tombstones separately and only deletes live incoming rows when `cs.removed_incoming` is populated, so the stale `state='received'` contact row remains persisted. On the next load or contact restore, that stale row can reappear as a visible incoming request even though the user rejected it.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3311-3417: Rejected tombstones are not restored
The new `restore_dashpay_contacts` bridge restores sent, incoming, and established contact rows, but `IdentityRestoreEntryFFI` has no rejected-tombstone input and this function never repopulates `ManagedIdentity.rejected_contact_requests`. SwiftData likewise treats rejected snapshots as delete-only visible-row operations. After relaunch, Rust has no suppression entry for the immutable rejected contactRequest, while stale incoming rows can still be restored or fetched again, so a rejected request can reappear despite the user's action.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs:50-79: purpose_mismatch survives hard validation errors
The type documents `purpose_mismatch` as true only when a key-purpose mismatch is the sole invalidity reason, and the sync code uses that flag to choose retry versus permanent broken-channel handling. However, `add_error` does not clear the flag, `add_purpose_error` sets it unconditionally, and `merge` ORs it. A request with both a hard key error and a purpose error can therefore be retried forever instead of being classified as permanently invalid.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3031-3047: Wallet deletion omits DashPay payment children
The SwiftData deletion workaround pre-deletes identity children with non-optional inverse relationships, but it still deletes only DPNS names, DashPay profile, and contact requests. `PersistentDashpayPayment.owner` is also non-optional and is reached through `PersistentIdentity.dashpayPayments`, so a wallet with DashPay payment history can hit the same SwiftData inverse-removal fatal during identity deletion. That aborts the wipe before the wallet row is removed and can leave payment metadata such as memo, counterparty id, amount, and txid on disk.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:230-232: DashPay sync cleanup can clear a newer cancel token
The DashPay sync worker still unconditionally sets `background_cancel` to `None` when its loop exits. If `stop()` cancels token A, `start()` installs token B, and the old thread exits afterward, the old cleanup erases B while the new loop is still running. Later `stop()` or shutdown cannot find the live token, so FFI callback context can be freed while an untracked DashPay thread continues calling through Swift-owned callback pointers. The identity and shielded sync managers already use generation guards to avoid this race.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "a35c2efe8eb96a98082ae78acfdcbf0d5672c1628268cdc2f4642481647a0911"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at af53ae0 (incremental over 8037a47). The latest delta — G6 fallback contract id, G7 pre-send key-pair validator, G8 real ciphertext capture, G9 process-wide contract cache — is small and well-scoped, and introduces no new blockers. G7 mitigates the worst-case security half of prior finding #10 on the send path (a disabled recipient key is now caught before ECDH/broadcast), but the underlying selectors still don't filter disabled_at and the contactInfo rootEncryptionKeyIndex selector is still ungated. The remaining nine prior blockers are unchanged at HEAD. Verified the send-side rotation lookup, the rejected-request changeset, the V001 in-place edits (no V002 exists), and the unconditional DashPay sync cancel-token clear by reading the worktree at HEAD. Recommending REQUEST_CHANGES.
🔴 4 blocking
1 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:178-212: V001 migration edited in place — refinery upgrade from v4.0.0-beta.4 will not pick up new column / table
Verified at HEAD: `contacts.payment_channel_broken` (line 189) and `CREATE TABLE rejected_contact_requests` (lines 203-212) are added directly into V001, and no V002 migration exists anywhere under `packages/rs-platform-wallet-storage/migrations/` (globbed `V*.rs` returns only V001). V001 (without these additions) shipped in v4.0.0-beta.4, and the storage README enforces append-only migrations because refinery checksums each migration in `refinery_schema_history`. Against an existing beta.4 DB this either aborts on divergent checksum or silently skips V001 as already-applied — neither the new column nor the new table will exist. The first runtime write touching `payment_channel_broken` (`mark_contact_channel_broken`) or `INSERT INTO rejected_contact_requests` (G5 tombstone) then fails at SQLite, disabling the PR's payment-channel-broken safety state and the G5 rejection tombstone on every upgraded wallet. Add a V002 migration: `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` plus a separate `CREATE TABLE rejected_contact_requests (...)`.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3311-3417: Rejected tombstones are written but never restored on rehydration
`restore_dashpay_contacts` restores sent, incoming, and established rows from the FFI restore spec, but `IdentityRestoreEntryFFI` exposes no rejected-tombstone field — there is no C struct or count/pointer pair for tombstones in the FFI surface at all. The Swift side persists rejected snapshots as write-only deletes on visible rows, and the SQLite reconstruction helper defaults `rejected_contact_requests` to empty. After relaunch, Rust has no suppression entry for the immutable rejected contactRequest, and the sync-loop tombstone check at contact_requests.rs:500 never fires — a rejected request resurfaces on the first sweep. Because the on-platform document is immutable, wiping the tombstone on restart defeats the user's explicit reject and re-exposes them to suppression-bypass by an unwanted sender. Add a per-wallet load reader for `rejected_contact_requests`, surface the rows via a new `rejected_tombstones` field on `IdentityRestoreEntryFFI`, and rehydrate `ManagedIdentity.rejected_contact_requests` from that input.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3037-3048: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR. The surrounding comment explicitly documents that this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 `save()`, aborting before the wallet row is removed. The user believes their data was wiped; plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression for users handing off or disposing devices.
In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs:301-308: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest selector both miss disabled_at
G7's pre-send validator at `contact_requests.rs:127-138` now gates the contactRequest send path against `validate_contact_request`, which checks `disabled_at`, closing the worst-case 'encrypt DIP-15 compact xpub to a revoked recipient key' confidentiality path. Two residual gaps remain in this PR's scope: (1) `contact_info.rs:301-308` still picks the owner's first ECDSA_SECP256K1 ENCRYPTION key for `rootEncryptionKeyIndex` with no `disabled_at` filter and no equivalent validator gate. The published contactInfo doc references that key id, and if an attacker has the revoked root private material the public id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias/note/hidden metadata — a real confidentiality exposure on the contactInfo path. (2) `contact_requests.rs:335-351` (`select_recipient_key_index`) still picks the first matching key without `disabled_at`; when a recipient has [disabled DECRYPTION + live ENCRYPTION], the new G7 gate now hard-fails the send instead of falling back to the live ENCRYPTION key — a behavioral regression introduced by this delta. Add `&& k.disabled_at().is_none()` to both selectors so contactInfo is safe and the contactRequest send path picks the live key cleanly.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 9441316. Latest delta is documentation (SPEC.md UAT round 2) plus a per-identity reset of optimisticSentIds in DashPayTabView — no Rust, FFI, or schema changes. All 10 prior blocking findings independently re-verified at HEAD against the worktree and remain valid; both Claude and Codex converge on every one of them. Recommending REQUEST_CHANGES.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
9 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3037-3048: Wallet deletion PHASE 1 omits PersistentDashpayPayment children — same fatal pattern as contactRequests
Verified at HEAD lines 3037-3047: PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` but NOT `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is the cascading inverse added in this PR (populated from `DashpayPaymentFFI` rows). The surrounding comment (lines 3013-3036) explicitly documents that this phase exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of the new payments relationship. A wallet with persisted DashPay payment history will hit the SwiftData fatal at PHASE 2 `save()`, aborting before the wallet row is removed. The user believes their data was wiped; plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression for users handing off or disposing devices.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at a919938. Two prior blockers are FIXED in this delta (send-side rotation lookup #1 via prior_sent_account_reference, and received-sweep collapse #2 via newest_received_per_sender, each with new tests). Eight prior blockers verified STILL VALID at HEAD: V001 in-place edit, reject not deleting incoming row via Rust storage, rejected tombstones not restored, transient DAPI hiccup classified permanent, purpose_mismatch flag sticky, Swift wallet-delete omits dashpayPayments (now guaranteed-fatal with H1 populating those rows), DashPay sync cancel-token clear race across FFI, and disabled-key filter missing on two selectors. Codex's new finding about send_payment returning Err(Persistence) after broadcast was dropped because the in-code comment shows it is an intentional design ("surfacing it lets the UI report the partial outcome (sent, but not recorded)"). Codex-security's new FFI-signature-change finding was dropped because the FFI crate and Swift caller ship together in this monorepo and the symbol is updated in lockstep. Recommending REQUEST_CHANGES.
🔴 8 blocking
8 additional finding(s)
blocking: V001 migration edited in place — upgrades from v4.0.0-beta.4 won't apply the new column/table
packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 178)
Verified at HEAD: contacts.payment_channel_broken (line 189) and CREATE TABLE rejected_contact_requests (lines 203-212) are added directly into V001, and no V002 exists. V001 without these additions shipped in v4.0.0-beta.4. Refinery checksums each migration in refinery_schema_history; against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write touching payment_channel_broken (mark_contact_channel_broken) or INSERT INTO rejected_contact_requests (the G5 tombstone) then fails at SQLite, disabling both the PR's payment-channel-broken safety state AND the G5 rejection tombstone on every upgraded wallet. Security impact: a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER plus CREATE TABLE rejected_contact_requests (...) and leave V001 byte-stable.
blocking: record_rejected_contact_request drops incoming map in memory but never persists the row deletion via the SQLite contacts writer
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 133)
Verified at HEAD lines 133-155: line 140 removes the entry from incoming_contact_requests, but the returned ContactChangeSet populates only cs.rejected. Verified at packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193: DELETE FROM contacts fires only when cs.removed_incoming is non-empty; the cs.rejected block (lines 233-255) only writes to rejected_contact_requests. The persisted state='received' row with its stale incoming_request blob therefore stays in SQLite. After a restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister's separate path happens to handle this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject. Emit a matching ReceivedContactRequestKey { owner_id, sender_id } into cs.removed_incoming alongside cs.rejected so both persistence backends converge.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Rejected tombstones are written but never restored on rehydration — FFI restore record has no rejected-tombstone field
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 281)
Verified at HEAD: IdentityRestoreEntryFFI (lines 281-316) now grows payments / payments_count (H1) but still has no rejected-tombstone field. grep rejected packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs returns zero matches. The persist side writes rejected tombstones into the rejected_contact_requests SQLite table, but there is no FFI ingress to surface those rows on load. restore_dashpay_contacts reconstructs sent/incoming/established rows only; ManagedIdentity.rejected_contact_requests is left empty after rehydration. The next DashPay sweep then finds the immutable rejected contactRequest on Platform, the in-memory suppression set is empty (the is_request_rejected check never fires), and the request resurfaces — defeating the user's explicit reject. Because contactRequest documents are immutable on Platform, this is the only suppression mechanism against an abusive sender. Add a #[repr(C)] RejectedContactRequestFFI row type plus rejected_tombstones / rejected_tombstones_count on IdentityRestoreEntryFFI, a Rust loader that writes the rows into managed.rejected_contact_requests, and a Swift producer that surfaces persisted tombstone rows.
blocking: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (line 913)
Verified at HEAD lines 913-933: build_contact_accounts treats any error from register_external_contact_account as permanent and calls mark_contact_channel_broken. The earlier pre-fetch does not short-circuit register_external_contact_account's own internal Identity::fetch, which wraps DAPI failures as PlatformWalletError::InvalidIdentityData. A single transient DAPI hiccup therefore permanently disables the payment channel; subsequent sweeps skip the contact via the payment_channel_broken filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint is a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into register_external_contact_account to remove the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type / disabled-key) by matching on the PlatformWalletError variant.
blocking: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs (line 49)
Verified at HEAD lines 49-79: add_error does not clear purpose_mismatch, add_purpose_error (line 58) sets it unconditionally, and merge (lines 70-79) ORs the flag. The struct's documented contract is purpose_mismatch = true only when key-purpose is the SOLE reason for invalidity, and build_contact_accounts at contact_requests.rs:893 uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up is_valid=false, purpose_mismatch=true, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path. Fix add_error to clear the flag, add_purpose_error to only set it when no prior hard error exists, and merge to track hard-error presence on both sides.
pub fn add_error(&mut self, error: String) {
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = false;
}
pub fn add_purpose_error(&mut self, error: String) {
let already_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = !already_has_hard_error;
}
pub fn add_warning(&mut self, warning: String) {
self.warnings.push(warning);
}
pub fn merge(&mut self, other: ContactRequestValidation) {
let self_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
let other_has_hard_error = !other.errors.is_empty() && !other.purpose_mismatch;
self.errors.extend(other.errors);
self.warnings.extend(other.warnings);
if !other.is_valid {
self.is_valid = false;
}
self.purpose_mismatch =
!self_has_hard_error && !other_has_hard_error && (self.purpose_mismatch || other.purpose_mismatch);
}
blocking: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — H1 cascading inverse now guarantees a SwiftData fatal on wipes with payment history
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3042)
Verified at HEAD lines 3042-3053: PHASE 1 iterates dpnsNames, dashpayProfile, and contactRequests only — not dashpayPayments. PersistentDashpayPayment.owner is non-optional and PersistentIdentity.dashpayPayments is its cascading inverse, populated by this PR's new H1 restore_dashpay_payments path on load AND on every refresh. The surrounding comment at lines 3030-3041 explicitly documents that PHASE 1 exists because SwiftData fatals during save() when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of dashpayPayments. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 save() before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression on device handoff. Add a for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) } loop alongside the existing children.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
}
try backgroundContext.save()
blocking: DashPaySyncManager thread cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across FFI
packages/rs-platform-wallet/src/manager/dashpay_sync.rs (line 214)
Verified at HEAD lines 214-235: the spawned worker writes *guard = None (lines 230-232) on loop exit regardless of which token currently occupies the slot. Race: stop() cancels token A and take()s it; start() (lines 192-198) installs a fresh token B and spawns thread B; thread A then reaches the cleanup arm and writes *guard = None, evicting token B while thread B is still alive. After that a subsequent stop() / quiesce() sees no token to cancel/await and treats DashPay sync as quiesced. In the Swift integration, the persister and DashPay event callbacks close over a Swift-allocated UnsafePointer<Context>; calling dashpay_sync_manager_destroy after the visible token was cleared frees that context while the surviving thread continues invoking callbacks (PersistenceCallbacks function pointers, event-handler dispatch) against the freed pointer — a use-after-free crossing the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/Arc::ptr_eq guards. Capture the spawned token in the closure and only clear the slot when it still holds the same token.
blocking: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs (line 347)
Verified at HEAD. contact_info.rs:347-354 selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for rootEncryptionKeyIndex with no disabled_at filter and no equivalent validator gate. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure that the disabled_at signal exists to prevent. Separately, contact_requests.rs:394-410 (select_recipient_key_index) still picks the first matching DECRYPTION/ENCRYPTION key without disabled_at; with the G7 pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — a behavioral denial-of-contact regression. Add && k.disabled_at().is_none() to both selectors.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:178-212: V001 migration edited in place — upgrades from v4.0.0-beta.4 won't apply the new column/table
Verified at HEAD: `contacts.payment_channel_broken` (line 189) and `CREATE TABLE rejected_contact_requests` (lines 203-212) are added directly into V001, and no V002 exists. V001 without these additions shipped in v4.0.0-beta.4. Refinery checksums each migration in `refinery_schema_history`; against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write touching `payment_channel_broken` (`mark_contact_channel_broken`) or `INSERT INTO rejected_contact_requests` (the G5 tombstone) then fails at SQLite, disabling both the PR's payment-channel-broken safety state AND the G5 rejection tombstone on every upgraded wallet. Security impact: a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` plus `CREATE TABLE rejected_contact_requests (...)` and leave V001 byte-stable.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:133-155: record_rejected_contact_request drops incoming map in memory but never persists the row deletion via the SQLite contacts writer
Verified at HEAD lines 133-155: line 140 removes the entry from `incoming_contact_requests`, but the returned `ContactChangeSet` populates only `cs.rejected`. Verified at `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:182-193`: `DELETE FROM contacts` fires only when `cs.removed_incoming` is non-empty; the `cs.rejected` block (lines 233-255) only writes to `rejected_contact_requests`. The persisted `state='received'` row with its stale `incoming_request` blob therefore stays in SQLite. After a restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister's separate path happens to handle this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id }` into `cs.removed_incoming` alongside `cs.rejected` so both persistence backends converge.
- [BLOCKING] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:281-316: Rejected tombstones are written but never restored on rehydration — FFI restore record has no rejected-tombstone field
Verified at HEAD: `IdentityRestoreEntryFFI` (lines 281-316) now grows `payments` / `payments_count` (H1) but still has no rejected-tombstone field. `grep rejected packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs` returns zero matches. The persist side writes rejected tombstones into the `rejected_contact_requests` SQLite table, but there is no FFI ingress to surface those rows on load. `restore_dashpay_contacts` reconstructs sent/incoming/established rows only; `ManagedIdentity.rejected_contact_requests` is left empty after rehydration. The next DashPay sweep then finds the immutable rejected contactRequest on Platform, the in-memory suppression set is empty (the `is_request_rejected` check never fires), and the request resurfaces — defeating the user's explicit reject. Because contactRequest documents are immutable on Platform, this is the only suppression mechanism against an abusive sender. Add a `#[repr(C)] RejectedContactRequestFFI` row type plus `rejected_tombstones` / `rejected_tombstones_count` on `IdentityRestoreEntryFFI`, a Rust loader that writes the rows into `managed.rejected_contact_requests`, and a Swift producer that surfaces persisted tombstone rows.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:913-933: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
Verified at HEAD lines 913-933: `build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. The earlier pre-fetch does not short-circuit `register_external_contact_account`'s own internal `Identity::fetch`, which wraps DAPI failures as `PlatformWalletError::InvalidIdentityData`. A single transient DAPI hiccup therefore permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint is a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into `register_external_contact_account` to remove the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type / disabled-key) by matching on the `PlatformWalletError` variant.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:49-79: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
Verified at HEAD lines 49-79: `add_error` does not clear `purpose_mismatch`, `add_purpose_error` (line 58) sets it unconditionally, and `merge` (lines 70-79) ORs the flag. The struct's documented contract is `purpose_mismatch = true` only when key-purpose is the SOLE reason for invalidity, and `build_contact_accounts` at `contact_requests.rs:893` uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up `is_valid=false, purpose_mismatch=true`, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path. Fix `add_error` to clear the flag, `add_purpose_error` to only set it when no prior hard error exists, and `merge` to track hard-error presence on both sides.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3042-3053: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — H1 cascading inverse now guarantees a SwiftData fatal on wipes with payment history
Verified at HEAD lines 3042-3053: PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` only — not `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is its cascading inverse, populated by this PR's new H1 `restore_dashpay_payments` path on load AND on every refresh. The surrounding comment at lines 3030-3041 explicitly documents that PHASE 1 exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of `dashpayPayments`. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 `save()` before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression on device handoff. Add a `for payment in Array(identity.dashpayPayments) { backgroundContext.delete(payment) }` loop alongside the existing children.
- [BLOCKING] In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:214-235: DashPaySyncManager thread cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across FFI
Verified at HEAD lines 214-235: the spawned worker writes `*guard = None` (lines 230-232) on loop exit regardless of which token currently occupies the slot. Race: `stop()` cancels token A and `take()`s it; `start()` (lines 192-198) installs a fresh token B and spawns thread B; thread A then reaches the cleanup arm and writes `*guard = None`, evicting token B while thread B is still alive. After that a subsequent `stop()` / `quiesce()` sees no token to cancel/await and treats DashPay sync as quiesced. In the Swift integration, the persister and DashPay event callbacks close over a Swift-allocated `UnsafePointer<Context>`; calling `dashpay_sync_manager_destroy` after the visible token was cleared frees that context while the surviving thread continues invoking callbacks (PersistenceCallbacks function pointers, event-handler dispatch) against the freed pointer — a use-after-free crossing the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/`Arc::ptr_eq` guards. Capture the spawned token in the closure and only clear the slot when it still holds the same token.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:347-354: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
Verified at HEAD. `contact_info.rs:347-354` selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for `rootEncryptionKeyIndex` with no `disabled_at` filter and no equivalent validator gate. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure that the disabled_at signal exists to prevent. Separately, `contact_requests.rs:394-410` (`select_recipient_key_index`) still picks the first matching DECRYPTION/ENCRYPTION key without `disabled_at`; with the G7 pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — a behavioral denial-of-contact regression. Add `&& k.disabled_at().is_none()` to both selectors.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 33c5a09. The a919938..HEAD delta is a Rust-only refactor (parse_compact_xpub returns CompactXpub struct) with no behavior change and no new defects. All 8 prior blocking findings were independently flagged by both Claude and Codex reviewers and verified STILL VALID against the checked-out worktree. Requesting changes.
🔴 8 blocking
8 additional finding(s)
blocking: V001 migration edited in place — upgrades from v4.0.0-beta.4 will not apply the new column/table
packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 178)
Verified at HEAD: contacts.payment_channel_broken (line 189) and CREATE TABLE rejected_contact_requests (lines 203-212) are inlined into V001, and the migrations directory contains only V001__initial.rs — no V002. V001 without these additions shipped in v4.0.0-beta.4. Refinery records per-migration checksums in refinery_schema_history, so against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write touching payment_channel_broken (mark_contact_channel_broken) or INSERT INTO rejected_contact_requests (G5 tombstone) then fails at SQLite, disabling both the payment-channel-broken safety state AND the rejected-request tombstone on every upgraded wallet. Anti-harassment regression: a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER plus CREATE TABLE rejected_contact_requests (...) and leave V001 byte-stable.
blocking: record_rejected_contact_request never persists the visible row deletion through the SQLite contacts writer
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 133)
Verified at HEAD lines 138-155: line 140 removes the entry from incoming_contact_requests, but the returned ContactChangeSet populates only cs.rejected (lines 151-153) — cs.removed_incoming is left empty. The unified SQLite contacts writer only DELETEs incoming rows when cs.removed_incoming is non-empty; the cs.rejected block only writes the tombstone table row. The persisted state='received' row with its stale incoming_request blob therefore stays in SQLite. After restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister happens to handle this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject. Emit a matching ReceivedContactRequestKey { owner_id, sender_id } into cs.removed_incoming alongside cs.rejected so both persistence backends converge.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: IdentityRestoreEntryFFI has no rejected-tombstone field — restore path silently drops a persistence dimension the persist path writes
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 249)
Verified at HEAD: IdentityRestoreEntryFFI (lines 249-316) carries dpns_names, contested_dpns_names, keys, contacts, payments, but no rejected-tombstone field. The Rust→Swift persistence callback writes rejected tombstones (rejected_ptr/rejected_count in persistence.rs), and Swift consumes them by deleting the visible incoming row. The reverse Swift→Rust load ABI has no slot to surface them: restore_dashpay_contacts reconstructs sent/incoming/established rows only, leaving ManagedIdentity.rejected_contact_requests empty after rehydration. On the next DashPay sweep, the immutable rejected contactRequest is fetched, the in-memory suppression set is empty, is_request_rejected never fires, and the explicitly-rejected request resurfaces — defeating the user's reject after every relaunch. Because contactRequest documents are immutable on Platform, this local tombstone is the only suppression vector against an abusive sender. Add a #[repr(C)] RejectedContactRequestFFI row type plus rejected_tombstones: *const RejectedContactRequestFFI + rejected_tombstones_count: usize on IdentityRestoreEntryFFI, a Rust loader feeding managed.rejected_contact_requests, and a Swift producer reading persisted SwiftData rows.
blocking: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (line 913)
Verified at HEAD lines 913-933: build_contact_accounts treats any error from register_external_contact_account as permanent and calls mark_contact_channel_broken. The callee performs an additional network-backed Identity::fetch plus decryption work, wrapping DAPI failures as PlatformWalletError::InvalidIdentityData. Earlier pre-fetches do not short-circuit this second internal fetch. A single transient DAPI hiccup permanently disables the payment channel; subsequent sweeps skip the contact via the payment_channel_broken filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint becomes a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into register_external_contact_account to eliminate the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type/disabled-key) by matching on the PlatformWalletError variant.
blocking: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs (line 49)
Verified at HEAD lines 49-79: add_error (lines 50-53) does not clear purpose_mismatch, add_purpose_error (lines 58-62) sets it unconditionally to true, and merge (lines 70-79) ORs the flag. The struct's documented contract is purpose_mismatch = true only when key-purpose is the SOLE reason for invalidity, and build_contact_accounts uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up is_valid=false, purpose_mismatch=true, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path where a crafted recipient identity keeps a victim's sync sweep doing repeated DAPI work indefinitely. Fix add_error to clear the flag, add_purpose_error to only set it when no prior hard error exists, and merge to track hard-error presence on both sides.
pub fn add_error(&mut self, error: String) {
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = false;
}
pub fn add_purpose_error(&mut self, error: String) {
let already_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = !already_has_hard_error;
}
pub fn add_warning(&mut self, warning: String) {
self.warnings.push(warning);
}
pub fn merge(&mut self, other: ContactRequestValidation) {
let self_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
let other_has_hard_error = !other.errors.is_empty() && !other.purpose_mismatch;
self.errors.extend(other.errors);
self.warnings.extend(other.warnings);
if !other.is_valid {
self.is_valid = false;
}
self.purpose_mismatch =
!self_has_hard_error && !other_has_hard_error && (self.purpose_mismatch || other.purpose_mismatch);
}
blocking: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — SwiftData fatal guaranteed on wipes with payment history
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3042)
Verified at HEAD lines 3042-3053: PHASE 1 iterates dpnsNames, dashpayProfile, and contactRequests only — not dashpayPayments. PersistentDashpayPayment.owner is non-optional and PersistentIdentity.dashpayPayments is its cascading inverse, populated by this PR's H1 restore_dashpay_payments path on load and on every refresh. The surrounding comment at lines 3025-3041 explicitly documents that PHASE 1 exists because SwiftData fatals during save() when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of dashpayPayments. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 save() before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression on device handoff/disposal. Add a dashpayPayments loop alongside the existing children.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
}
try backgroundContext.save()
blocking: DashPaySyncManager cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across the C ABI
packages/rs-platform-wallet/src/manager/dashpay_sync.rs (line 214)
Verified at HEAD lines 192-235: start() installs a fresh token at line 198 and spawns a dedicated OS thread that polls inside handle.block_on. On loop exit the cleanup arm at lines 230-232 writes *guard = None UNCONDITIONALLY — it never checks whether the token currently in the slot is still the one this thread was spawned with. Race: stop() (lines 246-255) cancels token A and take()s it; start() observes the slot empty, installs fresh token B, spawns thread B. Thread A — still finishing its block_on — then reaches the cleanup arm and writes *guard = None, evicting token B while thread B is alive. After that any subsequent stop()/quiesce() sees no token to cancel/await and treats DashPay sync as quiesced. Cross-FFI consequence: the Swift persister and DashPay event callbacks close over a Swift-allocated UnsafePointer<Context>. Once the visible token is cleared while a stale thread is alive, the host treats dashpay_sync_manager_destroy as safe and runs Context.deallocate(), while the surviving thread continues invoking PersistenceCallbacks / event-handler function pointers against the freed Swift pointer — concrete use-after-free across the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/Arc::ptr_eq guards; mirror that here by capturing the spawned token in the closure and only clearing the slot when it still holds the same token.
blocking: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs (line 347)
Verified at HEAD lines 347-354: selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for rootEncryptionKeyIndex filtering only on purpose+key_type, with no disabled_at check. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure the disabled_at signal exists to prevent. Separately, select_recipient_key_index in contact_requests.rs:394-410 still picks the first matching DECRYPTION/ENCRYPTION key without disabled_at; with the G7 pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — a behavioral denial-of-contact regression. Add && k.disabled_at().is_none() to both selectors.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:178-212: V001 migration edited in place — upgrades from v4.0.0-beta.4 will not apply the new column/table
Verified at HEAD: `contacts.payment_channel_broken` (line 189) and `CREATE TABLE rejected_contact_requests` (lines 203-212) are inlined into V001, and the migrations directory contains only `V001__initial.rs` — no V002. V001 without these additions shipped in v4.0.0-beta.4. Refinery records per-migration checksums in `refinery_schema_history`, so against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write touching `payment_channel_broken` (mark_contact_channel_broken) or `INSERT INTO rejected_contact_requests` (G5 tombstone) then fails at SQLite, disabling both the payment-channel-broken safety state AND the rejected-request tombstone on every upgraded wallet. Anti-harassment regression: a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` plus `CREATE TABLE rejected_contact_requests (...)` and leave V001 byte-stable.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:133-155: record_rejected_contact_request never persists the visible row deletion through the SQLite contacts writer
Verified at HEAD lines 138-155: line 140 removes the entry from `incoming_contact_requests`, but the returned `ContactChangeSet` populates only `cs.rejected` (lines 151-153) — `cs.removed_incoming` is left empty. The unified SQLite contacts writer only DELETEs incoming rows when `cs.removed_incoming` is non-empty; the `cs.rejected` block only writes the tombstone table row. The persisted `state='received'` row with its stale `incoming_request` blob therefore stays in SQLite. After restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister happens to handle this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id }` into `cs.removed_incoming` alongside `cs.rejected` so both persistence backends converge.
- [BLOCKING] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:249-316: IdentityRestoreEntryFFI has no rejected-tombstone field — restore path silently drops a persistence dimension the persist path writes
Verified at HEAD: `IdentityRestoreEntryFFI` (lines 249-316) carries `dpns_names`, `contested_dpns_names`, `keys`, `contacts`, `payments`, but no rejected-tombstone field. The Rust→Swift persistence callback writes rejected tombstones (rejected_ptr/rejected_count in persistence.rs), and Swift consumes them by deleting the visible incoming row. The reverse Swift→Rust load ABI has no slot to surface them: `restore_dashpay_contacts` reconstructs sent/incoming/established rows only, leaving `ManagedIdentity.rejected_contact_requests` empty after rehydration. On the next DashPay sweep, the immutable rejected contactRequest is fetched, the in-memory suppression set is empty, `is_request_rejected` never fires, and the explicitly-rejected request resurfaces — defeating the user's reject after every relaunch. Because contactRequest documents are immutable on Platform, this local tombstone is the only suppression vector against an abusive sender. Add a `#[repr(C)] RejectedContactRequestFFI` row type plus `rejected_tombstones: *const RejectedContactRequestFFI` + `rejected_tombstones_count: usize` on `IdentityRestoreEntryFFI`, a Rust loader feeding `managed.rejected_contact_requests`, and a Swift producer reading persisted SwiftData rows.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:913-933: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
Verified at HEAD lines 913-933: `build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. The callee performs an additional network-backed `Identity::fetch` plus decryption work, wrapping DAPI failures as `PlatformWalletError::InvalidIdentityData`. Earlier pre-fetches do not short-circuit this second internal fetch. A single transient DAPI hiccup permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint becomes a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into `register_external_contact_account` to eliminate the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type/disabled-key) by matching on the `PlatformWalletError` variant.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:49-79: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
Verified at HEAD lines 49-79: `add_error` (lines 50-53) does not clear `purpose_mismatch`, `add_purpose_error` (lines 58-62) sets it unconditionally to true, and `merge` (lines 70-79) ORs the flag. The struct's documented contract is `purpose_mismatch = true` only when key-purpose is the SOLE reason for invalidity, and `build_contact_accounts` uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up `is_valid=false, purpose_mismatch=true`, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path where a crafted recipient identity keeps a victim's sync sweep doing repeated DAPI work indefinitely. Fix `add_error` to clear the flag, `add_purpose_error` to only set it when no prior hard error exists, and `merge` to track hard-error presence on both sides.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3042-3053: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — SwiftData fatal guaranteed on wipes with payment history
Verified at HEAD lines 3042-3053: PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` only — not `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is its cascading inverse, populated by this PR's H1 `restore_dashpay_payments` path on load and on every refresh. The surrounding comment at lines 3025-3041 explicitly documents that PHASE 1 exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of `dashpayPayments`. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 `save()` before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — a confidentiality regression on device handoff/disposal. Add a `dashpayPayments` loop alongside the existing children.
- [BLOCKING] In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:214-235: DashPaySyncManager cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across the C ABI
Verified at HEAD lines 192-235: `start()` installs a fresh token at line 198 and spawns a dedicated OS thread that polls inside `handle.block_on`. On loop exit the cleanup arm at lines 230-232 writes `*guard = None` UNCONDITIONALLY — it never checks whether the token currently in the slot is still the one this thread was spawned with. Race: `stop()` (lines 246-255) cancels token A and `take()`s it; `start()` observes the slot empty, installs fresh token B, spawns thread B. Thread A — still finishing its block_on — then reaches the cleanup arm and writes `*guard = None`, evicting token B while thread B is alive. After that any subsequent `stop()`/`quiesce()` sees no token to cancel/await and treats DashPay sync as quiesced. Cross-FFI consequence: the Swift persister and DashPay event callbacks close over a Swift-allocated `UnsafePointer<Context>`. Once the visible token is cleared while a stale thread is alive, the host treats `dashpay_sync_manager_destroy` as safe and runs `Context.deallocate()`, while the surviving thread continues invoking `PersistenceCallbacks` / event-handler function pointers against the freed Swift pointer — concrete use-after-free across the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/`Arc::ptr_eq` guards; mirror that here by capturing the spawned token in the closure and only clearing the slot when it still holds the same token.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:347-354: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
Verified at HEAD lines 347-354: selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for `rootEncryptionKeyIndex` filtering only on purpose+key_type, with no `disabled_at` check. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure the `disabled_at` signal exists to prevent. Separately, `select_recipient_key_index` in `contact_requests.rs:394-410` still picks the first matching DECRYPTION/ENCRYPTION key without `disabled_at`; with the G7 pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — a behavioral denial-of-contact regression. Add `&& k.disabled_at().is_none()` to both selectors.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
…te-wallet FFI boundary create_wallet_from_seed_impl copied the 64-byte master secret into a plain [u8; 64] local and passed it BY VALUE into the manager method, so both the FFI-boundary copy and the method's owned stack copy lingered un-zeroized after the call. Now: - FFI local is Zeroizing<[u8; 64]> (wiped on drop), matching the sibling attach_wallet_seed_from_mnemonic. - create_wallet_from_seed_bytes takes seed_bytes: &[u8; 64] (by ref), so the method no longer owns a copy; the only remaining copy is the one consumed into key-wallet's Seed. Defense-in-depth, not a fix for a leak — the seed-in-Rust model is required (key-wallet derives all DashPay keys in Rust) and the mnemonic already crosses the ABI at create_wallet_from_mnemonic. The true "secret never crosses the ABI" path is the deferred G4 watch-only hook. 230/230 lib tests green.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All 8 prior blockers re-verified STILL VALID at 03ba3ee; the latest delta (zeroize seed at FFI boundary + ContactXpubData embedding CompactXpub) introduces one new compile-breaking defect — create_wallet_from_seed_bytes now takes &[u8; 64] but tests/spv_sync.rs and examples/basic_usage.rs (plus both shielded_sync examples) still pass by value. One suggestion-grade observation: the same commit's *seed_bytes deref at wallet_lifecycle.rs:110 materializes a non-zeroized [u8;64] stack copy that contradicts the new doc-comment invariant. Recommending REQUEST_CHANGES.
🔴 9 blocking | 🟡 1 suggestion(s)
10 additional finding(s)
blocking: V001 migration edited in place — upgrades from v4.0.0-beta.4 won't apply the new column/table
packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 178)
contacts.payment_channel_broken (line 189) and CREATE TABLE rejected_contact_requests (lines 203-212) are inlined into V001, and the migrations directory contains only V001__initial.rs — no V002. V001 without these additions already shipped in v4.0.0-beta.4. Refinery records per-migration checksums in refinery_schema_history, so against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write to payment_channel_broken (mark_contact_channel_broken) or INSERT INTO rejected_contact_requests (G5 tombstone) then fails at SQLite, disabling both the payment-channel-broken safety state AND the G5 rejection tombstone on every upgraded wallet — a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER plus CREATE TABLE rejected_contact_requests (...) and leave V001 byte-stable.
blocking: record_rejected_contact_request drops the incoming map entry but never persists the row deletion via the SQLite contacts writer
packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs (line 133)
Line 140 removes the entry from incoming_contact_requests, but the returned ContactChangeSet populates only cs.rejected (lines 151-153) — cs.removed_incoming is left empty. The unified SQLite contacts writer DELETEs incoming rows only when cs.removed_incoming is non-empty; the cs.rejected block only writes the tombstone table row. The persisted state='received' row with its stale incoming_request blob therefore stays in SQLite. After a restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister masks this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject — and the on-platform contactRequest document is immutable, so the local tombstone + delete is the only suppression mechanism against an abusive sender. Emit a matching ReceivedContactRequestKey { owner_id, sender_id } into cs.removed_incoming alongside cs.rejected.
let owner_id = self.id();
self.incoming_contact_requests.remove(sender_id);
let tombstone = RejectedContactRequest {
owner_id,
sender_id: *sender_id,
account_reference,
document_id,
};
self.rejected_contact_requests
.insert((*sender_id, account_reference), tombstone.clone());
let mut cs = ContactChangeSet::default();
cs.removed_incoming.insert(ReceivedContactRequestKey {
owner_id,
sender_id: *sender_id,
});
cs.rejected
.insert((owner_id, *sender_id, account_reference), tombstone);
cs
blocking: Rejected tombstones are written but never restored on rehydration — FFI restore record has no rejected-tombstone field
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs (line 281)
IdentityRestoreEntryFFI carries dpns_names, contested_dpns_names, keys, contacts, payments, but no rejected-tombstone field. The persist side writes rejected tombstones into the rejected_contact_requests SQLite table (and Swift consumes them by deleting the visible incoming row), but the reverse Swift→Rust load ABI has no slot to surface persisted tombstones — restore_dashpay_contacts reconstructs sent/incoming/established only, leaving ManagedIdentity.rejected_contact_requests empty after rehydration. The next DashPay sweep then fetches the immutable rejected contactRequest, the in-memory suppression set is empty (is_request_rejected never fires), and the request resurfaces — defeating the user's explicit reject after every relaunch. Because the on-platform document is immutable, this local tombstone is the only suppression vector against an abusive sender. Add a #[repr(C)] RejectedContactRequestFFI row type plus rejected_tombstones / rejected_tombstones_count on IdentityRestoreEntryFFI, a Rust loader that writes the rows into managed.rejected_contact_requests, and a Swift producer that surfaces persisted tombstone rows.
blocking: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs (line 913)
build_contact_accounts treats any error from register_external_contact_account as permanent and calls mark_contact_channel_broken. The callee performs an additional network-backed Identity::fetch plus decryption work, wrapping DAPI failures as PlatformWalletError::InvalidIdentityData; earlier pre-fetches do not short-circuit this second internal fetch. A single transient DAPI hiccup permanently disables the payment channel; subsequent sweeps skip the contact via the payment_channel_broken filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint becomes a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into register_external_contact_account to eliminate the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type/disabled-key) by matching on the PlatformWalletError variant.
blocking: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs (line 49)
add_error (lines 50-53) does not clear purpose_mismatch; add_purpose_error (lines 58-62) sets it unconditionally; merge (lines 70-79) ORs the flag. The struct's documented contract is purpose_mismatch = true only when key-purpose is the SOLE reason for invalidity, and build_contact_accounts uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up is_valid=false, purpose_mismatch=true, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path where a crafted recipient identity keeps a victim's sync sweep doing repeated DAPI work indefinitely. Fix add_error to clear the flag, add_purpose_error to only set it when no prior hard error exists, and merge to track hard-error presence on both sides.
pub fn add_error(&mut self, error: String) {
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = false;
}
pub fn add_purpose_error(&mut self, error: String) {
let already_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
self.errors.push(error);
self.is_valid = false;
self.purpose_mismatch = !already_has_hard_error;
}
pub fn add_warning(&mut self, warning: String) {
self.warnings.push(warning);
}
pub fn merge(&mut self, other: ContactRequestValidation) {
let self_has_hard_error = !self.errors.is_empty() && !self.purpose_mismatch;
let other_has_hard_error = !other.errors.is_empty() && !other.purpose_mismatch;
self.errors.extend(other.errors);
self.warnings.extend(other.warnings);
if !other.is_valid {
self.is_valid = false;
}
self.purpose_mismatch =
!self_has_hard_error && !other_has_hard_error && (self.purpose_mismatch || other.purpose_mismatch);
}
blocking: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — SwiftData fatal guaranteed on wipes with payment history
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 3042)
PHASE 1 iterates dpnsNames, dashpayProfile, and contactRequests only — not dashpayPayments. PersistentDashpayPayment.owner is non-optional and PersistentIdentity.dashpayPayments is its cascading inverse, populated by this PR's restore_dashpay_payments path on load and on every refresh. The surrounding comment at lines 3025-3041 explicitly documents that PHASE 1 exists because SwiftData fatals during save() when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of dashpayPayments. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 save() before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — confidentiality regression on device handoff/disposal. Add a dashpayPayments loop alongside the existing children.
for identity in identitiesToDelete {
for name in Array(identity.dpnsNames) {
backgroundContext.delete(name)
}
if let profile = identity.dashpayProfile {
backgroundContext.delete(profile)
}
for cr in Array(identity.contactRequests) {
backgroundContext.delete(cr)
}
for payment in Array(identity.dashpayPayments) {
backgroundContext.delete(payment)
}
}
try backgroundContext.save()
blocking: DashPaySyncManager cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across the C ABI
packages/rs-platform-wallet/src/manager/dashpay_sync.rs (line 214)
start() installs a fresh token at line 198 and spawns a dedicated OS thread that polls inside handle.block_on. On loop exit the cleanup arm at lines 230-232 writes *guard = None UNCONDITIONALLY — it never checks whether the token currently in the slot is still the one this thread was spawned with. Race: stop() (lines 246-255) cancels token A and take()s it; start() observes the slot empty, installs fresh token B, spawns thread B; thread A — still finishing its block_on — then reaches the cleanup arm and writes *guard = None, evicting token B's handle while thread B is alive. Any subsequent stop()/quiesce() sees no token to cancel/await and treats DashPay sync as quiesced. Cross-FFI consequence: the Swift persister and DashPay event callbacks close over a Swift-allocated UnsafePointer<Context>. Once the visible token is cleared while a stale thread is alive, the host treats dashpay_sync_manager_destroy as safe and runs Context.deallocate(), while the surviving thread continues invoking PersistenceCallbacks / event-handler function pointers against the freed Swift pointer — concrete use-after-free across the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/Arc::ptr_eq guards; mirror that here by capturing the spawned token in the closure and only clearing the slot when it still holds the same token.
blocking: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs (line 347)
contact_info.rs:347-354 selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for rootEncryptionKeyIndex filtering only on purpose+key_type — no disabled_at check. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure the disabled_at signal exists to prevent. Separately, select_recipient_key_index in contact_requests.rs:394-410 still picks the first matching DECRYPTION/ENCRYPTION key without disabled_at; with the pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — behavioral denial-of-contact regression. Add && k.disabled_at().is_none() to both selectors.
blocking: create_wallet_from_seed_bytes signature change breaks tests and examples — &[u8; 64] no longer matches by-value call sites
packages/rs-platform-wallet/tests/spv_sync.rs (line 183)
The latest delta changed wallet_lifecycle.rs:100-106 to take seed_bytes: &[u8; 64], but the in-repo call sites in tests/spv_sync.rs:184 (seed_bytes from mnemonic.to_seed("") passed by value), examples/basic_usage.rs:59-61 (also by value), examples/shielded_sync.rs:241, and examples/shielded_sync_paloma.rs:235 were not updated. Rust does not auto-borrow ordinary function arguments, so cargo test/cargo check on these targets fails with a type mismatch. The production call sites in payments.rs:460, payments.rs:795, dashpay_sync.rs:425, and wallet_lifecycle.rs:670/683 need to be reviewed too. Pass &seed_bytes (or &transparent_seed) at every stale call site.
suggestion: Zeroize-seed commit's `*seed_bytes` deref re-creates a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs (line 100)
The commit's stated intent (doc comment at lines 103-105) is that create_wallet_from_seed_bytes 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's Seed below'. But line 110 calls Wallet::from_seed_bytes(*seed_bytes, network, accounts) — the *seed_bytes deref on a [u8; 64] (which is Copy) materializes a fresh byte-for-byte copy in this caller's stack frame to pass by value into from_seed_bytes. That intermediate copy is not wrapped in Zeroizing and not explicitly cleared before this function returns; whether it survives the frame depends on stack reuse. The FFI-side Zeroizing wrapper still helps (the cross-FFI copy is zeroized), but the inner-Rust copy at this *deref site is the same defense-in-depth gap the commit was trying to close. Either change Wallet::from_seed_bytes to take &[u8; 64] (zeroizing internally), or wrap the local in Zeroizing here: let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, ...).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:178-212: V001 migration edited in place — upgrades from v4.0.0-beta.4 won't apply the new column/table
`contacts.payment_channel_broken` (line 189) and `CREATE TABLE rejected_contact_requests` (lines 203-212) are inlined into V001, and the migrations directory contains only `V001__initial.rs` — no V002. V001 without these additions already shipped in v4.0.0-beta.4. Refinery records per-migration checksums in `refinery_schema_history`, so against an existing beta.4 DB this either aborts on divergent checksum or skips V001 as already-applied and never creates the new column/table. The first runtime write to `payment_channel_broken` (`mark_contact_channel_broken`) or `INSERT INTO rejected_contact_requests` (G5 tombstone) then fails at SQLite, disabling both the payment-channel-broken safety state AND the G5 rejection tombstone on every upgraded wallet — a once-rejected, abusive sender's still-immutable on-chain contactRequest is no longer suppressed on relaunch. Add a V002 migration with `ALTER TABLE contacts ADD COLUMN payment_channel_broken INTEGER` plus `CREATE TABLE rejected_contact_requests (...)` and leave V001 byte-stable.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:133-155: record_rejected_contact_request drops the incoming map entry but never persists the row deletion via the SQLite contacts writer
Line 140 removes the entry from `incoming_contact_requests`, but the returned `ContactChangeSet` populates only `cs.rejected` (lines 151-153) — `cs.removed_incoming` is left empty. The unified SQLite contacts writer DELETEs incoming rows only when `cs.removed_incoming` is non-empty; the `cs.rejected` block only writes the tombstone table row. The persisted `state='received'` row with its stale `incoming_request` blob therefore stays in SQLite. After a restart the load path reinserts it as a live incoming entry; the sync sweep's tombstone check only suppresses on-chain re-ingest, not local rehydration. The Swift persister masks this in SwiftData, but the Rust SQLite path used in non-Swift hosts and in tests leaves the row behind, silently undoing the user's reject — and the on-platform contactRequest document is immutable, so the local tombstone + delete is the only suppression mechanism against an abusive sender. Emit a matching `ReceivedContactRequestKey { owner_id, sender_id }` into `cs.removed_incoming` alongside `cs.rejected`.
- [BLOCKING] In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:281-316: Rejected tombstones are written but never restored on rehydration — FFI restore record has no rejected-tombstone field
`IdentityRestoreEntryFFI` carries `dpns_names`, `contested_dpns_names`, `keys`, `contacts`, `payments`, but no rejected-tombstone field. The persist side writes rejected tombstones into the `rejected_contact_requests` SQLite table (and Swift consumes them by deleting the visible incoming row), but the reverse Swift→Rust load ABI has no slot to surface persisted tombstones — `restore_dashpay_contacts` reconstructs sent/incoming/established only, leaving `ManagedIdentity.rejected_contact_requests` empty after rehydration. The next DashPay sweep then fetches the immutable rejected contactRequest, the in-memory suppression set is empty (`is_request_rejected` never fires), and the request resurfaces — defeating the user's explicit reject after every relaunch. Because the on-platform document is immutable, this local tombstone is the only suppression vector against an abusive sender. Add a `#[repr(C)] RejectedContactRequestFFI` row type plus `rejected_tombstones` / `rejected_tombstones_count` on `IdentityRestoreEntryFFI`, a Rust loader that writes the rows into `managed.rejected_contact_requests`, and a Swift producer that surfaces persisted tombstone rows.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs`:913-933: Transient DAPI failures inside register_external_contact_account are classified as permanent payment-channel-broken
`build_contact_accounts` treats any error from `register_external_contact_account` as permanent and calls `mark_contact_channel_broken`. The callee performs an additional network-backed `Identity::fetch` plus decryption work, wrapping DAPI failures as `PlatformWalletError::InvalidIdentityData`; earlier pre-fetches do not short-circuit this second internal fetch. A single transient DAPI hiccup permanently disables the payment channel; subsequent sweeps skip the contact via the `payment_channel_broken` filter, and recovery requires the counterparty to send a superseding contactRequest. A flaky or malicious DAPI endpoint becomes a persistent denial-of-payment primitive against a chosen contact, also reachable by routine network flakiness. Either thread the pre-fetched contact identity into `register_external_contact_account` to eliminate the second fetch, or scope permanent-broken classification to genuinely non-recoverable failures (decrypt/decode, missing-key, key-type/disabled-key) by matching on the `PlatformWalletError` variant.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/crypto/validation.rs`:49-79: purpose_mismatch flag stays sticky when a non-purpose hard error is also present
`add_error` (lines 50-53) does not clear `purpose_mismatch`; `add_purpose_error` (lines 58-62) sets it unconditionally; `merge` (lines 70-79) ORs the flag. The struct's documented contract is `purpose_mismatch = true` only when key-purpose is the SOLE reason for invalidity, and `build_contact_accounts` uses that flag as the load-bearing predicate for retry-forever vs mark-broken. A request whose key has both a wrong key-TYPE (hard, permanent) and a wrong purpose ends up `is_valid=false, purpose_mismatch=true`, so the caller retries forever instead of marking broken — the opposite of intent, and an attacker-controllable resource-amplification path where a crafted recipient identity keeps a victim's sync sweep doing repeated DAPI work indefinitely. Fix `add_error` to clear the flag, `add_purpose_error` to only set it when no prior hard error exists, and `merge` to track hard-error presence on both sides.
- [BLOCKING] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:3042-3053: Wallet-delete PHASE 1 omits PersistentDashpayPayment children — SwiftData fatal guaranteed on wipes with payment history
PHASE 1 iterates `dpnsNames`, `dashpayProfile`, and `contactRequests` only — not `dashpayPayments`. `PersistentDashpayPayment.owner` is non-optional and `PersistentIdentity.dashpayPayments` is its cascading inverse, populated by this PR's `restore_dashpay_payments` path on load and on every refresh. The surrounding comment at lines 3025-3041 explicitly documents that PHASE 1 exists because SwiftData fatals during `save()` when it must null out a non-optional inverse on a child processed in the same batch — exactly the shape of `dashpayPayments`. Before this PR the failure mode was speculative; now it is guaranteed for any wallet ever exercised through DashPay payments. The fatal aborts PHASE 2 `save()` before the wallet row is removed; the user believes their data was wiped while plaintext memo + counterparty id + amount + txid rows remain on disk — confidentiality regression on device handoff/disposal. Add a `dashpayPayments` loop alongside the existing children.
- [BLOCKING] In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:214-235: DashPaySyncManager cleanup unconditionally clears the cancel-token slot — stop/start race enables use-after-free across the C ABI
`start()` installs a fresh token at line 198 and spawns a dedicated OS thread that polls inside `handle.block_on`. On loop exit the cleanup arm at lines 230-232 writes `*guard = None` UNCONDITIONALLY — it never checks whether the token currently in the slot is still the one this thread was spawned with. Race: `stop()` (lines 246-255) cancels token A and `take()`s it; `start()` observes the slot empty, installs fresh token B, spawns thread B; thread A — still finishing its block_on — then reaches the cleanup arm and writes `*guard = None`, evicting token B's handle while thread B is alive. Any subsequent `stop()`/`quiesce()` sees no token to cancel/await and treats DashPay sync as quiesced. Cross-FFI consequence: the Swift persister and DashPay event callbacks close over a Swift-allocated `UnsafePointer<Context>`. Once the visible token is cleared while a stale thread is alive, the host treats `dashpay_sync_manager_destroy` as safe and runs `Context.deallocate()`, while the surviving thread continues invoking `PersistenceCallbacks` / event-handler function pointers against the freed Swift pointer — concrete use-after-free across the C ABI, reachable by normal Swift gestures (DashPay tab toggle, login/logout, identity-picker switch). The sibling identity and shielded sync managers already use generation/`Arc::ptr_eq` guards; mirror that here by capturing the spawned token in the closure and only clearing the slot when it still holds the same token.
- [BLOCKING] In `packages/rs-platform-wallet/src/wallet/identity/network/contact_info.rs`:347-354: Disabled encryption keys are still selectable — contactInfo rootEncryptionKeyIndex and contactRequest recipient selector both miss disabled_at
`contact_info.rs:347-354` selects the owner's first ECDSA_SECP256K1 ENCRYPTION key for `rootEncryptionKeyIndex` filtering only on purpose+key_type — no `disabled_at` check. The published contactInfo doc references that key id, so if an attacker holds the revoked root private material the public key id + derivation index lets them derive the same contactInfo AES keys and decrypt the owner's private alias / note / hidden metadata — a confidentiality exposure the `disabled_at` signal exists to prevent. Separately, `select_recipient_key_index` in `contact_requests.rs:394-410` still picks the first matching DECRYPTION/ENCRYPTION key without `disabled_at`; with the pre-send validator hard-failing the send when the chosen key is disabled, a recipient with [disabled DECRYPTION + live ENCRYPTION] gets sends blocked rather than falling through to the live ENCRYPTION key — behavioral denial-of-contact regression. Add `&& k.disabled_at().is_none()` to both selectors.
- [BLOCKING] In `packages/rs-platform-wallet/tests/spv_sync.rs`:183-191: create_wallet_from_seed_bytes signature change breaks tests and examples — &[u8; 64] no longer matches by-value call sites
The latest delta changed `wallet_lifecycle.rs:100-106` to take `seed_bytes: &[u8; 64]`, but the in-repo call sites in `tests/spv_sync.rs:184` (`seed_bytes` from `mnemonic.to_seed("")` passed by value), `examples/basic_usage.rs:59-61` (also by value), `examples/shielded_sync.rs:241`, and `examples/shielded_sync_paloma.rs:235` were not updated. Rust does not auto-borrow ordinary function arguments, so `cargo test`/`cargo check` on these targets fails with a type mismatch. The production call sites in `payments.rs:460`, `payments.rs:795`, `dashpay_sync.rs:425`, and `wallet_lifecycle.rs:670/683` need to be reviewed too. Pass `&seed_bytes` (or `&transparent_seed`) at every stale call site.
- [SUGGESTION] In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:100-117: Zeroize-seed commit's `*seed_bytes` deref re-creates a non-zeroized [u8;64] stack copy, contradicting the doc-comment invariant
The commit's stated intent (doc comment at lines 103-105) is that `create_wallet_from_seed_bytes` 'never owns a non-zeroized stack copy of the master secret — the only copy is the one consumed into key-wallet's `Seed` below'. But line 110 calls `Wallet::from_seed_bytes(*seed_bytes, network, accounts)` — the `*seed_bytes` deref on a `[u8; 64]` (which is `Copy`) materializes a fresh byte-for-byte copy in this caller's stack frame to pass by value into `from_seed_bytes`. That intermediate copy is not wrapped in `Zeroizing` and not explicitly cleared before this function returns; whether it survives the frame depends on stack reuse. The FFI-side `Zeroizing` wrapper still helps (the cross-FFI copy is zeroized), but the inner-Rust copy at this `*deref` site is the same defense-in-depth gap the commit was trying to close. Either change `Wallet::from_seed_bytes` to take `&[u8; 64]` (zeroizing internally), or wrap the local in `Zeroizing` here: `let seed = zeroize::Zeroizing::new(*seed_bytes); Wallet::from_seed_bytes(*seed, ...)`.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
Resolves the rs-sdk-ffi/src/sdk.rs conflict in favor of v3.1-dev's BigStackRuntime::build_shared() — it sets the same 8 MiB worker stack (WORKER_STACK_SIZE) that the branch added manually to fix the on-device GroveDB-proof SIGBUS, plus a 16 MiB blocking stack, and matches the renamed Arc<BigStackRuntime> runtime type. Same fix, canonical form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… stop/start race The background loop's exit cleanup ran an unconditional `*guard = None`. A cancel-only `stop()` (takes + cancels loop A's token while A keeps draining its in-flight pass) followed by a quick `start()` (installs loop B's token) left loop A's later cleanup nulling loop B's *live* token — after which `is_running()` lies and a shutdown `stop()`/`quiesce()` silently no-ops while loop B keeps fanning out `persister.store(...)` through a freed FFI persister context (use-after-free). Fix: bump a monotonic `loop_generation` under the `background_cancel` lock on every install, and clear the stored token on exit only when the generation still matches (`clear_cancel_if_current`) — the newest loop is the only one that may clear, and only while it is still the newest. Extracted `install_cancel`/`clear_cancel_if_current` so the race is deterministically testable (the real loop runs on an OS thread under `Handle::block_on`, whose exit can't be timed reliably). Test would have caught this in CI: regression `stale_loop_cleanup_does_not_clobber_newer_loop_token` is ✖ with the unconditional clear, ✔ with the generation guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…transient failure
build_contact_accounts marked a contact's payment channel permanently
broken (G1c) on *any* error from register_external_contact_account — but
that method did an Identity::fetch().await internally, so a transient
DAPI blip was indistinguishable from a malformed request and killed
payments to the contact forever (recovery needs a rotation that may
never come).
Two changes:
- register_external_contact_account now takes the already-fetched
&Identity (both callers fetch it for the pre-ECDH key validation), so
it performs NO network I/O — eliminating the transient-DAPI path AND a
redundant second fetch of the same identity.
- It returns a typed RegisterExternalError::{Permanent,Transient}.
build_contact_accounts marks the channel broken only on Permanent
(malformed encrypted xpub / missing key — re-deriving won't help) and
leaves it intact on Transient (persistence/insert hiccup) for the next
sweep to retry.
Test would have caught this in CI: register_external_classifies_infra_miss_as_transient
is ✖ when the failure is flattened to all-permanent, ✔ with the split;
register_external_classifies_missing_key_as_permanent pins the other arm.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion error ContactRequestValidation::add_purpose_error set purpose_mismatch=true even when a genuinely-permanent hard error (disabled / missing / wrong-type key) was also recorded. build_contact_accounts read the bare purpose_mismatch flag to downgrade the failure to a non-permanent skip (G15) — so a request that tripped BOTH a purpose mismatch AND a hard error was retried forever instead of marking the channel broken. Track non-purpose hard errors separately (hard_error flag, set by add_error and propagated through merge) and gate the skip on a new is_purpose_only() == purpose_mismatch && !hard_error. The build path now downgrades to a skip only when the purpose mismatch is the SOLE cause. Test would have caught this in CI: purpose_mismatch_with_hard_error_is_not_purpose_only is ✖ when is_purpose_only() == purpose_mismatch (old semantics), ✔ with the hard_error guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ay ECDH select_recipient_key_index and the contactInfo root-encryption-key selector both picked the first key matching purpose+type via a bare .find() with no disabled-key gate — so a revoked key could be chosen to receive the contact's DIP-15 compact xpub (or to encrypt contactInfo), handing that material to whoever holds the compromised private half. The sibling AUTHENTICATION selector already excludes disabled keys. Add `&& k.disabled_at().is_none()` to both selectors (mirrors the validator's disabled-key check). Test would have caught this in CI: skips_disabled_decryption_key_and_falls_back_to_enabled_encryption is ✖ without the filter (selects the disabled key id 0), ✔ with it (falls back to the enabled key id 1); errors_when_only_candidate_key_is_disabled pins the no-enabled-key case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spec for extracting ManagedIdentity's 12 DashPay fields into an encapsulated DashPayState sub-struct, answering the PR #3841 review question about separating DashPay from identity-core concerns. Reviewed by four independent lenses (feasibility, scope, adversarial failure-modes, Rust/domain-fit); must-fixes folded in rev 2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o DashPayState Mechanical regroup — commit 1 of the DashPayState encapsulation spec (docs/dashpay/DASHPAY_STATE_ENCAPSULATION_SPEC.md, §5 stage 1). ManagedIdentity carried 12 flat DashPay fields (contacts, requests, profile, payments, sync cursors, deferred crypto) next to its identity-core fields with nothing marking the boundary. They now live in a single `dashpay: DashPayState` sub-struct (state/managed_identity/dashpay.rs); the three `dashpay_`-prefixed names drop the redundant prefix behind the new field (profile, payments, rescan_triggered). All fields remain pub in this commit — tier visibility and invariant-holding accessors land separately. ~500 access sites re-pathed mechanically across platform-wallet, platform-wallet-ffi, and platform-wallet-storage via exact compiler-span rewriting; the two constructors collapse their 12 per-field initializers into `dashpay: Default::default()`. Also deletes the orphaned zero-byte state/managed_identity/tests.rs left behind by the earlier identity/ relayering. Behavior-identical by construction: no method bodies, visibility, or persistence shapes change (ManagedIdentity is never serialized; the flat IdentityEntry/ContactChangeSet wire types are untouched). Tests: 338 platform-wallet lib + 9 contact_workflow + 159 ffi + full platform-wallet-storage suite, all passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… accessors
Encapsulation — commit 2 of the DashPayState spec (§3 D1-D7, §5 stage 2).
The dashpay field on ManagedIdentity becomes module-private (also
ruling out whole-value replacement via reassignment/mem::take, which
would silently wipe the guarded state), with:
- dashpay() borrow getter; Tier A relationship maps and sync cursors
are pub(super) with read getters on DashPayState itself; Tier B
caches (profile, payments, contact_profiles, rescan_triggered,
pending_contact_crypto) stay pub behind per-field *_mut accessors.
- advance_high_water_{received,sent}: the compare-and-advance cursor
rule moves from free fns in network code into the state layer as the
only cursor write path; a concurrent unignore rewind can no longer be
clobbered by a raw write. Free-fn tests ported to method-level tests
plus a snapshot-mismatch kill-the-mutant case.
- apply_* replay family (sent/incoming/established/ignored/unignored
pub for the FFI cold-load path, removed_* pub(crate)): named,
documented invariant bypasses for changeset replay and restore,
replacing raw map writes in wallet/apply.rs, manager/apply.rs, and
ffi/persistence.rs. Wholesale ignored-set assigns become per-element
apply loops (equivalent on constructor-fresh sets; pinned by test).
- established_contact_mut promoted to pub as the documented per-contact
sub-field escape hatch.
- New parity tests: replay must NOT auto-establish a reciprocal pair;
apply-loop == wholesale-assign for ignored senders; replay un-ignore
must not rewind the live cursor.
Raw Tier A field writes outside state/managed_identity/ are now
compile errors; invariant-bypassing writes are named apply_* and
greppable. On-disk format and FFI ABI untouched (nothing serializes
ManagedIdentity; entry types unchanged).
Tests: 340 platform-wallet lib + 9 contact_workflow + 159 ffi +
platform-wallet-storage suite, all passing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t::dashpay() Facade namespace — commit 3 of the DashPayState spec (§3 D8). The DashPay-contract operations (~10k lines across contact_requests / contacts / contact_info / payments / profile) sat as impl blocks directly on IdentityWallet<B>, indistinguishable at call sites from identity-lifecycle ops. They now live on DashPayView<'_, B> — a zero-cost borrowing view (one shared reference, Copy, Derefs to IdentityWallet) reached via wallet.identity().dashpay(). This is deliberately NOT a revival of the owned DashPayWallet<B> that was merged away: no second handle crosses the FFI, no extra clones, and straddling ops keep direct access to the identity-side plumbing (signer derivation, DPNS resolution) through Deref. Call sites convey the domain: wallet.identity().dashpay().send_contact_request(…). FFI function signatures and Swift are unchanged — the FFI functions re-path internally. The sync coordinator (manager/dashpay_sync.rs) and in-crate callers re-path mechanically. Tests: 340 platform-wallet lib + 9 contact_workflow + 159 ffi, all passing; zero new warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…action Review follow-ups from the four-lens code review of the extraction (behavior-parity, quality, silent-failure, spec-conformance — no semantic defects found; all findings were doc rot plus one visibility tightening): - wallet/apply.rs: the contacts-apply section comment claimed trivial map ops are inlined — they now route through the apply_* replay methods; rewritten to describe the actual convention. - network/payments.rs: fix broken intra-doc link to the moved rescan-guard field (now DashPayState::rescan_triggered). - network/contact_requests.rs: QR-scan block doc now points at IdentityWallet::resolve_name through the view's Deref; stale managed.established_contacts path in the rotation-heal comment. - FFI + storage prose that named the removed flat field paths (ManagedIdentity.dashpay_payments / .contact_profiles / .ignored_senders / .pending_contact_crypto) reworded against the DashPayState shape. - apply_unignored_sender tightened pub -> pub(crate): every caller is in-crate; cold-load restore replays the final ignored set (inserts only), never unignore events. Its pub siblings keep the FFI/storage callers that justify them. - Evened out the replay/persistence caveats across the Tier B *_mut accessor docs. Tests: 340 platform-wallet lib + 128 ffi lib, all passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Cumulative + incremental review at 018def2. The 6fb42e2→018def23 delta is a well-executed internal refactor: DashPay social fields on ManagedIdentity grouped into a private DashPayState behind invariant-holding accessors, and DashPay-contract methods on IdentityWallet namespaced behind a zero-cost .dashpay() view. The delta introduces no new FFI ABI changes, no new correctness/security issues, and no new findings. All 10 prior findings were re-verified against the current worktree and remain STILL VALID — none were fixed by the delta. Carried forward: 9 blocking FFI-safety issues (4 uninitialized destructor-backed out-params, 4 in-place C-ABI struct/callback expansions under unchanged symbols/slots, 1 in-place arg-order change on dash_sdk_sign_with_mnemonic_resolver_and_path) plus 1 defense-in-depth suggestion on established_contact_get_note. No new-delta findings.
Source: reviewers: opus (general), gpt-5.5 (general, recovered after retry), opus (security-auditor), gpt-5.5 (security-auditor, recovered after retry), opus (ffi-engineer), gpt-5.5 (ffi-engineer, recovered after retry); verifier: opus; specialists: security-auditor, ffi-engineer.
Carried-forward prior findings
All 10 prior findings from 6fb42e27 remain STILL_VALID at 018def23 and are carried forward below.
New findings in latest delta
None. The 6fb42e27..018def23 delta introduces no new verified findings.
Prior finding reconciliation
- prior-1 through prior-9: STILL_VALID blocking findings.
- prior-10: STILL_VALID suggestion.
🔴 9 blocking | 🟡 1 suggestion(s)
🤖 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-ffi/src/dashpay.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: platform_wallet_sync_contact_requests leaves *out_array uninitialized on failure
Verified STILL VALID at 018def23 (lines 171–185; delta only inserted `.dashpay()` before the async call on line 179). `check_ptr!(out_array)` on line 175 only validates non-null. The `PLATFORM_WALLET_STORAGE.with_item` lookup (177–180) and the async `identity.dashpay().sync_contact_requests().await` are both fallible; on either failure `unwrap_option_or_return!` / `unwrap_result_or_return!` (181–182) return without writing `*out_array` — the write happens only on line 183. The paired `platform_wallet_contact_request_handle_array_free` reconstructs `Box<[Handle]>` from any non-null pointer/count pair, so a C/Swift caller running symmetric cleanup-on-error feeds stale stack bytes into `Box::from_raw` — heap corruption / double-free reachable from any wallet-lookup miss or transient gRPC sync failure. Publish `ContactRequestHandleArray::empty()` immediately after the null check so every early return leaves an FFI-safe sentinel.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:491-507: platform_wallet_fetch_sent_contact_requests leaves *out_array uninitialized on failure
Verified STILL VALID at 018def23 (function shifted to 491–507; delta only added `.dashpay()` before `sent_contact_requests(&id).await` on line 501). Same defect pattern as the sync path: `check_ptr!(out_array)` on line 496 only null-checks, then `read_identifier` (497), the wallet lookup (499), `unwrap_option_or_return!` (503), and `unwrap_result_or_return!` (504) each early-return without writing `*out_array` — assigned only on the success path at line 505. Because this path consumes an external gRPC response, an attacker able to induce query failure (malformed response, disconnected endpoint, forged NotFound) can drive a cleanup-on-error caller into `platform_wallet_contact_request_handle_array_free` on stale stack bits — heap corruption / double-free. Zero-init to `ContactRequestHandleArray::empty()` before parsing the identifier.
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:323-380: create_or_update_dashpay_profile_with_signer leaves *out_profile uninitialized on failure
Verified STILL VALID at 018def23 (function at 323–380; delta only routed the two async branches through `identity.dashpay()` on lines 365/370). After `check_ptr!(out_profile)` / `check_ptr!(signer_handle)` (335–336) the function runs `read_identifier` (338), three `decode_opt_c_str` calls (340–342), an optional avatar `Vec<u8>` copy, wallet lookup, and the async `create_profile_with_external_signer` / `update_profile_with_external_signer` — all fallible — before assigning `*out_profile` on line 378. `DashPayProfileFFI` owns heap C-string / byte-buffer pointer fields freed by `dashpay_profile_ffi_free`, so a caller cleanup-on-error hands `CString::from_raw` uninitialized stack bytes as pointer fields — arbitrary free / heap corruption reachable via a hostile signer callback or any gRPC failure. Read-side profile helpers in this same file already publish `DashPayProfileFFI::empty()` first; this write path is inconsistent.
In `packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs:155-169: dash_sdk_sign_with_mnemonic_resolver_and_path changed C ABI in place
Verified STILL VALID at 018def23. `expected_key_data: *const u8` (163) and `expected_key_data_len: usize` (164) sit between `network: FFINetwork` and the output arguments while the exported `#[no_mangle]` symbol name is unchanged. Any Swift/C caller compiled against the pre-#3841 header resolves the same symbol at link time and passes the old argument order; under AArch64/SysV/Windows calling conventions the caller's `out_signature` pointer lands in the new `expected_key_data` slot and its capacity in `expected_key_data_len`. Consequences: (a) the new impersonation-resistance check silently reinterprets output-buffer bytes as expected-pubkey bytes (defeating the very defense being introduced), and (b) the real `out_signature*` writes shift to whatever registers/stack slots the caller left live — arbitrary memory write from a signing hot path bounded by `out_signature_capacity`. Rename to a versioned symbol (e.g. `..._v2`) and either drop the old symbol or keep it as a shim that calls the new one with `(null, 0)` for the expected-key parameters. If in-tree Swift is guaranteed to always rebuild against the current Rust ABI, state that assumption in the PR description.
In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/identity_persistence.rs:58-389: IdentityEntryFFI grew in place under the unchanged identity persistence callback
Verified STILL VALID at 018def23. The compile-time size guard on line 388 pins `IdentityEntryFFI` at 224 bytes (grown from 208 by appending `contact_profiles*` + `contact_profiles_count` on top of prior DashPay profile fields — `dashpay_profile_present`, three C-string pointers, `avatar_hash`, `avatar_fingerprint`, `public_message`, timestamps, `unique_dpns_username`). `PersistenceCallbacks.on_persist_identities_fn` still points at `*const IdentityEntryFFI` under the same callback slot and struct type name. A host compiled against an older header installs the callback and strides `upserts_ptr` using the old row size, misreading heap-owned `*const c_char` fields as scalar bytes or straddling row boundaries. Because Rust unconditionally calls `free_identity_entry_ffi` on every entry after the callback returns, that misinterpretation lands in `CString::from_raw` on non-CString memory — arbitrary free / heap corruption at persist-time. Version the callback slot or struct name (e.g. `on_persist_identities_fn_v2` + `IdentityEntryFFIV2`) so mixed artifacts fail at link time.
In `packages/rs-platform-wallet-ffi/src/contact_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/contact_persistence.rs:61-234: ContactRequestFFI grew in place under the unchanged contacts callback
Verified STILL VALID at 018def23. Line 233 pins `ContactRequestFFI` at 200 bytes — grown from a 144-byte base to 184 and now to 200 by appending `accepted_accounts: *const u32` and `accepted_accounts_len: usize`. `PersistenceCallbacks.on_persist_contacts_fn` and the `ContactRequestFFI` type name are unchanged. A host compiled against either intermediate size (144 or 184) installs the callback and iterates the upsert array with the wrong stride, treating tail fields or later rows as owned pointers. Because `free_contact_request_ffi` runs on every entry after the callback returns, the same misinterpretation drives `CString::from_raw` into non-CString bytes — arbitrary free / heap corruption. Version the callback slot or struct name.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:302-315: on_persist_contacts_fn signature grew (ignored_ptr/ignored_count) under unchanged slot
Verified STILL VALID at 018def23 (lines 302–315). The `on_persist_contacts_fn` callback function-pointer type takes two additional trailing arguments (`ignored_ptr: *const ContactIgnoredSenderFFI`, `ignored_count: usize`) after the existing removal args, but the callback field name and slot in `PersistenceCallbacks` are unchanged. A host binary compiled against the pre-#3841 (7-argument) header can still install a callback into this slot; the new Rust caller invokes it with the expanded 9-argument ABI — an incompatible function-pointer call. Consequences: (i) ignored-sender tombstones are silently dropped, so previously-ignored contacts resurface after restore (loss of the user's block/mute decision, enables reply-attack-style contact-request flooding); (ii) on many ABIs the mismatched trailing pointer/length arguments cause register/stack corruption in the callee. Version the callback slot or negotiate a callback-table version before invoking with the new arguments.
In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:249-343: IdentityRestoreEntryFFI grew under the unchanged wallet-load callback
Verified STILL VALID at 018def23 (delta on this file is doc-comment prose only; the `#[repr(C)]` layout is unchanged and still carries `contacts*/contacts_count`, `payments*/payments_count`, `ignored_senders*/ignored_senders_count`, `contact_profiles*/contact_profiles_count` on top of the base layout). `IdentityRestoreEntryFFI` is allocated by Swift `loadWalletListCallback` and consumed by Rust through `on_load_wallet_list_fn`; the load callback symbol/slot and the struct name are unchanged. A host compiled against the older layout allocates smaller rows; the new Rust side indexes that array using the larger `mem::size_of` stride and reads past each Swift-allocated row into unrelated heap memory — crashes or arbitrary heap reads reinterpreted as *const pointers and iterated by count during contacts/payments/ignored-senders/profile hydration at wallet load. Version the load callback or introduce a new restore struct for the expanded identity payload.
In `packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs:112-199: platform_address_wallet_withdraw_to_address leaves *out_changeset uninitialized on failure
Verified STILL VALID at 018def23 (lines 112–199 unchanged; withdrawal.rs not touched in the delta). `check_ptr!(out_changeset)` on line 127, then five fallible operations before `*out_changeset` is written on line 197 — with no sentinel publication in between: `CStr::from_ptr(core_address).to_str()` (131), `dashcore::Address::from_str` (135), `parse_input_selection` (137), the `PLATFORM_ADDRESS_WALLET_STORAGE.with_item` wallet lookup (149–150), the explicit `ErrorInvalidNetwork` return (159–170 — a caller-controlled path triggered by any mismatched Core address, e.g. testnet address pasted on mainnet, or a malicious QR/deeplink), and the async `wallet.withdraw(...)` itself (182–195). `PlatformAddressChangeSetFFI` is freed by `platform_address_wallet_free_changeset`, which reconstructs `Vec::from_raw_parts(cs.updated, cs.updated_count, cs.updated_count)` whenever `updated` is non-null and `updated_count > 0`; a C/Swift caller running symmetric cleanup-on-error can therefore pass stale stack bytes into a real `Vec::from_raw_parts` — double-free / heap corruption reachable purely from an attacker-controllable address string. In-tree Swift may pre-zero the struct today, but the Rust C ABI contract itself must publish the empty sentinel before any fallible work, matching the null-sentinel-first convention used on the read-side profile helpers.
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:157-170: established_contact_get_note should null *out_note before fallible lookup
Verified STILL VALID at 018def23 (function at 157–170). `check_ptr!(out_note)` (161) validates non-null; the two `unwrap_option_or_return!` calls (165, 166) and `unwrap_result_or_return!` (167) each return without writing `*out_note`. On the success path the FFI hands the caller an owned pointer requiring `platform_wallet_string_free`; a symmetric error-path caller loop that frees the same variable unconditionally risks freeing garbage. In-tree Swift currently zero-initializes `notePtr`, so this is defense-in-depth hardening rather than an in-tree exploit, but the rest of the FFI surface follows the null-sentinel-first convention.
…ippy type_complexity) The DashPayState-extraction refactor's client-side ECDH path annotated `ecdh_provider` with `EcdhProvider<fn(&IdentityPublicKey, u32) -> Ready<Result<SecretKey, Error>>, _, _, _>` — the never-called `fn` placeholder for the unused SdkSide `F` param — which trips `clippy::type_complexity` (denied in CI), failing the platform-wallet lib build. Factor the placeholder into a local `type UnusedSdkSideEcdh` alias; no behavior change. clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative + incremental verification at 0f22c34. The 018def2→0f22c340 delta is a single clippy type_complexity fix in packages/rs-platform-wallet/src/wallet/identity/network/sdk_writer.rs (aliasing an unused SdkSide fn-pointer type on EcdhProvider); no FFI file touched, no behavior change, no new findings. All 10 prior findings re-verified against current HEAD and remain STILL VALID — 9 blocking FFI-safety defects across rs-platform-wallet-ffi (4 uninitialized destructor-backed out-params reachable from attacker-controllable failure paths, 4 in-place C-ABI struct/callback layout expansions under unchanged symbols/slots, 1 in-place no_mangle argument-order change on dash_sdk_sign_with_mnemonic_resolver_and_path that undermines the impersonation check it introduces) plus 1 defense-in-depth suggestion on established_contact_get_note. Carried forward unchanged.
Source: reviewers: opus (general), gpt-5.5 (general), opus (security-auditor), gpt-5.5 (security-auditor), opus (ffi-engineer), gpt-5.5 (ffi-engineer); verifier: opus; specialists: security-auditor, ffi-engineer.
Carried-forward prior findings: all 10 prior findings from 018def23 remain STILL_VALID at 0f22c340 and are carried forward below.
New findings in latest delta: none. The 018def23..0f22c340 delta only aliases the unused SDK-side ECDH provider function type in sdk_writer.rs and introduces no new verified findings.
Prior finding reconciliation: prior-1 through prior-9 are STILL_VALID blocking findings; prior-10 is a STILL_VALID suggestion. No prior finding was fixed, outdated, or intentionally deferred.
🔴 9 blocking | 🟡 1 suggestion(s) | 💬 0 nitpick(s)
🤖 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-ffi/src/dashpay.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:171-185: platform_wallet_sync_contact_requests leaves *out_array uninitialized on failure
Carried-forward prior-1, verified STILL VALID at 0f22c340 (function body unchanged from 018def23). `check_ptr!(out_array)` on line 175 only null-checks. The `PLATFORM_WALLET_STORAGE.with_item` lookup (177–180) and the async `identity.dashpay().sync_contact_requests().await` are both fallible; on either failure `unwrap_option_or_return!` / `unwrap_result_or_return!` (181–182) return without writing `*out_array` — the write happens only on line 183. The paired `platform_wallet_contact_request_handle_array_free` reconstructs `Box<[Handle]>` from any non-null pointer/count pair, so a C/Swift caller running symmetric cleanup-on-error feeds stale stack bytes into `Box::from_raw` — heap corruption / double-free reachable from any wallet-lookup miss or transient gRPC sync failure. Publish `ContactRequestHandleArray::empty()` immediately after the null check so every early return leaves an FFI-safe sentinel.
Suggested replacement:
check_ptr!(out_array);
unsafe { *out_array = ContactRequestHandleArray::empty() };
let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| {
let identity = wallet.identity().clone();
block_on_worker(async move { identity.dashpay().sync_contact_requests().await })
});
let result = unwrap_option_or_return!(option);
let list = unwrap_result_or_return!(result);
unsafe { *out_array = ContactRequestHandleArray::from_requests(list) };
PlatformWalletFFIResult::ok()
}
In `packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay.rs:491-507: platform_wallet_fetch_sent_contact_requests leaves *out_array uninitialized on failure
Carried-forward prior-2, verified STILL VALID at 0f22c340 (function still at 491–507, unchanged). Same defect pattern as the sync path: `check_ptr!(out_array)` on line 496 only null-checks, then `read_identifier` (497), the wallet lookup (499), `unwrap_option_or_return!` (503) and `unwrap_result_or_return!` (504) each early-return without writing `*out_array` — assigned only on the success path at line 505. Because this path consumes an external gRPC response, an attacker able to induce query failure (malformed response, disconnected endpoint, forged NotFound) can drive a cleanup-on-error caller into `platform_wallet_contact_request_handle_array_free` on stale stack bits — heap corruption / double-free. Zero-init to `ContactRequestHandleArray::empty()` before parsing the identifier.
Suggested replacement:
check_ptr!(out_array);
unsafe { *out_array = ContactRequestHandleArray::empty() };
let id = unwrap_result_or_return!(unsafe { read_identifier(identity_id) });
let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| {
let identity = wallet.identity().clone();
block_on_worker(async move { identity.dashpay().sent_contact_requests(&id).await })
});
let result = unwrap_option_or_return!(option);
let list = unwrap_result_or_return!(result);
unsafe { *out_array = ContactRequestHandleArray::from_requests(list) };
PlatformWalletFFIResult::ok()
}
In `packages/rs-platform-wallet-ffi/src/dashpay_profile.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/dashpay_profile.rs:323-380: create_or_update_dashpay_profile_with_signer leaves *out_profile uninitialized on failure
Carried-forward prior-3, verified STILL VALID at 0f22c340 (unchanged at 323–380). After `check_ptr!(out_profile)` / `check_ptr!(signer_handle)` (335–336) the function runs `read_identifier` (338), three `decode_opt_c_str` calls (340–342), an optional avatar `Vec<u8>` copy, wallet lookup, and the async `create_profile_with_external_signer` / `update_profile_with_external_signer` — all fallible — before assigning `*out_profile` on line 378. `DashPayProfileFFI` owns heap C-string / byte-buffer pointer fields freed by `dashpay_profile_ffi_free`, so a caller cleanup-on-error hands `CString::from_raw` uninitialized stack bytes as pointer fields — arbitrary free / heap corruption reachable via a hostile signer callback or any gRPC failure. Read-side profile helpers in this same file already publish `DashPayProfileFFI::empty()` first; this write path is inconsistent.
Suggested replacement:
check_ptr!(out_profile);
check_ptr!(signer_handle);
unsafe { *out_profile = DashPayProfileFFI::empty() };
let id = unwrap_result_or_return!(read_identifier(identity_id));
In `packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs:155-169: dash_sdk_sign_with_mnemonic_resolver_and_path changed C ABI in place
Carried-forward prior-4, verified STILL VALID at 0f22c340 (signature unchanged; direct read confirms `expected_key_data`/`expected_key_data_len` inserted between `network: FFINetwork` and the output arguments while the exported `#[no_mangle]` symbol name is unchanged). Any Swift/C caller compiled against the pre-#3841 header resolves the same symbol at link time and passes the old argument order; under AArch64/SysV/Windows calling conventions the caller's `out_signature` pointer lands in the new `expected_key_data` slot and its capacity in `expected_key_data_len`. Consequences: (a) the new impersonation-resistance check silently reinterprets output-buffer bytes as expected-pubkey bytes (defeating the very defense being introduced), and (b) the real `out_signature*` writes shift to whatever registers/stack slots the caller left live — arbitrary memory write from a signing hot path bounded by `out_signature_capacity`. Rename to a versioned symbol (e.g. `..._v2`) and either drop the old symbol or keep it as a shim that calls the new one with `(null, 0)` for the expected-key parameters. If in-tree Swift is guaranteed to always rebuild against the current Rust ABI, state that assumption explicitly in the PR description.
In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/identity_persistence.rs:58-389: IdentityEntryFFI grew in place under the unchanged identity persistence callback
Carried-forward prior-5, verified STILL VALID at 0f22c340. The compile-time size guard on line 388 pins `IdentityEntryFFI` at 224 bytes (grown by appending `contact_profiles*` + `contact_profiles_count` on top of prior DashPay profile fields — `dashpay_profile_present`, three C-string pointers, `avatar_hash`, `avatar_fingerprint`, `public_message`, timestamps, `unique_dpns_username`). `PersistenceCallbacks.on_persist_identities_fn` still points at `*const IdentityEntryFFI` under the same callback slot and struct type name. A host compiled against an older header installs the callback and strides `upserts_ptr` using the old row size, misreading heap-owned `*const c_char` fields as scalar bytes or straddling row boundaries. Because Rust unconditionally calls `free_identity_entry_ffi` on every entry after the callback returns, that misinterpretation lands in `CString::from_raw` on non-CString memory — arbitrary free / heap corruption at persist-time. Version the callback slot or struct name (e.g. `on_persist_identities_fn_v2` + `IdentityEntryFFIV2`) so mixed artifacts fail at link time.
In `packages/rs-platform-wallet-ffi/src/contact_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/contact_persistence.rs:61-234: ContactRequestFFI grew in place under the unchanged contacts callback
Carried-forward prior-6, verified STILL VALID at 0f22c340. Line 233 still pins `ContactRequestFFI` at 200 bytes — grown from a 144-byte base to 184 and now to 200 by appending `accepted_accounts: *const u32` and `accepted_accounts_len: usize`. `PersistenceCallbacks.on_persist_contacts_fn` and the `ContactRequestFFI` type name are unchanged. A host compiled against either intermediate size (144 or 184) installs the callback and iterates the upsert array with the wrong stride, treating tail fields or later rows as owned pointers. Because `free_contact_request_ffi` runs on every entry after the callback returns, the same misinterpretation drives `CString::from_raw` into non-CString bytes — arbitrary free / heap corruption. Version the callback slot or struct name.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:302-315: on_persist_contacts_fn signature grew (ignored_ptr/ignored_count) under unchanged slot
Carried-forward prior-7, verified STILL VALID at 0f22c340 (lines 302–315 unchanged). The `on_persist_contacts_fn` callback function-pointer type takes two additional trailing arguments (`ignored_ptr: *const ContactIgnoredSenderFFI`, `ignored_count: usize`) after the existing removal args, but the callback field name and slot in `PersistenceCallbacks` are unchanged. A host binary compiled against the pre-#3841 (7-argument) header can still install a callback into this slot; the new Rust caller invokes it with the expanded 9-argument ABI — an incompatible function-pointer call. Consequences: (i) ignored-sender tombstones are silently dropped, so previously-ignored contacts resurface after restore (loss of the user's block/mute decision, enables reply-attack-style contact-request flooding); (ii) on many ABIs the mismatched trailing pointer/length arguments cause register/stack corruption in the callee. Version the callback slot or negotiate a callback-table version before invoking with the new arguments.
In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:249-343: IdentityRestoreEntryFFI grew under the unchanged wallet-load callback
Carried-forward prior-8, verified STILL VALID at 0f22c340. `IdentityRestoreEntryFFI` still carries `contacts*/contacts_count`, `payments*/payments_count`, `ignored_senders*/ignored_senders_count`, `contact_profiles*/contact_profiles_count` on top of the base layout. `IdentityRestoreEntryFFI` is allocated by Swift `loadWalletListCallback` and consumed by Rust through `on_load_wallet_list_fn`; the load callback symbol/slot and the struct name are unchanged. A host compiled against the older layout allocates smaller rows; the new Rust side indexes that array using the larger `mem::size_of` stride and reads past each Swift-allocated row into unrelated heap memory — crashes or arbitrary heap reads reinterpreted as *const pointers and iterated by count during contacts/payments/ignored-senders/profile hydration at wallet load. Version the load callback or introduce a new restore struct for the expanded identity payload.
In `packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs:112-199: platform_address_wallet_withdraw_to_address leaves *out_changeset uninitialized on failure
Carried-forward prior-9, verified STILL VALID at 0f22c340 (lines 112–199 unchanged; withdrawal.rs not touched in latest delta). `check_ptr!(out_changeset)` on line 127, then five fallible operations before `*out_changeset` is written on line 197 — with no sentinel publication in between: `CStr::from_ptr(core_address).to_str()` (131), `dashcore::Address::from_str` (135), `parse_input_selection` (137), the `PLATFORM_ADDRESS_WALLET_STORAGE.with_item` wallet lookup (149–150), the explicit `ErrorInvalidNetwork` return (159–170 — a caller-controlled path triggered by any mismatched Core address, e.g. testnet address pasted on mainnet, or a malicious QR/deeplink), and the async `wallet.withdraw(...)` itself (182–195). `PlatformAddressChangeSetFFI` is freed by `platform_address_wallet_free_changeset`, which reconstructs `Vec::from_raw_parts(cs.updated, cs.updated_count, cs.updated_count)` whenever `updated` is non-null and `updated_count > 0`; a C/Swift caller running symmetric cleanup-on-error can therefore pass stale stack bytes into a real `Vec::from_raw_parts` — double-free / heap corruption reachable purely from an attacker-controllable address string. In-tree Swift may pre-zero the struct today, but the Rust C ABI contract itself must publish the empty sentinel before any fallible work, matching the null-sentinel-first convention used on the read-side profile helpers.
Suggested replacement:
check_ptr!(out_changeset);
unsafe { *out_changeset = PlatformAddressChangeSetFFI { updated: std::ptr::null_mut(), updated_count: 0 } };
check_ptr!(core_address);
check_ptr!(signer_address_handle);
let address_str = unwrap_result_or_return!(std::ffi::CStr::from_ptr(core_address).to_str());
In `packages/rs-platform-wallet-ffi/src/established_contact.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/established_contact.rs:157-170: established_contact_get_note should null *out_note before fallible lookup
Carried-forward prior-10, verified STILL VALID at 0f22c340 (function unchanged at 157–170). `check_ptr!(out_note)` (161) validates non-null; the two `unwrap_option_or_return!` calls (165, 166) and `unwrap_result_or_return!` (167) each return without writing `*out_note`. On the success path the FFI hands the caller an owned pointer requiring `platform_wallet_string_free`; a symmetric error-path caller loop that frees the same variable unconditionally risks freeing garbage. In-tree Swift currently zero-initializes `notePtr`, so this is defense-in-depth hardening rather than an in-tree exploit, but the rest of the FFI surface follows the null-sentinel-first convention.
Suggested replacement:
check_ptr!(out_note);
unsafe { *out_note = std::ptr::null_mut() };
let option =
ESTABLISHED_CONTACT_STORAGE.with_item(contact_handle, |contact| contact.note.clone());
let option = unwrap_option_or_return!(option);
let note = unwrap_option_or_return!(option);
let c_str = unwrap_result_or_return!(std::ffi::CString::new(note));
unsafe { *out_note = c_str.into_raw() };
PlatformWalletFFIResult::ok()
}
…lible work Several FFI entry points only wrote their destructor-backed out-param on the success path, leaving it holding uninitialized stack bytes on any early return. A C/Swift caller that runs symmetric cleanup-on-error then hands those bytes to the paired free routine — `Box::from_raw` / `CString::from_raw` / `Vec::from_raw_parts` — for a double-free / heap corruption reachable from ordinary failures (wallet-lookup miss, gRPC error, a mismatched Core address string, a hostile signer callback). Publish the empty/null sentinel immediately after the out-param null check, before any fallible work, matching the null-sentinel-first convention already used by the read-side profile helpers: - dashpay: sync_contact_requests, fetch_sent_contact_requests (ContactRequestHandleArray::empty) - dashpay_profile: create_or_update_..._with_signer (DashPayProfileFFI::empty) - established_contact: get_note (null *out_note) - platform_addresses: all five changeset producers — transfer, withdraw, withdraw_to_address, fund_from_asset_lock, resume_fund_from_asset_lock — via a new PlatformAddressChangeSetFFI::empty() sentinel The changeset sibling functions (transfer / withdraw / fund) were not individually flagged but carry the identical defect, so they are hardened together for a uniformly safe changeset FFI surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ining FFI producers A verification sweep of the whole crate (following the same defect class fixed in the previous commit) found twelve more FFI entry points whose destructor-backed out-param is written only on the success path, leaving uninitialized stack bytes on any early return — same free-of-garbage / double-free exposure via the paired free routine: - IdentifierArray producers (freed via Vec::from_raw_parts in platform_wallet_identifier_array_free): the three contact-request / established-contact id getters in contact.rs, identity_manager_get_all_identity_ids, and both in-memory identity-id listers in memory_explorer.rs — new IdentifierArray::empty() sentinel - platform_wallet_register_dpns_name_with_signer (out C-string, freed via CString::from_raw in platform_wallet_string_free) - contact_request_get_encrypted_public_key (out bytes+len, freed via Vec::from_raw_parts in platform_wallet_bytes_free) - managed_identity_get_label (out C-string; the null write sat after the fallible handle lookup) and managed_identity_get_public_keys (out array+count, freed via managed_identity_free_public_keys which also frees each entry's owned data_ptr) - platform_wallet_serialize_to_json_bytes / platform_wallet_deserialize_from_json_bytes in utils.rs Every producer now publishes its (null, 0) / null sentinel immediately after the out-param null check, before any fallible work, completing the null-sentinel-first convention across the crate's out-param surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Disposition of the 10 review findings (at 0f22c34 → fixed in 303c17a + 4bc7c41)@coderabbitai — note for re-review: all findings were validated against HEAD; here is what was fixed vs. dismissed and why. Fixed — uninitialized destructor-backed out-params (findings 1, 2, 3, 9, suggestion 10) — commit 303c17aAll five confirmed valid. Each entry point now publishes an FFI-safe empty/null sentinel immediately after the out-param null check, before any fallible work, so every early return leaves the out-param well-defined for a cleanup-on-error caller:
Extended beyond the findings (same commits): the flagged withdraw-to-address function has siblings with the byte-identical defect, so the whole class was closed out:
The crate's entire out-param surface now follows the null-sentinel-first convention. Verified: Dismissed — in-place ABI/layout changes (findings 4, 5, 6, 7, 8): not applicable under this build modelThese all presuppose a host binary compiled against a pre-#3841 header linking the new Rust library. That scenario is structurally impossible here, per the ABI assumption finding 4 asked us to state explicitly:
Per-finding confirmation against in-tree consumers:
🤖 Generated with Claude Code |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus (ffi-engineer), opus (general), opus (security-auditor), gpt-5.5[high] (ffi-engineer, failed_or_unparseable), gpt-5.5[high] (general, failed_or_unparseable), gpt-5.5[high] (security-auditor, failed_or_unparseable); verifier: opus; specialists: security-auditor, ffi-engineer
Cumulative review at 4bc7c41. The latest two commits are a defensive sweep that publishes FFI-safe out-param sentinels immediately after null checks and before any fallible work — this FIXES prior findings 1, 2, 3, 9, 10 (verified byte-for-byte). All five ABI-shape prior findings (4-8) remain STILL VALID at HEAD: in-place C ABI reshuffle of dash_sdk_sign_with_mnemonic_resolver_and_path, and in-place growth of IdentityEntryFFI (224 bytes), ContactRequestFFI (200 bytes), on_persist_contacts_fn (10-arg), and IdentityRestoreEntryFFI — all under unchanged symbol/type/slot names. No new defects introduced by the latest delta. All three agents converged independently on the same five carried-forward blockers.
Carried-forward prior findings: priors 4-8 remain STILL VALID at 4bc7c412 and are carried forward below.
New findings in latest delta: none. The 0f22c340..4bc7c412 delta fixes the sentinel/out-param prior class and did not introduce new verified defects.
Prior finding reconciliation: priors 1, 2, 3, 9, and 10 are FIXED; priors 4, 5, 6, 7, and 8 are STILL VALID; no prior finding was outdated or intentionally deferred.
🔴 5 blocking
Fixed prior findings
- *prior-1: platform_wallet_sync_contact_requests leaves out_array uninitialized on failure — FIXED at dashpay.rs:181 —
*out_array = ContactRequestHandleArray::empty()is now published immediately after thecheck_ptr!(out_array)null check and before all fallible work (storage lookup, gRPC sync). Every early return leaves *out_array well-defined for a symmetric cleanup-on-error caller. - *prior-2: platform_wallet_fetch_sent_contact_requests leaves out_array uninitialized on failure — FIXED at dashpay.rs:506 —
*out_array = ContactRequestHandleArray::empty()is published immediately aftercheck_ptr!(out_array)and beforeread_identifier/ the gRPC query, so every early return path is FFI-safe. - *prior-3: create_or_update_dashpay_profile_with_signer leaves out_profile uninitialized on failure — FIXED at dashpay_profile.rs:341 —
*out_profile = DashPayProfileFFI::empty()is published immediately aftercheck_ptr!(out_profile)andcheck_ptr!(signer_handle), beforeread_identifier,decode_opt_c_str, or the async ST submit. A cleanup-on-error caller now consistently sees an empty profile with null string pointers on any failure path. - *prior-9: platform_address_wallet_withdraw_to_address leaves out_changeset uninitialized on failure — FIXED at withdrawal.rs:138 —
*out_changeset = PlatformAddressChangeSetFFI::empty()is published immediately aftercheck_ptr!(out_changeset), before thecore_address/signer_address_handlenull checks, address parsing,ErrorInvalidNetworkpath, or async withdraw. The newPlatformAddressChangeSetFFI::empty()helper is applied uniformly across the changeset producer surface. - *prior-10: established_contact_get_note should null out_note before fallible lookup — FIXED at established_contact.rs:166 —
*out_note = std::ptr::null_mut()is now written immediately aftercheck_ptr!(out_note)and before the note lookup /CString::newallocation. A cleanup-on-error caller that unconditionallyplatform_wallet_string_frees the variable now frees null, not garbage.
Carried-forward findings
- [BLOCKING]
packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs:155-169: [Carried-forward prior-4, STILL VALID] dash_sdk_sign_with_mnemonic_resolver_and_path changed C ABI in place
Re-verified STILL VALID at 4bc7c41 (unchanged since 0f22c34). The exported #[no_mangle] symbol name is unchanged, but expected_key_data: *const u8 (line 163) and expected_key_data_len: usize (line 164) were inserted between network: FFINetwork and the output arguments. A Swift/C caller compiled against a pre-#3841 header resolves the same symbol at link time and passes the old argument order; under AArch64/SysV/Windows x64 calling conventions the caller's out_signature pointer lands in the new expected_key_data slot, out_signature_capacity in expected_key_data_len, and out_signature_len / out_error shift by two argument slots. The expected_key_data.is_null() != (expected_key_data_len == 0) guard at line 202 evaluates to false != false = false (both non-null / non-zero from the old caller's out_signature args) — the impersonation-resistance check silently proceeds with garbage bytes, defeating the defense being introduced. If the guard does fire, fail() writes to addresses the caller intended as scalar fields — a bounded arbitrary write in a signing hot path. The latest sentinel commits only harden out-params; this ABI-in-place issue is untouched. Fix by renaming to a versioned symbol (e.g. ..._v2) with the old symbol kept as a shim that passes (null, 0) for the expected-key parameters, or by asserting in the PR description that every in-tree Swift consumer is guaranteed to rebuild against the current Rust ABI (per build_ios.sh) and documenting that assumption on the FFI function.
- [BLOCKING]
packages/rs-platform-wallet-ffi/src/identity_persistence.rs:58-389: [Carried-forward prior-5, STILL VALID] IdentityEntryFFI grew in place under the unchanged identity persistence callback
Re-verified STILL VALID at 4bc7c41. const _: [u8; 224] = [0u8; std::mem::size_of::<IdentityEntryFFI>()]; at line 388 pins the struct at 224 bytes — grown from a 208-byte intermediate by appending contact_profiles: *const ContactProfileRowFFI + contact_profiles_count: usize, on top of the earlier growth for the dashpay_profile_* C-string / hash / fingerprint / public-message / timestamp fields. PersistenceCallbacks.on_persist_identities_fn still points at *const IdentityEntryFFI under the same slot name and struct type name. A host binary compiled against an older layout installs the callback and strides upserts_ptr using the old row size, misreading heap-owned *const c_char fields (dpns_names, dashpay_profile_display_name, dashpay_profile_bio, dashpay_profile_avatar_url, dashpay_profile_public_message) as scalar bytes or straddling row boundaries. Because Rust unconditionally calls free_identity_entry_ffi on every entry after the callback returns, the misinterpretation drives CString::from_raw on non-CString memory — arbitrary free / heap corruption at persist-time on any mixed-artifact deployment. The compile-time size guard protects Rust-side reshapes but does nothing for header drift on the consumer side. Fix: version the callback slot or struct name (on_persist_identities_fn_v2 / IdentityEntryFFIV2), or bump a top-level PersistenceCallbacks version field so a stale host is rejected at registration.
- [BLOCKING]
packages/rs-platform-wallet-ffi/src/contact_persistence.rs:61-234: [Carried-forward prior-6, STILL VALID] ContactRequestFFI grew in place under the unchanged contacts callback
Re-verified STILL VALID at 4bc7c41. const _: [u8; 200] = [0u8; std::mem::size_of::<ContactRequestFFI>()]; at line 233 pins the struct at 200 bytes — grown from a 144-byte base first to 184 (contact_account_label) and now to 200 by appending accepted_accounts: *const u32 and accepted_accounts_len: usize at bytes 184..=199 (per the documented layout at lines 222-230). PersistenceCallbacks.on_persist_contacts_fn and the ContactRequestFFI type name are unchanged. A host binary compiled against either intermediate size (144 or 184) installs the callback into the same slot and iterates the upsert array with the wrong stride, so the trailing owner-private metadata pointers (alias, note, contact_account_label, accepted_accounts) — all owned by the Rust side and freed via free_contact_requests_ffi — are read at rotated offsets or straddle adjacent rows. Because free_contact_request_ffi runs on every entry after the callback returns, the same misinterpretation drives CString::from_raw on non-CString bytes — arbitrary free / heap corruption. Fix: version the struct/callback slot (e.g. ContactRequestFFIV3 / on_persist_contacts_fn_v3), or gate registration behind a callback-version handshake so an ABI-stale host is rejected before it sees a mis-strided array.
- [BLOCKING]
packages/rs-platform-wallet-ffi/src/persistence.rs:302-315: [Carried-forward prior-7, STILL VALID] on_persist_contacts_fn signature grew (ignored_ptr/ignored_count) under unchanged slot
Re-verified STILL VALID at 4bc7c41 (lines 302-315). The on_persist_contacts_fn callback function-pointer type is now a 10-argument unsafe extern "C" fn — the trailing ignored_ptr: *const ContactIgnoredSenderFFI and ignored_count: usize were added, but the callback field name and slot in PersistenceCallbacks are unchanged. A host binary compiled against the pre-#3841 (8-argument) callback header can still install a callback into this slot — function-pointer types are opaque bytes to the linker — and the new Rust caller invokes it with the expanded 10-argument sequence. Consequences: (i) ignored-sender tombstones are silently dropped, so previously-ignored contacts resurface after restore (loss of the user's block/mute decision, enables reply-attack-style contact-request flooding); (ii) on many ABIs the mismatched trailing arguments cause register/stack corruption in the callee. Fix: rename the slot (on_persist_contacts_fn_v2) or bundle the callback tuple into a versioned struct so a stale host cannot install a mismatched function pointer into the current slot.
- [BLOCKING]
packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:249-343: [Carried-forward prior-8, STILL VALID] IdentityRestoreEntryFFI grew under the unchanged wallet-load callback
Re-verified STILL VALID at 4bc7c41. #[repr(C)] pub struct IdentityRestoreEntryFFI (line 250) now carries four appended arrays on top of the base identity/dpns/keys layout: contacts* + contacts_count (line 303-304), payments* + payments_count (314-315), ignored_senders: *const [u8; 32]* + ignored_senders_count (327-328), and contact_profiles* + contact_profiles_count (341-342). IdentityRestoreEntryFFI is allocated by the Swift loadWalletListCallback implementation and consumed by Rust via on_load_wallet_list_fn. The struct type name and callback slot are unchanged from the pre-PR API. A host compiled against the older layout allocates smaller row buffers; Rust indexes with size_of::<IdentityRestoreEntryFFI>() = size_new and reads past the end of the Swift allocation for every element beyond the first — heap OOB in Swift's allocation for arrays with count > 1. Even for a single-row case, Rust dereferences the new contacts / payments / ignored_senders / contact_profiles fields whose bytes are uninitialized Swift heap memory (crash or corrupt-load) during hydration. Consequence: crash / arbitrary reads on wallet restore for any Swift binary that hasn't been rebuilt against the new header. Fix: version the restore struct + callback slot, or add an explicit layout_version field the host initializes so Rust can reject a mismatched host at load time.
🤖 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-ffi/src/sign_with_mnemonic_resolver.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs:155-169: [Carried-forward prior-4, STILL VALID] dash_sdk_sign_with_mnemonic_resolver_and_path changed C ABI in place
Re-verified STILL VALID at 4bc7c412 (unchanged since 0f22c340). The exported `#[no_mangle]` symbol name is unchanged, but `expected_key_data: *const u8` (line 163) and `expected_key_data_len: usize` (line 164) were inserted between `network: FFINetwork` and the output arguments. A Swift/C caller compiled against a pre-#3841 header resolves the same symbol at link time and passes the old argument order; under AArch64/SysV/Windows x64 calling conventions the caller's `out_signature` pointer lands in the new `expected_key_data` slot, `out_signature_capacity` in `expected_key_data_len`, and `out_signature_len` / `out_error` shift by two argument slots. The `expected_key_data.is_null() != (expected_key_data_len == 0)` guard at line 202 evaluates to `false != false = false` (both non-null / non-zero from the old caller's out_signature args) — the impersonation-resistance check silently proceeds with garbage bytes, defeating the defense being introduced. If the guard does fire, `fail()` writes to addresses the caller intended as scalar fields — a bounded arbitrary write in a signing hot path. The latest sentinel commits only harden out-params; this ABI-in-place issue is untouched. Fix by renaming to a versioned symbol (e.g. `..._v2`) with the old symbol kept as a shim that passes `(null, 0)` for the expected-key parameters, or by asserting in the PR description that every in-tree Swift consumer is guaranteed to rebuild against the current Rust ABI (per `build_ios.sh`) and documenting that assumption on the FFI function.
In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/identity_persistence.rs:58-389: [Carried-forward prior-5, STILL VALID] IdentityEntryFFI grew in place under the unchanged identity persistence callback
Re-verified STILL VALID at 4bc7c412. `const _: [u8; 224] = [0u8; std::mem::size_of::<IdentityEntryFFI>()];` at line 388 pins the struct at 224 bytes — grown from a 208-byte intermediate by appending `contact_profiles: *const ContactProfileRowFFI` + `contact_profiles_count: usize`, on top of the earlier growth for the `dashpay_profile_*` C-string / hash / fingerprint / public-message / timestamp fields. `PersistenceCallbacks.on_persist_identities_fn` still points at `*const IdentityEntryFFI` under the same slot name and struct type name. A host binary compiled against an older layout installs the callback and strides `upserts_ptr` using the old row size, misreading heap-owned `*const c_char` fields (`dpns_names`, `dashpay_profile_display_name`, `dashpay_profile_bio`, `dashpay_profile_avatar_url`, `dashpay_profile_public_message`) as scalar bytes or straddling row boundaries. Because Rust unconditionally calls `free_identity_entry_ffi` on every entry after the callback returns, the misinterpretation drives `CString::from_raw` on non-CString memory — arbitrary free / heap corruption at persist-time on any mixed-artifact deployment. The compile-time size guard protects Rust-side reshapes but does nothing for header drift on the consumer side. Fix: version the callback slot or struct name (`on_persist_identities_fn_v2` / `IdentityEntryFFIV2`), or bump a top-level `PersistenceCallbacks` version field so a stale host is rejected at registration.
In `packages/rs-platform-wallet-ffi/src/contact_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/contact_persistence.rs:61-234: [Carried-forward prior-6, STILL VALID] ContactRequestFFI grew in place under the unchanged contacts callback
Re-verified STILL VALID at 4bc7c412. `const _: [u8; 200] = [0u8; std::mem::size_of::<ContactRequestFFI>()];` at line 233 pins the struct at 200 bytes — grown from a 144-byte base first to 184 (contact_account_label) and now to 200 by appending `accepted_accounts: *const u32` and `accepted_accounts_len: usize` at bytes 184..=199 (per the documented layout at lines 222-230). `PersistenceCallbacks.on_persist_contacts_fn` and the `ContactRequestFFI` type name are unchanged. A host binary compiled against either intermediate size (144 or 184) installs the callback into the same slot and iterates the upsert array with the wrong stride, so the trailing owner-private metadata pointers (`alias`, `note`, `contact_account_label`, `accepted_accounts`) — all owned by the Rust side and freed via `free_contact_requests_ffi` — are read at rotated offsets or straddle adjacent rows. Because `free_contact_request_ffi` runs on every entry after the callback returns, the same misinterpretation drives `CString::from_raw` on non-CString bytes — arbitrary free / heap corruption. Fix: version the struct/callback slot (e.g. `ContactRequestFFIV3` / `on_persist_contacts_fn_v3`), or gate registration behind a callback-version handshake so an ABI-stale host is rejected before it sees a mis-strided array.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:302-315: [Carried-forward prior-7, STILL VALID] on_persist_contacts_fn signature grew (ignored_ptr/ignored_count) under unchanged slot
Re-verified STILL VALID at 4bc7c412 (lines 302-315). The `on_persist_contacts_fn` callback function-pointer type is now a 10-argument `unsafe extern "C" fn` — the trailing `ignored_ptr: *const ContactIgnoredSenderFFI` and `ignored_count: usize` were added, but the callback field name and slot in `PersistenceCallbacks` are unchanged. A host binary compiled against the pre-#3841 (8-argument) callback header can still install a callback into this slot — function-pointer types are opaque bytes to the linker — and the new Rust caller invokes it with the expanded 10-argument sequence. Consequences: (i) ignored-sender tombstones are silently dropped, so previously-ignored contacts resurface after restore (loss of the user's block/mute decision, enables reply-attack-style contact-request flooding); (ii) on many ABIs the mismatched trailing arguments cause register/stack corruption in the callee. Fix: rename the slot (`on_persist_contacts_fn_v2`) or bundle the callback tuple into a versioned struct so a stale host cannot install a mismatched function pointer into the current slot.
In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:249-343: [Carried-forward prior-8, STILL VALID] IdentityRestoreEntryFFI grew under the unchanged wallet-load callback
Re-verified STILL VALID at 4bc7c412. `#[repr(C)] pub struct IdentityRestoreEntryFFI` (line 250) now carries four appended arrays on top of the base identity/dpns/keys layout: `contacts* + contacts_count` (line 303-304), `payments* + payments_count` (314-315), `ignored_senders: *const [u8; 32]* + ignored_senders_count` (327-328), and `contact_profiles* + contact_profiles_count` (341-342). `IdentityRestoreEntryFFI` is allocated by the Swift `loadWalletListCallback` implementation and consumed by Rust via `on_load_wallet_list_fn`. The struct type name and callback slot are unchanged from the pre-PR API. A host compiled against the older layout allocates smaller row buffers; Rust indexes with `size_of::<IdentityRestoreEntryFFI>() = size_new` and reads past the end of the Swift allocation for every element beyond the first — heap OOB in Swift's allocation for arrays with `count > 1`. Even for a single-row case, Rust dereferences the new `contacts` / `payments` / `ignored_senders` / `contact_profiles` fields whose bytes are uninitialized Swift heap memory (crash or corrupt-load) during hydration. Consequence: crash / arbitrary reads on wallet restore for any Swift binary that hasn't been rebuilt against the new header. Fix: version the restore struct + callback slot, or add an explicit `layout_version` field the host initializes so Rust can reject a mismatched host at load time.
…urability, size cap, key selection (#4018) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Keep the durable spec set + the evidence base; move the working artifacts out of the tree: - TODO.md (603-line dated working backlog) → migrated verbatim to issue #4020, which the specs now cite as the authoritative tracker. - research/01-dip-spec.md (DIP paraphrase; SPEC Part 2 already condenses it — the DIPs are the canonical source) and research/04-sdk-and-contract.md (worktree-relative file:line survey, stale by design) → dropped; retrievable from branch history. - research/06-interop-desk-check.md → PROMOTED to docs/dashpay/INTEROP_DESK_CHECK.md: it is the cited evidence for the consensus-facing wire-format decisions (69-byte compact xpub, key-purpose envelope, ASK28 byte order) referenced by SPEC.md G14 and DIP_CONFORMANCE_GAPS.md §3, so it ships with the specs. - research/07-contactinfo-conventions.md (94 lines) → folded into CONTACTINFO_FORMAT_SPEC.md as Appendix A (its only citer). All citations updated; this also fixes the pre-existing dangling references to research/02, 03, and 05, which were cited by SPEC.md but never shipped. Link-checked: every relative link under docs/dashpay resolves; no code or CI references the removed paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ader `load_ignored_senders` is a read-only, one-shot load-time SELECT (`WHERE wallet_id = ?1`) — the direct sibling of `load_state`, whose SELECT is already exempt in `READ_ONLY_PREPARE_ALLOWED`. When the reader was added its SELECT was never allow-listed, so the TC-P1-003 compile-time guard flagged its `prepare(` call as a writer that must use `prepare_cached`, failing the macOS Rust workspace test run. Add the `ignored_senders` reader to the read-only allow-list alongside the other `load_state`-style per-area readers, matching the existing design intent (writers use `prepare_cached`; one-shot load SELECTs may use `prepare`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus (ffi-engineer), opus (general), opus (security-auditor), gpt-5.5 (ffi-engineer), gpt-5.5 (general), gpt-5.5 (security-auditor); verifier: opus; specialists: security-auditor, ffi-engineer
Cumulative re-review at 5b7c317. Latest delta (b2c9d41..5b7c317) is a single test-only allow-list addition in packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs — no new production defects. All six prior blocking findings re-verified STILL VALID at head: one persist-order/durability bug in apply_rotated_incoming_request, one exported C ABI changed in place (dash_sdk_sign_with_mnemonic_resolver_and_path), three repr(C) structs grown in place under unchanged type names (IdentityEntryFFI @ 224B, ContactRequestFFI @ 200B, IdentityRestoreEntryFFI), and one callback signature grown in place under an unchanged slot (on_persist_contacts_fn). All four agents (claude-general, codex-general, claude-security-auditor, codex-security-auditor, claude-ffi-engineer, codex-ffi-engineer) converge on the same six blocking findings with high inter-agent agreement.
Reviewed commit: 5b7c31741b51e31b41f6d12d6f2f4b80970eb0fe
Prior reviewed commit: b2c9d41fad7b58b07506468a5e467b641da45fe5
🔴 6 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Prior Reconciliation
- prior-1 through prior-6: STILL VALID at
5b7c3174and carried forward below. - Fixed prior findings: none.
- Outdated prior findings: none.
- Intentionally deferred prior findings: none.
New Findings In Latest Delta
- None. The latest delta
b2c9d41f..5b7c3174only adds theignored_sendersread-only compile-time allow-list query inpackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs.
Carried-Forward Prior Findings
1. [BLOCKING] [Carried-forward prior-1, STILL VALID] apply_rotated_incoming_request mutates memory before persisting — store failure silently drops the rotation and pins payment_channel_broken=false in memory
Location: packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:547-594
Source: claude, codex
Re-verified STILL VALID at 5b7c317 (latest delta is test-only). Lines 547–594 still commit to live in-memory state before invoking the fallible persister:
- Established branch (549–575): writes
contact.incoming_request = request(558), clearspayment_channel_broken(559),contact_account_label(564), andexternal_account_reference(568), then clones the already-mutatedcontactintocs.established(569–575). - Pending branch (577–587):
*slot = request.clone()(580) mutatesself.dashpay.incoming_contact_requestsin place before building the changeset.
Only at line 593 does the function call persister.store(cs.into())?. The persister is the FFI-backed on_persist_contacts_fn callback, so a Swift-side I/O failure, sandbox eviction, or transient SQLite lock contention returns Err here. Memory now holds the new request; disk still holds the old one. On any retry, the already_applied guard at 531–545 compares against the in-memory state (c.incoming_request == request / *r == request) and returns Ok(false) — the rotation is silently lost from disk for the process lifetime, and no future rotation observation re-tries the persist.
Secondary consequence: memory holds payment_channel_broken = false while disk keeps it true; on any relaunch the load callback re-hydrates the broken flag and this contact is silently DoS'd on the send path with no way to re-trigger the rotation heal.
The same file's add_sent_contact_request (~lines 90–97) explicitly documents the correct invariant: 'Persist BEFORE committing to memory... if the store fails, memory must stay on the old reference so the retry sweep doesn't hit the same-reference no-op guard above.' The incoming-rotation path violates the discipline the sent-side path established.
Fix: build the updated clone into cs first, call persister.store(cs.into())?, and only then commit to self.dashpay.established_contacts / self.dashpay.incoming_contact_requests — mirroring the sent-side pattern.
2. [BLOCKING] [Carried-forward prior-2, STILL VALID] dash_sdk_sign_with_mnemonic_resolver_and_path inserted expected_key_data/_len mid-signature under unchanged exported symbol
Location: packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs:155-169
Source: claude, codex
Re-verified STILL VALID at 5b7c317. The #[no_mangle] pub unsafe extern "C" fn dash_sdk_sign_with_mnemonic_resolver_and_path at line 155 retains its pre-PR exported symbol name, but expected_key_data: *const u8 (163) and expected_key_data_len: usize (164) are inserted between network: FFINetwork and the trailing out-params.
A C/Swift caller compiled against the pre-PR header resolves the same symbol at link/dlsym time and passes the old argument order. Under AArch64 AAPCS, SysV AMD64, and Windows x64 calling conventions, those two extra slots consume what the caller intended as out_signature (now landing in expected_key_data) and out_signature_capacity (now landing in expected_key_data_len); out_signature_len / out_error shift by two.
Consequences:
- The null-vs-length guard at line 202 (
expected_key_data.is_null() != (expected_key_data_len == 0)) evaluatesfalse != false = falsefor stale callers (their non-null out_signature pointer + non-zero capacity look valid), so the impersonation-resistance check silently runs against garbage bytes taken from the wrong argument slot — precisely defeating the defense being introduced. - On the
fail()path (172–183), writes throughout_error/out_signature_len/out_signaturego through pointers the stale caller intended as scalar fields — a bounded arbitrary write in a signing hot path.
In-tree Swift is regenerated via build_ios.sh, so exposure is external C ABI consumers of the pre-existing symbol. Fix: rename to a versioned symbol (..._v2) and keep the old symbol as a compatibility shim passing (NULL, 0) for the new expected-key args.
3. [BLOCKING] [Carried-forward prior-3, STILL VALID] IdentityEntryFFI grew in place (to 224 B) under unchanged identity persistence callback slot
Location: packages/rs-platform-wallet-ffi/src/identity_persistence.rs:58-389
Source: claude, codex
Re-verified STILL VALID at 5b7c317. const _: [u8; 224] = [0u8; std::mem::size_of::<IdentityEntryFFI>()]; at line 388 pins the struct at 224 bytes — grown across this PR by appending contact_profiles: *const ContactProfileRowFFI at bytes 208..=215 and contact_profiles_count: usize at 216..=223 (layout comment at 384–385 confirms). PersistenceCallbacks.on_persist_identities_fn still points at *const IdentityEntryFFI under the same slot name and struct type name.
A host binary compiled against the older layout installs the callback via the same field name and strides upserts_ptr using the old row size — misreading heap-owned *const c_char fields (dpns_names, dashpay_profile_display_name, dashpay_profile_bio, dashpay_profile_avatar_url, dashpay_profile_public_message) as scalar bytes or straddling row boundaries. Because Rust unconditionally calls free_identity_entry_ffi on every entry after the callback returns, the misinterpretation drives CString::from_raw on non-CString memory — arbitrary free / heap corruption at persist time.
The compile-time size guard at 388 protects Rust-side reshapes but does nothing about header drift on the consumer side. Fix: version the callback slot or struct name (on_persist_identities_fn_v2 / IdentityEntryFFIV2), or add an abi_version: u32 field to PersistenceCallbacks that Rust rejects at registration.
4. [BLOCKING] [Carried-forward prior-4, STILL VALID] ContactRequestFFI grew in place (to 200 B) under unchanged contacts callback slot
Location: packages/rs-platform-wallet-ffi/src/contact_persistence.rs:61-234
Source: claude, codex
Re-verified STILL VALID at 5b7c317. const _: [u8; 200] = [0u8; std::mem::size_of::<ContactRequestFFI>()]; at line 233 pins the struct at 200 bytes — grown from a 144-byte base first to 184 (contact_account_label) and now to 200 by appending accepted_accounts: *const u32 at bytes 184..=191 and accepted_accounts_len: usize at 192..=199 (layout comment at 229–230 confirms). PersistenceCallbacks.on_persist_contacts_fn and the ContactRequestFFI type name are unchanged.
A host binary compiled against either intermediate size (144 or 184) installs the callback into the same slot and iterates the upsert array with the wrong stride, so trailing Rust-owned metadata pointers (alias, note, contact_account_label, accepted_accounts) are read at rotated offsets or straddle adjacent rows. Rust unconditionally frees them via free_contact_requests_ffi afterward, driving CString::from_raw on non-CString bytes — arbitrary free / heap corruption.
This struct is also reused as the row shape for IdentityRestoreEntryFFI.contacts on the load path (see prior-6), so the ABI risk fans out to hydration.
Fix: version the struct/callback slot (e.g. ContactRequestFFIV3 / on_persist_contacts_fn_v3), or gate registration behind a callback-version handshake.
5. [BLOCKING] [Carried-forward prior-5, STILL VALID] on_persist_contacts_fn signature grew to 10 args (ignored_ptr/ignored_count) under unchanged callback slot
Location: packages/rs-platform-wallet-ffi/src/persistence.rs:302-315
Source: claude, codex
Re-verified STILL VALID at 5b7c317. on_persist_contacts_fn at lines 302–315 is now a 10-argument unsafe extern "C" fn — the trailing ignored_ptr: *const ContactIgnoredSenderFFI (312) and ignored_count: usize (313) are new in this PR, but the PersistenceCallbacks field name is unchanged. Function-pointer types are opaque bytes to the linker/dlsym registrar, so a host built against the pre-PR 6- or 8-argument callback header can still install a callback into this slot; the new Rust caller invokes it with the expanded 10-argument sequence.
Consequences at the FFI boundary:
- Ignored-sender tombstones (this PR's
ignore_senderblock/mute decision, persisted through the trailing pair) are silently dropped by an ABI-stale host, so previously-ignored senders resurface on the next sync sweep — a direct durability regression of the user's block decision, which this PR's ownignore_senderfix specifically depends on to durably delete the pending row. - On stack-passed argument portions of some ABIs, the caller pushes two extra slots the callee never pops, leaving stack imbalance / register clobber in the callee frame.
Fix: rename the slot (on_persist_contacts_fn_v2), or bundle the callback tuple into a versioned struct (with a leading abi_version: u32 gate) so a stale host cannot install a mismatched function pointer.
6. [BLOCKING] [Carried-forward prior-6, STILL VALID] IdentityRestoreEntryFFI grew under unchanged wallet-load callback — Swift-allocated struct now undersized relative to Rust reads
Location: packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:249-343
Source: claude, codex
Re-verified STILL VALID at 5b7c317. #[repr(C)] pub struct IdentityRestoreEntryFFI (line 250) now carries four appended arrays on top of the base identity/dpns/keys layout: contacts + contacts_count (303–304), payments + payments_count (314–315), ignored_senders: *const [u8; 32] + ignored_senders_count (327–328), and contact_profiles + contact_profiles_count (341–342). The struct is allocated by the Swift loadWalletListCallback and consumed by Rust via on_load_wallet_list_fn. Struct type name and callback slot are unchanged from the pre-PR API.
A host compiled against the older layout allocates smaller row buffers on the Swift side; Rust indexes with the larger current size_of::<IdentityRestoreEntryFFI>() and reads past the end of the Swift allocation for every element beyond the first (heap OOB in Swift's allocation for arrays with count > 1). Even for a single-row case, Rust dereferences the new appended fields as uninitialized Swift heap memory during contacts/payments/ignored-senders/contact-profiles hydration.
The latest delta (adding the ignored_senders load-time SQL to the read-only allow-list) reinforces that ignored_senders is now on the critical restore path — a stale host produces UB on the very code path the PR is guarding.
Fix: version the restore struct + callback slot, or add an explicit layout_version: u32 field the Swift host initializes so Rust can reject a mismatched host at load time before dereferencing any trailing pointer.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/contact_requests.rs:547-594: [Carried-forward prior-1, STILL VALID] apply_rotated_incoming_request mutates memory before persisting — store failure silently drops the rotation and pins payment_channel_broken=false in memory
Re-verified STILL VALID at 5b7c3174 (latest delta is test-only). Lines 547–594 still commit to live in-memory state before invoking the fallible persister:
- Established branch (549–575): writes `contact.incoming_request = request` (558), clears `payment_channel_broken` (559), `contact_account_label` (564), and `external_account_reference` (568), then clones the already-mutated `contact` into `cs.established` (569–575).
- Pending branch (577–587): `*slot = request.clone()` (580) mutates `self.dashpay.incoming_contact_requests` in place before building the changeset.
Only at line 593 does the function call `persister.store(cs.into())?`. The persister is the FFI-backed `on_persist_contacts_fn` callback, so a Swift-side I/O failure, sandbox eviction, or transient SQLite lock contention returns Err here. Memory now holds the new request; disk still holds the old one. On any retry, the `already_applied` guard at 531–545 compares against the *in-memory* state (`c.incoming_request == request` / `*r == request`) and returns `Ok(false)` — the rotation is silently lost from disk for the process lifetime, and no future rotation observation re-tries the persist.
Secondary consequence: memory holds `payment_channel_broken = false` while disk keeps it `true`; on any relaunch the load callback re-hydrates the broken flag and this contact is silently DoS'd on the send path with no way to re-trigger the rotation heal.
The same file's `add_sent_contact_request` (~lines 90–97) explicitly documents the correct invariant: 'Persist BEFORE committing to memory... if the store fails, memory must stay on the old reference so the retry sweep doesn't hit the same-reference no-op guard above.' The incoming-rotation path violates the discipline the sent-side path established.
Fix: build the updated clone into `cs` first, call `persister.store(cs.into())?`, and only then commit to `self.dashpay.established_contacts` / `self.dashpay.incoming_contact_requests` — mirroring the sent-side pattern.
In `packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/sign_with_mnemonic_resolver.rs:155-169: [Carried-forward prior-2, STILL VALID] dash_sdk_sign_with_mnemonic_resolver_and_path inserted expected_key_data/_len mid-signature under unchanged exported symbol
Re-verified STILL VALID at 5b7c3174. The `#[no_mangle] pub unsafe extern "C" fn dash_sdk_sign_with_mnemonic_resolver_and_path` at line 155 retains its pre-PR exported symbol name, but `expected_key_data: *const u8` (163) and `expected_key_data_len: usize` (164) are inserted between `network: FFINetwork` and the trailing out-params.
A C/Swift caller compiled against the pre-PR header resolves the same symbol at link/dlsym time and passes the old argument order. Under AArch64 AAPCS, SysV AMD64, and Windows x64 calling conventions, those two extra slots consume what the caller intended as `out_signature` (now landing in `expected_key_data`) and `out_signature_capacity` (now landing in `expected_key_data_len`); `out_signature_len` / `out_error` shift by two.
Consequences:
1. The null-vs-length guard at line 202 (`expected_key_data.is_null() != (expected_key_data_len == 0)`) evaluates `false != false = false` for stale callers (their non-null out_signature pointer + non-zero capacity look valid), so the impersonation-resistance check silently runs against garbage bytes taken from the wrong argument slot — precisely defeating the defense being introduced.
2. On the `fail()` path (172–183), writes through `out_error` / `out_signature_len` / `out_signature` go through pointers the stale caller intended as scalar fields — a bounded arbitrary write in a signing hot path.
In-tree Swift is regenerated via `build_ios.sh`, so exposure is external C ABI consumers of the pre-existing symbol. Fix: rename to a versioned symbol (`..._v2`) and keep the old symbol as a compatibility shim passing `(NULL, 0)` for the new expected-key args.
In `packages/rs-platform-wallet-ffi/src/identity_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/identity_persistence.rs:58-389: [Carried-forward prior-3, STILL VALID] IdentityEntryFFI grew in place (to 224 B) under unchanged identity persistence callback slot
Re-verified STILL VALID at 5b7c3174. `const _: [u8; 224] = [0u8; std::mem::size_of::<IdentityEntryFFI>()];` at line 388 pins the struct at 224 bytes — grown across this PR by appending `contact_profiles: *const ContactProfileRowFFI` at bytes 208..=215 and `contact_profiles_count: usize` at 216..=223 (layout comment at 384–385 confirms). `PersistenceCallbacks.on_persist_identities_fn` still points at `*const IdentityEntryFFI` under the same slot name and struct type name.
A host binary compiled against the older layout installs the callback via the same field name and strides `upserts_ptr` using the old row size — misreading heap-owned `*const c_char` fields (`dpns_names`, `dashpay_profile_display_name`, `dashpay_profile_bio`, `dashpay_profile_avatar_url`, `dashpay_profile_public_message`) as scalar bytes or straddling row boundaries. Because Rust unconditionally calls `free_identity_entry_ffi` on every entry after the callback returns, the misinterpretation drives `CString::from_raw` on non-CString memory — arbitrary free / heap corruption at persist time.
The compile-time size guard at 388 protects Rust-side reshapes but does nothing about header drift on the consumer side. Fix: version the callback slot or struct name (`on_persist_identities_fn_v2` / `IdentityEntryFFIV2`), or add an `abi_version: u32` field to `PersistenceCallbacks` that Rust rejects at registration.
In `packages/rs-platform-wallet-ffi/src/contact_persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/contact_persistence.rs:61-234: [Carried-forward prior-4, STILL VALID] ContactRequestFFI grew in place (to 200 B) under unchanged contacts callback slot
Re-verified STILL VALID at 5b7c3174. `const _: [u8; 200] = [0u8; std::mem::size_of::<ContactRequestFFI>()];` at line 233 pins the struct at 200 bytes — grown from a 144-byte base first to 184 (contact_account_label) and now to 200 by appending `accepted_accounts: *const u32` at bytes 184..=191 and `accepted_accounts_len: usize` at 192..=199 (layout comment at 229–230 confirms). `PersistenceCallbacks.on_persist_contacts_fn` and the `ContactRequestFFI` type name are unchanged.
A host binary compiled against either intermediate size (144 or 184) installs the callback into the same slot and iterates the upsert array with the wrong stride, so trailing Rust-owned metadata pointers (`alias`, `note`, `contact_account_label`, `accepted_accounts`) are read at rotated offsets or straddle adjacent rows. Rust unconditionally frees them via `free_contact_requests_ffi` afterward, driving `CString::from_raw` on non-CString bytes — arbitrary free / heap corruption.
This struct is also reused as the row shape for `IdentityRestoreEntryFFI.contacts` on the load path (see prior-6), so the ABI risk fans out to hydration.
Fix: version the struct/callback slot (e.g. `ContactRequestFFIV3` / `on_persist_contacts_fn_v3`), or gate registration behind a callback-version handshake.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:302-315: [Carried-forward prior-5, STILL VALID] on_persist_contacts_fn signature grew to 10 args (ignored_ptr/ignored_count) under unchanged callback slot
Re-verified STILL VALID at 5b7c3174. `on_persist_contacts_fn` at lines 302–315 is now a 10-argument `unsafe extern "C" fn` — the trailing `ignored_ptr: *const ContactIgnoredSenderFFI` (312) and `ignored_count: usize` (313) are new in this PR, but the `PersistenceCallbacks` field name is unchanged. Function-pointer types are opaque bytes to the linker/dlsym registrar, so a host built against the pre-PR 6- or 8-argument callback header can still install a callback into this slot; the new Rust caller invokes it with the expanded 10-argument sequence.
Consequences at the FFI boundary:
1. Ignored-sender tombstones (this PR's `ignore_sender` block/mute decision, persisted through the trailing pair) are silently dropped by an ABI-stale host, so previously-ignored senders resurface on the next sync sweep — a direct durability regression of the user's block decision, which this PR's own `ignore_sender` fix specifically depends on to durably delete the pending row.
2. On stack-passed argument portions of some ABIs, the caller pushes two extra slots the callee never pops, leaving stack imbalance / register clobber in the callee frame.
Fix: rename the slot (`on_persist_contacts_fn_v2`), or bundle the callback tuple into a versioned struct (with a leading `abi_version: u32` gate) so a stale host cannot install a mismatched function pointer.
In `packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs:249-343: [Carried-forward prior-6, STILL VALID] IdentityRestoreEntryFFI grew under unchanged wallet-load callback — Swift-allocated struct now undersized relative to Rust reads
Re-verified STILL VALID at 5b7c3174. `#[repr(C)] pub struct IdentityRestoreEntryFFI` (line 250) now carries four appended arrays on top of the base identity/dpns/keys layout: `contacts` + `contacts_count` (303–304), `payments` + `payments_count` (314–315), `ignored_senders: *const [u8; 32]` + `ignored_senders_count` (327–328), and `contact_profiles` + `contact_profiles_count` (341–342). The struct is allocated by the Swift `loadWalletListCallback` and consumed by Rust via `on_load_wallet_list_fn`. Struct type name and callback slot are unchanged from the pre-PR API.
A host compiled against the older layout allocates smaller row buffers on the Swift side; Rust indexes with the larger current `size_of::<IdentityRestoreEntryFFI>()` and reads past the end of the Swift allocation for every element beyond the first (heap OOB in Swift's allocation for arrays with `count > 1`). Even for a single-row case, Rust dereferences the new appended fields as uninitialized Swift heap memory during contacts/payments/ignored-senders/contact-profiles hydration.
The latest delta (adding the `ignored_senders` load-time SQL to the read-only allow-list) reinforces that `ignored_senders` is now on the critical restore path — a stale host produces UB on the very code path the PR is guarding.
Fix: version the restore struct + callback slot, or add an explicit `layout_version: u32` field the Swift host initializes so Rust can reject a mismatched host at load time before dereferencing any trailing pointer.
) + address-balance height pin Upstream #3841 rewrote the platform-wallet-ffi DashPay API and #3650 added the AddressFunds height pin; this adapts the Android port with full fidelity: - tokens.rs: reject → ignore/unignore contact-sender exports (local mute, DP-06); send/accept/payment gained the coreSignerHandle resolver param; Kotlin TokensNative/Dashpay wrappers + FriendsScreen updated (Reject button → Ignore). - persistence.rs: contacts vtable now carries ignored-sender deltas (new onPersistContactIgnored) and 6 new contact metadata fields; IdentityRestoreEntryFFI's contact/profile/ignored fields wired through the staging-vecs pipeline with matching free-trampoline reclaim; onPersistAddressBalance forwards as_of_height (stored as lastSeenHeight, the Swift handler's shape) and the transfer request path passes the documented 0 pin. - Room: DashDatabase v2 (MIGRATION_1_2) — dashpay_ignored_senders table + contact-request metadata columns; DashpayDao/entity extended; 3 new round-trip unit tests (metadata, ignore delta, restore). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Issue being fixed or feature implemented
Milestone 1 of the DashPay completion plan (
docs/dashpay/SPEC.md, included in this PR with its research base). DashPay's contact-request flow was broken in four independent, previously-unknown ways:send_contact_requestwas rejected by consensus — the broadcast carried a document id derived from the creation entropy but fresh entropy in the transition; drive-abci recomputes the id and rejects withInvalidDocumentTransitionIdError.ExtendedPubKey::encode()form; DIP-15 and both reference mobile clients (iOS dash-shared-core, Android dashj) use the 69-byte compactfingerprint‖chaincode‖pubkey. Our send failed its own 96-byte ciphertext check; our receive couldn't parse mobile payloads.What was done?
Three logical commits:
docs(dashpay)— the 7-agent-reviewed implementation spec (protocol reference, per-layer inventory, gaps G1–G15, 5-milestone plan, Swift UI design, test plan) + 6 research files including the cross-client interop desk-check and the testnet key-purpose census.fix(sdk)!— entropy threading (ContactRequestResult.entropyreused at broadcast), the DIP-15 69-byte compact-xpub codec inplatform-encryption+ the SDK callback contract switched to it, and the recipient key-purpose assertion relaxed to DECRYPTION-or-ENCRYPTION.fix(platform-wallet)— new recurringDashPaySyncManager(iterates the wallets map, not the token registry; per-identity log-and-continue); ingest-guard relaxation + sent-side reconcile with idempotent, metadata-preserving merge; Accept adopts an existing on-platform reciprocal instead of re-broadcasting; per-sweep account rebuild (external and receiving accounts) with validate-before-ECDH, guard-drop lock ordering, and a transient/permanent failure policy (payment_channel_brokenflag, persisted + FFI accessor); rejected-request tombstone keyed(owner, sender, accountReference)so rotated requests still surface; 69-byte compact parsing on receive with address-equality pinned; key-purpose envelope aligned with on-chain reality;DashPaySdkWriterseam making the write paths testable.How Has This Been Tested?
TDD throughout — every behavioral fix has a test that was red against the unfixed code and green after (red→green evidence recorded in the SPEC.md M1 DONE notes and the three commit messages):
platform-wallet: 196 lib + 8 integration tests green (was 170 before this branch; +34 new)dash-sdk(--features mocks,offline-testing): 139 lib tests green (incl. the entropy-id and 69→96-byte pins)platform-encryption: 7/7 (the crate's test target previously failed to compile — fixed dev-deps)cargo checkclean onrs-sdk-ffi,platform-wallet-ffi,platform-wallet-storage; clippy clean on touched cratesdp_001..dp_006) is specced to ride the e2e framework in test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 and is explicitly not gated on this PR (SPEC.md Part 7.4)Note
CI:
Rust workspace tests / Tests (macOS)red on 3 pre-existing tests — passing locally.The macOS check fails only on three receiver-payment tests
(
register_contact_account_persists_account_registration,reconcile_records_received_payments_from_receival_utxos,reconcile_does_not_clobber_existing_entry_for_same_txid), all withExternal signable wallet has no private key.These pass locally in every configuration tested (9):
cargo test,cargo nextest(isolated and full platform-wallet suite), the CI feature set,
--all-features, theplatform-wallet-family feature unification, under
cargo llvm-covcoverage, and theexact CI package set (
drive+dpp+drive-abci+…--all-featuresunder coverage) —all on the same macOS/aarch64 as the runner. All green.
The wallet is provably
WalletType::Seed-bearing through every code path (from_seed→Seed; the manager'sinsert_walletstores it verbatim;get_walletreturns a&Wallet),yet only the CI runner reads it as
ExternalSignable. Root cause is a use-after-zeroize inthe
key-walletgit dependency:Wallethas aDropthat zeroizes itsZeroize-derivedwallet_type, so the discriminant can corrupt under a particular memory layout (UB isenvironment-dependent — it manifests on the CI runner but not locally). This is outside
this PR's code — pre-existing branch tests plus an external-dependency bug being tracked for
the key-wallet maintainers; the DashPay changes themselves are correct and green.
Breaking Changes
get_extended_public_keycallback contract forcreate_contact_request/send_contact_requestis now "return the 69-byte DIP-15 compact form" (was an encodedExtendedPubKey); validated before encryption.ContactRequestResultgains a publicentropy: Bytes32field. Thers-sdk-ffiC ABI is unchanged (caller doc contract tightened).contacts.payment_channel_brokencolumn,rejected_contact_requeststable) in the initial migration;ContactChangeSetgains arejectedfield.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation