net, refactor: pass uint16 CService::port as uint16 #21328

As noticed during review today in #20685 (review) of the upcoming I2P network support, CService::port is uint16_t but is passed around the codebase and into the ctors as int, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.

Just discovered #8394, will check if it contains anything I've overlooked.

ACK f8b1c2140eea958ab0e6f42ef2daab7000375d82

I think this PR is good and safe as it is. Some non-blocker comments below. It would be even better if it adds some checks that ports given by the user are in range, like in https://github.com/bitcoin/bitcoin/pull/8394/files#diff-134ccc4e5308f84fb9c03a9a1e1599209560370e49e30c7f545fa7b9ba7e07deR153

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #20457 (util: Make Parse{Int,UInt}{32,64} use locale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull} by practicalswift)
• #20452 (util: Replace use of locale dependent atoi(…) with locale-independent std::from_chars(…) (C++17) by practicalswift)
• #20196 (net: fix GetListenPort() to derive the proper port by vasild)
• #18061 (util: Pass size to ParseHex to assist preallocation by elichai)

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.

Strong Concept ACK

Thanks for removing this implicit conversion gotcha :)

uint16_t should be used consistently for port numbers.

Somewhat related: #19415 (comment)

I like the idea of using specific types. But if we're going to change this, I think I would feel slightly better defining a typedef Port for this. What if we want to support a protocol in the future with 32-bit ports? I do not see this as likely… my intuition is that things are moving to one-endpoint-per-address conceptually instead of ports… and yes the port as a concept is typical to current Internet protocols, where they're all 16 bits. But I guess it's clearer too to have a named type.

Anyhow concept ACK.

Thank you @vasild, @practicalswift and @laanwj. Updated per all of your suggestions.

Not super excited by the introduction of Port TBH.

The problem is probably best illustrated by this UB quiz:

Q. Which of the two cases rely on undefined behaviour?

// Snippet 1
Port p;
if (p != other_p) {
}

// Snippet 2
uint16_t p;
if (p != other_p) {
}

<details> <summary>Click for answer</summary>

Both snippets rely on undefined behaviour (they are technically equivalent thanks to the typedef).

But that's not the point.

The point is that while the UB in snippet 2 is immediately clear (read of uninitialized uint16_t) the UB in snippet 1 requires looking up information that is not available in the code snippet (more specifically the typedef).

</details>

Trying to make it as easy as possible to spot unsafe code is an important part of safe coding practice.

Personally I find that the introduction of Port makes it somewhat harder to spot unsafe code as illustrated above.

ACK 9b7055f9a9bd7345fc7dfb51ed0bace8257e07cd

Notice that this is not the last commit - I tend to agree with @practicalswift wrt uint16_t vs Port so I ACK the commit before those changes. Two more things to consider about Port:

• The name Port may be too generic. I can imagine that "port" in some other context means something else, e.g. "usb port for connecting to a hardware wallet". Maybe IPPort would be a better name.
• At one place we do Port n; ParseUInt16(..., &n); which kind of defeats the purpose of the Port abstraction. I don't see IP ports ever changing to something other than 16 bit unsigned integers.

Thanks for the feedback!

I agree that Port is less greppable than IPPort (and we do already have a ToStringIPPort() method).

No strong opinion, but here are a few reasons why I tend to agree with @laanwj and took the time to do it:

• ISTM not having a type alias here is why we're in this situation in the first place (and since a long time)
• I'm happy that we have an alias for CAmount, for example; it makes clear to contributors which type should be used for amounts and the same would be true for port id numbers
• This was a somewhat tedious change to make and I don't plan to do it again, take advantage of it ;)
• I think we'll be happier down the road if this is done than if we don't do it

I'm not sure whether to wait for more feedback or separate the type alias to another pull. Keep the feedback coming :pray:

Yeah, those are also valid points :scroll: :thinking: :thought_balloon:

If a new name is introduce, wouldn't it be better to make it fully type safe?

Q. Which of the two cases rely on undefined behaviour?

Don't disagree with the point but whyy does C have to turn everything into UB, I hate this, I really do, wish we could use a sane language

I agree with MarcoFalke: if we are to introduce Port consider making it fully type safe in order to avoid the pitfall I described above.

Portability is important, but safety is more important (in general) :)

I suggest moving the Port commit to a separate PR since that is the only part of this PR that is not ready for merge.

Q. Which of the two cases rely on undefined behaviour?

Don't disagree with the point but whyy does C have to turn everything into UB, I hate this, I really do, wish we could use a sane language

Heh, just don't shoot the messenger! :)

ACK cf9952da0528b22af77c169822777587bb2a87ee

If in doubt - KISS.

Thanks @vasild! Yes, dropped three commits: the logging one, and the two for the type alias that could be a follow-up. No other changes.

Rebased following the merge of #19415 -- git range-diff eceb3f77 cf9952d ae6eb4c

ACK ae6eb4c8d549526232bcf6197a31de660b74b8af

cr ACK ae6eb4c8d549526232bcf6197a31de660b74b8af: patch looks correct

Rebased following merge of #21444 -- git range-diff 01bb3afb ae6eb4c a05736

ACK a0573615cb31670743b3915135e3a03418fb9629

Ah, the fuzz CI is signalling a needed change since the merge of #19415: "UndefinedBehaviorSanitizer: implicit-signed-integer-truncation test/fuzz/netbase_dns_lookup.cpp:48:45". Will update.

Thanks @vasild! Updated one line per git diff a057361 52dd40a to make a change needed after the merge of #19415.

+++ b/src/test/fuzz/netbase_dns_lookup.cpp
@@ -18,7 +18,7 @@ FUZZ_TARGET(netbase_dns_lookup)
     const std::string name = fuzzed_data_provider.ConsumeRandomLengthString(512);
     const unsigned int max_results = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
     const bool allow_lookup = fuzzed_data_provider.ConsumeBool();
-    const int default_port = fuzzed_data_provider.ConsumeIntegral<int>();
+    const uint16_t default_port = fuzzed_data_provider.ConsumeIntegral<uint16_t>();

cr ACK 52dd40a9febec1f4e70d777821b6764830bdec61: patch looks correct

ACK 52dd40a9febec1f4e70d777821b6764830bdec61

Looks like master (at 05757aa86) has a problem that would be fixed by this PR - my "private" CI got this error:

netbase.cpp:212:37: runtime error: implicit conversion from type 'int' of value -2147483632 (32-bit, signed) to type 'uint16_t' (aka 'unsigned short') changed the value to 16 (16-bit, unsigned)

which seems unrelated to the changes I am testing there.

Edit: also in another PR: https://cirrus-ci.com/task/6488952220680192?command=ci#L3334

@vasild Conversions to unsigned types are actually well-defined in the lamguage. Our sanitizer runs in sort of an over-eager mode and triggers on that because such conversions are often a sign of bugs. But here I suspect it's intentional, and valid.

@sipa, I agree. However, a failing CI is a problem :) it would be fixed either by this PR or an explicit cast like CService(vIP[i], static_cast<uint16_t>(port)).

cr ACK 52dd40a9febec1f4e70d777821b6764830bdec61