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

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

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.

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 🙏

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 52dd40a: patch looks correct

Looks like master (at 05757aa) 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 52dd40a