phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-10 20:03:34 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#17526: Add Single Random Draw as an additional coin selection algorithm

Browse source 
3633b667ff Use SelectCoinsSRD if it has less waste (Andrew Chow)
8bf789b4b4 Add SelectCoinsSRD function (Andrew Chow)
2ad3b5d2ad tests: wallet_basic lock needed unspents (Andrew Chow)
b77885f13e tests: wallet_txn explicilty specify inputs (Andrew Chow)
59ba7d2861 tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow)
a165bfbe44 tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow)
df765a484d 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 3633b667ff

Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92
This commit is contained in:
Samuel Dobson 2021-09-03 21:27:47 +12:00
commit d5d0a5c604
No known key found for this signature in database
GPG key ID: D300116E1C875A3D
7 changed files with 135 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

26
src/wallet/coinselection.cpp
View file

@ -5,9 +5,11 @@
#include <wallet/coinselection.h>
#include <policy/feerate.h>
#include <util/check.h>
#include <util/system.h>
#include <util/moneystr.h>
#include <numeric>
#include <optional>
// Descending order comparator
@ -168,6 +170,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)
{

11
src/wallet/coinselection.h
View file

@ -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
@ -185,6 +187,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);

9
src/wallet/spend.cpp
View file

@ -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;

78
test/functional/rpc_fundrawtransaction.py
View file

@ -47,7 +47,40 @@ class RawTransactionsTest(BitcoinTestFramework):
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 @@ class RawTransactionsTest(BitcoinTestFramework):
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 @@ class RawTransactionsTest(BitcoinTestFramework):
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 @@ class RawTransactionsTest(BitcoinTestFramework):
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 @@ class RawTransactionsTest(BitcoinTestFramework):
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 @@ class RawTransactionsTest(BitcoinTestFramework):
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")
@ -542,15 +587,18 @@ class RawTransactionsTest(BitcoinTestFramework):
# 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)
@ -944,31 +992,31 @@ class RawTransactionsTest(BitcoinTestFramework):
# 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

4
test/functional/wallet_basic.py
View file

@ -13,6 +13,7 @@ from test_framework.util import (
assert_equal,
assert_fee_amount,
assert_raises_rpc_error,
find_vout_for_address,
)
from test_framework.wallet_util import test_address
@ -463,6 +464,9 @@ class WalletTest(BitcoinTestFramework):
# 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])

13
test/functional/wallet_txn_clone.py
View file

@ -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 @@ class TxnMallTest(BitcoinTestFramework):
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 @@ class TxnMallTest(BitcoinTestFramework):
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 @@ class TxnMallTest(BitcoinTestFramework):
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)

13
test/functional/wallet_txn_doublespend.py
View file

@ -9,6 +9,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
find_output,
find_vout_for_address
)
@ -29,6 +30,13 @@ class TxnMallTest(BitcoinTestFramework):
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 @@ class TxnMallTest(BitcoinTestFramework):
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 @@ class TxnMallTest(BitcoinTestFramework):
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):