test, ci: add --v1transport option, add --v2transport to a CI task #29483

mzumsande · 2024-02-26T19:51:42Z

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).

DrahtBot · 2024-02-26T19:51:45Z

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	stratospher, tdb3, vasild, achow101
Concept ACK	kevkevinpal

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:

#29122 (test: adds outbound eviction functional tests, updates comment in ConsiderEviction by sr-gi)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28687 (C++20 std::views::reverse by stickies-v)
#28241 (Silent payment index (for light wallets and consistency check) by Sjors)
#26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)

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.

kevkevinpal · 2024-02-28T02:35:45Z

Concept ACK

it seems like we followed a similar approach for --legacy-wallet and --descriptors

fanquake · 2024-02-29T14:59:29Z

Could rebase / update PR description. I think this could also be worthwhile having in 27.

mzumsande · 2024-02-29T15:31:24Z

updated PR description, it's rebased already.

Since this runs a large numbers of tests in the CI with v2 that previously weren't, there is a certain risk of new intermittent failures (though I didn't encounter any locally and the CI runs so far didn't either).
If it's meant for 27, I'll add a temporary no-merge commit to enable v2 in each CI task running the functional tests - just to have a few more runs with different CIs, I'll remove that commit again this evening.

This option beats the --v2transport option and is meant to be used in test_runner.py. It applies these to a few tests that are particulary interesting in terms of the transport type. This ensures that these tests arei always run with both v1 and v2, irrespective of whether the global --v2transport test_runner option is set or not.

Otherwise, a run with test_runner.py --v2transport=1 --previous-releases --extended would hit the removed assert for wallet_backwards_compatibility.py

mzumsande · 2024-02-29T20:47:20Z

Since this runs a large numbers of tests in the CI with v2 that previously weren't, there is a certain risk of new intermittent failures (though I didn't encounter any locally and the CI runs so far didn't either).
If it's meant for 27, I'll add a temporary no-merge commit to enable v2 in each CI task running the functional tests - just to have a few more runs with different CIs, I'll remove that commit again this evening.

ok, no intermittent bug was found that way, but a non-intermittent problem, fixed in 3a25a57

stratospher

ACK ecc036c.

tdb3 · 2024-03-06T03:01:59Z

Great catch and fix.
ACK for ecc036c.

Checks

Ran a quick set of sanity checks. Received expected results in every check.

Added log.info() statements to BitcoinTestFramework.setup() showing v1transport and v2transport status, as well as in TestNode's constructor showing v2transport status. Checked test_framework logs for state of v1transport and v2transport.

Not globally specifying --v2transport:

test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp -u --nocleanup test/functional/p2p_block_sync*
p2p_block_sync.py --v1transport passed and logs showed v1transport enabled and v2transport disabled for the test (including for each node involved).
p2p_block_sync.py --v2transport passed and logs showed v1transport disabled and v2transport enabled for the test (including for each node involved).

Globally specifying --v2transport:

test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp -u --v2transport --nocleanup test/functional/p2p_block_sync*
p2p_block_sync.py --v1transport passed and logs showed v1transport enabled and v2transport disabled for the test (including for each node involved).
p2p_block_sync.py --v2transport passed and logs showed v1transport disabled and v2transport enabled for the test (including for each node involved).

