@laanwj Good point. Fixed! :-)

Added commits from #11547 ("Avoid unintended unsigned integer wraparounds in FormatScript(...) and SplitHostPort(...)") as requested :-)

Added a few more wrap-arounds and squashed. Now at 16 fixed wrap-arounds.

We tend to use C-style casts for primitive types... just for brevity.

@sipa I'll change! Other than that, do the changes look reasonable? :-)

@sipa Now using C-style casts for primitive types :-)

Added another wraparound fix (this time in AcceptBlock(…)) and squashed.

Anyone willing to review? :-)

Rebased! :-)

Anyone willing to review - ACK or NACK? :-)

Ping? :-)

Do we not care about integer wrap-arounds? If so let me know and I'll close :-)

ACK test changes, they seem straightforward. The other need a cautious review, since they change behavior.

(You can split the test changes into a separate pull request, if you want)

To increase the likelihood of this PR getting merged – should I limit it to a subset that is low-risk/non-controversial? Please help me find that subset :-)

It would be really nice to get rid of these unintended unsigned integer wraparounds.

Please help me find that subset :-)

@practicalswift see #11535 (comment)

@flack That subset has already been fixed – see #12516 :-)

I don't have any authority here, I can only offer advice how I would approach this.

What do I mean by "this"? I think the code should transition to using signed integers pervasively, everywhere by default, except the places where it needs unsigned integers (such as bit fiddling and modulo-2 arithmetic, a lot of crypto code falls into this category).

I think getting there has to be incremental, and during the transition there will probably be a net increase of signed/unsigned casts. But in the end the whole codebase could be built with -Wsign-conversion -Werror, which would be a great place to be in, and with fewer casts.

@practicalswift I believe you've found the issues you've addressed here with -fsanitize=unsigned, right? I think that's a good tool to find the most egregious misuses of unsigned integers, they should probably be the first ones to go. However, perhaps addressing all of these individual spots in a single commit isn't the right slicing or increment towards the goal of reducing misuses of unsigned integers. Perhaps instead, pick one, and fix that one properly, including all the ripple-effects it may have.

for example, take this part of the patch.

Maybe it would make more sense to explore what would happen if GetTxSize() would return an int64_t (which seems like a cleaner solution than casting its result like this).

If we decide to return sizes and lengths as int64_t, that should probably be documented in the developer notes (coding style) with rationale for doing so. Otherwise it seems like the code could go back and forth over this constantly.

@arvidn Good point regarding splitting this PR into smaller parts. I'll do that.

@arvidn @MarcoFalke Are there any parts of this PR that you can ACK or NACK? I'm trying to identify the appropriate parts to split it in :-)