Merge bitcoin#17526: Add Single Random Draw as an additional coin sel…

…ection algorithm

3633b66 Use SelectCoinsSRD if it has less waste (Andrew Chow)
8bf789b Add SelectCoinsSRD function (Andrew Chow)
2ad3b5d tests: wallet_basic lock needed unspents (Andrew Chow)
b77885f tests: wallet_txn explicilty specify inputs (Andrew Chow)
59ba7d2 tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow)
a165bfb tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow)
df765a4 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 bitcoin#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 3633b66

Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92

src/wallet/coinselection.cpp

@@ -5,11 +5,13 @@
 #include <wallet/coinselection.h>
 
 #include <policy/feerate.h>
+#include <util/check.h>
 #include <util/system.h>
 #include <util/moneystr.h>
 
 #include <coinjoin/common.h>
 
+#include <numeric>
 #include <optional>
 
 // Descending order comparator
@@ -173,6 +175,30 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_v
  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

@@ -10,6 +10,8 @@
 #include <primitives/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
@@ -122,6 +124,15 @@ struct OutputGroup
 
 bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& 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<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, bool fFulyMixedOnly, CAmount maxTxFee);
 #endif // BITCOIN_WALLET_COINSELECTION_H

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__':

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

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)

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):