Merge #16239: wallet/rpc: follow-up clean-up/fixes to avoid_reuse

71d0344cf25d3aaf60112c5248198c444bc98105 docs: release note wording (Karl-Johan Alm)
3d2ff379131a01e4e9f9648b150e806104a23795 wallet/rpc: use static help text (Karl-Johan Alm)
53c3c1ea9e20f881c843a9219e48cec202e962f8 wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm)

Pull request description:

  This addresses a few remaining issues pointed out in #13756:

  * First commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r284907468
  * Second commit addresses https://github.com/bitcoin/bitcoin/pull/13756#discussion_r294868973

  Ping jnewbery and achow101 as they pointed out these issues.

ACKs for commit 71d034:
  jnewbery:
    ACK 71d0344cf25d3aaf60112c5248198c444bc98105
  meshcollider:
    re-utACK 71d0344cf2

Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0

doc/release-notes-13756.md

@@ -7,8 +7,8 @@ A new wallet flag `avoid_reuse` has been added (default off). When enabled,
 a wallet will distinguish between used and unused addresses, and default to not
 use the former in coin selection.
 
-(Note: rescanning the blockchain is required, to correctly mark previously
+Rescanning the blockchain is required, to correctly mark previously
-used destinations.)
+used destinations.
 
 Together with "avoid partial spends" (present as of Bitcoin v0.17), this
 addresses a serious privacy issue where a malicious user can track spends by
@@ -30,10 +30,12 @@ These include:
 
 - createwallet
 - getbalance
+- getbalances
 - sendtoaddress
 
-In addition, `sendtoaddress` has been changed to enable `-avoidpartialspends` when
+In addition, `sendtoaddress` has been changed to avoid partial spends when `avoid_reuse`
-`avoid_reuse` is enabled.
+is enabled (if not already enabled via the  `-avoidpartialspends` command line flag),
+as it would otherwise risk using up the "wrong" UTXO for an address reuse case.
 
 The listunspent RPC has also been updated to now include a "reused" bool, for nodes
 with "avoid_reuse" enabled.

src/wallet/rpcwallet.cpp

@@ -366,7 +366,7 @@ static UniValue sendtoaddress(const JSONRPCRequest& request)
     "       \"UNSET\"\n"
     "       \"ECONOMICAL\"\n"
     "       \"CONSERVATIVE\""},
