test: Make existing functional tests compatible with --v2transport

mzumsande commented at 10:06 pm on November 6, 2023: contributor

This makes the functional test suite compatible with BIP324, so that python3 test_runner.py --v2transport should succeed (currently, 12 tests fail for me on master). Includes two commits by TheStack I found in an old discussion #28331 (review)

Note that even though all tests should pass, the python p2p.py module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2. Some of the fixed tests were added with --v2transport to the test runner. Though after #24748 we might also want to consider running the entire suite with --v2transport in some CI.

DrahtBot commented at 10:06 pm on November 6, 2023: 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	theStack, stratospher, sipa, achow101
Stale ACK	kashifs

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:

#28521 (net: additional disconnect logging by Sjors)

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 added the label Tests on Nov 6, 2023

mzumsande commented at 10:07 pm on November 6, 2023: contributor

FYI @stratospher @TheStack

theStack commented at 12:07 pm on November 7, 2023: contributor

Concept ACK

maflcko commented at 12:17 pm on November 7, 2023: member

Though after #24748 we might also want to consider running the entire suite with --v2transport in some CI.

I presume this will fix itself, once and if v2 is enabled by default?

kashifs commented at 2:05 pm on November 7, 2023: contributor

tACK 6b77f9

Read through all updated code, pulled branch, compiled from source, ran:

python3 wallet_multiwallet.py --usecli --v2transport

DrahtBot requested review from theStack on Nov 7, 2023

mzumsande commented at 3:12 pm on November 7, 2023: contributor

I presume this will fix itself, once and if v2 is enabled by default?

After enabling v2 by default for the functional tests, we’d probably still want to run some or all of the tests with -v2transport=0 to avoid regressions - so I don’t think that would change much.

maflcko commented at 5:33 pm on November 7, 2023: member

Ok, feel free to add it to any task, for example ci/test/00_setup_env_native_qt5.sh.

mzumsande commented at 5:51 pm on November 7, 2023: contributor

Ok, feel free to add it to any task, for example ci/test/00_setup_env_native_qt5.sh.

Thanks, though I won’t add it to this PR. I’m open to suggestions, but in my opinion the best way to proceed might be to first get #24748 in to upgrade the python p2p framework. I expect we will then need a similar PR to this one to fix some minor issues with the existing tests before turning it on for all tests (something I started working on), and maybe only after we should make a --v2transport task.

stratospher commented at 3:38 pm on November 8, 2023: contributor

Nice! Concept ACK.

“Before, a global -v2transport provided to the test would be dropped when restarting the node within a test.” @mzumsande, wanted to confirm which scenario you’re talking about. i tried it out in this file.

when test is run with --v2transport option and TestNode (with no extra_args passed in the test) is restarted? TestNode restarted as a v2 node on both master and PR.
when test is run with --v2transport option and TestNode (initially no extra_args passed) is restarted with extra_args "v2transport=0"? TestNode restarted as v1 node on master(with 2 lines modified) and restarted as v1 node on PR. is this the scenario which the PR fixes?
when test is run with --v2transport option and TestNode(initially extra_args "v2transport=0") is restarted? TestNode restarted as a v2 node on both master and PR. so both master and PR doesn’t seem to fix it.

mzumsande commented at 8:07 pm on November 8, 2023: contributor

@mzumsande, wanted to confirm which scenario you’re talking about.

I was observing that multiple following tests fail on master with --v2transport e.g. interface_zmq.py, p2p_permissions.py, feature_assumeutxo.py and some wallet tests. These tests have in common that they use self.restart_node() with changing some unrelated extra_args, which would then cause --v2transport to be dropped.

I think the problem is as follows:

On first startup of a node, self.extra_args is set in __init__ (this inserts --v2transport into self.extra_args on master, but not in my branch, which puts the global option into self.args)
If --v2transport=1 in args, and also --v2transport=0 in extra_args, extra_args takes precedence (this can happen with my branch) => this is fine in my opinion.
When we call restart_node without specifying any extra_args, self.extra_args will be used. This is also fine.
When we call restart_node and do specify extra_args, self.extra_args will be ignored, and the passed extra_args will be used, together with the args. That causes the above tests on master to fail.
However, self.extra_args is not updated in restart_node (so another restart_node will use the original self.extra_args), and other tests rely on this behavior! So the passed extra_args are transitory and not saved, so referring to self.extra_args here is not correct when the transitory extra_args are used (this causes your CASE 2 on my branch to fail).

Does this make sense? I’ll try to come up with a fix for this.

test: persist -v2transport over restarts and respect -v2transport=0

Before, a global -v2transport provided to the test would be dropped
when restarting the node within a test and specifying any extra_args.

Fix this by adding "v2transport=1" to args (not extra_args) based
on the global parameter, and deciding for each (re)start of the node
based on this default and test-specific extra_args
(which take precedence over args) whether v2 should be used.

