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

mzumsande · 2023-11-06T22:06:03Z

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 (comment)

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 · 2023-11-06T22:06:06Z

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.

mzumsande · 2023-11-06T22:07:05Z

FYI @stratospher @theStack

theStack · 2023-11-07T12:07:31Z

Concept ACK

maflcko · 2023-11-07T12:17:39Z

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 · 2023-11-07T14:05:50Z

tACK 6b77f9

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

python3 wallet_multiwallet.py --usecli --v2transport

mzumsande · 2023-11-07T15:12:38Z

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 · 2023-11-07T17:33:18Z

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

mzumsande · 2023-11-07T17:51:45Z

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 · 2023-11-08T15:38:26Z

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 · 2023-11-08T20:07:47Z

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

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.

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

- "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

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.

mzumsande · 2023-11-08T22:45:45Z

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.

stratospher

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.

stratospher · 2023-11-10T05:07:36Z

test/functional/test_framework/test_node.py

@@ -198,6 +204,8 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None
        if extra_args is None:
            extra_args = self.extra_args

+        self.use_v2transport = "-v2transport=1" in extra_args or (self.default_to_v2 and "-v2transport=0" not in extra_args)


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.

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.

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

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

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.

theStack

ACK 35fb993

stratospher

ACK 35fb993.

sipa · 2023-11-11T03:45:44Z

utACK 35fb993. Thanks for taking care of this.

achow101 · 2023-11-28T19:14:26Z

ACK 35fb993

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. Github-Pull: bitcoin#28805 Rebased-From: 68a9001

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). Github-Pull: bitcoin#28805 Rebased-From: 3598a1b

Github-Pull: bitcoin#28805 Rebased-From: cc961c2

- "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 Github-Pull: bitcoin#28805 Rebased-From: 2c1669c

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. Github-Pull: bitcoin#28805 Rebased-From: 35fb993

…-v2transport

…-v2transport Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

, bitcoin#28849, bitcoin#28805, bitcoin#28951, bitcoin#29058, bitcoin#29239, partial bitcoin#28331, bitcoin#28452 (BIP324 backports: part 2) 5dd60c4 merge bitcoin#29239: Make v2transport default for addnode RPC when enabled (Kittywhiskers Van Gogh) b2ac426 merge bitcoin#29058: use v2transport for manual/addrfetch connections, add to -netinfo (Kittywhiskers Van Gogh) 92e862a merge bitcoin#28951: damage ciphertext/aad in full byte range (Kittywhiskers Van Gogh) 4e96e26 merge bitcoin#28805: Make existing functional tests compatible with --v2transport (Kittywhiskers Van Gogh) 9371e2e merge bitcoin#28849: fix node index bug when comparing peerinfo (Kittywhiskers Van Gogh) 400c9dd merge bitcoin#28634: add check for missing garbage terminator detection (Kittywhiskers Van Gogh) 65eb194 merge bitcoin#28588: add checks for v1 prefix matching / wrong network magic detection (Kittywhiskers Van Gogh) 6074974 merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16 (Kittywhiskers Van Gogh) ff92d1a partial bitcoin#28331: BIP324 integration (Kittywhiskers Van Gogh) f9f8805 fix: drop `CConnman::mapNodesWithDataToSend`, use transport data (UdjinM6) d39d8a4 partial bitcoin#28452: Do not use std::vector = {} to release memory (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6280 * Dependent on #6276 * Dependency for #6329 * [bitcoin#28452](bitcoin#28452) was backported as the behavior it introduces is required for backports like [bitcoin#28331](bitcoin#28331). As it pre-dates earlier BIP324 backports, the `ClearShrink` usage from those backports have also been incorporated into this backport. * When backporting [bitcoin#28331](bitcoin#28331), in TestFramework's `add_nodes()`, `extra_args[i]` is not converted to a `list` like it is done upstream and avoiding duplication of `-v2transport=1` is instead done by an additional check. This is done to prevent test failures in `feature_mnehf.py`. * The local variable `args` is removed in [bitcoin#28805](bitcoin#28805) as it does nothing (the argument appending logic is removed as part of the backport) and upstream removes it anyways. Special thanks to UdjinM6 for significant contributions to this and dependent PRs! 🎉 ## 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: light ACK 5dd60c4 PastaPastaPasta: utACK 5dd60c4 Tree-SHA512: 7f3d0e54e1c96fc99b2d6b1e64485110aa73aeec0e4e4245ed4e6dc0529a7608e9c39103af636d5945d289775c40d3d15d7d4a75bea82462194dbf355fca48dc

