net: Return IPv6 scope id in `CNetAddr::ToStringIP()`

laanwj commented at 3:09 pm on May 17, 2021: member

If a scope id is provided, return it back in the string representation. Also bring back the test (now in platform independent fashion). Closes #21982. Includes #21961 (apart from the MacOS remark).

laanwj force-pushed on May 17, 2021

laanwj requested review from jonatack on May 17, 2021

laanwj requested review from practicalswift on May 17, 2021

laanwj force-pushed on May 17, 2021

in src/test/net_tests.cpp:303 in 90972a8962 outdated

300@@ -301,12 +301,18 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
301     // IPv6, scoped/link-local. See https://tools.ietf.org/html/rfc4007
302     // We support non-negative decimal integers (uint32_t) as zone id indices.
303     // Test with a fairly-high value, e.g. 32, to avoid locally reserved ids.

jonatack commented at 3:36 pm on May 17, 2021:

nit, can drop this line as we are no longer concerned about collisions with locally reserved zone ids

laanwj commented at 4:15 pm on May 17, 2021:

I wasn’t sure whether this was a problem for parsing or for formatting, that’s why I kept it. But makes sense, will remove it.

jonatack commented at 3:55 pm on May 17, 2021: member

ACK 90972a8962cfd9bf63f3b982f5a85fcdc41f253d according to https://datatracker.ietf.org/doc/html/rfc4007#section-11.2 and https://en.wikipedia.org/wiki/IPv6_address#zone_index numeric zone indices like these (% + uint32, default 0 that can be omitted) should be mandatory/universally supported

DrahtBot added the label P2P on May 17, 2021

laanwj force-pushed on May 17, 2021

jonatack commented at 6:06 pm on May 17, 2021: member

ACK c7d461b014422f1aaa13ae978d730864616ab7bb

practicalswift commented at 6:59 pm on May 17, 2021: contributor

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.

laanwj commented at 7:46 pm on May 17, 2021: member

@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:

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

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 :slightly_smiling_face:

practicalswift commented at 9:01 pm on May 17, 2021: contributor

@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? :)

laanwj commented at 6:13 am on May 18, 2021: member

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.

in src/test/net_tests.cpp:313 in c7d461b014 outdated

309     BOOST_REQUIRE(LookupHost(scoped_addr, addr, false));
310     BOOST_REQUIRE(addr.IsValid());
311     BOOST_REQUIRE(addr.IsIPv6());
312     BOOST_CHECK(!addr.IsBindAny());
313+    const std::string addr_str{addr.ToString()};
314+    BOOST_CHECK(addr_str == scoped_addr);

vasild commented at 12:53 pm on May 18, 2021:

BOOST_CHECK_EQUAL() would print both values in case of failure. The extra variable addr_str looks like just clutter to me.

0    BOOST_CHECK_EQUAL(addr.ToString(), scoped_addr);

laanwj commented at 3:50 pm on May 18, 2021:

FWIW, I’ve simply restored this code from git history. But sure I’m fine with updating it.

vasild approved

vasild commented at 12:57 pm on May 18, 2021: member

ACK c7d461b014422f1aaa13ae978d730864616ab7bb

If the zone was (mostly) printed before (by getnameinfo()), keep printing it.

Twan1409 commented at 1:02 pm on May 18, 2021: none

Ok

Twan1409 changes_requested

Twan1409 commented at 1:03 pm on May 18, 2021: none

Ok

net: Return IPv6 scope id in `CNetAddr::ToStringIP()`

If a scope id is provided, return it back in the string representation.
Also bring back the test. Closes #21982.

Co-authored-by: Jon Atack <jon@atack.com>

6c280adcd8

laanwj commented at 7:08 pm on May 18, 2021: member

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

laanwj force-pushed on May 18, 2021

practicalswift commented at 9:23 pm on May 18, 2021: contributor

cr ACK 6c280adcd865ae3da4df53d630c9bf737283a56f @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 :)

laanwj merged this on May 19, 2021

laanwj closed this on May 19, 2021

jonatack commented at 7:35 am on May 19, 2021: member

Post-merge ACK 6c280adcd865ae3da4df53d630c9bf737283a56f

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

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

sidhujag referenced this in commit b2255c1f29 on May 19, 2021

gwillen referenced this in commit 1cb298ae73 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

net: Return IPv6 scope id in CNetAddr::ToStringIP() #21985

net: Return IPv6 scope id in `CNetAddr::ToStringIP()` #21985