-
Notifications
You must be signed in to change notification settings - Fork 37.7k
test: fix AddNode
unit test failure on OpenBSD
#28891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: fix AddNode
unit test failure on OpenBSD
#28891
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-) |
There was a problem hiding this 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?)
Confirmed this on FreeBSD. I don't have OpenBSD or NetBSD. |
5c7b344
to
007d6f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 007d6f0
This code isn't in 26.x. So removed the backport label and milestone for now. |
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: The bitcoin unit test passes without this patch and |
This is the functional test counterpart of PR bitcoin#28891 / commit 007d6f0.
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
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
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
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
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
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
On OpenBSD 7.4, the following check of the unit test
test_addnode_getaddednodeinfo_and_connection_detection
currently fails:The reason for that is that this OS seemingly doesn't support the IPv4 shorthand notation with omitted zero-bytes:
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!).