Fix UBSan warnings triggered when loading corrupt mempool.dat files #19381
Conversation
|
Concept ACK: thanks for fixing! :)
Update: Negative deltas are allowed! |
|
@practicalswift I was thinking along the same line. Also, It seems better to initiate shutdown once something like this happens. As just returning So I made it to initiate shutdown if loadmempool fails. Not sure if that's strictly necessary, looking for suggestions here before I update the PR. The log Also please give suggestions for a better error message. |
|
Update: Negative deltas are allowed! |
8cc8548 to
c58261d
Compare
|
@practicalswift updated to return failure instead of skipping |
|
Update: Negative deltas are allowed! |
|
@practicalswift silly me. Should have done that. will push the same. |
c58261d to
f19dcd4
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
f19dcd4 to
069043e
Compare
|
@practicalswift addressed all your comments and rebased. |
|
@rajarshimaitra I think you still want
Update: Negative deltas allowed. Something along the lines of … ... and ... Also, would you mind also updating the PR title to the more descriptive commit message? :)~~ |
src/validation.cpp
Outdated
| pool.PrioritiseTransaction(i.first, i.second); | ||
| if (!MoneyRange(i.second)) { | ||
| return false; | ||
| } else { |
There was a problem hiding this comment.
| } else { |
nit: No need to change indentation on early-return. (same above)
There was a problem hiding this comment.
Sorry, I didn't get it. Can you explain a bit what are you suggesting here?
There was a problem hiding this comment.
This was fixed in your last push AFAICT :)
|
@practicalswift If i understand you correctly this is what you are suggesting? and Let me know if this seems correct. |
|
Update: Negative deltas are allowed! Also would you mind changing the PR title to |
069043e to
2cc2cbd
Compare
|
Fixed and updated. |
|
Update: Negative deltas are allowed! Restarted Travis which failed spuriously :) |
|
Code review ACK 2cc2cbd |
| if (!MoneyRange(amountdelta)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
There is no requirement for deltas to be in MoneyRange... Better use a numeric_limit max
There was a problem hiding this comment.
Before I push the commit does this look ok?
CAmount amountdelta = nFeeDelta;
if (amountdelta > numeric_limits<int64_t>::max() || amountdelta < numeric_limits<int64_t>::min()) {
return false;
}
There was a problem hiding this comment.
@luke-jr Oh, crap thanks for alerting about that! I naïvely assumed that the bounds of CAmount were defined by MoneyRange(…), but I now understand that this is a case where negative amounts are allowed for a CAmount. I assume the valid range here is [-MAX_MONEY, MAX_MONEY] then?
Should CAmount perhaps be reserved for cases where MoneyRange(…) defines the bounds?
@rajarshimaitra I don't think that is what @luke-jr is suggesting: note that CAmount is int64_t so amountdelta is guaranteed to be in the range you're checking for in the suggested diff ([std:: numeric_limits<int64_t>::min(), std:: numeric_limits<int64_t>::max()]) :)
There was a problem hiding this comment.
Oh, right. Well, this sounds quite deep for my grasp and I wouldn't know what exactly to do. Do suggest and will update the PR accordingly.
|
@rajarshimaitra To allow for negative deltas: what about replacing the NODISCARD bool IsValidFeeDelta(const CAmount fee_delta) {
return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY;
} |
|
🐙 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". |
There was a problem hiding this comment.
Part of this patch is already in ee11a41. The other part I am not sure it fixes the issue. See also #20383 (comment)
| if (amountdelta) { | ||
| pool.PrioritiseTransaction(tx->GetHash(), amountdelta); | ||
| } | ||
| TxValidationState state; | ||
| if (nTime + nExpiryTimeout > nNow) { | ||
| if (nTime > nNow - nExpiryTimeout) { |
|
Closing due to inactivity |
This fixes some undefined behavior observed in the issue #19728.
The bugs can be reproduced by using corpus added here and the harness added in PR#19259