diff options
| -rw-r--r-- | src/rpc/client.cpp | 2 | ||||
| -rw-r--r-- | src/wallet/feebumper.cpp | 25 | ||||
| -rw-r--r-- | src/wallet/feebumper.h | 5 | ||||
| -rw-r--r-- | src/wallet/rpc/spend.cpp | 13 | ||||
| -rwxr-xr-x | test/functional/wallet_bumpfee.py | 77 |
5 files changed, 113 insertions, 9 deletions
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 5f58eef1db..d289a9240e 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 0e1f1f9847..3720d144eb 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<bilingual_str>& errors, - CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs) + CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> 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<COutPoint, Coin> coins; @@ -233,14 +245,15 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo std::vector<CRecipient> 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 53cf16e0f1..f00bf15730 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<CTxOut>& outputs); + const std::vector<CTxOut>& outputs, + std::optional<uint32_t> 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 feee643f26..b695d4bed3 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<CTxOut> outputs; + std::optional<uint32_t> 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<uint32_t>(); + } } // 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: diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index b9ebf64c22..4bc01f3035 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -24,9 +24,11 @@ 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, + find_vout_for_address, ) from test_framework.wallet import MiniWallet @@ -109,6 +111,8 @@ 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) + self.test_single_output() # Context independent tests test_feerate_checks_replaced_outputs(self, rbf_node, peer_node) @@ -174,6 +178,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 +236,72 @@ 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_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) |