wallet: always do avoid partial spends if fees are within a specified range #14582

3862 | @@ -3823,6 +3863,21 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3863 |          walletInstance->m_min_fee = CFeeRate(n);
3864 |      }
3865 |  
3866 | +    if (gArgs.IsArgSet("-maxapsfee")) {
3867 | +        CAmount n = 0;
3868 | +        if (gArgs.GetArg("-maxapsfee", "") == "-1") {

fjahr commented at 3:01 PM on August 3, 2020:

I can't really think of a good reason why users would want to deactivate this instead of keeping it at 0. Maybe we don't need this option?

kallewoof commented at 7:15 AM on August 5, 2020:

If someone has a really big wallet with tons of UTXO:s, they may want to disable it as it effectively does coin selection twice every time, doubling the time. I don't think there are any other reasons, though.

  46 | @@ -47,6 +47,7 @@ void WalletInit::AddWalletOptions() const
  47 |      gArgs.AddArg("-fallbackfee=<amt>", strprintf("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data. 0 to entirely disable the fallbackfee feature. (default: %s)",
  48 |                                                                 CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
  49 |      gArgs.AddArg("-keypool=<n>", strprintf("Set key pool size to <n> (default: %u). Warning: Smaller sizes may increase the risk of losing funds when restoring from an old backup, if none of the addresses in the original keypool have been used.", DEFAULT_KEYPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
  50 | +    gArgs.AddArg("-maxapsfee=<n>", strprintf("Spend up to this amount in (absolute) fees (in %s) if it means prioritizing partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

fjahr commented at 3:07 PM on August 3, 2020:

As a wallet option maybe a percentage would be better? Personally I think I would prefer to be able to set a max % increase in the feerate for this option. An absolute value seems brittle in times of fluctuating fees and users might forget about updating it.

fjahr commented at 4:19 PM on August 3, 2020:

The text here confused me at first but the way I see it works in the code and this refers to additional fees makes more sense and this is probably equally usable as a percentage for users.

    gArgs.AddArg("-maxapsfee=<n>", strprintf("Spend up to this amount in additional (absolute) fees (in %s) if it allows to use partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);

kallewoof commented at 12:55 AM on August 6, 2020:

Thanks, tweaked a little.

3017 | +        const CCoinControl& coin_control,
3018 | +        bool sign)
3019 | +{
3020 | +    CAmount maxapsfee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);
3021 | +    int nChangePosIn = nChangePosInOut;
3022 | +    CTransactionRef tx2 = tx;

fjahr commented at 3:16 PM on August 3, 2020:

nit:

    const CAmount max_aps_fee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);
    const int change_pos_in = nChangePosInOut;
    CTransactionRef tx_aps = tx;

kallewoof commented at 1:00 AM on August 6, 2020:

Leaving most of this as is (first line is gone, 2nd is a primitive and the name is inherited, 3rd ~I did change the name~ I left name as is to match with the others).

3025 | +    if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && maxapsfee > -1 && !coin_control.m_avoid_partial_spends) {
3026 | +        CCoinControl tmp_cc = coin_control;
3027 | +        tmp_cc.m_avoid_partial_spends = true;
3028 | +        CAmount nFeeRet2;
3029 | +        int nChangePosInOut2 = nChangePosIn;
3030 | +        bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results

fjahr commented at 3:18 PM on August 3, 2020:

nit:

        CAmount fee_ret_aps;
        int change_pos_in_out_aps = nChangePosIn;
        bilingual_str error_aps; // fired and forgotten; if an error occurs, we discard the results

kallewoof commented at 1:02 AM on August 6, 2020:

Keeping these as is, as their names are inherited.

3015 | +        int& nChangePosInOut,
3016 | +        bilingual_str& error,
3017 | +        const CCoinControl& coin_control,
3018 | +        bool sign)
3019 | +{
3020 | +    CAmount maxapsfee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);

fjahr commented at 8:19 PM on August 3, 2020:

can use m_max_aps_fee

3020 | +    CAmount maxapsfee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);
3021 | +    int nChangePosIn = nChangePosInOut;
3022 | +    CTransactionRef tx2 = tx;
3023 | +    bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, sign);
3024 | +    // try with avoidpartialspends unless it's enabled already
3025 | +    if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && maxapsfee > -1 && !coin_control.m_avoid_partial_spends) {

fjahr commented at 8:19 PM on August 3, 2020:

    if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {

3029 | +        int nChangePosInOut2 = nChangePosIn;
3030 | +        bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results
3031 | +        if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, sign)) {
3032 | +            // if fee of this alternative one is within the range of the max fee, we use this one
3033 | +            WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, nFeeRet2 <= nFeeRet ? "grouped" : "non-grouped");
3034 | +            if (nFeeRet2 <= nFeeRet + maxapsfee) {

fjahr commented at 8:22 PM on August 3, 2020:

The ternary in the logging does not match the if below. I think it should work as it does in the if.

            const bool use_aps = nFeeRet2 <= nFeeRet + m_max_aps_fee;
            WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped");
            if (use_aps) {

kallewoof commented at 1:05 AM on August 6, 2020:

Nice bug.. thanks!

  63 | @@ -64,6 +64,52 @@ def run_test(self):
  64 |          assert_approx(v[0], 0.2)
  65 |          assert_approx(v[1], 1.3, 0.0001)
  66 |  
  67 | +        # Test 'avoid partial if warranted, even if disabled'

achow101 commented at 7:30 PM on August 14, 2020:

nit: use self.log.info instead of a comment.

kallewoof commented at 4:42 AM on August 15, 2020:

Sure, will tweak if I end up needing to rebase.

3917 | @@ -3878,6 +3918,21 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3918 |          walletInstance->m_min_fee = CFeeRate(n);
3919 |      }
3920 |  
3921 | +    if (gArgs.IsArgSet("-maxapsfee")) {
3922 | +        CAmount n = 0;
3923 | +        if (gArgs.GetArg("-maxapsfee", "") == "-1") {
3924 | +            n = -1;
3925 | +        } else if (!ParseMoney(gArgs.GetArg("-maxapsfee", ""), n)) {
3926 | +            error = AmountErrMsg("maxapsfee", gArgs.GetArg("-maxapsfee", ""));

jonatack commented at 12:42 PM on August 16, 2020:

nit if you retouch, could memoize for readability and clarity

     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;
         }

 101 | +        # Test wallet option maxapsfee with Node 3
 102 | +        addr_aps = self.nodes[3].getnewaddress()
 103 | +        self.nodes[0].sendtoaddress(addr_aps, 1.0)
 104 | +        self.nodes[0].sendtoaddress(addr_aps, 1.0)
 105 | +        self.nodes[0].generate(1)
 106 | +        txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)

jonatack commented at 1:17 PM on August 16, 2020:

suggestion: test the new logging

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

  89 | +        # the accumulated value should be 1.5, so the outputs should be
  90 | +        # ~0.1 and 1.4 and should come from the same destination
  91 | +        values = [vout["value"] for vout in tx3["vout"]]
  92 | +        values.sort()
  93 | +        assert_approx(values[0], 0.1, 0.0001)
  94 | +        assert_approx(values[1], 1.4)

jonatack commented at 1:21 PM on August 16, 2020:

nit: missing vspan arg here (iiuc it's the same value as the others, e.g. the default, but since it looks like the intention here is to make the vspans explicit, the missing one may have been an oversight)

can optionally use named args for the assert_approx calls when more than 1-2 args are passed:

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

3926 | +            error = AmountErrMsg("maxapsfee", gArgs.GetArg("-maxapsfee", ""));
3927 | +            return nullptr;
3928 | +        }
3929 | +        if (n > HIGH_APS_FEE) {
3930 | +            warnings.push_back(AmountHighWarn("-maxapsfee") + Untranslated(" ") +
3931 | +                              _("This is the maximum transaction fee you pay to prioritize partial spend avoidance over regular coin selection."));

meshcollider commented at 3:46 AM on August 17, 2020:

I think this should emphasize that the maxapsfee is in addition to the normal fee

 104 | +        self.nodes[0].sendtoaddress(addr_aps, 1.0)
 105 | +        self.nodes[0].generate(1)
 106 | +        txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
 107 | +        tx4 = self.nodes[3].getrawtransaction(txid4, True)
 108 | +        # tx4 should have 2 inputs and 2 outputs although one output would
 109 | +        # have been enough and the transaction caused higher fees

meshcollider commented at 3:54 AM on August 17, 2020:

It would be good to also test here that the higher fee is still capped by the parameter provided