Remove txmempool implicit-integer-sign-change sanitizer suppressions #23957

1. 
   MarcoFalke commented at 1:58 pm on January 3, 2022: member

   A file-wide suppression is problematic because it will wave through future violations, potentially bugs.

   Fix that by using per-statement casts.

   This refactor doesn’t change behavior because the now explicit casts were previously done implicitly.

   Similar to commit 8b5a4de904b414fb3a818732cd0a2c90b91bc275

2. Remove txmempool implicit-integer-sign-change sanitizer suppressions fa4314f1e6
3. MarcoFalke added the label Refactoring on Jan 3, 2022
4. 
   DrahtBot commented at 2:34 pm on January 3, 2022: 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:

   • #23629 (refactor tests to fix ubsan suppressions by MarcoFalke)
   • #21464 (Mempool Update Cut-Through Optimization by JeremyRubin)
   • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

   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.

5. 
   hebasto commented at 2:40 pm on January 3, 2022: member

   Could we just avoid conversion from int64_t to size_t in https://github.com/bitcoin/bitcoin/blob/75a227e39e37d475d6088209f24f32c070071219/src/txmempool.cpp#L114-L117 and return int64_t?
6. 
   MarcoFalke commented at 3:05 pm on January 3, 2022: member

   Yes, potentially. Though, the goal of this pull is to keep all integer widths and types the same, only make implicit conversions explicit for the reasons mentioned in OP. Also, changing the return type of GetTxSize won’t fix the implicit conversion for modifySize, which is already int64_t, so I think the changes here are needed either way.
7. 
   MarcoFalke commented at 3:06 pm on January 3, 2022: member

   I’ve checked that the binary doesn’t change with clang -O2 on my system. (It does change with gcc, seemingly because functions are for some reason re-ordered in the object file).
8. 
   hebasto commented at 3:16 pm on January 3, 2022: member

   Yes, potentially. Though, the goal of this pull is to keep all integer widths and types the same, only make implicit conversions explicit for the reasons mentioned in OP.

   A conversion sequence int64_t –> size_t –> int64_t looks unneeded, doesn’t it? Maybe it deserves its own PR?

   Also, changing the return type of GetTxSize won’t fix the implicit conversion for modifySize, which is already int64_t, so I think the changes here are needed either way.

   Sure. ACK on uint64_t(modifySize) and uint64_t(modifyCount).

9. 
   MarcoFalke commented at 3:48 pm on January 3, 2022: member

   Maybe it deserves its own PR?

   Yes, this should be done in another PR. I briefly looked into this, but this touches policy and miner as well. (It is a bit of a can of worms like #23633)

10. hebasto approved
11. 
    hebasto commented at 3:51 pm on January 3, 2022: member

    ACK fa4314f1e62526b4d576b048d6e8a0004fea0c83, I have reviewed the code and it looks OK, I agree it can be merged.
12. 
    hebasto commented at 5:59 pm on January 3, 2022: member

    Maybe it deserves its own PR?

    Yes, this should be done in another PR.

    Done in #23962.

13. shaavan approved
14. 
    shaavan commented at 6:21 am on January 4, 2022: contributor

    ACK fa4314f1e62526b4d576b048d6e8a0004fea0c83

    • I agree with the motivation of this PR.

    A file-wide suppression is problematic because it will wave through future violations, potentially bugs.

    • Checked that each variable is properly typecasted explicitly.
    • Since the line implicit-integer-sign-change:txmempool.cpp has been removed from santizer_supression/urban list, and the CI passed successfully, that means as of now, there are no more variables in txmempool.cpp that requires explicit type conversion.
15. 
    MarcoFalke commented at 9:33 am on January 4, 2022: member

    Done in #23962.

    Thanks, closing this for now to avoid merge conflicts.

16. MarcoFalke closed this on Jan 4, 2022

    ---

17. MarcoFalke deleted the branch on Jan 4, 2022
18. DrahtBot locked this on Jan 4, 2023

Contributors
MarcoFalke DrahtBot hebasto shaavan

Labels
Refactoring