wallet/rpc: follow-up clean-up/fixes to avoid

kallewoof commented at 3:01 am on June 19, 2019: member

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

First commit addresses #13756 (review)
Second commit addresses #13756 (review)

Ping jnewbery and achow101 as they pointed out these issues.

fanquake added the label Wallet on Jun 19, 2019

fanquake added the label RPC/REST/ZMQ on Jun 19, 2019

fanquake requested review from jnewbery on Jun 19, 2019

fanquake requested review from achow101 on Jun 19, 2019

DrahtBot commented at 7:20 am on June 19, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16240 (JSONRPCRequest-aware RPCHelpMan by kallewoof)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/rpcwallet.cpp:2412 in 7e7bb17352 outdated

2408@@ -2409,6 +2409,7 @@ static UniValue getbalances(const JSONRPCRequest& request)
2409             "      \"trusted\": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)\n"
2410             "      \"untrusted_pending\": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)\n"
2411             "      \"immature\": xxx                (numeric) balance from immature coinbase outputs\n"
2412+            "      \"used\": xxx                    (numeric) balance from coins sent to addresses that were previously spent from (potentially privacy violating)\n"

promag commented at 1:39 pm on June 19, 2019:

7e7bb17352a3a74e142eae20572a163384fa1502

Only present if avoid_reuse flag is set.

in src/wallet/rpcwallet.cpp:2438 in 7e7bb17352 outdated

2434@@ -2434,13 +2435,16 @@ static UniValue getbalances(const JSONRPCRequest& request)
2435 
2436     UniValue obj(UniValue::VOBJ);
2437 
2438+    bool avoid_reuse = wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);

promag commented at 1:39 pm on June 19, 2019:

7e7bb17352a3a74e142eae20572a163384fa1502

nit, const

in src/wallet/rpcwallet.cpp:2447 in 7e7bb17352 outdated

2442     {
2443         UniValue balances_mine{UniValue::VOBJ};
2444         balances_mine.pushKV("trusted", ValueFromAmount(bal.m_mine_trusted));
2445         balances_mine.pushKV("untrusted_pending", ValueFromAmount(bal.m_mine_untrusted_pending));
2446         balances_mine.pushKV("immature", ValueFromAmount(bal.m_mine_immature));
2447+        if (avoid_reuse) balances_mine.pushKV("used", ValueFromAmount(bal_used.m_mine_trusted + bal_used.m_mine_untrusted_pending - bal.m_mine_trusted - bal.m_mine_untrusted_pending));

promag commented at 1:41 pm on June 19, 2019:

7e7bb17352a3a74e142eae20572a163384fa1502

Maybe test that this is not present if avoid_reused flag is not set.

kallewoof commented at 2:22 pm on June 19, 2019:

Adding check to wallet_avoidreuse.py that node 0 does not include a “used” key.

in src/wallet/rpcwallet.cpp:2892 in 5e704ab663 outdated

2890@@ -2891,9 +2891,8 @@ static UniValue listunspent(const JSONRPCRequest& request)
2891 
2892     bool avoid_reuse = pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);

promag commented at 1:46 pm on June 19, 2019:

5e704ab66336eafef2955e3087f7bbf8c7ca26cd

Move after wallet lock in L3023.

in src/wallet/rpcwallet.cpp:2932 in 5e704ab663 outdated

2928@@ -2930,9 +2929,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
2929             "    \"witnessScript\" : \"script\" (string) witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n"
2930             "    \"spendable\" : xxx,        (bool) Whether we have the private keys to spend this output\n"
2931             "    \"solvable\" : xxx,         (bool) Whether we know how to spend this output, ignoring the lack of keys\n"
2932-            + (avoid_reuse ?
2933-            "    \"reused\" : xxx,           (bool) Whether this output is reused/dirty (sent to an address that was previously spent from)\n" :
2934-            "") +
2935+            "    \"reused\" : xxx,           (bool) Whether this output is reused/dirty (sent to an address that was previously spent from)\n"

promag commented at 1:47 pm on June 19, 2019:

5e704ab66336eafef2955e3087f7bbf8c7ca26cd

Mention “only present if avoid_reuse flag is set”.

promag commented at 1:48 pm on June 19, 2019: member

Concept ACK.

kallewoof force-pushed on Jun 19, 2019

kallewoof commented at 2:26 pm on June 19, 2019: member

@promag Thanks a lot for review! I addressed everything you noted.

in src/wallet/rpcwallet.cpp:3023 in c8568bb2a2 outdated

3019@@ -3017,6 +3020,8 @@ static UniValue listunspent(const JSONRPCRequest& request)
3020 
3021     LOCK(pwallet->cs_wallet);
3022 
3023+    bool avoid_reuse = pwallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);

promag commented at 2:45 pm on June 19, 2019:

nit, const.

in test/functional/wallet_avoidreuse.py:72 in c8568bb2a2 outdated

67+    '''Make assertions about a node's getbalances output'''
68+    balances = node.getbalances()
69+    if mine:
70+        got = balances["mine"]
71+        for k,v in mine.items():
72+            assert(k in got)

promag commented at 3:03 pm on June 19, 2019:

