aboutsummaryrefslogtreecommitdiff
path: root/src/wallet/test
AgeCommit message (Collapse)Author
2023-04-11wallet: Add wallet/types.h for simple public enum and struct typesRyan Ofsky
Move isminetype and isminefilter there this commit, add WalletPurpose type next commit.
2023-04-11Merge bitcoin/bitcoin#26699: wallet, gui: bugfix, getAvailableBalance skips ↵Andrew Chow
selected coins 68eed5df8656bed1be6526b014e58d3123102b03 test,gui: add coverage for PSBT creation on legacy watch-only wallets (furszy) 306aab5bb471904faed325d9f3b38b7e891c7bbb test,gui: decouple widgets and model into a MiniGui struct (furszy) 2f76ac0383904123676f1b4eeba0f772a4c5cb5d test,gui: decouple chain and wallet initialization from test case (furszy) cd98b717398f7b13ace91ea9efac9ce1e60b4d62 gui: 'getAvailableBalance', include watch only balance (furszy) 74eac3a82fc948467d5a15a5af420b36ce8eb04a test: add coverage for 'useAvailableBalance' functionality (furszy) dc1cc1c35995dc09085b3d9270c445b7923fdb51 gui: bugfix, getAvailableBalance skips selected coins (furszy) Pull request description: Fixes https://github.com/bitcoin-core/gui/issues/688 and https://github.com/bitcoin/bitcoin/issues/26687. First Issue Description (https://github.com/bitcoin-core/gui/issues/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 #25685. Context: Since #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 https://github.com/bitcoin-core/gui/pull/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 (https://github.com/bitcoin/bitcoin/issues/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 68eed5df8656bed1be6526b014e58d3123102b03 achow101: ACK 68eed5df8656bed1be6526b014e58d3123102b03 theStack: ACK 68eed5df8656bed1be6526b014e58d3123102b03 Tree-SHA512: 674f3e050024dabda2ff4a04b9ed3750cf54a040527204c920e1e38bd3d7f5fd4d096e4fd08a0fea84ee6abb5070f022b5c0d450c58fd30202ef05ebfd7af6d3
2023-04-03gui: bugfix, getAvailableBalance skips selected coinsfurszy
The previous behavior for getAvailableBalance when coin control has 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. This turns into a GUI-only issue for the "use available balance" button when the user manually select coins in the send screen. Reason: We missed to update the GetAvailableBalance function to include the coin control selected coins on #25685. Context: Since #25685 we skip the selected coins inside `AvailableCoins`, the reason is that there is no need to traverse the wallet's txes map just to get coins that can directly be fetched by their id.
2023-03-23refactor: Move fs.* to util/fs.*TheCharlatan
The fs.* files are already part of the libbitcoin_util library. With the introduction of the fs_helpers.* it makes sense to move fs.* into the util/ directory as well.
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-06test: coinselector_tests refactor, use CoinsResult instead of plain std::vectorfurszy
No functional changes. Only cosmetic changes to simplify the follow-up commit.
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-06test: wallet, add coverage for outputs grouping processfurszy
The following scenarios are covered: 1) 10 UTXO with the same script: partial spends is enabled --> outputs must not be grouped. 2) 10 UTXO with the same script: partial spends disabled --> outputs must be grouped. 3) 20 UTXO, 10 one from scriptA + 10 from scriptB: a) if partial spends is enabled --> outputs must not be grouped. b) if partial spends is not enabled --> 2 output groups expected (one per script). 3) Try to add a negative output (value - fee < 0): a) if "positive_only" is enabled --> negative output must be skipped. b) if "positive_only" is disabled --> negative output must be added. 4) Try to add a non-eligible UTXO (due not fulfilling the min depth target for "not mine" UTXOs) --> it must not be added to any group 5) Try to add a non-eligible UTXO (due not fulfilling the min depth target for "mine" UTXOs) --> it must not be added to any group 6) Surpass the 'OUTPUT_GROUP_MAX_ENTRIES' size and verify that a second partial group gets created.
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-17Merge bitcoin/bitcoin#26940: test: create random and coins utils, add amount ↵Andrew Chow
helper, dedupe add_coin 4275195606e6f42466d9a8ef766b3035833df4d5 De-duplicate add_coin methods to a test util helper (Jon Atack) 9d92c3d7f42c18939a9a6aa1ee185f1c958360a0 Create InsecureRandMoneyAmount() test util helper (Jon Atack) 81f5ade2a324167c03c5ce765a26bd42ed652723 Move random test util code from setup_common to random (Jon Atack) Pull request description: - Move random test utilities from `setup_common` to a new `random` file, as many tests don't use this code. - Create a helper to generate semi-random CAmounts up to `MONEY_RANGE` rather than only uint32, and use the helper in the unit tests. - De-duplicate a shared `add_coin` method by extracting it to a `coins` test utility. ACKs for top commit: pinheadmz: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 achow101: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 john-moffett: ACK 4275195606e6f42466d9a8ef766b3035833df4d5 Tree-SHA512: 3ed974251149c7417f935ef2f8865aa0dcc33b281b47522b0f96f1979dff94bb8527957f098fe4d210f40d715c00f29512f2ffe189097102229023b7284a3a27
2023-02-17Merge bitcoin/bitcoin#26889: refactor: wallet, remove global 'ArgsManager' ↵Andrew Chow
dependency 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
2023-02-15refactor: wallet, remove global 'ArgsManager' accessfurszy
we are not using it anymore
2023-02-15wallet: set keypool_size instead of access global args managerfurszy
2023-02-06Move random test util code from setup_common to randomJon Atack
as many of the unit tests don't use this code
2023-02-03Merge bitcoin/bitcoin#25966: test: Remove redundant testAndrew Chow
fb1c6c14c11ccd4833c1a24f77c507f098d369ad test: Remove redundant test (yancy) Pull request description: I can't think of any reason to keep this test case around labeled [fix me](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L242). The test was originally added [here](https://github.com/bitcoin/bitcoin/commit/4566ab75f277612425337bf7786c1d3a410d894a) however there was never an assertion about the coins that should be selected, only that a solution is found (which is a redundant solution to the test [above](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L222)). The comment was later added here to [fix](https://github.com/bitcoin/bitcoin/commit/384273260a6ccbcf79dade0830011f528e5a1581) it, however it's unclear what exactly it's testing. A test was later added [here](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L366) where if the [long term fee](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L357) is less than the current [fee](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L356), then select fewer UTXOs, which may have been the original intent. ACKs for top commit: S3RK: ACK fb1c6c14c11ccd4833c1a24f77c507f098d369ad Zero-1729: Concept ACK fb1c6c14c11ccd4833c1a24f77c507f098d369ad achow101: ACK fb1c6c14c11ccd4833c1a24f77c507f098d369ad Tree-SHA512: bce2cdae669c144ffaa130237a1643e3b6728e13d603cebf5d9493c4c7c68b3635868e4d93d210783c2ded2a871f185ca09a2053168c05b26a1e056ff6edf68f
2023-02-01Fix clang-tidy readability-const-return-type violationsMarcoFalke
2023-01-26Use DataStream where possibleMarcoFalke
2023-01-23Merge bitcoin/bitcoin#26690: wallet: Refactor database cursor into its own ↵fanquake
object with proper return codes 4aebd832a405090c2608e4b60bb4f34501bcea61 db: Change DatabaseCursor::Next to return status enum (Andrew Chow) d79e8dcf2981ef1964a2fde8c472b5de1ca1c963 wallet: Have cursor users use DatabaseCursor directly (Andrew Chow) 7a198bba0a1d0a0f0fd4ca947955cb52b84bdd4b wallet: Introduce DatabaseCursor RAII class for managing cursor (Andrew Chow) 69efbc011bb74fcd8dd9ed2a8a5d31bc9e323c10 Move SafeDbt out of BerkeleyBatch (Andrew Chow) Pull request description: Instead of having database cursors be tied to a particular `DatabaseBatch` object and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors. Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the `Next` function (formerly `ReadAtCursor`) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read. Extracted from #24914 ACKs for top commit: furszy: diff ACK 4aebd83 theStack: Code-review ACK 4aebd832a405090c2608e4b60bb4f34501bcea61 Tree-SHA512: 5d0be56a18de5b08c777dd5a73ba5a6ef1e696fdb07d1dca952a88ded07887b7c5c04342f9a76feb2f6fe24a45dc31f094f1f5d9500e6bdf4a44f4edb66dcaa1
2023-01-11Merge bitcoin/bitcoin#26758: refactor: Add `performance-no-automatic-move` ↵MarcoFalke
clang-tidy check 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
2023-01-05Merge bitcoin/bitcoin#23829: refactor: use braced init for integer literals ↵MarcoFalke
instead of c style casts 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
2023-01-04Merge bitcoin/bitcoin#26752: wallet: Remove `mempool_sequence` from ↵glozow
interface methods 55696a0ac30bcfbd555f71cbc8eac23b725f7dcf wallet: remove `mempool_sequence` from `transactionRemovedFromMempool` (w0xlt) bf19069c53501231a2f3ba59afa067913ec4d3b2 wallet: remove `mempool_sequence` from `transactionAddedToMempool` (w0xlt) Pull request description: This PR removes `mempool_sequence` from `transactionRemovedFromMempool` and `transactionAddedToMempool`. `mempool_sequence` is not used in these methods, only in ZMQ notifications. ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/26752/commits/55696a0ac30bcfbd555f71cbc8eac23b725f7dcf Tree-SHA512: 621e89230bcb6edfed83e2758601a2b093822fc2dc4e9bfb00487e340f2bc4c5ac3bf6df3ca00b7fe55bb3df15858820f2bf698f403d2e48b915dd9eb47b63e0
2023-01-04Merge bitcoin/bitcoin#26020: test: Change coinselection parameter location ↵Andrew Chow
to make tests independent 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
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-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-27clang-tidy: Add `performance-no-automatic-move` checkHennadii Stepanov
https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html
2022-12-26wallet: remove `mempool_sequence` from `transactionAddedToMempool`w0xlt
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: make SelectCoins flow return util::Resultfurszy
2022-12-16db: Change DatabaseCursor::Next to return status enumAndrew Chow
Next()'s result is a tri-state - failed, more to go, complete. Replace the way that this is returned with an enum with values FAIL, MORE, and DONE rather than with two booleans.
2022-12-16wallet: Have cursor users use DatabaseCursor directlyAndrew Chow
Instead of having the DatabaseBatch manage the cursor, having the consumer handle it directly
2022-12-14wallet: Introduce DatabaseCursor RAII class for managing cursorAndrew Chow
Instead of having DatabaseBatch deal with opening and closing database cursors, have a separate RAII class that deals with those. For now, DatabaseBatch manages DatabaseCursor, but this will change later.
2022-12-14test: move coins result test to wallet_tests.cppfurszy
The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy of `ListCoinsTestingSetup` that is inside wallet_tests.cpp.
2022-12-14test: extend and simplify availablecoins_testsfurszy
Clean redundant code and add coverage for 'AvailableCoins' incremental result.
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-05test: Check max transaction weight in CoinSelectionAurèle Oulès
Co-authored-by: Andrew Chow <github@achow101.com>
2022-12-05Merge bitcoin/bitcoin#26560: wallet: bugfix, invalid CoinsResult cached ↵Andrew Chow
total amount 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3 refactor: make CoinsResult total amounts members private (furszy) 3282fad59908da328f8323e1213344fe58ccf69e wallet: add assert to SelectionResult::Merge for safety (S3RK) c4e3b7d6a154e82cdb902fd7bcb7b725aebde5ea wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy) cac2725fd0f5baeb741dfe079a87332784c2adc7 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy) cf793846978a8783c23b66ba6b4f3f30e83ff3eb test: Coin Selection, duplicated preset inputs selection (furszy) 341ba7ffd8cdb56b4cde1f251768c3d2c2a9b4e9 test: wallet, coverage for CoinsResult::Erase function (furszy) f930aefff9690a1e830d897d0a8c53f4219ae4a8 wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) Pull request description: This comes with #26559. Solving few bugs inside the wallet's transaction creation process and adding test coverage for them. Plus, making use of the `CoinsResult::total_amount` cached value inside the Coin Selection process to return early if we don't have enough funds to cover the target amount. ### Bugs 1) The `CoinsResult::Erase` method removes only one output from the available coins vector (there is a [loop break](https://github.com/bitcoin/bitcoin/blob/c1061be14a515b0ed4f4d646fcd0378c62e6ded3/src/wallet/spend.cpp#L112) that should have never been there) and not all the preset inputs. Which on master is not a problem, because since [#25685](https://github.com/bitcoin/bitcoin/pull/25685) we are no longer using the method. But, it's a bug on v24 (check [#26559](https://github.com/bitcoin/bitcoin/pull/26559)). This method it's being fixed and not removed because I'm later using it to solve another bug inside this PR. 2) As we update the total cached amount of the `CoinsResult` object inside `AvailableCoins` and we don't use such function inside the coin selection tests (we manually load up the `CoinsResult` object), there is a discrepancy between the outputs that we add/erase and the total amount cached value. ### Improvements * This makes use of the `CoinsResult` total amount field to early return with an "Insufficient funds" error inside Coin Selection if the tx target amount is greater than the sum of all the wallet available coins plus the preset inputs amounts (we don't need to perform the entire coin selection process if we already know that there aren't enough funds inside our wallet). ### Test Coverage 1) Adds test coverage for the duplicated preset input selection bug that we have in v24. Where the wallet invalidly selects the preset inputs twice during the Coin Selection process. Which ends up with a "good" Coin Selection result that does not cover the total tx target amount. Which, alone, crashes the wallet due an insane fee. But.. to make it worst, adding the subtract fee from output functionality to this mix ends up with the wallet by-passing the "insane" fee assertion, decreasing the output amount to fulfill the insane fee, and.. sadly, broadcasting the tx to the network. 2) Adds test coverage for the `CoinsResult::Erase` method. ------------------------------------ TO DO: * [ ] Update [#26559 ](https://github.com/bitcoin/bitcoin/pull/26559) description. ACKs for top commit: achow101: ACK 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3 glozow: ACK 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there. josibake: ACK [7362f8e](https://github.com/bitcoin/bitcoin/pull/26560/commits/7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3) Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
2022-12-05Fixup clang-tidy named argument commentsfanquake
Fix comments so they are checked/consistent. Fix incorrect arguments.
2022-12-02test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of ↵furszy
direct member access 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.
2022-12-02test: Coin Selection, duplicated preset inputs selectionfurszy
This exercises the bug inside CoinsResult::Erase that ends up on (1) a wallet crash or (2) a created and broadcasted tx that contains a reduced recipient's amount. This is covered by making the wallet selects the preset inputs twice during the coin selection process. Making the wallet think that the selection process result covers the entire tx target when it does not. It's actually creating a tx that sends more coins than what inputs are covering for. Which, combined with the SFFO option, makes the wallet incorrectly reduce the recipient's amount by the difference between the original target and the wrongly counted inputs. Which means, a created and relayed tx sending less coins to the destination than what the user inputted.
2022-12-02test: wallet, coverage for CoinsResult::Erase functionfurszy
2022-11-30Merge bitcoin/bitcoin#25942: test: add `ismine` test for descriptor ↵Andrew Chow
ScriptPubKeyMan 1b77db265317a6470d0914b520f04eb64b3c0942 test: add `ismine` test for descriptor scriptpubkeyman (w0xlt) Pull request description: Currently `src/wallet/test/ismine_tests.cpp` has tests for the legacy ScriptPubKeyMan only. This PR adds tests for the descriptor ScriptPubKeyMan. ACKs for top commit: ishaanam: ACK 1b77db265317a6470d0914b520f04eb64b3c0942 achow101: ACK 1b77db265317a6470d0914b520f04eb64b3c0942 furszy: ACK 1b77db26 with a non-blocking comment. Tree-SHA512: 977b5d1e71f9468331aeb4ebaf3708dd651f9f3018d4544a395b87ca6d7fb8bfa6d20acc1a4f6e096e240e81d30fb7a6e8add190e52536e7a3cb5a80f392883f
2022-11-21test: load wallet, coverage for crypted keysfurszy
Adds test coverage for the wallet's crypted key loading from db process. The following scenarios are covered: 1) "All ckeys checksums valid" test: Loads an encrypted wallet with all the crypted keys with a valid checksum and verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write. (we force a complete ckeys re-write if we find any missing crypted key checksum during the wallet loading process) 2) "Missing checksum in one ckey" test: Verifies that loading up a wallet with, at least one, 'ckey' with no checksum triggers a complete re-write of the crypted keys. 3) "Invalid ckey checksum error" test: Verifies that loading up a ckey with an invalid checksum stops the wallet loading process with a corruption error. 4) "Invalid ckey pubkey error" test: Verifies that loading up a ckey with an invalid pubkey stops the wallet loading process with a corruption error.
2022-11-21refactor: move DuplicateMockDatabase to wallet/test/util.hfurszy
2022-11-21refactor: unify test/util/wallet.h with wallet/test/util.hfurszy
files share the same purpose, and we shouldn't have wallet code inside the test directory. This later is needed to use wallet util functions in the bench and test binaries without be forced to duplicate them.
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-18test: Remove unused txmempool include from testsMacroFake
2022-10-04Merge bitcoin/bitcoin#26203: wallet: Use correct effective value when ↵glozow
checking target d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9 test: Check external coin effective value is used in CoinSelection (Aurèle Oulès) 76b79c1a177afa006184d716bd3d5b22ebadb168 wallet: Use correct effective value when checking target (Aurèle Oulès) Pull request description: Fixes #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 d0d9cf7aea2ff36a14a19e6999400e4070b7b0c9 furszy: ACK d0d9cf7a Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9
generated by cgit v1.2.3 (git 2.26.2) at 2025-03-18 08:22:27 +0000