wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 #18418

pull fjahr wants to merge 2 commits into bitcoin:master from fjahr:groups100 changing 4 files +27 −27
  1. fjahr commented at 3:13 pm on March 24, 2020: member

    Follow-up to #17824.

    This increases OUTPUT_GROUP_MAX_ENTRIES to 100 which means that OutputGroups will now be up to 100 outputs large, up from previously 10. The main motivation for this change is that during the PR review club on #17824 several participants signaled that 100 might be a better value here.

    I think fees should be manageable for users but more importantly, users should know what they can expect when using the wallet with this configuration, so I also tried to clarify the documentation on -avoidpartialspends and avoid_reuse a bit. If there are other additional ways how or docs where users can be made aware of the potential consequences of using these parameters, please let me know. Another small upside is that there seem to be a high number of batching transactions with 100 and 200 inputs(source) giving these transactions a bit of a larger anonymity set, although that is probably a very weak argument.

  2. jonatack commented at 3:59 pm on March 24, 2020: member
    Concept ACK
  3. laanwj added the label Wallet on Mar 24, 2020
  4. DrahtBot commented at 4:20 pm on March 24, 2020: 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:

    • #21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
    • #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

    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.

  5. Angelika1920 approved
  6. Angelika1920 approved
  7. jonatack commented at 3:38 pm on March 31, 2020: member

    Will review when #17824 is merged and this is rebased.

    Some context for reviewers: https://bitcoincore.reviews/17824.html#l-327 (from here to the end of the discussion)

  8. fanquake deleted a comment on Apr 2, 2020
  9. fjahr force-pushed on Apr 2, 2020
  10. fjahr commented at 1:51 pm on April 2, 2020: member
    Added some minor cleanups from #17824
  11. DrahtBot added the label Needs rebase on Apr 7, 2020
  12. fjahr force-pushed on Apr 14, 2020
  13. fjahr commented at 1:07 pm on April 14, 2020: member
    Rebased, no code changes.
  14. DrahtBot removed the label Needs rebase on Apr 14, 2020
  15. DrahtBot added the label Needs rebase on Apr 17, 2020
  16. fjahr force-pushed on Apr 17, 2020
  17. fjahr commented at 2:33 pm on April 17, 2020: member
    Rebased, since #17824 was merged.
  18. DrahtBot removed the label Needs rebase on Apr 17, 2020
  19. DrahtBot added the label Needs rebase on Jul 30, 2020
  20. fjahr commented at 6:24 pm on July 30, 2020: member
    Rebased
  21. fjahr force-pushed on Jul 30, 2020
  22. DrahtBot removed the label Needs rebase on Jul 30, 2020
  23. DrahtBot added the label Needs rebase on Sep 30, 2020
  24. fjahr force-pushed on Sep 30, 2020
  25. fjahr commented at 7:30 pm on September 30, 2020: member
    rebased
  26. DrahtBot removed the label Needs rebase on Sep 30, 2020
  27. meshcollider commented at 9:13 am on October 7, 2020: contributor

    I don’t really feel comfortable commenting on the concept of increasing it from 10 to 100, but the actual code change itself looks good modulo that assumption.

    utACK 2c58390085e565fbf2d587eff104a50bcea13674

  28. DrahtBot added the label Needs rebase on Nov 17, 2020
  29. fjahr force-pushed on Nov 29, 2020
  30. fjahr commented at 0:21 am on November 29, 2020: member
    Rebased
  31. DrahtBot removed the label Needs rebase on Nov 29, 2020
  32. DrahtBot added the label Needs rebase on Feb 1, 2021
  33. in src/wallet/wallet.cpp:53 in f873503690 outdated 
    49@@ -50,7 +50,7 @@ const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
50     },
51 };
52 
53-static const size_t OUTPUT_GROUP_MAX_ENTRIES = 10;
54+static const size_t OUTPUT_GROUP_MAX_ENTRIES = 100;
    jonatack commented at 7:01 pm on February 6, 2021: 
    0static constexpr size_t OUTPUT_GROUP_MAX_ENTRIES = 100;
    fjahr commented at 8:39 pm on February 7, 2021:
    Done
  34. in src/wallet/init.cpp:46 in f873503690 outdated 
    42@@ -43,7 +43,7 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
