diff options
| -rw-r--r-- | doc/release-notes-13756.md | 10 | ||||
| -rw-r--r-- | src/wallet/rpcwallet.cpp | 30 | ||||
| -rwxr-xr-x | test/functional/wallet_avoidreuse.py | 24 |
3 files changed, 49 insertions, 15 deletions
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 3232c65bca..eae5f876ea 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -383,7 +383,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{ @@ -743,7 +743,7 @@ static UniValue getbalance(const JSONRPCRequest& request) {"dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Remains for backward compatibility. Must be excluded or set to \"*\"."}, {"minconf", RPCArg::Type::NUM, /* default */ "0", "Only include transactions confirmed at least this many times."}, {"include_watchonly", RPCArg::Type::BOOL, /* default */ "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{ "amount (numeric) The total amount in " + CURRENCY_UNIT + " received for this wallet.\n" @@ -2409,6 +2409,7 @@ static UniValue getbalances(const JSONRPCRequest& request) " \"trusted\": xxx (numeric) trusted balance (outputs created by the wallet or confirmed outputs)\n" " \"untrusted_pending\": xxx (numeric) untrusted pending balance (outputs created by others that are in the mempool)\n" " \"immature\": xxx (numeric) balance from immature coinbase outputs\n" + " \"used\": xxx (numeric) (only present if avoid_reuse is set) balance from coins sent to addresses that were previously spent from (potentially privacy violating)\n" " },\n" " \"watchonly\": { (object) watchonly balances (not present if wallet does not watch anything)\n" " \"trusted\": xxx (numeric) trusted balance (outputs created by the wallet or confirmed outputs)\n" @@ -2441,6 +2442,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.pushKV("mine", balances_mine); } if (wallet.HaveWatchOnly()) { @@ -2885,11 +2892,8 @@ static UniValue listunspent(const JSONRPCRequest& request) return NullUniValue; } - bool avoid_reuse = pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); - - if (request.fHelp || request.params.size() > 5) - throw std::runtime_error( - RPCHelpMan{"listunspent", + const RPCHelpMan help{ + "listunspent", "\nReturns array of unspent transaction outputs\n" "with between minconf and maxconf (inclusive) confirmations.\n" "Optionally filter to only include txouts paid to specified addresses.\n", @@ -2926,9 +2930,7 @@ static UniValue listunspent(const JSONRPCRequest& request) " \"witnessScript\" : \"script\" (string) witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n" " \"spendable\" : xxx, (bool) Whether we have the private keys to spend this output\n" " \"solvable\" : xxx, (bool) Whether we know how to spend this output, ignoring the lack of keys\n" - + (avoid_reuse ? - " \"reused\" : xxx, (bool) Whether this output is reused/dirty (sent to an address that was previously spent from)\n" : - "") + + " \"reused\" : xxx, (bool) (only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)\n" " \"desc\" : xxx, (string, only when solvable) A descriptor for spending this output\n" " \"safe\" : xxx (bool) Whether this output is considered safe to spend. Unconfirmed transactions\n" " from outside keys and unconfirmed replacement transactions are considered unsafe\n" @@ -2944,7 +2946,11 @@ static UniValue listunspent(const JSONRPCRequest& request) + HelpExampleCli("listunspent", "6 9999999 '[]' true '{ \"minimumAmount\": 0.005 }'") + HelpExampleRpc("listunspent", "6, 9999999, [] , true, { \"minimumAmount\": 0.005 } ") }, - }.ToString()); + }; + + if (request.fHelp || !help.IsValidNumArgs(request.params.size())) { + throw std::runtime_error(help.ToString()); + } int nMinDepth = 1; if (!request.params[0].isNull()) { @@ -3017,6 +3023,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 1dec040f68..58ad835d39 100755 --- a/test/functional/wallet_avoidreuse.py +++ b/test/functional/wallet_avoidreuse.py @@ -63,6 +63,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): @@ -140,6 +146,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) @@ -147,6 +157,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) @@ -154,11 +166,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) @@ -183,6 +199,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) @@ -190,6 +208,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) @@ -197,6 +217,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) @@ -208,6 +230,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) |