Fix overflow bug in analyzepsbt fee: CAmount instead of int #15582

1. 
   sipa commented at 10:17 pm on March 11, 2019: member

   This causes the fee and estimated_feerate values in the the analyzepsbt output to be off if the amount being spent exceed 21.47483647 BTC.
2. Fix overflow bug in analyzepsbt fee: CAmount instead of int c9963ae8b1
3. sipa added this to the milestone 0.18.0 on Mar 11, 2019
4. 
   promag commented at 10:24 pm on March 11, 2019: member

   ACK c9963ae8b1a4d26d19c58e18fde9c85783edb788, deserves a test?
5. fanquake added the label RPC/REST/ZMQ on Mar 11, 2019
6. 
   DrahtBot commented at 2:03 am on March 12, 2019: 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:

   • #15508 (Refactor analyzepsbt for use outside RPC code by gwillen)

   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.

7. 
   achow101 commented at 5:23 am on March 12, 2019: member

   utACK c9963ae8b1a4d26d19c58e18fde9c85783edb788
8. 
   jonasschnelli commented at 8:59 am on March 12, 2019: contributor

   utACK c9963ae8b1a4d26d19c58e18fde9c85783edb788
9. 
   practicalswift commented at 10:11 am on March 12, 2019: contributor

   @sipa Very nice find. Thanks for fixing.

   How did you find this issue? Did you find this by manual or tool assisted analysis?

   I had this issue in my back log of conversion warnings to investigate manually but you beat me to it :-)

   0rpc/rawtransaction.cpp:1986: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
   

   FWIW, these are all the related CAmount (int64_t) conversion warnings I have in my back log (I haven’t analysed any of these yet):

    0policy/fees.cpp:1002: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
    1policy/fees.cpp:1009: conversion to ‘std::set<double>::key_type {aka double}’ from ‘CAmount {aka long int}’ may alter its value
    2rpc/rawtransaction.cpp:1986: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
    3test/mempool_tests.cpp:554: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
    4test/mempool_tests.cpp:558: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
    5test/mempool_tests.cpp:562: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
    6test/script_tests.cpp:166: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
    7test/script_tests.cpp:332: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
    8txmempool.cpp:1012: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
    9txmempool.cpp:1013: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
   10txmempool.h:235: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
   11txmempool.h:309: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
   12wallet/test/coinselector_tests.cpp:481: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
   

   utACK c9963ae8b1a4d26d19c58e18fde9c85783edb788

10. 
    laanwj commented at 11:30 am on March 12, 2019: member

    utACK c9963ae8b1a4d26d19c58e18fde9c85783edb788
11. laanwj merged this on Mar 12, 2019
12. laanwj closed this on Mar 12, 2019

    ---

13. laanwj referenced this in commit 59b291966e on Mar 12, 2019
14. sipa added the label Needs backport on Mar 13, 2019
15. MarcoFalke removed the label Needs backport on Mar 13, 2019
16. MarcoFalke added the label Needs backport on Mar 13, 2019
17. MarcoFalke assigned laanwj on Mar 13, 2019
18. laanwj referenced this in commit 232ef630ec on Mar 13, 2019
19. MarcoFalke removed the label Needs backport on Mar 13, 2019
20. 
    fanquake commented at 1:22 am on March 14, 2019: member

    Backported in https://github.com/bitcoin/bitcoin/commit/232ef630ecd30b9a3d7c69ec8c7f53df85eb2771.
21. 
    practicalswift commented at 8:27 am on March 14, 2019: contributor

    @sipa Are you willing to share how this bug was found? I think it would be valuable to do an informal short “post mortem” of this kind of issues.

    That would allow us to reason about which mistakes that are recurring and could be candidates for identification by intelligent use of tooling.

22. HashUnlimited referenced this in commit c0adb5351a on Apr 19, 2019
23. MarcoFalke locked this on Dec 16, 2021

Contributors
sipa promag DrahtBot achow101 jonasschnelli practicalswift laanwj fanquake

Labels
RPC/REST/ZMQ

Milestone
0.18.0