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

pull kallewoof wants to merge 3 commits into bitcoin:master from kallewoof:2019-06-followup-avoidreuse changing 3 files +49 −15
  1. kallewoof commented at 3:01 am on June 19, 2019: member

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

    Ping jnewbery and achow101 as they pointed out these issues.

  2. fanquake added the label Wallet on Jun 19, 2019
  3. fanquake added the label RPC/REST/ZMQ on Jun 19, 2019
  4. fanquake requested review from jnewbery on Jun 19, 2019
  5. fanquake requested review from achow101 on Jun 19, 2019
  6. 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.

  7. 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.

  8. 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

  9. 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.
  10. 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.

  11. 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”.

  12. promag commented at 1:48 pm on June 19, 2019: member
    Concept ACK.
  13. kallewoof force-pushed on Jun 19, 2019
  14. kallewoof commented at 2:26 pm on June 19, 2019: member
    @promag Thanks a lot for review! I addressed everything you noted.
  15. 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.
  16. 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.
  17. promag commented at 3:05 pm on June 19, 2019: member
    Sorry for the 2nd round, just 2 nits.
  18. kallewoof force-pushed on Jun 19, 2019
  19. kallewoof commented at 3:46 pm on June 19, 2019: member
    @promag Thanks for thoroughness, it is very appreciated. Addressed all.
  20. MarcoFalke commented at 3:49 pm on June 19, 2019: member
    I left some review in the other pull, which could be addressed here?
  21. kallewoof force-pushed on Jun 19, 2019
  22. kallewoof commented at 4:09 pm on June 19, 2019: member
    Addressed @MarcoFalke comments in #13756. Thanks for feedback!
  23. 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
  24. 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) ...
  25. MarcoFalke commented at 4:33 pm on June 19, 2019: member
    ACK, but what about #13756 (review)
  26. kallewoof force-pushed on Jun 19, 2019
  27. 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.
  28. MarcoFalke commented at 4:59 pm on June 19, 2019: member
    ACK b59d4c7da1b886a0834211e70850127faf3e121e (scrolled through the diff on GitHub)
  29. 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)
  30. 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.
  31. kallewoof force-pushed on Jun 21, 2019
  32. kallewoof commented at 2:38 am on June 21, 2019: member
    @jnewbery thanks! Trimmed tests as suggested.
  33. 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

  34. 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.

  35. 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.

  36. achow101 approved
  37. achow101 commented at 1:50 pm on June 21, 2019: member
    ACK 7f39c40a332824043e3bef282580daf748131bb4 Read through the diff, looks good.
  38. 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”

  39. 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!
  40. 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
  41. 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.

  42. wallet/rpc/getbalances: add entry for 'mine.used' balance in results 53c3c1ea9e
  43. 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
  44. docs: release note wording 71d0344cf2
  45. kallewoof force-pushed on Jun 21, 2019
  46. jnewbery commented at 6:28 pm on June 21, 2019: member

    ACK 71d0344cf25d3aaf60112c5248198c444bc98105

    Thanks for quick turnaround!

  48. meshcollider merged this on Jun 22, 2019
  49. meshcollider closed this on Jun 22, 2019
  50. meshcollider referenced this in commit 2cbcc55ba6 on Jun 22, 2019
  51. sidhujag referenced this in commit ef2a6872bc on Jun 22, 2019
  52. kallewoof deleted the branch on Jun 24, 2019
  53. fanquake referenced this in commit ca80fec973 on Jun 26, 2019
  54. deadalnix referenced this in commit e6bbd99007 on Jun 10, 2020
  55. deadalnix referenced this in commit 14ba6321bf on Jun 10, 2020
  56. deadalnix referenced this in commit 00d87deb9d on Jun 10, 2020
  57. MarcoFalke locked this on Dec 16, 2021


kallewoof DrahtBot promag MarcoFalke jnewbery meshcollider achow101

Labels
Wallet RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me