Fix unsigned integer overflow in LoadMempool #17505

As reported by UB sanitizer:

validation.cpp:4958:19: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'uint64_t' (aka 'unsigned long long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp

Just throwing this out in case we want to shut up this UB error.

This sounds much scarier than it is. Can you please at least add "potential" or even "theoretical" to your commit message.

Is there even a way to trigger a bug with this? I'd say the 0 case works because it's a post-decrement. It won't enter the while loop in that case? And num is never used after the loop.

If not, NACK, I don't think "shutting up UB errors" from every kind of tooling is a goal in itself. AFAIK, wrapping around unsigned integers isn't even UB, only signed ones.

I was expecting a response like that, I also think the code is safe as is, agree @practicalswift?

@promag

Agreed :)

Unsigned integer wraparound is defined: so no problem from a standards perspective :)

With that said unsigned integer wraparounds can still be a source of errors: sometimes the wraparound is not intended which can lead to bugs. That could be a good reason to try to limit the wraparounds in all code except where the wraparound is intentional (hashing code is a good example for where one would expect intentional wraparounds).

These are the places where we are currently expecting/suppressing wraparounds:

https://github.com/bitcoin/bitcoin/blob/397c6d32c8f8a20a3605ef0d51d159adc21fd125/test/sanitizer_suppressions/ubsan#L9-L44

Unfortunately UBSan is a bit misleading in its warning message: from the warning message one gets the impression that unsigned wraparounds are scarier than they typically are in practice :)

I think there is an UBSAN_OPTIONS setting that makes the warning message slightly less misleading: will investigate :)

FWIW: check #17511 (review) for a recent example of an unintended unsigned integer wraparound caught by -fsanitize=integer. Defined behaviour, but unintended and thus worth flagging :)