13161ecf032b7a850686e5942c12222c8f3d0d52 opt: Skip over barren combinations of tiny UTXOs (Murch)
b7672c7cdd87acb105639f475744094b53cc9891 opt: Skip checking max_weight separately (Murch)
1edd2baa37a69ff69c2eaeceaad1028f1968cbab opt: Cut if last addition was minimal weight (Murch)
5248e2a60d243b3d5c77ecd9e4c335daca399a48 opt: Skip heavier UTXOs with same effective value (Murch)
9124c73742287b06dfe6e8a94be56ace25f07b2c opt: Tiebreak UTXOs by weight for CoinGrinder (Murch)
451be19dc10381122f705bbb2127b083f1d598c6 opt: Skip evaluation of equivalent input sets (Murch)
407b1e3432b77511900b77be84d5d7450352f462 opt: Track remaining effective_value in lookahead (Murch)
5f84f3cc043c5fb15072f5072fee752eaa01a2ec opt: Skip branches with worse weight (Murch)
d68bc74fb2e3ae4ae775ab544fe5b4ab46025abb fuzz: Test optimality of CoinGrinder (Murch)
67df6c629a2e9878b06c1903e90b67087eaa3688 fuzz: Add CoinGrinder fuzz target (Murch)
1502231229dbc32c94e80a2fc2959275c5d246ef coinselection: Track whether CG completed (Murch)
7488acc64685ae16e20b78e7ad018137f27fe404 test: Add coin_grinder_tests (Murch)
6cc9a46cd0f4ed80d4523bbef1508142e0c80d27 coinselection: Add CoinGrinder algorithm (Murch)
89d09566431f57034d9a7df32547ceb13d79c62c opt: Tie-break UTXO sort by waste for BnB (Murch)
aaee65823c6e620bef5cc96d8026567e64d822fe doc: Document max_weight on BnB (Murch)
Pull request description:
***Please refer to the [topic on Delving Bitcoin](https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279) discussing Gutter Guard/Coingrinder simulation results.***
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can significantly increase the fees
- Users are upset when fees are relatively large compared to the amount sent
- Some users struggle to maintain a sufficient count of UTXOs in their wallet
Approach
---
So far, Bitcoin Core has used a balanced approach to coin selection, where it will generate multiple input set candidates using various coin selection algorithms and pick the least wasteful among their results, but not explicitly minimize the input set weight. Under some circumstances, we _do_ want to minimize the weight of the input set. Sometimes changeless solutions require many or heavy inputs, and there is not always a changeless solution for Branch and Bound to find in the first place. This can cause expensive transactions unnecessarily. Given a wallet with sufficient funds, `CoinGrinder` will pick the minimal-waste input set for a transaction with a change output. The current implementation only runs `CoinGrinder` at feerates over 3×long-term-feerate-estimate (by default 30 ṩ/vB), which may be a decent compromise between our goal to reduce costs for the users, but still permit transactions at lower feerates to naturally reduce the wallet’s UTXO pool to curb bloat.
Trade-offs
---
Simulations for my thesis on coin selection ([see Section 6.3.2.1 [PDF]](https://murch.one/erhardt2016coinselection.pdf)) suggest that minimizing the input set for all transactions tends to grind a wallet’s UTXO pool to dust (pun intended): an approach selecting inputs per coin-age-priority (in effect similar to “largest first selection”) on average produced a UTXO pool with 15× the UTXO count as Bitcoin Core’s Knapsack-based Coin Selection then (in 2016). Therefore, I do not recommend running `CoinGrinder` under all circumstances, but only at extreme feerates or when we have another good reason to minimize the input set for other reasons. In the long-term, we should introduce additional metrics to score different input set candidates, e.g. on basis of their privacy and wallet health impact, to pick from all our coin selection results, but until then, we may want to limit use of `CoinGrinder` in other ways.
ACKs for top commit:
achow101:
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
sr-gi:
ACK [13161ec](13161ecf03)
sipa:
ACK 13161ecf032b7a850686e5942c12222c8f3d0d52
Tree-SHA512: 895b08b2ebfd0b71127949b7dba27146a6d10700bf8590402b14f261e7b937f4e2e1b24ca46de440c35f19349043ed2eba4159dc2aa3edae57721384186dae40
Instead turning tx.vout into a vector of `CRecipient`, make `FundTransaction`
take a `CRecipient` vector directly. This allows us to remove SFFO logic from
the wrapper RPC `FundTransaction` since the `CRecipient` objects have already
been created with the correct SFFO values. This also allows us to remove
SFFO from both `FundTransaction` function signatures.
This sets us up in a future PR to be able to use these RPCs with BIP352
static payment codes.
a44808fb437864878c2d9696b8a96193091446ee fuzz: rule-out too deep derivation paths in descriptor parsing targets (Antoine Poinsot)
Pull request description:
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.
ACKs for top commit:
sipa:
utACK a44808fb437864878c2d9696b8a96193091446ee
achow101:
ACK a44808fb437864878c2d9696b8a96193091446ee
dergoegge:
ACK a44808fb437864878c2d9696b8a96193091446ee - Not running into timeouts anymore
TheCharlatan:
ACK a44808fb437864878c2d9696b8a96193091446ee
Tree-SHA512: a5dd1dbe9adf8f088bdc435addab88b56f435e6d7d2065bd6d5c6d80a32e3f1f97d3d2323131ab233618cd6dcc477c458abe3c4c865ab569449b8bc176231e93
e03d6f7ed534f423f58236866f8e83beee1871e1 fuzz: set `m_fallback_fee`/`m_fee_mode` in `wallet_fees` target (brunoerg)
Pull request description:
`m_fallback_fee` and `m_fee_mode` are used in `GetMinimumFeeRate` but we're not setting any value for them in `wallet_fees` target. That's the reason fuzzing is never reaching the following code:
This PR fixes it.
ACKs for top commit:
maflcko:
review ACK e03d6f7ed534f423f58236866f8e83beee1871e1
achow101:
ACK e03d6f7ed534f423f58236866f8e83beee1871e1
murchandamus:
ACK e03d6f7ed534f423f58236866f8e83beee1871e1
Tree-SHA512: 5d364f5351d65762a3ddf88e3abb7bda401b7e4955285e083031d216fb50082b1ea98e2c065aff75a5a8a3d1bc4c2e5e3ca9f9478d902ee8f8d4347b6cbe53af
576bee88fd36e207b7288077626947a1fce0fc33 fuzz: disable BnB when SFFO is enabled (furszy)
05e5ff194c7722b4ebc2b9309fc0bf47b3cf1df7 test: add coverage for BnB-SFFO restriction (furszy)
0c5755761c3e544547899ad096121585dffa73df wallet: create tx, log resulting coin selection info (furszy)
5cea25ba795d6eb9ccc721d01560783ae576af34 wallet: skip BnB when SFFO is active (Murch)
Pull request description:
Solves #28918. Coming from https://github.com/bitcoin/bitcoin/issues/28918#issuecomment-1838626406 discussion.
The intention is to decouple only the bugfix relevant commits from #28985, allowing them to be included in the 26.x release. This way, we can avoid disabling the coin selection fuzzing test for an entire release.
Note:
Have introduced few changes to the bug fix commit so that the unit tests pass without the additional burden introduced in #28985.
ACKs for top commit:
josibake:
ACK 576bee88fd
murchandamus:
ACK 576bee88fd
achow101:
ACK 576bee88fd36e207b7288077626947a1fce0fc33
Tree-SHA512: f5d90eb3f3f524265afe4719495c9bf30f98b9af26cf039f7df5a7db977abae72caa7a3478cdd0ab10cd143bc1662e8fc5286b5bc10fc10f0dd582a45b45c31a
Instead of using the output parameters, return CreatedTransactionResult
from FundTransaction in the same way that CreateTransaction does.
Additionally, instead of modifying the original CMutableTransaction, the
result from CreateTransactionInternal is used.
Instead of having a separate CCoinControl::SelectExternal function, we
can use the normal CCoinControl::Select function and explicitly use
PreselectedInput::SetTxOut in the caller. The semantics of what an
external input is remains.
Also, add missing includes to scriptpubkeyman.
Also, export dependecies of the BasicTestingSetup from setup_common.h,
to avoid having to include them when setup_common.h is already included.
9e58c5bcd96e7ff2062274868814ccae0626589e Use Txid in COutpoint (dergoegge)
Pull request description:
This PR changes the type of the hash of a transaction outpoint from `uint256` to `Txid`.
ACKs for top commit:
Sjors:
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e
stickies-v:
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.
TheCharlatan:
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e
Tree-SHA512: 58f61ce1c58668f689513e62072a7775419c4d5af8f607669cd8cdc2e7be9645ba14af7f9e2d65da2670da3ec1ce7fc2a744037520caf799aba212fd1ac44b34
The valid results should have a target below the sum of
the selected inputs amounts. Also, it increases the
minimum value for target to make it more realistic.
Instead of using `cost_of_change` for `min_viable_change`
and `change_cost`, and 0 for `change_fee`, use values from
`coin_params`. The previous values don't generate any effects
that is relevant for that context.
1771daa815ec014276cfcb30c934b0eaff4d72bf [fuzz] Show that SRD budgets for non-dust change (Murch)
941b8c6539d72890fd4e36fc900be9c300e1d737 [bug] Increase SRD target by change_fee (Murch)
Pull request description:
I discovered via fuzzing of another coin selection approach that at extremely high feerates SRD may find input sets that lead to transactions without change outputs. This is an unintended outcome since SRD is meant to always produce a transaction with a change output—we use other algorithms to specifically search for changeless solutions.
The issue occurs when the flat allowance of 50,000 ṩ for change is insufficient to pay for the creation of a change output with a non-dust amount, at and above 1,613 ṩ/vB. Increasing the change budget by `change_fee` makes SRD behave as expected at any feerates.
Note: The intermittent failures of `test/functional/interface_usdt_mempool.py` are a known issue: https://github.com/bitcoin/bitcoin/issues/27380
ACKs for top commit:
achow101:
ACK 1771daa815ec014276cfcb30c934b0eaff4d72bf
S3RK:
ACK 1771daa815ec014276cfcb30c934b0eaff4d72bf
Tree-SHA512: 3f36a3e317ef0a711d0e409069c05032bff1d45403023f3728bf73dfd55ddd9e0dc2a9969d4d69fe0a426807ebb0bed1f54abfc05581409bfe42c327acf766d4
I discovered via fuzzing of another coin selection approach that at
extremely high feerates SRD may find input sets that lead to
transactions without change outputs. This is an unintended outcome since
SRD is meant to always produce a transaction with a change output—we use
other algorithms to specifically search for changeless solutions.
The issue occures when the flat allowance of 50,000 ṩ for change is
insufficient to pay for the creation of a change output with a non-dust
amount, at and above 1,613 ṩ/vB. Increasing the change budget by
change_fees makes SRD behave as expected at any feerates.
As the fuzzer test requires all blocks to be
scanned by the wallet (because it is asserting
the wallet balance at the end), we need to
ensure that no blocks are skipped by the recently
added wallet birth time functionality.
This just means setting the chain accumulated time
to the maximum value, so the wallet birth time is
always below it, and the block is always processed.
Uses a min-effective-value heap, so we can remove the least valuable input/s
while the selected weight exceeds the maximum allowed weight.
Co-authored-by: Murch <murch@murch.one>
The simplest scenario where this is useful is on the 'check_max_weight' unit test
already:
We create 1515 UTXOs with 0.033 BTC each, and 1 UTXO with 50 BTC. Then perform
Coin Selection.
As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the
expectation here is to receive a selection result that only contain the big
UTXO (which is not happening for the reasons stated below).
As knapsack returns a result that exceeds the max allowed transaction size, we
fallback to SRD, which selects coins randomly up until the target is met. So
we end up with a selection result with lot more coins than what is needed.
And not hide it inside the `OutputGroup::Insert` method.
This method does not return anything if insertion fails.
We can know before calling `Insert` whether the coin
will be accepted or not.
