refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp #27890
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Concept ACK.
A change in the suppression file is expected :) |
…supp This is a refactor as long as no signed integer overflow appears. In normal operation and absent bugs, signed integer overflow should never happen in the touched code paths. The main benefit of this refactor is to drop the file-wide ubsan suppression unsigned-integer-overflow:txmempool.cpp. For now, this only changes the internal private representation and the publicly returned type remains uint64_t.
maflcko
force-pushed
the
2306-ubsan-txmempool-
branch
from
fa01973
to
fa76f0d
Compare
| @@ -153,13 +153,13 @@ class CTxMemPoolEntry | |||
| lockPoints = lp; | |||
| } | |||
|
|
|||
| uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } | |||
| uint64_t GetCountWithDescendants() const { return m_count_with_descendants; } | |||
There was a problem hiding this comment.
In commit "refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp" (fa76f0d)
Maybe add a comment that this is intentionally returning uint64_t instead of int64_t for backwards compatibility.
Otherwise it might not be clear that a conversion is happening intentionally or which of the two types is wrong.
There was a problem hiding this comment.
Otherwise it might not be clear that a conversion is happening intentionally or which of the two types is wrong.
I don't think any type is wrong, because the underlying (and thus returned) value is never negative. So the result should always be the same, regardless of the return type. Anyone should be free to pick whatever type they want. It is just that I didn't want to change it here, because it may cause warnings in other code, see #23962 (comment), which I think shouldn't have been changed either, on a second thought.
| nCountWithDescendants += uint64_t(modifyCount); | ||
| assert(int64_t(nCountWithDescendants) > 0); |
There was a problem hiding this comment.
In commit "refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp" (fa76f0d)
Nice to see these casts go away. I thought it was confusing for this to intentionally a cast negative value to be unsigned and then rely on the way unsigned math wraps around (#23962 (comment)). Clearer to just use signed numbers.
|
ACK fa76f0d |
This is a refactor as long as no signed integer overflow appears. In normal operation and absent bugs, signed integer overflow should never happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan suppression
unsigned-integer-overflow:txmempool.cpp.For now, this only changes the internal private representation and the publicly returned type remains
uint64_t.