mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false #14226

1. 
   practicalswift commented at 5:27 pm on September 15, 2018: contributor

   Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false.

   0txmempool.cpp:219:44: runtime error: unsigned integer overflow: 18446744073709551615 * 155 cannot be represented in type 'unsigned long' !=
   

2. practicalswift renamed this:
   mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(…) when add=true
   mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is true
   on Sep 15, 2018
3. practicalswift renamed this:
   mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is true
   mempool: Fix unintended unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf(bool add, …) when add is false
   on Sep 15, 2018
4. practicalswift closed this on Sep 15, 2018

   ---

5. fanquake added the label Mempool on Sep 15, 2018
6. 
   sdaftuar commented at 7:04 pm on September 24, 2018: member

   Why did you decide to close this PR? After some digging I believe this change correctly fixes a bug, though I think the problem is not in the unsigned integer overflow (which is well-defined), but in casting the resulting unsigned integer to a signed type (which I have recently learned is not defined behavior, when the unsigned value would be negative).

   So I think this change, which casts to int64_t first before multiplying by -1, is exactly correct.

7. practicalswift reopened this on Sep 24, 2018

   ---

8. 
   practicalswift commented at 7:24 pm on September 24, 2018: contributor

   @sdaftuar I received some feedback about my PR:s creating too much review work – that’s the background to the close. But I’m happy to re-open :-)
9. 
   MarcoFalke commented at 7:56 pm on September 24, 2018: member

   I haven’t reviewed the claim that @sdaftuar makes, but I checked that on my machine the bitcoind compiled with clang doesn’t change at all, and for gcc the objdump is the same.
10. 
    DrahtBot commented at 8:52 pm on September 24, 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:

    • #13804 (WIP: Transaction Pool Layer by MarcoFalke)

    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.

11. 
    sdaftuar commented at 9:01 pm on September 24, 2018: member

    utACK @ryanofsky pointed me to this: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf, from section 4.7.3

    If the destination type is signed, the value is unchanged if it can be represented in the destination type (and bit-field width); otherwise, the value is implementation-defined.

12. 
    practicalswift commented at 9:12 pm on September 24, 2018: contributor

    @sdaftuar @ryanofsky You might want to take a look at #11551 (Fix unsigned integer wrap-around in GetBlockProofEquivalentTime) and #11535 :-)
13. 
    DrahtBot commented at 9:55 am on September 28, 2018: member

    Coverage	Change (pull 14226)	Reference (master)
    Lines	-0.0552 %	87.0361 %
    Functions	-0.0309 %	84.1130 %
    Branches	-0.0464 %	51.5451 %

14. mempool: Fix unsigned integer wraparound in CTxMemPool::UpdateAncestorsOf 8b1b039940
15. practicalswift force-pushed on Nov 6, 2018
16. practicalswift force-pushed on Nov 7, 2018
17. practicalswift closed this on Dec 19, 2018

    ---

18. practicalswift deleted the branch on Apr 10, 2021
19. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift sdaftuar MarcoFalke DrahtBot

Labels
Mempool