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

jonatack · 2020-09-13T10:54:30Z

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
build Bitcoin Core
run the test: cd src ; test/test_bitcoin -t net_tests/cnetaddr_basic -l all

you should see passing test output like this

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

/

change this line in the code to break the feature:

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

you should see test output with a few failed tests like this

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"

leave your review here

jonatack · 2020-09-13T10:56:04Z

Thanks to @Saibato for setting me straight on this in #19946 and to @michaelfolkson for looking at that PR.

michaelfolkson · 2020-09-13T11:22:36Z

Concept ACK. Will follow the above steps before ACKing the commit.

jonatack · 2020-09-13T12:01:01Z

I don't know why the travis xenial and macOS GUI/no depends are failing. All the other CI jobs pass on travis, cirrus, appveyor ones and bitcoin_builds.org.

Saibato · 2020-09-13T13:39:43Z

I don't know why the travis xenial and macOS GUI/no depends are failing. All the other CI jobs pass on travis, cirrus, appveyor ones and bitcoin_builds.org.

BOOST_CHECK_EQUAL(addr.ToString(), ipv6_addr); ipv6 with % != ?

test/net_tests.cpp:259: 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]

i.e. test, does the stack support scopes at all?

practicalswift · 2020-09-13T13:55:32Z

Concept ACK

Thanks for adding tests! ❤️

sipa · 2020-09-13T17:09:42Z

I don't think scope ids are intended to be portable.

They're a way for a user, on their local system, to specify what interface to use if you have multiple and are using a link-local address.

But in the end it's just a string that's passed to the OS for parsing. What interfaces exist, how they're named, and in what situation they're accepted is entirely up to it.

One idea that may make it more portable is restricting these scopes in tests to link-local addresses (inside fe80::/10), as the OS may refuse them outside that range.

If that still doesn't work... this is really no different than trying to unit test whether DNS resolving works, and then noticing the test fails on an offline system or so. It's dependent on local configuration, and there is no good cross-platform spec that all OSes adhere to.

jonatack · 2020-09-13T21:07:59Z

One idea that may make it more portable is restricting these scopes in tests to link-local addresses (inside fe80::/10), as the OS may refuse them outside that range.

This suggestion solved the Travis Xenial job and stabilized the macOS no-depends one enough to enable a workaround. Thank you, @sipa!

CI is now green; ready for review.

src/test/net_tests.cpp

eriknylund · 2020-09-14T15:47:29Z

ACK 88b6f85 I built the PR on macOS Catalina 10.15.6 and ran both tests and functional tests - all pass. I've reviewed the two commits and I think the changes look good.

I also repeated the test instructions to verify that the tests break accordingly (a bit different now because of the modified test):

test/net_tests.cpp:258: error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1" has failed
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

Saibato · 2020-09-14T16:51:56Z

