From 7d83502d3d52218e7b0b0634cff2a9aba9cc77ef Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 14:52:56 -0500 Subject: [PATCH 1/3] bumpfee: Allow original change position to be specified If the user used a custom change address, it may not be detected as a change output, resulting in an additional change output being added to the bumped transaction. We can avoid this issue by allowing the user to specify the position of the change output. --- src/rpc/client.cpp | 2 ++ src/wallet/feebumper.cpp | 25 +++++++++++++++++++------ src/wallet/feebumper.h | 5 ++++- src/wallet/rpc/spend.cpp | 13 +++++++++++-- 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index edc0fb05d72..a4c5a26a82b 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -259,11 +259,13 @@ static const CRPCConvertParam vRPCConvertParams[] = { "bumpfee", 1, "fee_rate"}, { "bumpfee", 1, "replaceable"}, { "bumpfee", 1, "outputs"}, + { "bumpfee", 1, "reduce_output"}, { "psbtbumpfee", 1, "options" }, { "psbtbumpfee", 1, "conf_target"}, { "psbtbumpfee", 1, "fee_rate"}, { "psbtbumpfee", 1, "replaceable"}, { "psbtbumpfee", 1, "outputs"}, + { "psbtbumpfee", 1, "reduce_output"}, { "logging", 0, "include" }, { "logging", 1, "exclude" }, { "disconnectnode", 1, "nodeid" }, diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index 0e1f1f98479..3720d144ebe 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -152,8 +152,14 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid) } Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector& errors, - CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector& outputs) + CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector& outputs, std::optional reduce_output) { + // 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")); + return Result::INVALID_PARAMETER; + } + // We are going to modify coin control later, copy to re-use CCoinControl new_coin_control(coin_control); @@ -166,6 +172,12 @@ 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()) { + errors.push_back(Untranslated("Change position is out of range")); + return Result::INVALID_PARAMETER; + } + // Retrieve all of the UTXOs and add them to coin control // While we're here, calculate the input amount std::map coins; @@ -233,14 +245,15 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo std::vector recipients; CAmount new_outputs_value = 0; const auto& txouts = outputs.empty() ? wtx.tx->vout : outputs; - for (const auto& output : txouts) { - if (!OutputIsChange(wallet, output)) { - CRecipient recipient = {output.scriptPubKey, output.nValue, false}; - recipients.push_back(recipient); - } else { + for (size_t i = 0; i < txouts.size(); ++i) { + const CTxOut& output = txouts.at(i); + if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) { CTxDestination change_dest; ExtractDestination(output.scriptPubKey, change_dest); new_coin_control.destChange = change_dest; + } else { + CRecipient recipient = {output.scriptPubKey, output.nValue, false}; + recipients.push_back(recipient); } new_outputs_value += output.nValue; } diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 53cf16e0f19..f00bf15730f 100644 --- a/src/wallet/feebumper.h +++ b/src/wallet/feebumper.h @@ -43,6 +43,8 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid); * @param[out] new_fee the fee that the bump transaction pays * @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 */ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, @@ -52,7 +54,8 @@ Result CreateRateBumpTransaction(CWallet& wallet, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, - const std::vector& outputs); + const std::vector& outputs, + std::optional reduce_output = std::nullopt); //! Sign the new transaction, //! @return false if the tx couldn't be found or if it was diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index feee643f26b..b695d4bed35 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1015,9 +1015,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name) "\"" + FeeModes("\"\n\"") + "\""}, {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "New outputs (key-value pairs) which will replace\n" "the original ones, if provided. Each address can only appear once and there can\n" - "only be one \"data\" object.\n", + "only be one \"data\" object.\n" + "Cannot be provided if 'reduce_output' 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."}, }, RPCArgOptions{.oneline_description="options"}}, }, @@ -1056,6 +1058,8 @@ static RPCHelpMan bumpfee_helper(std::string method_name) coin_control.m_signal_bip125_rbf = true; std::vector outputs; + std::optional reduce_output; + if (!request.params[1].isNull()) { UniValue options = request.params[1]; RPCTypeCheckObj(options, @@ -1066,6 +1070,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)}, }, true, true); @@ -1089,6 +1094,10 @@ static RPCHelpMan bumpfee_helper(std::string method_name) AddOutputs(tempTx, options["outputs"]); outputs = tempTx.vout; } + + if (options.exists("reduce_output")) { + reduce_output = options["reduce_output"].getInt(); + } } // Make sure the results are valid at least up to the most recent block @@ -1106,7 +1115,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); + res = feebumper::CreateRateBumpTransaction(*pwallet, hash, coin_control, errors, old_fee, new_fee, mtx, /*require_mine=*/ !want_psbt, outputs, reduce_output); if (res != feebumper::Result::OK) { switch(res) { case feebumper::Result::INVALID_ADDRESS_OR_KEY: From 4f4d4407e3d2cc5ac784524c0cb0602837dc7860 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 7 Nov 2022 15:07:51 -0500 Subject: [PATCH 2/3] test: Test bumpfee reduce_output --- test/functional/wallet_bumpfee.py | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index b9ebf64c22e..61d2132062b 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -27,6 +27,7 @@ from test_framework.util import ( assert_greater_than, assert_raises_rpc_error, get_fee, + find_vout_for_address, ) from test_framework.wallet import MiniWallet @@ -109,6 +110,7 @@ class BumpFeeTest(BitcoinTestFramework): test_small_output_with_feerate_succeeds(self, rbf_node, dest_address) test_no_more_inputs_fails(self, rbf_node, dest_address) self.test_bump_back_to_yourself() + self.test_provided_change_pos(rbf_node) # Context independent tests test_feerate_checks_replaced_outputs(self, rbf_node, peer_node) @@ -174,6 +176,13 @@ 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 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.clear_mempool() def test_bump_back_to_yourself(self): @@ -225,6 +234,35 @@ class BumpFeeTest(BitcoinTestFramework): node.unloadwallet("back_to_yourself") + def test_provided_change_pos(self, rbf_node): + self.log.info("Test the reduce_output option") + + change_addr = rbf_node.getnewaddress() + dest_addr = rbf_node.getnewaddress() + assert_equal(rbf_node.getaddressinfo(change_addr)["ischange"], False) + assert_equal(rbf_node.getaddressinfo(dest_addr)["ischange"], False) + + send_res = rbf_node.send(outputs=[{dest_addr: 1}], options={"change_address": change_addr}) + assert send_res["complete"] + txid = send_res["txid"] + + tx = rbf_node.gettransaction(txid=txid, verbose=True) + assert_equal(len(tx["decoded"]["vout"]), 2) + + 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}) + new_txid = bumped["txid"] + + new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True) + assert_equal(len(new_tx["decoded"]["vout"]), 2) + new_change_pos = find_vout_for_address(rbf_node, new_txid, change_addr) + new_change_value = new_tx["decoded"]["vout"][new_change_pos]["value"] + + assert_greater_than(change_value, new_change_value) + + def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address): self.log.info('Test simple bumpfee: {}'.format(mode)) rbfid = spend_one_input(rbf_node, dest_address) From e8c31f135c6e9a5f57325dbf4feceafd384f7762 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 17 Mar 2023 18:49:42 -0400 Subject: [PATCH 3/3] tests: Test for bumping single output transaction --- test/functional/wallet_bumpfee.py | 39 +++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 61d2132062b..4bc01f3035f 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -24,6 +24,7 @@ from test_framework.messages import ( from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_fee_amount, assert_greater_than, assert_raises_rpc_error, get_fee, @@ -111,6 +112,7 @@ class BumpFeeTest(BitcoinTestFramework): test_no_more_inputs_fails(self, rbf_node, dest_address) self.test_bump_back_to_yourself() self.test_provided_change_pos(rbf_node) + self.test_single_output() # Context independent tests test_feerate_checks_replaced_outputs(self, rbf_node, peer_node) @@ -263,6 +265,43 @@ class BumpFeeTest(BitcoinTestFramework): assert_greater_than(change_value, new_change_value) + def test_single_output(self): + self.log.info("Test that single output txs can be bumped") + node = self.nodes[1] + + node.createwallet("single_out_rbf") + wallet = node.get_wallet_rpc("single_out_rbf") + + addr = wallet.getnewaddress() + amount = Decimal("0.001") + # Make 2 UTXOs + self.nodes[0].sendtoaddress(addr, amount) + self.nodes[0].sendtoaddress(addr, amount) + self.generate(self.nodes[0], 1) + utxos = wallet.listunspent() + + 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}) + + # Reduce the only output successfully + bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 10, "reduce_output": 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 + 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) + assert_equal(len(bumped_tx["decoded"]["vin"]), 2) + assert_fee_amount(bumped["fee"], bumped_tx["decoded"]["vsize"], Decimal(20) / Decimal(1e8) * 1000) + + wallet.unloadwallet() + def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address): self.log.info('Test simple bumpfee: {}'.format(mode)) rbfid = spend_one_input(rbf_node, dest_address)