Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Make m_count_with_* in CTxMemPoolEntry int64_t, drop UBSAN supp #27890

Merged
merged 1 commit into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/kernel/mempool_entry.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ struct CompareIteratorByHash {
* ("descendant" transactions).
*
* When a new entry is added to the mempool, we update the descendant state
* (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for
* (m_count_with_descendants, nSizeWithDescendants, and nModFeesWithDescendants) for
* all ancestors of the newly added transaction.
*
*/
Expand Down Expand Up @@ -87,13 +87,13 @@ class CTxMemPoolEntry
// Information about descendants of this transaction that are in the
// mempool; if we remove this transaction we must remove all of these
// descendants as well.
uint64_t nCountWithDescendants{1}; //!< number of descendant transactions
int64_t m_count_with_descendants{1}; //!< number of descendant transactions
// Using int64_t instead of int32_t to avoid signed integer overflow issues.
int64_t nSizeWithDescendants; //!< ... and size
CAmount nModFeesWithDescendants; //!< ... and total fees (all including us)

// Analogous statistics for ancestor transactions
uint64_t nCountWithAncestors{1};
int64_t m_count_with_ancestors{1};
// Using int64_t instead of int32_t to avoid signed integer overflow issues.
int64_t nSizeWithAncestors;
CAmount nModFeesWithAncestors;
Expand Down Expand Up @@ -153,13 +153,13 @@ class CTxMemPoolEntry
lockPoints = lp;
}

uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
uint64_t GetCountWithDescendants() const { return m_count_with_descendants; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; }

bool GetSpendsCoinbase() const { return spendsCoinbase; }

uint64_t GetCountWithAncestors() const { return nCountWithAncestors; }
uint64_t GetCountWithAncestors() const { return m_count_with_ancestors; }
int64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
int64_t GetSigOpCostWithAncestors() const { return nSigOpCostWithAncestors; }
Expand Down
8 changes: 4 additions & 4 deletions src/txmempool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -370,17 +370,17 @@ void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFe
nSizeWithDescendants += modifySize;
assert(nSizeWithDescendants > 0);
nModFeesWithDescendants = SaturatingAdd(nModFeesWithDescendants, modifyFee);
nCountWithDescendants += uint64_t(modifyCount);
assert(int64_t(nCountWithDescendants) > 0);
Comment on lines -373 to -374
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

m_count_with_descendants += modifyCount;
assert(m_count_with_descendants > 0);
}

void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
{
nSizeWithAncestors += modifySize;
assert(nSizeWithAncestors > 0);
nModFeesWithAncestors = SaturatingAdd(nModFeesWithAncestors, modifyFee);
nCountWithAncestors += uint64_t(modifyCount);
assert(int64_t(nCountWithAncestors) > 0);
m_count_with_ancestors += modifyCount;
assert(m_count_with_ancestors > 0);
nSigOpCostWithAncestors += modifySigOps;
assert(int(nSigOpCostWithAncestors) >= 0);
}
Expand Down
1 change: 0 additions & 1 deletion test/sanitizer_suppressions/ubsan
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ unsigned-integer-overflow:hash.cpp
unsigned-integer-overflow:policy/fees.cpp
unsigned-integer-overflow:prevector.h
unsigned-integer-overflow:script/interpreter.cpp
unsigned-integer-overflow:txmempool.cpp
unsigned-integer-overflow:xoroshiro128plusplus.h
implicit-integer-sign-change:compat/stdin.cpp
implicit-integer-sign-change:compressor.h
Expand Down