From 870548ad3c9b55751a0a4c879ee1e4d8589854e1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 3 May 2025 22:24:08 +0000 Subject: [PATCH 01/12] merge bitcoin#17526: Add Single Random Draw as an additional coin selection algorithm UTXO (un)locking has been omitted from `test_fee_p2pkh` sub-test --- src/wallet/coinselection.cpp | 26 +++++++++ src/wallet/coinselection.h | 11 ++++ src/wallet/spend.cpp | 10 ++++ test/functional/rpc_fundrawtransaction.py | 71 ++++++++++++++++++----- test/functional/wallet_basic.py | 4 ++ test/functional/wallet_txn_clone.py | 13 ++++- test/functional/wallet_txn_doublespend.py | 13 ++++- 7 files changed, 128 insertions(+), 20 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index fdc6937acc3a..15db7d762dd2 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -5,11 +5,13 @@ #include #include +#include #include #include #include +#include #include // Descending order comparator @@ -170,6 +172,30 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selectio return true; } +std::optional, CAmount>> SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value) +{ + std::set out_set; + CAmount value_ret = 0; + + std::vector indexes; + indexes.resize(utxo_pool.size()); + std::iota(indexes.begin(), indexes.end(), 0); + Shuffle(indexes.begin(), indexes.end(), FastRandomContext()); + + CAmount selected_eff_value = 0; + for (const size_t i : indexes) { + const OutputGroup& group = utxo_pool.at(i); + Assume(group.GetSelectionAmount() > 0); + selected_eff_value += group.GetSelectionAmount(); + value_ret += group.m_value; + util::insert(out_set, group.m_outputs); + if (selected_eff_value >= target_value) { + return std::make_pair(out_set, value_ret); + } + } + return std::nullopt; +} + static void ApproximateBestSubset(const std::vector& groups, const CAmount& nTotalLower, const CAmount& nTargetValue, std::vector& vfBest, CAmount& nBest, int iterations = 1000) { diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 5e1a17862bea..49cf1950109f 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -10,6 +10,8 @@ #include #include +#include + //! target minimum change amount static constexpr CAmount MIN_CHANGE{COIN / 100}; //! final minimum change amount after paying for fees @@ -183,6 +185,15 @@ struct OutputGroup bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret); +/** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible + * outputs until the target is satisfied + * + * @param[in] utxo_pool The positive effective value OutputGroups eligible for selection + * @param[in] target_value The target value to select for + * @returns If successful, a pair of set of outputs and total selected value, otherwise, std::nullopt + */ +std::optional, CAmount>> SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value); + // Original coin selection algorithm as a fallback bool KnapsackSolver(const CAmount& nTargetValue, std::vector& groups, std::set& setCoinsRet, CAmount& nValueRet, bool fFulyMixedOnly, CAmount maxTxFee); #endif // BITCOIN_WALLET_COINSELECTION_H diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 366b838f072f..f271723cc671 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -399,6 +399,16 @@ bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const results.emplace_back(std::make_tuple(waste, std::move(knapsack_coins), knapsack_value)); } + // We include the minimum final change for SRD as we do want to avoid making really small change. + // KnapsackSolver does not need this because it includes MIN_CHANGE internally. + const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + MIN_FINAL_CHANGE; + auto srd_result = SelectCoinsSRD(positive_groups, srd_target); + // Note: SRD is disabled because it is unaware of mixed coins + if (/* DISABLES CODE */ (false) && srd_result != std::nullopt) { + const auto waste = GetSelectionWaste(srd_result->first, coin_selection_params.m_cost_of_change, srd_target, !coin_selection_params.m_subtract_fee_outputs); + results.emplace_back(std::make_tuple(waste, std::move(srd_result->first), srd_result->second)); + } + if (results.size() == 0) { // No solution found return false; diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 34ff51c7b3de..701656bf00a7 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -51,7 +51,36 @@ def setup_network(self): self.connect_nodes(0, 2) self.connect_nodes(0, 3) + def lock_outputs_type(self, wallet, outputtype): + """ + Only allow UTXOs of the given type + """ + if outputtype in ["legacy", "p2pkh", "pkh"]: + prefixes = ["pkh(", "sh(multi("] + else: + assert False, f"Unknown output type {outputtype}" + + to_lock = [] + for utxo in wallet.listunspent(): + if "desc" in utxo: + for prefix in prefixes: + if utxo["desc"].startswith(prefix): + to_lock.append({"txid": utxo["txid"], "vout": utxo["vout"]}) + wallet.lockunspent(False, to_lock) + + def unlock_utxos(self, wallet): + """ + Unlock all UTXOs except the watchonly one + """ + to_keep = [] + if self.watchonly_txid is not None and self.watchonly_vout is not None: + to_keep.append({"txid": self.watchonly_txid, "vout": self.watchonly_vout}) + wallet.lockunspent(True) + wallet.lockunspent(False, to_keep) + def run_test(self): + self.watchonly_txid = None + self.watchonly_vout = None self.log.info("Connect nodes, set fees, generate blocks, and sync") self.min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee'] # This test is not meant to test fee estimation and we'd like @@ -378,6 +407,7 @@ def test_fee_p2pkh(self): def test_fee_p2pkh_multi_out(self): """Compare fee of a standard pubkeyhash transaction with multiple outputs.""" self.log.info("Test fundrawtxn p2pkh fee with multiple outputs") + self.lock_outputs_type(self.nodes[0], "p2pkh") inputs = [] outputs = { self.nodes[1].getnewaddress():11, @@ -398,8 +428,11 @@ def test_fee_p2pkh_multi_out(self): feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) assert feeDelta >= 0 and feeDelta <= self.fee_tolerance + self.unlock_utxos(self.nodes[0]) + def test_fee_p2sh(self): """Compare fee of a 2-of-2 multisig p2sh transaction.""" + self.lock_outputs_type(self.nodes[0], "p2pkh") # Create 2-of-2 addr. addr1 = self.nodes[1].getnewaddress() addr2 = self.nodes[1].getnewaddress() @@ -422,9 +455,12 @@ def test_fee_p2sh(self): feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) assert feeDelta >= 0 and feeDelta <= self.fee_tolerance + self.unlock_utxos(self.nodes[0]) + def test_fee_4of5(self): """Compare fee of a standard pubkeyhash transaction.""" self.log.info("Test fundrawtxn fee with 4-of-5 addresses") + self.lock_outputs_type(self.nodes[0], "p2pkh") # Create 4-of-5 addr. addr1 = self.nodes[1].getnewaddress() @@ -463,6 +499,8 @@ def test_fee_4of5(self): feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) assert feeDelta >= 0 and feeDelta <= self.fee_tolerance + self.unlock_utxos(self.nodes[0]) + def test_spend_2of2(self): """Spend a 2-of-2 multisig transaction over fundraw.""" self.log.info("Test fundpsbt spending 2-of-2 multisig") @@ -528,10 +566,11 @@ def test_locked_wallet(self): # Drain the keypool. self.nodes[1].getnewaddress() - inputs = self.nodes[1].listunspent() - # Deduce fee to produce a changeless transaction + + # Choose 2 inputs # bnb doesn't work same way as in bitcoin, so, `value` is also calculated by different way - value = sum(input_it["amount"] for input_it in inputs) - Decimal("0.00002200") + inputs = self.nodes[1].listunspent() + value = sum(inp["amount"] for inp in inputs) - Decimal("0.00002200") outputs = {self.nodes[0].getnewaddress():value} rawtx = self.nodes[1].createrawtransaction(inputs, outputs) # fund a transaction that does not require a new key for the change output @@ -539,7 +578,7 @@ def test_locked_wallet(self): # fund a transaction that requires a new key for the change output # creating the key must be impossible because the wallet is locked - outputs = {self.nodes[0].getnewaddress():1.1} + outputs = {self.nodes[0].getnewaddress():value - Decimal("0.1")} rawtx = self.nodes[1].createrawtransaction(inputs, outputs) assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.", self.nodes[1].fundrawtransaction, rawtx) @@ -907,31 +946,31 @@ def test_include_unsafe(self): # We receive unconfirmed funds from external keys (unsafe outputs). addr = wallet.getnewaddress() - txid1 = self.nodes[2].sendtoaddress(addr, 6) - txid2 = self.nodes[2].sendtoaddress(addr, 4) - self.sync_all() - vout1 = find_vout_for_address(wallet, txid1, addr) - vout2 = find_vout_for_address(wallet, txid2, addr) + inputs = [] + for i in range(0, 2): + txid = self.nodes[2].sendtoaddress(addr, 5) + self.sync_mempools() + vout = find_vout_for_address(wallet, txid, addr) + inputs.append((txid, vout)) # Unsafe inputs are ignored by default. - rawtx = wallet.createrawtransaction([], [{self.nodes[2].getnewaddress(): 5}]) + rawtx = wallet.createrawtransaction([], [{self.nodes[2].getnewaddress(): 7.5}]) assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, rawtx) # But we can opt-in to use them for funding. fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True}) tx_dec = wallet.decoderawtransaction(fundedtx['hex']) - assert any([txin['txid'] == txid1 and txin['vout'] == vout1 for txin in tx_dec['vin']]) + assert all((txin["txid"], txin["vout"]) in inputs for txin in tx_dec["vin"]) signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) - wallet.sendrawtransaction(signedtx['hex']) + assert wallet.testmempoolaccept([signedtx['hex']])[0]["allowed"] # And we can also use them once they're confirmed. self.generate(self.nodes[0], 1) - rawtx = wallet.createrawtransaction([], [{self.nodes[2].getnewaddress(): 3}]) - fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True}) + fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": False}) tx_dec = wallet.decoderawtransaction(fundedtx['hex']) - assert any([txin['txid'] == txid2 and txin['vout'] == vout2 for txin in tx_dec['vin']]) + assert all((txin["txid"], txin["vout"]) in inputs for txin in tx_dec["vin"]) signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) - wallet.sendrawtransaction(signedtx['hex']) + assert wallet.testmempoolaccept([signedtx['hex']])[0]["allowed"] def test_22670(self): # In issue #22670, it was observed that ApproximateBestSubset may diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 814841a4b1df..f75442bccda4 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -15,6 +15,7 @@ assert_greater_than, assert_raises_rpc_error, count_bytes, + find_vout_for_address, ) from test_framework.wallet_util import test_address @@ -485,6 +486,9 @@ def run_test(self): # 1. Send some coins to generate new UTXO address_to_import = self.nodes[2].getnewaddress() txid = self.nodes[0].sendtoaddress(address_to_import, 1) + self.sync_mempools(self.nodes[0:3]) + vout = find_vout_for_address(self.nodes[2], txid, address_to_import) + self.nodes[2].lockunspent(False, [{"txid": txid, "vout": vout}]) self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_all(self.nodes[0:3])) self.log.info("Test sendtoaddress with fee_rate param (explicit fee rate in duff/B)") diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py index 146e62fbeeaa..130e5d3b74f7 100755 --- a/test/functional/wallet_txn_clone.py +++ b/test/functional/wallet_txn_clone.py @@ -7,6 +7,7 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + find_vout_for_address ) from test_framework.messages import ( COIN, @@ -31,6 +32,13 @@ def setup_network(self): super().setup_network() self.disconnect_nodes(1, 2) + def spend_txid(self, txid, vout, outputs): + inputs = [{"txid": txid, "vout": vout}] + tx = self.nodes[0].createrawtransaction(inputs, outputs) + tx = self.nodes[0].fundrawtransaction(tx) + tx = self.nodes[0].signrawtransactionwithwallet(tx['hex']) + return self.nodes[0].sendrawtransaction(tx['hex']) + def run_test(self): # All nodes should start with 12,500 DASH: starting_balance = 12500 @@ -42,6 +50,7 @@ def run_test(self): node0_address1 = self.nodes[0].getnewaddress() node0_txid1 = self.nodes[0].sendtoaddress(node0_address1, 12190) node0_tx1 = self.nodes[0].gettransaction(node0_txid1) + self.nodes[0].lockunspent(False, [{"txid":node0_txid1, "vout": find_vout_for_address(self.nodes[0], node0_txid1, node0_address1)}]) node0_address2 = self.nodes[0].getnewaddress() node0_txid2 = self.nodes[0].sendtoaddress(node0_address2, 290) @@ -54,8 +63,8 @@ def run_test(self): node1_address = self.nodes[1].getnewaddress() # Send tx1, and another transaction tx2 that won't be cloned - txid1 = self.nodes[0].sendtoaddress(node1_address, 400) - txid2 = self.nodes[0].sendtoaddress(node1_address, 200) + txid1 = self.spend_txid(node0_txid1, find_vout_for_address(self.nodes[0], node0_txid1, node0_address1), {node1_address: 400}) + txid2 = self.spend_txid(node0_txid2, find_vout_for_address(self.nodes[0], node0_txid2, node0_address2), {node1_address: 200}) # Construct a clone of tx1, to be malleated rawtx1 = self.nodes[0].getrawtransaction(txid1, 1) diff --git a/test/functional/wallet_txn_doublespend.py b/test/functional/wallet_txn_doublespend.py index e26630172634..a33720a4af3a 100755 --- a/test/functional/wallet_txn_doublespend.py +++ b/test/functional/wallet_txn_doublespend.py @@ -9,6 +9,7 @@ from test_framework.util import ( assert_equal, find_output, + find_vout_for_address ) @@ -29,6 +30,13 @@ def setup_network(self): super().setup_network() self.disconnect_nodes(1, 2) + def spend_txid(self, txid, vout, outputs): + inputs = [{"txid": txid, "vout": vout}] + tx = self.nodes[0].createrawtransaction(inputs, outputs) + tx = self.nodes[0].fundrawtransaction(tx) + tx = self.nodes[0].signrawtransactionwithwallet(tx['hex']) + return self.nodes[0].sendrawtransaction(tx['hex']) + def run_test(self): # All nodes should start with 12,500 DASH: starting_balance = 12500 @@ -47,6 +55,7 @@ def run_test(self): node0_address_foo = self.nodes[0].getnewaddress() fund_foo_txid = self.nodes[0].sendtoaddress(node0_address_foo, 12190) fund_foo_tx = self.nodes[0].gettransaction(fund_foo_txid) + self.nodes[0].lockunspent(False, [{"txid":fund_foo_txid, "vout": find_vout_for_address(self.nodes[0], fund_foo_txid, node0_address_foo)}]) node0_address_bar = self.nodes[0].getnewaddress() fund_bar_txid = self.nodes[0].sendtoaddress(node0_address_bar, 290) @@ -77,8 +86,8 @@ def run_test(self): assert_equal(doublespend["complete"], True) # Create two spends using 1 500 DASH coin each - txid1 = self.nodes[0].sendtoaddress(node1_address, 400) - txid2 = self.nodes[0].sendtoaddress(node1_address, 200) + txid1 = self.spend_txid(fund_foo_txid, find_vout_for_address(self.nodes[0], fund_foo_txid, node0_address_foo), {node1_address: 400}) + txid2 = self.spend_txid(fund_bar_txid, find_vout_for_address(self.nodes[0], fund_bar_txid, node0_address_bar), {node1_address: 200}) # Have node0 mine a block: if (self.options.mine_block): From aae8e919d3824d9a665f5d5c0a76962c3be64fa5 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 4 May 2025 13:42:37 +0000 Subject: [PATCH 02/12] partial bitcoin#17211: Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs includes: - a00eb388e8046fe105666445dff6c91e8f8664cb --- src/wallet/coinselection.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 49cf1950109f..5e900f8f2315 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -37,6 +37,18 @@ class CInputCoin { m_input_bytes = input_bytes; } + CInputCoin(const COutPoint& outpoint_in, const CTxOut& txout_in) + { + outpoint = outpoint_in; + txout = txout_in; + effective_value = txout.nValue; + } + + CInputCoin(const COutPoint& outpoint_in, const CTxOut& txout_in, int input_bytes) : CInputCoin(outpoint_in, txout_in) + { + m_input_bytes = input_bytes; + } + COutPoint outpoint; CTxOut txout; CAmount effective_value; From 40550c95cfe5f4428fb6dc63e21267663e4e4f65 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 3 May 2025 21:06:18 +0000 Subject: [PATCH 03/12] merge bitcoin#22928: Remove gArgs from wallet.h and wallet.cpp (2) --- src/bench/coin_selection.cpp | 2 +- src/bench/wallet_balance.cpp | 2 +- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/dump.cpp | 2 +- src/wallet/salvage.cpp | 2 +- src/wallet/test/coinjoin_tests.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 10 ++++----- src/wallet/test/ismine_tests.cpp | 20 ++++++++--------- src/wallet/test/scriptpubkeyman_tests.cpp | 2 +- src/wallet/test/spend_tests.cpp | 2 +- src/wallet/test/util.cpp | 4 ++-- src/wallet/test/util.h | 3 ++- src/wallet/test/wallet_test_fixture.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 26 +++++++++++------------ src/wallet/wallet.cpp | 14 ++++++------ src/wallet/wallet.h | 6 +++++- src/wallet/wallettool.cpp | 12 +++++------ 18 files changed, 60 insertions(+), 55 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index c9f27fd217eb..ac60e8c07df4 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -32,7 +32,7 @@ static void CoinSelection(benchmark::Bench& bench) { NodeContext node; auto chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), /*coinjoin_loader=*/nullptr, "", gArgs, CreateDummyWalletDatabase()); std::vector> wtxs; LOCK(wallet.cs_wallet); diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index c49df3085b81..dbc522615991 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -19,7 +19,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b const auto test_setup = MakeNoLogFileContext(); const auto& ADDRESS_WATCHONLY = ADDRESS_B58T_UNSPENDABLE; - CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase()}; + CWallet wallet{test_setup->m_node.chain.get(), test_setup->m_node.coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase()}; { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index f1e6bd118516..fa7a361b2683 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -64,7 +64,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) { TestChain100Setup test; node.setContext(&test.m_node); - const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase()); wallet->LoadWallet(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); { diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 04f5f10e7368..ae1f5fa71d45 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -110,7 +110,7 @@ void TestGUI(interfaces::Node& node) } node.setContext(&test.m_node); WalletContext& context = *node.walletLoader().context(); - const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), node.context()->coinjoin_loader.get(), "", gArgs, CreateMockWalletDatabase()); AddWallet(context, wallet); wallet->LoadWallet(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 617704d77846..fc5b61d4e06b 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -199,7 +199,7 @@ bool CreateFromDump(const std::string& name, const fs::path& wallet_path, biling // dummy chain interface bool ret = true; - std::shared_ptr wallet(new CWallet(nullptr /* chain */, /*coinjoin_loader=*/ nullptr, name, std::move(database)), WalletToolReleaseWallet); + std::shared_ptr wallet(new CWallet(/*chain=*/nullptr, /*coinjoin_loader=*/nullptr, name, gArgs, std::move(database)), WalletToolReleaseWallet); { LOCK(wallet->cs_wallet); DBErrors load_wallet_ret = wallet->LoadWallet(); diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 5456442955a9..cb9edf643aa0 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -133,7 +133,7 @@ bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::v } DbTxn* ptxn = env->TxnBegin(); - CWallet dummyWallet(nullptr, /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet dummyWallet(/*chain=*/nullptr, /*coinjoin_loader=*/nullptr, "", gArgs, CreateDummyWalletDatabase()); dummyWallet.SetupLegacyScriptPubKeyMan(); for (KeyValPair& row : salvagedData) { diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index bfdbe6872df5..3f5b7c1efbd4 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -131,7 +131,7 @@ class CTransactionBuilderTestSetup : public TestChain100Setup { public: CTransactionBuilderTestSetup() : - wallet{std::make_unique(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase())} + wallet{std::make_unique(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateMockWalletDatabase())} { context.args = &gArgs; context.chain = m_node.chain.get(); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 79678ac02292..287d59fa291e 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -280,7 +280,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -304,7 +304,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -327,7 +327,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) BOOST_AUTO_TEST_CASE(knapsack_solver_test) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -608,7 +608,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) BOOST_AUTO_TEST_CASE(ApproximateBestSubset) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -631,7 +631,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) // Tests that with the ideal conditions, the coin selector will always be able to find a solution that can pay the target value BOOST_AUTO_TEST_CASE(SelectCoins_test) { - std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase()); + std::unique_ptr wallet = std::make_unique(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase()); wallet->LoadWallet(); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index 09e9e3851072..4d7b487913d8 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -35,7 +35,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK compressed { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForRawPubKey(pubkeys[0]); @@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK uncompressed { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForRawPubKey(uncompressedPubkey); @@ -69,7 +69,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH compressed { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForDestination(PKHash(pubkeys[0])); @@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH uncompressed { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForDestination(PKHash(uncompressedPubkey)); @@ -103,7 +103,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -127,7 +127,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // (P2PKH inside) P2SH inside P2SH (invalid) { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -145,7 +145,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // scriptPubKey multisig { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -176,7 +176,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH multisig { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(uncompressedKey)); @@ -197,7 +197,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // OP_RETURN { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -211,7 +211,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // Nonstandard { - CWallet keystore(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 5015f448a730..4bc9e5074172 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -19,7 +19,7 @@ BOOST_AUTO_TEST_CASE(CanProvide) // Set up wallet and keyman variables. NodeContext node; std::unique_ptr chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), /*coinjoin_loader=*/ nullptr, "", CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateDummyWalletDatabase()); LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan(); // Make a 1 of 2 multisig script diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index 0dca8429720c..1c062ca16b6b 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -16,7 +16,7 @@ BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup) BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup) { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - auto wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), coinbaseKey); + auto wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), m_args, coinbaseKey); // Check that a subtract-from-recipient transaction slightly less than the // coinbase input amount does not create a change output (because it would diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index a020e4b04f76..9fda8df75fea 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -15,9 +15,9 @@ #include -std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, CChain& cchain, const CKey& key) +std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, CChain& cchain, ArgsManager& args, const CKey& key) { - auto wallet = std::make_unique(&chain, &coinjoin_loader, "", CreateMockWalletDatabase()); + auto wallet = std::make_unique(&chain, &coinjoin_loader, "", args, CreateMockWalletDatabase()); { LOCK(wallet->cs_wallet); wallet->SetLastBlockProcessed(cchain.Height(), cchain.Tip()->GetBlockHash()); diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index 4eaecdc7c70c..8d85f51f824b 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -7,6 +7,7 @@ #include +class ArgsManager; class CChain; class CKey; class CWallet; @@ -17,6 +18,6 @@ class Loader; } // namespace CoinJoin } // namespace interfaces -std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, CChain& cchain, const CKey& key); +std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, interfaces::CoinJoin::Loader& coinjoin_loader, CChain& cchain, ArgsManager& args, const CKey& key); #endif // BITCOIN_WALLET_TEST_UTIL_H diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index 277ce2e9df0d..05980556bf1c 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -10,7 +10,7 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName) : TestingSetup(chainName), m_coinjoin_loader{interfaces::MakeCoinJoinLoader(m_node)}, m_wallet_loader{interfaces::MakeWalletLoader(*m_node.chain, *Assert(m_node.args), m_node, *Assert(m_coinjoin_loader))}, - m_wallet(m_node.chain.get(), m_coinjoin_loader.get(), "", CreateMockWalletDatabase()) + m_wallet(m_node.chain.get(), m_coinjoin_loader.get(), "", m_args, CreateMockWalletDatabase()) { m_wallet.LoadWallet(); m_chain_notifications_handler = m_node.chain->handleNotifications({ &m_wallet, [](CWallet*) {} }); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 410743fbef16..44851fd53a04 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -113,7 +113,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions fails to read an unknown start block. { - CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); wallet.SetupLegacyScriptPubKeyMan(); { LOCK(wallet.cs_wallet); @@ -134,7 +134,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions picks up transactions in both the old // and new block files. { - CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -163,7 +163,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions only picks transactions in the new block // file. { - CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -190,7 +190,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions scans no blocks. { - CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -229,7 +229,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(newTip->nHeight, newTip->GetBlockHash())); WalletContext context; @@ -295,7 +295,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) { WalletContext context; context.args = &gArgs; - const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); { auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); @@ -317,7 +317,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME // were scanned, and no prior blocks were scanned. { - const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); LOCK(wallet->cs_wallet); wallet->SetupLegacyScriptPubKeyMan(); @@ -350,7 +350,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // debit functions. BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) { - CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + CWallet wallet(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); CWalletTx wtx(m_coinbase_txns.back()); LOCK(wallet.cs_wallet); @@ -526,7 +526,7 @@ class ListCoinsTestingSetup : public TestChain100Setup ListCoinsTestingSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), coinbaseKey); + wallet = CreateSyncedWallet(*m_node.chain, *m_node.coinjoin_loader, m_node.chainman->ActiveChain(), m_args, coinbaseKey); } ~ListCoinsTestingSetup() @@ -627,7 +627,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { { - const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); @@ -637,7 +637,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) BOOST_CHECK(!wallet->GetNewDestination("", dest, error)); } { - const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateDummyWalletDatabase()); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet->SetMinVersion(FEATURE_LATEST); @@ -889,7 +889,7 @@ BOOST_FIXTURE_TEST_CASE(rpc_getaddressinfo, TestChain100Setup) context.args = &gArgs; context.chain = m_node.chain.get(); context.coinjoin_loader = m_node.coinjoin_loader.get(); - const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateMockWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); AddWallet(context, wallet); JSONRPCRequest request; @@ -986,7 +986,7 @@ class CreateTransactionTestSetup : public TestChain100Setup const std::string strMaxFeeExceeded = "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)"; CreateTransactionTestSetup() - : wallet{std::make_unique(m_node.chain.get(), m_node.coinjoin_loader.get(), "", CreateMockWalletDatabase())} + : wallet{std::make_unique(m_node.chain.get(), m_node.coinjoin_loader.get(), "", m_args, CreateMockWalletDatabase())} { context.args = &gArgs; context.chain = m_node.chain.get(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e26c136494d4..4eae5bf2dba8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -950,7 +950,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio #if HAVE_SYSTEM // notify an external script when a wallet transaction comes in or is updated - std::string strCmd = gArgs.GetArg("-walletnotify", ""); + std::string strCmd = m_args.GetArg("-walletnotify", ""); if (!strCmd.empty()) { @@ -2718,7 +2718,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri const auto start{SteadyClock::now()}; // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. - const std::shared_ptr walletInstance(new CWallet(chain, coinjoin_loader, name, std::move(database)), ReleaseWallet); + const std::shared_ptr walletInstance(new CWallet(chain, coinjoin_loader, name, args, std::move(database)), ReleaseWallet); // TODO: refactor this condition: validation of error looks like workaround if (!walletInstance->AutoBackupWallet(fs::PathFromString(walletFile), error, warnings) && !error.original.empty()) { return nullptr; @@ -2954,11 +2954,11 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri walletInstance->m_default_max_tx_fee = max_fee.value(); } - if (gArgs.IsArgSet("-consolidatefeerate")) { - if (std::optional consolidate_feerate = ParseMoney(gArgs.GetArg("-consolidatefeerate", ""))) { + if (args.IsArgSet("-consolidatefeerate")) { + if (std::optional consolidate_feerate = ParseMoney(args.GetArg("-consolidatefeerate", ""))) { walletInstance->m_consolidate_feerate = CFeeRate(*consolidate_feerate); } else { - error = AmountErrMsg("consolidatefeerate", gArgs.GetArg("-consolidatefeerate", "")); + error = AmountErrMsg("consolidatefeerate", args.GetArg("-consolidatefeerate", "")); return nullptr; } } @@ -3371,7 +3371,7 @@ void CWallet::notifyTransactionLock(const CTransactionRef &tx, const std::shared NotifyISLockReceived(); #if HAVE_SYSTEM // notify an external script - std::string strCmd = gArgs.GetArg("-instantsendnotify", ""); + std::string strCmd = m_args.GetArg("-instantsendnotify", ""); if (!strCmd.empty()) { ReplaceAll(strCmd, "%s", txHash.GetHex()); #ifndef WIN32 @@ -3548,7 +3548,7 @@ bool CWallet::Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnl if(nWalletBackups == -2) { TopUpKeyPool(); WalletLogPrintf("Keypool replenished, re-initializing automatic backups.\n"); - nWalletBackups = gArgs.GetIntArg("-createwalletbackups", 10); + nWalletBackups = m_args.GetIntArg("-createwalletbackups", 10); } return true; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0067a73300e7..4f90325c3ae9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -345,6 +345,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati // Decreases amount of nKeysLeftSinceAutoBackup after KeepDestination void KeepDestinationCallback(bool erased) override; + /** Provider of aplication-wide arguments. */ + const ArgsManager& m_args; + /** Interface for accessing chain state. */ interfaces::Chain* m_chain; @@ -436,8 +439,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati unsigned int nMasterKeyMaxID = 0; /** Construct wallet with specified name and database implementation. */ - CWallet(interfaces::Chain* chain, interfaces::CoinJoin::Loader* coinjoin_loader, const std::string& name, std::unique_ptr database) + CWallet(interfaces::Chain* chain, interfaces::CoinJoin::Loader* coinjoin_loader, const std::string& name, const ArgsManager& args, std::unique_ptr database) : fOnlyMixingAllowed(false), + m_args(args), m_chain(chain), m_coinjoin_loader(coinjoin_loader), m_name(name), diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 5bef33127b21..9afc100d7b86 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -56,7 +56,7 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag wallet_instance->TopUpKeyPool(); } -static const std::shared_ptr MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options) +static const std::shared_ptr MakeWallet(const std::string& name, const fs::path& path, const ArgsManager& args, DatabaseOptions options) { DatabaseStatus status; bilingual_str error; @@ -67,7 +67,7 @@ static const std::shared_ptr MakeWallet(const std::string& name, const } // dummy chain interface - std::shared_ptr wallet_instance{new CWallet(/*chain=*/ nullptr, /*coinjoin_loader=*/ nullptr, name, std::move(database)), WalletToolReleaseWallet}; + std::shared_ptr wallet_instance{new CWallet(/*chain=*/nullptr, /*coinjoin_loader=*/nullptr, name, args, std::move(database)), WalletToolReleaseWallet}; DBErrors load_wallet_ret; try { load_wallet_ret = wallet_instance->LoadWallet(); @@ -149,7 +149,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) options.require_format = DatabaseFormat::SQLITE; } - const std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, args, options); if (wallet_instance) { WalletShowInfo(wallet_instance.get()); wallet_instance->Close(); @@ -157,7 +157,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) } else if (command == "info") { DatabaseOptions options; options.require_existing = true; - const std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, args, options); if (!wallet_instance) return false; WalletShowInfo(wallet_instance.get()); wallet_instance->Close(); @@ -183,7 +183,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) #ifdef USE_BDB DatabaseOptions options; options.require_existing = true; - const std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, args, options); if (wallet_instance == nullptr) return false; std::vector vHash; @@ -210,7 +210,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) } else if (command == "dump") { DatabaseOptions options; options.require_existing = true; - const std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, args, options); if (!wallet_instance) return false; bilingual_str error; bool ret = DumpWallet(*wallet_instance, error); From c352062b1768dd5c730b2364dd05fbeebd6fb729 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 3 May 2025 22:28:15 +0000 Subject: [PATCH 04/12] trivial: invert `SelectCoins` condition check to move bail-out condition Required to simplify the diff in the next commit --- src/wallet/spend.cpp | 48 ++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f271723cc671..2751bd2cd692 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -461,34 +461,30 @@ bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCo for (const COutPoint& outpoint : vPresetInputs) { std::map::const_iterator it = wallet.mapWallet.find(outpoint.hash); - if (it != wallet.mapWallet.end()) - { - const CWalletTx& wtx = it->second; - // Clearly invalid input, fail - if (wtx.tx->vout.size() <= outpoint.n) { - return false; - } - if (nCoinType == CoinType::ONLY_FULLY_MIXED) { - // Make sure to include mixed preset inputs only, - // even if some non-mixed inputs were manually selected via CoinControl - if (!wallet.IsFullyMixed(outpoint)) continue; - } - // Just to calculate the marginal byte size - CInputCoin coin(wtx.tx, outpoint.n, GetTxSpendSize(wallet, wtx, outpoint.n, false)); - nValueFromPresetInputs += coin.txout.nValue; - if (coin.m_input_bytes <= 0) { - return false; // Not solvable, can't estimate size for fee - } - coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); - if (coin_selection_params.m_subtract_fee_outputs) { - value_to_select -= coin.txout.nValue; - } else { - value_to_select -= coin.effective_value; - } - setPresetCoins.insert(coin); + if (it == wallet.mapWallet.end()) return false; // TODO: Allow non-wallet inputs + const CWalletTx& wtx = it->second; + // Clearly invalid input, fail + if (wtx.tx->vout.size() <= outpoint.n) { + return false; + } + if (nCoinType == CoinType::ONLY_FULLY_MIXED) { + // Make sure to include mixed preset inputs only, + // even if some non-mixed inputs were manually selected via CoinControl + if (!wallet.IsFullyMixed(outpoint)) continue; + } + // Just to calculate the marginal byte size + CInputCoin coin(wtx.tx, outpoint.n, GetTxSpendSize(wallet, wtx, outpoint.n, false)); + nValueFromPresetInputs += coin.txout.nValue; + if (coin.m_input_bytes <= 0) { + return false; // Not solvable, can't estimate size for fee + } + coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); + if (coin_selection_params.m_subtract_fee_outputs) { + value_to_select -= coin.txout.nValue; } else { - return false; // TODO: Allow non-wallet inputs + value_to_select -= coin.effective_value; } + setPresetCoins.insert(coin); } // remove preset inputs from vCoins so that Coin Selection doesn't pick them. From c5038c1d2f934539c9d293254ed5a8ac0b6df6a9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 9 May 2025 19:11:07 +0000 Subject: [PATCH 05/12] merge bitcoin#22019: Introduce SelectionResult for encapsulating a coin selection solution --- src/bench/coin_selection.cpp | 15 +- src/wallet/coinselection.cpp | 115 +++++--- src/wallet/coinselection.h | 48 ++- src/wallet/spend.cpp | 150 +++++----- src/wallet/spend.h | 41 +-- src/wallet/test/coinselector_tests.cpp | 394 ++++++++++++++----------- 6 files changed, 446 insertions(+), 317 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index ac60e8c07df4..89e443e432be 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -53,12 +53,10 @@ static void CoinSelection(benchmark::Bench& bench) /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); bench.run([&] { - std::set setCoinsRet; - CAmount nValueRet; - bool success = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params); - assert(success); - assert(nValueRet == 1003 * COIN); - assert(setCoinsRet.size() == 2); + auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params); + assert(result); + assert(result->GetSelectedValue() == 1003 * COIN); + assert(result->GetInputSet().size() == 2); }); } @@ -91,17 +89,14 @@ static void BnBExhaustion(benchmark::Bench& bench) { // Setup std::vector utxo_pool; - CoinSet selection; - CAmount value_ret = 0; bench.run([&] { // Benchmark CAmount target = make_hard_case(17, utxo_pool); - SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust + SelectCoinsBnB(utxo_pool, target, 0); // Should exhaust // Cleanup utxo_pool.clear(); - selection.clear(); }); } diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 15db7d762dd2..a5796a0e6014 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -57,17 +57,14 @@ struct { * bound of the range. * @param const CAmount& cost_of_change This is the cost of creating and spending a change output. * This plus selection_target is the upper bound of the range. - * @param std::set& out_set -> This is an output parameter for the set of CInputCoins - * that have been selected. - * @param CAmount& value_ret -> This is an output parameter for the total value of the CInputCoins - * that were selected. + * @returns The result of this coin selection algorithm, or std::nullopt */ static const size_t TOTAL_TRIES = 100000; -bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret) +std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) { - out_set.clear(); + SelectionResult result(selection_target); CAmount curr_value = 0; std::vector curr_selection; // select the utxo at this index @@ -81,7 +78,7 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selectio curr_available_value += utxo.GetSelectionAmount(); } if (curr_available_value < selection_target) { - return false; + return std::nullopt; } // Sort the utxo_pool @@ -157,25 +154,22 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selectio // Check for solution if (best_selection.empty()) { - return false; + return std::nullopt; } // Set output set - value_ret = 0; for (size_t i = 0; i < best_selection.size(); ++i) { if (best_selection.at(i)) { - util::insert(out_set, utxo_pool.at(i).m_outputs); - value_ret += utxo_pool.at(i).m_value; + result.AddInput(utxo_pool.at(i)); } } - return true; + return result; } -std::optional, CAmount>> SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value) +std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value) { - std::set out_set; - CAmount value_ret = 0; + SelectionResult result(target_value); std::vector indexes; indexes.resize(utxo_pool.size()); @@ -187,10 +181,9 @@ std::optional, CAmount>> SelectCoinsSRD(const std const OutputGroup& group = utxo_pool.at(i); Assume(group.GetSelectionAmount() > 0); selected_eff_value += group.GetSelectionAmount(); - value_ret += group.m_value; - util::insert(out_set, group.m_outputs); + result.AddInput(group); if (selected_eff_value >= target_value) { - return std::make_pair(out_set, value_ret); + return result; } } return std::nullopt; @@ -269,10 +262,9 @@ bool less_then_denom (const OutputGroup& group1, const OutputGroup& group2) return (!found1 && found2); } -bool KnapsackSolver(const CAmount& nTargetValue, std::vector& groups, std::set& setCoinsRet, CAmount& nValueRet, bool fFulyMixedOnly, CAmount maxTxFee) +std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee) { - setCoinsRet.clear(); - nValueRet = 0; + SelectionResult result(nTargetValue); // List of values less than target std::optional lowest_larger; @@ -284,7 +276,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group int tryDenomStart = 0; CAmount nMinChange = MIN_CHANGE; - if (fFulyMixedOnly) { + if (fFullyMixedOnly) { // larger denoms first std::sort(groups.rbegin(), groups.rend(), CompareByPriority()); // we actually want denoms only, so let's skip "non-denom only" step @@ -307,9 +299,8 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group continue; // we don't want denom values on first run } if (group.GetSelectionAmount() == nTargetValue) { - util::insert(setCoinsRet, group.m_outputs); - nValueRet += group.m_value; - return true; + result.AddInput(group); + return result; } else if (group.GetSelectionAmount() < nTargetValue + nMinChange) { applicable_groups.push_back(group); nTotalLower += group.GetSelectionAmount(); @@ -320,10 +311,9 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group if (nTotalLower == nTargetValue) { for (const auto& group : applicable_groups) { - util::insert(setCoinsRet, group.m_outputs); - nValueRet += group.m_value; + result.AddInput(group); } - return true; + return result; } if (nTotalLower < nTargetValue) { @@ -333,13 +323,15 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group continue; else // we looked at everything possible and didn't find anything, no luck - return false; + return std::nullopt; } - util::insert(setCoinsRet, lowest_larger->m_outputs); - nValueRet += lowest_larger->m_value; + result.AddInput(*lowest_larger); // There is no change in PS, so we know the fee beforehand, // can see if we exceeded the max fee and thus fail quickly. - return fFulyMixedOnly ? (nValueRet - nTargetValue <= maxTxFee) : true; + if (!fFullyMixedOnly || (result.GetSelectedValue() - nTargetValue <= maxTxFee)) { + return result; + } + return std::nullopt; } // nTotalLower > nTargetValue @@ -360,14 +352,12 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group // or the next bigger coin is closer), return the bigger coin if (lowest_larger && ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->GetSelectionAmount() <= nBest)) { - util::insert(setCoinsRet, lowest_larger->m_outputs); - nValueRet += lowest_larger->m_value; + result.AddInput(*lowest_larger); } else { std::string log_message{"Coin selection best subset: "}; for (unsigned int i = 0; i < applicable_groups.size(); i++) { if (vfBest[i]) { - util::insert(setCoinsRet, applicable_groups[i].m_outputs); - nValueRet += applicable_groups[i].m_value; + result.AddInput(applicable_groups[i]); log_message += strprintf("%s ", FormatMoney(applicable_groups[i].m_value)); } } @@ -376,7 +366,10 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector& group // There is no change in PS, so we know the fee beforehand, // can see if we exceeded the max fee and thus fail quickly. - return fFulyMixedOnly ? (nValueRet - nTargetValue <= maxTxFee) : true; + if (!fFullyMixedOnly || (result.GetSelectedValue() - nTargetValue <= maxTxFee)) { + return result; + } + return std::nullopt; } /****************************************************************************** @@ -455,3 +448,51 @@ CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cos return waste; } + +void SelectionResult::ComputeAndSetWaste(CAmount change_cost) +{ + m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective); +} + +CAmount SelectionResult::GetWaste() const +{ + Assume(m_waste != std::nullopt); + return *m_waste; +} + +CAmount SelectionResult::GetSelectedValue() const +{ + return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin.txout.nValue; }); +} + +void SelectionResult::Clear() +{ + m_selected_inputs.clear(); + m_waste.reset(); +} + +void SelectionResult::AddInput(const OutputGroup& group) +{ + util::insert(m_selected_inputs, group.m_outputs); + m_use_effective = !group.m_subtract_fee_outputs; +} + +const std::set& SelectionResult::GetInputSet() const +{ + return m_selected_inputs; +} + +std::vector SelectionResult::GetShuffledInputVector() const +{ + std::vector coins(m_selected_inputs.begin(), m_selected_inputs.end()); + Shuffle(coins.begin(), coins.end(), FastRandomContext()); + return coins; +} + +bool SelectionResult::operator<(SelectionResult other) const +{ + Assume(m_waste != std::nullopt); + Assume(other.m_waste != std::nullopt); + // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal. + return *m_waste < *other.m_waste || (*m_waste == *other.m_waste && m_selected_inputs.size() > other.m_selected_inputs.size()); +} diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 5e900f8f2315..401d9886fb4b 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -185,6 +185,8 @@ struct OutputGroup * where excess = selected_effective_value - target * change_cost = effective_feerate * change_output_size + long_term_feerate * change_spend_size * + * Note this function is separate from SelectionResult for the tests. + * * @param[in] inputs The selected inputs * @param[in] change_cost The cost of creating change and spending it in the future. * Only used if there is change, in which case it must be positive. @@ -195,17 +197,55 @@ struct OutputGroup */ [[nodiscard]] CAmount GetSelectionWaste(const std::set& inputs, CAmount change_cost, CAmount target, bool use_effective_value = true); -bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret); +struct SelectionResult +{ +private: + /** Set of inputs selected by the algorithm to use in the transaction */ + std::set m_selected_inputs; + /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */ + const CAmount m_target; + /** Whether the input values for calculations should be the effective value (true) or normal value (false) */ + bool m_use_effective{false}; + /** The computed waste */ + std::optional m_waste; + +public: + explicit SelectionResult(const CAmount target) + : m_target(target) {} + + SelectionResult() = delete; + + /** Get the sum of the input values */ + [[nodiscard]] CAmount GetSelectedValue() const; + + void Clear(); + + void AddInput(const OutputGroup& group); + + /** Calculates and stores the waste for this selection via GetSelectionWaste */ + void ComputeAndSetWaste(CAmount change_cost); + [[nodiscard]] CAmount GetWaste() const; + + /** Get m_selected_inputs */ + const std::set& GetInputSet() const; + /** Get the vector of CInputCoins that will be used to fill in a CTransaction's vin */ + std::vector GetShuffledInputVector() const; + + bool operator<(SelectionResult other) const; +}; + +std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change); /** Select coins by Single Random Draw. OutputGroups are selected randomly from the eligible * outputs until the target is satisfied * * @param[in] utxo_pool The positive effective value OutputGroups eligible for selection * @param[in] target_value The target value to select for - * @returns If successful, a pair of set of outputs and total selected value, otherwise, std::nullopt + * @returns If successful, a SelectionResult, otherwise, std::nullopt */ -std::optional, CAmount>> SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value); +std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value); // Original coin selection algorithm as a fallback -bool KnapsackSolver(const CAmount& nTargetValue, std::vector& groups, std::set& setCoinsRet, CAmount& nValueRet, bool fFulyMixedOnly, CAmount maxTxFee); +std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee); + #endif // BITCOIN_WALLET_COINSELECTION_H diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 2751bd2cd692..72ae054a858f 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -363,24 +363,18 @@ std::vector GroupOutputs(const CWallet& wallet, const std::vector coins, - std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType) +std::optional AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, + const CoinSelectionParams& coin_selection_params, CoinType nCoinType) { - setCoinsRet.clear(); - nValueRet = 0; - // Vector of results for use with waste calculation - // In order: calculated waste, selected inputs, selected input value (sum of input values) - // TODO: Use a struct representing the selection result - std::vector, CAmount>> results; + // Vector of results. We will choose the best one based on waste. + std::vector results; // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output. std::vector positive_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */); - std::set bnb_coins; - CAmount bnb_value; // Note: BnB is disabled because it is unaware of mixed coins - if (/* DISABLES CODE */ (false) && SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, bnb_coins, bnb_value)) { - const auto waste = GetSelectionWaste(bnb_coins, /* cost of change */ CAmount(0), nTargetValue, !coin_selection_params.m_subtract_fee_outputs); - results.emplace_back(std::make_tuple(waste, std::move(bnb_coins), bnb_value)); + if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}; /* DISABLES CODE */ (false)) { + bnb_result->ComputeAndSetWaste(CAmount(0)); + results.push_back(*bnb_result); } // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. @@ -392,39 +386,32 @@ bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const if (!coin_selection_params.m_subtract_fee_outputs && nCoinType != CoinType::ONLY_FULLY_MIXED) { target_with_change += coin_selection_params.m_change_fee; } - std::set knapsack_coins; - CAmount knapsack_value; - if (KnapsackSolver(target_with_change, all_groups, knapsack_coins, knapsack_value, nCoinType == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)) { - const auto waste = GetSelectionWaste(knapsack_coins, coin_selection_params.m_cost_of_change, nTargetValue + coin_selection_params.m_change_fee, !coin_selection_params.m_subtract_fee_outputs); - results.emplace_back(std::make_tuple(waste, std::move(knapsack_coins), knapsack_value)); + if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, nCoinType == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)}) { + knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + results.push_back(*knapsack_result); } // We include the minimum final change for SRD as we do want to avoid making really small change. // KnapsackSolver does not need this because it includes MIN_CHANGE internally. const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + MIN_FINAL_CHANGE; - auto srd_result = SelectCoinsSRD(positive_groups, srd_target); // Note: SRD is disabled because it is unaware of mixed coins - if (/* DISABLES CODE */ (false) && srd_result != std::nullopt) { - const auto waste = GetSelectionWaste(srd_result->first, coin_selection_params.m_cost_of_change, srd_target, !coin_selection_params.m_subtract_fee_outputs); - results.emplace_back(std::make_tuple(waste, std::move(srd_result->first), srd_result->second)); + if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target)}; /* DISABLES CODE */ (false)) { + srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + results.push_back(*srd_result); } if (results.size() == 0) { // No solution found - return false; + return std::nullopt; } // Choose the result with the least waste // If the waste is the same, choose the one which spends more inputs. - const auto& best_result = std::min_element(results.begin(), results.end(), [](const auto& a, const auto& b) { - return std::get<0>(a) < std::get<0>(b) || (std::get<0>(a) == std::get<0>(b) && std::get<1>(a).size() > std::get<1>(b).size()); - }); - setCoinsRet = std::get<1>(*best_result); - nValueRet = std::get<2>(*best_result); - return true; + auto& best_result = *std::min_element(results.begin(), results.end()); + return best_result; } -bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params) +std::optional SelectCoins(const CWallet& wallet, const std::vector& vAvailableCoins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { // Note: this function should never be used for "always free" tx types like dstx @@ -432,40 +419,41 @@ bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCo CoinType nCoinType = coin_control.nCoinType; CAmount value_to_select = nTargetValue; + OutputGroup preset_inputs(coin_selection_params); + // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) { - for (const COutput& out : vCoins) - { - if(!out.fSpendable) - continue; - - nValueRet += out.tx->tx->vout[out.i].nValue; - setCoinsRet.insert(out.GetInputCoin()); - - if (!coin_control.fRequireAllInputs && nValueRet >= nTargetValue) { + SelectionResult result(nTargetValue); + for (const COutput& out : vCoins) { + if (!out.fSpendable) continue; + /* Set depth, from_me, ancestors, and descendants to 0 or false as these don't matter for preset inputs as no actual selection is being done. + * positive_only is set to false because we want to include all preset inputs, even if they are dust. + */ + preset_inputs.Insert(out.GetInputCoin(), 0, false, 0, 0, false); + result.AddInput(preset_inputs); + if (!coin_control.fRequireAllInputs && result.GetSelectedValue() >= nTargetValue) { // stop when we added at least one input and enough inputs to have at least nTargetValue funds - return true; + return result; } } - - return (nValueRet >= nTargetValue); + if (result.GetSelectedValue() < nTargetValue) return std::nullopt; + return result; } // calculate value from preset inputs and store them std::set setPresetCoins; - CAmount nValueFromPresetInputs = 0; std::vector vPresetInputs; coin_control.ListSelected(vPresetInputs); for (const COutPoint& outpoint : vPresetInputs) { std::map::const_iterator it = wallet.mapWallet.find(outpoint.hash); - if (it == wallet.mapWallet.end()) return false; // TODO: Allow non-wallet inputs + if (it == wallet.mapWallet.end()) return std::nullopt; // TODO: Allow non-wallet inputs const CWalletTx& wtx = it->second; // Clearly invalid input, fail if (wtx.tx->vout.size() <= outpoint.n) { - return false; + return std::nullopt; } if (nCoinType == CoinType::ONLY_FULLY_MIXED) { // Make sure to include mixed preset inputs only, @@ -474,9 +462,8 @@ bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCo } // Just to calculate the marginal byte size CInputCoin coin(wtx.tx, outpoint.n, GetTxSpendSize(wallet, wtx, outpoint.n, false)); - nValueFromPresetInputs += coin.txout.nValue; if (coin.m_input_bytes <= 0) { - return false; // Not solvable, can't estimate size for fee + return std::nullopt; // Not solvable, can't estimate size for fee } coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); if (coin_selection_params.m_subtract_fee_outputs) { @@ -485,6 +472,10 @@ bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCo value_to_select -= coin.effective_value; } setPresetCoins.insert(coin); + /* Set depth, from_me, ancestors, and descendants to 0 or false as don't matter for preset inputs as no actual selection is being done. + * positive_only is set to false because we want to include all preset inputs, even if they are dust. + */ + preset_inputs.Insert(coin, 0, false, 0, 0, false); } // remove preset inputs from vCoins so that Coin Selection doesn't pick them. @@ -514,60 +505,62 @@ bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCo // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the // transaction at a target feerate. If an attempt fails, more attempts may be made using a more // permissive CoinEligibilityFilter. - const bool res = [&] { + std::optional res = [&] { // Pre-selected inputs already cover the target amount. - if (value_to_select <= 0) return true; + if (value_to_select <= 0) return std::make_optional(SelectionResult(nTargetValue)); // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six // confirmations on outputs received from other wallets and only spend confirmed change. - if (AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; - if (AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; + if (auto r1{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 6, 0), vCoins, coin_selection_params, nCoinType)}) return r1; + if (auto r2{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, coin_selection_params, nCoinType)}) return r2; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as // possible) if we cannot fund the transaction otherwise. if (wallet.m_spend_zero_conf_change) { - if (AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) return true; - if (AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), - vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { - return true; + if (auto r3{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, coin_selection_params, nCoinType)}) return r3; + if (auto r4{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), + vCoins, coin_selection_params, nCoinType)}) { + return r4; } - if (AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), - vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { - return true; + if (auto r5{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), + vCoins, coin_selection_params, nCoinType)}) { + return r5; } // If partial groups are allowed, relax the requirement of spending OutputGroups (groups // of UTXOs sent to the same address, which are obviously controlled by a single wallet) // in their entirety. - if (AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { - return true; + if (auto r6{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), + vCoins, coin_selection_params, nCoinType)}) { + return r6; } // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs // received from other wallets. - if (coin_control.m_include_unsafe_inputs - && AttemptSelection(wallet, value_to_select, + if (coin_control.m_include_unsafe_inputs) { + if (auto r7{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { - return true; + vCoins, coin_selection_params, nCoinType)}) { + return r7; + } } // Try with unlimited ancestors/descendants. The transaction will still need to meet // mempool ancestor/descendant policy to be accepted to mempool and broadcasted, but // OutputGroups use heuristics that may overestimate ancestor/descendant counts. - if (!fRejectLongChains && AttemptSelection(wallet, value_to_select, + if (!fRejectLongChains) { + if (auto r8{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max(), std::numeric_limits::max(), true /* include_partial_groups */), - vCoins, setCoinsRet, nValueRet, coin_selection_params, nCoinType)) { - return true; + vCoins, coin_selection_params, nCoinType)}) { + return r8; + } } } // Coin Selection failed. - return false; + return std::optional(); }(); - // AttemptSelection clears setCoinsRet, so add the preset inputs from coin_control to the coinset - util::insert(setCoinsRet, setPresetCoins); + if (!res) return std::nullopt; - // add preset inputs to the total value selected - nValueRet += nValueFromPresetInputs; + // Add preset inputs to result + res->AddInput(preset_inputs); return res; } @@ -774,9 +767,8 @@ static bool CreateTransactionInternal( AvailableCoins(wallet, vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); // Choose coins to use - CAmount inputs_sum = 0; - std::set setCoins; - if (!SelectCoins(wallet, vAvailableCoins, /* nTargetValue */ selection_target, setCoins, inputs_sum, coin_control, coin_selection_params)) { + std::optional result = SelectCoins(wallet, vAvailableCoins, /* nTargetValue */ selection_target, coin_control, coin_selection_params); + if (!result) { if (coin_control.nCoinType == CoinType::ONLY_NONDENOMINATED) { error = _("Unable to locate enough non-denominated funds for this transaction."); } else if (coin_control.nCoinType == CoinType::ONLY_FULLY_MIXED) { @@ -790,7 +782,7 @@ static bool CreateTransactionInternal( // Always make a change output // We will reduce the fee from this change output later, and remove the output if it is too small. - const CAmount change_and_fee = inputs_sum - recipients_sum; + const CAmount change_and_fee = result->GetSelectedValue() - recipients_sum; assert(change_and_fee >= 0); CTxOut newTxOut(change_and_fee, scriptChange); @@ -830,7 +822,7 @@ static bool CreateTransactionInternal( // The sequence number is set to non-maxint so that DiscourageFeeSniping // works. const uint32_t nSequence{CTxIn::SEQUENCE_FINAL - 1}; - for (const auto& coin : setCoins) { + for (const auto& coin : result->GetInputSet()) { txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence)); } DiscourageFeeSniping(txNew, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight()); @@ -872,7 +864,7 @@ static bool CreateTransactionInternal( fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); } - nFeeRet = inputs_sum - recipients_sum - change_amount; + nFeeRet = result->GetSelectedValue() - recipients_sum - change_amount; // Update nFeeRet in case fee_needed changed due to dropping the change output if (fee_needed <= change_and_fee - change_amount) { diff --git a/src/wallet/spend.h b/src/wallet/spend.h index ff23ea46967d..e5590015c75d 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -95,29 +95,34 @@ std::map> ListCoins(const CWallet& wallet) std::vector GroupOutputs(const CWallet& wallet, const std::vector& outputs, const CoinSelectionParams& coin_sel_params, const CoinEligibilityFilter& filter, bool positive_only); /** - * Shuffle and select coins until nTargetValue is reached while avoiding - * small change; This method is stochastic for some inputs and upon - * completion the coin set and corresponding actual target value is - * assembled - * param@[in] coins Set of UTXOs to consider. These will be categorized into - * OutputGroups and filtered using eligibility_filter before - * selecting coins. - * param@[out] setCoinsRet Populated with the coins selected if successful. - * param@[out] nValueRet Used to return the total value of selected coins. + * Attempt to find a valid input set that meets the provided eligibility filter and target. + * Multiple coin selection algorithms will be run and the input set that produces the least waste + * (according to the waste metric) will be chosen. + * + * param@[in] wallet The wallet which provides solving data for the coins + * param@[in] nTargetValue The target value + * param@[in] eligibility_filter A filter containing rules for which coins are allowed to be included in this selection + * param@[in] coins The vector of coins available for selection prior to filtering + * param@[in] coin_selection_params Parameters for the coin selection + * returns If successful, a SelectionResult containing the input set + * If failed, a nullopt */ -bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, - std::set& setCoinsRet, CAmount& nValueRet, const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS); +std::optional AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, std::vector coins, + const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS); /** - * Select a set of coins such that nValueRet >= nTargetValue and at least + * Select a set of coins such that nTargetValue is met and at least * all coins from coin_control are selected; never select unconfirmed coins if they are not ours - * param@[out] setCoinsRet Populated with inputs including pre-selected inputs from - * coin_control and Coin Selection if successful. - * param@[out] nValueRet Total value of selected coins including pre-selected ones - * from coin_control and Coin Selection if successful. + * param@[in] wallet The wallet which provides data necessary to spend the selected coins + * param@[in] vAvailableCoins The vector of coins available to be spent + * param@[in] nTargetValue The target value + * param@[in] coin_selection_params Parameters for this coin selection such as feerates, whether to avoid partial spends, + * and whether to subtract the fee from the outputs. + * returns If successful, a SelectionResult containing the selected coins + * If failed, a nullopt. */ -bool SelectCoins(const CWallet& wallet, const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, - const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); +std::optional SelectCoins(const CWallet& wallet, const std::vector& vAvailableCoins, const CAmount& nTargetValue, const CCoinControl& coin_control, + const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /** * Create a new transaction paying the recipients with a set of coins diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 287d59fa291e..feb57a383abb 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -33,20 +34,35 @@ typedef std::set CoinSet; static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); +static int nextLockTime = 0; static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { CMutableTransaction tx; tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes set.emplace_back(MakeTransactionRef(tx), nInput); } +static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) +{ + CMutableTransaction tx; + tx.vout.resize(nInput + 1); + tx.vout[nInput].nValue = nValue; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes + CInputCoin coin(MakeTransactionRef(tx), nInput); + OutputGroup group; + group.Insert(coin, 1, false, 0, 0, true); + result.AddInput(group); +} + static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fee = 0, CAmount long_term_fee = 0) { CMutableTransaction tx; tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; + tx.nLockTime = nextLockTime++; // so all transactions get different hashes CInputCoin coin(MakeTransactionRef(tx), nInput); coin.effective_value = nValue - fee; coin.m_fee = fee; @@ -56,7 +72,6 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set, CAmount fe static void add_coin(std::vector& coins, CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false) { - static int nextLockTime = 0; CMutableTransaction tx; tx.nLockTime = nextLockTime++; // so all transactions get different hashes tx.vout.resize(nInput + 1); @@ -88,10 +103,30 @@ static void add_coin(std::vector& coins, CWallet& wallet, const CAmount coins.push_back(output); } -static bool equal_sets(CoinSet a, CoinSet b) +/** Check if SelectionResult a is equivalent to SelectionResult b. + * Equivalent means same input values, but maybe different inputs (i.e. same value, different prevout) */ +static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) +{ + std::vector a_amts; + std::vector b_amts; + for (const auto& coin : a.GetInputSet()) { + a_amts.push_back(coin.txout.nValue); + } + for (const auto& coin : b.GetInputSet()) { + b_amts.push_back(coin.txout.nValue); + } + std::sort(a_amts.begin(), a_amts.end()); + std::sort(b_amts.begin(), b_amts.end()); + + std::pair::iterator, std::vector::iterator> ret = std::mismatch(a_amts.begin(), a_amts.end(), b_amts.begin()); + return ret.first == a_amts.end() && ret.second == b_amts.end(); +} + +/** Check if this selection is equal to another one. Equal means same inputs (i.e same value and prevout) */ +static bool EqualResult(const SelectionResult& a, const SelectionResult& b) { - std::pair ret = mismatch(a.begin(), a.end(), b.begin()); - return ret.first == a.end() && ret.second == b.end(); + std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin()); + return ret.first == a.GetInputSet().end() && ret.second == b.GetInputSet().end(); } static CAmount make_hard_case(int utxos, std::vector& utxo_pool) @@ -128,9 +163,9 @@ inline std::vector& GroupCoins(const std::vector& coins) return static_groups; } -inline bool KnapsackSolver(const CAmount& nTargetValue, std::vector& groups, std::set& setCoinsRet, CAmount& nValueRet) +inline std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue) { - return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet, /*fFulyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE); + return KnapsackSolver(groups, nTargetValue, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE); } inline std::vector& KnapsackGroupOutputs(const std::vector& coins, CWallet& wallet, const CoinEligibilityFilter& filter) @@ -149,17 +184,14 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) { // Setup std::vector utxo_pool; - CoinSet selection; - CoinSet actual_selection; - CAmount value_ret = 0; + SelectionResult expected_result(CAmount(0)); ///////////////////////// // Known Outcome tests // ///////////////////////// // Empty utxo pool - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret)); - selection.clear(); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT)); // Add utxos add_coin(1 * CENT, 1, utxo_pool); @@ -168,87 +200,86 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(4 * CENT, 4, utxo_pool); // Select 1 Cent - add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret)); - BOOST_CHECK(equal_sets(selection, actual_selection)); - BOOST_CHECK_EQUAL(value_ret, 1 * CENT); - actual_selection.clear(); - selection.clear(); + add_coin(1 * CENT, 1, expected_result); + const auto result1 = SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT); + BOOST_CHECK(result1); + BOOST_CHECK(EquivalentResult(expected_result, *result1)); + BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT); + expected_result.Clear(); // Select 2 Cent - add_coin(2 * CENT, 2, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret)); - BOOST_CHECK(equal_sets(selection, actual_selection)); - BOOST_CHECK_EQUAL(value_ret, 2 * CENT); - actual_selection.clear(); - selection.clear(); + add_coin(2 * CENT, 2, expected_result); + const auto result2 = SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT); + BOOST_CHECK(result2); + BOOST_CHECK(EquivalentResult(expected_result, *result2)); + BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 2 * CENT); + expected_result.Clear(); // Select 5 Cent - add_coin(4 * CENT, 4, actual_selection); - add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret)); - BOOST_CHECK(equal_sets(selection, actual_selection)); - BOOST_CHECK_EQUAL(value_ret, 5 * CENT); - actual_selection.clear(); - selection.clear(); + add_coin(4 * CENT, 4, expected_result); + add_coin(1 * CENT, 1, expected_result); + const auto result3 = SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT); + BOOST_CHECK(result3); + BOOST_CHECK(EquivalentResult(expected_result, *result3)); + BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 5 * CENT); + expected_result.Clear(); // Select 11 Cent, not possible - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret)); - actual_selection.clear(); - selection.clear(); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT)); + expected_result.Clear(); // Cost of change is greater than the difference between target value and utxo sum - add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret)); - BOOST_CHECK_EQUAL(value_ret, 1 * CENT); - BOOST_CHECK(equal_sets(selection, actual_selection)); - actual_selection.clear(); - selection.clear(); + add_coin(1 * CENT, 1, expected_result); + const auto result4 = SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT); + BOOST_CHECK(result4); + BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 1 * CENT); + BOOST_CHECK(EquivalentResult(expected_result, *result4)); + expected_result.Clear(); // Cost of change is less than the difference between target value and utxo sum - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0, selection, value_ret)); - actual_selection.clear(); - selection.clear(); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0)); + expected_result.Clear(); // Select 10 Cent add_coin(5 * CENT, 5, utxo_pool); - add_coin(5 * CENT, 5, actual_selection); - add_coin(4 * CENT, 4, actual_selection); - add_coin(1 * CENT, 1, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret)); - BOOST_CHECK(equal_sets(selection, actual_selection)); - BOOST_CHECK_EQUAL(value_ret, 10 * CENT); - actual_selection.clear(); - selection.clear(); + add_coin(5 * CENT, 5, expected_result); + add_coin(4 * CENT, 4, expected_result); + add_coin(1 * CENT, 1, expected_result); + const auto result5 = SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT); + BOOST_CHECK(result5); + BOOST_CHECK(EquivalentResult(expected_result, *result5)); + BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 10 * CENT); + expected_result.Clear(); // Negative effective value // Select 10 Cent but have 1 Cent not be possible because too small - add_coin(5 * CENT, 5, actual_selection); - add_coin(3 * CENT, 3, actual_selection); - add_coin(2 * CENT, 2, actual_selection); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret)); - BOOST_CHECK_EQUAL(value_ret, 10 * CENT); + add_coin(5 * CENT, 5, expected_result); + add_coin(3 * CENT, 3, expected_result); + add_coin(2 * CENT, 2, expected_result); + const auto result6 = SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000); + BOOST_CHECK(result6); + BOOST_CHECK_EQUAL(result6->GetSelectedValue(), 10 * CENT); // FIXME: this test is redundant with the above, because 1 Cent is selected, not "too small" - // BOOST_CHECK(equal_sets(selection, actual_selection)); + // BOOST_CHECK(EquivalentResult(expected_result, *result)); // Select 0.25 Cent, not possible - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret)); - actual_selection.clear(); - selection.clear(); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT)); + expected_result.Clear(); // Iteration exhaustion test CAmount target = make_hard_case(17, utxo_pool); - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should exhaust + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0)); // Should exhaust target = make_hard_case(14, utxo_pool); - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should not exhaust + const auto result7 = SelectCoinsBnB(GroupCoins(utxo_pool), target, 0); // Should not exhaust + BOOST_CHECK(result7); // Test same value early bailout optimization utxo_pool.clear(); - add_coin(7 * CENT, 7, actual_selection); - add_coin(7 * CENT, 7, actual_selection); - add_coin(7 * CENT, 7, actual_selection); - add_coin(7 * CENT, 7, actual_selection); - add_coin(2 * CENT, 7, actual_selection); + add_coin(7 * CENT, 7, expected_result); + add_coin(7 * CENT, 7, expected_result); + add_coin(7 * CENT, 7, expected_result); + add_coin(7 * CENT, 7, expected_result); + add_coin(2 * CENT, 7, expected_result); add_coin(7 * CENT, 7, utxo_pool); add_coin(7 * CENT, 7, utxo_pool); add_coin(7 * CENT, 7, utxo_pool); @@ -257,9 +288,10 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) for (int i = 0; i < 50000; ++i) { add_coin(5 * CENT, 7, utxo_pool); } - BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret)); - BOOST_CHECK_EQUAL(value_ret, 30 * CENT); - BOOST_CHECK(equal_sets(selection, actual_selection)); + const auto result8 = SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000); + BOOST_CHECK(result8); + BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 30 * CENT); + BOOST_CHECK(EquivalentResult(expected_result, *result8)); //////////////////// // Behavior tests // @@ -271,7 +303,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } // Run 100 times, to make sure it is never finding a solution for (int i = 0; i < 100; ++i) { - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT)); } // Make sure that effective value is working in AttemptSelection when BnB is used @@ -287,20 +319,19 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) wallet->SetupDescriptorScriptPubKeyMans(); std::vector coins; - CoinSet setCoinsRet; - CAmount nValueRet; add_coin(coins, *wallet, 1); coins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail - BOOST_CHECK(!SelectCoinsBnB(GroupCoins(coins), 1 * CENT, coin_selection_params_bnb.m_cost_of_change, setCoinsRet, nValueRet)); + BOOST_CHECK(!SelectCoinsBnB(GroupCoins(coins), 1 * CENT, coin_selection_params_bnb.m_cost_of_change)); // Test fees subtracted from output: coins.clear(); add_coin(coins, *wallet, 1 * CENT); coins.at(0).nInputBytes = 40; coin_selection_params_bnb.m_subtract_fee_outputs = true; - BOOST_CHECK(SelectCoinsBnB(GroupCoins(coins), 1 * CENT, coin_selection_params_bnb.m_cost_of_change, setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); + const auto result9 = SelectCoinsBnB(GroupCoins(coins), 1 * CENT, coin_selection_params_bnb.m_cost_of_change); + BOOST_CHECK(result9); + BOOST_CHECK_EQUAL(result9->GetSelectedValue(), 1 * CENT); } { @@ -311,8 +342,6 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) wallet->SetupDescriptorScriptPubKeyMans(); std::vector coins; - CoinSet setCoinsRet; - CAmount nValueRet; add_coin(coins, *wallet, 5 * CENT, 6 * 24, false, 0, true); add_coin(coins, *wallet, 3 * CENT, 6 * 24, false, 0, true); @@ -321,7 +350,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_control.fAllowOtherInputs = true; coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i)); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); - BOOST_CHECK(SelectCoins(*wallet, coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb)); + const auto result10 = SelectCoins(*wallet, coins, 10 * CENT, coin_control, coin_selection_params_bnb); + BOOST_CHECK(result10); } } @@ -333,8 +363,6 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet->SetupDescriptorScriptPubKeyMans(); - CoinSet setCoinsRet, setCoinsRet2; - CAmount nValueRet; std::vector coins; // test multiple times to allow for differences in the shuffle order @@ -343,25 +371,27 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) coins.clear(); // with an empty wallet we can't even pay one cent - BOOST_CHECK(!KnapsackSolver(1 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_standard), setCoinsRet, nValueRet)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1 * CENT)); add_coin(coins, *wallet, 1*CENT, 4); // add a new 1 cent coin // with a new 1 cent coin, we still can't find a mature 1 cent - BOOST_CHECK(!KnapsackSolver(1 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_standard), setCoinsRet, nValueRet)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1 * CENT)); // but we can find a new 1 cent - BOOST_CHECK(KnapsackSolver(1 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); + const auto result1 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 1 * CENT); + BOOST_CHECK(result1); + BOOST_CHECK_EQUAL(result1->GetSelectedValue(), 1 * CENT); add_coin(coins, *wallet, 2*CENT); // add a mature 2 cent coin // we can't make 3 cents of mature coins - BOOST_CHECK(!KnapsackSolver(3 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_standard), setCoinsRet, nValueRet)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 3 * CENT)); // we can make 3 cents of new coins - BOOST_CHECK(KnapsackSolver(3 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 3 * CENT); + const auto result2 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 3 * CENT); + BOOST_CHECK(result2); + BOOST_CHECK_EQUAL(result2->GetSelectedValue(), 3 * CENT); add_coin(coins, *wallet, 5*CENT); // add a mature 5 cent coin, add_coin(coins, *wallet, 10*CENT, 3, true); // a new 10 cent coin sent from one of our own addresses @@ -370,35 +400,41 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // now we have new: 1+10=11 (of which 10 was self-sent), and mature: 2+5+20=27. total = 38 // we can't make 38 cents only if we disallow new coins: - BOOST_CHECK(!KnapsackSolver(38 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_standard), setCoinsRet, nValueRet)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 38 * CENT)); // we can't even make 37 cents if we don't allow new coins even if they're from us - BOOST_CHECK(!KnapsackSolver(38 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_standard_extra), setCoinsRet, nValueRet)); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard_extra), 38 * CENT)); // but we can make 37 cents if we accept new coins from ourself - BOOST_CHECK(KnapsackSolver(37 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_standard), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 37 * CENT); + const auto result3 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 37 * CENT); + BOOST_CHECK(result3); + BOOST_CHECK_EQUAL(result3->GetSelectedValue(), 37 * CENT); // and we can make 38 cents if we accept all new coins - BOOST_CHECK(KnapsackSolver(38 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 38 * CENT); + const auto result4 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 38 * CENT); + BOOST_CHECK(result4); + BOOST_CHECK_EQUAL(result4->GetSelectedValue(), 38 * CENT); // try making 34 cents from 1,2,5,10,20 - we can't do it exactly - BOOST_CHECK(KnapsackSolver(34 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 35 * CENT); // but 35 cents is closest - BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) + const auto result5 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 34 * CENT); + BOOST_CHECK(result5); + BOOST_CHECK_EQUAL(result5->GetSelectedValue(), 35 * CENT); // but 35 cents is closest + BOOST_CHECK_EQUAL(result5->GetInputSet().size(), 3U); // the best should be 20+10+5. it's incredibly unlikely the 1 or 2 got included (but possible) // when we try making 7 cents, the smaller coins (1,2,5) are enough. We should see just 2+5 - BOOST_CHECK(KnapsackSolver(7 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 7 * CENT); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); + const auto result6 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 7 * CENT); + BOOST_CHECK(result6); + BOOST_CHECK_EQUAL(result6->GetSelectedValue(), 7 * CENT); + BOOST_CHECK_EQUAL(result6->GetInputSet().size(), 2U); // when we try making 8 cents, the smaller coins (1,2,5) are exactly enough. - BOOST_CHECK(KnapsackSolver(8 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK(nValueRet == 8 * CENT); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); + const auto result7 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 8 * CENT); + BOOST_CHECK(result7); + BOOST_CHECK(result7->GetSelectedValue() == 8 * CENT); + BOOST_CHECK_EQUAL(result7->GetInputSet().size(), 3U); // when we try making 9 cents, no subset of smaller coins is enough, and we get the next bigger coin (10) - BOOST_CHECK(KnapsackSolver(9 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 10 * CENT); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + const auto result8 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 9 * CENT); + BOOST_CHECK(result8); + BOOST_CHECK_EQUAL(result8->GetSelectedValue(), 10 * CENT); + BOOST_CHECK_EQUAL(result8->GetInputSet().size(), 1U); // now clear out the wallet and start again to test choosing between subsets of smaller coins and the next biggest coin coins.clear(); @@ -410,45 +446,52 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, 30*CENT); // now we have 6+7+8+20+30 = 71 cents total // check that we have 71 and not 72 - BOOST_CHECK(KnapsackSolver(71 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK(!KnapsackSolver(72 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); + const auto result9 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 71 * CENT); + BOOST_CHECK(result9); + BOOST_CHECK(!KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 72 * CENT)); // now try making 16 cents. the best smaller coins can do is 6+7+8 = 21; not as good at the next biggest coin, 20 - BOOST_CHECK(KnapsackSolver(16 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 20 * CENT); // we should get 20 in one coin - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + const auto result10 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 16 * CENT); + BOOST_CHECK(result10); + BOOST_CHECK_EQUAL(result10->GetSelectedValue(), 20 * CENT); // we should get 20 in one coin + BOOST_CHECK_EQUAL(result10->GetInputSet().size(), 1U); add_coin(coins, *wallet, 5*CENT); // now we have 5+6+7+8+20+30 = 75 cents total // now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, better than the next biggest coin, 20 - BOOST_CHECK(KnapsackSolver(16 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 3 coins - BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); + const auto result11 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 16 * CENT); + BOOST_CHECK(result11); + BOOST_CHECK_EQUAL(result11->GetSelectedValue(), 18 * CENT); // we should get 18 in 3 coins + BOOST_CHECK_EQUAL(result11->GetInputSet().size(), 3U); add_coin(coins, *wallet, 18*CENT); // now we have 5+6+7+8+18+20+30 // and now if we try making 16 cents again, the smaller coins can make 5+6+7 = 18 cents, the same as the next biggest coin, 18 - BOOST_CHECK(KnapsackSolver(16 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 18 * CENT); // we should get 18 in 1 coin - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); // because in the event of a tie, the biggest coin wins + const auto result12 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 16 * CENT); + BOOST_CHECK(result12); + BOOST_CHECK_EQUAL(result12->GetSelectedValue(), 18 * CENT); // we should get 18 in 1 coin + BOOST_CHECK_EQUAL(result12->GetInputSet().size(), 1U); // because in the event of a tie, the biggest coin wins // now try making 11 cents. we should get 5+6 - BOOST_CHECK(KnapsackSolver(11 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 11 * CENT); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); + const auto result13 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 11 * CENT); + BOOST_CHECK(result13); + BOOST_CHECK_EQUAL(result13->GetSelectedValue(), 11 * CENT); + BOOST_CHECK_EQUAL(result13->GetInputSet().size(), 2U); // check that the smallest bigger coin is used add_coin(coins, *wallet, 1*COIN); add_coin(coins, *wallet, 2*COIN); add_coin(coins, *wallet, 3*COIN); add_coin(coins, *wallet, 4*COIN); // now we have 5+6+7+8+18+20+30+100+200+300+400 = 1094 cents - BOOST_CHECK(KnapsackSolver(95 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * COIN); // we should get 1 BTC in 1 coin - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + const auto result14 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 95 * CENT); + BOOST_CHECK(result14); + BOOST_CHECK_EQUAL(result14->GetSelectedValue(), 1 * COIN); // we should get 1 BTC in 1 coin + BOOST_CHECK_EQUAL(result14->GetInputSet().size(), 1U); - BOOST_CHECK(KnapsackSolver(195 * CENT, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 2 * COIN); // we should get 2 BTC in 1 coin - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + const auto result15 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 195 * CENT); + BOOST_CHECK(result15); + BOOST_CHECK_EQUAL(result15->GetSelectedValue(), 2 * COIN); // we should get 2 BTC in 1 coin + BOOST_CHECK_EQUAL(result15->GetInputSet().size(), 1U); // empty the wallet and start again, now with fractions of a cent, to test small change avoidance @@ -461,23 +504,26 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // try making 1 * MIN_CHANGE from the 1.5 * MIN_CHANGE // we'll get change smaller than MIN_CHANGE whatever happens, so can expect MIN_CHANGE exactly - BOOST_CHECK(KnapsackSolver(MIN_CHANGE, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); + const auto result16 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), MIN_CHANGE); + BOOST_CHECK(result16); + BOOST_CHECK_EQUAL(result16->GetSelectedValue(), MIN_CHANGE); // but if we add a bigger coin, small change is avoided add_coin(coins, *wallet, 1111*MIN_CHANGE); // try making 1 from 0.1 + 0.2 + 0.3 + 0.4 + 0.5 + 1111 = 1112.5 - BOOST_CHECK(KnapsackSolver(1 * MIN_CHANGE, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount + const auto result17 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 1 * MIN_CHANGE); + BOOST_CHECK(result17); + BOOST_CHECK_EQUAL(result17->GetSelectedValue(), 1 * MIN_CHANGE); // we should get the exact amount // if we add more small coins: add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 7 / 10); // and try again to make 1.0 * MIN_CHANGE - BOOST_CHECK(KnapsackSolver(1 * MIN_CHANGE, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1 * MIN_CHANGE); // we should get the exact amount + const auto result18 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 1 * MIN_CHANGE); + BOOST_CHECK(result18); + BOOST_CHECK_EQUAL(result18->GetSelectedValue(), 1 * MIN_CHANGE); // we should get the exact amount // run the 'mtgox' test (see https://blockexplorer.com/tx/29a3efd3ef04f9153d47a990bd7b048a4b2d213daaa5fb8ed670fb85f13bdbcf) // they tried to consolidate 10 50k coins into one 500k coin, and ended up with 50k in change @@ -485,9 +531,10 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int j = 0; j < 20; j++) add_coin(coins, *wallet, 50000 * COIN); - BOOST_CHECK(KnapsackSolver(500000 * COIN, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 500000 * COIN); // we should get the exact amount - BOOST_CHECK_EQUAL(setCoinsRet.size(), 10U); // in ten coins + const auto result19 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 500000 * COIN); + BOOST_CHECK(result19); + BOOST_CHECK_EQUAL(result19->GetSelectedValue(), 500000 * COIN); // we should get the exact amount + BOOST_CHECK_EQUAL(result19->GetInputSet().size(), 10U); // in ten coins // if there's not enough in the smaller coins to make at least 1 * MIN_CHANGE change (0.5+0.6+0.7 < 1.0+1.0), // we need to try finding an exact subset anyway @@ -498,9 +545,10 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 7 / 10); add_coin(coins, *wallet, 1111 * MIN_CHANGE); - BOOST_CHECK(KnapsackSolver(1 * MIN_CHANGE, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1111 * MIN_CHANGE); // we get the bigger coin - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + const auto result20 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 1 * MIN_CHANGE); + BOOST_CHECK(result20); + BOOST_CHECK_EQUAL(result20->GetSelectedValue(), 1111 * MIN_CHANGE); // we get the bigger coin + BOOST_CHECK_EQUAL(result20->GetInputSet().size(), 1U); // but sometimes it's possible, and we use an exact subset (0.4 + 0.6 = 1.0) coins.clear(); @@ -508,9 +556,10 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 6 / 10); add_coin(coins, *wallet, MIN_CHANGE * 8 / 10); add_coin(coins, *wallet, 1111 * MIN_CHANGE); - BOOST_CHECK(KnapsackSolver(MIN_CHANGE, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE); // we should get the exact amount - BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); // in two coins 0.4+0.6 + const auto result21 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), MIN_CHANGE); + BOOST_CHECK(result21); + BOOST_CHECK_EQUAL(result21->GetSelectedValue(), MIN_CHANGE); // we should get the exact amount + BOOST_CHECK_EQUAL(result21->GetInputSet().size(), 2U); // in two coins 0.4+0.6 // test avoiding small change coins.clear(); @@ -519,14 +568,16 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) add_coin(coins, *wallet, MIN_CHANGE * 100); // trying to make 100.01 from these three coins - BOOST_CHECK(KnapsackSolver(MIN_CHANGE * 10001 / 100, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, MIN_CHANGE * 10105 / 100); // we should get all coins - BOOST_CHECK_EQUAL(setCoinsRet.size(), 3U); + const auto result22 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), MIN_CHANGE * 10001 / 100); + BOOST_CHECK(result22); + BOOST_CHECK_EQUAL(result22->GetSelectedValue(), MIN_CHANGE * 10105 / 100); // we should get all coins + BOOST_CHECK_EQUAL(result22->GetInputSet().size(), 3U); // but if we try to make 99.9, we should take the bigger of the two small coins to avoid small change - BOOST_CHECK(KnapsackSolver(MIN_CHANGE * 9990 / 100, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 101 * MIN_CHANGE); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); + const auto result23 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), MIN_CHANGE * 9990 / 100); + BOOST_CHECK(result23); + BOOST_CHECK_EQUAL(result23->GetSelectedValue(), 101 * MIN_CHANGE); + BOOST_CHECK_EQUAL(result23->GetInputSet().size(), 2U); } // test with many inputs @@ -538,18 +589,19 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // We only create the wallet once to save time, but we still run the coin selection RUN_TESTS times. for (int i = 0; i < RUN_TESTS; i++) { - BOOST_CHECK(KnapsackSolver(2000, KnapsackGroupOutputs(coins, *wallet, filter_confirmed), setCoinsRet, nValueRet)); + const auto result24 = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_confirmed), 2000); + BOOST_CHECK(result24); if (amt - 2000 < MIN_CHANGE) { // needs more than one input: uint16_t returnSize = std::ceil((2000.0 + MIN_CHANGE)/amt); CAmount returnValue = amt * returnSize; - BOOST_CHECK_EQUAL(nValueRet, returnValue); - BOOST_CHECK_EQUAL(setCoinsRet.size(), returnSize); + BOOST_CHECK_EQUAL(result24->GetSelectedValue(), returnValue); + BOOST_CHECK_EQUAL(result24->GetInputSet().size(), returnSize); } else { // one input is sufficient: - BOOST_CHECK_EQUAL(nValueRet, amt); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 1U); + BOOST_CHECK_EQUAL(result24->GetSelectedValue(), amt); + BOOST_CHECK_EQUAL(result24->GetInputSet().size(), 1U); } } } @@ -564,9 +616,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) for (int i = 0; i < RUN_TESTS; i++) { // picking 50 from 100 coins doesn't depend on the shuffle, // but does depend on randomness in the stochastic approximation code - BOOST_CHECK(KnapsackSolver(50 * COIN, GroupCoins(coins), setCoinsRet, nValueRet)); - BOOST_CHECK(KnapsackSolver(50 * COIN, GroupCoins(coins), setCoinsRet2, nValueRet)); - BOOST_CHECK(!equal_sets(setCoinsRet, setCoinsRet2)); + const auto result25 = KnapsackSolver(GroupCoins(coins), 50 * COIN); + BOOST_CHECK(result25); + const auto result26 = KnapsackSolver(GroupCoins(coins), 50 * COIN); + BOOST_CHECK(result26); + BOOST_CHECK(!EqualResult(*result25, *result26)); int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) @@ -575,9 +629,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) // When choosing 1 from 100 identical coins, 1% of the time, this test will choose the same coin twice // which will cause it to fail. // To avoid that issue, run the test RANDOM_REPEATS times and only complain if all of them fail - BOOST_CHECK(KnapsackSolver(COIN, GroupCoins(coins), setCoinsRet, nValueRet)); - BOOST_CHECK(KnapsackSolver(COIN, GroupCoins(coins), setCoinsRet2, nValueRet)); - if (equal_sets(setCoinsRet, setCoinsRet2)) + const auto result27 = KnapsackSolver(GroupCoins(coins), COIN); + BOOST_CHECK(result27); + const auto result28 = KnapsackSolver(GroupCoins(coins), COIN); + BOOST_CHECK(result28); + if (EqualResult(*result27, *result28)) fails++; } BOOST_CHECK_NE(fails, RANDOM_REPEATS); @@ -596,9 +652,11 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test) int fails = 0; for (int j = 0; j < RANDOM_REPEATS; j++) { - BOOST_CHECK(KnapsackSolver(90*CENT, GroupCoins(coins), setCoinsRet, nValueRet)); - BOOST_CHECK(KnapsackSolver(90*CENT, GroupCoins(coins), setCoinsRet2, nValueRet)); - if (equal_sets(setCoinsRet, setCoinsRet2)) + const auto result29 = KnapsackSolver(GroupCoins(coins), 90 * CENT); + BOOST_CHECK(result29); + const auto result30 = KnapsackSolver(GroupCoins(coins), 90 * CENT); + BOOST_CHECK(result30); + if (EqualResult(*result29, *result30)) fails++; } BOOST_CHECK_NE(fails, RANDOM_REPEATS); @@ -614,8 +672,6 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet->SetupDescriptorScriptPubKeyMans(); - CoinSet setCoinsRet; - CAmount nValueRet; std::vector coins; // Test vValue sort order @@ -623,9 +679,10 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset) add_coin(coins, *wallet, 1000 * COIN); add_coin(coins, *wallet, 3 * COIN); - BOOST_CHECK(KnapsackSolver(1003 * COIN, KnapsackGroupOutputs(coins, *wallet, filter_standard), setCoinsRet, nValueRet)); - BOOST_CHECK_EQUAL(nValueRet, 1003 * COIN); - BOOST_CHECK_EQUAL(setCoinsRet.size(), 2U); + const auto result = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1003 * COIN); + BOOST_CHECK(result); + BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN); + BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U); } // Tests that with the ideal conditions, the coin selector will always be able to find a solution that can pay the target value @@ -667,11 +724,10 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), /* tx_noinputs_size= */ 0, /* avoid_partial= */ false); - CoinSet out_set; - CAmount out_value = 0; CCoinControl cc; - BOOST_CHECK(SelectCoins(*wallet, coins, target, out_set, out_value, cc, cs_params)); - BOOST_CHECK_GE(out_value, target); + const auto result = SelectCoins(*wallet, coins, target, cc, cs_params); + BOOST_CHECK(result); + BOOST_CHECK_GE(result->GetSelectedValue(), target); } } From d9ee3dbe07b4c7403b8cc986cdab770ac8206b4c Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 9 May 2025 16:41:58 +0300 Subject: [PATCH 06/12] wallet: fast-fail coin selection when supplied empty output groups --- src/wallet/coinselection.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index a5796a0e6014..7c9d58b75d76 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -64,6 +64,8 @@ static const size_t TOTAL_TRIES = 100000; std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) { + if (utxo_pool.empty()) return std::nullopt; + SelectionResult result(selection_target); CAmount curr_value = 0; @@ -169,6 +171,8 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value) { + if (utxo_pool.empty()) return std::nullopt; + SelectionResult result(target_value); std::vector indexes; @@ -264,6 +268,8 @@ bool less_then_denom (const OutputGroup& group1, const OutputGroup& group2) std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee) { + if (groups.empty()) return std::nullopt; + SelectionResult result(nTargetValue); // List of values less than target From 66b530262e98c875ae5c6592c9420c04c1233e68 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 9 May 2025 16:43:10 +0300 Subject: [PATCH 07/12] wallet: trigger coin selection fast-fail condition over ignoring result --- src/wallet/spend.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 72ae054a858f..0500b17a5932 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -371,8 +371,8 @@ std::optional AttemptSelection(const CWallet& wallet, const CAm // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output. std::vector positive_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */); - // Note: BnB is disabled because it is unaware of mixed coins - if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}; /* DISABLES CODE */ (false)) { + positive_groups.clear(); // Cleared to skip BnB and SRD as they're unaware of mixed coins + if (auto bnb_result{SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) { bnb_result->ComputeAndSetWaste(CAmount(0)); results.push_back(*bnb_result); } @@ -394,8 +394,7 @@ std::optional AttemptSelection(const CWallet& wallet, const CAm // We include the minimum final change for SRD as we do want to avoid making really small change. // KnapsackSolver does not need this because it includes MIN_CHANGE internally. const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + MIN_FINAL_CHANGE; - // Note: SRD is disabled because it is unaware of mixed coins - if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target)}; /* DISABLES CODE */ (false)) { + if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target)}) { srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); results.push_back(*srd_result); } From 84d9194a9cfef399fad4f34eae51370c0440ac9f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 13 Dec 2021 14:45:48 +0100 Subject: [PATCH 08/12] merge bitcoin#23762: Replace Assume with Assert where needed in coinselection --- src/wallet/coinselection.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 7c9d58b75d76..0854eed079c0 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -462,8 +462,7 @@ void SelectionResult::ComputeAndSetWaste(CAmount change_cost) CAmount SelectionResult::GetWaste() const { - Assume(m_waste != std::nullopt); - return *m_waste; + return *Assert(m_waste); } CAmount SelectionResult::GetSelectedValue() const @@ -497,8 +496,8 @@ std::vector SelectionResult::GetShuffledInputVector() const bool SelectionResult::operator<(SelectionResult other) const { - Assume(m_waste != std::nullopt); - Assume(other.m_waste != std::nullopt); + Assert(m_waste.has_value()); + Assert(other.m_waste.has_value()); // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal. return *m_waste < *other.m_waste || (*m_waste == *other.m_waste && m_selected_inputs.size() > other.m_selected_inputs.size()); } From 3260e07a1d244414f3e6670318b6b71e76bdd64d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 4 May 2025 12:34:38 +0000 Subject: [PATCH 09/12] merge bitcoin#24067: Actually treat (un)confirmed txs as (un)confirmed --- src/interfaces/chain.h | 3 -- src/interfaces/wallet.h | 1 - src/node/interfaces.cpp | 5 --- src/qt/transactiondesc.cpp | 9 ------ src/qt/transactionrecord.cpp | 15 +-------- src/qt/transactionrecord.h | 2 -- src/qt/transactiontablemodel.cpp | 9 ------ src/wallet/interfaces.cpp | 1 - src/wallet/receive.cpp | 2 -- src/wallet/rpcwallet.cpp | 8 ++--- src/wallet/spend.cpp | 3 -- test/functional/test_runner.py | 1 + test/functional/wallet_timelock.py | 50 ++++++++++++++++++++++++++++++ 13 files changed, 56 insertions(+), 53 deletions(-) create mode 100755 test/functional/wallet_timelock.py diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 8006de91dbeb..6a8dd5bbc3c8 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -133,9 +133,6 @@ class Chain //! or one of its ancestors. virtual std::optional findLocatorFork(const CBlockLocator& locator) = 0; - //! Check if transaction will be final given chain height current time. - virtual bool checkFinalTx(const CTransaction& tx) = 0; - //! Check if transaction is locked by InstantSendManager virtual bool isInstantSendLockedTx(const uint256& hash) = 0; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 9feef86db5c9..c0fbd86cfc2f 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -433,7 +433,6 @@ struct WalletTxStatus int depth_in_main_chain; unsigned int time_received; uint32_t lock_time; - bool is_final; bool is_trusted; bool is_abandoned; bool is_coinbase; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index b2a2aff11319..3f1a7e9517e3 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -771,11 +771,6 @@ class ChainImpl : public Chain } return std::nullopt; } - bool checkFinalTx(const CTransaction& tx) override - { - LOCK(cs_main); - return CheckFinalTxAtTip(*Assert(m_node.chainman->ActiveChain().Tip()), tx); - } bool isInstantSendLockedTx(const uint256& hash) override { if (m_node.llmq_ctx == nullptr || m_node.llmq_ctx->isman == nullptr) return false; diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index e50d87bf60d3..47f8918b2056 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -18,7 +18,6 @@ #include #include #include -#include