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

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:CNetAddr-scoped-ipv6-unit-tests-docs-and-rename-scopeId-to-m_scope_id changing 3 files +27 −4
  1. jonatack commented at 10:54 am on September 13, 2020: member

    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:

     0Running 1 test case...
 1Entering test module "Bitcoin Core Test Suite"
 2test/net_tests.cpp(85): Entering test suite "net_tests"
 3test/net_tests.cpp(204): Entering test case "cnetaddr_basic"
 4test/net_tests.cpp(210): info: check LookupHost("0.0.0.0", addr, false) has passed
 5test/net_tests.cpp(211): info: check !addr.IsValid() has passed
 6test/net_tests.cpp(212): info: check addr.IsIPv4() has passed
 7test/net_tests.cpp(214): info: check addr.IsBindAny() has passed
 8test/net_tests.cpp(215): info: check addr.ToString() == "0.0.0.0" has passed
 9test/net_tests.cpp(218): info: check LookupHost("255.255.255.255", addr, false) has passed
10test/net_tests.cpp(219): info: check !addr.IsValid() has passed
11test/net_tests.cpp(220): info: check addr.IsIPv4() has passed
12test/net_tests.cpp(222): info: check !addr.IsBindAny() has passed
13test/net_tests.cpp(223): info: check addr.ToString() == "255.255.255.255" has passed
14test/net_tests.cpp(226): info: check LookupHost("12.34.56.78", addr, false) has passed
15test/net_tests.cpp(227): info: check addr.IsValid() has passed
16test/net_tests.cpp(228): info: check addr.IsIPv4() has passed
17test/net_tests.cpp(230): info: check !addr.IsBindAny() has passed
18test/net_tests.cpp(231): info: check addr.ToString() == "12.34.56.78" has passed
19test/net_tests.cpp(234): info: check LookupHost("::", addr, false) has passed
20test/net_tests.cpp(235): info: check !addr.IsValid() has passed
21test/net_tests.cpp(236): info: check addr.IsIPv6() has passed
22test/net_tests.cpp(238): info: check addr.IsBindAny() has passed
23test/net_tests.cpp(239): info: check addr.ToString() == "::" has passed
24test/net_tests.cpp(242): info: check LookupHost(ipv6_addr, addr, false) has passed
25test/net_tests.cpp(243): info: check addr.IsValid() has passed
26test/net_tests.cpp(244): info: check addr.IsIPv6() has passed
27test/net_tests.cpp(246): info: check !addr.IsBindAny() has passed
28test/net_tests.cpp(247): info: check addr.ToString() == ipv6_addr has passed
29test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
30test/net_tests.cpp(255): info: check addr.IsValid() has passed
31test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
32test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
33test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed
34test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
35test/net_tests.cpp(255): info: check addr.IsValid() has passed
36test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
37test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
38test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed
39test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
40test/net_tests.cpp(255): info: check addr.IsValid() has passed
41test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
42test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
43test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed
44test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
45test/net_tests.cpp(255): info: check addr.IsValid() has passed
46test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
47test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
48test/net_tests.cpp(258): info: check addr.ToString() == scoped_addr has passed
49test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed
50test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed
51test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed
52test/net_tests.cpp(266): info: check LookupHost(ipv6_addr + "%0", addr, false) has passed
53test/net_tests.cpp(267): info: check addr.IsValid() has passed
54test/net_tests.cpp(268): info: check addr.IsIPv6() has passed
55test/net_tests.cpp(269): info: check !addr.IsBindAny() has passed
56test/net_tests.cpp(270): info: check addr.ToString() == ipv6_addr has passed
57test/net_tests.cpp(274): info: check addr.IsValid() has passed
58test/net_tests.cpp(275): info: check addr.IsTor() has passed
59test/net_tests.cpp(277): info: check !addr.IsBindAny() has passed
60test/net_tests.cpp(278): info: check addr.ToString() == "6hzph5hv6337r6p2.onion" has passed
61test/net_tests.cpp(282): info: check !addr.IsValid() has passed
62test/net_tests.cpp(283): info: check addr.IsInternal() has passed
63test/net_tests.cpp(285): info: check !addr.IsBindAny() has passed
64test/net_tests.cpp(286): info: check addr.ToString() == "esffpvrt3wpeaygy.internal" has passed
65test/net_tests.cpp(204): Leaving test case "cnetaddr_basic"; testing time: 30933us
66test/net_tests.cpp(85): Leaving test suite "net_tests"; testing time: 30975us
67Leaving test module "Bitcoin Core Test Suite"; testing time: 31169us
68
69*** No errors detected
    • change this line in the code to break the feature:
    0--- a/src/netaddress.cpp
