Fix unsigned integer overflow in LoadMempool #24227

1. 
   MarcoFalke commented at 2:51 pm on February 1, 2022: member

   It doesn’t seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.

   This removes one of the two violations.

   This should be a refactor.

2. MarcoFalke added the label Refactoring on Feb 1, 2022
3. MarcoFalke added the label Validation on Feb 1, 2022
4. MarcoFalke force-pushed on Feb 1, 2022
5. PastaPastaPasta approved
6. 
   PastaPastaPasta commented at 4:16 pm on February 1, 2022: contributor

   utACK aaaae5349d1d5985081751c3a3b7386333d48cef, I reviewed the code, and agree it makes sense to merge
7. Fix unsigned integer overflow in LoadMempool fadcd03139
8. MarcoFalke force-pushed on Feb 1, 2022
9. 
   MarcoFalke commented at 7:01 pm on February 1, 2022: member

   I’ve pushed a new version, which:

   • is a smaller diff and easier to review, as it doesn’t change any types
   • doesn’t introduce a presumed signed-integer-overflow when the mempool.dat on disk is badly corrupted
10. luke-jr approved
11. 
    luke-jr commented at 9:33 pm on February 5, 2022: member

    utACK
12. unknown approved
13. 
    unknown commented at 1:42 am on February 6, 2022: none

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/24227/commits/fadcd031390dd4588bbb1c07e5020a7131312050

    Tried running below code to confirm:

    0  uint64_t i = 4;
    1  while (--i)
    2  { 
    3  cout<<i;
    4  }
    5  cout<<i;
    6  
    

    0  uint64_t i = 4;
    1  while (i--)
    2  { 
    3  cout<<i;
    4  }
    5  cout<<i;
    6  
    

    0  uint64_t i = 4;
    1  while (i)
    2  { 
    3   --i;
    4  cout<<i;
    5  }
    6  cout<<i;
    7  
    

    It is also according to coding style mentioned in docs: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

14. 
    PastaPastaPasta commented at 9:22 am on February 7, 2022: contributor

    Is the PR name really accurate, this can’t overflow, maybe only underflow?
15. 
    MarcoFalke commented at 9:50 am on February 7, 2022: member

    No idea, but the suppression is called unsigned-integer-overflow, which is why I picked the title.
16. 
    PastaPastaPasta commented at 10:23 am on February 7, 2022: contributor

    No idea, but the suppression is called unsigned-integer-overflow, which is why I picked the title.

    I presume there are more unsigned-integer-overflow in validation.cpp preventing you from removing the suppression?

17. 
    MarcoFalke commented at 10:35 am on February 7, 2022: member

    Have you seen the pull request you commented on? #24196#pullrequestreview-868681878
18. 
    PastaPastaPasta commented at 11:58 am on February 7, 2022: contributor

    Have you seen the pull request you commented on? #24196 (review)

    Ahh, thanks, I forgot they were related / about that PR

19. MarcoFalke merged this on Feb 7, 2022
20. MarcoFalke closed this on Feb 7, 2022

    ---

21. MarcoFalke deleted the branch on Feb 7, 2022
22. sidhujag referenced this in commit eac3f64704 on Feb 7, 2022
23. PastaPastaPasta referenced this in commit cbc14612ba on Apr 7, 2022
24. PastaPastaPasta referenced this in commit ef93418b9a on Apr 11, 2022
25. gades referenced this in commit 707272a142 on Apr 19, 2022
26. Fabcien referenced this in commit 293b7411ec on Dec 19, 2022
27. DrahtBot locked this on Feb 7, 2023

Contributors
MarcoFalke PastaPastaPasta luke-jr

Labels
Refactoring Validation