Skip to content
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: Add missing sync on send_version in peer_connect #28782

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 3, 2023

Without the sync, the logic will be racy. For example, p2p_sendtxrcncl.py is failing locally (and on CI occasionally), because non-version messages will be sent before the version message:

        self.log.info('SENDTXRCNCL with version=0 triggers a disconnect')
        sendtxrcncl_low_version = create_sendtxrcncl_msg()
        sendtxrcncl_low_version.version = 0
        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
        with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]):
            peer.send_message(sendtxrcncl_low_version)
            peer.wait_for_disconnect()
 test  2023-11-02T08:15:19.620000Z TestFramework (INFO): SENDTXRCNCL with version=0 triggers a disconnect 
 test  2023-11-02T08:15:19.621000Z TestFramework.p2p (DEBUG): Connecting to Bitcoin Node: 127.0.0.1:11312 
 test  2023-11-02T08:15:19.624000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:11312 
 test  2023-11-02T08:15:19.798000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_sendtxrcncl(version=0, salt=2) 
 test  2023-11-02T08:15:19.799000Z TestFramework.p2p (DEBUG): Send message to 127.0.0.1:11312: msg_version(nVersion=70016 nServices=9 nTime=Thu Nov  2 08:15:19 2023 addrTo=CAddress(nServices=1 net=IPv4 addr=127.0.0.1 port=11312) addrFrom=CAddress(nServices=1 net=IPv4 addr=0.0.0.0 port=0) nNonce=0x369AC031CDA96022 strSubVer=/python-p2p-tester:0.0.3/ nStartingHeight=-1 relay=1) 
 node0 2023-11-02T08:15:19.804409Z [net] [net.cpp:3676] [CNode] [net] Added connection peer=0 
 node0 2023-11-02T08:15:19.805256Z [net] [net.cpp:1825] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:55964 accepted 
 node0 2023-11-02T08:15:19.809861Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: sendtxrcncl (12 bytes) peer=0 
 node0 2023-11-02T08:15:19.810297Z [msghand] [net_processing.cpp:3582] [ProcessMessage] [net] non-version message before version handshake. Message "sendtxrcncl" from peer=0 
 node0 2023-11-02T08:15:19.810928Z [msghand] [net_processing.cpp:3356] [ProcessMessage] [net] received: version (111 bytes) peer=0 
...
 test  2023-11-02T09:35:20.166000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: '''' 
                                           def test_function():
                                               if check_connected:
                                                   assert self.is_connected
                                               return test_function_in()
                                   '''
 test  2023-11-02T09:35:20.187000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/p2p_sendtxrcncl.py", line 188, in run_test
                                       peer.wait_for_disconnect()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 478, in wait_for_disconnect
                                       self.wait_until(test_function, timeout=timeout, check_connected=False)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/p2p.py", line 470, in wait_until
                                       wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-s390x-linux-gnu/test/functional/test_framework/util.py", line 275, in wait_until_helper_internal
                                       raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
                                   AssertionError: Predicate ''''
                                           def test_function():
                                               if check_connected:
                                                   assert self.is_connected
                                               return test_function_in()
                                   ''' not true after 4800.0 seconds

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 3, 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 mzumsande

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
  • #24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

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

ACK fa02598

I tested this by adding a sleep to connection_made before sending the message.
However p2p_sendtxrcncl.py will then fail later in the SENDTXRCNCL if block-relay-only triggers a disconnect part because add_outbound_p2p_connection has the same problem. Do you want to fix that too here, or in another PR?

@fanquake fanquake merged commit 1162d04 into bitcoin:master Nov 8, 2023
16 checks passed
@maflcko maflcko deleted the 2311-test-less-p2p-race- branch November 8, 2023 10:20
fanquake added a commit to bitcoin-core/gui that referenced this pull request Nov 9, 2023
… sent in add_outbound_p2p_connection

faa2ad8 test: Add missing wait for version to be sent in add_outbound_p2p_connection (MarcoFalke)

Pull request description:

  Can be tested with:

  ```diff
  diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
  index b1ed97b794..eb4f72c6b6 100755
  --- a/test/functional/test_framework/p2p.py
  +++ b/test/functional/test_framework/p2p.py
  @@ -205,6 +205,7 @@ class P2PConnection(asyncio.Protocol):
           assert not self._transport
           logger.debug("Connected & Listening: %s:%d" % (self.dstaddr, self.dstport))
           self._transport = transport
  +        import time;time.sleep(.1);
           if self.on_connection_send_msg:
               self.send_message(self.on_connection_send_msg)
               self.on_connection_send_msg = None  # Never used again
  ```

  Found and reported by mzumsande in bitcoin/bitcoin#28782 (review)

ACKs for top commit:
  mzumsande:
    ACK faa2ad8

Tree-SHA512: 863f06125dec40cccaa852d0d8ca2e2b9c0b74610205e9fd6c9c279bdf36801ff475e3d873fd1b18172eb8220e17b2caff60069ce63512e569934a43f27d03fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants