mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-02-08 10:31:50 -05:00
Merge #16377: [rpc] don't automatically append inputs in walletcreatefundedpsbt
e5327f947c[rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost)79804fe24b[rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost) Pull request description: When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`. Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`. This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection. ACKs for top commit: achow101: ACKe5327f947cmeshcollider: utACKe5327f947cTree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb
This commit is contained in:
Samuel Dobson
commit
6bb5f6d8e3
7 changed files with 67 additions and 20 deletions
9
doc/release-notes-16377.md
Normal file
9
doc/release-notes-16377.md
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
RPC changes
|
||||
-----------
|
||||
- The `walletcreatefundedpsbt` RPC call will now fail with
|
||||
`Insufficient funds` when inputs are manually selected but are not enough to cover
|
||||
the outputs and fee. Additional inputs can automatically be added through the
|
||||
new `add_inputs` option.
|
||||
|
||||
- The `fundrawtransaction` RPC now supports `add_inputs` option that when `false`
|
||||
prevents adding more inputs if necessary and consequently the RPC fails.
|
||||
|
|
@ -10,6 +10,7 @@ void CCoinControl::SetNull()
|
|||
{
|
||||
destChange = CNoDestination();
|
||||
m_change_type.reset();
|
||||
m_add_inputs = true;
|
||||
fAllowOtherInputs = false;
|
||||
fAllowWatchOnly = false;
|
||||
m_avoid_partial_spends = gArgs.GetBoolArg("-avoidpartialspends", DEFAULT_AVOIDPARTIALSPENDS);
|
||||
|
|
@ -23,4 +24,3 @@ void CCoinControl::SetNull()
|
|||
m_min_depth = DEFAULT_MIN_DEPTH;
|
||||
m_max_depth = DEFAULT_MAX_DEPTH;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -26,6 +26,8 @@ public:
|
|||
CTxDestination destChange;
|
||||
//! Override the default change type if set, ignored if destChange is set
|
||||
Optional<OutputType> m_change_type;
|
||||
//! If false, only selected inputs are used
|
||||
bool m_add_inputs;
|
||||
//! If false, allows unselected inputs, but requires all selected inputs be used
|
||||
bool fAllowOtherInputs;
|
||||
//! Includes watch only addresses which are solvable
|
||||
|
|
|
|||
|
|
@ -2918,13 +2918,12 @@ static UniValue listunspent(const JSONRPCRequest& request)
|
|||
return results;
|
||||
}
|
||||
|
||||
void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options)
|
||||
void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl)
|
||||
{
|
||||
// Make sure the results are valid at least up to the most recent block
|
||||
// the user could have gotten from another RPC command prior to now
|
||||
pwallet->BlockUntilSyncedToCurrentChain();
|
||||
|
||||
CCoinControl coinControl;
|
||||
change_position = -1;
|
||||
bool lockUnspents = false;
|
||||
UniValue subtractFeeFromOutputs;
|
||||
|
|
@ -2939,6 +2938,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
|
|||
RPCTypeCheckArgument(options, UniValue::VOBJ);
|
||||
RPCTypeCheckObj(options,
|
||||
{
|
||||
{"add_inputs", UniValueType(UniValue::VBOOL)},
|
||||
{"changeAddress", UniValueType(UniValue::VSTR)},
|
||||
{"changePosition", UniValueType(UniValue::VNUM)},
|
||||
{"change_type", UniValueType(UniValue::VSTR)},
|
||||
|
|
@ -2952,6 +2952,10 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
|
|||
},
|
||||
true, true);
|
||||
|
||||
if (options.exists("add_inputs") ) {
|
||||
coinControl.m_add_inputs = options["add_inputs"].get_bool();
|
||||
}
|
||||
|
||||
if (options.exists("changeAddress")) {
|
||||
CTxDestination dest = DecodeDestination(options["changeAddress"].get_str());
|
||||
|
||||
|
|
@ -3039,8 +3043,8 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
|
|||
static UniValue fundrawtransaction(const JSONRPCRequest& request)
|
||||
{
|
||||
RPCHelpMan{"fundrawtransaction",
|
||||
"\nAdd inputs to a transaction until it has enough in value to meet its out value.\n"
|
||||
"This will not modify existing inputs, and will add at most one change output to the outputs.\n"
|
||||
"\nIf the transaction has no inputs, they will be automatically selected to meet its out value.\n"
|
||||
"It will add at most one change output to the outputs.\n"
|
||||
"No existing outputs will be modified unless \"subtractFeeFromOutputs\" is specified.\n"
|
||||
"Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n"
|
||||
"The inputs added will not be signed, use signrawtransactionwithkey\n"
|
||||
|
|
@ -3054,6 +3058,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
|
|||
{"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
|
||||
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}",
|
||||
{
|
||||
{"add_inputs", RPCArg::Type::BOOL, /* default */ "true", "For a transaction with existing inputs, automatically include more if they are not enough."},
|
||||
{"changeAddress", RPCArg::Type::STR, /* default */ "pool address", "The bitcoin address to receive the change"},
|
||||
{"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
|
||||
{"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if changeAddress is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
|
||||
|
|
@ -3123,7 +3128,10 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
|
|||
|
||||
CAmount fee;
|
||||
int change_position;
|
||||
FundTransaction(pwallet, tx, fee, change_position, request.params[1]);
|
||||
CCoinControl coin_control;
|
||||
// Automatically select (additional) coins. Can be overriden by options.add_inputs.
|
||||
coin_control.m_add_inputs = true;
|
||||
FundTransaction(pwallet, tx, fee, change_position, request.params[1], coin_control);
|
||||
|
||||
UniValue result(UniValue::VOBJ);
|
||||
result.pushKV("hex", EncodeHexTx(CTransaction(tx)));
|
||||
|
|
@ -3976,10 +3984,10 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request)
|
|||
UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
|
||||
{
|
||||
RPCHelpMan{"walletcreatefundedpsbt",
|
||||
"\nCreates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"
|
||||
"\nCreates and funds a transaction in the Partially Signed Transaction format.\n"
|
||||
"Implements the Creator and Updater roles.\n",
|
||||
{
|
||||
{"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs",
|
||||
{"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs. Leave empty to add inputs automatically. See add_inputs option.",
|
||||
{
|
||||
{"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
|
||||
{
|
||||
|
|
@ -4010,6 +4018,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
|
|||
{"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
|
||||
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
|
||||
{
|
||||
{"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "If inputs are specified, automatically include more if they are not enough."},
|
||||
{"changeAddress", RPCArg::Type::STR_HEX, /* default */ "pool address", "The bitcoin address to receive the change"},
|
||||
{"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
|
||||
{"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if changeAddress is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
|
||||
|
|
@ -4071,7 +4080,11 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
|
|||
rbf = replaceable_arg.isTrue();
|
||||
}
|
||||
CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf);
|
||||
FundTransaction(pwallet, rawTx, fee, change_position, request.params[3]);
|
||||
CCoinControl coin_control;
|
||||
// Automatically select coins, unless at least one is manually selected. Can
|
||||
// be overriden by options.add_inputs.
|
||||
coin_control.m_add_inputs = rawTx.vin.size() == 0;
|
||||
FundTransaction(pwallet, rawTx, fee, change_position, request.params[3], coin_control);
|
||||
|
||||
// Make a blank psbt
|
||||
PartiallySignedTransaction psbtx(rawTx);
|
||||
|
|
|
|||
|
|
@ -2160,6 +2160,11 @@ void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const
|
|||
}
|
||||
|
||||
for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
|
||||
// Only consider selected coins if add_inputs is false
|
||||
if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(entry.first, i))) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (wtx.tx->vout[i].nValue < nMinimumAmount || wtx.tx->vout[i].nValue > nMaximumAmount)
|
||||
continue;
|
||||
|
||||
|
|
|
|||
|
|
@ -271,7 +271,11 @@ class RawTransactionsTest(BitcoinTestFramework):
|
|||
assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
|
||||
assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex'])
|
||||
|
||||
# Should fail without add_inputs:
|
||||
assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False})
|
||||
# add_inputs is enabled by default
|
||||
rawtxfund = self.nodes[2].fundrawtransaction(rawtx)
|
||||
|
||||
dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex'])
|
||||
totalOut = 0
|
||||
matchingOuts = 0
|
||||
|
|
@ -299,7 +303,10 @@ class RawTransactionsTest(BitcoinTestFramework):
|
|||
dec_tx = self.nodes[2].decoderawtransaction(rawtx)
|
||||
assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
|
||||
|
||||
rawtxfund = self.nodes[2].fundrawtransaction(rawtx)
|
||||
# Should fail without add_inputs:
|
||||
assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False})
|
||||
rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True})
|
||||
|
||||
dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex'])
|
||||
totalOut = 0
|
||||
matchingOuts = 0
|
||||
|
|
@ -330,7 +337,10 @@ class RawTransactionsTest(BitcoinTestFramework):
|
|||
dec_tx = self.nodes[2].decoderawtransaction(rawtx)
|
||||
assert_equal(utx['txid'], dec_tx['vin'][0]['txid'])
|
||||
|
||||
rawtxfund = self.nodes[2].fundrawtransaction(rawtx)
|
||||
# Should fail without add_inputs:
|
||||
assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False})
|
||||
rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True})
|
||||
|
||||
dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex'])
|
||||
totalOut = 0
|
||||
matchingOuts = 0
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@
|
|||
from decimal import Decimal
|
||||
from test_framework.test_framework import BitcoinTestFramework
|
||||
from test_framework.util import (
|
||||
assert_approx,
|
||||
assert_equal,
|
||||
assert_greater_than,
|
||||
assert_raises_rpc_error,
|
||||
|
|
@ -85,6 +86,13 @@ class PSBTTest(BitcoinTestFramework):
|
|||
# Create and fund a raw tx for sending 10 BTC
|
||||
psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']
|
||||
|
||||
# If inputs are specified, do not automatically add more:
|
||||
utxo1 = self.nodes[0].listunspent()[0]
|
||||
assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[0].walletcreatefundedpsbt, [{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():90})
|
||||
|
||||
psbtx1 = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():90}, 0, {"add_inputs": True})['psbt']
|
||||
assert_equal(len(self.nodes[0].decodepsbt(psbtx1)['tx']['vin']), 2)
|
||||
|
||||
# Node 1 should not be able to add anything to it but still return the psbtx same as before
|
||||
psbtx = self.nodes[1].walletprocesspsbt(psbtx1)['psbt']
|
||||
assert_equal(psbtx1, psbtx)
|
||||
|
|
@ -152,13 +160,13 @@ class PSBTTest(BitcoinTestFramework):
|
|||
self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex'])
|
||||
|
||||
# feeRate of 0.1 BTC / KB produces a total fee slightly below -maxtxfee (~0.05280000):
|
||||
res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1})
|
||||
assert_greater_than(res["fee"], 0.05)
|
||||
assert_greater_than(0.06, res["fee"])
|
||||
res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1, "add_inputs": True})
|
||||
assert_approx(res["fee"], 0.055, 0.005)
|
||||
|
||||
# feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
|
||||
# previously this was silently capped at -maxtxfee
|
||||
assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10})
|
||||
assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10, "add_inputs": True})
|
||||
assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():1}, 0, {"feeRate": 10, "add_inputs": False})
|
||||
|
||||
# partially sign multisig things with node 1
|
||||
psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2wsh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2sh_p2wsh_pos}], outputs={self.nodes[1].getnewaddress():29.99}, options={'changeAddress': self.nodes[1].getrawchangeaddress()})['psbt']
|
||||
|
|
@ -239,7 +247,7 @@ class PSBTTest(BitcoinTestFramework):
|
|||
# replaceable arg
|
||||
block_height = self.nodes[0].getblockcount()
|
||||
unspent = self.nodes[0].listunspent()[0]
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable": False}, False)
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable": False, "add_inputs": True}, False)
|
||||
decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
|
||||
for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
|
||||
assert_greater_than(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
|
||||
|
|
@ -247,7 +255,7 @@ class PSBTTest(BitcoinTestFramework):
|
|||
assert_equal(decoded_psbt["tx"]["locktime"], block_height+2)
|
||||
|
||||
# Same construction with only locktime set and RBF explicitly enabled
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height, {"replaceable": True}, True)
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height, {"replaceable": True, "add_inputs": True}, True)
|
||||
decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
|
||||
for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
|
||||
assert_equal(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
|
||||
|
|
@ -255,7 +263,7 @@ class PSBTTest(BitcoinTestFramework):
|
|||
assert_equal(decoded_psbt["tx"]["locktime"], block_height)
|
||||
|
||||
# Same construction without optional arguments
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}])
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([], [{self.nodes[2].getnewaddress():unspent["amount"]+1}])
|
||||
decoded_psbt = self.nodes[0].decodepsbt(psbtx_info["psbt"])
|
||||
for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
|
||||
assert_equal(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
|
||||
|
|
@ -264,7 +272,7 @@ class PSBTTest(BitcoinTestFramework):
|
|||
|
||||
# Same construction without optional arguments, for a node with -walletrbf=0
|
||||
unspent1 = self.nodes[1].listunspent()[0]
|
||||
psbtx_info = self.nodes[1].walletcreatefundedpsbt([{"txid":unspent1["txid"], "vout":unspent1["vout"]}], [{self.nodes[2].getnewaddress():unspent1["amount"]+1}], block_height)
|
||||
psbtx_info = self.nodes[1].walletcreatefundedpsbt([{"txid":unspent1["txid"], "vout":unspent1["vout"]}], [{self.nodes[2].getnewaddress():unspent1["amount"]+1}], block_height, {"add_inputs": True})
|
||||
decoded_psbt = self.nodes[1].decodepsbt(psbtx_info["psbt"])
|
||||
for tx_in, psbt_in in zip(decoded_psbt["tx"]["vin"], decoded_psbt["inputs"]):
|
||||
assert_greater_than(tx_in["sequence"], MAX_BIP125_RBF_SEQUENCE)
|
||||
|
|
@ -275,7 +283,7 @@ class PSBTTest(BitcoinTestFramework):
|
|||
self.nodes[0].walletcreatefundedpsbt([], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"changeAddress":self.nodes[1].getnewaddress()}, False)
|
||||
|
||||
# Regression test for 14473 (mishandling of already-signed witness transaction):
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}])
|
||||
psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], 0, {"add_inputs": True})
|
||||
complete_psbt = self.nodes[0].walletprocesspsbt(psbtx_info["psbt"])
|
||||
double_processed_psbt = self.nodes[0].walletprocesspsbt(complete_psbt["psbt"])
|
||||
assert_equal(complete_psbt, double_processed_psbt)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue