aboutsummaryrefslogtreecommitdiff
path: root/src/wallet/spend.cpp
AgeCommit message (Collapse)Author
2023-03-08wallet: APS, don't create empty groupsfurszy
By moving the "positive-only" flag out of the lambda function.
2023-03-08wallet: do not make two COutputs, use shared_ptrfurszy
2023-03-07wallet: refactor coin selection algos to return util::Resultfurszy
so the selection processes can retrieve different errors and not uninformative std::nullopt
2023-03-06wallet: single output groups filtering and grouping processfurszy
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).
2023-03-06wallet: unify outputs grouping processfurszy
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).
2023-03-06wallet: decouple outputs grouping process from each ChooseSelectionResultfurszy
Another step towards the single OutputGroups calculation goal
2023-03-06refactor: make OutputGroup::m_outputs field a vector of shared_ptrfurszy
Initial steps towards sharing COutput instances across all possible OutputGroups (instead of copying them time after time).
2023-03-03wallet: make OutputGroup "positive_only" filter explicitfurszy
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.
2023-02-23wallet: skip R-value grinding for external signersSjors Provoost
When producing a dummy signature for the purpose of estimating the transaction fee, do not assume an external signer performs R-value grinding on the signature. In particular, this avoids a scenario where the fee rate is 1 sat / vbyte and a transaction with a 72 byte signature is not accepted into our mempool. This commit also drops the nullptr default for CCoinControl arguments for functions that it touches. This is because having a boolean argument right next to an optional pointer is error prone. Co-Authored-By: S3RK <1466284+S3RK@users.noreply.github.com>
2023-02-20Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX ↵fanquake
with avoidpartialspends 14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin) Pull request description: Closes https://github.com/bitcoin/bitcoin/issues/27051 When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain. If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected. I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582 ACKs for top commit: achow101: ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
2023-02-16Merge bitcoin/bitcoin#24149: Signing support for Miniscript Descriptorsfanquake
6c7a17a8e0eec377f83ed1399f003ae70b898270 psbt: support externally provided preimages for Miniscript satisfaction (Antoine Poinsot) 840a396029316896beda46600aec3c1af09a899c qa: add a "smart" Miniscript fuzz target (Antoine Poinsot) 17e3547241d593bc92c5c6b36c54284d9d9f3feb qa: add a fuzz target generating random nodes from a binary encoding (Antoine Poinsot) 611e12502a5887ffb751bb92fadaa334d484824b qa: functional test Miniscript signing with key and timelocks (Antoine Poinsot) d57b7f2021d2369f6e88cdf0f562aab27c51beaf refactor: make descriptors in Miniscript functional test more readable (Antoine Poinsot) 0a8fc9e200b5018c1efd6f9126eb405ca0beeea3 wallet: check solvability using descriptor in AvailableCoins (Antoine Poinsot) 560e62b1e221832ae99ff8684559a7b8f9df84a7 script/sign: signing support for Miniscripts with hash preimage challenges (Antoine Poinsot) a2f81b6a8f1ff3b0750711409c7538812a52ef40 script/sign: signing support for Miniscript with timelocks (Antoine Poinsot) 61c6d1a8440db09c44d7fd367a6f2c641ea93d40 script/sign: basic signing support for Miniscript descriptors (Antoine Poinsot) 4242c1c52127df3a24be0c15b88d4fc463af04fc Align 'e' property of or_d and andor with website spec (Pieter Wuille) f5deb417804b9f267830bd40177677987df4526d Various additional explanations of the satisfaction logic from Pieter (Pieter Wuille) 22c5b00345063bdeb8b6d3da8b5692d18f92bfb7 miniscript: satisfaction support (Antoine Poinsot) Pull request description: This makes the Miniscript descriptors solvable. Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR. ACKs for top commit: achow101: ACK 6c7a17a8e0eec377f83ed1399f003ae70b898270 sipa: utACK 6c7a17a8e0eec377f83ed1399f003ae70b898270 (to the extent that it's not my own code). Tree-SHA512: a71ec002aaf66bd429012caa338fc58384067bcd2f453a46e21d381ed1bacc8e57afb9db57c0fb4bf40de43b30808815e9ebc0ae1fbd9e61df0e7b91a17771cc
2023-02-15wallet: reuse change dest when recreating TX with avoidpartialspendsMatthew Zipkin
2023-02-11wallet: check solvability using descriptor in AvailableCoinsAntoine Poinsot
This is a workaround for Miniscript descriptors containing hash challenges. For those we can't mock the signature creator without making OP_EQUAL mockable in the interpreter, so CalculateMaximumInputSize will always return -1 and outputs for these descriptors would appear unsolvable while they actually are.
2023-01-18Merge bitcoin/bitcoin#25659: wallet: simplify ListCoins implementationAndrew Chow
a2ac6f9582c4c996fa36e4801fa0aac756235754 wallet: unify FindNonChangeParentOutput functions (furszy) b3f4e827378e010cd2a5d1b876d01db52c054d26 wallet: simplify ListCoins implementation (furszy) Pull request description: Focused on the following changes: 1) Removed the entire locked coins lookup that was inside `ListCoins` by including them directly on the `AvailableCoins` result (where we were skipping them before). 2) Unified both `FindNonChangeParentOutput` functions (only called from `ListCoins`) ACKs for top commit: achow101: ACK a2ac6f9582c4c996fa36e4801fa0aac756235754 aureleoules: ACK a2ac6f9582c4c996fa36e4801fa0aac756235754, LGTM theStack: Code-review ACK a2ac6f9582c4c996fa36e4801fa0aac756235754 Tree-SHA512: f72b21ee1600c5992559b5dcd8ff260527afac2ec696737f998343a0850b84d439e7f86ea52a14cc0cddabf132a30bf5b52fb34950578ac323083d4375b937f1
2023-01-03refactor: use braced init for integer constants instead of c style castsPasta
2023-01-03Merge bitcoin/bitcoin#26661: wallet: Coin Selection, return accurate error ↵Andrew Chow
messages 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 :city_sunrise: Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
2023-01-03wallet: unify FindNonChangeParentOutput functionsfurszy
The function is only used in ListCoins.
2023-01-03wallet: simplify ListCoins implementationfurszy
Can remove the locked coins lookup if we include them directly inside the AvailableCoins result
2023-01-03Merge bitcoin/bitcoin#25789: test: clean and extend availablecoins_tests ↵Andrew Chow
coverage 9622fe64b8785430c71d4abc8637075026dc690c test: move coins result test to wallet_tests.cpp (furszy) f69347d0588647ff9a4e986c7be987827a0417f4 test: extend and simplify availablecoins_tests (furszy) 212ccdf2c2b70d973b18ae78f0158ec5f0c3bbb4 wallet: AvailableCoins, add arg to include/skip locked coins (furszy) Pull request description: Negative PR with extended test coverage :). 1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result. 2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`. So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`. 3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount. Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well. ACKs for top commit: achow101: ACK 9622fe64b8785430c71d4abc8637075026dc690c theStack: ACK 9622fe64b8785430c71d4abc8637075026dc690c aureleoules: reACK 9622fe64b8785430c71d4abc8637075026dc690c, nice cleanup! Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388
2022-12-24scripted-diff: Bump copyright headersHennadii Stepanov
-BEGIN VERIFY SCRIPT- ./contrib/devtools/copyright_header.py update ./ -END VERIFY SCRIPT- Commits of previous years: - 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7 - 2020: fa0074e2d82928016a43ca408717154a1c70a4db - 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
2022-12-21wallet: return accurate error messages from Coin Selectionfurszy
and not the general "Insufficient funds" when the wallet actually have funds. Two new error messages: 1) If the selection result exceeds the maximum transaction weight, we now will return: "The inputs size exceeds the maximum weight". 2) If the user preselected 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".
2022-12-21wallet: make SelectCoins flow return util::Resultfurszy
2022-12-21wallet: refactor eight consecutive 'AttemptSelection' calls into a loopfurszy
and remove 'CoinEligibilityFilter' default constructor to prevent mistakes.
2022-12-14Merge bitcoin/bitcoin#26668: wallet: if only have one output type, don't ↵Andrew Chow
perform "mixed" coin selection 89c1491d35389eac0c1fecc59333cdfae3b1bd2c wallet: if only have one output type, don't perform "mixed" coin selection (furszy) Pull request description: For wallets that only have one output type, we are currently performing the same selection process over the same coins twice. The "mixed coin selection" doesn't add any value to the result (there is nothing to mix if the available coins struct has only one type). ACKs for top commit: achow101: ACK 89c1491d35389eac0c1fecc59333cdfae3b1bd2c john-moffett: ACK 89c1491d35389eac0c1fecc59333cdfae3b1bd2c kristapsk: cr utACK 89c1491d35389eac0c1fecc59333cdfae3b1bd2c Tree-SHA512: 672eaeed3ba911d13fa61a46f719c8fe1ebe4d2dc7d723040e71937c693659411bc99cdbd9f0014e836b70eebeff1b8ca861f4d81d39e6f79f437364a526edbe
2022-12-09wallet: Sanity check fee paid cannot be negativeAndrew Chow
We need to check that the fee is not negative even before it is finalized. The setting of fees for SFFO may adjust the fee to be "correct" and no longer negative, but erroneously reduce the amounts too far. So we need to check this condition before we do those adjustments.
2022-12-09wallet: Move fee underpayment check to after fee settingAndrew Chow
It doesn't make sense to be checking whether the fee paid is underpaying before we've finished setting the fees. So do that after we have done the reduction for SFFO and change adjustment for fee overpayment.
2022-12-08wallet: if only have one output type, don't perform "mixed" coin selectionfurszy
there is nothing to mix.
2022-12-06wallet: Rename nFeeRet in CreateTransactionInternal to current_feeAndrew Chow
nFeeRet represents the fee that the transaction currently pays. Update it's name to reflect that.
2022-12-06Merge bitcoin/bitcoin#25729: wallet: Check max transaction weight in ↵Andrew Chow
CoinSelection c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 test: Check max transaction weight in CoinSelection (Aurèle Oulès) 6b563cae92957dc30dc35103a7c321fdb0115ef3 wallet: Check max tx weight in coin selector (Aurèle Oulès) Pull request description: This PR is an attempt to fix #5782. I have added 4 test scenarios, 3 of them provided here https://github.com/bitcoin/bitcoin/issues/5782#issuecomment-73819058 (slightly modified to use a segwit wallet). Here are my benchmarks : ## PR | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,466,341.00 | 681.97 | 0.6% | 11,176,762.00 | 3,358,752.00 | 3.328 | 1,897,839.00 | 0.3% | 0.02 | `CoinSelection` ## Master | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 1,526,029.00 | 655.30 | 0.5% | 11,142,188.00 | 3,499,200.00 | 3.184 | 1,994,156.00 | 0.2% | 0.02 | `CoinSelection` ACKs for top commit: achow101: reACK c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25729/commits/c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 furszy: diff ACK c7c7ee9d Tree-SHA512: ef0b28576ff845174651ba494aa9adee234c96e6f886d0e032eceb7050296e45b099dda0039d1dfb9944469f067627b2101f3ff855c70353cf39d1fc7ee81828
2022-12-06Merge bitcoin/bitcoin#26238: clang-tidy: fixup named argument commentsMarcoFalke
203886c443c4ad76f8a1dba740a286e383e55206 Fixup clang-tidy named argument comments (fanquake) Pull request description: Fix comments so they are checked/consistent. Fix incorrect comments. ACKs for top commit: hebasto: ACK 203886c443c4ad76f8a1dba740a286e383e55206, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: e1257840f91fe39842e2b19299c1633604697b8584fe44b1977ada33cdde5433c877ed0b669fa334e20b04971dc89cd47d58b2783b6f7004521f01d05a1245da
2022-12-06Merge bitcoin/bitcoin#26611: wallet: Change coin selection fee assert to errorMarcoFalke
3eb041f014870954db564369a4be4bd0dea48fbe wallet: Change coin selection fee assert to error (Andrew Chow) c6e7f224c119f47af250a9e0c5b185cb98b30c4c util: Add StrFormatInternalBug and STR_INTERNAL_BUG (MarcoFalke) Pull request description: Returning an error instead of asserting for the low fee check will be better as it does not crash the node and instructs users to report the bug. ACKs for top commit: S3RK: ACK 3eb041f014870954db564369a4be4bd0dea48fbe aureleoules: ACK 3eb041f014870954db564369a4be4bd0dea48fbe furszy: ACK 3eb041f0 Tree-SHA512: 118c13d7cdfce492080edd4cb12e6d960695377b978c7573f9c58b6d918664afd0e8e591eed0605d08ac756fa8eceed456349de5f3a025174069abf369bb5a5f
2022-12-05wallet: Check max tx weight in coin selectorAurèle Oulès
Co-authored-by: Andrew Chow <github@achow101.com>
2022-12-05wallet: Change coin selection fee assert to errorAndrew Chow
Returning an error instead of asserting for the low fee check will be better as it does not crash the node and instructs users to report the bug.
2022-12-05Fixup clang-tidy named argument commentsfanquake
Fix comments so they are checked/consistent. Fix incorrect arguments.
2022-12-02refactor: make CoinsResult total amounts members privatefurszy
2022-12-02wallet: SelectCoins, return early if wallet's UTXOs cannot cover the targetfurszy
The CoinsResult class will now count the raw total amount and the effective total amount internally (inside the 'CoinsResult::Add' and 'CoinsResult::Erase' methods). So there is no discrepancy between what we add/erase and the total values. (which is what was happening on the coinselector_test because the 'CoinsResult' object is manually created there, and we were not keeping the total amount in sync with the outputs being added/removed).
2022-11-29wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the setfurszy
The loop break shouldn't have being there.
2022-11-16wallet: AvailableCoins, add arg to include/skip locked coinsfurszy
2022-10-29scripted-diff: wallet: rename AvailableCoinsParams members to snake_casefurszy
-BEGIN VERIFY SCRIPT- sed -i 's/nMinimumAmount/min_amount/g' $(git grep -l nMinimumAmount) sed -i 's/nMaximumAmount/max_amount/g' $(git grep -l nMaximumAmount) sed -i 's/nMinimumSumAmount/min_sum_amount/g' $(git grep -l nMinimumSumAmount) sed -i 's/nMaximumCount/max_count/g' $(git grep -l nMaximumCount) -END VERIFY SCRIPT-
2022-10-29wallet: group AvailableCoins filtering parameters in a single structfurszy
Plus clean callers that use the params default values
2022-10-29RPC: listunspent, add "include immature coinbase" flagfurszy
so we can return the immature coinbase UTXOs as well.
2022-10-26wallet: SelectCoins, return early if target is covered by preset-inputsfurszy
2022-10-26wallet: simplify preset inputs selection target checkfurszy
we are already computing the preset inputs total amount inside `PreSelectedInputs::Insert`, which internally decides whether to use the effective value or the raw output value based on the 'subtract_fee_outputs' flag.
2022-10-26wallet: remove fetch pre-selected-inputs responsibility from SelectCoinsfurszy
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.
2022-10-26wallet: encapsulate pre-selected-inputs lookup into its own functionfurszy
First step towards decoupling the pre-selected-inputs fetching functionality from `SelectCoins`. Which, will let us not waste resources calculating the available coins if one of the pre-set inputs has an error. (right now, if one of the pre-set inputs is invalid, we first walk through the entire wallet txes map just to end up failing right after it finish)
2022-10-26wallet: skip manually selected coins from 'AvailableCoins' resultfurszy
No need to walk through the entire wallet's txes map just to get coins that we could have gotten by just doing a simple map.find(out.hash). (Which is what we are doing inside `SelectCoins` anyway)
2022-10-26wallet: skip available coins fetch if "other inputs" are disallowedfurszy
no need to waste resources calculating the wallet available coins if they are not going to be used. The 'm_allow_other_inputs=true` default value change is to correct an ugly misleading behavior: The tx creation process was having a workaround patch to automatically fall back to select coins from the wallet if `m_allow_other_inputs=false` (previous default value) and no manual inputs were selected. This could be seen in master in flows like `sendtoaddress`, `sendmany` and even the GUI, where the `m_allow_other_inputs` value isn't customized and the wallet still selects and adds coins to the tx internally.
2022-10-06refactor: wallet, do not translate init arguments namesfurszy
2022-10-02wallet: Use correct effective value when checking targetAurèle Oulès
2022-09-21Merge bitcoin/bitcoin#25933: wallet: AvailableCoins, simplify output script ↵Andrew Chow
type acquisition 58b7df3caa21519de61e10f6ee42f0be9ac3cc30 wallet: AvailableCoins, simplify output script type acquisition (furszy) Pull request description: There is an unnecessary `ExtractDestination()` call and subsequent result parse into an `CScriptID`. The `Solver()` call, which we are already doing below anyway, retrieves the script type and, in the P2SH case, the program id. ACKs for top commit: achow101: ACK 58b7df3caa21519de61e10f6ee42f0be9ac3cc30 aureleoules: re-ACK 58b7df3caa21519de61e10f6ee42f0be9ac3cc30 rajarshimaitra: ACK 58b7df3caa21519de61e10f6ee42f0be9ac3cc30 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25933/commits/58b7df3caa21519de61e10f6ee42f0be9ac3cc30 Tree-SHA512: 51080766877c34cb2232ee3a1cb6b6a62b829c9297c67b99577742b94854a737a74d248015a4603ca9b6cd0a3c9e1d6d78673ff3cc9fc65dd82deea72dc537fd
generated by cgit v1.2.3 (git 2.26.2) at 2024-10-16 22:30:24 +0000