Skip to content

Conversation

theStack
Copy link
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!).

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 16, 2023

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

Code Coverage

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

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.

@DrahtBot DrahtBot added the Tests label Nov 16, 2023
@theStack
Copy link
Contributor Author

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

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko
Copy link
Member

maflcko commented Nov 16, 2023

NetBSD and FreeBSD, 127.1 is resolved correctly

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

@theStack theStack force-pushed the 202311-test-fix-net_peer_connection_tests-on-BSDs branch from 5c7b344 to 007d6f0 Compare November 16, 2023 15:02
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 007d6f0

@DrahtBot DrahtBot requested a review from jonatack November 19, 2023 10:56
@maflcko maflcko added this to the 26.0 milestone Nov 19, 2023
@fanquake fanquake merged commit 3dca308 into bitcoin:master Nov 22, 2023
@theStack theStack deleted the 202311-test-fix-net_peer_connection_tests-on-BSDs branch November 22, 2023 11:19
@fanquake
Copy link
Member

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

@fanquake fanquake removed this from the 26.0 milestone Nov 22, 2023
@pinheadmz
Copy link
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.

theStack added a commit to theStack/bitcoin that referenced this pull request Dec 8, 2023
This is the functional test counterpart of PR bitcoin#28891 /
commit 007d6f0.
fanquake added a commit that referenced this pull request Dec 11, 2023
fd0bde2 test: fix `addnode` functional test failure on OpenBSD (Sebastian Falbesoner)

Pull request description:

  This is the functional test counterpart of PR #28891 / commit 007d6f0 (unfortunately, I missed it back then and only ran the unit tests -- sorry for the noise).

  master branch on OpenBSD 7.4:
  ```
  $ ./test/functional/rpc_net.py
  2023-12-08T17:29:05.057000Z TestFramework (INFO): PRNG seed is: 6024296850131317403
  2023-12-08T17:29:05.058000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_au3zchif
  2023-12-08T17:29:05.618000Z TestFramework (INFO): Test getconnectioncount
  2023-12-08T17:29:05.618000Z TestFramework (INFO): Test getpeerinfo
  2023-12-08T17:29:06.643000Z TestFramework (INFO): Check getpeerinfo output before a version message was sent
  2023-12-08T17:29:06.709000Z TestFramework (INFO): Test getnettotals
  2023-12-08T17:29:06.773000Z TestFramework (INFO): Test getnetworkinfo
  2023-12-08T17:29:06.978000Z TestFramework (INFO): Test addnode and getaddednodeinfo
  2023-12-08T17:29:06.980000Z TestFramework (ERROR): Assertion failed
  Traceback (most recent call last):
    File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
      self.run_test()
    File "/home/thestack/bitcoin/./test/functional/rpc_net.py", line 65, in run_test
      self.test_addnode_getaddednodeinfo()
    File "/home/thestack/bitcoin/./test/functional/rpc_net.py", line 224, in test_addnode_getaddednodeinfo
      assert_raises_rpc_error(-23, "Node already added", self.nodes[0].addnode, node=ip_port2, command='add')
    File "/home/thestack/bitcoin/test/functional/test_framework/util.py", line 131, in assert_raises_rpc_error
      assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
  AssertionError: No exception raised
  ```

  On the PR branch, the same call succeeds.

ACKs for top commit:
  kevkevinpal:
    ACK [fd0bde2](fd0bde2)

Tree-SHA512: ae20816fa4025c212e115ebd267b5e5784bfcdf0051219eb686faaade47ec4f91a3947af6d24258b159290000d2dcc3f6e65e788b83b8a9297282945dbdafbfb
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 28, 2024
007d6f0 test: fix `AddNode` unit test failure on OpenBSD (Sebastian Falbesoner)

Pull request description:

  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!).

ACKs for top commit:
  vasild:
    ACK 007d6f0

Tree-SHA512: 8ab8393c490e1ecc140e8ff74f6fa4d26d0dd77e6a77a241cd198314b8c5afee7422f95351ca05f4c1742433dab77016a8ccb8d28062f8edd4b703a918a2bbda
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 28, 2024
007d6f0 test: fix `AddNode` unit test failure on OpenBSD (Sebastian Falbesoner)

Pull request description:

  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!).

ACKs for top commit:
  vasild:
    ACK 007d6f0

Tree-SHA512: 8ab8393c490e1ecc140e8ff74f6fa4d26d0dd77e6a77a241cd198314b8c5afee7422f95351ca05f4c1742433dab77016a8ccb8d28062f8edd4b703a918a2bbda
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 28, 2024
007d6f0 test: fix `AddNode` unit test failure on OpenBSD (Sebastian Falbesoner)

Pull request description:

  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!).

ACKs for top commit:
  vasild:
    ACK 007d6f0

Tree-SHA512: 8ab8393c490e1ecc140e8ff74f6fa4d26d0dd77e6a77a241cd198314b8c5afee7422f95351ca05f4c1742433dab77016a8ccb8d28062f8edd4b703a918a2bbda
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 29, 2024
007d6f0 test: fix `AddNode` unit test failure on OpenBSD (Sebastian Falbesoner)

Pull request description:

  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!).

ACKs for top commit:
  vasild:
    ACK 007d6f0

Tree-SHA512: 8ab8393c490e1ecc140e8ff74f6fa4d26d0dd77e6a77a241cd198314b8c5afee7422f95351ca05f4c1742433dab77016a8ccb8d28062f8edd4b703a918a2bbda
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 29, 2024
fbc0bce Merge bitcoin#29665: build, depends: Fix `libmultiprocess` cross-compilation (fanquake)
59a2145 Merge bitcoin#29747: depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)
bdb2fa6 Merge bitcoin#29671: index: avoid "failed to commit" errors on initialization (Ava Chow)
65377ea Merge bitcoin#29192: Weaken serfloat tests (fanquake)
4122404 Merge bitcoin#28891: test: fix `AddNode` unit test failure on OpenBSD (fanquake)
812ef53 Merge bitcoin#25677: refactor: make active_chain_tip a reference (MacroFake)
34e12fa Merge bitcoin#24564: doc: Clarify that CheckSequenceLocksAtTip is a validation function (glozow)
d529751 Merge bitcoin#24788: doc: Add gpg key import instructions for Windows (fanquake)
61bae78 Merge bitcoin#24784: refactor: deduplicate integer serialization in RollingBloom benchmark (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  Trivial backports

  ## What was done?

  ## How Has This Been Tested?
  built locally

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK fbc0bce

Tree-SHA512: 731f70b747712810d4f74fe64a90139b02ddb62e9ac260705fa2588595feb19bd6e5ffed521fd878bacaab0015683e582fed19ed1855c3e955f93cd223862a17
@bitcoin bitcoin locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants