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

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:181026-try-avoidpartialspends changing 5 files +119 −3
  1. 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.

  2. kallewoof force-pushed on Oct 26, 2018
  3. fanquake added the label Wallet on Oct 26, 2018
  4. fanquake deleted a comment on Oct 26, 2018
  5. 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.

  6. 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
  7. DrahtBot added the label Needs rebase on Nov 9, 2018
  8. 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.

  9. kallewoof force-pushed on Nov 12, 2018
  10. kallewoof force-pushed on Nov 12, 2018
  11. DrahtBot removed the label Needs rebase on Nov 12, 2018
  12. DrahtBot added the label Needs rebase on Jul 10, 2019
  13. kallewoof force-pushed on Jul 10, 2019
  14. kallewoof force-pushed on Jul 10, 2019
  15. DrahtBot removed the label Needs rebase on Jul 10, 2019
  16. kallewoof force-pushed on Jul 11, 2019
  17. kallewoof force-pushed on Jul 11, 2019
  18. kallewoof force-pushed on Jul 18, 2019
  19. DrahtBot removed the label Needs rebase on Jul 18, 2019
  20. DrahtBot added the label Needs rebase on Oct 24, 2019
  21. kallewoof force-pushed on Dec 24, 2019
  22. DrahtBot removed the label Needs rebase on Dec 24, 2019
  23. kallewoof force-pushed on Jan 3, 2020
  24. kallewoof force-pushed on Jan 8, 2020
  25. gmaxwell commented at 11:14 am on January 8, 2020: contributor
    I’m super happy to see work on this!
  26. 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. :)
  27. kallewoof force-pushed on Jan 8, 2020
  28. 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
  29. kallewoof force-pushed on Jan 8, 2020
  30. kallewoof force-pushed on Jan 8, 2020
  31. kallewoof force-pushed on Jan 9, 2020
  32. DrahtBot added the label Needs rebase on Jan 29, 2020
  33. kallewoof force-pushed on Jan 30, 2020
  34. DrahtBot removed the label Needs rebase on Jan 30, 2020
  35. DrahtBot added the label Needs rebase on May 1, 2020
  36. kallewoof force-pushed on May 2, 2020
  37. kallewoof force-pushed on May 2, 2020
  38. DrahtBot removed the label Needs rebase on May 2, 2020
  39. DrahtBot added the label Needs rebase on May 4, 2020
  40. kallewoof force-pushed on May 7, 2020
  41. kallewoof force-pushed on May 7, 2020
  42. DrahtBot removed the label Needs rebase on May 7, 2020
  43. kallewoof force-pushed on May 7, 2020
  44. DrahtBot added the label Needs rebase on Jul 30, 2020
  45. 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.
  46. kallewoof closed this on Jul 31, 2020

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

  48. 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.
  49. 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). :)

  50. kallewoof reopened this on Aug 3, 2020

  51. sipa commented at 4:07 am on August 3, 2020: member
    Concept ACK
  52. 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.
  53. 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.
  54. 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).
  55. 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.
  56. 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

  57. 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) {
    
  58. 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!
  59. 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.

  60. 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
  61. kallewoof force-pushed on Aug 6, 2020
  62. test: test the implicit avoid partial spends functionality
    Co-authored-by: Fabian Jahr <fjahr@protonmail.com>
    7f13dfb587
  63. kallewoof force-pushed on Aug 6, 2020
  64. 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.
  65. DrahtBot removed the label Needs rebase on Aug 6, 2020
  66. fjahr approved
  67. fjahr commented at 3:44 pm on August 8, 2020: member
    tested ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
  68. 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.
  69. achow101 commented at 7:33 pm on August 14, 2020: member
    ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
  70. fanquake requested review from meshcollider on Aug 15, 2020
  71. fanquake commented at 0:32 am on August 15, 2020: member
    @Xekyo you might also be interested in this?
  72. jonatack commented at 4:55 am on August 15, 2020: member
    Concept ACK
  73. 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         }
    
  74. 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)
    
  75. 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)
    
  76. 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.

  77. jonatack commented at 3:48 pm on August 16, 2020: member
    As a follow-up, will need a release note.
  78. meshcollider added the label Needs release note on Aug 17, 2020
  79. 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
  80. 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
  81. 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

  82. meshcollider merged this on Aug 17, 2020
  83. meshcollider closed this on Aug 17, 2020

  84. kallewoof deleted the branch on Aug 17, 2020
  85. sidhujag referenced this in commit fb6af86609 on Aug 17, 2020
  86. MarcoFalke removed the label Needs release note on Aug 17, 2020
  87. meshcollider referenced this in commit a2a250c7d0 on Aug 18, 2020
  88. jasonbcox referenced this in commit 81fe776b23 on Oct 8, 2020
  89. DrahtBot locked this on Oct 31, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me