DrahtBot added the Tests label Nov 6, 2023

This was referenced Nov 7, 2023

net, net_processing: additional and consistent disconnect logging #28521

Merged

p2p: make block download logic aware of limited peers threshold #28120

Merged

DrahtBot requested a review from theStack November 7, 2023 14:05

mzumsande and others added 5 commits November 8, 2023 17:30

test: enable v2 transport for p2p_node_network_limited.py

cc961c2

test: enable v2 transport for p2p_timeouts.py

35fb993

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.

mzumsande force-pushed the 202311_test_v2transport4all branch from 6b77f9f to 35fb993 Compare November 8, 2023 22:38

stratospher reviewed Nov 10, 2023

View reviewed changes

theStack approved these changes Nov 11, 2023

View reviewed changes

DrahtBot requested a review from stratospher November 11, 2023 02:56

stratospher reviewed Nov 11, 2023

View reviewed changes

sipa mentioned this pull request Nov 11, 2023

BIP324 tracking issue #27634

Closed

43 tasks

DrahtBot mentioned this pull request Nov 13, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

achow101 merged commit 30a0557 into bitcoin:master Nov 28, 2023

mzumsande mentioned this pull request Nov 28, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

mzumsande deleted the 202311_test_v2transport4all branch November 28, 2023 22:28

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2024

test: enable v2 transport for p2p_node_network_limited.py

1dc0afe

Github-Pull: bitcoin#28805 Rebased-From: cc961c2

kwvg added a commit to kwvg/dash that referenced this pull request Sep 17, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

4eb3524

…-v2transport

kwvg added a commit to kwvg/dash that referenced this pull request Sep 17, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

4bc1c4d

…-v2transport

kwvg added a commit to kwvg/dash that referenced this pull request Sep 17, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

ce0bc1f

…-v2transport

kwvg added a commit to kwvg/dash that referenced this pull request Oct 2, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

f9f198c

…-v2transport

kwvg added a commit to kwvg/dash that referenced this pull request Oct 9, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

1a1c964

…-v2transport Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg mentioned this pull request Oct 9, 2024

backport: merge bitcoin#28577, #28588, #28634, #28849, #28805, #28951, #29058, #29239, partial bitcoin#28331, #28452 (BIP324 backports: part 2) dashpay/dash#6324

Merged

5 tasks

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

merge bitcoin#28805: Make existing functional tests compatible with -…

be6714f

…-v2transport Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

01e4588

…-v2transport Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024

merge bitcoin#28805: Make existing functional tests compatible with -…

4e96e26

…-v2transport Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

bitcoin locked and limited conversation to collaborators Nov 27, 2024

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

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

Uh oh!

Conversation

mzumsande commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

mzumsande commented Nov 6, 2023

Uh oh!

theStack commented Nov 7, 2023

Uh oh!

maflcko commented Nov 7, 2023

Uh oh!

kashifs commented Nov 7, 2023

Uh oh!

mzumsande commented Nov 7, 2023

Uh oh!

maflcko commented Nov 7, 2023

Uh oh!

mzumsande commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stratospher commented Nov 8, 2023

Uh oh!

mzumsande commented Nov 8, 2023

Uh oh!

mzumsande commented Nov 8, 2023

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher Nov 10, 2023 • 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!

theStack Nov 11, 2023

Choose a reason for hiding this comment

Uh oh!

stratospher Nov 11, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 11, 2023

Uh oh!

achow101 commented Nov 28, 2023

Uh oh!

Uh oh!

mzumsande commented Nov 6, 2023 •

edited

Loading

DrahtBot commented Nov 6, 2023 •

edited

Loading

mzumsande commented Nov 7, 2023 •

edited

Loading

stratospher Nov 10, 2023 •

edited

Loading