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

kallewoof commented at 6:45 am on October 26, 2018: member

The -avoidpartialspends feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter -maxapsfee (max avoid partial spends fee) which acts on the following values:

-1: disable partial spend avoidance completely (do not even try it)
0: only do partial spend avoidance if fees are the same or better as the regular coin selection
1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee

For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.

Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.

Edit: updated this to reflect the fact we are now using a max fee.

kallewoof force-pushed on Oct 26, 2018

fanquake added the label Wallet on Oct 26, 2018

fanquake deleted a comment on Oct 26, 2018

DrahtBot commented at 11:52 pm on November 1, 2018: 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:

#19501 (send* RPCs in the wallet returns the “fee reason” by stackman27)
#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#17484 (wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload by ariard)

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.

meshcollider commented at 12:57 pm on November 9, 2018: contributor

Don’t the existing coin selection algorithms take into account the case where having no change output would reduce the fee? Wouldn’t that have the same effect already? Or am I misunderstanding what this does / the current behaviour

DrahtBot added the label Needs rebase on Nov 9, 2018

kallewoof commented at 4:37 am on November 12, 2018: member

@MeshCollider

Don’t the existing coin selection algorithms take into account the case where having no change output would reduce the fee? Wouldn’t that have the same effect already? Or am I misunderstanding what this does / the current behaviour

The wallet will not use output grouping unless the user turns on -avoidpartialspends. The biggest reason is that output grouping will, in certain cases, result in higher fees because certain outputs will be bound together. This patch will try output grouping and use it if the fees do not increase, if users have turned -avoidpartialspends off.

kallewoof force-pushed on Nov 12, 2018

DrahtBot removed the label Needs rebase on Nov 12, 2018

DrahtBot added the label Needs rebase on Jul 10, 2019

kallewoof force-pushed on Jul 10, 2019

DrahtBot removed the label Needs rebase on Jul 10, 2019

kallewoof force-pushed on Jul 11, 2019

kallewoof force-pushed on Jul 18, 2019

DrahtBot removed the label Needs rebase on Jul 18, 2019

DrahtBot added the label Needs rebase on Oct 24, 2019

kallewoof force-pushed on Dec 24, 2019

DrahtBot removed the label Needs rebase on Dec 24, 2019

kallewoof force-pushed on Jan 3, 2020

kallewoof force-pushed on Jan 8, 2020

gmaxwell commented at 11:14 am on January 8, 2020: contributor

I’m super happy to see work on this!

kallewoof commented at 12:09 pm on January 8, 2020: member

@gmaxwell Glad to hear it! I actually wrote code to turn this into a variable fee (see last commit), but was hesitant whether to push it now or wait. Your comment made me decide to push now. :)

kallewoof force-pushed on Jan 8, 2020

kallewoof renamed this:
~~wallet: try -avoidpartialspends mode and use its result if fees do not change~~
wallet: always do avoid partial spends if fees are within a specified range
on Jan 8, 2020

kallewoof force-pushed on Jan 8, 2020

kallewoof force-pushed on Jan 9, 2020

DrahtBot added the label Needs rebase on Jan 29, 2020

kallewoof force-pushed on Jan 30, 2020

DrahtBot removed the label Needs rebase on Jan 30, 2020

DrahtBot added the label Needs rebase on May 1, 2020

kallewoof force-pushed on May 2, 2020

DrahtBot removed the label Needs rebase on May 2, 2020

DrahtBot added the label Needs rebase on May 4, 2020

kallewoof force-pushed on May 7, 2020

DrahtBot removed the label Needs rebase on May 7, 2020

kallewoof force-pushed on May 7, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

kallewoof commented at 2:59 am on July 31, 2020: member

I’ve been rebasing this for 8 months now, with no progress. Taking this as lack of interest and closing. Ping me if this is ever interesting and I’ll reopen / rebase.

kallewoof closed this on Jul 31, 2020

jonatack commented at 2:39 pm on July 31, 2020: member

This looks interesting. I was unaware of this work, as it was before I began following this repo closely. Pinging @fjahr for thoughts.

(Don’t hesitate to occasionally put out a review beg on IRC for work like this.)

fjahr commented at 11:51 am on August 1, 2020: member

Thanks for the ping @jonatack . I was unaware of this as well and also think this is interesting. @kallewoof how about a smaller scoped PR that adds the behavior of -maxapsfee=0 or another sensible default without adding the parameter itself? I think some developers have parameter fatigue and fear if a parameter implication is hard to understand for users then it will not be used. I also think this may not be used much because if users understand the implications and want that behavior they will probably just use -avoidpartialspends anyway. It would make this PR smaller and easier to review as well.

kallewoof commented at 3:41 am on August 3, 2020: member

@jonatack @fjahr Thanks for shown interest, reopening.

This initially was actually smaller scope, but it didn’t require a lot of changes to make it fully featured, so I went ahead and did it. I am open to trimming this down, but I don’t think the size was the reason for lack of reviews, more my reluctancy to review beg (and timezones). :)

kallewoof reopened this on Aug 3, 2020

sipa commented at 4:07 am on August 3, 2020: member

Concept ACK

in src/wallet/wallet.cpp:3923 in dd634f2470 outdated

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.

in src/wallet/init.cpp:50 in dd634f2470 outdated

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.

0    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 0:55 am on August 6, 2020:

Thanks, tweaked a little.

in src/wallet/wallet.cpp:3081 in dd634f2470 outdated

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:

0    const CAmount max_aps_fee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);
1    const int change_pos_in = nChangePosInOut;
2    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).

in src/wallet/wallet.cpp:3089 in dd634f2470 outdated

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:

0        CAmount fee_ret_aps;
1        int change_pos_in_out_aps = nChangePosIn;
2        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.

in src/wallet/wallet.cpp:3020 in dd634f2470 outdated

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

in src/wallet/wallet.cpp:3025 in dd634f2470 outdated

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:

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

in src/wallet/wallet.cpp:3034 in dd634f2470 outdated

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.

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

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

Nice bug.. thanks!

fjahr commented at 8:41 pm on August 3, 2020: member

Concept ACK

Some suggestions, feel free to ignore the renamings. And I have expanded the test here: https://github.com/fjahr/bitcoin/commit/aafa6c38a94ae4df3b7af5f3c8c7255024498b0b#diff-c2a150736b92621b8db2d91b546fbf85. Please take a look and feel free use.

wallet: try -avoidpartialspends mode and use its result if fees are below threshold

The threshold is defined by a new max avoid partial spends fee flag, which defaults to 0 (i.e. if fees are unchanged, use the grouped option).

b82067bf69

kallewoof force-pushed on Aug 6, 2020

test: test the implicit avoid partial spends functionality

Co-authored-by: Fabian Jahr <fjahr@protonmail.com>

7f13dfb587

kallewoof force-pushed on Aug 6, 2020

kallewoof commented at 1:26 am on August 6, 2020: member

Thanks for review, @fjahr! I believe I’ve addressed all your points & added your test code.

DrahtBot removed the label Needs rebase on Aug 6, 2020

fjahr approved

fjahr commented at 3:44 pm on August 8, 2020: member

tested ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9

in test/functional/wallet_groups.py:67 in 7f13dfb587

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.

achow101 commented at 7:33 pm on August 14, 2020: member

ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9

fanquake requested review from meshcollider on Aug 15, 2020

fanquake commented at 0:32 am on August 15, 2020: member

@Xekyo you might also be interested in this?

jonatack commented at 4:55 am on August 15, 2020: member

Concept ACK

in src/wallet/wallet.cpp:3926 in 7f13dfb587

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

 0     if (gArgs.IsArgSet("-maxapsfee")) {
 1+        const std::string max_aps_fee{gArgs.GetArg("-maxapsfee", "")};
 2         CAmount n = 0;
 3-        if (gArgs.GetArg("-maxapsfee", "") == "-1") {
 4+        if (max_aps_fee == "-1") {
 5             n = -1;
 6-        } else if (!ParseMoney(gArgs.GetArg("-maxapsfee", ""), n)) {
 7-            error = AmountErrMsg("maxapsfee", gArgs.GetArg("-maxapsfee", ""));
 8+        } else if (!ParseMoney(max_aps_fee, n)) {
 9+            error = AmountErrMsg("maxapsfee", max_aps_fee);
10             return nullptr;
11         }

in test/functional/wallet_groups.py:106 in 7f13dfb587

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

0-        txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)
1+        with self.nodes[3].assert_debug_log(['Fee non-grouped = 2820, grouped = 4160, using grouped']):
2+            txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)

in test/functional/wallet_groups.py:94 in 7f13dfb587

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:

0-        assert_approx(values[0], 0.1, 0.0001)
1-        assert_approx(values[1], 1.4)
2+        assert_approx(values[0], vexp=0.1, vspan=0.0001)
3+        assert_approx(values[1], vexp=1.4, vspan=0.0001)

jonatack commented at 1:27 pm on August 16, 2020: member

ACK 7f13dfb58, code review, debug build, verified the test fails with AssertionError: not(2 == 1) for the number of vouts when -maxapsfee=0.0001 is changed to 0, and verified the new logging with an added assertion.

Some feedback below that can be ignored unless need to retouch, or for a follow-up. The added tests are good. I have some ideas on increasing the coverage a bit further.

jonatack commented at 3:48 pm on August 16, 2020: member

As a follow-up, will need a release note.

meshcollider added the label Needs release note on Aug 17, 2020

in src/wallet/wallet.cpp:3931 in 7f13dfb587

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

in test/functional/wallet_groups.py:109 in 7f13dfb587

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

meshcollider commented at 3:55 am on August 17, 2020: contributor

Code review ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9

Neither of my comments are critical and could be done in a followup

meshcollider merged this on Aug 17, 2020

meshcollider closed this on Aug 17, 2020

kallewoof deleted the branch on Aug 17, 2020

sidhujag referenced this in commit fb6af86609 on Aug 17, 2020

MarcoFalke removed the label Needs release note on Aug 17, 2020

meshcollider referenced this in commit a2a250c7d0 on Aug 18, 2020

jasonbcox referenced this in commit 81fe776b23 on Oct 8, 2020

DrahtBot locked this on Oct 31, 2022

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

Conflicts