68a9001751

test: enable --v2transport in combination with --usecli

By renaming the "command" send_cli arg. The old name was unsuitable
because the "addnode" RPC has its own "command" arg, leading to
ambiguity when included in kwargs.
Can be tested with
"python3 wallet_multiwallet.py --usecli --v2transport"
which fails on master because of this (python throws a TypeError).

3598a1b5c9

test: enable v2 transport for p2p_node_network_limited.py cc961c2695

test: enable v2 transport for rpc_net.py

- "transport_protocol_type" of inbound peer before version handshake
  is "detecting" on p2p v2 nodes (as opposed to "v1" for p2p v1)
- size of a ping/pong message is 29 bytes (as opposed to 32 for p2p v1)
- for the sendmsgtopeer RPC sub-test, enforce p2p v1 connection to
  have a peer id of zero

2c1669c37a

test: enable v2 transport for p2p_timeouts.py

by skipping the part where we send a non-version message
before the version - this message would be interpreted as
part of the v2 handshake.

35fb9930ad

mzumsande force-pushed on Nov 8, 2023

mzumsande commented at 10:45 pm on November 8, 2023: contributor

I changed the first commit to save the default behavior (global --v2transport) in the TestNode init and add an entry to args, and then deciding in TestNode::start() (which is called for both the initial startup and for restarts) based on this and possible extra_args whether to actually use v2 or not.

in test/functional/test_framework/test_node.py:207 in 68a9001751 outdated

203@@ -198,6 +204,8 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None
204         if extra_args is None:
205             extra_args = self.extra_args
206 
207+        self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args)

stratospher commented at 5:07 am on November 10, 2023:

68a9001: there’s a behaviour difference when test is run with --v2transport option and TestNode(initially "v2transport=0") is restarted with:

extra_args supplied => TestNode restarts as v2 node
extra_args not supplied => TestNode restarts as v1 node

maybe when extra_args is None, we could remove "v2transport" stuff it gets from the old self.extra_args using something like extra_args = list(filter(lambda s: not s.startswith("-v2transport="), extra_args))?

EDIT: realised this won’t work because during TestNode initialisation, extra_args is None and not just during restarts.

theStack commented at 2:54 am on November 11, 2023:

One small drawback of the current way of inspecting extra_args for the v2transport option is that it doesn’t work if the argument is specified in a slightly different format with same meaning. For example, -v2transport=0/-v2transport=1 could also be passed as -nov2transport/-v2transport, -nov2transport=1/-nov2transport=0 (admittedly, no sane person would use the last one though). It might be worth it to consider that in a follow-up, e.g. by enforcing with an assertion that arguments that contain the string “v2transport” must be prefixed by “-” and suffixed by either “=0” or “=1”. As far as I can see, that’s the first time that the test framework needs to inspect bitcoind arguments, so that’s a new problem. @stratospher: I agree that this is a bit counter-intuitive, but it’s in line with the current behavior of restart_node (that apparently many tests rely on), i.e. “if no extra_args are passed, restart with the ones that were initially specified (self.extra_args)”, in your example -v2transport=0.

stratospher commented at 3:22 am on November 11, 2023:

“if no extra_args are passed, restart with the ones that were initially specified (self.extra_args)”

oh interesting! so #28805 (review) is out of scope for this PR.

mzumsande commented at 4:10 pm on November 20, 2023:

One small drawback of the current way of inspecting extra_args for the v2transport option is that it doesn’t work if the argument is specified in a slightly different format with same meaning.

Good point - I agree that it makes sense to leave this for a possible follow-up. I’m not sur if we want to port all of the argsmanager logic into python, but having something a bit more general so that it could be reused by other args than just -v2transport in the future would be nice.

stratospher commented at 5:18 am on November 10, 2023: contributor

These tests have in common that they use self.restart_node() with changing some unrelated extra_args, which would then cause –v2transport to be dropped.

thanks for the detailed explanation! so the problem was test failures happening when unrelated extra_args were passed during TestNode restart!

i was also initially confused about whether we really need an extra v2transport argument in TestNode::__init()__ but #28805 (comment) makes it clear why we need to extract that information from args for persisting global v2transport option during restarts. this information would get lost if it’s passed on to extra_args like it’s done in master.

theStack approved

theStack commented at 2:56 am on November 11, 2023: contributor

ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe

DrahtBot requested review from stratospher on Nov 11, 2023

stratospher commented at 3:23 am on November 11, 2023: contributor

ACK 35fb993.

sipa commented at 3:45 am on November 11, 2023: member

utACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe. Thanks for taking care of this.

achow101 commented at 7:14 pm on November 28, 2023: member

ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe

achow101 merged this on Nov 28, 2023

achow101 closed this on Nov 28, 2023

mzumsande deleted the branch on Nov 28, 2023

test: Make existing functional tests compatible with –v2transport #28805

Code Coverage

Reviews

Conflicts