Merge #19951: net, test: CNetAddr scoped ipv6 test coverage, rename scopeId to m_scope_id

laanwj · laanwj · commit d9935222d53a · 2020-10-02T17:06:59.000+02:00
f36887f net: rename CNetAddr scopeId to m_scope_id, improve code doc (Jon Atack) 5cb5fd3 test: add test coverage for CNetAddr ipv6 scoped addresses (Jon Atack) Pull request description: Add some test coverage for the IPv6 scoped address feature in `netaddress`/`netbase` per https://tools.ietf.org/html/rfc4007, update the member name `scopeId` to `m_scope_id` per `doc/developer-notes.md`, and improve the code doc. ---- Reviewers can manually verify the test with these steps: - [pull down the changes locally and check out this branch](https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core#pull-down-the-code-locally) - [build Bitcoin Core](https://jonatack.github.io/articles/how-to-compile-bitcoin-core-and-run-the-tests) - run the test: `cd src ; test/test_bitcoin -t net_tests/cnetaddr_basic -l all` <details><summary><em>you should see passing test output like this</em></summary><p> ``` Running 1 test case... Entering test module "Bitcoin Core Test Suite" test/net_tests.cpp(85): Entering test suite "net_tests" test/net_tests.cpp(204): Entering test case "cnetaddr_basic" test/net_tests.cpp(210): info: check LookupHost("0.0.0.0", addr, false) has passed test/net_tests.cpp(211): info: check !addr.IsValid() has passed test/net_tests.cpp(212): info: check addr.IsIPv4() has passed test/net_tests.cpp(214): info: check addr.IsBindAny() has passed test/net_tests.cpp(215): info: check addr.ToString() == "0.0.0.0" has passed test/net_tests.cpp(218): info: check LookupHost("255.255.255.255", addr, false) has passed test/net_tests.cpp(219): info: check !addr.IsValid() has passed test/net_tests.cpp(220): info: check addr.IsIPv4() has passed test/net_tests.cpp(222): info: check !addr.IsBindAny() has passed test/net_tests.cpp(223): info: check addr.ToString() == "255.255.255.255" has passed test/net_tests.cpp(226): info: check LookupHost("12.34.56.78", addr, false) has passed test/net_tests.cpp(227): info: check addr.IsValid() has passed test/net_tests.cpp(228): info: check addr.IsIPv4() has passed test/net_tests.cpp(230): info: check !addr.IsBindAny() has passed test/net_tests.cpp(231): info: check addr.ToString() == "12.34.56.78" has passed test/net_tests.cpp(234): info: check LookupHost("::", addr, false) has passed test/net_tests.cpp(235): info: check !addr.IsValid() has passed test/net_tests.cpp(236): info: check addr.IsIPv6() has passed test/net_tests.cpp(238): info: check addr.IsBindAny() has passed test/net_tests.cpp(239): info: check addr.ToString() == "::" has passed test/net_tests.cpp(242): info: check LookupHost(ipv6_addr, addr, false) has passed test/net_tests.cpp(243): info: check addr.IsValid() has passed test/net_tests.cpp(244): info: check addr.IsIPv6() has passed test/net_tests.cpp(246): info: check !addr.IsBindAny() has passed test/net_tests.cpp(247): info: check addr.ToString() == ipv6_addr has passed test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed test/net_tests.cpp(266): info: check LookupHost(ipv6_addr + "%0", addr, false) has passed test/net_tests.cpp(267): info: check addr.IsValid() has passed test/net_tests.cpp(268): info: check addr.IsIPv6() has passed test/net_tests.cpp(269): info: check !addr.IsBindAny() has passed test/net_tests.cpp(270): info: check addr.ToString() == ipv6_addr has passed test/net_tests.cpp(274): info: check addr.IsValid() has passed test/net_tests.cpp(275): info: check addr.IsTor() has passed test/net_tests.cpp(277): info: check !addr.IsBindAny() has passed test/net_tests.cpp(278): info: check addr.ToString() == "6hzph5hv6337r6p2.onion" has passed test/net_tests.cpp(282): info: check !addr.IsValid() has passed test/net_tests.cpp(283): info: check addr.IsInternal() has passed test/net_tests.cpp(285): info: check !addr.IsBindAny() has passed test/net_tests.cpp(286): info: check addr.ToString() == "esffpvrt3wpeaygy.internal" has passed test/net_tests.cpp(204): Leaving test case "cnetaddr_basic"; testing time: 30933us test/net_tests.cpp(85): Leaving test suite "net_tests"; testing time: 30975us Leaving test module "Bitcoin Core Test Suite"; testing time: 31169us *** No errors detected ``` </p>/</details> - change this line in the code to break the feature: ```diff --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -716,7 +716,7 @@ bool CService::GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const memset(paddrin6, 0, *addrlen); if (!GetIn6Addr(&paddrin6->sin6_addr)) return false; - paddrin6->sin6_scope_id = m_scope_id; + // paddrin6->sin6_scope_id = m_scope_id; ``` - rebuild, e.g. `make` - run the test: `test/test_bitcoin -t net_tests/cnetaddr_basic -l all`, verify that the added tests break <details><summary><em>you should see test output with a few failed tests like this</em></summary><p> ``` Running 1 test case... Entering test module "Bitcoin Core Test Suite" test/net_tests.cpp(85): Entering test suite "net_tests" test/net_tests.cpp(204): Entering test case "cnetaddr_basic" test/net_tests.cpp(210): info: check LookupHost("0.0.0.0", addr, false) has passed test/net_tests.cpp(211): info: check !addr.IsValid() has passed test/net_tests.cpp(212): info: check addr.IsIPv4() has passed test/net_tests.cpp(214): info: check addr.IsBindAny() has passed test/net_tests.cpp(215): info: check addr.ToString() == "0.0.0.0" has passed test/net_tests.cpp(218): info: check LookupHost("255.255.255.255", addr, false) has passed test/net_tests.cpp(219): info: check !addr.IsValid() has passed test/net_tests.cpp(220): info: check addr.IsIPv4() has passed test/net_tests.cpp(222): info: check !addr.IsBindAny() has passed test/net_tests.cpp(223): info: check addr.ToString() == "255.255.255.255" has passed test/net_tests.cpp(226): info: check LookupHost("12.34.56.78", addr, false) has passed test/net_tests.cpp(227): info: check addr.IsValid() has passed test/net_tests.cpp(228): info: check addr.IsIPv4() has passed test/net_tests.cpp(230): info: check !addr.IsBindAny() has passed test/net_tests.cpp(231): info: check addr.ToString() == "12.34.56.78" has passed test/net_tests.cpp(234): info: check LookupHost("::", addr, false) has passed test/net_tests.cpp(235): info: check !addr.IsValid() has passed test/net_tests.cpp(236): info: check addr.IsIPv6() has passed test/net_tests.cpp(238): info: check addr.IsBindAny() has passed test/net_tests.cpp(239): info: check addr.ToString() == "::" has passed test/net_tests.cpp(242): info: check LookupHost(ipv6_addr, addr, false) has passed test/net_tests.cpp(243): info: check addr.IsValid() has passed test/net_tests.cpp(244): info: check addr.IsIPv6() has passed test/net_tests.cpp(246): info: check !addr.IsBindAny() has passed test/net_tests.cpp(247): info: check addr.ToString() == ipv6_addr has passed test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): error: in "net_tests/cnetaddr_basic": check addr.ToString() == scoped_addr has failed [1122:3344:5566:7788:9900:aabb:ccdd:eeff != 1122:3344:5566:7788:9900:aabb:ccdd:eeff%1] test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): error: in "net_tests/cnetaddr_basic": check addr.ToString() == scoped_addr has failed [1122:3344:5566:7788:9900:aabb:ccdd:eeff != 1122:3344:5566:7788:9900:aabb:ccdd:eeff%21] test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): error: in "net_tests/cnetaddr_basic": check addr.ToString() == scoped_addr has failed [1122:3344:5566:7788:9900:aabb:ccdd:eeff != 1122:3344:5566:7788:9900:aabb:ccdd:eeff%19] test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed test/net_tests.cpp(255): info: check addr.IsValid() has passed test/net_tests.cpp(256): info: check addr.IsIPv6() has passed test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed test/net_tests.cpp(258): error: in "net_tests/cnetaddr_basic": check addr.ToString() == scoped_addr has failed [1122:3344:5566:7788:9900:aabb:ccdd:eeff != 1122:3344:5566:7788:9900:aabb:ccdd:eeff%3] test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed test/net_tests.cpp(266): info: check LookupHost(ipv6_addr + "%0", addr, false) has passed test/net_tests.cpp(267): info: check addr.IsValid() has passed test/net_tests.cpp(268): info: check addr.IsIPv6() has passed test/net_tests.cpp(269): info: check !addr.IsBindAny() has passed test/net_tests.cpp(270): info: check addr.ToString() == ipv6_addr has passed test/net_tests.cpp(274): info: check addr.IsValid() has passed test/net_tests.cpp(275): info: check addr.IsTor() has passed test/net_tests.cpp(277): info: check !addr.IsBindAny() has passed test/net_tests.cpp(278): info: check addr.ToString() == "6hzph5hv6337r6p2.onion" has passed test/net_tests.cpp(282): info: check !addr.IsValid() has passed test/net_tests.cpp(283): info: check addr.IsInternal() has passed test/net_tests.cpp(285): info: check !addr.IsBindAny() has passed test/net_tests.cpp(286): info: check addr.ToString() == "esffpvrt3wpeaygy.internal" has passed test/net_tests.cpp(204): Leaving test case "cnetaddr_basic"; testing time: 32316us test/net_tests.cpp(85): Leaving test suite "net_tests"; testing time: 32354us Leaving test module "Bitcoin Core Test Suite"; testing time: 32522us *** 4 failures are detected in the test module "Bitcoin Core Test Suite" ``` </p></details> - leave your review here ACKs for top commit: laanwj: ACK f36887f Tree-SHA512: 8e77e512db130642be7d3a910735ca803a2afdb5a704f713f163f8b5a80be3077a2be6f0a3ca43d299731dcd2545ac35571f8c74e5250a72a48233c26f9a3ab5
diff --git a/src/netaddress.cpp b/src/netaddress.cpp
@@ -281,7 +281,7 @@ CNetAddr::CNetAddr(const struct in_addr& ipv4Addr)
 CNetAddr::CNetAddr(const struct in6_addr& ipv6Addr, const uint32_t scope)
 {
     SetLegacyIPv6(Span<const uint8_t>(reinterpret_cast<const uint8_t*>(&ipv6Addr), sizeof(ipv6Addr)));
-    scopeId = scope;
+    m_scope_id = scope;
 }
 
 bool CNetAddr::IsBindAny() const
