Fix UBSan warnings triggered when loading corrupt mempool.dat files #19381

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

Concept ACK, will review

Concept ACK: thanks for fixing! :)

In case of CAmount:s outside of the money range: should we perhaps let LoadMempool return false on those to indicate that something went (very) wrong (instead of just skipping PrioritiseTransaction)?

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 false in LoadMempool doesn't seem to be triggering any catastrophe, and I am not sure what the node is doing about its mempool when loading from disk fails. The node seems to keep on running with just a failure message, My guess is its creates a fresh mempool and override the data in the disk. Which can seem fine, unless someone specifically wants a particular mempool and if the disk data is corrupted they might not notice that something is wrong.

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

2020-06-27T14:19:55Z Cant load mempool, something went wrong, initiating shutdown.
2020-06-27T14:19:55Z Bound to [::]:18444
2020-06-27T14:19:55Z Bound to 0.0.0.0:18444
2020-06-27T14:19:55Z init message: Loading P2P addresses...
2020-06-27T14:19:55Z Loaded 0 addresses from peers.dat  1ms
2020-06-27T14:19:55Z init message: Starting network threads...
2020-06-27T14:19:55Z net thread start
2020-06-27T14:19:55Z dnsseed thread start
2020-06-27T14:19:55Z 0 addresses found from DNS seeds
2020-06-27T14:19:55Z dnsseed thread exit
2020-06-27T14:19:55Z init message: Done loading
2020-06-27T14:19:55Z addcon thread start
2020-06-27T14:19:55Z opencon thread start
2020-06-27T14:19:55Z torcontrol thread start
2020-06-27T14:19:55Z msghand thread start
2020-06-27T14:19:55Z tor: Thread interrupt
2020-06-27T14:19:55Z opencon thread exit
2020-06-27T14:19:55Z Shutdown: In progress...
2020-06-27T14:19:55Z torcontrol thread exit
2020-06-27T14:19:55Z addcon thread exit
2020-06-27T14:19:55Z net thread exit
2020-06-27T14:19:55Z msghand thread exit
2020-06-27T14:19:55Z scheduler thread exit
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2020-06-27T14:19:55Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2020-06-27T14:19:55Z Shutdown: done

Also please give suggestions for a better error message.

@rajarshimaitra This is such an unlikely error that I don't think we need to add any special logic beyond returning false in case of !MoneyRange(…) :)

Update: Negative deltas are allowed!

@practicalswift updated to return failure instead of skipping PrioritiseTransaction.

@rajarshimaitra Great! What about doing the same thing for the other PrioritiseTransaction call -- that is return false in the case of !MoneyRange(i.second) :)

Update: Negative deltas are allowed!

@practicalswift silly me. Should have done that. will push the same.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

ACK modulo nits

Could perhaps use a more descriptive PR title and commit message: what about something along the lines of "Fix UBSan warnings triggered when loading corrupt mempool.dat files"? :)

@practicalswift addressed all your comments and rebased.

@rajarshimaitra I think you still want if (amountdelta) { before doing pool.PrioritiseTransaction(tx->GetHash(), amountdelta); in order to not change the existing logic.

I suggest leaving the existing PrioritiseTransaction logic intact in both cases, and simply add two stand-alone MoneyRange checks:

Update: Negative deltas allowed.

Something along the lines of …

if (!MoneyRange(amountdelta)) {
    return false;
}
if (amountdelta) {
    pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
}

... and ...

if (!MoneyRange(i.second)) {
    return false;
}
pool.PrioritiseTransaction(i.first, i.second);

Also, would you mind also updating the PR title to the more descriptive commit message? :)~~

@practicalswift If i understand you correctly this is what you are suggesting?

CAmount amountdelta = nFeeDelta;
            if (!MoneyRange(amountdelta)) {
                return false;
            }
            if (amountdelta) {
                pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
            }

and

for (const auto& i : mapDeltas) {
            if (!MoneyRange(i.second)) {
                return false;
            }
            if (i.second) {
                pool.PrioritiseTransaction(i.first, i.second);
            }
        }

Let me know if this seems correct.

@rajarshimaitra Yes for the first case, but for the second case pool.PrioritiseTransaction(i.first, i.second); should be done unconditionally (skip if (i.second)) in order to not change the existing logic :)

Update: Negative deltas are allowed!

Also would you mind changing the PR title to Fix UBSan warnings triggered when loading corrupt mempool.dat files?

Fixed and updated.

ACK XXX

Update: Negative deltas are allowed!

Restarted Travis which failed spuriously :)

Code review ACK 2cc2cbde13072a2d244912aba7df8a97c81646a1

@rajarshimaitra To allow for negative deltas: what about replacing the MoneyRange(…) usages in this PR with IsValidFeeDelta(…) which could be something along the lines of:

NODISCARD bool IsValidFeeDelta(const CAmount fee_delta) {
    return fee_delta >= -MAX_MONEY && fee_delta <= MAX_MONEY;
}

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 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".}

Part of this patch is already in ee11a412a537f62aa46e8862678ce2069a2df5b7. The other part I am not sure it fixes the issue. See also #20383 (comment)

Closing due to inactivity