test: fix `AddNode` unit test failure on OpenBSD #28891

1. 
   theStack commented at 11:42 AM on November 16, 2023: contributor

   On OpenBSD 7.4, the following check of the unit test test_addnode_getaddednodeinfo_and_connection_detection currently fails:

   BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true}));
   

   The reason for that is that this OS seemingly doesn't support the IPv4 shorthand notation with omitted zero-bytes:

   $ ping 127.1
   ping: no address associated with name
   

   As a simple fix, this PR skips the check for this with a pre-processor #if. On NetBSD and FreeBSD, 127.1 is resolved correctly to localhost and hence the test passes (thanks to vasild for verifying on the latter!).

2. 
   DrahtBot commented at 11:42 AM on November 16, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	vasild
   Concept ACK	jonatack

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Nov 16, 2023
4. 
   theStack commented at 11:44 AM on November 16, 2023: contributor

   cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-)

5. in src/test/net_peer_connection_tests.cpp:119 in 5c7b344116 outdated

    115 | @@ -116,7 +116,12 @@ BOOST_AUTO_TEST_CASE(test_addnode_getaddednodeinfo_and_connection_detection)
    116 |  
    117 |      BOOST_TEST_MESSAGE("\nCall AddNode() with 2 addrs resolving to existing localhost addnode entry; neither should be added");
    118 |      BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.0.0.1", /*m_use_v2transport=*/true}));
    119 | +    // Some operating systems don't support the IPv4 shorthand notation with omitted zero-bytes.
   

   ---

   ---

   

   jonatack commented at 2:30 PM on November 16, 2023:

   I think you can drop lines 120-121 (and wrt to "Skip the second AddNode call on those", it's the first rather than second call on those, IIUC).

   Also, perhaps just name the platform in question.

       // OpenBSD doesn't support the IPv4 shorthand notation with omitted zero-bytes.
   

   ---

   

   theStack commented at 3:03 PM on November 16, 2023:

   Thanks, done. At the time of writing this comment I was sure that this also affects the other BSDs, but apparently it doesn't. Also fixed the commit message accordingly.

6. 
   jonatack commented at 2:31 PM on November 16, 2023: member

   Concept ACK (is there a reason not to have a BSD CI task?)

7. 
   maflcko commented at 2:36 PM on November 16, 2023: member

   NetBSD and FreeBSD, 127.1 is resolved correctly

   Confirmed this on FreeBSD. I don't have OpenBSD or NetBSD.

8. test: fix `AddNode` unit test failure on OpenBSD 007d6f0e85
9. theStack force-pushed on Nov 16, 2023
10. vasild approved
11. 
    vasild commented at 10:56 AM on November 19, 2023: contributor

    ACK 007d6f0e85bc329040bb405ef6016339518caa66

12. DrahtBot requested review from jonatack on Nov 19, 2023
13. maflcko added this to the milestone 26.0 on Nov 19, 2023
14. maflcko added the label Needs backport (26.x) on Nov 19, 2023
15. fanquake merged this on Nov 22, 2023
16. fanquake closed this on Nov 22, 2023

    ---

17. theStack deleted the branch on Nov 22, 2023
18. 
    fanquake commented at 11:30 AM on November 22, 2023: member

    This code isn't in 26.x. So removed the backport label and milestone for now.

19. fanquake removed the label Needs backport (26.x) on Nov 22, 2023
20. fanquake removed this from the milestone 26.0 on Nov 22, 2023
21. 
    pinheadmz commented at 6:13 PM on November 22, 2023: member

    cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-)

    Hi, post merge code review ACK.

    I have a FreeBSD container on google cloud I just fire up when needed.

    The bug you're fixing here is apparently not an issue on my OS: FreeBSD freebsd-13-1 13.2-RELEASE-p4 FreeBSD 13.2-RELEASE-p4 GENERIC amd64

    The bitcoin unit test passes without this patch and ping 127.1 also works fine.

22. theStack referenced this in commit fd0bde2793 on Dec 8, 2023
23. fanquake referenced this in commit 09ab9d4fa7 on Dec 11, 2023
24. PastaPastaPasta referenced this in commit 4420aab046 on Oct 28, 2024
25. PastaPastaPasta referenced this in commit 9c6827ee55 on Oct 28, 2024
26. PastaPastaPasta referenced this in commit c789121e6f on Oct 28, 2024
27. PastaPastaPasta referenced this in commit 4122404b81 on Oct 29, 2024
28. PastaPastaPasta referenced this in commit 841ea27d7b on Oct 29, 2024
29. bitcoin locked this on Nov 21, 2024

Contributors

theStackDrahtBotjonatackmaflckovasildfanquakepinheadmz

Review Requested
jonatack