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

mzumsande commented at 7:51 pm on February 26, 2024: contributor

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 (review), 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 commented at 7:51 pm on February 26, 2024: contributor

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.

mzumsande marked this as a draft on Feb 26, 2024

mzumsande force-pushed on Feb 27, 2024

mzumsande marked this as ready for review on Feb 28, 2024

kevkevinpal commented at 2:35 am on February 28, 2024: contributor

Concept ACK

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

fanquake commented at 2:59 pm on February 29, 2024: member

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

mzumsande commented at 3:31 pm on February 29, 2024: contributor

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.

mzumsande marked this as a draft on Feb 29, 2024

DrahtBot added the label CI failed on Feb 29, 2024

test: add -v1transport option and use it in test_runner

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.

547aacff08

mzumsande force-pushed on Feb 29, 2024

DrahtBot removed the label CI failed on Feb 29, 2024

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

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

3a25a575f0

ci: add --v2transport to an existing CI job ecc036c5d6

mzumsande force-pushed on Feb 29, 2024

mzumsande commented at 8:47 pm on February 29, 2024: contributor

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 3a25a575f0c455b50abf95d85c0ba8de3cf53a87

mzumsande marked this as ready for review on Feb 29, 2024

stratospher commented at 7:13 am on March 1, 2024: contributor

ACK ecc036c.

tdb3 commented at 3:01 am on March 6, 2024: contributor

Great catch and fix. ACK for ecc036c5d63fb4bdff5caeede88baeb85bf62b05.

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

in test/functional/test_framework/test_framework.py:194 in ecc036c5d6

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

vasild commented at 8:46 am on March 8, 2024:

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

in test/functional/test_framework/test_framework.py:212 in ecc036c5d6

207@@ -206,6 +208,8 @@ def parse_args(self):
208         config = configparser.ConfigParser()
209         config.read_file(open(self.options.configfile))
210         self.config = config
211+        if self.options.v1transport:
212+            self.options.v2transport=False

vasild commented at 8:56 am on March 8, 2024:

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.

mzumsande commented at 2:40 pm on March 11, 2024:

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.

vasild approved

vasild commented at 8:58 am on March 8, 2024: contributor

ACK ecc036c5d63fb4bdff5caeede88baeb85bf62b05

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

achow101 commented at 1:10 pm on March 11, 2024: member

ACK ecc036c5d63fb4bdff5caeede88baeb85bf62b05

achow101 merged this on Mar 11, 2024

achow101 closed this on Mar 11, 2024

mzumsande deleted the branch on Mar 11, 2024

bitcoin deleted a comment on Mar 11, 2024

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

Code Coverage

Reviews

Conflicts

Checks