mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-22 06:52:36 +01:00
Code Issues Projects Releases Wiki Activity

rpc: bumpfee, improve doc for 'reduce_output' arg

Browse source 
The current argument name and description are dangerous as it don't
describe the case where the user selects the recipient output as the
change address. This one could end up been increased by the inputs
minus outputs remainder. Which, when bumpfee adds new inputs
to the transaction, leads the process to send more coins to the
recipient. Which is not what the user would expect from a
'reduce_output' param naming.

Co-authored-by: Murch <murch@murch.one>
This commit is contained in:
furszy 2023-09-19 08:09:23 -03:00
parent 1d4846a844
commit b3db8c9d5c
No known key found for this signature in database
GPG key ID: 5DD23CCC686AA623
5 changed files with 34 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/rpc/client.cpp
View file

@ -263,13 +263,13 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "bumpfee", 1, "fee_rate"},
{ "bumpfee", 1, "replaceable"},
{ "bumpfee", 1, "outputs"},
{ "bumpfee", 1, "reduce_output"},
{ "bumpfee", 1, "original_change_index"},
{ "psbtbumpfee", 1, "options" },
{ "psbtbumpfee", 1, "conf_target"},
{ "psbtbumpfee", 1, "fee_rate"},
{ "psbtbumpfee", 1, "replaceable"},
{ "psbtbumpfee", 1, "outputs"},
{ "psbtbumpfee", 1, "reduce_output"},
{ "psbtbumpfee", 1, "original_change_index"},
{ "logging", 0, "include" },
{ "logging", 1, "exclude" },
{ "disconnectnode", 1, "nodeid" },

14
src/wallet/feebumper.cpp
View file

