wallet: Prefer full destination groups in coin selection

When a wallet uses avoid_reuse and has a large number of outputs in
a single destination, it groups these outputs in OutputGroups that
are no larger than OUTPUT_GROUP_MAX_ENTRIES. The goal is to spend
as many outputs as possible from the destination while not breaking
consensus due to a huge number of inputs and also not surprise the
use with high fees. If there are n outputs in a destination and
n > OUTPUT_GROUP_MAX_ENTRIES then this results in one or many groups
of size OUTPUT_GROUP_MAX_ENTRIES and possibly one group of size
< OUTPUT_GROUP_MAX_ENTRIES.

Prior to this commit the coin selection in the case where
n > OUTPUT_GROUP_MAX_ENTRIES was skewed towards the one group of
size < OUTPUT_GROUP_MAX_ENTRIES if it exists and the amount to be
spent by the transaction is smaller than the aggregate of those
of the group size < OUTPUT_GROUP_MAX_ENTRIES. The reason is that
the coin selection decides between the different groups based on
fees and mostly the smaller group will cause smaller fees.

The behavior that users of the avoid_reuse flag seek is that the
full groups of size OUTPUT_GROUP_MAX_ENTRIES get used first. This
commit implements this by pretending that the small group has
a large number of ancestors (one smallet than the maximum allowed
for this wallet). This dumps the small group to the bottom of the
list of priorities in the coin selection algorithm.

1 files changed, 65 insertions, 0 deletions

diff --git a/test/functional/wallet_avoidreuse.py b/test/functional/wallet_avoidreuse.py
index 2ce8d459c6..42a8c923ef 100755
--- a/test/functional/wallet_avoidreuse.py
+++ b/test/functional/wallet_avoidreuse.py
@@ -94,6 +94,10 @@ class AvoidReuseTest(BitcoinTestFramework):
         self.test_fund_send_fund_send("bech32")
         reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
         self.test_getbalances_used()
+        reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
+        self.test_full_destination_group_is_preferred()
+        reset_balance(self.nodes[1], self.nodes[0].getnewaddress())
+        self.test_all_destination_groups_are_used()
 
     def test_persistence(self):
         '''Test that wallet files persist the avoid_reuse flag.'''
@@ -313,5 +317,66 @@ class AvoidReuseTest(BitcoinTestFramework):
         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})
 
+    def test_full_destination_group_is_preferred(self):
+        '''
+        Test the case where [1] only has 11 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
+        single 1 BTC input, in order to join several outputs from the reused
+        address.
+        '''
+        self.log.info("Test that full destination groups are preferred in coin selection")
+
+        # Node under test should be empty
+        assert_equal(self.nodes[1].getbalance(avoid_reuse=False), 0)
+
+        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):
+            self.nodes[0].sendtoaddress(new_addr, 1)
+
+        self.nodes[0].generate(1)
+        self.sync_all()
+
+        # Sending a transaction that is smaller than each one of the
+        # available outputs
+        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)
+
+    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.
+        '''
+        self.log.info("Test that all destination groups are used")
+
+        # Node under test should be empty
+        assert_equal(self.nodes[1].getbalance(avoid_reuse=False), 0)
+
+        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):
+            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)
+        inputs = self.nodes[1].getrawtransaction(txid, 1)["vin"]
+
+        # The transaction should use 22 inputs exactly
+        assert_equal(len(inputs), 22)
+
+
 if __name__ == '__main__':
     AvoidReuseTest().main()