nit, IMO these assertions aren’t necessary, assert_approx are enough.

kallewoof commented at 3:46 pm on June 19, 2019:

Yeah, agreed, removing.

promag commented at 3:05 pm on June 19, 2019: member

Sorry for the 2nd round, just 2 nits.

kallewoof force-pushed on Jun 19, 2019

kallewoof commented at 3:46 pm on June 19, 2019: member

@promag Thanks for thoroughness, it is very appreciated. Addressed all.

MarcoFalke commented at 3:49 pm on June 19, 2019: member

I left some review in the other pull, which could be addressed here?

kallewoof force-pushed on Jun 19, 2019

kallewoof commented at 4:09 pm on June 19, 2019: member

Addressed @MarcoFalke comments in #13756. Thanks for feedback!

in doc/release-notes-13756.md:10 in 5ff6aaa2d2 outdated

 6@@ -7,7 +7,7 @@ A new wallet flag `avoid_reuse` has been added (default off). When enabled,
 7 a wallet will distinguish between used and unused addresses, and default to not
 8 use the former in coin selection.
 9 
10-(Note: rescanning the blockchain is required, to correctly mark previously
11+(Rescanning the blockchain is required, to correctly mark previously

MarcoFalke commented at 4:29 pm on June 19, 2019:

0Rescanning the blockchain is required, to correctly mark previously

in doc/release-notes-13756.md:36 in 5ff6aaa2d2 outdated

29@@ -30,10 +30,12 @@ These include:
30 
31 - createwallet
32 - getbalance
33+- getbalances
34 - sendtoaddress
35 
36-In addition, `sendtoaddress` has been changed to enable `-avoidpartialspends` when
37-`avoid_reuse` is enabled.
38+In addition, `sendtoaddress` has been changed to always use `-avoidpartialspends`

MarcoFalke commented at 4:30 pm on June 19, 2019:

0In addition, `sendtoaddress` has been changed to avoid partial spends ... (if not already enabled via the  `-avoidpartialspends` command line flag) ...

MarcoFalke commented at 4:33 pm on June 19, 2019: member

ACK, but what about #13756 (review)

kallewoof force-pushed on Jun 19, 2019

kallewoof commented at 4:55 pm on June 19, 2019: member

@MarcoFalke Thanks, I missed that one. I believe I got all of them this time. Also updated release notes based on your suggestions.

MarcoFalke commented at 4:59 pm on June 19, 2019: member

ACK b59d4c7da1b886a0834211e70850127faf3e121e (scrolled through the diff on GitHub)

in test/functional/wallet_avoidreuse.py:66 in b59d4c7da1 outdated

62@@ -63,6 +63,18 @@ def assert_unspent(node, total_count=None, total_sum=None, reused_supported=None
63     if reused_sum is not None:
64         assert_approx(stats["reused"]["sum"], reused_sum, 0.001)
65 
66+def assert_balances(node, mine=None, watchonly=None):

jnewbery commented at 5:34 pm on June 20, 2019:

I don’t think watchonly is ever used. Just remove it (and the if watchonly block in the function)

in test/functional/wallet_avoidreuse.py:15 in b59d4c7da1 outdated

11@@ -12,11 +12,11 @@
12 )
13 
14 # TODO: Copied from wallet_groups.py -- should perhaps move into util.py
15-def assert_approx(v, vexp, vspan=0.00001):
16+def assert_approx(v, vexp, vspan=0.00001, descr=None):

jnewbery commented at 5:36 pm on June 20, 2019:

You can remove these change since assert_balances() is never called with watchonly, and so assert_approx() is only called for mine balances.

kallewoof force-pushed on Jun 21, 2019

kallewoof commented at 2:38 am on June 21, 2019: member

@jnewbery thanks! Trimmed tests as suggested.

meshcollider commented at 7:55 am on June 21, 2019: contributor

Code looks good, code review ACK https://github.com/bitcoin/bitcoin/pull/16239/commits/7f39c40a332824043e3bef282580daf748131bb4

I’m just slightly unsure about the word “used” in the getbalance RPC, could seem like the balance which has been used (spent) rather than the balance which is “dirty” or “reused” or something

kallewoof commented at 12:29 pm on June 21, 2019: member

I initially used the word “dirty”, but then realized that is internally used for something else in the wallet, and there was a comment about “dirty” being confusing in #13756 (comment)

I think “dirty” is a confusing concept. Maybe add an exclamation mark (or a detective icon) next to the transaction and when the user clicks on that, say something like “This address has been used before, for privacy reasons it’s better to create a new address each time you wish to receive coins, even from the same person”.

(in reference to a GUI comment).

I’m open for changing it, but I’m not sure what would be a good name for it.

jnewbery commented at 1:29 pm on June 21, 2019: member

I’m open for changing it, but I’m not sure what would be a good name for it.

How about “used address balance” or “reused address balance”? Bit of a mouthful, but at least it’s explicit.

achow101 approved

achow101 commented at 1:50 pm on June 21, 2019: member

ACK 7f39c40a332824043e3bef282580daf748131bb4 Read through the diff, looks good.

kallewoof commented at 1:54 pm on June 21, 2019: member

@jnewbery A bit long, maybe.. I mean, if we change it I would ideally like to change it everywhere, including the “avoid_reuse” flags in RPC commands and such.

Even “used”/“reused” does not accurately describe what this is referring to, as evident by the fact I as the author screwed it up (thankfully @jnewbery caught that one).

“UTXO/balance that was sent to an address after it was spent from”

in src/wallet/rpcwallet.cpp:2440 in 7f39c40a33 outdated

2434@@ -2434,13 +2435,16 @@ static UniValue getbalances(const JSONRPCRequest& request)
2435 
2436     UniValue obj(UniValue::VOBJ);
2437 
2438+    const bool avoid_reuse = wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
2439     const auto bal = wallet.GetBalance();
2440+    const auto bal_used = avoid_reuse ? wallet.GetBalance(0, false) : bal;

jnewbery commented at 3:33 pm on June 21, 2019:

I felt like this logic could have used more commenting, and the variable was a bit confusingly named (and also could potentially be scoped more tightly).

I think the following is equivalent, but perhaps a little clearer:

 0→ git d
 1diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
 2index ce1c94905..f53469ce6 100644
 3--- a/src/wallet/rpcwallet.cpp
 4+++ b/src/wallet/rpcwallet.cpp
 5@@ -2435,16 +2435,19 @@ static UniValue getbalances(const JSONRPCRequest& request)
 6 
 7     UniValue obj(UniValue::VOBJ);
 8 
 9-    const bool avoid_reuse = wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE);
10     const auto bal = wallet.GetBalance();
11-    const auto bal_used = avoid_reuse ? wallet.GetBalance(0, false) : bal;
12     UniValue balances{UniValue::VOBJ};
13     {
14         UniValue balances_mine{UniValue::VOBJ};
15         balances_mine.pushKV("trusted", ValueFromAmount(bal.m_mine_trusted));
16         balances_mine.pushKV("untrusted_pending", ValueFromAmount(bal.m_mine_untrusted_pending));
17         balances_mine.pushKV("immature", ValueFromAmount(bal.m_mine_immature));
18-        if (avoid_reuse) balances_mine.pushKV("used", ValueFromAmount(bal_used.m_mine_trusted + bal_used.m_mine_untrusted_pending - bal.m_mine_trusted - bal.m_mine_untrusted_pending));
19+        if (wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {
20+            // If the AVOID_REUSE flag is set, bal has been set to just the un-reused address balance. Get
21+            // the total balance, and then subtract bal to get the reused address balance.
22+            const auto full_bal = wallet.GetBalance(0, false);
23+            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));
24+        }
25         balances.pushKV("mine", balances_mine);
26     }
27     if (wallet.HaveWatchOnly()) {

Feel free to use if you think that’s better.

kallewoof commented at 5:43 pm on June 21, 2019:

Yep, that looks cleaner. Thanks!

in test/functional/wallet_avoidreuse.py:69 in 7f39c40a33 outdated

62@@ -63,6 +63,13 @@ def assert_unspent(node, total_count=None, total_sum=None, reused_supported=None
63     if reused_sum is not None:
64         assert_approx(stats["reused"]["sum"], reused_sum, 0.001)
65 
66+def assert_balances(node, mine):
67+    '''Make assertions about a node's getbalances output'''
68+    balances = node.getbalances()
69+    got = balances["mine"]

jnewbery commented at 3:35 pm on June 21, 2019:

micronit (not worth changing): could be combined with line above

jnewbery commented at 3:41 pm on June 21, 2019: member

ACK 7f39c40a332824043e3bef282580daf748131bb4 (reviewed code and ran tests locally)

I’ve left a couple of style nits inline. Feel free to take or leave.

wallet/rpc/getbalances: add entry for 'mine.used' balance in results 53c3c1ea9e

wallet/rpc: use static help text

Always show the same help topic regardless of wallet flags, and explain that something is not always available, rather than runtime-modifying the help output.

3d2ff37913

docs: release note wording 71d0344cf2

kallewoof force-pushed on Jun 21, 2019

jnewbery commented at 6:28 pm on June 21, 2019: member

ACK 71d0344cf25d3aaf60112c5248198c444bc98105

Thanks for quick turnaround!

meshcollider commented at 9:58 am on June 22, 2019: contributor

re-utACK https://github.com/bitcoin/bitcoin/pull/16239/commits/71d0344cf25d3aaf60112c5248198c444bc98105

meshcollider merged this on Jun 22, 2019

meshcollider closed this on Jun 22, 2019

meshcollider referenced this in commit 2cbcc55ba6 on Jun 22, 2019

sidhujag referenced this in commit ef2a6872bc on Jun 22, 2019

kallewoof deleted the branch on Jun 24, 2019

fanquake referenced this in commit ca80fec973 on Jun 26, 2019

deadalnix referenced this in commit e6bbd99007 on Jun 10, 2020

deadalnix referenced this in commit 14ba6321bf on Jun 10, 2020

deadalnix referenced this in commit 00d87deb9d on Jun 10, 2020

MarcoFalke locked this on Dec 16, 2021

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

Conflicts