backport: wallet coin selection prerequisites for #7400#7479
Draft
thepastaclaw wants to merge 6 commits into
Draft
backport: wallet coin selection prerequisites for #7400#7479thepastaclaw wants to merge 6 commits into
thepastaclaw wants to merge 6 commits into
Conversation
4fef534 wallet: use GetChange() when computing waste (S3RK) 87e0ef9 wallet: use GetChange() in tx building (S3RK) 15e97a6 wallet: add SelectionResult::GetChange (S3RK) 72cad28 wallet: calculate and store min_viable_change (S3RK) e3210a7 wallet: account for preselected inputs in target (S3RK) f8e7963 wallet: add SelectionResult::Merge (S3RK) 06f558e wallet: accurate SelectionResult::m_target (S3RK) c8cf08e wallet: ensure m_min_change_target always covers change fee (S3RK) Pull request description: Benefits: 1. more accurate waste calculation for knapsack. Waste calculation is now consistent with tx building code. Before we always assumed change for knapsack even when the solution is changeless4. 2. simpler tx building code. Only create change output when it's needed 3. makes it easier to correctly account for fees for CPFP inputs (should be done in a follow up) In the first three commits we fix the code to accurately track selection target in `SelectionResult::m_target` Then we introduce new variable `min_change` that represents the minimum viable change amount Then we introduce `SelectionResult::GetChange()` which incapsulates dropping change for fee logic and uses correct values of `SelectionResult::m_target` Then we use `SelectionResult::GetChange()` in both tx building and waste calculation code This PR is a refactoring and shouldn't change the behaviour. There is only one known small change (arguably a bug fix). Before we dropped change output if it's smaller than `cost_of_change` after paying change fees. This is incorrect as `cost_of_change` already includes `change_fee`. ACKs for top commit: achow101: ACK 4fef534 Xekyo: crACK 4fef534 furszy: Code review ACK 4fef534 w0xlt: ACK bitcoin@4fef534 Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941 (cherry picked from commit 2bd9aa5)
…g target d0d9cf7 test: Check external coin effective value is used in CoinSelection (Aurèle Oulès) 76b79c1 wallet: Use correct effective value when checking target (Aurèle Oulès) Pull request description: Fixes bitcoin#26185. The following assert failed because it was not checked in the parent function. https://github.com/bitcoin/bitcoin/blob/2bd9aa5a44b88c866c4d98f8a7bf7154049cba31/src/wallet/coinselection.cpp#L391 ACKs for top commit: glozow: reACK d0d9cf7 furszy: ACK d0d9cf7 Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9 (cherry picked from commit cda6c79)
…pre-set-inputs fetching responsibility from Coin Selection 3fcb545 bench: benchmark transaction creation process (furszy) a8a7534 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy) f41712a wallet: simplify preset inputs selection target check (furszy) 5baedc3 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy) 295852f wallet: encapsulate pre-selected-inputs lookup into its own function (furszy) 37e7887 wallet: skip manually selected coins from 'AvailableCoins' result (furszy) 94c0766 wallet: skip available coins fetch if "other inputs" are disallowed (furszy) Pull request description: #### # Context (Current Flow on Master) In the transaction creation process, in order to select which coins the new transaction will spend, we first obtain all the available coins known by the wallet, which means walking-through the wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector. This coins vector is then provided to the Coin Selection process, which first checks if the user has manually selected any input (which could be internal, aka known by the wallet, or external), and if it does, it fetches them by searching each of them inside the wallet and/or inside the Coin Control external tx data. Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection process walks-through the entire available coins vector once more just to erase coins that are in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside the same transaction). #### # Process Workflow Changes Now, a new method, `FetchCoins` will be responsible for: 1) Lookup the user pre-selected-inputs (which can be internal or external). 2) And, fetch the available coins in the wallet (excluding the already fetched ones). Which will occur prior to the Coin Selection process. Which allows us to never include the pre-selected-inputs inside the available coins vector in the first place, as well as doing other nice improvements (written below). So, Coin Selection can perform its main responsibility without mixing it with having to fetch internal/external coins nor any slow and unneeded duplicate coins verification. #### # Summarizing the Improvements: 1) If any pre-selected-input lookup fail, the process will return the error right away. (before, the wallet was fetching all the wallet available coins, walking through the entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins) 2) The pre-selected-inputs lookup failure causes are properly described on the return error. (before, we were returning an "Insufficient Funds" error for everything, even if the failure was due a not solvable external input) 3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected). Now, the available coins vector, which is built after the pre-selected-inputs fetching, doesn’t include the already selected inputs in the first place. 4) **Faster transaction creation** for transactions that only use manually selected inputs. We now will return early, as soon as we finish fetching the pre-selected-inputs and not perform the resources expensive calculation of walking-through the entire wallet txes map to obtain the available coins (coins that we will not use). --------------------------- Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically. #### Result on this PR (tip f6d0bb2d): | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction` vs #### Result on master (tip 4a4289e): | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction` The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 . ACKs for top commit: S3RK: Code Review ACK 3fcb545 achow101: ACK 3fcb545 aureleoules: reACK 3fcb545 Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731 (cherry picked from commit f37bd15)
…elected coins 68eed5d test,gui: add coverage for PSBT creation on legacy watch-only wallets (furszy) 306aab5 test,gui: decouple widgets and model into a MiniGui struct (furszy) 2f76ac0 test,gui: decouple chain and wallet initialization from test case (furszy) cd98b71 gui: 'getAvailableBalance', include watch only balance (furszy) 74eac3a test: add coverage for 'useAvailableBalance' functionality (furszy) dc1cc1c gui: bugfix, getAvailableBalance skips selected coins (furszy) Pull request description: Fixes bitcoin-core/gui#688 and bitcoin#26687. First Issue Description (bitcoin-core/gui#688): The previous behavior for `getAvailableBalance`, when the coin control had selected coins, was to return the sum of them. Instead, we are currently returning the wallet's available total balance minus the selected coins total amount. Reason: Missed to update the `GetAvailableBalance` function to include the coin control selected coins on bitcoin#25685. Context: Since bitcoin#25685 we skip the selected coins inside `AvailableCoins`, the reason is that there is no need to waste resources walking through the entire wallet's txes map just to get coins that could have gotten by just doing a simple `mapWallet.find`). Places Where This Generates Issues (only when the user manually select coins via coin control): 1) The GUI balance check prior the transaction creation process. 2) The GUI "useAvailableBalance" functionality. Note 1: As the GUI uses a balance cache since bitcoin-core/gui#598, this issue does not affect the regular spending process. Only arises when the user manually select coins. Note 2: Added test coverage for the `useAvailableBalance` functionality. ---------------------------------- Second Issue Description (bitcoin#26687): As we are using a cached balance on `WalletModel::getAvailableBalance`, the function needs to include the watch-only available balance for wallets with private keys disabled. ACKs for top commit: Sjors: tACK 68eed5d achow101: ACK 68eed5d theStack: ACK 68eed5d Tree-SHA512: 674f3e050024dabda2ff4a04b9ed3750cf54a040527204c920e1e38bd3d7f5fd4d096e4fd08a0fea84ee6abb5070f022b5c0d450c58fd30202ef05ebfd7af6d3 (cherry picked from commit 27dcc07)
…ing) Dash-specific follow-up to the bitcoin#25647 backport. Coin selection estimates CompactSize prefixes before the final input and output counts are known. Wider vin-count prefixes at 253 or 65,536 inputs, or a wider vout-count prefix when change raises 252 or 65,535 recipients across a boundary, can leave the selected value a few duffs short of the accurately measured final fee. After selecting inputs, recompute the non-input fee with the actual vin-count prefix and the prospective change output count, then retry selection if the target must increase. This keeps change-bearing and exact no-change transactions on the same path and preserves the final Fee needed > fee paid check for unrelated accounting defects. Add regression coverage for 252 inputs, 253 inputs with change, an exact-target 253-input transaction without change, and 252 recipients plus change.
…ification) Dash's current wallet notification interface accepts interfaces::BlockInfo after the kernel chain interface backport. Construct the notification with kernel::MakeBlockInfo while preserving the bitcoin#25685 benchmark behavior.
thepastaclaw
force-pushed
the
backport-7400-prereq-coin-selection
branch
from
July 23, 2026 12:50
b4cf789 to
bc52f0a
Compare
Author
|
✅ Final review complete — no blockers (commit bc52f0a) |
Author
|
CI triage for current head @knst could you rerun the failed job when convenient? No branch change looks warranted from this failure. |
thepastaclaw
commented
Jul 24, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Author
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The final tree correctly carries the four advertised wallet backports and the Dash-specific CompactSize and BlockInfo adaptations; the omitted upstream legacy watch-only PSBT GUI coverage is an explicit architecture-specific exclusion, while the applicable selected-coin balance behavior is adapted and tested in Dash. No runtime or Dash-subsystem correctness defects remain at the PR head. The only retained finding is commit-stack hygiene: the benchmark is introduced with an uncompilable wallet notification call and repaired three commits later.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— backport-reviewer (completed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 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 `src/bench/wallet_create_tx.cpp`:
- [SUGGESTION] src/bench/wallet_create_tx.cpp:73: bench target does not compile for three intermediate commits (fixed by final commit in this same PR)
Commit `c392040ca08` registers this new source in `bench_bench_dash_SOURCES` but calls `wallet.blockConnected(block, pindex->nHeight)`. At that commit, `CWallet` declares only `blockConnected(const interfaces::BlockInfo&)`, so a wallet-enabled benchmark build fails at `c392040ca08` and remains broken through `0969ce0fe7e` and `cde726cc5e1`. The final commit correctly changes the call to `kernel::MakeBlockInfo`, but Dash preserves commits when merging, so fold that adaptation into `c392040ca08` or place it immediately afterward to keep the permanent history independently buildable and bisectable.
|
|
||
| // notify wallet | ||
| const auto& pindex = WITH_LOCK(::cs_main, return context.chainman->ActiveChain().Tip()); | ||
| wallet.blockConnected(kernel::MakeBlockInfo(pindex, &block)); |
Author
There was a problem hiding this comment.
🟡 Suggestion: bench target does not compile for three intermediate commits (fixed by final commit in this same PR)
Commit c392040ca08 registers this new source in bench_bench_dash_SOURCES but calls wallet.blockConnected(block, pindex->nHeight). At that commit, CWallet declares only blockConnected(const interfaces::BlockInfo&), so a wallet-enabled benchmark build fails at c392040ca08 and remains broken through 0969ce0fe7e and cde726cc5e1. The final commit correctly changes the call to kernel::MakeBlockInfo, but Dash preserves commits when merging, so fold that adaptation into c392040ca08 or place it immediately afterward to keep the permanent history independently buildable and bisectable.
source: ['claude', 'codex']
5 tasks
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
1 participant
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.
Issue being fixed or feature implemented
Dash PR #7400 currently combines its target wallet changes with a longer Bitcoin Core prerequisite chain. This extracts the first independently mergeable prerequisite batch so the backports can be reviewed and landed in smaller units.
This branch was rebuilt from
upstream/developat87692f7b1506f8454e1b9c75c8fcf260b2c0dcd4. It contains four logical Bitcoin Core backports, within the requested limit of at most five per prerequisite PR. It does not modify or replace #7400; that PR must remain unchanged until the prerequisite PRs merge.What was done?
Backported:
SelectionResultThe four Bitcoin backport commits retain matching stable patch IDs to their counterparts extracted from the exact #7400 source head (
80450061d0e0e5f22c334c2975fcb7531c7229bb). bitcoin#26699 intentionally travels with bitcoin#25685 because omitting it makes manual coin-control balance checks exclude the selected coins.Dash-specific adaptations included with the backports:
FundSpecialTxselected-input handling inrpc/evo.cpp.interfaces::BlockInfonotification API.Dash's existing watch-only GUI architecture differs from upstream: it selects watch-only coins directly through the wallet interface rather than the upstream cached-balance path. The upstream-only legacy watch-only PSBT GUI regression test was therefore not carried over.
Later sequencing, after this PR merges:
descriptorprocesspsbtrpc bitcoin/bitcoin#25796, IntroduceMockableDatabasefor wallet unit tests bitcoin/bitcoin#26715, walletdb: Remove unused CreateMockWalletDatabase bitcoin/bitcoin#27665, and wallet, bench: Move commonly used functions to their own file and fix a bug bitcoin/bitcoin#27666.No later prerequisite PR has been opened yet.
How Has This Been Tested?
Tested on macOS arm64 using the repository's autotools build configured against the existing Dash depends prefix.
Build targets:
test/test_dashdashdbench/bench_dashqt/test/test_dash-qtdash-clidash-txdash-walletUnit and benchmark coverage:
test_dash --run_test=coinselector_tests,spend_tests,wallet_tests— 34 test cases passedWalletCreateTxUseOnlyPresetInputsbenchmark passedWalletCreateTxUsePresetInputsAndCoinSelectionbenchmark passedFunctional coverage, run serially:
rpc_psbt.py --descriptorsrpc_psbt.py --legacy-walletwallet_fundrawtransaction.py --descriptorswallet_fundrawtransaction.py --legacy-walletwallet_fundrawtransaction.py --legacy-wallet --nohdwallet_send.py --descriptorswallet_send.py --legacy-walletAll listed variants passed.
Runtime RPC verification against the built
dashd/dash-cli:Insufficient fundsRPC error rather than an internal fee-accounting error.Additional checks:
qt/test/test_dash-qt -platform minimalpassed; the existing macOS/minimal guards skipped theWalletTests,AppTests, andAddressBookTestsbodies.test/lint/lint-whitespace.pytest/lint/lint-circular-dependencies.pytest/lint/lint-python.pytest/lint/lint-include-guards.pytest/lint/lint-includes.pygit diff --checkBreaking Changes
None.
Checklist: