Skip to content

Conversation

stratospher
Copy link
Contributor

Fixes #29508.

Make sure that v2 handshake is complete before comparing getpeerinfo outputs so that transport_protocol_type isn't stuck at 'detecting'.

This is done by adding a wait_until statement till transport_protocol_type = v2 so that bitcoind waits until the v2 handshake is complete. (on the python side, this is ensured by default since wait_for_handshake = True inside add_p2p_connection())

Make sure that v2 handshake is complete before comparing getpeerinfo
outputs so that `transport_protocol_type` isn't stuck at 'detecting'.

- on the python side, this is ensured by default
`wait_for_handshake = True`  inside `add_p2p_connection()`.
- on the c++ side, add a wait_until statement till
`transport_protocol_type = v2`  so that v2 handshake is complete.

Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 29, 2024

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 Sjors, vasild, mzumsande, achow101
Concept ACK hebasto

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 Feb 29, 2024
@stratospher
Copy link
Contributor Author

One way to test this patch would be using this diff. The intermittent failure happens when v2 handshake is complete on the python side but not on bitcoind side. That is - bitcoind is in SendState::READY (meaning bitcoind has sent garbage terminator + version packet to our python P2PInterface and wait_for_v2_handshake=True) and RecvState paused in something like RecvState::GARB_GARBTERM.

this diff delays the garbage terminator being sent from the python side using sleep so that bitcoind can be in SendState::READY before RecvState::GARB_GARBTERM.

diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index dc04696114..b546bad4cf 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -270,10 +270,10 @@ class P2PConnection(asyncio.Protocol):
             # and sends response after deriving shared ECDH secret using received ellswift bytes
             length, response = self.v2_state.complete_handshake(BytesIO(self.recvbuf))
             self.recvbuf = self.recvbuf[length:]
-            if response:
-                self.send_raw_message(response)
-            else:
-                return  # only after response is sent can `authenticate_handshake()` be done
 
         # `self.v2_state.peer` is instantiated only after shared ECDH secret/BIP324 derived keys and ciphers
         # is derived in `complete_handshake()`.
@@ -283,6 +283,9 @@ class P2PConnection(asyncio.Protocol):
         if not is_mac_auth:
             raise ValueError("invalid v2 mac tag in handshake authentication")
         self.recvbuf = self.recvbuf[length:]
+        if response:
+            import time; time.sleep(5)
+            self.send_raw_message(response)
         if self.v2_state.tried_v2_handshake:
             # for v2 outbound connections, send version message immediately after v2 handshake
             if self.p2p_connected_to_node:

@maflcko maflcko added this to the 27.0 milestone Feb 29, 2024
@Sjors
Copy link
Member

Sjors commented Feb 29, 2024

ACK 0487f91

Using your patch I was able to reproduce the failure, and using this PR the test would pass.

I might actually be useful to have a (deterministically random) delay in test_framework/p2p.py where you put it. 0.1 seconds already does the trick for me.

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.

Concept ACK

I opened #29515 before seeing this one, sorry for the noise.

Comment on lines +116 to +117
if self.options.v2transport:
self.wait_until(lambda: self.nodes[0].getpeerinfo()[no_version_peer_id]["transport_protocol_type"] == "v2")
Copy link
Contributor

@vasild vasild Feb 29, 2024

Choose a reason for hiding this comment

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

Does it never go in "detecting" for v1?

Edit: I guess it does, and in this case, this would be more robust as waiting for the value to become one of "v1" or "v2" or waiting for it to not be "detecting", regardless of self.options.v2transport

Copy link
Contributor

Choose a reason for hiding this comment

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

Not in this test, because the test framework always sets the -v2transport bitcoind arg explicitly, see here. If we just run python3 rpc_net.py, V1Transport::GetInfo() is called, and that can never be "detecting". If it wasn't like this, the value here would always be "detecting" instead of "v1", because with send_version=False ,the node cannot actually know whether it's v1 having received no data at all - it just assumes it because it doesn't know about v2.

@hebasto
Copy link
Member

hebasto commented Feb 29, 2024

Concept ACK.

@fanquake fanquake requested a review from mzumsande February 29, 2024 12:19
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 0487f91

@DrahtBot DrahtBot requested a review from hebasto February 29, 2024 15:37
@mzumsande
Copy link
Contributor

Code Review ACK 0487f91

@DrahtBot DrahtBot removed the request for review from mzumsande February 29, 2024 15:43
@achow101
Copy link
Member

ACK 0487f91

@achow101 achow101 merged commit 61aa981 into bitcoin:master Feb 29, 2024
@stratospher stratospher deleted the wait-v2-detecting-over branch March 1, 2024 03:07
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024
Make sure that v2 handshake is complete before comparing getpeerinfo
outputs so that `transport_protocol_type` isn't stuck at 'detecting'.

