#34405 wallet: skip APS when no partial spend exists
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34405 wallet: skip APS when no partial spend exists | Ready | 0 | Positive | 33 + 81 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Gates the Avoid Partial Spends (APS) coin selection pass on whether the initial ungrouped pass left any scriptPubKey partially spent. It compares available and selected UTXO counts per scriptPubKey during transaction creation—including preselected inputs—and sets a flag on the transaction result to skip the second pass when no partial spend exists. USDT tracepoint tests and wallet group tests are updated to verify APS execution behavior.
Problem: The wallet unconditionally runs a second grouped coin selection pass whenever fee constraints allow, even if the initial selection already selected all or none of the UTXOs for every scriptPubKey. This causes unnecessary coin selection computation and causes USDT tracepoints and debug logs to falsely report that APS was utilized to avoid a partial spend.
Evidence of importance:
- Fixes #25150 ('Coin Selection tracepoint overreports use of APS')
- PR description: 'The second pass runs even when the first selection left no partial spend for it to avoid, and aps_create_tx_internal then reports APS as used whenever the grouped result is taken, overreporting how often a partial spend was avoided.'
Risk of a mistake: medium (wallet). A logic error in partial spend detection or coin selection branching could cause transactions to fail creation or select unexpected inputs.
Changed since the previous assessment: Now explicitly accounts for preselected coins in preset_inputs when counting available UTXOs per scriptPubKey, and separates the implementation from new functional test coverage across two commits.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: re-review from polespinasa after author implemented suggestions 2026-10-03
- reviewer: polespinasa to review author's pushed changes addressing suggestions 2026-10-03
Agreement
Positive
support without stated reasons, no open objection (polespinasa)
Thread notes: Added achow101's test suggestion and the author's answer. Marked polespinasa's suggestions as fixed following the author's push and replies. / Added earlier suggestions from achow101 that were missing under schema 1. Marked polespinasa's three test and allocation suggestions as fixed following the author's updates and reply.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| achow101 (member) | suggestion, style | group condition with the others rather than having a separate return statement | fixed · fix pushed · author combined the condition checks | answered | yes | 2026-02-02: 'This condition can be grouped with the others in the line above, there's no need to separate it with it's own return statement.' Status from: 2026-02-03: 8144225309: 'Done, combined.' |
| achow101 (member) | suggestion, interface | set default wallet_name to self.default_wallet_name instead of None | answered · author explained that self is not available at function definition time in Python | answered | yes | 2026-02-02: 'The default for `wallet_name` is essentially `self.default_wallet_name`, so set it to that instead of using `None`.' Status from: 2026-02-03: 8144225309: '`self` isn't available at function definition time in Python.' |
| polespinasa (member) | suggestion, approach | avoid creating temporary COutput vectors by iterating the underlying CoinsResult vectors directly | fixed · fix pushed · implemented without All() temporary copies | answered | yes | 2026-10-01: 'By calling `available_coins.All()` and `preset_inputs.All()` (both are `CoinsResult::All()`) you are creating two new temporary vectors of `COutput`.' Status from: 2026-10-03: 8144225309: 'the partial spend check now iterates `CoinsResult::coins` instead of copying through `All()`' |
| polespinasa (member) | suggestion, maintenance | assert the length of vin in the partial spend detection test | fixed · fix pushed · assert added to test | answered | yes | 2026-10-01: 'I would probably assert also here the len of `vin` similar to what you did in line 206' Status from: 2026-10-03: 8144225309: 'the no-partial-spend case asserts its premise and the input count' |
| polespinasa (member) | suggestion, maintenance | assert that the wallet's UTXOs have different scriptPubKeys to document the test premise | fixed · fix pushed · premise assertion added to test | answered | yes | 2026-10-01: 'The test has a premise and is that the two UTXOs that the wallet has are owned by different scriptpubkey, maybe asserting it in the code would serve as documentation.' Status from: 2026-10-03: 8144225309: 'the no-partial-spend case asserts its premise and the input count' |
Support:
- polespinasa (member) (Concept ACK, 2026-10-01) [read]: looks pretty good despite unfamiliarity with this part of the wallet codebase [not substantive]
Participants:
- achow101 (member, neutral, 2 statements 2026-02-02 to 2026-02-02): suggested using default_wallet_name in test helper
- sedited (member, neutral, 1 statements 2026-09-22 to 2026-09-22): asked if author was still working on the PR and noted need for a rebase
- polespinasa (member, support, 4 statements 2026-10-01 to 2026-10-01): concept ACK; suggested iterating coins directly and adding test assertions
Files
91 lines under test/bench/ci.
- test/functional/interface_usdt_coinselection.py +50/-10
- src/wallet/spend.cpp +29/-3
- test/functional/wallet_groups.py +31/-0
- src/wallet/types.h +4/-2
Card
The short text the ranking pass compares across PRs.
This pull request ensures the wallet only executes a second Avoid Partial Spends (APS) coin selection pass if the initial ungrouped pass actually left a scriptPubKey partially spent. Previously, the wallet ran the APS pass whenever fee limits allowed, wasting computation during transaction creation and causing USDT tracepoints and debug logs to overreport APS usage (fixing issue #25150). The change compares selected against available coins per scriptPubKey, taking manually preselected inputs into account, and adds test coverage in wallet_groups.py and USDT tracepoint tests.
Data
thread read through 2026-10-03 (15 statements), head 559cd158c2
labels: Wallet
Categories
Wallet
P3 · bug fix (ranking pass: Moved up: it corrects misreported APS usage in tracepoints/logs, which is more than a pure cleanup.)
Avoids redundant computation during transaction creation and fixes misleading diagnostics. As noted in issue #25150, the wallet runs a second grouped coin selection pass even when no partial spend exists to avoid, causing tracepoints to overreport APS usage.
Membership: Wallet label and changes to coin selection logic in src/wallet/spend.cpp and src/wallet/types.h
Factors: security/stability 0, bug 1, performance 1, user value 0, leverage 0