fa875349e22f2f0f9c2c98ee991372d08ff90318 Fix iwyu (MacroFake)
faad673716cfbad1e715f1bdf8ac00938a055aea Fix issues when calling std::move(const&) (MacroFake)
Pull request description:
Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways:
* Remove the `const`, or
* Remove the `std::move`
ACKs for top commit:
ryanofsky:
Code review ACK fa875349e22f2f0f9c2c98ee991372d08ff90318. Looks good. Good for univalue to support c++11 move optimizations
Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
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
Instead of choosing whether to use the wallet or external data when
estimating the size of an input, first use the wallet, then try external
data if that failed.
If an external input's utxo was created by a transaction that the wallet
knows about, then it would not be selected using SelectExternal. This
results in either funding failure or incorrect weight calculation.
8cd21bb2799d37ed00dc9d0490bb5f5f1375932b refactor: improve readability for AttemptSelection (josibake)
f47ff717611182da27461e29b3c23933eb22fbce test: only run test for descriptor wallets (josibake)
0760ce0b9e646b6c86f4cc890c6ab78103a242ab test: add missing BOOST_ASSERT (josibake)
db09aec9378c5e8cc49c866fa50bfcb6c567d66c wallet: switch to new shuffle, erase, push_back (josibake)
b6b50b0f2b055d81c5d4ff9e21dd88cdc9a88ccb scripted-diff: Uppercase function names (josibake)
3f27a2adce12c6b0e7b43ba7c024331657bcf335 refactor: add new helper methods (josibake)
f5649db9d5e984ba7f376ccfd5b0a627f5c42402 refactor: add UNKNOWN OutputType (josibake)
Pull request description:
This PR is to address follow-ups for #24584, specifically:
* Remove redundant, hard-to-read code by adding a new `OutputType` and adding shuffle, erase, and push_back methods for `CoinsResult`
* Add missing `BOOST_ASSERT` to unit test
* Ensure functional test only runs if using descriptor wallets
* Improve readability of `AttemptSelection` by removing triple-nested if statement
Note for reviewers: commit `refactor: add new helper methods` should throw an "unused function warning"; the function is used in the next commit. Also, commit `wallet: switch to new shuffle, erase, push_back` will fail to compile, but this is fixed in the next commit with a scripted-diff. the commits are separate like this (code change then scripted-diff) to improve legibility.
ACKs for top commit:
achow101:
ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b
aureleoules:
ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b.
LarryRuane:
Concept, code review ACK 8cd21bb2799d37ed00dc9d0490bb5f5f1375932b
furszy:
utACK 8cd21bb2. Left a small, non-blocking, comment.
Tree-SHA512: a1bbc5962833e3df4f01a4895d8bd748cc4c608c3f296fd94e8afd8797b8d2e94e7bd44d598bd76fa5c9f5536864f396fcd097348fa0bb190a49a86b0917d60e
When we have preselected inputs the coin selection search target is reduced
by the sum of (effective) values. This causes incorrect m_target value.
Create separate instance of SelectionResult for all the preselected inputs and
set the target equal to the sum of (effective) values. Target for preselected
SelectionResult is equal to the delta for the search target. To get the final
SelectionResult with accurate m_target we merge both SelectionResult instances.
SelectionResult::m_target should be equal to actual selection target.
Selection target is the sum of all recipient amounts plus non input fees.
So we need to remove change_fee from the m_target. It's safe because change
target is always greater than the change fee, so we can always cover fees
if change output is created.
it was pointed out by a few reviewers that the code block at the end
of attempt selection was difficult to follow and lacked comments.
refactor to get rid of triple nested if statement and improve
readibility.
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>
add Shuffle, Erase, and Add to CoinsResult struct
add a helper function for mapping TxoutType to OutputType
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
76b3c37fcb93b4bcb047e0500fdaa605160e25d5 refactor: wallet: return util::Result from `GetReservedDestination` methods (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #25218, as suggested in comment https://github.com/bitcoin/bitcoin/pull/25218#discussion_r907710067. The interfaces of the methods `ReserveDestination::GetReservedDestination`, `{Legacy,Descriptor,}ScriptPubKeyMan::GetReservedDestination` are improved by returning `util::Result<CTxDestination>` instead of `bool` in order to get rid of the two `CTxDestination&` and `bilingual_str&` out-parameters.
ACKs for top commit:
furszy:
ACK 76b3c37f
Tree-SHA512: bf15560a88d645bcf8768024013d36012cd65caaa4a613e8a055dfd8f29cb4a219c19084606992bad177920cdca3a732ec168e9b9526f9295491f2cf79cc6815
bc886fcb31e1afa7bbf7b86bfd93e51da7076ccf Change mapWallet to be a std::unordered_map (Andrew Chow)
272356024db978c92112167f8d8e4cc62adad63d Change getWalletTxs to return a set instead of a vector (Andrew Chow)
97532867cf51db3e941231fbdc60f9f4fa0012a0 Change mapTxSpends to be a std::unordered_multimap (Andrew Chow)
1f798fe85ba952273005f68e36ed48cfc36f4c9d wallet: Cache SigningProviders (Andrew Chow)
8a105ecd1aeff15f84c3883e2762bf71ad59d920 wallet: Use CalculateMaximumSignedInputSize to indicate solvability (Andrew Chow)
Pull request description:
While running my coin selection simulations, I noticed that towards the end of the simulation, the wallet would become slow to make new transactions. The wallet generally performs much more slowly when there are a large number of transactions and/or a large number of keys. The improvements here are focused on wallets with a large number of transactions as that is what the simulations produce.
Most of the slowdown I observed was due to `DescriptorScriptPubKeyMan::GetSigningProvider` re-deriving keys every time it is called. To avoid this, it will now cache the `SigningProvider` produced so that repeatedly fetching the `SigningProvider` for the same script will not result in the same key being derived over and over. This has a side effect of making the function non-const, which makes a lot of other functions non-const as well. This helps with wallets with lots of address reuse (as my coin selection simulations are), but not if addresses are not reused as keys will end up needing to be derived the first time `GetSigningProvider` is called for a script.
The `GetSigningProvider` problem was also exacerbated by unnecessarily fetching a `SigningProvider` for the same script multiple times. A `SigningProvider` is retrieved to be used inside of `IsSolvable`. A few lines later, we use `GetTxSpendSize` which fetches a `SigningProvider` and then calls `CalculateMaximumSignedInputSize`. We can avoid a second call to `GetSigningProvider` by using `CalculateMaximumSignedInputSize` directly with the `SigningProvider` already retrieved for `IsSolvable`.
There is an additional slowdown where `ProduceSignature` with a dummy signer is called twice for each output. The first time is `IsSolvable` checks that `ProduceSignature` succeeds, thereby informing whether we have solving data. The second is `CalculateMaximumSignedInputSize` which returns -1 if `ProduceSignature` fails, and returns the input size otherwise. We can reduce this to one call of `ProduceSignature` by using `CalculateMaximumSignedInputSize`'s result to set `solvable`.
Lastly, a lot of time is spent looking in `mapWallet` and `mapTxSpends` to determine whether an output is already spent. The performance of these lookups is slightly improved by changing those maps to use `std::unordered_map` and `std::unordered_multimap` respectively.
ACKs for top commit:
Xekyo:
ACK bc886fcb31e1afa7bbf7b86bfd93e51da7076ccf
furszy:
diff re-reACK bc886fcb
Tree-SHA512: fd710fe1224ef67d2bb83d6ac9e7428d9f76a67f14085915f9d80e1a492d2c51cb912edfcaad1db11c2edf8d2d97eb7ddd95bfb364587fb1f143490fd72c9ec1
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.
In AvailableCoins, we need to know whether we can solve for an output.
This was done by using IsSolvable, which just calls ProduceSignature and
produces a dummy signature. However, we already do that in order to get
the size of the input by using CalculateMaximumSignedInputSize. As this
function returns -1 if ProduceSignature fails, we can just remove the
use of IsSolvable and check that input_bytes is not -1 to determine
the solvability of an output.
Run coin selection on each OutputType separately, choosing the best
solution according to the waste metric.
This is to avoid mixing UTXOs that are of different OutputTypes,
which can hurt privacy.
If no single OutputType can fund the transaction, then coin selection
considers the entire wallet, potentially mixing (current behavior).
This is done inside AttemptSelection so that all OutputTypes are
considered at each back-off in coin selection.
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
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
Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins.
Full explanation is inside #25118 comments but brief summary is:
`vCoins` at `SelectCoins` time could not be containing the manually selected input because, even when they were selected by the user, the current `AvailableCoins` flow skips locked and spent coins.
Extra note: this is an intermediate step to unify the `fAllowOtherInputs`/`m_add_inputs` concepts. It will not be a problem anymore in the future when we finally decouple the wtx-outputs lookup process from `SelectCoins` and don't skip the user's manually selected coins in `AvailableCoins`.
Seeking to make the `CoinControl` option less confusing/redundant.
In #16377 the `CoinControl` flag ‘m_add_inputs’ was added to tell the coin filtering and selection process two things:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: Search the wtxs-outputs and append all the `CoinControl` selected outpoints to the selection result (skipping all the available output checks). Nothing else.
Meanwhile, in `CoinControl` we already have a flag ‘fAllowOtherInputs’ which is already saying:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: If false, no selection process -> append all the `CoinControl` selected outpoints to the selection result (while they passed all the `AvailableCoins` checks and are available in the 'vCoins' vector).
As can notice, the first point in the coin filtering process is duplicated in the two option flags. And the second one, is slightly different merely because it takes into account whether the coin is on the `AvailableCoins` vector or not.
So it makes sense to merge ‘m_add_inputs’ and ‘fAllowOtherInputs’ into a single field for the coin filtering process while introduce other changes to add the missing/skipped coins into 'vCoins' vector if they were manually selected by the user (follow-up commits).
fd5c996d1609e6f88769f6f3ef0c322e3435b3aa wallet: GetAvailableBalance, remove double walk-through every available coin (furszy)
162d4ad10f28c5fa38551d69ce9b296ab3933c77 wallet: add 'only_spendable' filter to AvailableCoins (furszy)
cdf185ccfb2085e5a4bf82d833392d74b748aeff wallet: remove unused IsSpentKey(hash, index) method (furszy)
4b83bf8dbcf6b8b1c1293575391e90ac7e21b0e0 wallet: avoid extra IsSpentKey -> GetWalletTx lookups (furszy)
3d8a2822570e3cf4d1bc4f9d59b5dcb0145920ad wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) (furszy)
a06fa94ff81e2bccef0316ea5ec4eca0f4de5071 wallet: IsSpent, 'COutPoint' arg instead of (hash, index) (furszy)
91902b77202fc636edb3db587cb6e87d9fb9b60a wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) (furszy)
9472ca0a65396206b3078bddf98f4c1807be2d82 wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times (furszy)
4ce235ef8f9a9dddc52d7ab60c8f71bda1d38873 wallet: return 'CoinsResult' struct in `AvailableCoins` (furszy)
Pull request description:
This started in #24845 but grew out of scope of it.
So, points tackled:
1) Avoid extra `GetWalletTx` lookups inside `AvailableCoins -> IsSpentKey`.
`IsSpentKey` was receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all the `IsSpentKey` function callers already have the wtx available, them can provide the `scriptPubKey` directly.
2) Most of the time, we call `Wallet::AvailableCoins`, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by "only_spendable" coins inside `Wallet::AvailableCoins` directly.
(the non-spendable coins skip examples are inside `AttemptSelection->GroupOutputs` and `GetAvailableBalance`).
4) Refactored `AvailableCoins` in several ways:
a) Now it will return a new struct `CoinsResult` instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from #24845 but cherry-picked it here too to make the following commits look nicer.
b) Unified all the 'wtx.tx->vout[I]' calls into a single call (coming from this comment https://github.com/bitcoin/bitcoin/pull/24699#discussion_r854163032).
5) The wallet `IsLockedCoin` and `IsSpent` methods now accept an `OutPoint` instead of a hash:index. Which let me cleanup a bunch of extra code.
6) Speeded up the wallet 'GetAvailableBalance': filtering `AvailableCoins` by spendable outputs only and using the 'AvailableCoins' retrieved `total_amount` instead of looping over all the retrieved coins once more.
-------------------------------------------------------
Side topic, all this process will look even nicer with #25218
ACKs for top commit:
achow101:
ACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa
brunoerg:
crACK fd5c996d1609e6f88769f6f3ef0c322e3435b3aa
w0xlt:
Code Review ACK fd5c996d16
Tree-SHA512: 376a85476f907f4f7d1fc3de74b3dbe159b8cc24687374d8739711ad202ea07a33e86f4e66dece836da3ae6985147119fe584f6e672f11d0450ba6bd165b3220
7832e9438f5c66b88f60676d14e1e11d669eb109 test: fundrawtransaction preset input weight calculation (S3RK)
c3981e379fa088aa7aa03b2f505342a5b3bc3436 wallet: do not count wallet utxos as external (S3RK)
Pull request description:
Correctly differentiating between external vs non-external utxos in coin control produces more accurate weight and fee estimations.
Weight for external utxos is estimated based on the maximum signature size, while for the wallet utxos we expect minimal signature due to signature grinding.
ACKs for top commit:
achow101:
re-ACK 7832e9438f5c66b88f60676d14e1e11d669eb109
Xekyo:
re-ACK 7832e9438f5c66b88f60676d14e1e11d669eb109
furszy:
ACK 7832e943
Tree-SHA512: bb5635b0bd85fa9a76922a53ad3fa062286424c06a695a0e87407c665713e80a33555b644fbb13bcc1ab503dcd7f53aacbdc368d69ac0ecff8005603623ac94f
We are skipping the non-spendable coins that appear in vCoins ('AvailableCoins' result) later, in several parts of the CreateTransaction and GetBalance flows:
GetAvailableBalance (1) gets all the available coins calling AvailableCoins and, right away, walk through the entire vector, skipping the non-spendable coins, to calculate the total balance.
Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins.
So, Purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times.
Note: this speedup is for all the processes that call to CreateTransaction and GetBalance* internally.