43 void WalletInit::AddWalletOptions(ArgsManager& argsman) const
44 {
45     argsman.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
46-    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u (always enabled for wallets with \"avoid_reuse\" enabled))", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
47+    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by public key, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as the public key is mostly swept and outputs are aggregated in a clean change address. It may result in higher fees due to suboptimal coin selection caused by this added limitation and possibly larger-than-necessary number of inputs being used (default: %u (always enabled for wallets with \"avoid_reuse\" enabled))", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
    jonatack commented at 7:43 pm on February 6, 2021: 
    0    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by public key, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as the public key is mostly swept and outputs are aggregated in a clean change address. It may result in higher fees due to less optimal coin selection caused by this added limitation and possibly a larger-than-necessary number of inputs being used. Always enabled for wallets with \"avoid_reuse\" enabled, otherwise default: %u.", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

    which would change this from

    0       clean change address. It may result in higher fees due to
1       suboptimal coin selection caused by this added limitation and
2       possibly larger-than-necessary number of inputs being used
3       (default: 0 (always enabled for wallets with "avoid_reuse"
4       enabled))

    to

    0       clean change address. It may result in higher fees due to less
1       optimal coin selection caused by this added limitation and
2       possibly a larger-than-necessary number of inputs being used.
3       Always enabled for wallets with "avoid_reuse" enabled, otherwise
4       default: 0.
    fjahr commented at 8:39 pm on February 7, 2021:
    Done
  35. jonatack commented at 7:58 pm on February 6, 2021: member
    LGTM with a couple of suggestions below. Build and tested that the changed margin (vspan) argument in test/functional/wallet_avoidreuse.py is needed. Needs rebase, otherwise seems good modulo sign-off by some experienced coin selection reviewers.
  36. fjahr force-pushed on Feb 7, 2021
  37. fjahr commented at 8:41 pm on February 7, 2021: member
    Thanks a lot for reviving this @jonatack ! I took your suggestions and rebased. The changes in the first commit were made redundant by changes in master, so I dropped it.
  38. DrahtBot removed the label Needs rebase on Feb 7, 2021
  39. DrahtBot added the label Needs rebase on Apr 19, 2021
  40. fjahr force-pushed on Apr 19, 2021
  41. fjahr commented at 7:34 pm on April 19, 2021: member
    Rebased
  42. DrahtBot removed the label Needs rebase on Apr 19, 2021
  43. in src/wallet/init.cpp:46 in 345b5abfb8 outdated 
    42@@ -43,7 +43,7 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
43 void WalletInit::AddWalletOptions(ArgsManager& argsman) const
44 {
45     argsman.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
46-    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u (always enabled for wallets with \"avoid_reuse\" enabled))", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
47+    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by public key, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as the public key is mostly swept and outputs are aggregated in a clean change address. It may result in higher fees due to less optimal coin selection caused by this added limitation and possibly a larger-than-necessary number of inputs being used. Always enabled for wallets with \"avoid_reuse\" enabled, otherwise default: %u.", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
    Xekyo commented at 4:28 pm on April 21, 2021:

    The ‘mostly’ was a bit begging the question in what cases it doesn’t sweep all UTXOs. Maybe:

    0    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by public key, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as public keys are swept with fewer transactions and outputs are aggregated in clean change addresses. It may result in higher fees due to less optimal coin selection caused by this added limitation and possibly a larger-than-necessary number of inputs being used. Always enabled for wallets with \"avoid_reuse\" enabled, otherwise default: %u.", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
    fjahr commented at 9:34 pm on April 21, 2021:
    Thanks, makes sense! Done.
  44. Xekyo commented at 4:37 pm on April 21, 2021: member
    Concept ACK and code-review ACK 345b5abfb838dc162bac47098f445f250e1643bd Perhaps the motivation could be strengthened by elaborating in which scenarios address reuse may occur in combination with avoidpartialspends to highlight why a higher value would make sense.
  45. fjahr force-pushed on Apr 21, 2021
  46. Xekyo commented at 2:43 pm on April 22, 2021: member
    tACK 241e14162fdfdddf697536113a68a3b11449db63
  47. in src/wallet/wallet.cpp:56 in 241e14162f outdated 
    52@@ -53,7 +53,7 @@ const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
53     },
54 };
55 
56-static const size_t OUTPUT_GROUP_MAX_ENTRIES = 10;
57+static constexpr size_t OUTPUT_GROUP_MAX_ENTRIES = 100;
    glozow commented at 1:50 pm on May 15, 2021: 
    0static constexpr size_t OUTPUT_GROUP_MAX_ENTRIES{100};
    mzumsande commented at 9:03 pm on May 17, 2021:
    There is a comment that became outdated and should maybe just refer to OUTPUT_GROUP_MAX_ENTRIES: // Cases where we have 11+ outputs all pointing to the same destination may result in privacy leaks as they will potentially be deterministically sorted.
    fjahr commented at 0:13 am on May 18, 2021:
    done
    fjahr commented at 0:15 am on May 18, 2021:
    Very nice find! Fixed!
  48. in src/wallet/init.cpp:46 in 241e14162f outdated 
    42@@ -43,7 +43,7 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
43 void WalletInit::AddWalletOptions(ArgsManager& argsman) const
44 {
45     argsman.AddArg("-addresstype", strprintf("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")", FormatOutputType(DEFAULT_ADDRESS_TYPE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
46-    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting all or none, instead of selecting on a per-output basis. Privacy is improved as an address is only used once (unless someone sends to it after spending from it), but may result in slightly higher fees as suboptimal coin selection may result due to the added limitation (default: %u (always enabled for wallets with \"avoid_reuse\" enabled))", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
47+    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by public key, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as public keys are mostly swept with fewer transactions and outputs are aggregated in clean change addresses. It may result in higher fees due to less optimal coin selection caused by this added limitation and possibly a larger-than-necessary number of inputs being used. Always enabled for wallets with \"avoid_reuse\" enabled, otherwise default: %u.", DEFAULT_AVOIDPARTIALSPENDS), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
    glozow commented at 5:17 pm on May 15, 2021:
    Isn’t it by scriptPubKey? It seems to me that “address” might be more accurate / intuitive for users than public key?
    jnewbery commented at 1:40 pm on May 17, 2021:
    I agree. UTXOs could be sent to multiple addresses corresponding to the same pubkey, and they’d be grouped into different Output Groups.
    fjahr commented at 0:14 am on May 18, 2021:
    A scriptPubKey can also have multiple address types and those would be grouped in the same output group, which was the motivation to make this more precise if I remember correctly. So spk would be the correct term to use here IMO. But it seems there is a lot of support for not making a change and spk might be too technical here, so I have reverted this bit.
    jnewbery commented at 9:02 am on May 19, 2021:

    A scriptPubKey can also have multiple address types

    I don’t believe this is true. A scriptPubKey will encode deterministically to a single address (base58 for P2PKH and P2SH, bech32 for P2WPKH and P2WSH, bech32m for segwit v1+).

    fjahr commented at 8:34 pm on May 26, 2021:
    I guess this code changed since I opened this PR then. It used to be an issue, see #17605. As sipa mentions there it was never an issue on descriptor wallets but I didn’t expect that behavior was changed on legacy wallets as well.
    fjahr commented at 8:59 pm on May 26, 2021:
    Never mind, this was about marking as reused, not the grouping. I guess this is what made me do the doc change in the first place but I was just confused.
  49. in test/functional/wallet_avoidreuse.py:343 in 241e14162f outdated 
    341@@ -342,14 +342,14 @@ def test_full_destination_group_is_preferred(self):
342         txid = self.nodes[1].sendtoaddress(address=ret_addr, amount=0.5)
343         inputs = self.nodes[1].getrawtransaction(txid, 1)["vin"]
    glozow commented at 6:11 pm on May 15, 2021:

    Hmm, I added a line to grab the amount and fee for this transaction here, i.e.

    0txinfo = self.nodes[1].gettransaction(txid)
1print("Paid fee of {} when sending {}".format(txinfo["fee"], txinfo["amount"]))

    It looks like the fee would be 0.00136966BTC :scream: I thought it might get caught by maxapsfee but it went through. Am I missing something else that would catch this case?

    I understand this scenario isn’t typical, but is this maybe… not the best default option for users? 0.00137 is like $66, and they could be paying this fee on a 0.001BTC transaction?

    jnewbery commented at 1:48 pm on May 17, 2021:

    I don’t think this is a problem (or at least not worse than any alternative). The high fees are intrinsic in the UTXOs that are already owned by the wallet. The fact that they get realized when making this 0.001BTC payment is no better/worse than if they were realized when making a subsequent payment for 100BTC, or if they were used to make a subsequent payment of 0.00001 BTC.

    The only way this could be improved is if our coin selection algorithm favored using relatively few coins when the prevailing fee rate is high, and favored using relatively many coins when the prevailing fee rate is low. I don’t believe we have such logic in our wallet.

    fjahr commented at 0:14 am on May 18, 2021:

    (typed this before github showed johns anwer so there is some duplication here I think) It’s not great in theory. But I think we can expect that users know what they are getting into here. The users have reused addresses a lot (willingly or unwillingly) and realized that this is a threat to their privacy. Then they have opted in to avoid reuse which warns users that this can create higher fees via high numbers of inputs. And there isn’t even much of a risk here if somebody sets this option just for the fun of it. As long as there is no address reuse in that wallet, avoid reuse also won’t kick in.

    Because this is a privacy feature that users have opted into it seems the better trade-off to me that someone is surprised by a higher fee rather than being surprised that they could only consolidate 10 outputs in one transaction when they had 30 outputs in that destination. And then they have to further watch out to no compromise themselves when consolidating the remaining 20 outputs. What users are getting aside from privacy protection is a consolidation transaction. While the fees might be high on this first tx it will hopefully also create a change output that is larger and will be much cheaper to spend later on.

    maxapsfee is only relevant if avoid partial spends is not activated. If avoid partial spends is not set, the wallet will still try to avoid partial spends and prefer to use those coins that avoid partial spends of those that don’t if the premium is below the threshold.

    glozow commented at 11:03 am on May 19, 2021:

    Ahh thanks for explaining this. I can see how a user turning -avoidpartialspends on would be consciously choosing to opt for privacy over lower short-term fees.

    maxapsfee is only relevant if avoid partial spends is not activated. If avoid partial spends is not set, the wallet will still try to avoid partial spends and prefer to use those coins that avoid partial spends of those that don’t if the premium is below the threshold.

    Ohh my bad, thanks for being patient with me!

  50. in src/wallet/rpcwallet.cpp:449 in 241e14162f outdated 
    445@@ -446,7 +446,7 @@ static RPCHelpMan sendtoaddress()
446                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
447             "       \"" + FeeModes("\"\n\"") + "\""},
448                     {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{true}, "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n"
449-                                         "dirty if they have previously been used in a transaction."},
450+                                         "dirty if they have previously been used in a transaction. If true, also activates avoidpartialspends."},
    jnewbery commented at 1:53 pm on May 17, 2021:
    I think this could be confusing for users if you don’t also describe what avoidpartialspends means.
    fjahr commented at 0:14 am on May 18, 2021:
    I added a very minimal description but I think more is not needed because we reference so many things in these docs (BIPs etc.) without explanation that I would expect users to look up what they don’t know in general.
    jnewbery commented at 8:58 am on May 19, 2021:
    Minimal description looks good. Thanks!
  51. wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 8f073076b1
  52. rpc: Improve avoidpartialspends and avoid_reuse documentation e6fe1c37d0
  53. fjahr force-pushed on May 18, 2021
  54. in src/wallet/rpcwallet.cpp:449 in e6fe1c37d0 
    445@@ -446,7 +446,7 @@ static RPCHelpMan sendtoaddress()
446                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
447             "       \"" + FeeModes("\"\n\"") + "\""},
448                     {"avoid_reuse", RPCArg::Type::BOOL, RPCArg::Default{true}, "(only available if avoid_reuse wallet flag is set) Avoid spending from dirty addresses; addresses are considered\n"
449-                                         "dirty if they have previously been used in a transaction."},
450+                                         "dirty if they have previously been used in a transaction. If true, this also activates avoidpartialspends, grouping outputs by their addresses."},
    unknown commented at 7:57 pm on May 18, 2021:
    nit: Can mention avoidpartialspends in single quotes or a complete sentence to describe what it does: If true, it will avoid partial spends and group unspent outputs by address.
  55. ghost commented at 8:37 pm on May 18, 2021: none

    Reading the comments about increasing OUTPUT_GROUP_MAX_ENTRIES from 10 to 100: https://bitcoincore.reviews/17824.html

    nit: I think it will be better if we have MIN, MAX and DEFAULT in such cases and allow user to set anything between MIN and MAX. For OUTPUT_GROUP_MAX_ENTRIES:

    OUTPUT_GROUP_DEFAULT_ENTRIES:10 OUTPUT_GROUP_MIN_ENTRIES: 10 OUTPUT_GROUP_MAX_ENTRIES: 100

    Mentioned it as ’nit’ because use of constants in Bitcoin Core is already discussed in few PRs: #21815 (comment) (4)

    Increasing it from 10 to 100 is not an issue, real issues are with coin selection however that is out of scope for this PR. Had one related question which was answered by Andrew Chow: https://bitcoin.stackexchange.com/q/106418/

    Other issue:

    Tx: aa6986b774b1a7bdf45c2d4df43161996fa4caf00725889fb91ca79a3bd8fd9c has 14 inputs out of which 9 are associated with one address: tb1qyqd9p9d2rc5a9v6cphucn999ljfcvdhymthypy

    avoidpartialspends=1 in bitcoin.conf and tried sending 0.051 BTC

    I am not sure why other inputs were used because sum of inputs associated with tb1qyqd9p9d2rc5a9v6cphucn999ljfcvdhymthypy = 0.004 + 0.0076 + 0.01 + 0.009 + 0.007 + 0.0045 + 0.0066 + 0.0055 + 0.005 = 0.0592 (Assume these are 90 instead of 9)

    Maybe because it will create a small change? But avoidpartialspends=1 is mentioned as an option that improves privacy. In this case user is associating himself with extra 5 addresses which could have been avoided in this transaction.

  56. jnewbery commented at 9:08 am on May 19, 2021: member
    ACK e6fe1c37d0
  57. michaelfolkson commented at 10:53 am on May 19, 2021: contributor
    @prayank23: Please try to focus on the PR in question. Additional features can be added in a separate PR e.g. user configurable max. The StackExchange post is interesting but I would argue not relevant to the intent of this PR. Relay connections are certainly not relevant.
  58. ghost commented at 11:04 am on May 19, 2021: none

    The StackExchange post is interesting but I would argue not relevant to the intent of this PR.

    To review this PR you need to test avoidpartialspends, it didn’t work as I expected it to be.

    Relay connections are certainly not relevant.

    Never mentioned anything about relay. The link of other PR is about use of different constants in Bitcoin Core. It is relevant because value of OUTPUT_GROUP_MAX_ENTRIES was 10 earlier and 100 after this PR with not enough reasons to justify those numbers which affects Bitcoin Core users unaware of these things.

  59. michaelfolkson commented at 11:16 am on May 19, 2021: contributor

    It is relevant because value of OUTPUT_GROUP_MAX_ENTRIES was 10 earlier and 100 after this PR with not enough reasons to justify those numbers which affects Bitcoin Core users unaware of these things.

    I agree with you that the rationale (upsides and downsides to this change) for increasing from 10 to 100 seems weakly explained/explored and hence it is hard to assess Concept ACK/NACK. As @Xekyo said in the PR review club:

    05:43 Regarding Q4, I find the 10 limit a bit arbitrary, but 100 is just as arbitrary.

    It seems we only have gut feel of wallet devs as a reason for Concept ACKing. Ideally we would have more than that but we can explore in the PR review club later.

    I think how constants are set generally (e.g. relay connections) are a much broader subject best not explored in the comments of this PR.

  60. glozow commented at 1:19 pm on May 19, 2021: member

    I agree with you that the rationale (upsides and downsides to this change) for increasing from 10 to 100 seems weakly explained/explored and hence it is hard to assess Concept ACK/NACK. @michaelfolkson I’ve had a think about this and my rough framework for the soundness of a OUTPUT_GROUP_MAX_ENTRIES is

    • high enough so that, if a user turned on avoid_reuse and that triggered -avoidpartialspends, transactions would probably sweep all outputs belonging to an address (because they’d be in a bit of a pickle otherwise).
    • low enough to not be a footgun (fee-wise) for users that turn on -avoidpartialspends for funsies.

    It seems to me that 10 is too low for this and weakens the utility of -avoidpartialspends. I found the graph in the PR description helpful in understanding why 100 might be reasonable.

    I think it will be better if we have MIN, MAX and DEFAULT in such cases and allow user to set anything between MIN and MAX. For OUTPUT_GROUP_MAX_ENTRIES @prayank An OUTPUT_GROUP_MIN_ENTRIES doesn’t make much sense to me. The default is just 1 if they don’t want to -avoidpartialspends. Perhaps it could be useful to let the user pick a maximum size if they want to limit how much they’re paying for the consolidation, but I think setting a maximum fee using -maxtxfee would capture their intent better.

  61. Xekyo commented at 3:03 pm on May 19, 2021: member
    OUTPUT_GROUP_MAX_ENTRIES is only relevant when users are privacy conscious enough to enable avoid partial spend, but then on the other hand also heavily reuse addresses. It seems unlikely that this constellation occurs so frequently that it would be worth adding and maintaining configuration options for it.
  62. Xekyo commented at 4:12 pm on May 19, 2021: member
    retACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
  63. rajarshimaitra commented at 9:22 am on May 21, 2021: contributor

    tACK e6fe1c3

    Verified the tests are checking against intended behaviour.

    No strong feeling over the value of OUTPUT_GROUP_MAX_ENTRIES. @glozow ’s rationale here #18418 (comment) makes sense to me.

    But in the end this is also as arbitrary as 10. Would be better if this rationale could be included in the PR description. Was a little hard to find reasoning behind why it was decided to change it.

  64. ghost commented at 1:22 pm on May 21, 2021: none

    OUTPUT_GROUP_MAX_ENTRIES is only relevant when users are privacy conscious enough to enable avoid partial spend, but then on the other hand also heavily reuse addresses. It seems unlikely that this constellation occurs so frequently

    I was thinking about possible examples in which a user has saved avoidpartialspends=1 in bitcoin.conf but still reusing addresses. Two examples that I could think of:

    1. User read about avoidpartialspends being good for privacy, saves it in bitcoin.conf assuming it will manage everything for the user, forgets about reuse after sometime being careless or receives donation to one of his address regularly or whitelisted one address on an exchange and use it regularly for withdrawals or one bitcoin address used for payouts in some website/app or some other mistake.
    2. Forced address reuse attack: https://en.bitcoin.it/wiki/Privacy#Forced_address_reuse

    In both cases its difficult to know if 100 will be good enough for all the users. Maybe it works for most of them.

    Perhaps it could be useful to let the user pick a maximum size if they want to limit how much they’re paying for the consolidation, but I think setting a maximum fee using -maxtxfee would capture their intent better.

    Agree. It will be useful if users could decide this and save in bitcoin.conf. Using maxtxfee is just a workaround which I had also suggested to one user who had issues with more inputs being selected by coin selection algorithm: #20598 (comment) which will abort the transaction.

    Since we had discussed about other wallets in PR review club meeting, I was curious if they use any such max limit:

    Electrum: if any coin is spent from a user address, all coins are.

    Wasabi: Treat same-scriptpubkey coins as one big coin. If group has an unconfirmed, then the whole group is unconfirmed.

    Joinmarket : To prevent forced address reuse attacks, (sometimes called dust attacks, but that’s a poor name), any deposit to an already-used address is freezed by default.

    Samourai: Not sure but I tested with 15 UTXOs for same address and they were all added as inputs for the transaction. I am assuming it is same as Electrum or Wasabi.

  65. achow101 commented at 4:54 am on May 25, 2021: member
    ACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
  66. ghost commented at 6:05 am on May 25, 2021: none

    Concept NACK

    • It should not just be 100 but ‘all` same-scriptpubkey coins
    • If there needs to be value for it, let the users decide with some option in bitcoin.conf

    Reason: Privacy. There are few other related issues as well which are mentioned in https://github.com/bitcoin/bitcoin/issues/22018

  67. michaelfolkson commented at 12:26 pm on May 25, 2021: contributor

    @prayank23: I suspect that increasing OUTPUT_GROUP_MAX_ENTRIES to unlimited (no max) would get some NACKs based on the discussions at the PR review clubs and @glozow’s “low enough to not be a footgun (fee-wise) for users that turn on -avoidpartialspends”

    And an option in bitcoin.conf would have greater scope and more complexity than this PR though anyone is free to open that PR before or after this PR is merged.

    Hence I think this PR is a choice between the status quo (10) or 100 rather than the two choices you outline as your preferences. And there seems to be consensus amongst the wallet devs that 100 is better than 10. Hence Concept ACK, Approach ACK on this PR.

    You have to set some defaults. You can’t have users needing to decide before they get started on a large number of variables where many of them will have no idea what they represent. Hence a new default of 100 with a possible future PR to make it an option seems the best path forward.

  68. ghost commented at 12:45 pm on May 25, 2021: none

    Hence I think this PR is a choice between the status quo (10) or 100 rather than the two choices you outline as your preferences. And there seems to be consensus amongst the wallet devs that 100 is better than 10.

    10 or 100, both are bad for privacy. Increasing it from 10 to 100 is worse if you consider attacker will be able to know more about the version of core used by victim.

    I suspect that increasing OUTPUT_GROUP_MAX_ENTRIES to unlimited (no max) would get some NACKs

    Then we should mention the privacy issues in docs, trade-offs and reasons for not improving privacy.

  69. michaelfolkson commented at 12:55 pm on May 25, 2021: contributor

    10 or 100, both are bad for privacy. Increasing it from 10 to 100 is worse if you consider attacker can now know more about the version of core used by victim.

    I’m pretty sure keeping the version of Core private is near the bottom of the privacy priority wishlist. Otherwise we would make no improvements and add no features to Core that leak the Core version being used. The general advice appears to be that users should run a recent version (if not the most recent version) of Core. That doesn’t leak anything regarding your transactions or your net worth.

    Then we should mention the privacy issues in docs, trade-offs and reasons for not improving privacy.

    I think everyone supports improving the docs.

    edit: This discussion should move to IRC and away from this PR I think. Your Concept NACK has been logged with a rationale, other reviewers can assess whether they agree with it or not.

  71. fanquake merged this on May 26, 2021
  72. fanquake closed this on May 26, 2021
  73. fanquake commented at 11:33 am on May 26, 2021: member
    cc @meshcollider @instagibbs
  74. sidhujag referenced this in commit 0faef2b905 on May 27, 2021
  75. gwillen referenced this in commit 5f8081db1a on Jun 1, 2022
  76. DrahtBot locked this on Aug 16, 2022


fjahr jonatack DrahtBot Angelika1920 meshcollider Xekyo glozow mzumsande jnewbery ghost michaelfolkson rajarshimaitra achow101 fanquake

Labels
Wallet

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-07-01 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me