-            {"avoid_reuse", RPCArg::Type::BOOL, /* default */ pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) ? "true" : "unavailable", "Avoid spending from dirty addresses; addresses are considered\n"
+            {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n"
     "                             dirty if they have previously been used in a transaction."},
         },
         RPCResult{
@@ -753,7 +753,7 @@ static UniValue getbalance(const JSONRPCRequest& request)
             {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."},
             {"addlocked", RPCArg::Type::BOOL, /* default */ "false", "Whether to include transactions locked via InstantSend in the wallet's balance."},
             {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also include balance in watch-only addresses (see 'importaddress')"},
-            {"avoid_reuse", RPCArg::Type::BOOL, /* default */ pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE) ? "true" : "unavailable", "Do not include balance in dirty outputs; addresses are considered dirty if they have previously been used in a transaction."},
+            {"avoid_reuse", RPCArg::Type::BOOL, /* default */ "true", "(only available if avoid_reuse wallet flag is set) Do not include balance in dirty outputs; addresses are considered dirty if they have previously been used in a transaction."},
         },
         RPCResult{
             RPCResult::Type::STR_AMOUNT, "amount", "The total amount in " + CURRENCY_UNIT + " received for this wallet."
@@ -2394,6 +2394,7 @@ static UniValue getbalances(const JSONRPCRequest& request)
                                 {RPCResult::Type::STR_AMOUNT, "trusted", " trusted balance (outputs created by the wallet or confirmed outputs)"},
                                 {RPCResult::Type::STR_AMOUNT, "untrusted_pending", " untrusted pending balance (outputs created by others that are in the mempool)"},
                                 {RPCResult::Type::STR_AMOUNT, "immature", " balance from immature coinbase outputs"},
+                                {RPCResult::Type::STR_AMOUNT, "used", "(only present if avoid_reuse is set) balance from coins sent to addresses that were previously spent from (potentially privacy violating)"},
                                 {RPCResult::Type::STR_AMOUNT, "coinjoin", " CoinJoin balance (outputs with enough rounds created by the wallet via mixing)"},
                         }},
                         {RPCResult::Type::OBJ, "watchonly", "watchonly balances (not present if wallet does not watch anything)",
@@ -2429,6 +2430,12 @@ static UniValue getbalances(const JSONRPCRequest& request)
         balances_mine.pushKV("trusted", ValueFromAmount(bal.m_mine_trusted));
         balances_mine.pushKV("untrusted_pending", ValueFromAmount(bal.m_mine_untrusted_pending));
         balances_mine.pushKV("immature", ValueFromAmount(bal.m_mine_immature));
+        if (wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {
+            // If the AVOID_REUSE flag is set, bal has been set to just the un-reused address balance. Get
+            // the total balance, and then subtract bal to get the reused address balance.
+            const auto full_bal = wallet.GetBalance(0, false);
+            balances_mine.pushKV("used", ValueFromAmount(full_bal.m_mine_trusted + full_bal.m_mine_untrusted_pending - bal.m_mine_trusted - bal.m_mine_untrusted_pending));
+        }
         balances_mine.pushKV("coinjoin", ValueFromAmount(bal.m_anonymized));
         balances.pushKV("mine", balances_mine);
     }
@@ -2965,7 +2972,6 @@ static UniValue listunspent(const JSONRPCRequest& request)
     if (!wallet) return NullUniValue;
     CWallet* const pwallet = wallet.get();
 
-    bool avoid_reuse = pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
     RPCHelpMan{"listunspent",
         "\nReturns array of unspent transaction outputs\n"
         "with between minconf and maxconf (inclusive) confirmations.\n"
@@ -3008,7 +3014,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
                     {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"},
                     {RPCResult::Type::BOOL, "solvable", "Whether we know how to spend this output, ignoring the lack of keys"},
                     {RPCResult::Type::STR, "desc", "(only when solvable) A descriptor for spending this output"},
-                    {RPCResult::Type::BOOL, "reused", /* optional*/ true, "Whether this output is reused/dirty (sent to an address that was previously spent from)"},
+                    {RPCResult::Type::BOOL, "reused", /* optional*/ true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"},
                     {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions"
                                                     "                             from outside keys and unconfirmed replacement transactions are considered unsafe\n"
                                                     "and are not eligible for spending by fundrawtransaction and sendtoaddress."},
@@ -3121,6 +3127,8 @@ static UniValue listunspent(const JSONRPCRequest& request)
 
     LOCK(pwallet->cs_wallet);
 
+    const bool avoid_reuse = pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
+
     for (const COutput& out : vecOutputs) {
         CTxDestination address;
         const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey;

test/functional/wallet_avoidreuse.py

@@ -62,6 +62,12 @@ def assert_unspent(node, total_count=None, total_sum=None, reused_supported=None
     if reused_sum is not None:
         assert_approx(stats["reused"]["sum"], reused_sum, 0.001)
 
+def assert_balances(node, mine):
+    '''Make assertions about a node's getbalances output'''
+    got = node.getbalances()["mine"]
+    for k,v in mine.items():
+        assert_approx(got[k], v, 0.001)
+
 class AvoidReuseTest(BitcoinTestFramework):
 
     def set_test_params(self):
@@ -149,6 +155,10 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 1 single, unused 10 btc output
         assert_unspent(self.nodes[1], total_count=1, total_sum=10, reused_supported=True, reused_count=0)
+        # getbalances should show no used, 10 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 0, "trusted": 10})
+        # node 0 should not show a used entry, as it does not enable avoid_reuse
+        assert("used" not in self.nodes[0].getbalances()["mine"])
 
         self.nodes[1].sendtoaddress(retaddr, 5)
         self.nodes[0].generate(1)
@@ -156,6 +166,8 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 1 single, unused 5 btc output
         assert_unspent(self.nodes[1], total_count=1, total_sum=5, reused_supported=True, reused_count=0)
+        # getbalances should show no used, 5 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 0, "trusted": 5})
 
         self.nodes[0].sendtoaddress(fundaddr, 10)
         self.nodes[0].generate(1)
@@ -163,11 +175,15 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 2 total outputs (5, 10 btc), one unused (5), one reused (10)
         assert_unspent(self.nodes[1], total_count=2, total_sum=15, reused_count=1, reused_sum=10)
+        # getbalances should show 10 used, 5 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 10, "trusted": 5})
 
         self.nodes[1].sendtoaddress(address=retaddr, amount=10, avoid_reuse=False)
 
         # listunspent should show 1 total outputs (5 btc), unused
         assert_unspent(self.nodes[1], total_count=1, total_sum=5, reused_count=0)
+        # getbalances should show no used, 5 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 0, "trusted": 5})
 
         # node 1 should now have about 5 btc left (for both cases)
         assert_approx(self.nodes[1].getbalance(), 5, 0.001)
@@ -193,6 +209,8 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 1 single, unused 10 btc output
         assert_unspent(self.nodes[1], total_count=1, total_sum=10, reused_supported=True, reused_count=0)
+        # getbalances should show no used, 10 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 0, "trusted": 10})
 
         self.nodes[1].sendtoaddress(retaddr, 5)
         self.nodes[0].generate(1)
@@ -200,6 +218,8 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 1 single, unused 5 btc output
         assert_unspent(self.nodes[1], total_count=1, total_sum=5, reused_supported=True, reused_count=0)
+        # getbalances should show no used, 5 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 0, "trusted": 5})
 
         self.nodes[0].sendtoaddress(fundaddr, 10)
         self.nodes[0].generate(1)
@@ -207,6 +227,8 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 2 total outputs (5, 10 btc), one unused (5), one reused (10)
         assert_unspent(self.nodes[1], total_count=2, total_sum=15, reused_count=1, reused_sum=10)
+        # getbalances should show 10 used, 5 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 10, "trusted": 5})
 
         # node 1 should now have a balance of 5 (no dirty) or 15 (including dirty)
         assert_approx(self.nodes[1].getbalance(), 5, 0.001)
@@ -218,6 +240,8 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # listunspent should show 2 total outputs (1, 10 btc), one unused (1), one reused (10)
         assert_unspent(self.nodes[1], total_count=2, total_sum=11, reused_count=1, reused_sum=10)
+        # getbalances should show 10 used, 1 btc trusted
+        assert_balances(self.nodes[1], mine={"used": 10, "trusted": 1})
 
         # node 1 should now have about 1 btc left (no dirty) and 11 (including dirty)
         assert_approx(self.nodes[1].getbalance(), 1, 0.001)