1+++ b/src/netaddress.cpp
2@@ -716,7 +716,7 @@ bool CService::GetSockAddr(struct sockaddr* paddr, socklen_t *addrlen) const
3         memset(paddrin6, 0, *addrlen);
4         if (!GetIn6Addr(&paddrin6->sin6_addr))
5             return false;
6-        paddrin6->sin6_scope_id = m_scope_id;
7+        // 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

     0Running 1 test case...
 1Entering test module "Bitcoin Core Test Suite"
 2test/net_tests.cpp(85): Entering test suite "net_tests"
 3test/net_tests.cpp(204): Entering test case "cnetaddr_basic"
 4test/net_tests.cpp(210): info: check LookupHost("0.0.0.0", addr, false) has passed
 5test/net_tests.cpp(211): info: check !addr.IsValid() has passed
 6test/net_tests.cpp(212): info: check addr.IsIPv4() has passed
 7test/net_tests.cpp(214): info: check addr.IsBindAny() has passed
 8test/net_tests.cpp(215): info: check addr.ToString() == "0.0.0.0" has passed
 9test/net_tests.cpp(218): info: check LookupHost("255.255.255.255", addr, false) has passed
10test/net_tests.cpp(219): info: check !addr.IsValid() has passed
11test/net_tests.cpp(220): info: check addr.IsIPv4() has passed
12test/net_tests.cpp(222): info: check !addr.IsBindAny() has passed
13test/net_tests.cpp(223): info: check addr.ToString() == "255.255.255.255" has passed
14test/net_tests.cpp(226): info: check LookupHost("12.34.56.78", addr, false) has passed
15test/net_tests.cpp(227): info: check addr.IsValid() has passed
16test/net_tests.cpp(228): info: check addr.IsIPv4() has passed
17test/net_tests.cpp(230): info: check !addr.IsBindAny() has passed
18test/net_tests.cpp(231): info: check addr.ToString() == "12.34.56.78" has passed
19test/net_tests.cpp(234): info: check LookupHost("::", addr, false) has passed
20test/net_tests.cpp(235): info: check !addr.IsValid() has passed
21test/net_tests.cpp(236): info: check addr.IsIPv6() has passed
22test/net_tests.cpp(238): info: check addr.IsBindAny() has passed
23test/net_tests.cpp(239): info: check addr.ToString() == "::" has passed
24test/net_tests.cpp(242): info: check LookupHost(ipv6_addr, addr, false) has passed
25test/net_tests.cpp(243): info: check addr.IsValid() has passed
26test/net_tests.cpp(244): info: check addr.IsIPv6() has passed
27test/net_tests.cpp(246): info: check !addr.IsBindAny() has passed
28test/net_tests.cpp(247): info: check addr.ToString() == ipv6_addr has passed
29test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
30test/net_tests.cpp(255): info: check addr.IsValid() has passed
31test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
32test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
33test/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]
34test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
35test/net_tests.cpp(255): info: check addr.IsValid() has passed
36test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
37test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
38test/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]
39test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
40test/net_tests.cpp(255): info: check addr.IsValid() has passed
41test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
42test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
43test/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]
44test/net_tests.cpp(254): info: check LookupHost(scoped_addr, addr, false) has passed
45test/net_tests.cpp(255): info: check addr.IsValid() has passed
46test/net_tests.cpp(256): info: check addr.IsIPv6() has passed
47test/net_tests.cpp(257): info: check !addr.IsBindAny() has passed
48test/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]
49test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed
50test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed
51test/net_tests.cpp(263): info: check !LookupHost(ipv6_addr + "%" + zone_id, addr, false) has passed
52test/net_tests.cpp(266): info: check LookupHost(ipv6_addr + "%0", addr, false) has passed
53test/net_tests.cpp(267): info: check addr.IsValid() has passed
54test/net_tests.cpp(268): info: check addr.IsIPv6() has passed
55test/net_tests.cpp(269): info: check !addr.IsBindAny() has passed
56test/net_tests.cpp(270): info: check addr.ToString() == ipv6_addr has passed
57test/net_tests.cpp(274): info: check addr.IsValid() has passed
58test/net_tests.cpp(275): info: check addr.IsTor() has passed
59test/net_tests.cpp(277): info: check !addr.IsBindAny() has passed
60test/net_tests.cpp(278): info: check addr.ToString() == "6hzph5hv6337r6p2.onion" has passed
61test/net_tests.cpp(282): info: check !addr.IsValid() has passed
62test/net_tests.cpp(283): info: check addr.IsInternal() has passed
63test/net_tests.cpp(285): info: check !addr.IsBindAny() has passed
64test/net_tests.cpp(286): info: check addr.ToString() == "esffpvrt3wpeaygy.internal" has passed
65test/net_tests.cpp(204): Leaving test case "cnetaddr_basic"; testing time: 32316us
66test/net_tests.cpp(85): Leaving test suite "net_tests"; testing time: 32354us
67Leaving test module "Bitcoin Core Test Suite"; testing time: 32522us
68
69*** 4 failures are detected in the test module "Bitcoin Core Test Suite"
    • leave your review here
  2. fanquake added the label P2P on Sep 13, 2020
  3. jonatack commented at 10:56 am on September 13, 2020: member
    Thanks to @Saibato for setting me straight on this in #19946 and to @michaelfolkson for looking at that PR.
  4. jonatack force-pushed on Sep 13, 2020
  5. michaelfolkson commented at 11:22 am on September 13, 2020: contributor
    Concept ACK. Will follow the above steps before ACKing the commit.
  6. jonatack force-pushed on Sep 13, 2020
  7. jonatack commented at 12:01 pm on September 13, 2020: member
    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.
  8. jonatack force-pushed on Sep 13, 2020
  9. Saibato commented at 1:39 pm on September 13, 2020: contributor

    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 % != ?

    0test/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?

  10. practicalswift commented at 1:55 pm on September 13, 2020: contributor

    Concept ACK

    Thanks for adding tests! ❤️

  11. sipa commented at 5:09 pm on September 13, 2020: member

    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.

  12. jonatack force-pushed on Sep 13, 2020
  13. jonatack force-pushed on Sep 13, 2020
  14. jonatack commented at 9:07 pm on September 13, 2020: member

    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.

  15. in src/test/net_tests.cpp:261 in 431ebcefc6 outdated 
    256+        BOOST_REQUIRE(LookupHost(scoped_addr, addr, false));
