Merge bitcoin/bitcoin#18418: wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100

e6fe1c37d0a2f8037996dd80619d6c23ec028729 rpc: Improve avoidpartialspends and avoid_reuse documentation (Fabian Jahr)
8f073076b102b77897e5a025ae555baae3d1f671 wallet: Increase OUTPUT_GROUP_MAX_ENTRIES to 100 (Fabian Jahr)

Pull request description:

  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](https://bitcoincore.reviews/17824.html#l-339) 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](https://miro.medium.com/max/3628/1*sZ5eaBSbsJsHx-J9iztq2g.png)([source](https://medium.com/@hasufly/an-analysis-of-batching-in-bitcoin-9bdf81a394e0)) giving these transactions a bit of a larger anonymity set, although that is probably a very weak argument.

ACKs for top commit:
  jnewbery:
    ACK e6fe1c37d0
  Xekyo:
    retACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
  rajarshimaitra:
    tACK `e6fe1c3`
  achow101:
    ACK e6fe1c37d0a2f8037996dd80619d6c23ec028729
  glozow:
    code review ACK https://github.com/bitcoin/bitcoin/pull/18418/commits/e6fe1c37d0a2f8037996dd80619d6c23ec028729

Tree-SHA512: 79685c58bafa64ed8303b0ecd616fce50fc9a2b758aa79833e4ad9f15760e09ab60c007bc16ab4cbc4222e644cfd154f1fa494b0f3a5d86faede7af33a6f2826

4 files changed, 27 insertions, 27 deletions

diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 632aae87c9..eb0d6316c0 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -43,7 +43,7 @@ const WalletInitInterface& g_wallet_init_interface = WalletInit();
 void WalletInit::AddWalletOptions(ArgsManager& argsman) const
 {
     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);
-    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);
+    argsman.AddArg("-avoidpartialspends", strprintf("Group outputs by address, selecting many (possibly all) or none, instead of selecting on a per-output basis. Privacy is improved as addresses 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);
     argsman.AddArg("-changetype", "What type of change to use (\"legacy\", \"p2sh-segwit\", or \"bech32\"). Default is same as -addresstype, except when -addresstype=p2sh-segwit a native segwit output is used when sending to a native segwit address)", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg("-disablewallet", "Do not load the wallet and disable wallet RPC calls", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg("-discardfee=<amt>", strprintf("The fee rate (in %s/kvB) that indicates your tolerance for discarding change by adding it to the fee (default: %s). "
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index a2124cc05e..f270e1ad05 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -447,7 +447,7 @@ static RPCHelpMan sendtoaddress()
                     {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
             "       \"" + FeeModes("\"\n\"") + "\""},
                     {"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"
-                                         "dirty if they have previously been used in a transaction."},
+                                         "dirty if they have previously been used in a transaction. If true, this also activates avoidpartialspends, grouping outputs by their addresses."},
                     {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
                     {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "If true, return extra information about the transaction."},
                 },
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6c97a48561..1bb2438d88 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -53,7 +53,7 @@ const std::map<uint64_t,std::string> WALLET_FLAG_CAVEATS{
     },
 };
 
-static const size_t OUTPUT_GROUP_MAX_ENTRIES = 10;
+static constexpr size_t OUTPUT_GROUP_MAX_ENTRIES{100};
 
 RecursiveMutex cs_wallets;
 static std::vector<std::shared_ptr<CWallet>> vpwallets GUARDED_BY(cs_wallets);
@@ -2483,7 +2483,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
     // form groups from remaining coins; note that preset coins will not
     // automatically have their associated (same address) coins included
     if (coin_control.m_avoid_partial_spends && vCoins.size() > OUTPUT_GROUP_MAX_ENTRIES) {
-        // Cases where we have 11+ outputs all pointing to the same destination may result in
+        // Cases where we have 101+ outputs all pointing to the same destination may result in
         // privacy leaks as they will potentially be deterministically sorted. We solve that by
         // explicitly shuffling the outputs before processing
         Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext());
diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py
index 1d3736d9b1..c13d8de4b5 100755
--- a/test/functional/wallet_avoidreuse.py
+++ b/test/functional/wallet_avoidreuse.py
@@ -42,25 +42,25 @@ def count_unspent(node):
     r["reused"]["supported"] = supports_reused
     return r
 
-def assert_unspent(node, total_count=None, total_sum=None, reused_supported=None, reused_count=None, reused_sum=None):
+def assert_unspent(node, total_count=None, total_sum=None, reused_supported=None, reused_count=None, reused_sum=None, margin=0.001):
     '''Make assertions about a node's unspent output statistics'''
     stats = count_unspent(node)
     if total_count is not None:
         assert_equal(stats["total"]["count"], total_count)
     if total_sum is not None:
-        assert_approx(stats["total"]["sum"], total_sum, 0.001)
+        assert_approx(stats["total"]["sum"], total_sum, margin)
     if reused_supported is not None:
         assert_equal(stats["reused"]["supported"], reused_supported)
     if reused_count is not None:
         assert_equal(stats["reused"]["count"], reused_count)
     if reused_sum is not None:
-        assert_approx(stats["reused"]["sum"], reused_sum, 0.001)
+        assert_approx(stats["reused"]["sum"], reused_sum, margin)
 
-def assert_balances(node, mine):
+def assert_balances(node, mine, margin=0.001):
     '''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)
+        assert_approx(got[k], v, margin)
 
 class AvoidReuseTest(BitcoinTestFramework):
 
@@ -299,7 +299,7 @@ class AvoidReuseTest(BitcoinTestFramework):
         ret_addr = self.nodes[0].getnewaddress()
 
         # send multiple transactions, reusing one address
-        for _ in range(11):
+        for _ in range(101):
             self.nodes[0].sendtoaddress(new_addr, 1)
 
         self.nodes[0].generate(1)
@@ -311,14 +311,14 @@ class AvoidReuseTest(BitcoinTestFramework):
 
         # getbalances and listunspent should show the remaining outputs
         # in the reused address as used/reused
-        assert_unspent(self.nodes[1], total_count=2, total_sum=6, reused_count=1, reused_sum=1)
-        assert_balances(self.nodes[1], mine={"used": 1, "trusted": 5})
+        assert_unspent(self.nodes[1], total_count=2, total_sum=96, reused_count=1, reused_sum=1, margin=0.01)
+        assert_balances(self.nodes[1], mine={"used": 1, "trusted": 95}, margin=0.01)
 
     def test_full_destination_group_is_preferred(self):
         '''
-        Test the case where [1] only has 11 outputs of 1 BTC in the same reused
+        Test the case where [1] only has 101 outputs of 1 BTC in the same reused
         address and tries to send a small payment of 0.5 BTC. The wallet
-        should use 10 outputs from the reused address as inputs and not a
+        should use 100 outputs from the reused address as inputs and not a
         single 1 BTC input, in order to join several outputs from the reused
         address.
         '''
@@ -330,8 +330,8 @@ class AvoidReuseTest(BitcoinTestFramework):
         new_addr = self.nodes[1].getnewaddress()
         ret_addr = self.nodes[0].getnewaddress()
 
-        # Send 11 outputs of 1 BTC to the same, reused address in the wallet
-        for _ in range(11):
+        # Send 101 outputs of 1 BTC to the same, reused address in the wallet
+        for _ in range(101):
             self.nodes[0].sendtoaddress(new_addr, 1)
 
         self.nodes[0].generate(1)
@@ -342,14 +342,14 @@ class AvoidReuseTest(BitcoinTestFramework):
         txid = self.nodes[1].sendtoaddress(address=ret_addr, amount=0.5)
         inputs = self.nodes[1].getrawtransaction(txid, 1)["vin"]
 
-        # The transaction should use 10 inputs exactly
-        assert_equal(len(inputs), 10)
+        # The transaction should use 100 inputs exactly
+        assert_equal(len(inputs), 100)
 
     def test_all_destination_groups_are_used(self):
         '''
-        Test the case where [1] only has 22 outputs of 1 BTC in the same reused
-        address and tries to send a payment of 20.5 BTC. The wallet
-        should use all 22 outputs from the reused address as inputs.
+        Test the case where [1] only has 202 outputs of 1 BTC in the same reused
+        address and tries to send a payment of 200.5 BTC. The wallet
+        should use all 202 outputs from the reused address as inputs.
         '''
         self.log.info("Test that all destination groups are used")
 
@@ -359,20 +359,20 @@ class AvoidReuseTest(BitcoinTestFramework):
         new_addr = self.nodes[1].getnewaddress()
         ret_addr = self.nodes[0].getnewaddress()
 
-        # Send 22 outputs of 1 BTC to the same, reused address in the wallet
-        for _ in range(22):
+        # Send 202 outputs of 1 BTC to the same, reused address in the wallet
+        for _ in range(202):
             self.nodes[0].sendtoaddress(new_addr, 1)
 
         self.nodes[0].generate(1)
         self.sync_all()
 
         # Sending a transaction that needs to use the full groups
-        # of 10 inputs but also the incomplete group of 2 inputs.
-        txid = self.nodes[1].sendtoaddress(address=ret_addr, amount=20.5)
+        # of 100 inputs but also the incomplete group of 2 inputs.
+        txid = self.nodes[1].sendtoaddress(address=ret_addr, amount=200.5)
         inputs = self.nodes[1].getrawtransaction(txid, 1)["vin"]
 
-        # The transaction should use 22 inputs exactly
-        assert_equal(len(inputs), 22)
+        # The transaction should use 202 inputs exactly
+        assert_equal(len(inputs), 202)
 
 
 if __name__ == '__main__':