Merge #19743: -maxapsfee follow-up

7e31ea9 -maxapsfee: follow-up fixes (Karl-Johan Alm)
9f77b82 doc: release notes for -maxapsfee (Karl-Johan Alm)

Pull request description:

  Addresses feedback from jonatack and meshcollider in #14582.

ACKs for top commit:
  jonatack:
    ACK 7e31ea9
  meshcollider:
    re-ACK 7e31ea9

Tree-SHA512: 5d159d78e917e41140e1394bef05e821430dbeac585e9bd708f897041dd7104c2a6b563bfab8b2c85e6d923441160a3880d7864d768aa8e0e66860e0a2c4f56b

Author: meshcollider

doc/release-notes-14582.md

@@ -0,0 +1,14 @@
+Configuration
+-------------
+
+A new configuration flag `-maxapsfee` has been added, which sets the max allowed
+avoid partial spends (APS) fee. It defaults to 0 (i.e. fee is the same with
+and without APS). Setting it to -1 will disable APS, unless `-avoidpartialspends`
+is set. (#14582)
+
+Wallet
+------
+
+The wallet will now avoid partial spends (APS) by default, if this does not result
+in a difference in fees compared to the non-APS variant. The allowed fee threshold
+can be adjusted using the new `-maxapsfee` configuration option. (#14582)

src/wallet/wallet.cpp

@@ -3890,16 +3890,17 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
     }
 
     if (gArgs.IsArgSet("-maxapsfee")) {
+        const std::string max_aps_fee{gArgs.GetArg("-maxapsfee", "")};
         CAmount n = 0;
-        if (gArgs.GetArg("-maxapsfee", "") == "-1") {
+        if (max_aps_fee == "-1") {
             n = -1;
-        } else if (!ParseMoney(gArgs.GetArg("-maxapsfee", ""), n)) {
-            error = AmountErrMsg("maxapsfee", gArgs.GetArg("-maxapsfee", ""));
+        } else if (!ParseMoney(max_aps_fee, n)) {
+            error = AmountErrMsg("maxapsfee", max_aps_fee);
             return nullptr;
         }
         if (n > HIGH_APS_FEE) {
             warnings.push_back(AmountHighWarn("-maxapsfee") + Untranslated(" ") +
-                              _("This is the maximum transaction fee you pay to prioritize partial spend avoidance over regular coin selection."));
+                              _("This is the maximum transaction fee you pay (in addition to the normal fee) to prioritize partial spend avoidance over regular coin selection."));
         }
         walletInstance->m_max_aps_fee = n;
     }

test/functional/wallet_groups.py

@@ -15,8 +15,14 @@
 class WalletGroupTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
-        self.num_nodes = 4
-        self.extra_args = [[], [], ['-avoidpartialspends'], ["-maxapsfee=0.0001"]]
+        self.num_nodes = 5
+        self.extra_args = [
+            [],
+            [],
+            ["-avoidpartialspends"],
+            ["-maxapsfee=0.00002719"],
+            ["-maxapsfee=0.00002720"],
+        ]
         self.rpc_timeout = 480
 
     def skip_test_if_missing_module(self):
@@ -50,8 +56,8 @@ def run_test(self):
         # one output should be 0.2, the other should be ~0.3
         v = [vout["value"] for vout in tx1["vout"]]
         v.sort()
-        assert_approx(v[0], 0.2)
-        assert_approx(v[1], 0.3, 0.0001)
+        assert_approx(v[0], vexp=0.2, vspan=0.0001)
+        assert_approx(v[1], vexp=0.3, vspan=0.0001)
 
         txid2 = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 0.2)
         tx2 = self.nodes[2].getrawtransaction(txid2, True)
@@ -61,8 +67,8 @@ def run_test(self):
         # one output should be 0.2, the other should be ~1.3
         v = [vout["value"] for vout in tx2["vout"]]
         v.sort()
