Merge d5d0a5c into merged_master (Bitcoin PR bitcoin/bitcoin#17526)

src/wallet/coinselection.cpp

@@ -7,9 +7,11 @@
 
 #include <policy/feerate.h>
 #include <policy/policy.h>
+#include <util/check.h>
 #include <util/system.h>
 #include <util/moneystr.h>
 
+#include <numeric>
 #include <optional>
 
 CInputCoin::CInputCoin(const CWallet& wallet, const CWalletTx* wtx, unsigned int i) {
@@ -185,6 +187,30 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selectio
     return true;
 }
 
+std::optional<std::pair<std::set<CInputCoin>, CAmount>> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value)
+{
+    std::set<CInputCoin> out_set;
+    CAmount value_ret = 0;
+
+    std::vector<size_t> 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<OutputGroup>& groups, const CAmount& nTotalLower, const CAmount& nTargetValue,
                                   std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
 {

src/wallet/coinselection.h

@@ -12,6 +12,8 @@
 #include <primitives/bitcoin/transaction.h>
 #include <random.h>
 
+#include <optional>
+
 //! target minimum change amount
 static constexpr CAmount MIN_CHANGE{COIN / 100};
 //! final minimum change amount after paying for fees
@@ -221,6 +223,15 @@ struct OutputGroup
 
 bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& 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<std::pair<std::set<CInputCoin>, CAmount>> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value);
+
 // Original coin selection algorithm as a fallback
 bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& groups, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet);
 

src/wallet/spend.cpp

@@ -460,6 +460,17 @@ bool AttemptSelection(const CWallet& wallet, const CAmountMap& mapTargetValue, c
             const CAmountMap bnb_value_map {{asset, bnb_value}};
             results.emplace_back(std::make_tuple(waste, std::move(bnb_coins), bnb_value_map));
         }
+
+        // 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);
+            std::set<CInputCoin> srd_coins = srd_result->first;
+            const CAmountMap srd_value_map {{asset, srd_result->second}};
+            results.emplace_back(std::make_tuple(waste, std::move(srd_coins), srd_value_map));
+        }
     }
 
     // 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.

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
@@ -400,6 +433,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)
@@ -413,9 +447,12 @@ def test_fee_p2pkh(self):
         feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee)
         assert 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},
@@ -440,8 +477,11 @@ def test_fee_p2pkh_multi_out(self):
         feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee)
         assert 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()
@@ -464,9 +504,12 @@ def test_fee_p2sh(self):
         feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee)
         assert 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()
@@ -505,6 +548,8 @@ def test_fee_4of5(self):
         feeDelta = Decimal(fundedTx['fee']) - Decimal(signedFee)
         assert 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")
@@ -574,9 +619,12 @@ def test_locked_wallet(self):
         # Drain the keypool.
         self.nodes[1].getnewaddress()
         self.nodes[1].getrawchangeaddress()
+
         inputs = []
-        outputs = [{self.nodes[0].getnewaddress():1.09997500}]
+        value = 1.09997500
+        outputs = [{self.nodes[0].getnewaddress():value}]
         rawtx = self.nodes[1].createrawtransaction(inputs, outputs)
+        print(self.nodes[1].decoderawtransaction(rawtx))
         # fund a transaction that does not require a new key for the change output
         self.nodes[1].fundrawtransaction(rawtx)
 
@@ -1015,33 +1063,33 @@ 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"])
         blindedtx = wallet.blindrawtransaction(fundedtx['hex'])
         signedtx = wallet.signrawtransactionwithwallet(blindedtx)
-        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"])
         blindedtx = wallet.blindrawtransaction(fundedtx['hex'])
         signedtx = wallet.signrawtransactionwithwallet(blindedtx)
-        wallet.sendrawtransaction(signedtx['hex'])
+        assert wallet.testmempoolaccept([signedtx['hex']])[0]["allowed"]
 
     def test_22670(self):
         # In issue #22670, it was observed that ApproximateBestSubset may

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
 
@@ -465,6 +466,9 @@ def run_test(self):
             # 1. Send some coins to generate new UTXO
             address_to_import = self.nodes[2].getaddressinfo(self.nodes[2].getnewaddress())["confidential"]
             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])
 

test/functional/wallet_groups.py

@@ -61,6 +61,11 @@ def run_test(self):
         # one output should be 0.2, the other should be ~0.3
         v = [vout["value"] for vout in tx1["vout"] if vout["scriptPubKey"]["type"] != "fee"]
         v.sort()
+        # JAMES/BYRON DELETE ME
+        #print(self.nodes[1].listunspent())
+        print("vin amount = {}".format(self.nodes[0].gettxout(tx1["vin"][0]["txid"], tx1["vin"][0]["vout"], True)["value"]))
+        print("vout amounts = {}".format(v))
+        # END JAMES?BYRON
         assert_approx(v[0], vexp=0.2, vspan=0.0001)
         assert_approx(v[1], vexp=0.3, vspan=0.0001)
 

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)

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)
@@ -75,8 +84,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):