diff --git a/doc/release-notes-13756.md b/doc/release-notes-13756.md index 21006f46a0..a500aceb0f 100644 --- a/doc/release-notes-13756.md +++ b/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 -used destinations.) +Rescanning the blockchain is required, to correctly mark previously +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 -`avoid_reuse` is enabled. +In addition, `sendtoaddress` has been changed to avoid partial spends when `avoid_reuse` +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. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f6ad0ba7e2..f983b03414 100644 --- a/src/wallet/rpcwallet.cpp +++ b/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; diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py index f26f83db24..47d16c35a0 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/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)