test: use v2 everywhere for P2PConnection if --v2transport is enabled #29358
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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
fd334e0 to
606f4f3
Compare
|
Hmm, "test each commit fails" The problem is that commit 606f4f3 (which switches |
would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits. |
|
Testing looks good so far: (Noob question) I'm struggling to test the changes in |
Rather than disabling, would it make sense to keep these subtests enabled but just not use v2transport? |
That wouldn't work either, because with that last commit we actually use v2 when using P2PConnection (if
Yes, that makes sense I'll try that out! |
606f4f3 to
d8165db
Compare
|
rebased and always use v1 instead of disabling completely in the three slow (sub)tests. |
|
Concept ACK |
|
Tested ACK d8165db.
Thanks for the clarification. Certain tests take longer to run, to be expected with the added sub-tests. |
|
Will address feedback and #29356 (comment) after the merge of #29356 |
By adding to the test framework a wait until the v2 handshake is completed, so that p2p_sendtxrcncl.py (which doesn't need to be changed itself) doesnt't send out any other messages before that.
by disabling some sub-tests that test v1-specific features, and adapting others to v2.
Sending multiple large messages is rather slow with the non-optimized python implementation of ChaCha20. Apart from the slowness, these tests would also run successfully with v2.
d8165db to
b9912e2
Compare
|
I have addressed feedback and squashed several test fixes into the last commit, so that the "test each commit" CI is hopefully green now. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review. |
| @@ -69,11 +69,8 @@ def run_test(self): | |||
| with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']): | |||
| no_verack_node.send_message(msg_ping()) | |||
|
|
|||
| # With v2, non-version messages before the handshake would be interpreted as part of the key exchange. | |||
There was a problem hiding this comment.
test/functional/test_runner.py contains:
'rpc_net.py',
'rpc_net.py --v2transport',So, on master, when I execute test_runner.py it will run this test two times, once in v1 mode and once in v2 mode. What about if I run, with this PR, test_runner.py --v2transport? Would that execute rpc_net.py two times and both in v2 mode? If yes, then what about adding --v1transport option as well? Or maybe just change the above to just:
'rpc_net.py',and expect that callers will run test_runner.py and test_runener.py --v2transport?
There was a problem hiding this comment.
Would that execute
rpc_net.pytwo times and both in v2 mode?
Yes, although it's not really related to this PR it's the same on master (just that P2PConnections use v1 both times there).
I had thought about this before and was thinking of addressing it in a follow-up PR, which would also add a CI job for --v2transport.
The downside of dropping the --v2transport specific jobs from test_runner.py is that these would be executed a lot less by individuals running the test suite (even if we have a --v2transport CI job for one specific platform). So I kind of tend to prefer something like --v1transport, especially since the test that run twice currently don't take a lot of time compared to the longest running jobs.
There was a problem hiding this comment.
Alright, "it's the same on master" that is a good point!
So, if we have --v1transport (for each individual test and for test_runner.py?) then what would happen if ran without either one of --v1transport or --v2transport?
There was a problem hiding this comment.
We'd have to make some kind of decision what to default to - maybe v1 now and switch to v2 after a majority of the network has upgraded?
My idea for now was to only add --v1transport in test_runner.py to the few tests that are run twice, not everywhere.
There was a problem hiding this comment.
I would either expect that the internally set option can not be overridden outside because the developers have decided that this test has to run this setting to keep good coverage no matter what the user thinks or that the test is skipped when the internal option is not compatible with what is provided from outside. Depends a bit on how important v1 test coverage is for us in the short term I think.
There was a problem hiding this comment.
I have opened the draft PR #29483 with a proposal / to continue this discussion about CI and test_runner.
This changes the default behavior, individual tests can overwrite this option. As a result, it is possible to run the entire test suite with --v2transport, and all connections to the python p2p will then use it. Also adjust several tests that are already running with --v2transport in the test runner (although they actually made v1 connection before this change). This is done in the same commit so that there isn't an intermediate commit in which the CI fails.
b9912e2 to
bf5662c
Compare
| if supports_v2_p2p: | ||
| p2p_conn.wait_until(lambda: p2p_conn.v2_state.tried_v2_handshake) |
There was a problem hiding this comment.
in commit 5fc9db5, method add_outbound_p2p_connection:
Is this wait needed? IIUC, the wait in the line below should be sufficient, as it can only pass if a v2 handshake has already happened
There was a problem hiding this comment.
You are right, it's not strictly needed in add_outbound_p2p_connection, (though it is in add_p2p_connection, where the corresponding wait depends on the send_version option which may be unset).
Though I still think it makes sense conceptually because it kind of decouples the v2 logic from the message logic. If we'd introduce a similar parameter send_version also for add_outbound_p2p_connection (e.g. because we might want to craft a custom version message like p2p_sendtxrcncl.py does) we'd need it.
There was a problem hiding this comment.
Thanks for elaborating, that makes sense!
|
tACK bf5662c |
…o a CI task ecc036c ci: add --v2transport to an existing CI job (Martin Zumsande) 3a25a57 test: ignore --v2transport for older versions instead of asserting (Martin Zumsande) 547aacf test: add -v1transport option and use it in test_runner (Martin Zumsande) Pull request description: This suggests a strategy to run the functional tests with both v1 and v2 transport in the CI. **Status Quo:** There is both the global `--v2transport` option for the `test_runner.py` (not enabled by default), plus the possibility to specify `--v2transport` for particular tests, which is used for a handful of tests. Currently, when running `test_runner.py --v2transport`, these tests are run twice with the same `--v2transport` configuration, as has been noted in #29358 (comment), which is wasteful. **Suggested Change:** Fix this by adding a `--v1transport` option and using it in `test_runner.py`, so that irrespective of the global `--v2transport` flag, the tests that run twice use v1 in one run and v2 in the other. Also add `--v2transport` to one CI task (`multiprocess, i686, DEBUG`). This means, that for each CI task, the majority of functional tests will run once using the global `--v2transport` option if provided, while a few selected tests will always run two times, once with `v1` and once with `v2`. **Rationale:** A simpler alternative would have been to remove all test-specific `--v2transport` commands from `test_runner.py` and just enable `--v2transport` option for a few CI tasks. I didn't do that because it would have meant that v2 would never be running in the CI for some platforms, and also be run a lot less locally by users and devs (who would have to actively enable the `--v2transport` option). ACKs for top commit: tdb3: ACK for ecc036c. achow101: ACK ecc036c stratospher: ACK ecc036c. vasild: ACK ecc036c Tree-SHA512: 375b2293d49991f2fbd8e1bb646c0034004a09cee36063bc32996b721323eb19a43d7b2f36b3f9a3fdca846d74f48d8f1387565c03ef5d34b3481d2a0fe1d328
, 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
#24748 added v2 transport to the python
P2PConnection, but so far each test that wants to make use of it needs to enable it on an individual basis.This PR changes it so that if the test suite is run with
--v2transportoption, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also whereverP2PConnectionis used. Individual tests can override this global option.To do that, a few tests need to be adjusted.
In addition, I added a commit to always use v1 in a few select subtests that send a large number of large messages (e.g. large reorgs). These tests don't have a fundamental problem with v2 but become very slow due to the unoptimised python ChaCha20 implementation (~30 minutes on my computer, so probably not suitable to be run in the CI).
As a result,
python3 test_runner.py --v2transportshould succeed and usev2everywhere (unless v1 is chosen explicitly).[Edit]: To make the "test each commit" CI pass, several test fixes were squashed into the last commit, which actually enables v2 p2p for
P2PConnection. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review.