From df765a484d84702f066e127813fa45a7d440a957 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 26 Aug 2021 19:21:25 -0400 Subject: [PATCH 1/7] tests: rpc_fundrawtx lock to UTXO types For some of the tests within rpc_fundrawtx, there is the expectation that two independent calls to coin selection RPCs will use the same type of UTXO. This is not necessarily guaranteed, so to make sure it is, use lockunspent prior to those tests. --- test/functional/rpc_fundrawtransaction.py | 45 +++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 56312dc6e5..17102f84d4 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -47,7 +47,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 @@ -373,6 +406,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():1.1} rawtx = self.nodes[0].createrawtransaction(inputs, outputs) @@ -386,9 +420,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():1.1, @@ -409,8 +446,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() @@ -433,9 +473,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() @@ -474,6 +517,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") From a165bfbe44b4db3a40b8d1ba8095035367c932a7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 26 Aug 2021 20:32:18 -0400 Subject: [PATCH 2/7] tests: rpc_fundrawtx use specific inputs for unavailable change test For the test that checks that there is no error when change is unavailable but change is also not needed, use specific UTXOs so that SRD does not cause this to fail when it chooses random inputs. --- test/functional/rpc_fundrawtransaction.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 17102f84d4..5264e501e0 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -587,15 +587,18 @@ def test_locked_wallet(self): # Drain the keypool. self.nodes[1].getnewaddress() self.nodes[1].getrawchangeaddress() - inputs = [] - outputs = {self.nodes[0].getnewaddress():1.19999500} + + # Choose 2 inputs + inputs = self.nodes[1].listunspent()[0:2] + value = sum(inp["amount"] for inp in inputs) - Decimal("0.00000500") # Pay a 500 sat fee + 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 self.nodes[1].fundrawtransaction(rawtx) # 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.", self.nodes[1].fundrawtransaction, rawtx) From 59ba7d2861359f19fa740da9daad1a199409583f Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 26 Aug 2021 20:33:22 -0400 Subject: [PATCH 3/7] tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe Don't assume that specific inputs are going to be used when they aren't specified explicitly. Also fixes a bug in the include_unsafe test where after the inputs confirm, include_unsafe should be set to False rather than True. --- test/functional/rpc_fundrawtransaction.py | 24 +++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 5264e501e0..cda0ae0eeb 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -992,31 +992,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 From b77885f13e480304085c654f1b948b20bba63452 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 6 Nov 2019 16:09:36 -0500 Subject: [PATCH 4/7] tests: wallet_txn explicilty specify inputs Instead of relying on coin selection to deterministically choose the correct inputs to use, just specify them explicitly and use the raw transaction RPCs. --- test/functional/wallet_txn_clone.py | 13 +++++++++++-- test/functional/wallet_txn_doublespend.py | 13 +++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/test/functional/wallet_txn_clone.py b/test/functional/wallet_txn_clone.py index 3eb525a9bc..7f178d7d46 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, @@ -33,6 +34,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): if self.options.segwit: output_type = "p2sh-segwit" @@ -49,6 +57,7 @@ def run_test(self): node0_address1 = self.nodes[0].getnewaddress(address_type=output_type) node0_txid1 = self.nodes[0].sendtoaddress(node0_address1, 1219) 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(address_type=output_type) node0_txid2 = self.nodes[0].sendtoaddress(node0_address2, 29) @@ -61,8 +70,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, 40) - txid2 = self.nodes[0].sendtoaddress(node1_address, 20) + txid1 = self.spend_txid(node0_txid1, find_vout_for_address(self.nodes[0], node0_txid1, node0_address1), {node1_address: 40}) + txid2 = self.spend_txid(node0_txid2, find_vout_for_address(self.nodes[0], node0_txid2, node0_address2), {node1_address: 20}) # 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 bfa171d913..150e4083b9 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 1,250 BTC: starting_balance = 1250 @@ -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, 1219) 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, 29) @@ -77,8 +86,8 @@ def run_test(self): assert_equal(doublespend["complete"], True) # Create two spends using 1 50 BTC coin each - txid1 = self.nodes[0].sendtoaddress(node1_address, 40) - txid2 = self.nodes[0].sendtoaddress(node1_address, 20) + txid1 = self.spend_txid(fund_foo_txid, find_vout_for_address(self.nodes[0], fund_foo_txid, node0_address_foo), {node1_address: 40}) + txid2 = self.spend_txid(fund_bar_txid, find_vout_for_address(self.nodes[0], fund_bar_txid, node0_address_bar), {node1_address: 20}) # Have node0 mine a block: if (self.options.mine_block): From 2ad3b5d2ad03f781f564ee697ef11e18b2edcea3 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 2 Sep 2021 15:25:13 -0400 Subject: [PATCH 5/7] tests: wallet_basic lock needed unspents To avoid accidentally spending UTXOs that are needed later in the test, lock those UTXOs after they're creation. --- test/functional/wallet_basic.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 6372e1acd7..950dd82699 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -13,6 +13,7 @@ assert_equal, assert_fee_amount, assert_raises_rpc_error, + find_vout_for_address, ) from test_framework.wallet_util import test_address @@ -427,6 +428,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) self.sync_all(self.nodes[0:3]) From 8bf789b4b4b26082aea1d91c4d7aa8b01aedfdcf Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 6 Nov 2019 14:35:11 -0500 Subject: [PATCH 6/7] Add SelectCoinsSRD function --- src/wallet/coinselection.cpp | 26 ++++++++++++++++++++++++++ src/wallet/coinselection.h | 11 +++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 1699424657..d2f30abf23 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -5,9 +5,11 @@ #include #include +#include #include #include +#include #include // Descending order comparator @@ -168,6 +170,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 35617d455b..0f07017ef2 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); From 3633b667ffca5a715d9fb27e977515c1e24f600a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 20 May 2021 20:20:49 -0400 Subject: [PATCH 7/7] Use SelectCoinsSRD if it has less waste Try to find a solution with SelectCoinsSRD. If we do have one, add it to the list of solutions from which we choose the one with the least waste as the solution to use. --- src/wallet/spend.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4a7a268982..1724375f4c 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -387,6 +387,15 @@ 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); + if (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;