- on the python side, this is ensured by default
`wait_for_handshake = True`  inside `add_p2p_connection()`.
- on the c++ side, add a wait_until statement till
`transport_protocol_type = v2`  so that v2 handshake is complete.

Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>

Github-Pull: bitcoin#29511
Rebased-From: 0487f91
kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2024
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
, bitcoin#29372, bitcoin#29460, bitcoin#29358, bitcoin#29511, bitcoin#29390, bitcoin#29431, bitcoin-core/gui#788 (BIP324 backports: part 4)

4735b82 merge bitcoin#29431: disconnection scenarios during v2 handshake (Kittywhiskers Van Gogh)
cc6b88e merge bitcoin-core/gui#788: update session ID tooltip (Kittywhiskers Van Gogh)
2455862 merge bitcoin#29390: speedup bip324_cipher.py unit test (Kittywhiskers Van Gogh)
062aaf1 merge bitcoin#29511: Fix intermittent failure in rpc_net.py --v2transport (Kittywhiskers Van Gogh)
54972e8 merge bitcoin#29358: use v2 everywhere for P2PConnection if --v2transport is enabled (Kittywhiskers Van Gogh)
4cce72f test: add missing debug log assertion in `p2p_invalid_messages.py` (Kittywhiskers Van Gogh)
bd2fe61 merge bitcoin#29460: assert rpc error for addnode v2transport not enabled (Kittywhiskers Van Gogh)
5ee15fa merge bitcoin#29372: fix intermittent failure in `rpc_setban.py --v2transport` (Kittywhiskers Van Gogh)
e278818 merge bitcoin#29352: fix intermittent failure in p2p_v2_earlykeyresponse (Kittywhiskers Van Gogh)
6b2a8b5 merge bitcoin#24748: functional tests for v2 P2P encryption (Kittywhiskers Van Gogh)
32500f2 merge bitcoin#27653: add unit test coverage for Python ECDSA implementation (Kittywhiskers Van Gogh)
9f476c6 net: add Dash network message short IDs, allocate range 128 onwards (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6329

  * Dash-specific P2P messages have been allocated short IDs after 128 based on a prior suggestion ([source](#6280 (comment))) as there are 255 potential short IDs (ID `0` is reserved for V1 fallback, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1019)) and upstream uses 28 short IDs (though Dash has left ID `5` blank as we do not implement the `FEEFILTER` message, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1024)).

    As it is unlikely that upstream will utilize more than 127 short IDs (and the spec refers to IDs after 32 as "undefined", [source](https://github.com/bitcoin/bips/blob/52894e1aa7af27f4ae7836a391643124c71e2639/bip-0324.mediawiki#v2-bitcoin-p2p-message-structure)), there shouldn't be an adverse effect to utilizing IDs >=128. The unified array of short IDs are accessible through `V2ShortIDs()`.

    * As there are checks to see if an ID *can* be valid by checking against the array size (which wouldn't work here as we create an array of 256 entries combining both upstream and Dash's allocated short IDs, filling the rest with blank values and we cannot ignore blank values to know if a value is unallocated as the blanking could also signal a reservation, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1048-L1052)), such a check needs to be done by using `IsValidV2ShortID()`.
    * `V2ShortIDs()` isn't as elegant as desired as `std::fill` and `std::copy` are not `constexpr` until C++20 ([source](https://en.cppreference.com/w/cpp/algorithm/fill), [source](https://en.cppreference.com/w/cpp/algorithm/copy)) and until we drop C++17 support, we have to be mindful of that.

  * `masternode connect` will now _attempt_ to establish a P2Pv2 connection if the node *initiating* the connection has opted-in using the new argument (`v2transport`) and the node was started with P2Pv2 enabled (using the launch argument, `-v2transport`).

    This mirrors changes to behavior to `addconnection` introduced in [bitcoin#24748](bitcoin#24748)

  * The oversized payload test in `p2p_invalid_messages.py` will expect an excessively large message of size of `3145729` bytes (and in P2Pv2, `3145742` bytes), as opposed to upstream's `4000001` and `4000014` bytes respectively as Dash has a lower protocol limit of 3MiB ([source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.h#L80-L81)) vs Bitcoin's 4MB ([source](https://github.com/bitcoin/bitcoin/blob/225718eda89d65a7041c33e1994d3bf7bd71bbdd/src/net.h#L62-L63))

  ## 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
  - [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:
  PastaPastaPasta:
    utACK 4735b82
  UdjinM6:
    light ACK 4735b82

Tree-SHA512: 4a9d586133633c109e6a8f20a6c6c5e4b24185fb616bcd8568e546b6e9f886e7a8707e811fd070bbe32c40df269f89a56343a24b67242c6147f9df27275af599
@bitcoin bitcoin locked and limited conversation to collaborators Mar 6, 2025
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.

ci: failure in rpc_net.py --v2transport
9 participants