v2transport is now enabled by default (PR #29347). Initially, I was considering suggesting v2transport being enabled by default for tests, however the suggestion (in #29358 (comment)) to default to v1 now and switch to v2 after more v2 adoption (e.g. when the majority of the network is using v2) makes more sense. For now, it seems that using v2 would require either the global setting (test_runner.py --v2transport) or explicit declaration (--v2transport) in the test in BASE_SCRIPTS (so new tests should be explicit). A future PR could introduce defaulting to v2 for tests.

vasild

ACK ecc036c

Looks good as it is, but I wonder if we can do better, see the comment below.

vasild · 2024-03-08T08:46:10Z

test/functional/test_framework/test_framework.py

@@ -191,6 +191,8 @@ def parse_args(self):
        parser.add_argument("--timeout-factor", dest="timeout_factor", type=float, help="adjust test timeouts by a factor. Setting it to 0 disables all timeouts")
        parser.add_argument("--v2transport", dest="v2transport", default=False, action="store_true",
                            help="use BIP324 v2 connections between all nodes by default")
+        parser.add_argument("--v1transport", dest="v1transport", default=False, action="store_true",


nit, in the commit message of 547aacf test: add -v1transport option and use it in test_runner:
s/-v1transport/--v1transport/
s/arei/are/

vasild · 2024-03-08T08:56:15Z

test/functional/test_framework/test_framework.py

+        if self.options.v1transport:
+            self.options.v2transport=False


v1 overriding v2 is somewhat non intuitive (v2 overriding v1 is as well). What we really want here is a local option overriding a global one, regardless of which one is v1 and v2. Can we have this somehow? Maybe have test_runner.py when ran like e.g. test_runner.py --v1 and when picking entries from BASE_SCRIPTS[] to check that an entry like p2p_timeouts.py --v2 already contains a conflicting option and omit that entry altogether?

Also, it would be best if an individual test is ran like test/functional/p2p_fingerprint.py --v1 --v2 to emit an error and refuse to start (e.g. "conflicting options given") instead of running in v1 mode.

Concept ACK for a possible follow-up - though we should probably be careful with introspection of args so that we don't create a second monster like the bitcoind args parser for the functional tests.

achow101 · 2024-03-11T13:10:12Z

ACK ecc036c

… CI task

, bitcoin#29353, bitcoin#29452, bitcoin#29483, bitcoin#30545, bitcoin#31383 (BIP324 backports: part 5) c6f23a7 merge bitcoin#31383: Add missing node.setmocktime(self.mocktime) to p2p_ibd_stalling.py (Kittywhiskers Van Gogh) 9072a10 merge bitcoin#30545: fix intermittent failures in feature_proxy.py (Kittywhiskers Van Gogh) 7e2d435 merge bitcoin#29483: add --v1transport option, add --v2transport to a CI task (Kittywhiskers Van Gogh) 7e59a96 merge bitcoin#29452: document that BIP324 on by default (Kittywhiskers Van Gogh) 0f3b5e0 merge bitcoin#29353: adhere to typical VERSION message protocol flow (Kittywhiskers Van Gogh) dfdddfd rpc: enable `v2transport` for `masternode connect` when capable (Kittywhiskers Van Gogh) 3c16361 merge bitcoin#29356: make v2transport arg in addconnection mandatory and few cleanups (Kittywhiskers Van Gogh) c53cd93 merge bitcoin#29347: enable v2transport by default (Kittywhiskers Van Gogh) 7dcf561 merge bitcoin#27452: cover addrv2 anchors by adding TorV3 to CAddress in messages.py (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * When backporting [bitcoin#27452](bitcoin#27452) in `feature_anchors.py`, `P2P_SERVICES` (`NODE_NETWORK | NODE_HEADERS_COMPRESSED`) has been replaced with `NODE_NETWORK` as the former evaluates to a value greater than `256` (specifically `2049`), which causes test failure. The replacement value is acceptable as `NODE_NETWORK` is the desired service flag expected by Dash Core ([source](https://github.com/dashpay/dash/blob/779e4295ad253bf909a7c45ec02743e580abc61b/src/protocol.cpp#L249-L254)). <details> <summary>Test failure:</summary> ``` dash@89afd55ae77e:/src/dash$ ./test/functional/feature_anchors.py 2024-12-14T12:31:22.244000Z TestFramework (INFO): PRNG seed is: 8365703189892653614 2024-12-14T12:31:22.244000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_j0ya02yu 2024-12-14T12:31:22.776000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist 2024-12-14T12:31:22.776000Z TestFramework (INFO): Add 2 block-relay-only connections to node 2024-12-14T12:31:24.781000Z TestFramework (INFO): Add 5 inbound connections to node 2024-12-14T12:31:29.843000Z TestFramework (INFO): Check node connections 2024-12-14T12:31:30.848000Z TestFramework (INFO): Check the addresses in anchors.dat 2024-12-14T12:31:30.848000Z TestFramework (INFO): Perturb anchors.dat to test it doesn't throw an error during initialization 2024-12-14T12:31:31.356000Z TestFramework (INFO): When node starts, check if anchors.dat doesn't exist anymore 2024-12-14T12:31:31.357000Z TestFramework (INFO): Ensure addrv2 support 2024-12-14T12:31:32.364000Z TestFramework (INFO): Add 256-bit-address block-relay-only connections to node 2024-12-14T12:31:33.368000Z TestFramework (INFO): Check for addrv2 addresses in anchors.dat 2024-12-14T12:31:33.369000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 161, in main self.run_test() File "/src/dash/./test/functional/feature_anchors.py", line 135, in run_test new_data[services_index] = P2P_SERVICES ValueError: byte must be in range(0, 256) 2024-12-14T12:31:33.870000Z TestFramework (INFO): Stopping nodes 2024-12-14T12:31:33.871000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_j0ya02yu 2024-12-14T12:31:33.871000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_j0ya02yu/test_framework.log ``` </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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK c6f23a7 PastaPastaPasta: utACK c6f23a7; Tree-SHA512: ca134432d000d521827a20c75c03913421fe52a31fda1cbf632e0b207c31728406feb090469d592d8e2fd8d64298faa2752ff703de79f737a62c276c6a231097

…sport to 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 bitcoin#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

mzumsande marked this pull request as draft February 26, 2024 19:51

mzumsande mentioned this pull request Feb 26, 2024

test: use v2 everywhere for P2PConnection if --v2transport is enabled #29358

Merged

mzumsande force-pushed the 202402_run_v2_in_ci branch from 108cbce to c2408cf Compare February 27, 2024 15:18

DrahtBot mentioned this pull request Feb 27, 2024

Do not log p2p bip61 reject messages, improve log, add tests #28429

Closed

mzumsande marked this pull request as ready for review February 28, 2024 01:26

mzumsande marked this pull request as draft February 29, 2024 17:14

DrahtBot added the CI failed label Feb 29, 2024

mzumsande force-pushed the 202402_run_v2_in_ci branch from 9df6ce8 to a6da22b Compare February 29, 2024 17:44

DrahtBot removed the CI failed label Feb 29, 2024

mzumsande added 2 commits February 29, 2024 13:50

test: ignore --v2transport for older versions instead of asserting

3a25a57

Otherwise, a run with test_runner.py --v2transport=1 --previous-releases --extended would hit the removed assert for wallet_backwards_compatibility.py

ci: add --v2transport to an existing CI job

ecc036c

mzumsande force-pushed the 202402_run_v2_in_ci branch 2 times, most recently from 56b2d87 to ecc036c Compare February 29, 2024 20:43

mzumsande marked this pull request as ready for review February 29, 2024 20:47

DrahtBot mentioned this pull request Feb 29, 2024

build: Bump g++ minimum supported version to 11 #29091

Merged

stratospher reviewed Mar 1, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 1, 2024

Remove the legacy wallet and BDB dependency #28710

Merged

vasild approved these changes Mar 8, 2024

View reviewed changes

achow101 merged commit 02c7fd8 into bitcoin:master Mar 11, 2024

mzumsande deleted the 202402_run_v2_in_ci branch March 11, 2024 14:41

bitcoin deleted a comment from MarcusMWilliams Mar 11, 2024

kwvg added a commit to kwvg/dash that referenced this pull request Dec 14, 2024

merge bitcoin#29483: add --v1transport option, add --v2transport to a…

7e2d435

… CI task

bitcoin locked and limited conversation to collaborators Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test, ci: add --v1transport option, add --v2transport to a CI task #29483

test, ci: add --v1transport option, add --v2transport to a CI task #29483

Uh oh!

mzumsande commented Feb 26, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Feb 26, 2024 •

edited

Loading

Uh oh!

kevkevinpal commented Feb 28, 2024

Uh oh!

fanquake commented Feb 29, 2024

Uh oh!

mzumsande commented Feb 29, 2024 •

edited

Loading

Uh oh!

mzumsande commented Feb 29, 2024

Uh oh!

stratospher left a comment

Uh oh!

tdb3 commented Mar 6, 2024 •

edited

Loading

Uh oh!

vasild left a comment •

edited

Loading

Uh oh!

vasild Mar 8, 2024

Uh oh!

vasild Mar 8, 2024

Uh oh!

mzumsande Mar 11, 2024

Uh oh!

achow101 commented Mar 11, 2024

Uh oh!

Uh oh!

test, ci: add --v1transport option, add --v2transport to a CI task #29483

test, ci: add --v1transport option, add --v2transport to a CI task #29483

Uh oh!

Conversation

mzumsande commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

kevkevinpal commented Feb 28, 2024

Uh oh!

fanquake commented Feb 29, 2024

Uh oh!

mzumsande commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Feb 29, 2024

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checks

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

vasild Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

mzumsande Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 11, 2024

Uh oh!

Uh oh!

mzumsande commented Feb 26, 2024 •

edited

Loading

DrahtBot commented Feb 26, 2024 •

edited

Loading

mzumsande commented Feb 29, 2024 •

edited

Loading

tdb3 commented Mar 6, 2024 •

edited

Loading

vasild left a comment •

edited

Loading