Fix integer sanitizer suppressions in validation.cpp #24196

1. 
   MarcoFalke commented at 4:38 pm on January 28, 2022: member

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

   Fix it with a refactor and remove the suppression.

2. MarcoFalke added the label Refactoring on Jan 28, 2022
3. 
   DrahtBot commented at 4:00 am on January 29, 2022: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

4. luke-jr approved
5. 
   luke-jr commented at 3:51 am on January 31, 2022: member

   int is only guaranteed to have a max value of 32768. In a 2 MB block, that’s 61 bytes per input. Seems quite possible to hit it?

   OTOH, I don’t think we support/work on such platforms right now, so this probably isn’t a real issue. So utACK anyway.

6. 
   MarcoFalke commented at 7:13 am on January 31, 2022: member

   It wouldn’t be possible to start Bitcoin Core if int max was 32768. See also:

   0src/compat/assumptions.h:static_assert(sizeof(short) == 2, "16-bit short assumed");
   1src/compat/assumptions.h:static_assert(sizeof(int) == 4, "32-bit int assumed");
   2src/compat/assumptions.h:static_assert(sizeof(unsigned) == 4, "32-bit unsigned assumed");
   3src/compat/assumptions.h:static_assert(sizeof(size_t) == 4 || sizeof(size_t) == 8, "size_t assumed to be 32-bit or 64-bit");
   

7. in src/validation.cpp:1790 in fa2d0c7008 outdated

   1786@@ -1787,8 +1787,8 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
   1787                 error("DisconnectBlock(): transaction and undo data inconsistent");
   1788                 return DISCONNECT_FAILED;
   1789             }
   1790-            for (unsigned int j = tx.vin.size(); j-- > 0;) {
   1791-                const COutPoint &out = tx.vin[j].prevout;
   1792+            for (int j = int(tx.vin.size()); j-- > 0;) {
   

   ---

   ---

   

   PastaPastaPasta commented at 5:15 am on February 1, 2022:

   nit: Is this equivalent to below? If so, is that more clear?

   0            for (int j = int(tx.vin.size()) - 1; j >= 0; j--) {
   

   ---

   

   MarcoFalke commented at 2:32 pm on February 7, 2022:

   pushed something else
8. PastaPastaPasta approved
9. 
   PastaPastaPasta commented at 5:18 am on February 1, 2022: contributor

   utACK fa2d0c7008864384012c1bd84a602b85231f5983 modulo nit
10. MarcoFalke marked this as a draft on Feb 1, 2022
11. MarcoFalke force-pushed on Feb 1, 2022
12. Fix integer sanitizer suppressions in validation.cpp fac62056b5
13. MarcoFalke force-pushed on Feb 7, 2022
14. MarcoFalke marked this as ready for review on Feb 7, 2022
15. 
    MarcoFalke commented at 2:33 pm on February 7, 2022: member

    Changed to a pure refactor, that doesn’t change any types. Also, it doesn’t change the binary with clang++ -O2 on my system, though with gcc it does. It is the same refactor that was used in #https://github.com/bitcoin/bitcoin/pull/24227
16. hebasto approved
17. 
    hebasto commented at 2:40 pm on February 7, 2022: member

    ACK fac62056b56e0a28baf0b6f285752d83fbf96074, I have reviewed the code and it looks OK, I agree it can be merged.
18. unknown approved
19. 
    unknown commented at 2:51 pm on February 7, 2022: none

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/24196/commits/fac62056b56e0a28baf0b6f285752d83fbf96074
20. MarcoFalke merged this on Feb 9, 2022
21. MarcoFalke closed this on Feb 9, 2022

    ---

22. MarcoFalke deleted the branch on Feb 9, 2022
23. sidhujag referenced this in commit 89a770bc17 on Feb 9, 2022
24. Fabcien referenced this in commit 295dde9e88 on Dec 9, 2022
25. DrahtBot locked this on Feb 9, 2023

Contributors
MarcoFalke DrahtBot luke-jr PastaPastaPasta hebasto

Labels
Refactoring