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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See 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.
The main benefit of this refactor is to drop the file-wide ubsan suppression
unsigned-integer-overflow:txmempool.cpp.
A change in the suppression file is expected :)
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.
152@@ -153,13 +153,13 @@ class CTxMemPoolEntry
153 lockPoints = lp;
154 }
155
156- uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
157+ uint64_t GetCountWithDescendants() const { return m_count_with_descendants; }
In commit “refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp” (fa76f0d0efccd1ea272a46060022eea3e998268e)
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.
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.
369@@ -370,17 +370,17 @@ void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFe
370 nSizeWithDescendants += modifySize;
371 assert(nSizeWithDescendants > 0);
372 nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee);
373- nCountWithDescendants += uint64_t(modifyCount);
374- assert(int64_t(nCountWithDescendants) > 0);
In commit “refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp” (fa76f0d0efccd1ea272a46060022eea3e998268e)
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 (https://github.com/bitcoin/bitcoin/pull/23962#discussion_r1218400476). Clearer to just use signed numbers.
