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

kallewoof · 2018-10-26T06:45:04Z

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.

DrahtBot · 2018-11-01T23:52:31Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

send* RPCs in the wallet returns the "fee reason" #19501 (send* RPCs in the wallet returns the "fee reason" by stackman27)
Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #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 · 2018-11-09T12:57:30Z

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

kallewoof · 2018-11-12T04:37:41Z

@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.

gmaxwell · 2020-01-08T11:14:39Z

I'm super happy to see work on this!

kallewoof · 2020-01-08T12:09:52Z

@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. :)

sipa · 2020-08-03T04:07:30Z

Concept ACK

fjahr

Concept ACK

Some suggestions, feel free to ignore the renamings. And I have expanded the test here: fjahr@aafa6c3#diff-c2a150736b92621b8db2d91b546fbf85. Please take a look and feel free use.

fjahr · 2020-08-03T15:01:20Z

src/wallet/wallet.cpp


+    if (gArgs.IsArgSet("-maxapsfee")) {
+        CAmount n = 0;
+        if (gArgs.GetArg("-maxapsfee", "") == "-1") {


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?

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.

fjahr · 2020-08-03T15:07:44Z

src/wallet/init.cpp

    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)",
                                                               CURRENCY_UNIT, FormatMoney(DEFAULT_FALLBACK_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
    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);
+    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);


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.

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.

Suggested change

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

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

Thanks, tweaked a little.

fjahr · 2020-08-03T15:16:56Z

src/wallet/wallet.cpp

+    CAmount maxapsfee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);
+    int nChangePosIn = nChangePosInOut;
+    CTransactionRef tx2 = tx;


nit:

Suggested change

CAmount maxapsfee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);

int nChangePosIn = nChangePosInOut;

CTransactionRef tx2 = tx;

const CAmount max_aps_fee = gArgs.GetArg("-maxapsfee", DEFAULT_MAX_AVOIDPARTIALSPEND_FEE);

const int change_pos_in = nChangePosInOut;

CTransactionRef tx_aps = tx;

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

fjahr · 2020-08-03T15:18:27Z

src/wallet/wallet.cpp

+        CAmount nFeeRet2;
+        int nChangePosInOut2 = nChangePosIn;
+        bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results


nit:

Suggested change

CAmount nFeeRet2;

int nChangePosInOut2 = nChangePosIn;

bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results

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

Keeping these as is, as their names are inherited.

src/wallet/wallet.cpp

…elow 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).

Co-authored-by: Fabian Jahr <[email protected]>

kallewoof · 2020-08-06T01:26:48Z

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

fjahr

tested ACK 7f13dfb

achow101

ACK 7f13dfb

achow101 · 2020-08-14T19:30:47Z

test/functional/wallet_groups.py

        assert_approx(v[0], 0.2)
        assert_approx(v[1], 1.3, 0.0001)

+        # Test 'avoid partial if warranted, even if disabled'


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

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

fanquake · 2020-08-15T00:32:39Z

@xekyo you might also be interested in this?

jonatack · 2020-08-15T04:55:29Z

Concept ACK

jonatack

ACK 7f13dfb, 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 · 2020-08-16T12:42:31Z

src/wallet/wallet.cpp

+        if (gArgs.GetArg("-maxapsfee", "") == "-1") {
+            n = -1;
+        } else if (!ParseMoney(gArgs.GetArg("-maxapsfee", ""), n)) {
+            error = AmountErrMsg("maxapsfee", gArgs.GetArg("-maxapsfee", ""));


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

jonatack · 2020-08-16T13:17:27Z

test/functional/wallet_groups.py

+        self.nodes[0].sendtoaddress(addr_aps, 1.0)
+        self.nodes[0].sendtoaddress(addr_aps, 1.0)
+        self.nodes[0].generate(1)
+        txid4 = self.nodes[3].sendtoaddress(self.nodes[0].getnewaddress(), 0.1)


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)

jonatack · 2020-08-16T13:21:14Z

test/functional/wallet_groups.py

+        values = [vout["value"] for vout in tx3["vout"]]
+        values.sort()
+        assert_approx(values[0], 0.1, 0.0001)
+        assert_approx(values[1], 1.4)


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)

jonatack · 2020-08-16T15:48:40Z

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

meshcollider

Code review ACK 7f13dfb

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

meshcollider · 2020-08-17T03:46:10Z

src/wallet/wallet.cpp

+        }
+        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."));


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

meshcollider · 2020-08-17T03:54:49Z

test/functional/wallet_groups.py

+        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


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

…re within a specified range 7f13dfb test: test the implicit avoid partial spends functionality (Karl-Johan Alm) b82067b wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm) Pull request description: 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. ACKs for top commit: fjahr: tested ACK 7f13dfb achow101: ACK 7f13dfb jonatack: ACK 7f13dfb, 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. meshcollider: Code review ACK 7f13dfb Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c

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

… range Summary: * 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). * test: test the implicit avoid partial spends functionality Co-authored-by: Fabian Jahr <[email protected]> This is a backport of Core [[bitcoin/bitcoin#14582 | PR14582]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D7807

kallewoof force-pushed the 181026-try-avoidpartialspends branch from dbdbca2 to 89e42d7 Compare October 26, 2018 06:46

fanquake added the Wallet label Oct 26, 2018

bitcoin deleted a comment from SrMaus Oct 26, 2018

DrahtBot added the Needs rebase label Nov 9, 2018

kallewoof force-pushed the 181026-try-avoidpartialspends branch 2 times, most recently from dc223c9 to 15342ad Compare November 12, 2018 04:45

DrahtBot removed the Needs rebase label Nov 12, 2018

DrahtBot added the Needs rebase label Jul 10, 2019

kallewoof force-pushed the 181026-try-avoidpartialspends branch 2 times, most recently from 7035fce to c74d0e9 Compare July 10, 2019 13:47

DrahtBot removed the Needs rebase label Jul 10, 2019

kallewoof force-pushed the 181026-try-avoidpartialspends branch 2 times, most recently from 431bb20 to f48a63e Compare July 11, 2019 04:35

kallewoof force-pushed the 181026-try-avoidpartialspends branch from f48a63e to d14d744 Compare July 18, 2019 05:33

DrahtBot removed the Needs rebase label Jul 18, 2019

DrahtBot added the Needs rebase label Oct 24, 2019

laanwj mentioned this pull request Nov 26, 2019

Stop (avoid?) spending malleable change #17583

Closed

kallewoof force-pushed the 181026-try-avoidpartialspends branch from d14d744 to 7e25756 Compare December 24, 2019 04:30

DrahtBot removed the Needs rebase label Dec 24, 2019

kallewoof force-pushed the 181026-try-avoidpartialspends branch 2 times, most recently from 4864149 to eda938a Compare January 8, 2020 07:05

kallewoof force-pushed the 181026-try-avoidpartialspends branch from c022659 to 75212a5 Compare January 8, 2020 12:13

kallewoof changed the title ~~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 Jan 8, 2020

kallewoof force-pushed the 181026-try-avoidpartialspends branch 2 times, most recently from 76bad7a to ed515f4 Compare January 8, 2020 12:58

fjahr reviewed Aug 3, 2020

View reviewed changes

wallet: try -avoidpartialspends mode and use its result if fees are b…

b82067b

…elow 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).

kallewoof force-pushed the 181026-try-avoidpartialspends branch from a923fc3 to 8519d4e Compare August 6, 2020 01:09

test: test the implicit avoid partial spends functionality

7f13dfb

Co-authored-by: Fabian Jahr <[email protected]>

kallewoof force-pushed the 181026-try-avoidpartialspends branch from 8519d4e to 7f13dfb Compare August 6, 2020 01:25

DrahtBot removed the Needs rebase label Aug 6, 2020

This was referenced Aug 6, 2020

Refactor: Improve setup_clean_chain semantics #19168

Closed

Run clang-tidy -*,performance-* #19675

Closed

fjahr approved these changes Aug 8, 2020

View reviewed changes

achow101 reviewed Aug 14, 2020

View reviewed changes

fanquake requested a review from meshcollider August 15, 2020 00:31

jonatack reviewed Aug 16, 2020

View reviewed changes

meshcollider added the Needs release note label Aug 17, 2020

meshcollider reviewed Aug 17, 2020

View reviewed changes

meshcollider merged commit c831e10 into bitcoin:master Aug 17, 2020

kallewoof deleted the 181026-try-avoidpartialspends branch August 17, 2020 05:14

kallewoof mentioned this pull request Aug 17, 2020

-maxapsfee follow-up #19743

Merged

maflcko removed the Needs release note label Aug 17, 2020

jonatack mentioned this pull request Aug 17, 2020

Intermittent CI failure: wallet_groups.py #19749

Closed

jonatack mentioned this pull request Jan 20, 2021

doc: APS explanation clarified in 0.21.0 release notes #20951

Closed

jonatack mentioned this pull request Mar 22, 2022

test: coinselection edge cases #24580

Closed

bitcoin locked and limited conversation to collaborators Oct 31, 2022

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

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

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

Uh oh!

Conversation

kallewoof commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider commented Nov 9, 2018

Uh oh!

kallewoof commented Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Jan 8, 2020

Uh oh!

kallewoof commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Aug 3, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Aug 6, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 15, 2020

Uh oh!

jonatack commented Aug 15, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 16, 2020

Uh oh!

kallewoof commented Oct 26, 2018 •

edited

Loading

DrahtBot commented Nov 1, 2018 •

edited

Loading

kallewoof commented Nov 12, 2018 •

edited

Loading

kallewoof commented Jan 8, 2020 •

edited

Loading

kallewoof Aug 6, 2020 •

edited

Loading

jonatack Aug 16, 2020 •

edited

Loading

jonatack Aug 16, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading