Fix rounding bug in calculation of minimum change #11145

1. 
   morcos commented at 8:14 PM on August 25, 2017: member

   Thanks to @juscamarena for reporting this.

   Please backport to 0.15.

   There was a potential rounding error where the fee for the change added to the fee for the original tx could be less than the fee for the tx including change.

   This is fixed in the first commit. The second commit adds one more snippet of information in the fee calculation report. I actually realized that there is more information that would be nice to report, but we can add that post 0.15.

   An open question is whether we should be returning failure if the test in line 2885 is hit or just resetting pick_new_inputs and continuing. Originally I made it a failure to avoid any possible infinite loops. But the case hit here is an example of where that logic possibly backfired.

2. Fix rounding errors in calculation of minimum change size a54c7b94f8
3. Output a bit more information for fee calculation report. 6af49dddea
4. gmaxwell approved
5. 
   gmaxwell commented at 8:21 PM on August 25, 2017: contributor

   utACK (reviewed this earlier based on the report; this is the fix I was expecting.)

6. MarcoFalke added the label Wallet on Aug 25, 2017
7. MarcoFalke added the label Needs backport on Aug 25, 2017
8. MarcoFalke added this to the milestone 0.15.1 on Aug 25, 2017
9. MarcoFalke added this to the milestone 0.15.0 on Aug 25, 2017
10. MarcoFalke removed this from the milestone 0.15.1 on Aug 25, 2017
11. instagibbs approved
12. 
    instagibbs commented at 8:39 PM on August 25, 2017: member

    utACK

13. 
    ryanofsky commented at 9:01 PM on August 25, 2017: member

    utACK 6af49dddeaeec7f134e86d6f8cf839c55870b7ab.

    Some code is shuffled around in the first commit, but only actual change in behavior seems to be computing minimum fee as GetMinimumFee(nBytes + change_prototype_size + 2) instead of GetMinimumFee(nBytes) + GetMinimumFee(change_prototype_size).

    Unclear whether it's the order of operations change or the +2 bytes which fixes the originally reported bug.

    A test would be nice here but I think I say that every time I review CreateTransaction code (#9343, #10712)

14. 
    gmaxwell commented at 10:09 PM on August 25, 2017: contributor

    Unclear whether it's the order of operations change or the +2 bytes which fixes the originally reported bug.

    Both. GetMinimumFee(nBytes) + GetMinimumFee(change_prototype_size) has a rounding error compared to GetMinimumFee(nBytes + change_prototype_size) and the +2 deals with the case that the output count becomes large enough to increase the compact size.

15. 
    juscamarena commented at 10:49 PM on August 25, 2017: none

    tACK, I no longer get the specific above issue. I ran into some other thing, that @morcos would probably better explain as he did as well.

16. 
    sdaftuar commented at 2:46 AM on August 26, 2017: member

    utACK

17. 
    kallewoof commented at 3:57 AM on August 26, 2017: member

    utACK 6af49dddeaeec7f134e86d6f8cf839c55870b7ab

18. gmaxwell approved
19. 
    gmaxwell commented at 10:47 PM on August 26, 2017: contributor

    ACK. (upgrading earlier utACK-- I left a node running for a few hours in regtest in a sending stress test and didn't manage to hit any issues.)

20. 
    meshcollider commented at 11:57 PM on August 26, 2017: contributor

    utACK https://github.com/bitcoin/bitcoin/commit/a54c7b94f8825e9b52fec9066fe7c1d5b6f53482

21. 
    sipa commented at 7:14 PM on August 27, 2017: member

    utACK 6af49dddeaeec7f134e86d6f8cf839c55870b7ab

22. laanwj merged this on Aug 28, 2017
23. laanwj closed this on Aug 28, 2017

    ---

24. laanwj referenced this in commit 9c833f471c on Aug 28, 2017
25. laanwj referenced this in commit e51bb71e4a on Aug 28, 2017
26. laanwj referenced this in commit 5b059a833e on Aug 28, 2017
27. 
    MarcoFalke commented at 3:56 PM on September 5, 2017: member

    This was backported. Removing label

28. MarcoFalke removed the label Needs backport on Sep 5, 2017
29. PastaPastaPasta referenced this in commit 814408867c on Sep 19, 2019
30. codablock referenced this in commit b3821de0fd on Sep 20, 2019
31. codablock referenced this in commit 1bce70c5da on Sep 22, 2019
32. codablock referenced this in commit f9453e0558 on Sep 23, 2019
33. barrystyle referenced this in commit 3eac4c7c08 on Jan 22, 2020
34. DrahtBot locked this on Sep 8, 2021

Contributors

morcosgmaxwellinstagibbsryanofskyjuscamarenasdaftuarkallewoofmeshcollidersipaMarcoFalke

Milestone
0.15.0