btw: That macOS edge case results fromstd::string CNetAddr::ToStringIP() const in src/netaddress.cpp
if we can not GetSockAddr(... and result there in fallback.

If we convert the addr boost or https://tools.ietf.org/html/rfc4291 style we do not need the special addr representation string fe80:0:0:0:0:0:0:1"

That patch would do the trick?

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index e7156d1d3..108b1ace8 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -19,6 +19,7 @@
 #include <version.h>
 
 #include <boost/test/unit_test.hpp>
+#include <boost/asio/ip/address.hpp>
 
 #include <memory>
 #include <string>
@@ -251,12 +252,12 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
     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.
+    const std::string addr_str{(boost::asio::ip::address::from_string(addr.ToString())).to_string()};
+    BOOST_CHECK(addr_str == scoped_addr);
     // 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());

eriknylund · 2020-09-15T05:28:06Z

btw: That macOS edge case results fromstd::string CNetAddr::ToStringIP() const in src/netaddress.cpp
if we can not GetSockAddr(... and result there in fallback.

If we convert the addr boost or https://tools.ietf.org/html/rfc4291 style we do not need the special addr representation string fe80:0:0:0:0:0:0:1"

That patch would do the trick?

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index e7156d1d3..108b1ace8 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -19,6 +19,7 @@
 #include <version.h>
 
 #include <boost/test/unit_test.hpp>
+#include <boost/asio/ip/address.hpp>
 
 #include <memory>
 #include <string>
@@ -251,12 +252,12 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
     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.
+    const std::string addr_str{(boost::asio::ip::address::from_string(addr.ToString())).to_string()};
+    BOOST_CHECK(addr_str == scoped_addr);
     // 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());

It's an interesting thought @Saibato, but I'm unsure if it will work here? I tried the patch by adding more verbose output with the following result:

    const std::string addr_str{(boost::asio::ip::address::from_string(addr.ToString())).to_string()};
    BOOST_TEST_MESSAGE(scoped_addr);
    BOOST_TEST_MESSAGE(addr_str);
    BOOST_CHECK(addr_str == scoped_addr);

test/test_bitcoin -t net_tests/cnetaddr_basic -l all gives me:

fe80::1%32
fe80::1
test/net_tests.cpp:267: error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr has failed

jonatack · 2020-09-15T05:47:17Z

Thanks @Saibato for the idea and @eriknylund for testing. MacOS 10.14+ seems to be a special case either way and I'd rather test what our code in ToStringIP() actually returns, so leaving this as-is for now.

michaelfolkson · 2020-09-15T10:26:18Z

Replicated steps @eriknylund followed here and got the same results on Mac OS Catalina 10.15.6. All tests passed and the single test failed with the broken feature.

Code changes excluding the test changes are clearly fine.

Anything else one could do before ACKing the commit? Running the old tests (tests before they were updated in this PR) on the broken feature to see that the old tests wouldn't have failed with this change?

jonatack · 2020-10-02T12:09:06Z

Up for grabs.

jonatack · 2020-10-02T13:07:35Z

src/test/net_tests.cpp

@@ -22,6 +22,7 @@

 #include <memory>
 #include <string>
+#include <vector>


This line is left over from an earlier push and should be removed.

Other than that, I think this PR was finished, provided missing coverage+context, and just needed some review begging or sales effort.

src/test/net_tests.cpp

laanwj · 2020-10-02T14:09:24Z

FWIW, I still think this is worth doing.

jonatack · 2020-10-02T14:35:45Z

Rebased for the merged addrv2 changes, and updated to remove an unneeded include from an earlier push here and update a comment. Diff: git range-diff df2129a 88b6f85 f36887f

/src/test/net_tests.cpp
 
 #include <string>
-#include <vector>
 
@@ -256,7 +255,7 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
     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.
+    // 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.

laanwj · 2020-10-02T15:02:25Z

ACK f36887f

ryanofsky · 2021-04-11T22:57:28Z

src/test/net_tests.cpp

+    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.


I saw this check fail in a recent cirrus run [previous releases, uses qt5 dev package and some depends packages] [unsigned char] [bionic]:

https://cirrus-ci.com/task/5950587750580224?logs=ci#L4679

I'm restarting the test, will see if happens again.

Thanks for reporting. It may be hitting a reserved id; have had it happen on occasion locally and bumping the 32 to a higher value resolved it. What happened after restarting?

// 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"};

Thanks, interesting! (I don't know what a reserved id is and googling didn't seem to turn up anything helpful, so I'd be curious for any pointers.)

But test passed on restart so it is an intermittent issue like you suggested. Maybe the test can be written so the check is less strict and just depends on parsing behavior not OS behavior. But it doesn't seem too urgent if this is a known infrequent issue.

My bad for ambiguous wording! s/reserved id/id in use/...will fix issue and wording.

…ename scopeId to m_scope_id 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>you should see passing test output like this</summary> ``` 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 ``` /</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>you should see test output with a few failed tests like this</summary> ``` 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" ``` </details> - leave your review here ACKs for top commit: laanwj: ACK f36887f Tree-SHA512: 8e77e512db130642be7d3a910735ca803a2afdb5a704f713f163f8b5a80be3077a2be6f0a3ca43d299731dcd2545ac35571f8c74e5250a72a48233c26f9a3ab5

Summary: This is a backport of [[bitcoin/bitcoin#19951 | core#19951]] [1/2] bitcoin/bitcoin@5cb5fd3 Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10425

Summary: This is a backport of [[bitcoin/bitcoin#19951 | core#19951]] [2/2] bitcoin/bitcoin@f36887f Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10426

…ope_id backport of bitcoin/bitcoin#19951 to our codebase.

hebasto · 2024-12-30T15:35:38Z

src/test/net_tests.cpp

+    // 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);


Apparently, this test is not portable. See: #31580.

fanquake added the P2P label Sep 13, 2020

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch from 1ea6f55 to a6399d2 Compare September 13, 2020 11:09

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch from a6399d2 to 805231b Compare September 13, 2020 11:59

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch from 805231b to 1672b16 Compare September 13, 2020 12:47

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch 2 times, most recently from 1b677ba to f3c8163 Compare September 13, 2020 19:49

eriknylund reviewed Sep 14, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch 2 times, most recently from 73d93e4 to 88b6f85 Compare September 14, 2020 11:22

eriknylund approved these changes Sep 14, 2020

View reviewed changes

jonatack closed this Oct 2, 2020

jonatack commented Oct 2, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

laanwj added the Tests label Oct 2, 2020

jonatack reopened this Oct 2, 2020

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch from 88b6f85 to 79f4740 Compare October 2, 2020 14:32

jonatack added 2 commits October 2, 2020 16:40

test: add test coverage for CNetAddr ipv6 scoped addresses

5cb5fd3

net: rename CNetAddr scopeId to m_scope_id, improve code doc

f36887f

jonatack force-pushed the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch from 79f4740 to f36887f Compare October 2, 2020 14:43

laanwj merged commit d993522 into bitcoin:master Oct 2, 2020

jonatack deleted the CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id branch October 2, 2020 15:08

ryanofsky reviewed Apr 11, 2021

View reviewed changes

jonatack mentioned this pull request Apr 14, 2021

intermittent unit test failure: test/net_tests.cpp(311): error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1" has failed #21682

Closed

DeckerSU added a commit to DeckerSU/KomodoOcean that referenced this pull request Feb 8, 2022

net, test: CNetAddr scoped ipv6 test coverage, rename scopeId to m_sc…

4cb98fc

…ope_id backport of bitcoin/bitcoin#19951 to our codebase.

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

hebasto reviewed Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonatack commented Sep 13, 2020

jonatack commented Sep 13, 2020

michaelfolkson commented Sep 13, 2020

jonatack commented Sep 13, 2020 •

edited

Loading

Saibato commented Sep 13, 2020 •

edited

Loading

practicalswift commented Sep 13, 2020

sipa commented Sep 13, 2020

jonatack commented Sep 13, 2020

eriknylund commented Sep 14, 2020

Saibato commented Sep 14, 2020

eriknylund commented Sep 15, 2020

jonatack commented Sep 15, 2020

michaelfolkson commented Sep 15, 2020 •

edited

Loading

jonatack commented Oct 2, 2020

jonatack Oct 2, 2020

laanwj commented Oct 2, 2020

jonatack commented Oct 2, 2020 •

edited

Loading

laanwj commented Oct 2, 2020

ryanofsky Apr 11, 2021

jonatack Apr 12, 2021

ryanofsky Apr 12, 2021

jonatack Apr 14, 2021

hebasto Dec 30, 2024

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

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

Conversation

jonatack commented Sep 13, 2020

jonatack commented Sep 13, 2020

michaelfolkson commented Sep 13, 2020

jonatack commented Sep 13, 2020 • edited Loading

Saibato commented Sep 13, 2020 • edited Loading

practicalswift commented Sep 13, 2020

sipa commented Sep 13, 2020

jonatack commented Sep 13, 2020

eriknylund commented Sep 14, 2020

Saibato commented Sep 14, 2020

eriknylund commented Sep 15, 2020

jonatack commented Sep 15, 2020

michaelfolkson commented Sep 15, 2020 • edited Loading

jonatack commented Oct 2, 2020

jonatack Oct 2, 2020

Choose a reason for hiding this comment

laanwj commented Oct 2, 2020

jonatack commented Oct 2, 2020 • edited Loading

laanwj commented Oct 2, 2020

ryanofsky Apr 11, 2021

Choose a reason for hiding this comment

jonatack Apr 12, 2021

Choose a reason for hiding this comment

ryanofsky Apr 12, 2021

Choose a reason for hiding this comment

jonatack Apr 14, 2021

Choose a reason for hiding this comment

hebasto Dec 30, 2024

Choose a reason for hiding this comment

jonatack commented Sep 13, 2020 •

edited

Loading

Saibato commented Sep 13, 2020 •

edited

Loading

michaelfolkson commented Sep 15, 2020 •

edited

Loading

jonatack commented Oct 2, 2020 •

edited

Loading