Repository: bitcoin/bitcoin (language: cpp)

Issue:
Privacy Issue - Increase Fee with Custom Change Address grabs new UTXO  
I am relaying a privacy issue for Bitcoin Core that was [reported on r/Bitcoin](https://reddit.com/r/Bitcoin/comments/klk2wh) (Reddit).

The [message](https://pastebin.com/raw/ptFQityt) was:

- _Using coin control_
- _Custom change address_

_When one clicks the increase transaction fee button, it adds another input rather than increasing the fee by reducing the change output. Major privacy leak since you have no control of the input used._

From that it sounds like the observed behavior is that the replacement transaction includes an additional UTXO that was not chosen by the user, even though the transaction was composed as a Coin Control transaction.

So I'm assuming the expected behavior would be for Bitcoin Core to remember that this OUTPUT was actually a Custom Change Address and to then lower the amount of that OUTPUT by the amount needed for the fee bump.

And, now that I am thinking of this, isn't there the expectation that if my initial transaction was done using Coin Control, and then if my fee bump (RBF) needs another UTXO input to cover the increase in fee, that Coin Selection for that will occur (regardless of whether or not the change address was a Custom Change Address)?  


Discussion / hints:
Just tried to reproduce this issue and it looks like a bug which might be the reason for lot of transactions overpaying fees, privacy issues and the user might think he just increased the fee for a transaction in which "coin control" was used. Surprised that nobody replied anything yet here.

> And, now that I am thinking of this, isn't there the expectation that if my initial transaction was done using Coin Control, and then if my fee bump (RBF) needs another UTXO input to cover the increase in fee, that Coin Selection for that will occur (regardless of whether or not the change address was a Custom Change Address)?

Atleast user should be given warning if inputs are being changed. Let me share the details of my test transaction below:

![image](https://user-images.githubusercontent.com/13405205/104236072-f00c9880-547b-11eb-945f-a433fab1ded8.png)


Initial tx with RBF enabled, selected one input from wallet, a custom change address (new address from same wallet):

![image](https://user-images.githubusercontent.com/13405205/104235649-4a592980-547b-11eb-8be7-9df32955a909.png)

Replacement transaction: https://blockstream.info/testnet/tx/ae3975a21c646c1c74c228e388672f652883f4d312cc7a437b9180cc1390515b

![image](https://user-images.githubusercontent.com/13405205/104235750-69f05200-547b-11eb-9893-061990e3ef14.png)



@achow101 @Xekyo Your thoughts on this issue? It happens only when using custom change address for initial transaction which was RBF enabled and coin control features were used. 

Can we include a warning when replacement transaction is being confirmed? Something like "_Different inputs might be selected for replacement transaction since custom change address was used earlier_"

Or we need to fix something else related to coin selection algorithm being used for replacement transaction?
Note: Reducing the change output also could be a privacy problem, since it identifies which output was change.
> It happens only when using custom change address

I don't think that's the case. It also happens when you use a regular change address that you have labelled after using it. When a change address is added to the 'address book' Core no longer recognizes it as a change address and so doesn't know that it can be reduced when bumping the fee.

I made a related issue #20935 recently.

> we need to fix something else related to coin selection algorithm being used for replacement transaction?

Producing a warning when bumping a fee is about to throw away your privacy is a step in the right direction but it would be nice if there was an option to bump a fee without throwing away your privacy.
> I don't think that's the case. It also happens when you use a regular change address that you have labelled after using it.

Correct. I tested it and below are the results: 

1. New change address created with `getrawchangeaddress`: tb1qqt7w0m00xkth4ugsymw93t74ugkdey6c6g57k9

Used for a RBF transaction and replacement transaction had no extra inputs/outputs: https://tbtc.bitaps.com/f7b6c65165e4f7fbce11c8e3426deb207b55dd5b599c8a3e36b30169060421b6

No issues. Maybe because change addresses follow  `m/0'/1'/i'` derivation path in bitcoin core wallet.

2. Labelled the same change address with: `setlabel "tb1qqt7w0m00xkth4ugsymw93t74ugkdey6c6g57k9" "label1"` and used in a transaction. 

Used for a RBF transaction and replacement transaction had extra inputs/outputs: https://tbtc.bitaps.com/17f3208ecb34c6c14574de53e73b1039215bed9f67bc4606771c5b032e51d0bf

3. Created a new 'change address' and labelled it with: `setlabel "tb1qd4ld5clqv0maky0l9fqr38ck4qr605jx0xqezd" "label2"`

Used for a RBF transaction and replacement transaction had extra inputs/outputs: https://tbtc.bitaps.com/9d2d0960d06d09b630ad7e2599d0a608247f548e6f7b91d891bef459229e01af

>  When a change address is added to the 'address book' Core no longer recognizes it as a change address and so doesn't know that it can be reduced when bumping the fee.

Interesting. It should still be a 'change address' maybe not recognized as change address to improve privacy (address re-use).

> Producing a warning when bumping a fee is about to throw away your privacy is a step in the right direction but it would be nice if there was an option to bump a fee without throwing away your privacy.

Agree. I need to do more tests and this issue is interesting for two reasons: 1. Privacy and Fees can be improved while using Bitcoin Core Wallet 2. I will learn a lot during the research

> Interesting. It should still be a 'change address' maybe not recognized as change address to improve privacy

It's simpler than that. The implementation of IsChange() simply checks the address book and declares that labelled addresses aren't change:

    bool CWallet::IsChange(const CScript& script) const
    {
        // TODO: fix handling of 'change' outputs. The assumption is that any
        // payment to a script that is ours, but is not in the address book
        // is change. That assumption is likely to break when we implement multisignature
        // wallets that return change back into a multi-signature-protected address;
        // a better way of identifying which outputs are 'the send' and which are
        // 'the change' will need to be implemented (maybe extend CWalletTx to remember
        // which output, if any, was change).
        AssertLockHeld(cs_wallet);
        if (IsMine(script))
        {
            CTxDestination address;
            if (!ExtractDestination(script, address))
                return true;
            if (!FindAddressBookEntry(address)) {
                return true;
            }
        }
        return false;
    }

> When a change address is added to the 'address book' Core no longer recognizes it as a change address and so doesn't know that it can be reduced when bumping the fee.

This is not true

Tx 1: https://tbtc.bitaps.com/4d4cdd5b237882ae77ef3fac5c71b5639e59b3a888e88a1190f443c194211ec9

New change address automatically added by wallet (I just selected input): `tb1qpc8wx9sec3s7sm3j949eeg2pj3e6n3df9c5w2u`

This is a "change address" in address book which is already used with below result for `getaddressinfo`:

<pre>
{
  "address": "tb1qpc8wx9sec3s7sm3j949eeg2pj3e6n3df9c5w2u",
  "scriptPubKey": "00140e0ee31619c461e86e322d4b9ca1419473a9c5a9",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([6ba51506/0'/1'/21']0318d8f780c0074518dda438f445ead485561d285543480111dc91f66a86b06ff4)#63xh5ffu",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "0e0ee31619c461e86e322d4b9ca1419473a9c5a9",
  "pubkey": "0318d8f780c0074518dda438f445ead485561d285543480111dc91f66a86b06ff4",
  "ischange": true,
  "timestamp": 1604500822,
  "hdkeypath": "m/0'/1'/21'",
  "hdseedid": "65d2a44de43ed92fc2e85974b0ceec4b3824aade",
  "hdmasterfingerprint": "6ba51506",
  "labels": [
  ]
}
</pre>

I used the same address as "custom change address" in another transaction Tx2: https://tbtc.bitaps.com/6f7fa9bb497cc0c07cf541ec4667881abe18def7c6732c1c1309e1d2706658d2

Replacment transaction Tx3 for Tx2 after increasing fee: https://tbtc.bitaps.com/19c31ec81f301a6b83ab849e0ce64cdb925d5b3a7d30f618c487f393da8b7788 (Fee increased by decreasing the change amount)

![image](https://user-images.githubusercontent.com/13405205/106325791-62191600-62a1-11eb-8aac-b5164db90aa4.png)

![image](https://user-images.githubusercontent.com/13405205/106325815-6ba27e00-62a1-11eb-902e-8073fb34cbf4.png)
  
I thought being in the address book was the same as being labelled. Your output has no labels, so is it in the address book?

I'm talking about labeling the change address so that it doesn't show up with an empty list of labels like yours did:

    "labels": [
    ]
@prayank23 Specifically this is what I mean about adding a change address to the address book making bitcoin think it is no longer change. I started with a regular change address, verified that it is known to be a change address then labelled it and verified that it is no longer seen as a change address:

    $ bitcoin-cli -testnet getaddressinfo tb1qh6lk6exzxtgsqsrjc6cf9nc6wm8ru9xn67uh9t | jq .ischange
    true
    $ bitcoin-cli -testnet setlabel tb1qh6lk6exzxtgsqsrjc6cf9nc6wm8ru9xn67uh9t change1
    $ bitcoin-cli -testnet getaddressinfo tb1qh6lk6exzxtgsqsrjc6cf9nc6wm8ru9xn67uh9t | jq .ischange
    false
    $ 


Fix patch:
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index edc0fb05d72ee..a4c5a26a82baa 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 0e1f1f984795a..3720d144ebef5 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 53cf16e0f1925..f00bf15730fc6 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 feee643f26b9e..b695d4bed354a 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:


Test patch:
diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py
index b9ebf64c22ea0..4bc01f3035f72 100755
--- a/test/functional/wallet_bumpfee.py
+++ b/test/functional/wallet_bumpfee.py
@@ -24,9 +24,11 @@
 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 @@ def run_test(self):
         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 @@ def test_invalid_parameters(self, rbf_node, peer_node, dest_address):
         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 @@ def test_bump_back_to_yourself(self):
 
         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)