From e7573b6f2b8053236bd6e3bc1bd547321a9f6f79 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Fri, 3 Sep 2021 21:27:47 +1200 Subject: [PATCH] Merge bitcoin/bitcoin#17526: Add Single Random Draw as an additional coin selection algorithm 3633b667ffca5a715d9fb27e977515c1e24f600a Use SelectCoinsSRD if it has less waste (Andrew Chow) 8bf789b4b4b26082aea1d91c4d7aa8b01aedfdcf Add SelectCoinsSRD function (Andrew Chow) 2ad3b5d2ad03f781f564ee697ef11e18b2edcea3 tests: wallet_basic lock needed unspents (Andrew Chow) b77885f13e480304085c654f1b948b20bba63452 tests: wallet_txn explicilty specify inputs (Andrew Chow) 59ba7d2861359f19fa740da9daad1a199409583f tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow) a165bfbe44b4db3a40b8d1ba8095035367c932a7 tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow) df765a484d84702f066e127813fa45a7d440a957 tests: rpc_fundrawtx lock to UTXO types (Andrew Chow) Pull request description: To ease in the use of SRD as our fallback mechanism, this PR adds it as a secondary fallback algorithm in addition to the knapsack solver. Since #22009, the solution with the least waste will be chosen. This pattern is continued with SRD simply being another solution whose waste is compared. ACKs for top commit: glozow: reACK 3633b66 via `git range-diff 981b9d1...3633b66`, thanks for taking the suggestions laanwj: Concept and code review ACK 3633b667ffca5a715d9fb27e977515c1e24f600a Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92 --- src/wallet/coinselection.cpp | 26 ++++++++ src/wallet/coinselection.h | 11 ++++ test/functional/rpc_fundrawtransaction.py | 73 ++++++++++++++++++----- test/functional/wallet_basic.py | 5 ++ test/functional/wallet_txn_clone.py | 13 +++- test/functional/wallet_txn_doublespend.py | 15 ++++- 6 files changed, 124 insertions(+), 19 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index be9fdbb1c2f33c..f0948fc695f62b 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 @@ -173,6 +175,30 @@ bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_v 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 0734927ad28e8e..32b84890dcf2ea 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 @@ -122,6 +124,15 @@ struct OutputGroup bool SelectCoinsBnB(std::vector& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set& out_set, CAmount& value_ret, CAmount not_input_fees); +/** 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/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 300f3d7fcf2090..33fc246f869642 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -48,7 +48,40 @@ 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("] + elif outputtype in ["p2sh-segwit", "sh_wpkh"]: + prefixes = ["sh(wpkh(", "sh(wsh("] + elif outputtype in ["bech32", "wpkh"]: + prefixes = ["wpkh(", "wsh("] + 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 @@ -361,6 +394,7 @@ def test_invalid_input(self): def test_fee_p2pkh(self): """Compare fee of a standard pubkeyhash transaction.""" self.log.info("Test fundrawtxn p2pkh fee") + self.lock_outputs_type(self.nodes[0], "p2pkh") inputs = [] outputs = {self.nodes[1].getnewaddress():11} rawtx = self.nodes[0].createrawtransaction(inputs, outputs) @@ -374,9 +408,12 @@ def test_fee_p2pkh(self): feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee) assert feeDelta >= 0 and feeDelta <= self.fee_tolerance + self.unlock_utxos(self.nodes[0]) + 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, @@ -397,8 +434,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() @@ -421,9 +461,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() @@ -462,6 +505,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") @@ -529,7 +574,7 @@ def test_locked_wallet(self): # Drain the keypool. self.nodes[1].getnewaddress() - inputs = self.nodes[1].listunspent() + inputs = self.nodes[1].listunspent()[0:2] # Deduce fee to produce a changeless transaction # 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") @@ -540,7 +585,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) @@ -824,31 +869,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.nodes[0].generate(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"] if __name__ == '__main__': diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index ea2b49b9949461..c195a0d3956850 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -13,6 +13,7 @@ assert_fee_amount, assert_raises_rpc_error, count_bytes, + find_vout_for_address, ) from test_framework.wallet_util import test_address @@ -421,6 +422,10 @@ def run_test(self): address_to_import = self.nodes[2].getnewaddress() txid = self.nodes[0].sendtoaddress(address_to_import, 1) self.nodes[0].generate(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) self.sync_all(self.nodes[0:3]) # send with explicit dash/kb fee diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py index 820f73513cac0a..d28d90543abaa1 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 322a4bebb9b6ff..11df39ef05ab30 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) @@ -76,9 +85,9 @@ def run_test(self): doublespend = self.nodes[0].signrawtransactionwithwallet(rawtx) 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) + # Create two spends using 1 500 DASH coin coin each + 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):