Repository navigation
fix(platform-wallet): build our receiving account for one-way DashPay contacts - #5256
HashEngineering wants to merge 4 commits into
Conversation
… contacts A contact we sent a contact request to, who never sent one back, got no DashpayReceivingFunds account. DIP-15 puts our receiving xpub in the request we send, so that contact can pay us without reciprocating. But the sweep built accounts only for established contacts, and the rescan reconcile skipped every contact that was not established. Payments on that chain were never seen, at any rescan depth. The live send path registers the account itself, so this hit wallets that learned of the sent request from Platform (restore from seed, a second device) or whose live registration failed after the request was saved. Seen on a topple testnet wallet: a 0.001 DASH receive (e5169bfc…, height 1,475,820) on our chain for a contact whose only request is ours, sent 19 blocks earlier. It is missing from every archived SDK store and present in dashj's, and its later spend is recorded with a positive net amount because the funding transaction is unknown. The sweep now queues RegisterReceiving, and only that, for each unreciprocated sent request that has no receival account. There is no xpub of theirs to decrypt, so no external account can be built until they reciprocate. The rescan reconcile rewinds a sent-only receival account to our request's core height. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… alone The previous commit queued our receiving account for a contact whose request we sent and who never replied. Two established contacts were still left without one: a contact whose payment channel is marked broken, and a contact whose external account was built but whose receiving build failed once. The regular candidate gate skips both for good, and it is the only thing that re-queues a build after a relaunch. Our receiving account needs our identity, theirs and the signer. It never touches their xpub, so neither failure is a reason to skip it. The receiving-side collector now lists every contact that holds a request we sent, in sent_contact_requests or established_contacts, with no receival account, and ignores the broken flag. RegisterReceiving makes no fetch and no decrypt, so it cannot retry without bound. The external account keeps its own gate, and the overlap with the regular candidates is harmless because enqueueing is idempotent per (owner, contact, kind). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
✅ Final review complete — no blockers (commit e968545) · triage: normal |
|
Bots are done — your move: post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head de7bfa8. The receiving-only enqueue and sent-request rescan fallback match the PR's goal; no blocking issues were confirmed, and one method-documentation nitpick remains. Validation was static only: in the supplied CI snapshot, Rust workspace tests were still running and the dedicated Rust wallet tests were skipped.
💬 1 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: Sweep method docs omit the new receiving-only step
packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1334-1338
The per-sweep documentation on sync_contact_requests, whose implementation delegates to sync_contact_requests_reporting, describes account builds only for established contacts missing a sending account. The reporting implementation now also calls enqueue_receiving_account_builds for contacts holding our sent request, including unreciprocated contacts and established contacts excluded by the external-account gate. Add that receiving-only step to the method documentation so its behavior summary covers the recovery path introduced by this PR.
/// 4. For **every** established contact missing a sending account
/// (not only newly-established ones — this also repairs
/// restore-from-seed and best-effort-accept gaps), rebuilds both
/// the `DashpayReceivingFunds` and `DashpayExternalAccount`
/// accounts, with the transient/permanent failure policy.
/// 5. For every contact holding a request we sent, reciprocated or not,
/// and missing a receiving account, enqueues a
/// `DashpayReceivingFunds` build alone for the signer-backed drain.
/// This receiving-only step does not depend on the contact's xpub
/// or the external-account gate.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff adds receiving-account candidate discovery and queueing in contact_requests.rs and adjusts rescan eligibility in payments.rs, but does not change funds movement, coin selection, cryptography, key derivation, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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`:
- [NITPICK] packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs:1334-1338: Sweep method docs omit the new receiving-only step
The per-sweep documentation on `sync_contact_requests`, whose implementation delegates to `sync_contact_requests_reporting`, describes account builds only for established contacts missing a sending account. The reporting implementation now also calls `enqueue_receiving_account_builds` for contacts holding our sent request, including unreciprocated contacts and established contacts excluded by the external-account gate. Add that receiving-only step to the method documentation so its behavior summary covers the recovery path introduced by this PR.
The step list on sync_contact_requests described account builds for established contacts only. Add the receiving-account step the sweep now runs for every contact holding a request we sent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
/self-review |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete diff at e968545 addresses receiving-account discovery and rescan eligibility for one-way DashPay contacts without introducing a confirmed production defect. One new regression assertion does not actually exercise the single-shot rescan guard; the prior documentation finding is fixed. Validation was static only: no builds or tests were run, and the supplied CI snapshot still shows build and validation checks pending or running.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff adds receiving-account discovery and deferred registration plus rescan eligibility changes in contact_requests.rs and payments.rs, but does not alter funds movement, coin selection, cryptography, or key derivation itself. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:2962-2970: Advance the scan height before asserting the rescan is single-shot
After the first reconciliation, synced_height equals the sent request's funding height. The second call therefore returns None even if this one-way contact was never inserted into rescan_triggered, because funding < synced_height is already false. The assertion cannot detect a regression in the single-shot behavior it claims to verify. Advance synced_height before reconciling again and assert that the advanced height is preserved; without the guard, that call would rewind the wallet again.
| assert_eq!(synced_height(&manager, wallet_id).await, 1_475_801); | ||
| assert_eq!( | ||
| iw.dashpay() | ||
| .reconcile_dashpay_rescan() | ||
| .await | ||
| .expect("rescan 2"), | ||
| None, | ||
| "the guard makes it single-shot, as for established contacts" | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Advance the scan height before asserting the rescan is single-shot
After the first reconciliation, synced_height equals the sent request's funding height. The second call therefore returns None even if this one-way contact was never inserted into rescan_triggered, because funding < synced_height is already false. The assertion cannot detect a regression in the single-shot behavior it claims to verify. Advance synced_height before reconciling again and assert that the advanced height is preserved; without the guard, that call would rewind the wallet again.
| assert_eq!(synced_height(&manager, wallet_id).await, 1_475_801); | |
| assert_eq!( | |
| iw.dashpay() | |
| .reconcile_dashpay_rescan() | |
| .await | |
| .expect("rescan 2"), | |
| None, | |
| "the guard makes it single-shot, as for established contacts" | |
| ); | |
| assert_eq!(synced_height(&manager, wallet_id).await, 1_475_801); | |
| set_synced_height(&manager, wallet_id, 1_561_776).await; | |
| assert_eq!( | |
| iw.dashpay() | |
| .reconcile_dashpay_rescan() | |
| .await | |
| .expect("rescan 2"), | |
| None, | |
| "the guard makes it single-shot, as for established contacts" | |
| ); | |
| assert_eq!( | |
| synced_height(&manager, wallet_id).await, | |
| 1_561_776, | |
| "scan progress must not be rewound again" | |
| ); |
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
Issue being fixed or feature implemented
Suppose we send a DashPay contact request and the contact never sends one back. That contact never got a
DashpayReceivingFundsaccount. DIP-15 puts our receiving xpub inside the request we send, so the contact can pay us on that chain without ever reciprocating. Two places assumed the opposite:collect_account_build_candidates(contact_requests.rs) walksestablished_contacts()only, so the sweep never queued a receiving account for a one-way contact.reconcile_dashpay_rescan(payments.rs) skips every receival account whose contact is not established. Even with the account built, the history before it was registered would never be rescanned.Together, payments on that chain stay invisible at any rescan depth. The live send path (
send_contact_request_with_external_signer) registers the account itself. The gap therefore hits wallets that learned of the sent request from Platform (restore from seed, a second device), plus any live registration that failed after the request had been saved.Seen on testnet (topple wallet
7dc06ad3…):e5169bfc4989585abd4b0476188611b981e3c750539da5b8a39fe135e3bbb957, height 1,475,820: 0.001 DASH toyMNNc2UZz62V9N7SZfQnsk79G5okCP7eSN. That is our receiving chain15'/0'/(us)/(4193bbe6…)/0for contact5QzA7GnST….6ac8356c…is stored withnetAmount = +99734, because the funding transaction is unknown. The balance is right today only because the coin has been spent.Fixes #5246.
What was done?
enqueue_receiving_account_buildsafter the existing account builds. It callscollect_receiving_account_candidates, which lists every contact that holds a request we sent, insent_contact_requests()orestablished_contacts(), and has no receival account. Each one gets aRegisterReceivingqueue entry, and only that. The gate is on our side alone, because our receiving account depends only on our own request: it needs our identity, theirs and the signer, never their xpub. Besides the one-way contact, that covers two established cases the regular gate skips for good: a contact whose channel is markedpayment_channel_broken(the failure was in decrypting their xpub, which our receiving side never touches, andRegisterReceivingmakes no fetch and no decrypt, so it cannot retry without bound), and a contact whose external account was built but whose receiving build failed once (the external row survives a relaunch, so the regularhas_externalgate then skips it forever; this is gap 2 of platform-wallet: restored wallets miss historical DashPay contact payments — no registration-time rescan, and failed receival-account builds are never re-enqueued #4475). The external account keeps its own gate: it still needs the contact's xpub from a request they send us. Overlap with the regular candidates is harmless, since enqueueing is idempotent per(owner, contact, kind). Identities without an HD index are skipped. Because the queue is not restored on load (platform-wallet: restore the deferred DashPay contact-crypto queue on load #5091), re-discovering candidates every sweep is what carries a pending build across a relaunch.reconcile_dashpay_rescanrewinds a sent-only receival account to our request'score_height_created_at(the request carries our xpub). It no longer skips such accounts. Established contacts keepmin(outgoing, incoming).collect_account_build_candidatesandAccountBuildCandidateare unchanged on purpose (see "How this composes" below). Folding the receiving-only case into that struct would be cleaner, but fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026 makes bothpub(super)and uses them in tests; that consolidation is better done once the open work lands.How this composes with open work
f9426a4236. Its description names this exact scenario (Alice sends Bob a request from device A, Bob pays, device B scans the block before it sees the request, "the relationship may stay one-way"). But device B never gets the receival account under fix(platform-wallet): rescan DashPay contact accounts from the contact request height #4740: its production call sites ofregister_contact_accountare the same three as onv5.1-dev(live send, the drain'sRegisterReceivingarm, the accept path), its onlyRegisterReceivingenqueue is still reached throughcollect_account_build_candidates, which is unchanged and walksestablished_contacts()only, and its sent sweep (ingest_sent_sweep,note_sent_request_core_height) only records heights. Its own one-way test,should_cover_restored_sent_only_account_in_rescan, creates the account by callingregister_contact_accountdirectly, which models device A relaunching with its persisted account row, not device B. The topple store is the device-B case: our request for4193bbe6…and no account row. What fix(platform-wallet): rescan DashPay contact accounts from the contact request height #4740 does cover, better than this PR's rescan hunk, is the rescan once a sent-only account exists:receiving_scan_checkpointuses the earliest sent request height, andregister_contact_accountapplies the checkpoint at registration time. The two compose cleanly: this PR's sweep queues the build, the drain callsregister_contact_account, and fix(platform-wallet): rescan DashPay contact accounts from the contact request height #4740's version of it rewinds to the right height, which the sweep has recorded by then. contact_requests.rs merges cleanly. payments.rs conflicts only insidereconcile_dashpay_rescanand where the tests are inserted. Resolution: take fix(platform-wallet): rescan DashPay contact accounts from the contact request height #4740's version of the function, drop this PR's rescan hunk, and droprescan_backfills_a_one_way_contact_from_our_sent_request_heightin favour of fix(platform-wallet): rescan DashPay contact accounts from the contact request height #4740's test.reconcile_dashpay_rescanstill requires an established contact. Without this PR's rescan change, a sent-only account would be built but never rewound for and never recorded. contact_requests.rs merges cleanly: fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026 only makes the candidate collector and structpub(super), and those lines are untouched here. payments.rs has two conflict blocks in the same function. Resolution: keep fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026's record logic and replace its established-onlycheckpointlookup with this PR's fallback to the sent request's height. A newly watched one-way contact then gets a coverage entry like any other.fix/dashpay-contact-rescan-4475(a026f0059b, the same diff as2d230d0865): it widens the established-contact gate tohas_external && has_receival, re-queuing both ops for an established contact whose receiving build failed. This PR's receiving-side gate now covers that contact's receival account too, so the two overlap on that case; the overlap is harmless (idempotent enqueue) and the fork's change still adds the pairedRegisterExternalretry. It merges cleanly with this PR, because the new collector sits beside the existing one instead of changing it.How Has This Been Tested?
New tests:
one_way_contact_tests::should_build_receiving_account_for_one_way_contact_on_sweepruns the realsync_contact_requests_reportingon a mock SDK that answers both contact-request queries with no documents, over a wallet that holds a one-way sent request. It asserts both fetches were answered, then that exactly oneRegisterReceivingis queued. After draining with a seed provider, it asserts the receival account exists.payments::tests::rescan_backfills_a_one_way_contact_from_our_sent_request_height: a one-way contact's receival account rewindssynced_heightfrom 1,561,776 to 1,475,801, and only once.one_way_contact_tests::should_collect_every_contact_holding_our_request_as_receiving_candidatecovers which contacts are candidates: one-way sent and established are, one-way received is not.one_way_contact_tests::should_still_build_receiving_account_when_channel_is_broken: an established contact withpayment_channel_brokenand no accounts is skipped by the regular gate and listed by the receiving gate.one_way_contact_tests::should_queue_a_payload_free_receiving_op_for_one_way_contactchecks that re-enqueueing is idempotent and the queued op carries no payload.Without the fix: I reverted only the sweep's new call and the rescan change, keeping the helpers so the test module still compiled. Both regression tests failed:
[]instead of[RegisterReceiving]Noneinstead ofSome(1475801)With the fix:
cargo test -p platform-wallet --features shielded: 1,413 lib tests plus all integration test binaries pass (the full lib suite was re-run after the second commit; the integration binaries after the first).cargo test -p platform-wallet-ffi --features shielded: 430 lib tests plus all integration test binaries pass.cargo check --tests -p platform-wallet-storage: clean.Known limitation. The rescan hunk uses the tracked (newest) sent request's height. After a rotation re-send, that misses the span between the first publication of our xpub and the re-send. The established path on
v5.1-devhas the same limitation, and #4740 fixes both with its earliest-height checkpoint.Build note. This branch is based on
v5.1-devat218cb89f18, whose tip did not compile:packages/rs-platform-version/src/version/v15.rsstill importeddrive_abci_query_versions::v3::DRIVE_ABCI_QUERY_VERSIONS_V3after #5057 folded V3 into V2. #5212 (662c1fc945) has since fixed that onv5.1-dev. The local runs above used an equivalent uncommitted patch (both references changed toV2), which is not part of this PR. The branch merges into the currentv5.1-devtip (1ebcedb028) without conflicts, so it was left unrebased.QA on a device (topple, testnet)
Run by the kotlin-sdk int28 integration build (
integration/v42int28-pin@4dd8e3d0a2on the HashEngineering fork), upgraded in place over int27 on the topple wallet (7dc06ad3…) on 2026-10-02. What that build contains, checked withgit range-diffagainst this branch:fc9142a30d,9877f00e42);collect_receiving_account_candidates,enqueue_receiving_account_builds, the call site) byte-identical to this branch;reconcile_dashpay_rescan. This PR's 10-line checkpoint fallback (established →min(outgoing, incoming); one-way → our sent request'score_height_created_at; neither → skip) was placed inside fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026's version of that function; the diff of int28 against fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026's head is that insertion and nothing else. So the rescan fallback ran with fix(platform-wallet)!: persist DashPay coreHeight backfill coverage so a relaunch resumes instead of rewinding again #5026's durable guard rather than the in-memoryrescan_triggeredguard this PR targets onv5.1-dev. The guard decides whether a contact is rewound for a second time; the fallback decides whether and to what height it is rewound at all, and that logic is the same in both;integer_range_clausesdropped from the mockedDocumentQuery, a field int28's older base does not have;Results, from the SDK store captured two minutes after launch (
store-after1) and the logcat:e5169bfc…intransactionse5169bfc:0intxos6ac8356c…6ac8356c…netAmount4193bbe6…,c7037829…,ce3cff2f…)RegisterReceivingbuilds; the reconcile loggedlowered SPV synced_height … floor=1475801 rewound_from=1564627 contacts=3. 1,475,801 is our sent request's height for4193bbe6…. SPV climbed back to the tip in about 27 s.Caveats: the run exercised this PR's rescan fallback inside #5026's function, not inside
v5.1-dev's; the fallback as it sits in this PR has unit-test coverage only (rescan_backfills_a_one_way_contact_from_our_sent_request_height). Whichever of #5026 and this PR lands second conflicts in that one function;fc9142a30don int28 is the resolution. One host-side observation, not an SDK defect: dash-wallet'sWalletTransactionMetadataProviderlater logged "DROPPED — no wallet tx and no fallback row" fore5169bfc…although the SDK store holds it; filed as dashpay/dash-wallet#1598.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
e968545/skip-botsproceeds without the ones not yet reported/self-reviewedrs-platform-wallet(packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs,packages/rs-platform-wallet/src/wallet/identity/network/contacts.rs,packages/rs-platform-wallet/src/wallet/identity/network/payments.rs) — ZocoLini or llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.