test: p2p: check that connecting to ourself leads to disconnect #30362
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The "connect to ourself" detection logic has been first introduced by Satoshi in October 2009, together with a couple of other changes and a version bump to "v0.1.6 BETA" (see commit cc0b4c3).
|
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. |
fjahr
reviewed
Jun 28, 2024
| @@ -88,6 +89,12 @@ def run_test(self): | |||
| with node.assert_debug_log([f"feeler connection completed"]): | |||
| self.add_outbound_connection(node, "feeler", NODE_NONE, wait_for_disconnect=True) | |||
|
|
|||
| self.log.info("Check that connecting to ourself leads to immediate disconnect") | |||
| with node.assert_debug_log(["connected to self", "disconnecting"]): | |||
| node_listen_addr = f"127.0.0.1:{p2p_port(0)}" | |||
There was a problem hiding this comment.
nit: Could pull this up two lines and use it to check the exact log line I think, but not a blocker for me
There was a problem hiding this comment.
Tried that initially, but the log line shows not the destination, but the source address of the connection, which contains an ephemeral port (i.e. randomly assigned by the OS) and hence can't be predicted.
|
tACK 5d2fb14 looks good to me and I ran it locally on my machine and looks to work well |
tdb3
approved these changes
Jun 29, 2024
There was a problem hiding this comment.
ACK 5d2fb14
Good addition.
Tested locally with the following:
- Running the p2p_handshake test variants with
test_runner(passed) - Running a purposefully separate node (
src/bitcoind -regtest -bind=127.0.0.1:1234), and modifyingp2p_handshaketo run the test against it (node_listen_addr = f"127.0.0.1:{1234}"). Test failed as expected (expected messages weren't found). - Stopping the node at 1234 and running the modified test. Test failed as expected (connection refused).
| self.log.info("Check that connecting to ourself leads to immediate disconnect") | ||
| with node.assert_debug_log(["connected to self", "disconnecting"]): | ||
| node_listen_addr = f"127.0.0.1:{p2p_port(0)}" | ||
| node.addconnection(node_listen_addr, "outbound-full-relay", self.options.v2transport) |
There was a problem hiding this comment.
I like how this is using self.options.v2transport as the argument, enabling the script list in test_runner to specify whether or not v2transport should be used for the test.
|
ACK 5d2fb14 |
|
So this tiny new test revealed a race and a bug in the existent code, excellent! ❤️ |
fanquake
added a commit
that referenced
this pull request
Jul 2, 2024
…ds to disconnect" 9ec2c53 Revert "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner) Pull request description: As suggested in #30368 (comment), this PR reverts the recently added test #30362 that causes frequent CI failures. A TODO is added in the functional test file to re-add it later when the race condition is fixed. ACKs for top commit: mzumsande: utACK 9ec2c53 brunoerg: utACK 9ec2c53 tdb3: ACK 9ec2c53 Tree-SHA512: df211ab194dc47f2ff8192f3827382974db922ed9fa54bc44fac75de4edfb3af43c1340cd5434b15b0b573f7b0ddd4451a0bbbbd7deaf7f4244e4865b9d5977e
glozow
added a commit
that referenced
this pull request
Jul 16, 2024
16bd283 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner) 0dbcd4c net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner) 66673f1 net: fix race condition in self-connect detection (Sebastian Falbesoner) Pull request description: This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368). Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method): 1. set up node state 2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net_processing.cpp#L1662-L1683)) 3. add new node to vector `m_nodes` If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`). Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`. The temporarily reverted test introduced in #30362 is readded. Fixes #30368. Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see #30368 (comment) ff. and #30394 (comment)). ACKs for top commit: naiyoma: tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully. mzumsande: ACK 16bd283 tdb3: ACK 16bd283 glozow: ACK 16bd283 dergoegge: ACK 16bd283 Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This small PR adds test coverage for the scenario of connecting to ourself, leading to an immediate disconnect:
bitcoin/src/net_processing.cpp
Lines 3729 to 3735 in 2f6dca4
This logic has been first introduced by Satoshi in October 2009, together with a couple of other changes and a version bump to "v0.1.6 BETA" (see commit cc0b4c3).