From 7bffec6715a75bb2c8631177d39f984aabc656ba Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 3 Mar 2023 11:58:27 -0300 Subject: [PATCH 1/2] bumpfee: enable send coins back to yourself Simple example: 1) User_1 sends 0.1 btc to user_2 on a low fee transaction. 2) After few hours, the tx is still in the mempool, user_2 is not interested anymore, so user_1 decides to cancel it by sending coins back to himself. 3) User_1 has the bright idea of opening the explorer and copy the change output address of the transaction. Then call bumpfee providing such output (in the "outputs" arg). Currently, this is not possible. The wallet fails with "Unable to create transaction. Transaction must have at least one recipient" error. The error reason is that we discard the provided output from the recipients list and set it inside the coin control so the process adds it later (when the change is calculated). But.. there is no later if the tx has no outputs. --- src/wallet/feebumper.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index d127c41c43d..b6b1fa1d3e2 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -231,6 +231,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // is one). If outputs vector is non-empty, replace original // outputs with its contents, otherwise use original outputs. 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)) { @@ -241,6 +242,21 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo ExtractDestination(output.scriptPubKey, change_dest); new_coin_control.destChange = change_dest; } + new_outputs_value += output.nValue; + } + + // If no recipients, means that we are sending coins to a change address + if (recipients.empty()) { + // Just as a sanity check, ensure that the change address exist + if (std::get_if(&new_coin_control.destChange)) { + errors.emplace_back(Untranslated("Unable to create transaction. Transaction must have at least one recipient")); + return Result::INVALID_PARAMETER; + } + + // Add change as recipient with SFFO flag enabled, so fees are deduced from it. + // If the output differs from the original tx output (because the user customized it) a new change output will be created. + recipients.emplace_back(CRecipient{GetScriptForDestination(new_coin_control.destChange), new_outputs_value, /*fSubtractFeeFromAmount=*/true}); + new_coin_control.destChange = CNoDestination(); } if (coin_control.m_feerate) { From be72663a1521bc6cdf16d43a4feae7c5b57735c0 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 3 Mar 2023 11:59:21 -0300 Subject: [PATCH 2/2] test: bumpfee, add coverage for "send coins back to yourself" --- test/functional/wallet_bumpfee.py | 64 +++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 8 deletions(-) diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 4f7af328c1b..f0b8728933f 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -41,6 +41,10 @@ NORMAL = 100 HIGH = 500 TOO_HIGH = 100000 +def get_change_address(tx, node): + tx_details = node.getrawtransaction(tx, 1) + txout_addresses = [txout['scriptPubKey']['address'] for txout in tx_details["vout"]] + return [address for address in txout_addresses if node.getaddressinfo(address)["ischange"]] class BumpFeeTest(BitcoinTestFramework): def add_options(self, parser): @@ -105,6 +109,7 @@ class BumpFeeTest(BitcoinTestFramework): # These tests wipe out a number of utxos that are expected in other tests 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() def test_invalid_parameters(self, rbf_node, peer_node, dest_address): self.log.info('Test invalid parameters') @@ -169,6 +174,54 @@ class BumpFeeTest(BitcoinTestFramework): self.clear_mempool() + def test_bump_back_to_yourself(self): + self.log.info("Test that bumpfee can send coins back to yourself") + node = self.nodes[1] + + node.createwallet("back_to_yourself") + wallet = node.get_wallet_rpc("back_to_yourself") + + # Make 3 UTXOs + addr = wallet.getnewaddress() + for _ in range(3): + self.nodes[0].sendtoaddress(addr, 5) + self.generate(self.nodes[0], 1) + + # Create a tx with two outputs. recipient and change. + tx = wallet.send(outputs={wallet.getnewaddress(): 9}, fee_rate=2) + tx_info = wallet.gettransaction(txid=tx["txid"], verbose=True) + assert_equal(len(tx_info["decoded"]["vout"]), 2) + assert_equal(len(tx_info["decoded"]["vin"]), 2) + + # Bump tx, send coins back to change address. + change_addr = get_change_address(tx["txid"], wallet)[0] + out_amount = 10 + bumped = wallet.bumpfee(txid=tx["txid"], options={"fee_rate": 20, "outputs": [{change_addr: out_amount}]}) + bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True) + assert_equal(len(bumped_tx["decoded"]["vout"]), 1) + assert_equal(len(bumped_tx["decoded"]["vin"]), 2) + assert_equal(bumped_tx["decoded"]["vout"][0]["value"] + bumped["fee"], out_amount) + + # Bump tx again, now test send fewer coins back to change address. + out_amount = 6 + bumped = wallet.bumpfee(txid=bumped["txid"], options={"fee_rate": 40, "outputs": [{change_addr: out_amount}]}) + 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 any(txout['value'] == out_amount - bumped["fee"] and txout['scriptPubKey']['address'] == change_addr for txout in bumped_tx['decoded']['vout']) + # Check that total out amount is still equal to the previously bumped tx + assert_equal(bumped_tx["decoded"]["vout"][0]["value"] + bumped_tx["decoded"]["vout"][1]["value"] + bumped["fee"], 10) + + # Bump tx again, send more coins back to change address. The process will add another input to cover the target. + out_amount = 12 + bumped = wallet.bumpfee(txid=bumped["txid"], options={"fee_rate": 80, "outputs": [{change_addr: out_amount}]}) + bumped_tx = wallet.gettransaction(txid=bumped["txid"], verbose=True) + assert_equal(len(bumped_tx["decoded"]["vout"]), 2) + assert_equal(len(bumped_tx["decoded"]["vin"]), 3) + assert any(txout['value'] == out_amount - bumped["fee"] and txout['scriptPubKey']['address'] == change_addr for txout in bumped_tx['decoded']['vout']) + assert_equal(bumped_tx["decoded"]["vout"][0]["value"] + bumped_tx["decoded"]["vout"][1]["value"] + bumped["fee"], 15) + + node.unloadwallet("back_to_yourself") def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address): self.log.info('Test simple bumpfee: {}'.format(mode)) @@ -628,21 +681,16 @@ def test_locked_wallet_fails(self, rbf_node, dest_address): def test_change_script_match(self, rbf_node, dest_address): self.log.info('Test that the same change addresses is used for the replacement transaction when possible') - def get_change_address(tx): - tx_details = rbf_node.getrawtransaction(tx, 1) - txout_addresses = [txout['scriptPubKey']['address'] for txout in tx_details["vout"]] - return [address for address in txout_addresses if rbf_node.getaddressinfo(address)["ischange"]] - # Check that there is only one change output rbfid = spend_one_input(rbf_node, dest_address) - change_addresses = get_change_address(rbfid) + change_addresses = get_change_address(rbfid, rbf_node) assert_equal(len(change_addresses), 1) # Now find that address in each subsequent tx, and no other change bumped_total_tx = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL}) - assert_equal(change_addresses, get_change_address(bumped_total_tx['txid'])) + assert_equal(change_addresses, get_change_address(bumped_total_tx['txid'], rbf_node)) bumped_rate_tx = rbf_node.bumpfee(bumped_total_tx["txid"]) - assert_equal(change_addresses, get_change_address(bumped_rate_tx['txid'])) + assert_equal(change_addresses, get_change_address(bumped_rate_tx['txid'], rbf_node)) self.clear_mempool()