Remove UBSan suppressions for CTxMemPool* #17791
Conversation
|
Concept ACK |
ed1a916 to
7aee08c
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Nit regarding the commit message "Fix UB in CTxMemPool*": Unsigned integer wraparound and implicit integer sign change are not UB, but they are often unintentional - that is why UBSan is flagging them :) Perhaps the PR title ("Remove UBSan suppressions for CTxMemPool*") could be used as commit message instead?
|
7aee08c to
52a45c3
Compare
Agree with every word ;) |
|
ACK 52a45c3 -- diff looks correct |
| @@ -217,10 +217,11 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors | |||
| UpdateChild(piter, it, add); | |||
| } | |||
| const int64_t updateCount = (add ? 1 : -1); | |||
| const int64_t updateSize = updateCount * it->GetTxSize(); | |||
| const int64_t updateSize = it->GetTxSize(); | |||
| assert(updateSize >= 0); | |||
There was a problem hiding this comment.
nit: this is now the size of the tx, not the update
Maybe assert against the it->GetTxSize directly? Would mean just adding one line here. If you're concerned about the repeated calls, something like this: Empact@0018dd1 would make them trivial.
|
Concept ACK |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
In
CTxMemPoolEntryclass the type of four data members has been changed fromuint64_ttoint64_t. The type of getters,uint64_t, remains unchanged.In
CTxMemPool::UpdateAncestorsOf()the only expression was needed to be reworked (the result looks a bit ugly, I'm still open for suggestions).