257+        BOOST_REQUIRE(addr.IsValid());
258+        BOOST_REQUIRE(addr.IsIPv6());
259+        BOOST_CHECK(!addr.IsBindAny());
260+        const std::string addr_str{addr.ToString()};
261+        BOOST_CHECK(addr_str == scoped_addr || addr_str == link_local_edge_case);
    eriknylund commented at 9:56 am on September 14, 2020:

    I tested this locally on macOS Catalina 10.15.6 and I get an error on this line:

    0test/net_tests.cpp:261: error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr || addr_str == link_local_edge_case has failed

    Update: It fails on scope 8 and 21, but not on 32.

    jonatack commented at 10:38 am on September 14, 2020:

    Thanks @eriknylund! If you don’t mind, can you give the output of rebuilding and running the test after adding these 2 lines?

    0         const std::string addr_str{addr.ToString()};
1+        BOOST_TEST_MESSAGE(scoped_addr);
2+        BOOST_TEST_MESSAGE(addr_str);
3         BOOST_CHECK(addr_str == scoped_addr || addr_str == link_local_edge_case);
4     }
    jonatack commented at 10:43 am on September 14, 2020:
    (ISTM the lower scope values are often used by the local machine, so higher values may indeed make the test more agnostic.)
    eriknylund commented at 10:53 am on September 14, 2020:

    (ISTM the lower scope values are often used by the local machine, so higher values may indeed make the test more agnostic.)

    That seems to be the issue, en2 and vmnet8 are two network interfaces on my local machine:

    0fe80::1%8
