fix(key-wallet): never offer an outpoint the builder already holds - #931
Merged
Conversation
`add_funding` appends every unreserved UTXO of a funding account to the candidate set. When the builder was already seeded with one of those UTXOs — `add_inputs` with a caller-chosen outpoint, or an earlier `add_funding` of an overlapping account — the same outpoint became TWO candidates. Coin selection does not deduplicate by outpoint: `SelectionStrategy::All` takes every candidate, so the duplicate was guaranteed to reach the transaction, and the ordinary strategies could pick both copies and double-count them toward the target. Either way the result is a transaction spending one prevout twice, which Core rejects. This is specific to additive funding. The `set_funding` it replaced assigned `self.inputs`, so a pre-seeded outpoint was silently dropped rather than duplicated — an invalid transaction is the strictly worse failure, so fix it where the candidates are built. The account still records every unreserved outpoint it owns that is in the candidate pool, including a pre-seeded one, so that if selection picks it the owning account reserves it. Recording only the UTXOs this call appended would leave a pre-seeded input unreserved and free for a concurrent build to select. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesFunding outpoint deduplication
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #931 +/- ##
==========================================
+ Coverage 75.28% 75.51% +0.23%
==========================================
Files 328 328
Lines 77982 78017 +35
==========================================
+ Hits 58710 58918 +208
+ Misses 19272 19099 -173
|
QuantumExplorer
added a commit
to dashpay/platform
that referenced
this pull request
Aug 7, 2026
…and guard duplicate prevouts Review follow-ups on the pooled send path. Contributors vs offered accounts. `funding_accounts` was pushed when an account's UTXOs were OFFERED to selection, so a transaction funded entirely from BIP44 still reported BIP32 and every DashPay contact as a contributor — violating the field's documented contract and scaling release and registry bookkeeping with the address book. Membership now rides a HashSet (the linear `contains` made pooled funding quadratic in contact count), the ordered offered list drives build-time cleanup only, and the list stored on the transaction is derived by mapping each selected prevout back to the account that owns it. Pooled shortfalls. A pooled build's `available`/`required` describe the union of every offered source, so attributing them to the first preference reported aggregate figures as "insufficient funds on BIP44 account 0" — and could name BIP44 even when no such account existed and it was skipped. Single-source builds keep the account-specific error; pooled builds get `CorePooledInsufficientFunds`, carrying the source list instead of one account. Both map to the same FFI code, so hosts classify a shortfall exactly as before. Duplicate prevouts. Additive funding can offer an outpoint the builder was already seeded with through `add_inputs` (the `add_inputs_from_outpoints` FFI draws from the wallet's own account), and coin selection does not deduplicate, so the transaction could spend one prevout twice — invalid, and Core rejects it. Fixed upstream in dashpay/rust-dashcore#931; asserted here as well rather than handing a signer and then the network a transaction that cannot confirm. Swift `buildSignedPayment` defaulted to `.bip44` while its Kotlin counterpart defaults to pooled, so Swift callers omitting `accountType` could hit an insufficient-funds error with a sufficient pooled balance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ZocoLini
approved these changes
Aug 7, 2026
6 tasks
5 tasks
4 tasks
QuantumExplorer
added a commit
that referenced
this pull request
Aug 10, 2026
…ounts (#944) * feat(key-wallet): fund an asset lock from a caller-chosen list of accounts An asset lock could only ever be funded from ONE account. A wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them into BIP44 first and lock out of that — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. The send path stopped needing that in dashpay/platform#4329; this is the same change for asset locks. Both builders now take a LIST of `AccountTypePreference` sources plus a `source_index` instead of a single `AssetLockFundingAccount`, and fold them through the same `transaction_building::fund` the send path uses: coin selection draws from the union, the first source supplies the change address, overlapping sources fund each account once (#931's dedup is what makes the repeated `add_funding` safe), and derivation paths are collected across every contributing account so the inputs can be signed. Which accounts to pool is the CALLER's decision, not this library's. The FFI entry point takes the source list as a parameter (`FFIAccountTypePreference`, a tag plus the two identity IDs the DashPay kinds read) rather than applying a default of its own: a client wanting today's behavior passes a single `BIP44` source, one wanting the whole spendable balance passes the wider list, and one funding out of a specific contact names that friendship. An empty list is rejected instead of falling through to `AccountTypePreference::DEFAULT`, since defaulting there would be this layer choosing a funding policy that only the client library knows. Reservation bookkeeping is the part that had to change shape. A pooled build reserves in EACH contributing account's own set under the one owner token, so the post-build failure paths — credit-key derivation on the soft-wallet builder, the peek/sign/commit loop on the signer builder, both running after the transaction is already signed — now release across every funded account instead of just the one. Releasing a single account's set would have stranded the rest of the inputs until the 24-block TTL sweep. `AssetLockResult` carries the contributing accounts so the caller's rejected-broadcast release can reach them all; it is the contributor list, not everything the sources offered, so a wallet's address book does not inflate the caller's bookkeeping. `fund`'s strictness rule now matches platform's: a SINGLE named source is strict (a caller asking for exactly one account's funds must not silently be given another's), while a pooled list skips the sources this wallet has nothing for — no BIP32 account, no contacts — and errors only when none of them funds anything. Without that, a pooled set would fail on the very wallets it is meant to serve. CoinJoin funding is unchanged and stays excluded from pooling: it remains drain-only, and it must now be the sole source, because spending mixed outputs alongside transparent ones in one transaction links them and undoes the mixing. The `AssetLockFundingAccount::CoinJoin` + `drain: true` flow that dashpay/platform#4327 ships on converts to a single-element source list and behaves exactly as before. `AssetLockError::AccountNotFound(u32)` is removed — account resolution is now the builder's, and it reports `BuilderError::AccountNotFound` with the source that failed. `AssetLockFundingAccount` remains as the drain flows' single-account vocabulary, with a `From` conversion into the source list. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * docs(key-wallet): drop the private intra-doc link from build_asset_lock `validate_funding_sources` is a private free function, so linking to it from a public doc comment fails `rustdoc::private_intra_doc_links` under the Documentation job's `-D warnings`. The link was also useless to a reader of the public docs, who cannot follow it — state the CoinJoin rule inline instead. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * docs(key-wallet-ffi): flag CoinJoin as unusable on the non-drain asset lock `wallet_build_and_sign_asset_lock_transaction` always builds with `drain: false`, and `validate_funding_sources` rejects a CoinJoin source outside drain mode — so a caller selecting that kind here always gets `InvalidData`, even passing it alone. The kind's own doc described the builder's rule ("sole source, drain only") without saying this entry point never drains, which reads as though the sole-source form would work. Raised by CodeRabbit on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * refactor(key-wallet): share the asset-lock funding and reservation prologue `build_asset_lock` and `build_asset_lock_with_signer` had drifted into two near-identical copies of the same prologue: payload assembly, the drain strategy switch, `fund`, the per-account `ReservationSet` capture, and the release closure. They differed only in which signer reached `build_signed_reserved` and in comment wording. That duplication is a hazard on this specific logic. The reservation capture has to happen between funding and signing — a pooled build reserves in each contributing account's own set, and both builders reach their release paths after the transaction is already signed, with the caller holding no token. A fix applied to one copy and not the other silently reintroduces stranded inputs on the path that was missed. `build_signed_asset_lock` is now the single prologue, generic over `TransactionSigner` so the soft-wallet builder passes `wallet` and the signer builder passes `signer`. `BuildReservations` owns the sets, the reserved outpoints and the token, so releasing is one call that cannot reach only some of the funded accounts. Also from review: - `contributing_accounts` iterates the transaction's inputs rather than every UTXO of every offered account. A transaction has few inputs; an offered account can hold many UTXOs. - `From<AssetLockFundingAccount> for AccountTypePreference` documents that the index does not survive. The type's own doc said "convert to hand one to a builder", and a caller doing only that funds source_index 0 rather than the account it named. - The FFI entry point documents that `funding_sources[i].kind` must be a declared discriminant, since reading any other value as the enum is UB and so cannot be rejected at the boundary. Raised by CodeRabbit on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * test(key-wallet-ffi): cover the caller-supplied asset-lock funding sources The new boundary had no test of its own: the entry point's marshalling body is where the funding policy now lives, and nothing exercised it. Three cases, driven against a created-but-unfunded wallet so an accepted call fails in coin selection rather than at the guards — which is what tells the two apart: - an empty list is rejected with `InvalidInput` instead of being forwarded, where it would mean `AccountTypePreference::DEFAULT` and reinstate a policy this layer must not choose; - a well-formed pooled list gets past the guards into the build; - `CoinJoin` is rejected, pinning the behavior documented in f0d2cbf — this entry point always builds non-drain, so mixed funds can never back it. Prompted by the patch-coverage report on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * docs(key-wallet-ffi): regenerate FFI_API.md for the discriminant safety note The `# Safety` requirement added in 5a19149 changed the doc comment that `scripts/generate_ffi_docs.py` extracts, and the generated file was not refreshed alongside it — which the verify-ffi pre-commit hook catches. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * test(key-wallet-ffi): free the wallet handle the asset-lock test borrows `wallet_manager_get_wallet` returns an independently boxed clone that the caller owns — its own docs say to free it with `wallet_free_const` — and the test dropped it, leaking a whole `Wallet` per case. The Address Sanitizer job caught it: 42144 bytes in 24 allocations, three tests' worth. Also frees `tx_bytes` defensively. Every case here fails before a transaction is produced, so it is always null today, but a future success case would leak it the same way. Verified by reproducing the exact CI figure locally under `-Zsanitizer=address` with `detect_leaks=1`, then confirming it goes away. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * test(key-wallet-ffi): run the asset-lock source cases on both networks The helper hardcoded Testnet, so every FFI contract case here skipped Mainnet — against the repo's standing rule to test both configurations. Account derivation is coin-type-scoped, so a guard exercised on one network only could hide a network-conditional path. `call_with_sources` now takes the network and each case loops over both, naming the network in its assertion messages so a one-sided failure says which. Raised by CodeRabbit on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: bfoss765 <brian.foster@dash.org> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was fixed
add_funding(the additive replacement forset_funding, #925) appends every unreserved UTXO of a funding account to the candidate set without checking what the builder already holds. If the builder was seeded with one of those same UTXOs —add_inputswith a caller-chosen outpoint, or an earlieradd_fundingof an overlapping account — the outpoint became two candidates.Coin selection performs no outpoint deduplication.
SelectionStrategy::Allclones every candidate, so the duplicate was guaranteed to reach the transaction; the ordinary strategies can select both copies and double-count them toward the target. Either way the built transaction spends one prevout twice, and Core rejects it.This hazard is specific to additive funding: the
set_fundingit replaced didself.inputs = …, so a pre-seeded outpoint was silently dropped. Losing a caller's explicit input is a bug too, but building an invalid transaction is strictly worse, so the fix filters candidates rather than restoring the overwrite.Reservation correctness
The funding entry still records every unreserved outpoint the account owns that is in the candidate pool — including one pre-seeded by
add_inputs— so if selection picks it, the owning account reserves it. Recording only the UTXOs this call appended would leave a pre-seeded input unreserved and free for a concurrent build to select, which is the double-spend window the reservation system exists to close.Testing
add_funding_does_not_duplicate_a_pre_seeded_inputseeds a UTXO viaadd_inputs, funds the account that owns it, and drains withSelectionStrategy::All. It asserts the outpoint is offered exactly once, that the built transaction has no duplicate prevouts, and that the pre-seeded input is reserved. Verified it fails without the fix (the outpoint appears twice in the candidate list) and passes with it. Fullkey-walletsuite green (628 passed), clippy--all-targets -D warningsclean,cargo fmtclean.Why now
Found while reviewing dashpay/platform#4329, which adopts multi-account funding: platform's FFI exposes
core_wallet_tx_builder_add_inputs_from_outpoints(SwiftaddInputs), which seeds the builder with UTXOs drawn from the wallet's own account, and then finalizes throughadd_fundingon that same account — exactly the sequence that duplicates. That platform PR needs a re-pin onto this fix before it can merge.🤖 Generated with Claude Code
Summary by CodeRabbit