Repository navigation
backport: v0.26 bitcoin#25806, bitcoin#26715, bitcoin#27647, bitcoin#27665, bitcoin#27666, bitcoin#27917 - #96
Merged
Conversation
3 tasks
DCG-Claude
force-pushed
the
backport-0.26-b181-src-wallet
branch
from
October 18, 2026 05:20
623900b to
c8b3651
Compare
…m Coin Selection 6a302d4 wallet: single output groups filtering and grouping process (furszy) bd91ed1 wallet: unify outputs grouping process (furszy) 5596200 test: coinselector_tests refactor, use CoinsResult instead of plain std::vector (furszy) 34f54a0 wallet: decouple outputs grouping process from each ChooseSelectionResult (furszy) 461f082 refactor: make OutputGroup::m_outputs field a vector of shared_ptr (furszy) d8e749b test: wallet, add coverage for outputs grouping process (furszy) 06ec8f9 wallet: make OutputGroup "positive_only" filter explicit (furszy) Pull request description: The idea originates from bitcoin#24845 (comment). Note: For clarity, it's recommended to start reviewing from the end result to understand the structure of the flow. #### GroupOutputs function rationale: If "Avoid Partial Spends" is enabled, the function gathers outputs with the same script together inside a container. So Coin Selection can treats them as if them were just one possible input and either select them all or not select them. #### How the Inputs Fetch + Selection process roughly works: ``` 1. Fetch user’s manually selected inputs. 2. Fetch wallet available coins (walks through the entire wallet txes map) and insert them into a set of vectors (each vector store outputs from a single type). 3. Coin Selection Process: Call `AttemptSelection` 8 times. Each of them expands the coin eligibility filter (accepting a larger subset of coins in the calculation) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds. Each `AttemptSelection` call performs the following actions: - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and a combination of all of them): Call ‘ChooseSelectionResult’ providing the respective, filtered by type, coins vector. Which: I. Groups the outputs vector twice (one for positive only and a second one who includes the negative ones as well). - GroupOutputs walks-through the entire inputted coins vector one time at least, + more if we are avoiding partial spends, to generate a vector of OutputGroups. II. Then performs every coin selection algorithm using the recently created vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD. III. Then returns the best solution out of them. ``` We perform the general operation of gathering outputs, with the same script, into a single container inside: Each coins selection attempt (8 times —> each coin eligibility filter), for each of the outputs vector who were filtered by type (plus another one joining all the outputs as well if needed), twice (one for the positive only outputs effective value and a second one for all of them). So, in the worst case scenario where no solution is found after the 8 Coin Selection attempts, the `GroupOutputs` function is called 80 times (8 * 5 * 2). #### Improvements: This proposal streamlines the process so that the output groups, filtered by coin eligibility and type, are created in a single loop outside of the Coin Selection Process. The new process is as follows: ``` 1. Fetch user’s manually selected inputs. 2. Fetch wallet available coins. 3. Group outputs by each coin eligibility filter and each different output type found. 4. Coin Selection Process: Call AttemptSelection 8 times. Each of them expands the coin eligibility filter (accepting different output groups) until it founds a solutions or completely fails if no solutions gets founds after the 8 rounds. Each ‘AttemptSelection’ call performs the following actions: - For each output type supported by the wallet (P2SH, P2PK, P2WPKH, P2WSH and all of them): A. Call ‘ChooseSelectionResult’ providing the respective, filtered by type, output group. Which: I. Performs every coin selection algorithm using the provided vector of OutputGroup: (1) BnB, (2) knapsack and (3) SRD. II. Then returns the best solution out of them. ``` Extra Note: The next steps after this PR will be to: 1) Merge `AvailableCoins` and `GroupOutputs` processes. 2) Skip entire coin selection rounds if no new coins are added into the subsequent round. 3) Remove global feerates from the OutputGroup class. 4) Remove secondary "grouped" tx creation from `CreateTransactionInternal` by running Coin Selection results over the aps grouped outputs vs non-aps ones. ACKs for top commit: S3RK: ReACK 6a302d4 achow101: ACK 6a302d4 theStack: re-ACK 6a302d4 🥥 Tree-SHA512: dff849063be328e7d9c358ec80239a6db2cd6131963b511b83699b95b337d3106263507eaba0119eaac63e6ac21c6c42d187ae23d79d9220b90c323d44b01d24 Dash adaptations: - src/wallet/spend.cpp: GroupOutputs keeps Dash's InstantSend check, calling EligibleForSpending(filter, isISLocked) per filter; isGroupISLocked now reads output->outpoint - src/wallet/spend.cpp, src/wallet/spend.h: AttemptSelection/ChooseSelectionResult keep the `const CWallet& wallet` parameter because Dash's KnapsackSolver needs wallet.m_default_max_tx_fee - src/wallet/spend.cpp: ChooseSelectionResult still empties groups.positive_group to skip BnB/SRD (Dash's existing behaviour) and keeps Dash's max_inputs_weight/fully-mixed/max-fee solver arguments - src/wallet/spend.cpp: CreateTransactionInternal keeps Dash's GetInputSet() + BIP69/shuffle vin path, updated to coin->outpoint, instead of upstream's GetShuffledInputVector/RBF nSequence - src/wallet/coinselection.cpp: no WITNESS_SCALE_FACTOR in OutputGroup::Insert/AddInputs (Dash weight is in bytes); Dash-only GetSelectionWeight helpers switched to shared_ptr<COutput> - src/wallet/coinselection.h: adds only <outputtype.h>, not upstream's <consensus/consensus.h>; EligibleForSpending keeps Dash's isISLocked argument - src/bench/coin_selection.cpp: AttemptSelection call passes the wallet, matching Dash's kept parameter - src/wallet/test/coinselector_tests.cpp: Dash's SelectCoinsSRDResult (from bitcoin#26720) uses the new GroupOutputs(...)[filter].all_groups.positive_group form, as upstream's post-25806 version does - src/wallet/test/coinselector_tests.cpp: Dash's own check_max_weight/has_coin and the Dash-only knapsack max-weight checks dereference shared_ptr<COutput>; upstream's relocated check_max_weight copy is dropped - src/wallet/test/group_outputs_tests.cpp: OutputType::BECH32 becomes LEGACY, OutputTypeFromDestination becomes LEGACY, GetNewDestination(label), and SetupDescriptorScriptPubKeyMans("", "") (Dash has no segwit; its API has a mnemonic)
33e2b82 wallet, bench: Remove unused database options from WalletBenchLoading (Andrew Chow) 80ace04 tests: Modify records directly in wallet ckey loading test (Andrew Chow) b3bb17d tests: Update DuplicateMockDatabase for MockableDatabase (Andrew Chow) f0eecf5 scripted-diff: Replace CreateMockWalletDB with CreateMockableWalletDB (Andrew Chow) 075962b wallet, tests: Include wallet/test/util.h (Andrew Chow) 14aa4cb wallet: Move DummyDatabase to salvage (Andrew Chow) f67a385 wallet, tests: Replace usage of dummy db with mockable db (Andrew Chow) 33c6245 Introduce MockableDatabase for wallet unit tests (Andrew Chow) Pull request description: For the wallet's unit tests, we currently use either `DummyDatabase` or memory-only versions of either BDB or SQLite. The tests that use `DummyDatabase` just need a `WalletDatabase` so that the `CWallet` can be constructed, while the tests using the memory-only databases just need a backing data store. There is also a `FailDatabase` that is similar to `DummyDatabase` except it fails be default or can have a configured return value. Having all of these different database types can make it difficult to write tests, particularly tests that work when either BDB or SQLite is disabled. This PR unifies all of these different unit test database classes into a single `MockableDatabase`. Like `DummyDatabase`, most functions do nothing and just return true. Like `FailDatabase`, the return value of some functions can be configured on the fly to test various failure cases. Like the memory-only databases, records can actually be written to the `MockableDatabase` and be retrieved later, but all of this is still held in memory. Using `MockableDatabase` completely removes the need for having BDB or SQLite backed wallets in the unit tests for the tests that are not actually testing specific database behaviors. Because `MockableDatabase`s can be created by each unit test, we can also control what records are stored in the database. Records can be added and removed externally from the typical database modification functions. This will give us greater ability to test failure conditions, particularly those involving corrupted records. Possible alternative to bitcoin#26644 ACKs for top commit: furszy: ACK 33e2b82 TheCharlatan: ACK 33e2b82 Tree-SHA512: c2b09eff9728d063d2d4aea28a0f0e64e40b76483e75dc53f08667df23bd25834d52656cd4eafb02e552db0b9e619cfdb1b1c65b26b5436ee2c971d804768bcc Dash adaptations: - src/wallet/salvage.cpp: DummyDatabase replaces CreateDummyWalletDatabase() but keeps Dash's DB_TXN_WRITE_NOSYNC and dummyWallet.SetupLegacyScriptPubKeyMan() - src/wallet/test/coinselector_tests.cpp: Dash has no NewWallet() (its bitcoin#26720 backport skipped 25ab147), so the 11 inline wallet creations it replaces upstream use CreateMockableWalletDatabase - src/wallet/test/wallet_tests.cpp: removes Dash's relocated FailDatabase/FailBatch/FailCursor and ports Dash-only interface_coin_lock_failed_persist to CreateMockableWalletDatabase + GetMockableDatabase().m_pass - src/wallet/test/wallet_tests.cpp: keeps Dash-only SetupLegacyScriptPubKeyMan() and tip lookups around the renamed wallet constructions - src/wallet/test/coinjoin_tests.cpp: fixture wallet uses CreateMockableWalletDatabase so DuplicateMockDatabase(db) works; persist-retry test fails the write via m_pass, not an open SQLite txn, and drops its USE_SQLITE guard - src/qt/test/wallettests.cpp: Dash's own wallet setup is renamed to CreateMockableWalletDatabase; upstream's SetupLegacyWatchOnlyWallet/SetupDescriptorsWallet/MiniGUI do not exist in Dash - src/wallet/test/walletload_tests.cpp: takes upstream's comment but keeps Dash's GetNewDestination() without an OutputType argument - src/wallet/test/util.cpp, wallet_test_fixture.cpp, scriptpubkeyman_tests.cpp: rename only; Dash's ChainInfo snapshot, string chain name/loader args and local NodeContext chain are kept - build_msvc: Dash's autotools build already links wallet/test/util.cpp via libtest_util.a into bench, test_dash and test_dash-qt Not applicable to Dash (intentionally omitted): - build_msvc/libtest_util/libtest_util.vcxproj.in: Dash has no build_msvc; its counterpart libtest_util.a (Makefile.test_util.include) already contains wallet/test/util.cpp - build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj: Dash has no build_msvc; test_dash-qt already links $(LIBTEST_UTIL) in Makefile.qttest.include - src/wallet/test/ismine_tests.cpp: renames inside P2WPKH/P2WSH/P2TR/witness test blocks, which Dash dropped because it has no segwit/taproot; all 19 remaining Dash cases are renamed
162602b fuzz: wallet, add target for `fees` (brunoerg) Pull request description: This PR adds fuzz coverage for `wallet/fees`. Some functions may use or not (non default) values from `wallet`, `CCoinControl` or `FeeCalculation`. So the logic is to make the test sometimes fill up some attributes and others no. Obs: As soon as this PR gets some reviews, I can open the proper PR to `qa-assets` as well. ACKs for top commit: Xekyo: ACK 162602b MarcoFalke: lgtm ACK 162602b dergoegge: Code review ACK 162602b Tree-SHA512: 6545802f27aafb60bf5a119af514e9425b643780dea6650bba766bb5be813f2aaddb7afc7f0efa2943ceb26f5ea08b42c95a3c0df897493c71f2d2f99e9e4236
0282b21 walletdb: Remove unused CreateMockWalletDatabase (Andrew Chow) Pull request description: This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing. I thought this was included in bitcoin#26715, maybe it got lost in a rebase. ACKs for top commit: furszy: utACK 0282b21 brunoerg: crACK 0282b21 Tree-SHA512: 18445c4d8a4e2609ef7471c6d75297f43694b79e768f6c993a5addb1dc0e88bd12bac263c9d8e903d828ddf6bf50434f9e2f72048f4fc528e98fed8ee65123ca
…heir own file and fix a bug 7379a54 bench: Remove incorrect LoadWallet call in WalletBalance (Andrew Chow) 846b2fe tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h (Andrew Chow) c61d3f0 tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper (Andrew Chow) Pull request description: I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner. The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`. The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues currently, it ends up causing issues in some PRs and branches that I'm working on. ACKs for top commit: Sjors: utACK 7379a54 furszy: ACK 7379a54 Tree-SHA512: 47773887a16c69ac7121c699d3446a8c399bd792a6a31714998b7b7a19fea179c6d3b29cb898b04397b2962c1b4120d57009352b8460b8283e188d4cb480c9ba Dash adaptations: - src/wallet/test/util.h: moves Dash's ADDRESS_B58T_UNSPENDABLE (yXXX… regtest address) to the shared header instead of upstream's ADDRESS_BCRT1_UNSPENDABLE bech32 address - src/bench/wallet_balance.cpp: keeps Dash's epoch_iters parameter and SetupDescriptorScriptPubKeyMans("", "") call while wrapping in namespace wallet and dropping the bogus LoadWallet() - src/wallet/test/util.cpp: keeps Dash's <uint256.h>/<validation.h> includes alongside the new <wallet/context.h> - src/wallet/test/wallet_tests.cpp: drops Dash's context-taking TestUnloadWallet and its RemoveWallet() call; it is a no-op since TestLoadWallet no longer calls AddWallet - src/wallet/test/wallet_tests.cpp: keeps the Dash-only interface_coin_lock_* tests that sat next to the removed static helpers
…/fees fuzz target faa05d1 fuzz: Fix implicit-integer-sign-change in wallet/fees fuzz target (MarcoFalke) Pull request description: This fixes a bug in the fuzz target. ``` echo 'OiAAAPr//wAAAAAAAAA=' | base64 --decode > /tmp/a UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=wallet_fees ./src/test/fuzz/fuzz /tmp/a ``` ``` wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed) #0 0x5625ef46a094 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) src/wallet/fees.cpp:58:58 #1 0x5625eedd467f in wallet::(anonymous namespace)::wallet_fees_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/fees.cpp:64:11 ... SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change wallet/fees.cpp:58:58 in ACKs for top commit: dergoegge: tACK faa05d1 brunoerg: ACK faa05d1 Tree-SHA512: 66a4020d6a4153a92c7023e9f94ec6279862566db7236ce3cf6951b7fbee616dc88a56fe9502de4099d74f9840439b20a984b0733fb432e43129e774bcc2a6e6
knst
force-pushed
the
backport-0.26-b181-src-wallet
branch
from
October 21, 2026 16:55
c8b3651 to
580be34
Compare
Collaborator
Author
|
merged as dashpay#7903 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated Bitcoin Core v0.26 backports, batch
backport-0.26-b181-src-wallet.68b15017de275fd7ab7ca60680a5dbwallet_feesfuzz target and registers it in FUZZ_WALLET_SRC. The only change f9853964026d0a01a8853c8b3651676Provenance
Each commit passed: cherry-pick (adapted by an Opus lane only where conflicts existed), build, touched tests, a mechanical diff-of-diffs check (every upstream hunk landed; no added line without an upstream counterpart), and an independent Opus verification lane where anything was adapted. Gate rows and lane artifacts are in the backportsys DB.