ACK c7d461b014422f1aaa13ae978d730864616ab7bb

Code looks correct, so code review ACK c7d461b014422f1aaa13ae978d730864616ab7bb

As stated in #21682 (comment) I have a slightly preference towards not including scope id in CNetAddr::ToStringIP() out. That is how Rust's std::net::Ipv6Addr work. That said: if others feels strongly about including scope id then I'm fine with that too. The important thing for me is that IPv6 addresses are printed identically across platforms and that goal has been achieved :)

Also it is probably worth noting that the requirements for parsing and printing IPv6 addresses are not the same.

@practicalswift Thank you for the review! I do not entirely understand your insistence on making our IPv6 formatting function the same as that of rust's IPv6 address type which is a different animal. It doesn't include a scope id, so cannot include it for formatting. In rust, the scope_id is part of SocketAddrV6. When formatted as string it uses the % syntax, just like us here:

use std::net::{Ipv6Addr, SocketAddrV6};

fn main() {
    let socket = SocketAddrV6::new(Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 1), 8080, 0, 32);
    assert_eq!("[2001:db8::1%32]:8080".parse(), Ok(socket));
}

Also it is probably worth noting that the requirements for parsing and printing IPv6 addresses are not the same.

In any case I like to have the roundtrip here. The scope_id is part of the type so it makes some sense to be part of the string representation. This is clearly also useful for testing. Without the test, the scope id might as well be ignored by the parser 🙂

@laanwj

Thanks for providing the Rust SocketAddrV6 string representation example. I see your point regarding it being different animals, and I guess this entire discussion boils down to if the expectation is that CNetAddr::ToStringIP() should print the IPv6 address or the IPv6 socket address:

• IPv6 address (std::net::Ipv6Addr): "IPv6 addresses are defined as 128-bit integers in IETF RFC 4291. They are usually represented as eight 16-bit segments."
• IPv6 socket address (std::net::SocketAddrV6): "IPv6 socket addresses consist of an IPv6 address, a 16-bit port number, as well as fields containing the traffic class, the flow label, and a scope identifier (see IETF RFC 2553, Section 3.3 for more details)."

Is that a fair summary? :)

For better or worse our data representation has an intermediate nesting level that doesn't neatly fit into that division.
CService is the full "socket address" [addres + scope_id] + port, CNetAddr is address + scope_id.

As this is our data representation I think the string representation should reflect this.

Updated the test for @vasild's comment.
c7d461b014422f1aaa13ae978d730864616ab7bb..6c280adcd865ae3da4df53d630c9bf737283a56f

cr ACK 6c280ad

@laanwj

[…] I guess this entire discussion boils down to if the expectation is that CNetAddr::ToStringIP() should print the IPv6 address or the IPv6 socket address:

• IPv6 address (std::net::Ipv6Addr): "IPv6 addresses are defined as 128-bit integers in IETF RFC 4291. They are usually represented as eight 16-bit segments."
• IPv6 socket address (std::net::SocketAddrV6): "IPv6 socket addresses consist of an IPv6 address, a 16-bit port number, as well as fields containing the traffic class, the flow label, and a scope identifier (see IETF RFC 2553, Section 3.3 for more details)."

For better or worse our data representation has an intermediate nesting level that doesn't neatly fit into that division.
CService is the full "socket address" [addres + scope_id] + port, CNetAddr is address + scope_id.

As this is our data representation I think the string representation should reflect this.

OK, I can see that point and I'm fine with that :)

Post-merge ACK 6c280ad

addr.ToString() was previously memoized because it was used twice, once for checking the behavior on most platforms and a second time for the different behavior exhibited by macOS. We now only need it once, so good idea to simplify it.

git diff c7d461b 6c280ad

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index c0d3c3196e..7a122bd8b0 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -309,8 +309,7 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
     BOOST_REQUIRE(addr.IsValid());
     BOOST_REQUIRE(addr.IsIPv6());
     BOOST_CHECK(!addr.IsBindAny());
-    const std::string addr_str{addr.ToString()};
-    BOOST_CHECK(addr_str == scoped_addr);
+    BOOST_CHECK_EQUAL(addr.ToString(), scoped_addr);