Skip to content

test: Add missing sync on send_version in peer_connect #28782

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2023
Merged

test: Add missing sync on send_version in peer_connect #28782

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 •
edited
Loading

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.

@DrahtBot DrahtBot added the Tests label Nov 3, 2023
@fanquake fanquake requested a review from mzumsande November 3, 2023 14:53
This was referenced Nov 3, 2023
Open
Merged
mzumsande
mzumsande reviewed Nov 7, 2023
View reviewed changes
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
@maflcko maflcko deleted the 2311-test-less-p2p-race- branch November 8, 2023 10:20
@maflcko maflcko mentioned this pull request Nov 8, 2023
Merged
fanquake added a commit to bitcoin-core/gui that referenced this pull request Nov 9, 2023
@fanquake
Merge bitcoin/bitcoin#28822: test: Add missing wait for version to be…
c2da8c5 
… 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 b1ed97b..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
kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024
@kwvg
merge bitcoin#28782: Add missing sync on send_version in peer_connect
e6d5ecd
kwvg added a commit to kwvg/dash that referenced this pull request Oct 16, 2024
@kwvg
merge bitcoin#28782: Add missing sync on send_version in peer_connect
c0b3062
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 22, 2024
@PastaPastaPasta
Merge #6329: backport: merge bitcoin#20524, bitcoin#26553, bitcoin#27986
dd629cf 
, bitcoin#28645, bitcoin#28632, bitcoin#28782, bitcoin#28822, bitcoin#29006, bitcoin#29212, merge bitcoin-core/gui#754, partial bitcoin#23443, bitcoin#26448 (BIP324 backports: part 3)

aa5311d merge bitcoin#29212: Fix -netinfo backward compat with getpeerinfo pre-v26 (Kittywhiskers Van Gogh)
1a293c7 merge bitcoin#29006: fix v2 transport intermittent test failure (Kittywhiskers Van Gogh)
d0804d4 merge bitcoin#28822: Add missing wait for version to be sent in add_outbound_p2p_connection (Kittywhiskers Van Gogh)
c0b3062 merge bitcoin#28782: Add missing sync on send_version in peer_connect (Kittywhiskers Van Gogh)
35253cd merge bitcoin#28632: make python p2p not send getaddr on incoming connections (Kittywhiskers Van Gogh)
6a4ca62 merge bitcoin#28645: fix `assert_debug_log` call-site bugs, add type checks (Kittywhiskers Van Gogh)
deaee14 merge bitcoin-core/gui#754: Add BIP324-specific labels to peer details (Kittywhiskers Van Gogh)
fffe6e7 merge bitcoin#27986: remove race in the user-agent reception check (Kittywhiskers Van Gogh)
1bf135b merge bitcoin#26553: Fix intermittent failure in rpc_net.py (Kittywhiskers Van Gogh)
5bf245b partial bitcoin#26448: fix intermittent failure in p2p_sendtxrcncl.py (Kittywhiskers Van Gogh)
b7c0030 partial bitcoin#23443: Erlay support signaling (Kittywhiskers Van Gogh)
c709df7 merge bitcoin#20524: Move MIN_VERSION_SUPPORTED to p2p.py (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6324

  * Dependency for #6330

  ## How Has This Been Tested?

  * Changes to Qt client were validated by running the client

    <details>

    <summary>Screenshot</summary>

    ![Transport reporting in Qt](https://github.com/user-attachments/assets/0d551e19-f3a2-4ce7-83d6-5cb3d03b1765)

    </details>

  * Changes to `dash-cli` were validated by running it with different node versions

    **Against a node built on this PR**

    <details>

    <summary>Screenshot</summary>

    ![getinfo with node running the latest build](https://github.com/user-attachments/assets/8cda68cc-727a-4cf3-a4d8-dd6a33331d78)

    </details>

    **Against a node built running the last release**

    <details>

    <summary>Screenshot</summary>

    ![getinfo with node running the latest release](https://github.com/user-attachments/assets/0c6ff476-7cc9-4297-bae5-35d423aba480)

    </details>

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas (note: N/A)
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation (note: N/A)
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    ACK aa5311d
  PastaPastaPasta:
    utACK aa5311d

Tree-SHA512: 8cca324ac988a73c0590a4e9b318e81ce951ac55fb173cf507fa647cab01ab4981e6a06d4792376b4bfb44ff09d4811de05fadb9ba793dd00b4c7965b4b22654
@bitcoin bitcoin locked and limited conversation to collaborators Nov 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@mzumsande mzumsande mzumsande left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @DrahtBot @mzumsande @fanquake