@ -162,11 +162,11 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
}
Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<bilingual_str>& errors,
CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> reduce_output)
CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> original_change_index)
{
// Cannot both specify new outputs and an output to reduce
if (!outputs.empty() && reduce_output.has_value()) {
errors.push_back(Untranslated("Cannot specify both new outputs to use and an output index to reduce"));
// For now, cannot specify both new outputs to use and an output index to send change
if (!outputs.empty() && original_change_index.has_value()) {
errors.push_back(Untranslated("The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled."));
return Result::INVALID_PARAMETER;
}
@ -182,8 +182,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
}
const CWalletTx& wtx = it->second;
// Make sure that reduce_output is valid
if (reduce_output.has_value() && reduce_output.value() >= wtx.tx->vout.size()) {
// Make sure that original_change_index is valid
if (original_change_index.has_value() && original_change_index.value() >= wtx.tx->vout.size()) {
errors.push_back(Untranslated("Change position is out of range"));
return Result::INVALID_PARAMETER;
}
@ -259,7 +259,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
const CTxOut& output = txouts.at(i);
CTxDestination dest;
ExtractDestination(output.scriptPubKey, dest);
if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
if (original_change_index.has_value() ? original_change_index.value() == i : OutputIsChange(wallet, output)) {
new_coin_control.destChange = dest;
} else {
CRecipient recipient = {dest, output.nValue, false};

4
src/wallet/feebumper.h
View file

@ -44,7 +44,7 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
* @param[out] mtx The bump transaction itself
* @param[in] require_mine Whether the original transaction must consist of inputs that can be spent by the wallet
* @param[in] outputs Vector of new outputs to replace the bumped transaction's outputs
* @param[in] reduce_output The position of the change output to deduct the fee from in the transaction being bumped
* @param[in] original_change_index The position of the change output to deduct the fee from in the transaction being bumped
*/
Result CreateRateBumpTransaction(CWallet& wallet,
const uint256& txid,
@ -55,7 +55,7 @@ Result CreateRateBumpTransaction(CWallet& wallet,
CMutableTransaction& mtx,
bool require_mine,
const std::vector<CTxOut>& outputs,
std::optional<uint32_t> reduce_output = std::nullopt);
std::optional<uint32_t> original_change_index = std::nullopt);
//! Sign the new transaction,
//! @return false if the tx couldn't be found or if it was

18
src/wallet/rpc/spend.cpp
View file

@ -1016,10 +1016,14 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
{"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "The outputs specified as key-value pairs.\n"
"Each key may only appear once, i.e. there can only be one 'data' output, and no address may be duplicated.\n"
"At least one output of either type must be specified.\n"
"Cannot be provided if 'reduce_output' is specified.",
"Cannot be provided if 'original_change_index' is specified.",
OutputsDoc(),
RPCArgOptions{.skip_type_check = true}},
{"reduce_output", RPCArg::Type::NUM, RPCArg::DefaultHint{"not set, detect change automatically"}, "The 0-based index of the output from which the additional fees will be deducted. In general, this should be the position of change output. Cannot be provided if 'outputs' is specified."},
{"original_change_index", RPCArg::Type::NUM, RPCArg::DefaultHint{"not set, detect change automatically"}, "The 0-based index of the change output on the original transaction. "
"The indicated output will be recycled into the new change output on the bumped transaction. "
"The remainder after paying the recipients and fees will be sent to the output script of the "
"original change output. The change output’s amount can increase if bumping the transaction "
"adds new inputs, otherwise it will decrease. Cannot be used in combination with the 'outputs' option."},
},
RPCArgOptions{.oneline_description="options"}},
},
@ -1058,7 +1062,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
coin_control.m_signal_bip125_rbf = true;
std::vector<CTxOut> outputs;
std::optional<uint32_t> reduce_output;
std::optional<uint32_t> original_change_index;
if (!request.params[1].isNull()) {
UniValue options = request.params[1];
@ -1070,7 +1074,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
{"replaceable", UniValueType(UniValue::VBOOL)},
{"estimate_mode", UniValueType(UniValue::VSTR)},
{"outputs", UniValueType()}, // will be checked by AddOutputs()
{"reduce_output", UniValueType(UniValue::VNUM)},
{"original_change_index", UniValueType(UniValue::VNUM)},
},
true, true);
@ -1095,8 +1099,8 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
outputs = tempTx.vout;
}
if (options.exists("reduce_output")) {
reduce_output = options["reduce_output"].getInt<uint32_t>();
if (options.exists("original_change_index")) {
original_change_index = options["original_change_index"].getInt<uint32_t>();
}
}
@ -1115,7 +1119,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
CMutableTransaction mtx;
feebumper::Result res;
// Targeting feerate bump.
res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs, reduce_output);
res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs, original_change_index);
if (res != feebumper::Result::OK) {
switch(res) {
case feebumper::Result::INVALID_ADDRESS_OR_KEY:

24
test/functional/wallet_bumpfee.py
View file

@ -178,12 +178,12 @@ class BumpFeeTest(BitcoinTestFramework):
assert_raises_rpc_error(-8, "Invalid parameter, duplicate key: data",
rbf_node.bumpfee, rbfid, {"outputs": [{"data": "deadbeef"}, {"data": "deadbeef"}]})
self.log.info("Test reduce_output option")
assert_raises_rpc_error(-1, "JSON integer out of range", rbf_node.bumpfee, rbfid, {"reduce_output": -1})
assert_raises_rpc_error(-8, "Change position is out of range", rbf_node.bumpfee, rbfid, {"reduce_output": 2})
self.log.info("Test original_change_index option")
assert_raises_rpc_error(-1, "JSON integer out of range", rbf_node.bumpfee, rbfid, {"original_change_index": -1})
assert_raises_rpc_error(-8, "Change position is out of range", rbf_node.bumpfee, rbfid, {"original_change_index": 2})
self.log.info("Test outputs and reduce_output cannot both be provided")
assert_raises_rpc_error(-8, "Cannot specify both new outputs to use and an output index to reduce", rbf_node.bumpfee, rbfid, {"reduce_output": 2, "outputs": [{dest_address: 0.1}]})
self.log.info("Test outputs and original_change_index cannot both be provided")
assert_raises_rpc_error(-8, "The options 'outputs' and 'original_change_index' are incompatible. You can only either specify a new set of outputs, or designate a change output to be recycled.", rbf_node.bumpfee, rbfid, {"original_change_index": 2, "outputs": [{dest_address: 0.1}]})
self.clear_mempool()
@ -237,7 +237,7 @@ class BumpFeeTest(BitcoinTestFramework):
node.unloadwallet("back_to_yourself")
def test_provided_change_pos(self, rbf_node):
self.log.info("Test the reduce_output option")
self.log.info("Test the original_change_index option")
change_addr = rbf_node.getnewaddress()
dest_addr = rbf_node.getnewaddress()
@ -254,7 +254,7 @@ class BumpFeeTest(BitcoinTestFramework):
change_pos = find_vout_for_address(rbf_node, txid, change_addr)
change_value = tx["decoded"]["vout"][change_pos]["value"]
bumped = rbf_node.bumpfee(txid, {"reduce_output": change_pos})
bumped = rbf_node.bumpfee(txid, {"original_change_index": change_pos})
new_txid = bumped["txid"]
new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True)
@ -282,18 +282,18 @@ class BumpFeeTest(BitcoinTestFramework):
tx = wallet.sendall(recipients=[wallet.getnewaddress()], fee_rate=2, options={"inputs": [utxos[0]]})
# Reduce the only output with a crazy high feerate, should fail as the output would be dust
assert_raises_rpc_error(-4, "The transaction amount is too small to pay the fee", wallet.bumpfee, txid=tx["txid"], options={"fee_rate": 1100, "reduce_output": 0})
# Set the only output with a crazy high feerate as change, should fail as the output would be dust
assert_raises_rpc_error(-4, "The transaction amount is too small to pay the fee", wallet.bumpfee, txid=tx["txid"], options={"fee_rate": 1100, "original_change_index": 0})
# Reduce the only output successfully
bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "reduce_output": 0})
# Specify single output as change successfully
bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "original_change_index": 0})
bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True)
assert_equal(len(bumped_tx["decoded"]["vout"]), 1)
assert_equal(len(bumped_tx["decoded"]["vin"]), 1)
assert_equal(bumped_tx["decoded"]["vout"][0]["value"] + bumped["fee"], amount)
assert_fee_amount(bumped["fee"], bumped_tx["decoded"]["vsize"], Decimal(10) / Decimal(1e8) * 1000)
# Bumping without reducing adds a new input and output
# Bumping without specifying change adds a new input and output
bumped = wallet.bumpfee(txid=bumped["txid"], options={"fee_rate": 20})
bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True)
assert_equal(len(bumped_tx["decoded"]["vout"]), 2)