diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 9a29deac63..578773953a 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -262,11 +262,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 0d6a305321..c5409b0c53 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -153,8 +153,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); @@ -167,6 +173,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; @@ -226,7 +238,8 @@ 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) { + for (size_t i = 0; i < txouts.size(); ++i) { + const CTxOut& output = txouts.at(i); // ELEMENTS: bool is_change = OutputIsChange(wallet, output); bool is_fee = output.IsFee(); @@ -235,13 +248,13 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo return Result::WALLET_ERROR; } - if (!is_change && !is_fee) { - wallet::CRecipient recipient = {output.scriptPubKey, output.nValue.GetAmount(), output.nAsset.GetAsset(), CPubKey(output.nNonce.vchCommitment), false}; - recipients.push_back(recipient); - } else if (is_change) { + if (reduce_output.has_value() ? reduce_output.value() == i : is_change) { CTxDestination change_dest; ExtractDestination(output.scriptPubKey, change_dest); destinations[output.nAsset.GetAsset()] = change_dest; + } else if (!is_change && !is_fee) { + wallet::CRecipient recipient = {output.scriptPubKey, output.nValue.GetAmount(), output.nAsset.GetAsset(), CPubKey(output.nNonce.vchCommitment), false}; + recipients.push_back(recipient); } new_outputs_value += output.nValue.GetAmount(); } diff --git a/src/wallet/feebumper.h b/src/wallet/feebumper.h index 9c9d0b000b..c4e9cab5cf 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 07e63e37df..abbe79c720 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1119,9 +1119,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"}}, }, @@ -1160,6 +1162,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, @@ -1170,6 +1174,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); @@ -1193,6 +1198,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 @@ -1210,7 +1219,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 1d8eaa7648..d270114d16 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -24,10 +24,12 @@ 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_greater_than_or_equal, assert_raises_rpc_error, get_fee, + find_vout_for_address, ) from test_framework import util from test_framework.wallet import MiniWallet @@ -113,6 +115,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) @@ -178,6 +182,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": 3}) # ELEMENTS + + 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): @@ -229,6 +240,80 @@ 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"]), 3) # ELEMENTS + + 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"]), 3) # ELEMENTS + 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() + + # ELEMENTS FIXME: use createrawtransaction until sendall is fixed + # tx = wallet.sendall(recipients=[wallet.getnewaddress()], fee_rate=2, options={"inputs": [utxos[0]]}) + fee = Decimal("0.00001000") + tx = wallet.createrawtransaction([{"txid": utxos[0]["txid"], "vout": utxos[0]["vout"]}], [{wallet.getnewaddress(): utxos[0]["amount"] - fee}, {"fee": fee}]) + signed = wallet.signrawtransactionwithwallet(tx) + assert signed["complete"] + txid = wallet.sendrawtransaction(signed["hex"]) + tx = {"txid": txid} + + # Reduce the only output with a crazy high feerate, should fail as the output would be dust + # ELEMENTS FIXME + # 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": 11, "reduce_output": 0}) # ELEMENTS + bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True) + assert_equal(len(bumped_tx["decoded"]["vout"]), 2) # ELEMENTS + 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(11) / Decimal(1e8) * 1000) # ELEMENTS + + # 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"]), 3) # ELEMENTS + 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)