Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 12, 2019

Conversation

Projects
None yet
9 participants
@sipa
Copy link
Member

commented Mar 11, 2019

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.

@sipa sipa added this to the 0.18.0 milestone Mar 11, 2019

@promag
Copy link
Member

left a comment

ACK c9963ae, deserves a test?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

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.

@achow101

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

utACK c9963ae

1 similar comment
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

utACK c9963ae

@practicalswift

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

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

rpc/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):

policy/fees.cpp:1002: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
policy/fees.cpp:1009: conversion to ‘std::set<double>::key_type {aka double}’ from ‘CAmount {aka long int}’ may alter its value
rpc/rawtransaction.cpp:1986: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
test/mempool_tests.cpp:554: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
test/mempool_tests.cpp:558: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
test/mempool_tests.cpp:562: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
test/script_tests.cpp:166: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
test/script_tests.cpp:332: conversion to ‘int’ from ‘CAmount {aka long int}’ may alter its value
txmempool.cpp:1012: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
txmempool.cpp:1013: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
txmempool.h:235: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
txmempool.h:309: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value
wallet/test/coinselector_tests.cpp:481: conversion to ‘double’ from ‘CAmount {aka long int}’ may alter its value

utACK c9963ae

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

utACK c9963ae

@laanwj laanwj merged commit c9963ae into bitcoin:master Mar 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 12, 2019

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

c9963ae Fix overflow bug in analyzepsbt fee: CAmount instead of int (Pieter Wuille)

Pull request description:

  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.

Tree-SHA512: 61c1e26894617c51cc5fc026a3677a0b759fcbac1e70efa7fdc68d57cfd484525e18c906f1b8c06fd5d846b74a3cb4bc0bbd302a6eeaade79055a47d6d0dacc2

@sipa sipa added the Needs backport label Mar 13, 2019

laanwj added a commit that referenced this pull request Mar 13, 2019

Fix overflow bug in analyzepsbt fee: CAmount instead of int
Github-Pull: #15582
Rebased-From: c9963ae
Tree-SHA512: ed1dcfafb7015de5405112938b04c4009bec3349a8d4e8aca641598ccab31a34c7f16b5045670909fd2e795fc40640a79658ef6b1771e9f21abd0ca759b239b5
@fanquake

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Backported in 232ef63.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

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

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 19, 2019

Fix overflow bug in analyzepsbt fee: CAmount instead of int
Github-Pull: bitcoin#15582
Rebased-From: c9963ae
Tree-SHA512: ed1dcfafb7015de5405112938b04c4009bec3349a8d4e8aca641598ccab31a34c7f16b5045670909fd2e795fc40640a79658ef6b1771e9f21abd0ca759b239b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.