Don't create change at dust limit #9343

1. 
   morcos commented at 9:49 PM on December 13, 2016: member

   @cozz @gmaxwell @laanwj

   This might be controversial since it directly reverts behavior that was explicitly included in #4331 (merged as #5831)

   That PR took the approach that if subtractFeeFromAmount was selected then it would be unexpected for the user to ever spend (net of sent - received) more bitcoins than the amount they requested to pay. Therefore if any change was under the dust limit, instead of sending it to fee, the change was increased to the dust limit.

   I believe this is misguided. Creating a change output at the dust limit is likely uneconomical. There is a difference between having to draw a threshold below which all outputs are useless, and inverting that to say that an output just above that limit is useful. Giving yourself change at the dust limit is no better (and maybe even worse) than not giving yourself change in the first place, so I don't believe it will adversely affect anyone who was not expecting to spend more coins than the pay amount.

   The primary reason for this change is to make our wallet smarter about not creating uselessly small outputs. To increase the change to the point where it was sufficiently big to be useful would require potentially removing too much from the recipients.

2. fanquake added the label Wallet on Dec 13, 2016
3. 
   laanwj commented at 12:49 PM on December 14, 2016: member

   The main motivation behind this feature was to be able to send the whole balance without having to guess or bisect. Will that be broken by this change?

4. 
   morcos commented at 12:59 PM on December 14, 2016: member

   No. That will still work the same. In that case you wouldn't have had any change anyway.

   In any case, it won't result in you sending more/different coins than you otherwise would have, it just might result in you not receiving back a very small (at the dust limit) coin, which I think is a positive change.

5. 
   laanwj commented at 1:32 PM on December 14, 2016: member

   Yes, seems like an improvement then! Concept ACK

6. 
   gmaxwell commented at 9:31 AM on December 21, 2016: contributor

   Concept ACK.

7. 
   gmaxwell commented at 8:52 AM on January 8, 2017: contributor

   utACK. Especially at the current IsDust level, those outputs are not useful.

8. morcos force-pushed on Jan 17, 2017
9. 
   morcos commented at 6:17 PM on January 17, 2017: member

   rebased

10. 
    kallewoof commented at 4:57 AM on February 9, 2017: member

    utACK

11. in src/qt/coincontroldialog.cpp:None in baf24f4abc outdated

     556 | -                        nChange = 0;
     557 | -                    }
     558 | +                    nPayFee += nChange;
     559 | +                    nChange = 0;
     560 | +                    if (CoinControlDialog::fSubtractFeeFromAmount)
     561 | +                        nBytes -= 34; // we didn't detect lack of change above
    

    ---

    ---

    

    ryanofsky commented at 1:20 AM on February 28, 2017:

    There are three instances where nBytes -= 34 is occurs in this function now. I wonder if they could be unified, maybe by deleting the && !CoinControlDialog::fSubtractFeeFromAmount condition on the third instance, and then getting rid of the first two instances?

12. ryanofsky referenced this in commit 7b4852ee13 on Feb 28, 2017
13. 
    ryanofsky commented at 1:25 AM on February 28, 2017: member

    utACK for wallet.cpp. I don't currently understand the coincontroldialog.cpp code well enough to ACK that.

    I wrote a test for the wallet.cpp change in https://github.com/ryanofsky/bitcoin/commit/7b4852ee1365004c875876e29453f08c88b7f221, which you could consider including.

14. laanwj added this to the milestone 0.15.0 on Apr 20, 2017
15. 
    instagibbs commented at 7:27 PM on May 11, 2017: member

    concept ACK will review, needs rebase.

16. Don't create change at the dust limit, even if it means paying more than expected 61718268b5
17. morcos force-pushed on Jun 14, 2017
18. 
    morcos commented at 7:36 PM on June 14, 2017: member

    rebased

19. 
    instagibbs commented at 8:43 PM on June 14, 2017: member

    utACK https://github.com/bitcoin/bitcoin/pull/9343/commits/61718268b5067acd1b8af4a4e94b1bf60334e1f7

    less is more

20. 
    ryanofsky commented at 7:00 PM on June 21, 2017: member

    Confirmed no changes since my last review other than rebase, so utACK 61718268b5067acd1b8af4a4e94b1bf60334e1f7 again just for wallet.cpp.

    I didn't go back and try to decipher the coincontroldialog.cpp change, though it seems like my previous question there still stands. And maybe my test from https://github.com/ryanofsky/bitcoin/commit/7b4852ee1365004c875876e29453f08c88b7f221 still applies, but none of this should hold up merge.

21. laanwj merged this on Jun 22, 2017
22. laanwj closed this on Jun 22, 2017

    ---

23. laanwj referenced this in commit 209eef60a9 on Jun 22, 2017
24. PastaPastaPasta referenced this in commit 550d644d59 on Jul 6, 2019
25. PastaPastaPasta referenced this in commit 46a9c36f3f on Jul 8, 2019
26. PastaPastaPasta referenced this in commit 7124145c9e on Jul 9, 2019
27. PastaPastaPasta referenced this in commit 1c4981d719 on Jul 11, 2019
28. barrystyle referenced this in commit 0efef5f37e on Jan 22, 2020
29. DrahtBot locked this on Sep 8, 2021

Milestone
0.15.0