feat(platform-wallet): reconstruct sent DashPay payments from tx history - #4300
Conversation
Received DashPay payments already recover after a restore-from-seed: `reconcile_incoming_payments` walks `dashpay_receival_accounts`, which persist with their UTXOs. The sending direction had no equivalent, so a restored wallet showed "No payments with this contact yet" for every contact it had paid — the transactions were on chain and in local history, just not attributed. `reconcile_sent_payments_from_tx_history` closes that gap. It walks the wallet's persisted core transactions, matches outputs against the addresses derived from each contact's `DashpayExternalAccount`, and records one `Sent` entry per (owner, contact, txid). Local-only, no network round-trips, idempotent — an existing entry for a txid is never overwritten, so the live send path and the incoming reconcile both keep priority. It runs as a step of `dashpay_sync()` after `reconcile_incoming_payments`. Matching reads `record.transaction.output` and compares script pubkeys. It deliberately does not read `record.output_details`: records handed back by `get_core_tx_record` are rebuilt from the host's raw transaction bytes, so only `transaction`, `txid` and `context` carry real data and the details vec is always empty. Comparing scripts rather than rendered addresses also sidesteps address-encoding differences. Contacts are skipped once they have a `Sent` entry, or once swept this launch. The direction matters: the incoming reconcile runs first, so a "has any payment with this contact" test would have hidden the outgoing history of every contact we had also received from. The per-launch marker is in-memory only and is not set when a persister read or write failed, so a transient error cannot permanently strand a contact. Enumerating the wallet's transactions needs a new persistence hook, `list_wallet_core_txids`, defaulting to an empty list so existing persisters keep compiling.
|
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:
📝 WalkthroughWalkthroughChangesSent-payment history recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DashPaySync
participant DashPayView
participant WalletPersister
participant SwiftPersistence
participant PaymentStore
DashPaySync->>DashPayView: reconcile_sent_payments_from_tx_history()
DashPayView->>WalletPersister: list_wallet_core_txids()
WalletPersister->>SwiftPersistence: enumerate wallet transaction IDs
SwiftPersistence-->>WalletPersister: transaction IDs and funding flags
WalletPersister-->>DashPayView: decoded transaction records
DashPayView->>PaymentStore: write missing Sent entries
DashPayView-->>DashPaySync: result or logged failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (3)
3274-3331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the read-error and write-failure retry paths.
The tests cover the success and idempotency branches. Two branches decide whether recovery retries after a failure, and both are currently untested:
had_read_errorset by aget_core_tx_recorderror at lines 304-312, which must leave the guard unstamped so the next sweep retries.write_failed_forpopulated by arecord_dashpay_paymentfailure at lines 372-379, which must leave that contact's guard unstamped.A regression in either branch silently converts a transient failure into permanently missing sent history for the launch, with no assertion to catch it.
RecordStorePersisterneeds a failure toggle, in the shape of the existingToggleFailPersister.Do you want me to write these two tests?
🤖 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/payments.rs` around lines 3274 - 3331, Add tests for read-error and write-failure retry behavior alongside reconcile_sent_payments_from_tx_history_skips_repeat_empty_sweeps. Extend RecordStorePersister with a failure toggle matching ToggleFailPersister, then verify get_core_tx_record errors leave the reconciliation guard unstamped and cause the next sweep to retry, while record_dashpay_payment failures leave only the affected contact guard unstamped and retry that contact on the next sweep.
600-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this helper in
reconcile_sent_payments.Lines 465-466 inline the same finality test:
record.is_confirmed() || matches!(record.context, TransactionContext::InstantSend(_)). Two copies of the definition of "final" can diverge. Call the helper from both sites.♻️ Proposed refactor at lines 465-466
- let is_final = record.is_confirmed() - || matches!(record.context, TransactionContext::InstantSend(_)); - if !is_final { + if sent_payment_status_for_record(&record) != PaymentStatus::Confirmed { continue; }🤖 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/payments.rs` around lines 600 - 611, Update reconcile_sent_payments to call sent_payment_status_for_record for the finality decision instead of duplicating the record.is_confirmed() || InstantSend check, while preserving the existing confirmed and pending behavior. Remove any now-unneeded TransactionContext usage from that call site.
290-333: 🚀 Performance & Scalability | 🔵 TrivialConsider a batched record read if wallet transaction counts grow.
The scan issues one
get_core_tx_recordFFI call per wallet transaction. Each call crosses the C ABI and runs a SwiftData fetch on the host's serial queue. The per-launch guard bounds how often the full scan runs, so the current cost is one pass per launch. On a wallet with thousands of transactions that pass becomes a single long stall on the sync task, and the host queue is blocked for its duration.If transaction counts grow, a batched enumeration that returns records (not just txids) would collapse the round trips. No change is needed for the current volumes.
🤖 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/payments.rs` around lines 290 - 333, The scan in reconcile_sent_payments_from_tx_history currently does one get_core_tx_record lookup per txid, which can stall the sync task when wallet histories grow. Update the tx-history reconciliation path to prefer a batched record enumeration from the persister that returns records directly, and reuse that in place of the per-txid loop while keeping the existing txid-only fallback behavior for current volumes. Preserve the current totals, records_read, outputs_scanned, and had_read_error accounting around the new batch path so the reconciliation result stays unchanged.
🤖 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/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 2816-2821: In the txid-buffer handling around get_core_tx_record,
replace the ineffective raw.len comparison with a pre-slice overflow check that
rejects count when count multiplied by 32 cannot be represented safely,
including the isize::MAX bound required by from_raw_parts. Perform this
validation before constructing the slice, and add a SAFETY comment documenting
the pointer, length, and allocation assumptions consistent with the existing
get_core_tx_record pattern.
- Around line 630-657: Move on_list_wallet_core_txids_fn and
on_list_wallet_core_txids_free_fn to positions after
PersistenceCallbacks.release_fn, preserving the existing order and offsets of
all prior fields for older host bindings. Keep their signatures and callback
behavior unchanged.
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 383-395: Update the recovery guard in
payments::reconcile_sent_payments_from_tx_history so the stamp-and-return path
does not run on an empty txid enumeration from list_wallet_core_txids. Treat a
successful but zero-length txid result as inconclusive, like a read error, and
keep retrying instead of marking every eligible contact in eligible_contacts as
attempted. Preserve the existing write_failed_for filtering and
managed_identity_mut update flow for real recovery runs, and adjust
reconcile_sent_payments_from_tx_history_skips_repeat_empty_sweeps to reflect the
new empty-enumeration retry behavior.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 7512-7517: The txid enumeration callback must distinguish failures
from an empty transaction list. In PlatformWalletPersistenceHandler.swift at
lines 7512-7517, return a non-zero status when walletIdPtr, outTxids, or
outCount is nil, and when the fetch handler reports failure. At lines 5781-5791,
replace try? with do/catch, log fetch errors, and propagate an errored flag to
the shim instead of converting failures to an empty result.
- Around line 7512-7517: Update the guard handling required arguments in the
transaction enumeration method to return a non-zero failure code when context,
walletIdPtr, outTxids, or outCount is nil. Preserve the successful zero-result
behavior for valid arguments so Rust distinguishes host wiring failures and
retries reconciliation.
- Around line 34-36: Guard the faulted wallet relationship access inside
PlatformWalletPersistenceHandler’s involvedAccounts predicate so
`wallet.walletId` is only read after safely confirming the relationship is
available, matching the existing defensive pattern used later in the same file.
Update the `walletCoreTxids` filtering path around the
`transaction.involvedAccounts.contains` check to avoid crashing when a persisted
`PersistentAccount` row has an inconsistent or unloaded `wallet` relationship,
and preserve the current true/false matching behavior for valid rows.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 3274-3331: Add tests for read-error and write-failure retry
behavior alongside
reconcile_sent_payments_from_tx_history_skips_repeat_empty_sweeps. Extend
RecordStorePersister with a failure toggle matching ToggleFailPersister, then
verify get_core_tx_record errors leave the reconciliation guard unstamped and
cause the next sweep to retry, while record_dashpay_payment failures leave only
the affected contact guard unstamped and retry that contact on the next sweep.
- Around line 600-611: Update reconcile_sent_payments to call
sent_payment_status_for_record for the finality decision instead of duplicating
the record.is_confirmed() || InstantSend check, while preserving the existing
confirmed and pending behavior. Remove any now-unneeded TransactionContext usage
from that call site.
- Around line 290-333: The scan in reconcile_sent_payments_from_tx_history
currently does one get_core_tx_record lookup per txid, which can stall the sync
task when wallet histories grow. Update the tx-history reconciliation path to
prefer a batched record enumeration from the persister that returns records
directly, and reuse that in place of the per-txid loop while keeping the
existing txid-only fallback behavior for current volumes. Preserve the current
totals, records_read, outputs_scanned, and had_read_error accounting around the
new batch path so the reconciliation result stays unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e27ff56-9d16-4913-bcf8-cdb07e4b2efc
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/manager/dashpay_sync.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/dashpay.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/rs-platform-wallet/src/wallet/persister.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
CI: - `cargo fmt` on the long test signature. - Android's JNI vtable builds `PersistenceCallbacks` by literal, so the two new slots have to be named there. Left `None`, matching the existing `on_persist_invitations_fn` precedent: Android keeps today's behaviour rather than reporting a reconstruction it cannot perform. Review: - Append the txid callbacks after `release_fn`. The struct is a C-bound vtable whose trailing slots are documented as end-safe; inserting before them would shift the layout for hosts built against the previous header. - Replace the tautological buffer-length check with the check that matters: `count * 32` must not overflow and must fit in `isize::MAX` before `from_raw_parts` sees it. Adds the missing SAFETY note. - Do not stamp the per-launch guard when the enumeration came back empty. After a restore the recurring sweep can fire before the host has repopulated its transaction table, and a zero-txid answer is indistinguishable from "nothing to reconstruct" — stamping there ended recovery for the rest of the process, the exact symptom this pass exists to fix. `..._skips_repeat_empty_sweeps` pinned that behaviour and is replaced by two tests: one that an empty enumeration is retried, one that a conclusive scan is not repeated. - Guard the fault-loaded `account.wallet` access in `walletOwnsTransaction` the way `loadWalletList` already does; this predicate runs over every persisted transaction row, so the exposure is wider. - Report failures from the txid callback. A nil argument or a failed fetch returned 0 with an empty list, which Rust could not tell from an empty wallet.
|
Pushed 188b486 addressing both CI failures and all five review findings. CI
Review
|
…payment-reconstruction # Conflicts: # packages/rs-unified-sdk-jni/src/persistence.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4300 +/- ##
============================================
- Coverage 87.78% 87.61% -0.18%
============================================
Files 2677 2704 +27
Lines 342371 345182 +2811
============================================
+ Hits 300551 302416 +1865
- Misses 41820 42766 +946
🚀 New features to boost your workflow:
|
`clippy::type_complexity` is denied workspace-wide and the inline tuple annotation tripped it. Extracting `OwnerContact` and `ContactScriptIndex` also gives the script-pubkey keying an obvious place to be explained.
|
🔍 Review in progress — actively reviewing now (commit c9bb8d5) |
`ffi_capability_projection_has_stable_v1_layout_values` pins the callback vtable's size and asserts the last-appended field is terminal. Both move when a slot is added, exactly as they did for invitations and then for `release_fn`. The two txid callbacks sit after `release_fn`, so no previously-defined slot changes offset — which is the property that actually matters for hosts built against an older header, and the reason growth is only ever safe at the end.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The implementation adds useful sent-payment recovery coverage, but verification confirms five in-scope blockers. The new FFI slots break the existing vtable ABI, while the reconstruction can fabricate sent payments or permanently omit records after partial reads, partial writes, and restores involving more than 20 payment addresses.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 5 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:683-714: Appending callbacks does not preserve the FFI vtable ABI
Appending fields preserves existing offsets but not binary compatibility for a struct passed through `*const PersistenceCallbacks`. `platform_wallet_manager_create_impl` copies the value with `std::ptr::read(persistence)`, so the new library reads the full 24-slot or 40-slot struct. A host compiled against the previous 22-slot or 38-slot definition allocated only the old extent, making manager creation read beyond that object before it can determine that the new callbacks are unset. Adjacent memory can then be interpreted as callback pointers and invoked. Preserve the old struct size and expose these hooks through a size/version-negotiated v2 structure, a separate extension structure, or a new creation entry point.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:258-268: One successful write prevents retrying another failed payment
This contact-level guard treats one existing `Sent` entry as proof that the contact's entire outgoing history is complete. If transaction A is persisted successfully and transaction B fails, `write_failed_for` correctly avoids setting the completion marker, but the next sweep skips the contact because A now exists. A restart also restores A and continues skipping B, so the failed entry is permanently stranded despite the retry log. Eligibility must use a genuine completion marker or retry state; the per-txid `contains_key` check later in the loop already provides idempotence for successfully recorded entries.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:306-309: Unavailable listed transactions are treated as a completed scan
A txid returned by `list_wallet_core_txids` followed by `Ok(None)` is not a conclusive absence. The FFI implementation maps callback failures, missing or empty transaction bytes, decode failures, unknown contexts, and every InstantSend record to `Ok(None)`; Swift can also enumerate placeholder rows whose bytes are populated later. This arm leaves `had_read_error` false, so a nonempty enumeration stamps every eligible contact as completed and prevents later sweeps from reconsidering the unavailable record. This also exposes the broader contract mismatch: `get_core_tx_record` documents that `transaction` may be a placeholder, but this reconstruction requires a complete decoded output list. Treat unavailable listed records as an incomplete scan or introduce a dedicated history API that guarantees a decoded transaction.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:270-277: Freshly rebuilt accounts only search the first 20 payment addresses
The candidate index contains only scripts currently materialized in the external account pool. `ManagedCoreFundsAccount::from_account` initializes a `DashpayExternalAccount` with 20 addresses, and live sends derive index 20 and above only after prior addresses have been marked used. After a seed restore that rebuilds the contact account without its historical pool state, persisted or rescanned transaction history can contain payments to later indices while the candidate set contains only indices 0 through 19. The sweep then records the early matches and marks the contact complete, permanently omitting later payments. Derive a sufficient historical range from the contact xpub or restore/reconstruct the pool's used range before setting the completion marker.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5805-5807: Incoming and third-party transactions can be recorded as sent payments
`walletOwnsTransaction` includes transactions through wallet outputs and `involvedAccounts`, not only transactions spending wallet-owned inputs. A `DashpayExternalAccount` is watch-only but is matched whenever an output pays one of the contact's derived addresses, so an unrelated third-party transaction paying that contact is persisted as wallet-involved. An incoming transaction that pays both this wallet and the contact is also included. The Rust sweep then assumes every matching contact output was funded by this wallet and records it as `Sent`, producing false payment history. Enumeration or the returned record must carry reliable per-wallet outgoing ownership, and reconstruction must reject transactions without a wallet-owned input.
|
Notes from the Android side — we hit the same class of problem, so two things that may be useful here. No action needed from us: the 1. Blocker: We ran into exactly this. Matching a transaction because it pays a contact's watch-only derived address is true for any payer, so an incoming payment (or a third party paying that same contact) matches too. What worked for us was to stop asking "does this tx touch the contact's address" and instead compute a signed net per party, explicitly excluding the contact's watch-only external account from both sides of the sum. On Android that's the DashPay external account (type 13): its TXOs are deliberately excluded from both the sent and received sums, with an address-level fallback for rows where the account id is NULL. Direction then falls out of the sign of the net rather than out of address membership, so an incoming payment can't be misfiled as Reference, if it helps: 2. ABI caveat on the vtable extension
Android is immune — One design note, offered only as a data point: because Android recomputes attribution from the transaction store on every read (checking both the sending and receiving friend key chains), it has no persisted-cache asymmetry to lose on restore — the cache is derived, not a source of truth. That's a different trade-off from the |
On the vtable ABI@thepastaclaw @bfoss765 — you both flagged this and the facts aren't in dispute. Two things worth putting on the record. This is a property of the struct, not of this change. The same growth happened twice already — Who is exposed today. Android builds Recommendation: accept the growth here, consistent with the two prior appends, and track versioned negotiation separately so it covers every future slot at once rather than just these two. A size/version-negotiated struct or a new creation entry point changes the contract for every host, which is its own review, not a rider on a DashPay payment-history fix. If you'd rather hold this PR until that structure exists, that's a fine call too — but then the enumeration callback should be designed together with the per-transaction ownership data the sweep needs (the Either way I'd like it to be a decision rather than an omission, so I'm leaving the call to maintainers. Meanwhile the two "concluded more than the evidence supports" findings are fixed in 82dd151, and I'm working the two correctness blockers that don't depend on this: false |
|
Thanks for making the tradeoff explicit. I agree the underlying extensibility flaw predates this PR, but this change still creates a new incompatible size boundary: a host built against the current header can be over-read when paired with the new library. The prior appends are precedent for the pattern, not evidence that another append is safe. From my review perspective this remains blocking unless a Platform maintainer explicitly confirms that lockstep XCFramework/header/app builds are the supported ABI contract and accepts no cross-version compatibility, or the callback surface gains size/version negotiation (ideally together with the ownership data now being reworked). The PR head is still |
…ial scans Addresses the four reconstruction blockers from review: - Contact eligibility no longer treats one existing Sent entry as proof of completion. A write that fails after a sibling write succeeded is retried on the next sweep; per-txid dedup already makes re-sweeping recorded entries a no-op. - A listed txid whose record resolves to Ok(None) now marks the scan incomplete. The FFI collapses backend failures, missing/undecodable tx bytes and pending InstantSend rows into a miss, so a miss on a txid the host itself enumerated means "not available yet" — the completion guard stays unstamped and the sweep retries. - Candidate scripts are derived from the contact xpub over the historical range (matched index + gap limit, iterated to a fixed point) on a pool clone, instead of matching only the addresses the restored pool materialized. Payments past the initial gap window are now found after restore-from-seed. - The txid enumeration carries a per-txid spends-wallet-input flag computed by the host from persisted TXO rows (inputs tracked under a watch-only DashPay external account do not count). Transactions the wallet did not fund — a third party paying the watched contact address, or incoming payments — are skipped without a record read and can no longer fabricate Sent history. The get_core_tx_record field contract now requires the real decoded transaction (or None); reconcile_sent_payments reuses the shared finality helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed e3b193c addressing the review blockers. Partial writes stranded a sibling payment (payments.rs:258-268) — Fixed. Eligibility no longer consults the payment map at all; it is gated only by the per-launch
Only the first 20 addresses searched after restore (payments.rs:270-277) — Fixed. The sweep now clones the contact pool, and extends it from the contact xpub ( Incoming/third-party transactions recorded as sent (PlatformWalletPersistenceHandler.swift:5805-5807) — Fixed. The (new-in-this-PR) enumeration callback now returns a per-txid flags buffer alongside the txids; bit Appending callbacks breaks the FFI vtable ABI (persistence.rs:683-714) — Not changed, deliberately. There is no supported deployment where a host compiled against the old 22/38-slot struct loads this library: every consumer of Also applied CodeRabbit's suggestions: the read-error/write-failure retry paths are now covered by the two tests above, and Verified: |
There was a problem hiding this comment.
Actionable comments posted: 2
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-ffi/src/persistence.rs (1)
2829-2841: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefine buffer ownership when the callback returns non-zero.
_txid_guardis constructed before therccheck, so Rust invokeson_list_wallet_core_txids_free_fneven when the callback reported failure. The sibling contracts differ:on_load_wallet_list_fndocuments "on failure Rust does not call the free callback", andloadbuilds itsLoadGuardonly after therccheck (lines 2099-2110). The new doc at lines 719-722 says "exactly once per successful hit", which contradicts the code.A host that writes both buffers and then returns non-zero, and frees them itself, would double-free. The in-repo Swift shim leaves
outTxidsnil on every failure path, so it is unaffected today.Pick one semantic and make the code and the doc agree. Either move the guard construction after the
rccheck, or state in the callback doc that Rust frees whatever pointers the host wrote, including on failure.Also note the narrower gap in the guard itself: it keys on
self.txids.is_null(), so a host that sets onlyflags_ptrleaks the flags buffer.🛡️ Proposed fix: construct the guard after the status check
- let _txid_guard = TxidBytesGuard { - txids: txids_ptr, - flags: flags_ptr, - count, - free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, - ctx: self.callbacks.context, - }; - if rc != 0 { return Err(PersistenceError::backend(format!( "on_list_wallet_core_txids_fn returned non-zero status {rc}" ))); } + let _txid_guard = TxidBytesGuard { + txids: txids_ptr, + flags: flags_ptr, + count, + free_fn: self.callbacks.on_list_wallet_core_txids_free_fn, + ctx: self.callbacks.context, + };🤖 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-ffi/src/persistence.rs` around lines 2829 - 2841, Move the TxidBytesGuard construction in the callback handling flow to after the rc != 0 error check, matching the ownership contract that Rust does not invoke the free callback on failure. Update the associated callback documentation to state that buffers are freed exactly once only for successful results, and adjust TxidBytesGuard cleanup to release flags_ptr independently when txids_ptr is null so a flags-only allocation cannot leak.
🤖 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/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 413-442: Set incomplete_scan before the range-walk loop for each
window whose key_source cannot derive, including empty pools and pools with no
observed matches. Preserve the existing warning and once-per-window behavior,
then remove the now-redundant !window.key_source.can_derive() branch inside the
loop while leaving derivable pools’ range-walk behavior unchanged.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 5805-5813: Update walletFundedTransaction to resolve each TXO’s
wallet ID through its linked account wallet when txo.walletId is empty, matching
the fallback used by loadWalletList. Compare the resolved ID with walletId while
preserving the existing external-account exclusion and current behavior for
populated walletId values.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 2829-2841: Move the TxidBytesGuard construction in the callback
handling flow to after the rc != 0 error check, matching the ownership contract
that Rust does not invoke the free callback on failure. Update the associated
callback documentation to state that buffers are freed exactly once only for
successful results, and adjust TxidBytesGuard cleanup to release flags_ptr
independently when txids_ptr is null so a flags-only allocation cannot leak.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bb94c2a-f618-457b-bfaa-99fb7866fbe4
📒 Files selected for processing (6)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-platform-wallet/src/wallet/persister.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
|
Note for reviewers: the "Kotlin SDK build + tests" failure on this run is pre-existing v4.2-dev breakage, not this PR — #4015 renamed a Swift test without updating the SDK parity manifest, so the check fails on every PR against the current base. Fix is up in #4304 (CI green); once it merges, re-running the Kotlin job here will clear it. Everything else on this PR's latest commit is green. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: none remain actionable—four are fixed, and the FFI ABI concern is intentionally deferred under the maintainer-confirmed lockstep deployment contract. Latest delta: two blocking recovery gaps remain around partially populated transaction history and legacy TXO wallet attribution, plus one FFI ownership suggestion; the missing-xpub report is refuted by the supported account-pairing invariant, and all 11 focused Rust reconstruction tests pass but do not cover the retained edge cases.
Source: reviewers gpt-5.6-sol (general, rust-quality, ffi-engineer); verifier gpt-5.6-sol; openclaw-agent coordinator is orchestration-only.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 2 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/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:531-553: A partially populated transaction table is treated as complete
The completion condition distinguishes an empty enumeration from a completed scan, but any nonempty prefix is still treated as conclusive. The code and its regression test explicitly acknowledge that `dashpay_sync()` can run while the host is repopulating transaction history after restore. If one historical row exists when this snapshot is fetched but later rows have not arrived yet, every listed record can be read successfully and `txid_count > 0` causes the contact to be marked attempted. Rows added after that snapshot are then ignored for the rest of the process because the in-memory guard suppresses future sweeps. Completion needs an explicit host/core-history completeness signal, a transaction-history generation or high-water mark, or another mechanism that cannot certify a snapshot while restoration is still adding rows.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5805-5812: Legacy TXOs are classified as unfunded after restore
`PersistentTxo.walletId` defaults to empty for rows migrated from before the denormalized field existed, and `loadWalletList` already resolves those rows through `txo.account.wallet.walletId`. `walletFundedTransaction` instead compares only the raw denormalized field, so a real spend of an untouched legacy TXO is reported as not wallet-funded. Rust skips that transaction and can still stamp the contact as swept, leaving its sent payment absent for the process lifetime. The earlier `walletOwnsTransaction` helper has the same raw-field-only checks for outputs, inputs, and pending inputs, so a transaction composed entirely of legacy TXOs can be filtered out before the funding classifier runs. Use one shared wallet-resolution helper in both ownership selection and funding classification: prefer a populated `txo.walletId`, otherwise resolve through the linked account wallet, while continuing to exclude DashPay external account type 13 from wallet-funded inputs.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2822-2841: Align txid-buffer cleanup with callback failure ownership
The callback documentation says Rust invokes the paired free callback exactly once for a successful result, but `TxidBytesGuard` is installed before `rc` is checked, so a non-null buffer returned with a failure status is also freed. That conflicts with a host following the documented success-only ownership transfer and can double-free if the host retains responsibility for failure-path allocations. The guard also runs only when `txids` is non-null, leaking a flags-only allocation from a malformed successful callback. The in-repository Swift callback returns errors before allocating, so this is not a current supported-host crash, but the new FFI contract should be internally consistent: check `rc` before installing the guard and invoke the free callback when either successful output pointer is non-null.
Three findings from the review of e3b193c. **A partially populated transaction table was treated as complete.** Distinguishing an empty enumeration from a completed one was not enough: any non-empty prefix still certified the snapshot. The sweep runs on a timer, so it can read the table while a DashPay rescan is still delivering rows into it — one early row makes `txid_count > 0`, the per-launch guard gets stamped, and every row arriving afterwards is ignored for the rest of the process. `reconcile_dashpay_rescan` now records the tip it rewound from, and the sweep refuses to certify a contact while `synced_height` is still below that mark. In-memory like `rescan_triggered`, and self-healing for the same reason: a relaunch restores `synced_height` at its high-water and re-arms the mark with the re-triggered backfill. **Legacy TXOs were classified as unfunded.** `PersistentTxo.walletId` is empty on rows written before the denormalized field existed, and `loadWalletList` already resolves those through the owning account's wallet. Both `walletOwnsTransaction` and `walletFundedTransaction` compared the raw field, so a real spend of an untouched legacy TXO read as "not ours" — the transaction was skipped and the contact could still be stamped. Both now share one resolver that prefers the populated field and falls back to the account's wallet, while still excluding the watch-only DashPay external account from funded inputs. **Txid-buffer ownership contradicted its own contract.** The free callback is documented as transferring ownership on success only, but the guard was installed before the status check, so a buffer returned alongside a failure was freed too — a double free for any host that cleans up its own failed allocation. The guard now goes up after the check and fires when either output pointer is non-null, so a flags-only allocation from a malformed success can't leak.
|
Pushed 54758c2 addressing the three findings on A partially populated transaction table was treated as complete — fixed. Distinguishing empty from non-empty enumeration wasn't enough: any non-empty prefix still certified the snapshot, and the sweep runs on a timer, so one early row is sufficient to stamp the guard while the backfill is still delivering the rest.
Legacy TXOs classified as unfunded — fixed. Added one shared Txid-buffer cleanup vs failure ownership — fixed, and thanks for catching it: that guard placement was mine. It now goes up after the Missing-xpub branch — agreed with your refutation; left as is. Verified: Note the Kotlin job here is still the pre-existing |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (1)
30-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the repository indentation width.
These changed Swift lines use 4-space indentation. The repository EditorConfig requires 2-space indentation. Reindent the changed lines.
As per coding guidelines,
**/*requires 2-space indentation.Also applies to: 1308-1309, 5832-5837
🤖 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/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift` around lines 30 - 74, Reindent the changed Swift code to use the repository’s required 2-space indentation instead of 4 spaces. Apply this consistently to the shown resolvedWalletId and walletOwnsTransaction code and the additional changed sections around lines 1308-1309 and 5832-5837, without altering behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 30-74: Reindent the changed Swift code to use the repository’s
required 2-space indentation instead of 4 spaces. Apply this consistently to the
shown resolvedWalletId and walletOwnsTransaction code and the additional changed
sections around lines 1308-1309 and 5832-5837, without altering behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13d7dc8b-cc3c-40e4-a0bd-76f82827e921
📒 Files selected for processing (5)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/dashpay.rspackages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs
- packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/dashpay.rs
- packages/rs-platform-wallet-ffi/src/persistence.rs
- packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
One carried-forward blocking issue remains: sent-payment reconstruction can certify a partial transaction snapshot before the rescan coordinator arms its completeness target. The latest delta correctly fixes legacy TXO attribution and FFI buffer ownership, and no separate latest-delta defect survives verification. The FFI ownership test request is useful hardening but does not identify a current production defect.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 1 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/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:467-489: A partially populated transaction table is treated as complete
`sync_wallet_dashpay` invokes `reconcile_sent_payments_from_tx_history()` before `reconcile_dashpay_rescan()`, which is the only production path that writes `rescan_backfill_target`. On the first pass after contact discovery or restoration, the target is therefore still `None`; any non-empty current transaction prefix can pass the completion condition at `payments.rs:562-580` and stamp `sent_payment_reconcile_attempted`. The subsequent rescan rewinds the wallet and arms the target too late, because later passes exclude that contact at `payments.rs:284-290` before consulting the target. The `synced_height == 0` early return at `payments.rs:111-115` leaves the same gap during an initial genesis scan: once the first transaction appears, a partial scan can be certified without any target. The regression test at `payments.rs:3555-3646` manually preloads `Some(1000)` and therefore does not exercise the production coordinator ordering. Establish a conclusive settled-history signal before reconstruction, including for an initial scan, and add a test that runs the actual coordinator sequence.
There was a problem hiding this comment.
Preliminary review — Codex only
The latest push only merges unrelated v4.2-dev message-signing changes and does not modify the sent-payment reconstruction flow. One carried-forward blocker remains: reconstruction can certify a partial transaction snapshot before transaction-history completeness is established, permanently suppressing payments restored later in the process. No separate latest-delta defect was found.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 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/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:467-489: A partially populated transaction table is treated as complete
`sync_wallet_dashpay` calls `reconcile_sent_payments_from_tx_history()` before `reconcile_dashpay_rescan()`, the only production writer of `rescan_backfill_target`. The persistence boundary returns only the transaction rows currently available, without a settled-history signal, so the first post-restore sweep can see a non-empty prefix while the target is still `None` and stamp `sent_payment_reconcile_attempted` at `payments.rs:562-580`. The subsequent rescan arms the target too late because later sweeps exclude the stamped contact at `payments.rs:284-290`. An initial or ongoing forward scan has the same gap when no rewind occurs, including the `synced_height == 0` return at `payments.rs:111-115`. The regression test at `payments.rs:3555-3646` manually installs `Some(1000)` and therefore verifies only an already-armed gate, not the production coordinator sequence. Establish a reliable settled-history or high-water signal before stamping completion, including for no-rewind forward scans, and test the actual coordinator ordering.
…payment-reconstruction
There was a problem hiding this comment.
Preliminary review — Codex only
The latest push only merges newer v4.2-dev changes and leaves the sent-payment reconstruction files unchanged. At the current head, reconstruction can still permanently certify a partial transaction snapshot, and its derivation walk can miss a valid send after a full gap of off-chain consumed addresses. Both issues undermine the feature's restore-from-seed recovery goal and require changes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 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/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:467-489: A partially populated transaction table is treated as complete
`sync_wallet_dashpay` runs `reconcile_sent_payments_from_tx_history()` before `reconcile_dashpay_rescan()`, although the latter is the only production path that arms `rescan_backfill_target`. A first post-restore sweep can therefore see a non-empty prefix of the persisted transaction table while the target is still `None` and insert the contact into `sent_payment_reconcile_attempted` at `payments.rs:562-580`; the following rescan arms the target too late because later sweeps exclude that contact at `payments.rs:284-290`. The same completeness gap remains during an initial or forward-only scan: `reconcile_dashpay_rescan` returns at `payments.rs:111-115` when `synced_height == 0` and does not arm a target when no rewind is needed, even though transaction rows may still be arriving. The Swift persistence callback exposes only the rows currently visible, not whether history is settled, and the regression test at `payments.rs:3555-3646` manually pre-arms `Some(1000)` instead of exercising this coordinator ordering. Require an explicit settled-history/high-water signal before stamping the completion marker, including initial and forward-only scans, and test the production sequence.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:425-439: The range walk cannot cross a full unused address gap
The derivation loop extends the pool only after an observed transaction matches an address in the currently materialized window; when there is no initial match, line 432 stops immediately. `send_payment` does not preserve the assumed on-chain gap invariant: it marks the selected contact address used at lines 1033-1048 before `build_signed`, and a failed build returns without rolling that in-memory consumption back. After `gap_limit` such failures, a later successful retry can pay index `gap_limit`; on seed restoration the failed attempts are unknowable, so the recreated pool contains only indices `0..gap_limit-1` and no transaction bridges the walk to the successful address. Reconstruction then records nothing and can stamp `sent_payment_reconcile_attempted` at lines 562-580, permanently suppressing recovery for that contact. The existing past-gap test includes an on-chain match at the initial frontier and therefore does not cover this valid hole. Roll back/enforce address consumption in the send path, or reconstruct from a reliable derivation high-water or bounded search that does not require an earlier on-chain match.
…flag Two blockers from the review of de9ea9b. **A partial transaction snapshot could be certified as complete.** The previous guard was a boolean armed by `reconcile_dashpay_rescan`, but `sync_wallet_dashpay` runs the reconstruction sweep *before* that reconcile — so the first post-restore pass saw no target, took whatever prefix of the transaction table was visible as the whole of history, and stamped the contact. The same gap remained on an initial or forward-only scan, where `reconcile_dashpay_rescan` arms nothing at all because there is no rewind, yet rows keep arriving with each new block. The marker is now the `synced_height` a sweep certified against, not a flag. A pass is conclusive for the table as it stood at that height and says nothing about later blocks, so a contact becomes eligible again the moment the height advances — which is exactly when new rows can appear. That needs no "history is settled" signal (nothing in the persistence callbacks reports one) and removes the ordering dependency entirely: a rewind lowers the height, so the stamp stops matching whichever reconcile ran first. In steady state the height stops moving and so does the sweep. `rescan_backfill_target` existed only to paper over the ordering and is deleted. **The derivation walk could not cross a full unused gap.** It extended the pool only past an address it had already seen paid, so a stretch of unused indices stopped it. Those stretches are reachable: `send_payment` marks the chosen contact address used before `build_signed` and does not roll that back when the build fails, so after enough failures a real payment sits past a hole no on-chain output bridges — and a restored pool stops short of it. The seed window is now five gap limits wide and is derived whether or not the pool already materialized addresses, so the walk no longer depends on an earlier match. Both regression tests were verified to fail without their fix.
…ruction' into fix/dashpay-sent-payment-reconstruction
|
Pushed e156123 for both blockers. A partial transaction snapshot could be certified as complete — fixed, and you were right that the ordering was the giveaway. Rather than look for a settled-history signal that the persistence callbacks do not provide, the guard is now the
The range walk could not cross a full unused gap — fixed. Your reading of the cause is right: Rolling back address consumption in Regression tests, both verified to fail without their fix:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest push fixes the exact one-gap reconstruction failure, but the replacement five-gap bound still lacks an enforced invariant. The scan-height guard remains unsafe under the production rewind and asynchronous persistence ordering, and a second height read can certify a snapshot against a height it never inspected. The new guard also turns restore-time reconstruction into a recurring full-history scan after each block.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Source: logical Codex/Sol reviewers and verifier used gpt-5.6-sol; coordinator cliproxy/gpt-5.6-sol was orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/dashpay_sync.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/dashpay_sync.rs:467-489: A partially populated transaction table is treated as complete
The height stamp is not a completeness signal for the persisted transaction table. This coordinator runs reconstruction at tip H before `reconcile_dashpay_rescan` lowers the wallet to a contact's funding floor F. Once reconstruction stamps H, the predicate at `payments.rs:292-297` excludes the contact throughout the entire backfill because `H >= synced_height` remains true from F through H, so rows delivered by that backfill are not examined unless the chain later reaches H+1. The same failure exists without a rewind: the wallet-event adapter persists transaction rows asynchronously, and `core_bridge.rs:241-276` explicitly allows in-memory block processing to run ahead of that adapter while preserving only event order. A sweep can therefore observe in-memory height H with only a prefix of the corresponding rows durable, stamp H, and ignore rows committed later at the same height; mempool rows can also arrive without any height advance. Use a settled durable-history watermark, synchronization barrier, or retry cursor tied to persistence rather than the independently advancing in-memory scan height.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:600-612: A concurrent scan advance certifies an unscanned height
Candidate selection captures `synced_height` at line 281, but that value is discarded with the read guard. After enumerating and reading the persisted transaction snapshot, this code reacquires the manager lock and reads the height again. If SPV advances from H to H+1 while the persistence reads are running, the pass inspected an H-era snapshot but stamps H+1. The next pass at H+1 then skips the contact, so a transaction associated with the newly reached height can remain missing until another block arrives. Carry the initially captured height through the method and stamp exactly that value, or reject and retry the pass when the height changes.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:431-440: The five-gap seed only moves the unrecoverable gap
`HISTORICAL_SEED_GAP_MULTIPLE = 5` fixes the previous one-gap reproducer, but no invariant limits failed sends to fewer than five gap windows. `send_payment` marks an address used at lines 1065-1084 before the fallible `build_signed` call at lines 1148-1153, and an error neither rolls the consumption back nor enforces a retry bound. With a gap limit of 20, 100 failed builds consume indices 0 through 99 and the next successful payment uses index 100. Seed restoration derives only indices 0 through 99 here; with no earlier on-chain match, the loop exits at lines 453-461 and never reaches the actual payment. Recovery needs rollback of pre-broadcast address consumption, a persisted derivation high-water, or a search bound enforced by the send API rather than a heuristic constant.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:289-298: Every new block triggers another full-history scan
Every increase in `synced_height` makes every external contact eligible again. On the next recurring DashPay sync, reconstruction enumerates the complete wallet transaction table, reads every wallet-funded record, and derives at least five gap windows for each contact even when restore-time recovery completed long ago. This changes a one-time recovery operation into an O(transaction history + contacts × seed window) scan after each new block for the lifetime of the wallet. A durable or session-level recovery state combined with an incremental transaction cursor would preserve retries for genuinely late rows without repeatedly reading all historical records.
…e itself Replaces the scan-height guard with a digest of the enumerated (txid, wallet-funded) rows, and stops failed sends from consuming payment addresses. A chain height was never a completeness signal for the persisted transaction table: the sweep could stamp tip H before the rescan coordinator rewound below it (excluding the whole backfill until H+1), the wallet-event adapter commits rows asynchronously behind the in-memory height, mempool rows arrive with no height advance at all, and a second height read after the persistence I/O could certify a height the pass never inspected. Stamping the digest of exactly the enumeration the pass scanned closes all four: any row that lands afterwards changes the next enumeration's digest and re-opens the affected contacts, whichever order the reconciles ran in, and there is no second read to race. It also ends the scan-per-block regression — an unchanged table costs later sweeps one txid enumeration and zero record reads, and a block with no new wallet rows re-triggers nothing. The five-gap seed window guarded against failed sends marching the next derivation index past any recoverable range, but nothing bounded the march. Bound it at the source instead: a failed build_signed now returns the consumed address to the pool (state -> Available) before anything was persisted or broadcast, restoring the BIP44 invariant that used indices chain within the gap limit. The seed window stays as tolerance for histories written before this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed b55002f for the three blockers on e156123, plus the scan-per-block suggestion. A partially populated table treated as complete (dashpay_sync.rs:467-489) — you were right that the height stamp was the wrong invariant, in both directions it can fail. The guard is no longer a height (or any proxy for host-side completeness, which no callback reports): the sweep now stamps a digest of exactly the A concurrent scan advance certifies an unscanned height (payments.rs:600-612) — resolved by the same change, structurally: there is no second read to race. The stamped value is computed from the very buffer the pass enumerated before doing any I/O, so the stamp can never claim more than the pass inspected. If the table changes mid-pass, the next enumeration's digest differs and the contact re-opens. The five-gap seed only moves the unrecoverable gap (payments.rs:431-440) — agreed that no constant fixes an unbounded march; the bound now lives at the source. A failed Every new block triggers another full-history scan (suggestion, payments.rs:289-298) — fixed by the digest as well, and this was the deciding argument for it over patching the height: a block with no new wallet rows leaves the enumeration unchanged, so nothing re-runs. Steady-state cost per recurring sync pass is one txid enumeration (no record reads, no derivations), paid only while DashPay contacts exist; Verified: |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Digest-based certification fixes the prior partial-table, concurrent-height, and per-block reconstruction issues. Two blocking defects remain: definitive broadcast rejections can still create an address gap beyond the fixed recovery window, and the enlarged C callback table is read past the end when used by a host compiled against the previous ABI. The recurring sync path also performs avoidable work on both Swift and persistence backends that do not support transaction enumeration.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Source: logical Codex/Sol reviewers and verifier used gpt-5.6-sol; coordinator cliproxy/gpt-5.6-sol was orchestration-only.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
1 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-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:685-739: Appending callbacks breaks the existing C vtable ABI
These fields grow the established `#[repr(C)] PersistenceCallbacks` object from 22 to 24 pointer-sized fields without supplying a size or version to the unchanged manager-create API. Preserving earlier field offsets does not enlarge an object allocated by a host compiled against the previous header. Such a host still passes a 22-field object, while `platform_wallet_manager_create_impl` copies the current full struct with `std::ptr::read(persistence)` at `manager.rs:138-142`, reading two words beyond the caller's allocation. That is undefined behavior, and adjacent memory can be interpreted as the new callback pointers. Move transaction enumeration into a size-negotiated or versioned extension API rather than enlarging the by-value callback table while claiming compatibility with older hosts.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:298-308: Every sync still loads and hashes the entire transaction table
The digest comparison can happen only after `list_wallet_core_txids`, so every 15-second DashPay sync enumerates the transaction table even when nothing changed. The Swift implementation performs an unfiltered `FetchDescriptor<PersistentTransaction>`, loads rows across all wallets, traverses ownership relationships to filter them in memory, allocates two FFI buffers, and then Rust copies, sorts, and hashes the result. This work runs synchronously on the serial persistence queue for each wallet with an external contact, so its cost grows with the host's complete transaction history and can delay unrelated persistence operations. Expose a cheap wallet-scoped generation token or digest before materializing the full table, and enumerate rows only when that token changes.
In `packages/rs-platform-wallet/src/changeset/traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/traits.rs:366-389: The default enumeration fallback retries expensive recovery forever
The empty vector represents both a supported backend with no transactions and a backend that does not implement enumeration. Reconstruction intentionally refuses to certify an empty enumeration, so an unsupported backend repeatedly clones each contact pool, derives the five-gap historical seed, scans nothing, and emits candidate/scan logs on every recurring sync. Android takes this path because its FFI vtable explicitly leaves both enumeration callbacks unset. Represent unsupported enumeration separately with an `Option`, typed error, or capability bit so the caller can skip recovery without treating the backend as a perpetually incomplete empty table.
Two review findings on b55002f: - A definitively rejected broadcast (BroadcastError::Rejected — the transaction provably never reached the network) now returns the consumed contact payment address to the pool AND persists the revert. Unlike the build-failure rollback, the used flip was already durable by broadcast time, so an in-memory revert alone would be undone at the next relaunch; left consumed, every definitive rejection widens the off-chain gap in the used range by one with no bound — the same unrecoverable-gap class as an unrolled-back build failure. An indeterminate failure (MaybeSent) keeps the consumption: the transaction may have propagated. The un-mark logic is shared between both paths (return_contact_payment_address_to_pool). - list_wallet_core_txids now returns Option, separating "backend does not index wallet-scoped tx history" (None — the default, and the FFI answer when the enumeration callbacks are unset, i.e. Android) from "supported, no rows yet" (Some(vec![])). The sweep skips outright on None instead of treating the backend as a perpetually incomplete empty table and re-deriving per-contact candidate windows on every recurring sync pass forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Continuing in 4326 |
5 participants
Issue being fixed or feature implemented
Received DashPay payments already recover after a restore-from-seed:
reconcile_incoming_paymentswalksdashpay_receival_accounts, which persist along with their UTXOs. The sending direction had no equivalent.The result is that a restored wallet shows "No payments with this contact yet" for every contact it had paid. The transactions are on chain and in local history — they simply are not attributed to the contact. The counterparty's wallet, meanwhile, still lists them all, so the data is provably recoverable.
The asymmetry is called out in this file's own comment (
payments.rs:207-208).What was done?
reconcile_sent_payments_from_tx_historywalks the wallet's persisted core transactions, matches outputs against the addresses derived from each contact'sDashpayExternalAccount, and records oneSententry per(owner, contact, txid). It runs as a step ofdashpay_sync(), afterreconcile_incoming_paymentsso the entry with real ground truth wins. Local-only, no network round-trips, idempotent — an existing entry for a txid is never overwritten.Three details worth a reviewer's attention:
Matching reads
record.transaction.output, notrecord.output_details. Records handed back byget_core_tx_recordare rebuilt from the host's raw transaction bytes, so onlytransaction,txidandcontextcarry real data — the details vec is always empty (rs-platform-wallet-ffi/src/persistence.rs). A version of this that readoutput_detailspassed every unit test and matched nothing on device. Comparing script pubkeys rather than rendered addresses also sidesteps address-encoding differences.The skip-guard is direction-aware. A contact is skipped once it has a
Sententry, or once swept this launch. Testing "any payment with this contact" would hide the outgoing history of every contact we had also received from, because the incoming reconcile runs first and fills the map.The per-launch marker is in-memory only and is not set when a persister read or write failed, so a transient error cannot permanently strand a contact. A relaunch retries once for still-empty contacts, which is far cheaper than re-scanning persisted history every 15s forever.
Enumerating the wallet's transactions needs a new persistence hook,
list_wallet_core_txids, defaulting to an empty list so existing persisters keep compiling. The Swift implementation packs only well-formed 32-byte txids and reports how many it packed — the earlier shape could hand Rust an uninitialized slot as a txid.How Has This Been Tested?
cargo test -p platform-wallet— 519 passed, 0 failed.Two of those are regression tests for the defects above, and both were verified to fail without their fix:
reconcile_sent_payments_from_tx_history_matches_without_output_details— builds the record shape the FFI actually returns (output_detailscleared).reconcile_sent_payments_from_tx_history_reconstructs_for_contact_with_received_history— a contact with prior incoming history still gets its sends reconstructed.End to end on an iOS device (testnet), wallet restored from seed with two established contacts:
Six
Sententries recorded, amounts and dates matching the counterparty wallet, and they survive relaunch. Steady-state passes afterwards report no eligible contacts, i.e. no repeated full scans. The receiving side was checked in the same run and is unaffected.Needs the companion app change (dashwallet-ios
fix/bug-28-dashpay-payment-history) to drain the deferred contact-crypto queue — without it a restored wallet has no external accounts for this pass to match against.Breaking Changes
None.
list_wallet_core_txidsis a defaulted trait method.Checklist:
Summary by CodeRabbit
New Features
Bug Fixes