Merge bitcoin/bitcoin#27195: bumpfee: allow send coins back to yourself

be72663 test: bumpfee, add coverage for "send coins back to yourself" (furszy)
7bffec6 bumpfee: enable send coins back to yourself (furszy)

Pull request description:

  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 because 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.

ACKs for top commit:
  ishaanam:
    reACK be72663
  achow101:
    ACK be72663

Tree-SHA512: c2c38290a998f9b426a830d9624c7feb730158980ac186f8fb0138d5e200935d6538307bc60a2c3d0b7b6ee2b4ffb77a1e98baf8feb1d20a7d825f6055ac377f

Author: achow101

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<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)) {
@@ -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<CNoDestination>(&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) {

test/functional/wallet_bumpfee.py

@@ -41,6 +41,10 @@
 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):
@@ -104,6 +108,7 @@ def run_test(self):
         # 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()
 
         # Context independent tests
         test_feerate_checks_replaced_outputs(self, rbf_node, peer_node)
@@ -171,6 +176,54 @@ def test_invalid_parameters(self, rbf_node, peer_node, dest_address):
 
         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))
@@ -635,21 +688,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()