1fe80::1%en2
2test/net_tests.cpp:264: error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr || addr_str == link_local_edge_case has failed
3fe80::1%21
4fe80::1%vmnet8
5test/net_tests.cpp:264: error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr || addr_str == link_local_edge_case has failed
6fe80::1%32
7fe80:0:0:0:0:0:0:1
    jonatack commented at 11:16 am on September 14, 2020:
    Perfect, thanks. Simplified the test to only the 32 and 0 values. No need for more than that.
  16. jonatack force-pushed on Sep 14, 2020
  17. jonatack force-pushed on Sep 14, 2020
  18. eriknylund approved
  19. eriknylund commented at 3:47 pm on September 14, 2020: contributor

    ACK 88b6f85419b65122d5a4469a8585b276960bbb1e 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):

    0test/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*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
  20. Saibato commented at 4:51 pm on September 14, 2020: contributor

    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?

     0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
 1index e7156d1d3..108b1ace8 100644
 2--- a/src/test/net_tests.cpp
 3+++ b/src/test/net_tests.cpp
 4@@ -19,6 +19,7 @@
 5 #include <version.h>
 6 
 7 #include <boost/test/unit_test.hpp>
 8+#include <boost/asio/ip/address.hpp>
 9 
10 #include <memory>
11 #include <string>
12@@ -251,12 +252,12 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
13     const std::string link_local{"fe80::1"};
14     const std::string scoped_addr{link_local + "%32"};
15     BOOST_REQUIRE(LookupHost(scoped_addr, addr, false));
16+
17     BOOST_REQUIRE(addr.IsValid());
18     BOOST_REQUIRE(addr.IsIPv6());
19     BOOST_CHECK(!addr.IsBindAny());
20-    const std::string addr_str{addr.ToString()};
21-    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
22-    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15.
23+    const std::string addr_str{(boost::asio::ip::address::from_string(addr.ToString())).to_string()};
24+    BOOST_CHECK(addr_str == scoped_addr);
25     // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope.
26     BOOST_REQUIRE(LookupHost(link_local + "%0", addr, false));
27     BOOST_REQUIRE(addr.IsValid());
  21. eriknylund commented at 5:28 am on September 15, 2020: contributor

    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?

     0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
 1index e7156d1d3..108b1ace8 100644
 2--- a/src/test/net_tests.cpp
 3+++ b/src/test/net_tests.cpp
 4@@ -19,6 +19,7 @@
 5 #include <version.h>
 6 
 7 #include <boost/test/unit_test.hpp>
 8+#include <boost/asio/ip/address.hpp>
 9 
10 #include <memory>
11 #include <string>
12@@ -251,12 +252,12 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
13     const std::string link_local{"fe80::1"};
14     const std::string scoped_addr{link_local + "%32"};
15     BOOST_REQUIRE(LookupHost(scoped_addr, addr, false));
16+
17     BOOST_REQUIRE(addr.IsValid());
18     BOOST_REQUIRE(addr.IsIPv6());
19     BOOST_CHECK(!addr.IsBindAny());
20-    const std::string addr_str{addr.ToString()};
21-    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
22-    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15.
23+    const std::string addr_str{(boost::asio::ip::address::from_string(addr.ToString())).to_string()};
24+    BOOST_CHECK(addr_str == scoped_addr);
25     // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope.
26     BOOST_REQUIRE(LookupHost(link_local + "%0", addr, false));
27     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:

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

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

    0fe80::1%32