-        assert_approx(v[0], 0.2)
-        assert_approx(v[1], 1.3, 0.0001)
+        assert_approx(v[0], vexp=0.2, vspan=0.0001)
+        assert_approx(v[1], vexp=1.3, vspan=0.0001)
 
         # Test 'avoid partial if warranted, even if disabled'
         self.sync_all()
@@ -75,8 +81,8 @@ def run_test(self):
         # - C0 1.0      - E1 0.5
         # - C1 0.5      - F  ~1.3
         # - D ~0.3
-        assert_approx(self.nodes[1].getbalance(), 4.3, 0.0001)
-        assert_approx(self.nodes[2].getbalance(), 4.3, 0.0001)
+        assert_approx(self.nodes[1].getbalance(), vexp=4.3, vspan=0.0001)
+        assert_approx(self.nodes[2].getbalance(), vexp=4.3, vspan=0.0001)
         # Sending 1.4 btc should pick one 1.0 + one more. For node #1,
         # this could be (A / B0 / C0) + (B1 / C1 / D). We ensure that it is
         # B0 + B1 or C0 + C1, because this avoids partial spends while not being
@@ -90,8 +96,8 @@ def run_test(self):
         # ~0.1 and 1.4 and should come from the same destination
         values = [vout["value"] for vout in tx3["vout"]]
         values.sort()
-        assert_approx(values[0], 0.1, 0.0001)
-        assert_approx(values[1], 1.4)
+        assert_approx(values[0], vexp=0.1, vspan=0.0001)
+        assert_approx(values[1], vexp=1.4, vspan=0.0001)
 
         input_txids = [vin["txid"] for vin in tx3["vin"]]
         input_addrs = [self.nodes[1].gettransaction(txid)['details'][0]['address'] for txid in input_txids]
@@ -104,13 +110,38 @@ def run_test(self):
         self.nodes[0].sendtoaddress(addr_aps, 1.0)
         self.nodes[0].generate(1)
         self.sync_all()
-        txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
+        with self.nodes[3].assert_debug_log(['Fee non-grouped = 2820, grouped = 4160, using grouped']):
+            txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
         tx4 = self.nodes[3].getrawtransaction(txid4, True)
         # tx4 should have 2 inputs and 2 outputs although one output would
         # have been enough and the transaction caused higher fees
         assert_equal(2, len(tx4["vin"]))
         assert_equal(2, len(tx4["vout"]))
 
+        addr_aps2 = self.nodes[3].getnewaddress()
+        [self.nodes[0].sendtoaddress(addr_aps2, 1.0) for _ in range(5)]
+        self.nodes[0].generate(1)
+        self.sync_all()
+        with self.nodes[3].assert_debug_log(['Fee non-grouped = 5520, grouped = 8240, using non-grouped']):
+            txid5 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
+        tx5 = self.nodes[3].getrawtransaction(txid5, True)
+        # tx5 should have 3 inputs (1.0, 1.0, 1.0) and 2 outputs
+        assert_equal(3, len(tx5["vin"]))
+        assert_equal(2, len(tx5["vout"]))
+
+        # Test wallet option maxapsfee with node 4, which sets maxapsfee
+        # 1 sat higher, crossing the threshold from non-grouped to grouped.
+        addr_aps3 = self.nodes[4].getnewaddress()
+        [self.nodes[0].sendtoaddress(addr_aps3, 1.0) for _ in range(5)]
+        self.nodes[0].generate(1)
+        self.sync_all()
+        with self.nodes[4].assert_debug_log(['Fee non-grouped = 5520, grouped = 8240, using grouped']):
+            txid6 = self.nodes[4].sendtoaddress(self.nodes[0].getnewaddress(), 2.95)
+        tx6 = self.nodes[4].getrawtransaction(txid6, True)
+        # tx6 should have 5 inputs and 2 outputs
+        assert_equal(5, len(tx6["vin"]))
+        assert_equal(2, len(tx6["vout"]))
+
         # Empty out node2's wallet
         self.nodes[2].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=self.nodes[2].getbalance(), subtractfeefromamount=True)
         self.sync_all()