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
Verify the transaction creation process does not produce
a BnB solution when SFFO is enabled.
This is currently problematic because it could require a
change output. And BnB is specialized on changeless solutions.
Co-authored-by: Andrew Chow <achow101@gmail.com>
Co-authored-by: Murch <murch@murch.one>
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.
When a transaction uses an unconfirmed input, preceding this commit it
would not consider the feerate of the parent transaction. Given a parent
transaction with a lower ancestor feerate, this resulted in the new
transaction's ancestor feerate undershooting the target feerate.
This commit changes how we calculate the effective value of unconfirmed UTXOs.
The effective value of unconfirmed UTXOs is decreased by the fee
necessary to bump its ancestry to the target feerate. This also impacts
the calculation of the waste metric: since the estimate for the current
fee is increased by the bump fees, unconfirmed UTXOs current fees appear less
favorable compared to their unchanged long term fees.
This has one caveat: if multiple UTXOs have overlapping ancestries, each
of their individual estimates will account for bumping all ancestors.
GetSelectionWaste will need to access more context within a selection
result, and so should be a private member function rather than a static
function. It's only use outside of SelectionResult was for tests which
have now been updated to just make a SelectionResult.
Co-authored-by: Murch <murch@murch.one>
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.
Since we have a mockable wallet database, we don't really need to be
using BDB or SQLite's in-memory database capabilities. It doesn't really
help us to be using those as we aren't doing anything that requires one
type of db over the other, and will just prefer SQLite if it's
available.
MockableDatabase is suitable for these uses, so use
CreateMockableWalletDatabase to use that.
-BEGIN VERIFY SCRIPT-
sed -i "s/CreateMockWalletDatabase(options)/CreateMockableWalletDatabase()/" $(git grep -l "CreateMockWalletDatabase(options)" -- ":(exclude)src/wallet/walletdb.*")
sed -i "s/CreateMockWalletDatabase/CreateMockableWalletDatabase/" $(git grep -l "CreateMockWalletDatabase" -- ":(exclude)src/wallet/walletdb.*")
-END VERIFY SCRIPT-
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.
Optimizes coin selection by performing the "group outputs"
procedure only once, outside the "attempt selection" process.
Avoiding the repeated execution of the 'GroupOutputs' operation
that occurs on each coin eligibility filters (up to 8 of them);
then for every coin vector type plus one for all the coins together.
This also let us not perform coin selection over coin eligibility
filtered groups that don't add new elements.
(because, if the previous round failed, and the subsequent one has
the same coins, then this new round will fail again).
The 'GroupOutputs()' function performs the same
calculations for only-positive and mixed groups,
the only difference is that when we look for
only-positive groups, we discard negative utxos.
So, instead of wasting resources calling GroupOutputs()
for positive-only first, then call it again to include
the negative ones in the result, we can execute
GroupOutputs() only once, including in the response
both group types (positive-only and mixed).
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.
52f4d567d69425dfd514489079db80483024a80d refactor: remove <util/system.h> include from wallet.h (furszy)
6c9b342c306b9e17024762c4ba8f1c64e9810ee2 refactor: wallet, remove global 'ArgsManager' access (furszy)
d8f5fc446216258a68e256076c889ec23471855f wallet: set '-walletnotify' script instead of access global args manager (furszy)
3477a28dd3b4bc6c1993554c5ce589d69fa86070 wallet: set keypool_size instead of access global args manager (furszy)
Pull request description:
Structurally, the wallet class shouldn't access the global `ArgsManager` class, its internal behavior shouldn't be coupled to a global command line args parsing object.
So this PR migrates the only two places where we depend on it: (1) the keypool size, and (2) the "-walletnotify" script. And cleans up the, now unneeded, wallet `ArgsManager` ref member.
Extra note:
In the process of removing the args ref member, discovered and fixed files that were invalidly depending on the wallet header including `util/system.h`.
ACKs for top commit:
achow101:
ACK 52f4d567d69425dfd514489079db80483024a80d
TheCharlatan:
Re-ACK 52f4d567d69425dfd514489079db80483024a80d
hebasto:
re-ACK 52f4d567d69425dfd514489079db80483024a80d
Tree-SHA512: 0cffd99b4dd4864bf618aa45aeaabbef2b6441d27b6dbb03489c4e013330877682ff17b418d07aa25fbe1040bdf2c67d7559bdeb84128c5437bf0e6247719016
9567bfeab95cc0932073641dd162903850987d43 clang-tidy: Add `performance-no-automatic-move` check (Hennadii Stepanov)
Pull request description:
Split from bitcoin/bitcoin#26642 as [requested](https://github.com/bitcoin/bitcoin/pull/26642#discussion_r1054673201).
For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html.
The following types are affected:
- `std::pair<CAddress, NodeSeconds>`
- `std::vector<CAddress>`
- `UniValue`, also see bitcoin/bitcoin#25429
- `QColor`
- `CBlock`
- `MempoolAcceptResult`
- `std::shared_ptr<CWallet>`
- `std::optional<SelectionResult>`
- `CTransactionRef`, which is `std::shared_ptr<const CTransaction>`
ACKs for top commit:
andrewtoth:
ACK 9567bfeab95cc0932073641dd162903850987d43
aureleoules:
ACK 9567bfeab95cc0932073641dd162903850987d43
Tree-SHA512: 9b6a5d539205b41d2c86402d384318ed2e1d89e66333ebd200a48fd7df3ce6f6c60a3e989eda5cc503fb34b8d82526f95e56776e1af51e63b49e3a1fef72dbcb
f2fc03ec856d7d19a20c482514350cced38f9504 refactor: use braced init for integer constants instead of c style casts (Pasta)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/23810 for more context. This is broken out from that PR, as it is less breaking, and should be trivial to review and merge.
EDIT: Long term, the intention is to remove all C-style casts, as they can dangerously introduce reinterpret_casts. This is one step which removes a number of trivially removable C-style casts
ACKs for top commit:
aureleoules:
ACK f2fc03ec856d7d19a20c482514350cced38f9504
Tree-SHA512: 2fd11b92c9147e3f970ec3e130e3b3dce70e707ff02950a8c697d4b111ddcbbfa16915393db20cfc8f384bc76f13241c9b994a187987fcecd16a61f8cc0af14c
b942c94d153f83b77ef5d603211252d9abadde95 test: Change coinselection parameter location to make tests independent (yancy)
Pull request description:
the `subtract_fee_outputs` param is expected to be `true` for all subsequent tests. It should be defined outside of a single test so that if it's removed or changed, all subsequent tests won't fail. Currently if you remove this [test](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L304:L325) the following [test](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L327:L345) fails. This change makes the tests independent.
ACKs for top commit:
achow101:
ACK b942c94d153f83b77ef5d603211252d9abadde95
aureleoules:
ACK b942c94d153f83b77ef5d603211252d9abadde95.
rajarshimaitra:
tACK b942c94d153f83b77ef5d603211252d9abadde95
theStack:
ACK b942c94d153f83b77ef5d603211252d9abadde95
Tree-SHA512: 461e19d15351318102ef9f96c68442365d8ca238c48ad7aefe23e8532b33b91dadf6c7840c7894574bccede6da162a55ad7a6f6a330d61a11ce804e68ddc5e9c
76dc547ee7b05864e7b1b6c55fc0301d47aa3a15 gui: create tx, launch error dialog if backend throws runtime_error (furszy)
f4d79477ff0946b0bd340ade9251fa38e3b95dd7 wallet: coin selection, add duplicated inputs checks (furszy)
0aa065b14e67592d5be8f46ebbe5d59a083ff0a5 wallet: return accurate error messages from Coin Selection (furszy)
7e8340ab1a970a14e180b1fcf420b46a5657b062 wallet: make SelectCoins flow return util::Result (furszy)
e5e147fe97f706e82bc51358f8bdc355f355be57 wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy)
Pull request description:
Work decoupled from #25806, which cleanup and improves the Coin Selection flow further.
Adding the capability to propagate specific error messages from the Coin Selection process to the user.
Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally.
Letting us instruct the user how to proceed under certain circumstances.
The following error messages were added:
1) If the selection result exceeds the maximum transaction weight,
we now will return:
-> "The inputs size exceeds the maximum weight. Please try sending
a smaller amount or manually consolidating your wallet's UTXOs".
2) If the user pre-selected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return:
-> "The preselected coins total amount does not cover the transaction
target. Please allow other inputs to be automatically selected or include
more coins manually".
3) The double-counted preset inputs during Coin Selection error will now
throw an "internal bug detected" message instead of crashing the node.
The essence of this work comes from several comments:
1. https://github.com/bitcoin/bitcoin/pull/26560#discussion_r1037395665
2. https://github.com/bitcoin/bitcoin/pull/25729#discussion_r940619491
3. https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-1135240825
4. https://github.com/bitcoin/bitcoin/issues/23144 (which is connected to #24845)
ACKs for top commit:
ishaanam:
crACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15
achow101:
ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15
aureleoules:
ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15
theStack:
ACK 76dc547ee7b05864e7b1b6c55fc0301d47aa3a15 🌇
Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
Aside from the cleanup, this solves a bug in the following-up commit. Because, in these
tests, we are manually adding/erasing outputs from the CoinsResult object but never
updating the internal total amount field.
so if there is an error in any of the pre-set coins, we can fail right away
without computing the wallet available coins set (calling `AvailableCoins`)
which is a slow operation as it goes through the entire wallet's txes map.
----------------------
And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions:
1) AutomaticCoinSelection.
2) SelectCoins.
1) AutomaticCoinSelection:
Receives a set of coins and selects the best subset of them to
cover the target amount.
2) SelectCoins
In charge of select all the user manually selected coins first ("pre-set inputs"), and
if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a
subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount
remaining value.
4fef5344288e454460b80db0316294e1ec1ad8ad wallet: use GetChange() when computing waste (S3RK)
87e0ef903133492e76b7c7556209554d4a0c3d66 wallet: use GetChange() in tx building (S3RK)
15e97a6886902ebb378829993a972dc52558aa92 wallet: add SelectionResult::GetChange (S3RK)
72cad28da05cfce9e4950f2dc5a709da41d251f4 wallet: calculate and store min_viable_change (S3RK)
e3210a722542a9cb5f7e4be72470dbe488c281fd wallet: account for preselected inputs in target (S3RK)
f8e796348b644c011ad9a8312356d4426c16cc4b wallet: add SelectionResult::Merge (S3RK)
06f558e4e2164d1916f258c731efe4586728a23b wallet: accurate SelectionResult::m_target (S3RK)
c8cf08ea743e430c2bf3fe46439594257b0937e5 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 4fef5344288e454460b80db0316294e1ec1ad8ad
Xekyo:
crACK 4fef5344288e454460b80db0316294e1ec1ad8ad
furszy:
Code review ACK 4fef5344
w0xlt:
ACK 4fef534428
Tree-SHA512: 31a7455d4129bc39a444da0f16ad478d690d4d9627b2b8fdb5605facc6488171926bf02f5d7d9a545b2b59efafcf5bb3d404005e4da15c7b44b3f7d441afb941
switch to new methods, remove old code. this also
updates the Size, All, and Clear methods to now use
the coins map.
this commit is not strictly a refactor because previously
coin selection was never run over the UNKNOWN type until the last
step when being run over all. now that we are iterating over each,
it is run over UNKNOWN but this is expected to be empty most of the time.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Rename `BResult` class to `util::Result` and update the class interface to be
more compatible with `std::optional` and with a full-featured result class
implemented in https://github.com/bitcoin/bitcoin/pull/25665. Motivation for
this change is to update existing `BResult` usages now so they don't have to
change later when more features are added in #25665.
This change makes the following improvements originally implemented in #25665:
- More explicit API. Drops potentially misleading `BResult` constructor that
treats any bilingual string argument as an error. Adds `util::Error`
constructor so it is never ambiguous when a result is being assigned an error
or non-error value.
- Better type compatibility. Supports `util::Result<bilingual_str>` return
values to hold translated messages which are not errors.
- More standard and consistent API. `util::Result` supports most of the same
operators and methods as `std::optional`. `BResult` had a less familiar
interface with `HasRes`/`GetObj`/`ReleaseObj` methods. The Result/Res/Obj
naming was also not internally consistent.
- Better code organization. Puts `src/util/` code in the `util::` namespace so
naming reflects code organization and it is obvious where the class is coming
from. Drops "B" from name because it is undocumented what it stands for
(bilingual?)
- Has unit tests.
Pass the whole CoinsResult struct to SelectCoins instead of only a
vector. This means we now have to remove preselected coins from each
OutputType vector and shuffle each vector individually.
Pass the whole CoinsResult struct to AttemptSelection. This involves
moving the logic in AttemptSelection to a newly named function,
ChooseSelectionResult. This will allow us to run ChooseSelectionResult
over each OutputType in a later commit. This ensures the backoffs work
properly.
Update unit and bench tests to use CoinResult.
111ea3ab711414236f8678566a7884d48619b2d8 wallet: refactor GetNewDestination, use BResult (furszy)
22351725bc4c5eb63ee45f607374bbf2d76e2b8c send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy)
198fcca162f4d2f877feab485e629ff89818ff56 wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy)
7a45c33d1f8a758850cf8e7bd6ad508939ba5c0d Introduce generic 'Result' class (furszy)
Pull request description:
Based on a common function signature pattern that we have all around the sources:
```cpp
bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
// do something...
if (error) {
error_string = "something bad happened";
return false;
}
result = goodResult;
return true;
}
```
Introduced a generic class `BResult` that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.
Obtaining in this way cleaner function signatures and removing boilerplate code:
```cpp
BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
// do something...
if (error) return "something bad happened";
return goodResult;
}
```
Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:
Before:
```cpp
Obj result_obj;
std::string error_string;
if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
LogPrintf("Error: %s", error_string);
}
return result_obj;
```
Now:
```cpp
BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
if (!op_res) {
LogPrintf("Error: %s", op_res.GetError());
}
return op_res.GetObjResult();
```
### Initial Implementation:
Have connected this new concept to two different flows for now:
1) The `CreateTransaction` flow. --> 7ba2b87c
2) The `GetNewDestination` flow. --> bcee0912
Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).
Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the `GetNewDestination` changes nor the inclusion of the `FeeCalculation` field inside `CreatedTransactionResult`).
ACKs for top commit:
achow101:
ACK 111ea3ab711414236f8678566a7884d48619b2d8
w0xlt:
reACK 111ea3ab71
theStack:
re-ACK 111ea3ab711414236f8678566a7884d48619b2d8
MarcoFalke:
review ACK 111ea3ab711414236f8678566a7884d48619b2d8 🎏
Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69
98ea43d5e9fc7b001f55a5bb1602afc2661cdb0f test: add tests for negative waste during coin selection (ishaanam)
Pull request description:
#25495 mentions that waste can be negative when the current feerate is less than the long term feerate. There are currently no waste tests for negative waste, so this PR adds two of them.
ACKs for top commit:
achow101:
ACK 98ea43d5e9fc7b001f55a5bb1602afc2661cdb0f
glozow:
light code review ACK 98ea43d5e9fc7b001f55a5bb1602afc2661cdb0f, good to have tests for negative waste
Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9 wallet: use CCoinControl to estimate signature size (S3RK)
a94659c84ee10ac5915eb5a6b654435183d88521 wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize (S3RK)
Pull request description:
Currently `DummySignTx` and `DummySignInput` use different ways to determine signature size.
This PR unifies the way wallet estimates signature size for various inputs.
Instead of passing boolean flags from calling code the `use_max_sig` is now calculated at the place of signature creation using information available in `CCoinControl`
ACKs for top commit:
achow101:
ACK d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9
theStack:
Code-review ACK d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9
Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