1fe80::1
2test/net_tests.cpp:267: error: in "net_tests/cnetaddr_basic": check addr_str == scoped_addr has failed
  22. jonatack commented at 5:47 am on September 15, 2020: member
    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.
  23. michaelfolkson commented at 10:26 am on September 15, 2020: contributor

    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?

  24. jonatack commented at 12:09 pm on October 2, 2020: member
    Up for grabs.
  25. jonatack closed this on Oct 2, 2020
  26. in src/test/net_tests.cpp:25 in 88b6f85419 outdated 
    21@@ -22,6 +22,7 @@
22 
23 #include <memory>
24 #include <string>
25+#include <vector>
    jonatack commented at 1:07 pm on October 2, 2020:

    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.

  27. in src/test/net_tests.cpp:259 in 88b6f85419 outdated 
    254+    BOOST_REQUIRE(addr.IsValid());
255+    BOOST_REQUIRE(addr.IsIPv6());
256+    BOOST_CHECK(!addr.IsBindAny());
257+    const std::string addr_str{addr.ToString()};
258+    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
259+    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15.
    jonatack commented at 1:09 pm on October 2, 2020:
    Maybe “macOS 10.14/10.15” -> “macOS 10.14+”
  28. laanwj added the label Tests on Oct 2, 2020
  29. laanwj commented at 2:09 pm on October 2, 2020: member
    FWIW, I still think this is worth doing.
  30. jonatack reopened this on Oct 2, 2020
  31. jonatack force-pushed on Oct 2, 2020
  32. jonatack commented at 2:35 pm on October 2, 2020: member

    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

     0/src/test/net_tests.cpp
 1 
 2 #include <string>
 3-#include <vector>
 4 
 5@@ -256,7 +255,7 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
 6     BOOST_CHECK(!addr.IsBindAny());
 7     const std::string addr_str{addr.ToString()};
 8     BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
 9-    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15.
10+    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15 and (probably) later.
11     // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope.
  33. test: add test coverage for CNetAddr ipv6 scoped addresses 5cb5fd3005
  34. net: rename CNetAddr scopeId to m_scope_id, improve code doc f36887fa47
  35. jonatack force-pushed on Oct 2, 2020
  36. laanwj commented at 3:02 pm on October 2, 2020: member
    ACK f36887fa47b42af60f8a06a3995baca7c73ca310
  37. laanwj merged this on Oct 2, 2020
  38. laanwj closed this on Oct 2, 2020
  39. jonatack deleted the branch on Oct 2, 2020
  40. in src/test/net_tests.cpp:261 in f36887fa47 
    256+    BOOST_REQUIRE(addr.IsIPv6());
257+    BOOST_CHECK(!addr.IsBindAny());
258+    const std::string addr_str{addr.ToString()};
259+    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
260+    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15 and (probably) later.
261+    // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope.
    ryanofsky commented at 10:57 pm on April 11, 2021:

    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.

    jonatack commented at 6:22 pm on April 12, 2021:

    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?

    0    // Test with a fairly-high value, e.g. 32, to avoid locally reserved ids.
1    const std::string link_local{"fe80::1"};
2    const std::string scoped_addr{link_local + "%32"};
    ryanofsky commented at 6:45 pm on April 12, 2021:

    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.

    jonatack commented at 6:25 pm on April 14, 2021:
    My bad for ambiguous wording! s/reserved id/id in use/…will fix issue and wording.
  41. PastaPastaPasta referenced this in commit e0b0a312a7 on Jun 27, 2021
  42. PastaPastaPasta referenced this in commit dd7f6d697c on Jun 28, 2021
  43. PastaPastaPasta referenced this in commit 9cc2838154 on Jun 29, 2021
  44. Fabcien referenced this in commit 710e9aeb38 on Nov 3, 2021
  45. Fabcien referenced this in commit de3ebfa9af on Nov 3, 2021
  46. DeckerSU referenced this in commit 4cb98fc3f3 on Feb 8, 2022
  47. bitcoin locked this on Aug 18, 2022


jonatack michaelfolkson Saibato practicalswift sipa eriknylund laanwj ryanofsky hebasto

Labels
Tests P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me