@@ -918,7 +918,7 @@ bool CService::GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const
         memset(paddrin6, 0, *addrlen);
         if (!GetIn6Addr(&paddrin6->sin6_addr))
             return false;
-        paddrin6->sin6_scope_id = scopeId;
+        paddrin6->sin6_scope_id = m_scope_id;
         paddrin6->sin6_family = AF_INET6;
         paddrin6->sin6_port = htons(port);
         return true;
diff --git a/src/netaddress.h b/src/netaddress.h
@@ -130,7 +130,11 @@ class CNetAddr
          */
         Network m_net{NET_IPV6};
 
-        uint32_t scopeId{0}; // for scoped/link-local ipv6 addresses
+        /**
+         * Scope id if scoped/link-local IPV6 address.
+         * See https://tools.ietf.org/html/rfc4007
+         */
+        uint32_t m_scope_id{0};
 
     public:
         CNetAddr();
@@ -388,7 +392,7 @@ class CNetAddr
                     "Address too long: %u > %u", address_size, MAX_ADDRV2_SIZE));
             }
 
-            scopeId = 0;
+            m_scope_id = 0;
 
             if (SetNetFromBIP155Network(bip155_net, address_size)) {
                 m_addr.resize(address_size);
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
@@ -246,6 +246,25 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
     BOOST_CHECK(!addr.IsBindAny());
     BOOST_CHECK_EQUAL(addr.ToString(), "1122:3344:5566:7788:9900:aabb:ccdd:eeff");
 
+    // IPv6, scoped/link-local. See https://tools.ietf.org/html/rfc4007
+    // We support non-negative decimal integers (uint32_t) as zone id indices.
+    // Test with a fairly-high value, e.g. 32, to avoid locally reserved ids.
+    const std::string link_local{"fe80::1"};
+    const std::string scoped_addr{link_local + "%32"};
+    BOOST_REQUIRE(LookupHost(scoped_addr, addr, false));
+    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 || addr_str == "fe80:0:0:0:0:0:0:1");
+    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15 and (probably) later.
+    // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope.
+    BOOST_REQUIRE(LookupHost(link_local + "%0", addr, false));
+    BOOST_REQUIRE(addr.IsValid());
+    BOOST_REQUIRE(addr.IsIPv6());
+    BOOST_CHECK(!addr.IsBindAny());
+    BOOST_CHECK_EQUAL(addr.ToString(), link_local);
+
     // TORv2
     BOOST_REQUIRE(addr.SetSpecial("6hzph5hv6337r6p2.onion"));
     BOOST_REQUIRE(addr.IsValid());
