test: improve robustness of connect_nodes() #30118

furszy · 2024-05-15T20:58:22Z

Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.

The connect_nodes function in the test framework relies on a stable number of peer
connections to verify that the new connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent wait_until checks to stall indefinitely
even when the peers in question were connected successfully.

This commit improves the situation by using the nodes' subversion and the connection
direction (inbound/outbound) to identify the exact peer connection and perform the
checks exclusively on it.

DrahtBot · 2024-05-15T20:58:25Z

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	maflcko, AngusP, stratospher, achow101
Stale ACK	rkrux

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:

#27837 (net: introduce block tracker to retry to download blocks after failure by furszy)

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.

rkrux

tACK 59ba873

Make successful, so are all the functional tests.

Overall I agree with this change because it makes the nodes connection verification dependent more on connect_nodes() arguments a, b instead of the earlier approach that relied more on the state of the whole response of getpeerinfo(), which seemed brittle as mentioned in the PR description.

I can see connect_nodes() being used in numerous functional tests, thereby increasing robustness for all of them!

@furszy Were you able to identify few connections that were dropped in logs of this CI run? https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200

test/functional/test_framework/test_framework.py

furszy

@furszy Were you able to identify few connections that were dropped in logs of this CI run? https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200

Just one. One of the P2PInterface connections I create on the test side gets disconnected after advancing the node time prior to connecting the test nodes again. Need to expand the complete CI logs to find it.
But the fragility is easy to reproduce. Just launch a thread that disconnects a node before calling connect_nodes().

test/functional/test_framework/test_framework.py

AngusP

Approach ACK, some nits/questions

Tested, functional tests all pass

test/functional/test_framework/test_framework.py

AngusP

tACK f4c588c

Tested, functional tests all pass

stratospher

tested ACK f4c588c.

very cool! i like how the logic depends only on the node we're connecting to.
reproduced the intermittent failure using this patch and verified that the new logic fixes it!

    self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False)
    self.connect_nodes(0, 2)

also didn't observe that much of a time increase (only around 70 s) when running all the functional tests in my local config (without wallets).

test/functional/test_framework/test_framework.py

maflcko · 2024-05-21T06:30:31Z

This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent wait_until checks to stall indefinitely
even when the peers in question were connected successfully.

I'd say the tests should avoid racy code and deterministically execute the test code line-by-line, but enforcing this in connect_nodes is the wrong approach. So Concept ACK.

The 'connect_nodes' function in the test framework relies on a stable number of peer connections to verify the new connection between the nodes is successfully established. This approach is fragile, as any of the peers involved in the process can drop, lose, or create a connection at any step, causing subsequent 'wait_until' checks to stall indefinitely even when the peers in question are connected successfully. This commit improves the situation by using the nodes' subversion and the connection direction (inbound/outbound) to identify the exact peer connection and perform the checks exclusively on it.

maflcko

utACK 6629d1d

maflcko · 2024-05-21T15:13:59Z

test/functional/test_framework/test_node.py

@@ -106,7 +106,7 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor,
            "-debugexclude=libevent",
            "-debugexclude=leveldb",
            "-debugexclude=rand",
-            "-uacomment=testnode%d" % i,
+            "-uacomment=testnode%d" % i,  # required for subversion uniqueness across peers


Suggested change

"-uacomment=testnode%d" % i, # required for subversion uniqueness across peers

f"-uacomment=testnode{i}", # required for subversion uniqueness across peers

style-nit, if you retouch

maflcko · 2024-05-21T15:20:50Z

test/functional/test_framework/test_framework.py

+            return peer['bytesrecv_per_msg'].pop(msg_type, 0) >= min_bytes_recv
+
+        self.wait_until(lambda: check_bytesrecv(find_conn(from_connection, to_connection_subver, inbound=False), 'verack', 21))
+        self.wait_until(lambda: check_bytesrecv(find_conn(to_connection, from_connection_subver, inbound=True), 'verack', 21))


nit: As you remove the version check, I wonder if this one can be removed as well for the same reason? The final pong test should be necessary and sufficient already.

nit: As you remove the version check, I wonder if this one can be removed as well for the same reason? The final pong test should be necessary and sufficient already.

I removed the version check because veracks are direct responses to version messages at the protocol level. I'm not sure we should rely on the pong alone because that might change over time?

I'm not sure we should rely on the pong alone because that might change over time?

If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.

See the next line comment:

# The message bytes are counted before processing the message, so make # sure it was fully processed by waiting for a ping.

I'm not sure we should rely on the pong alone because that might change over time?

If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.

I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed in this phase. Any other received message will be ignored and it is a reason for banning the other party.

See the next line comment:

# The message bytes are counted before processing the message, so make # sure it was fully processed by waiting for a ping.

What about including the flag indicating that the connection is ready (fSuccessfullyConnected) in the getpeerinfo response? It seems generally useful and would clean up all this code.

I'm not sure we should rely on the pong alone because that might change over time?

If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.

I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed in this phase. Any other received message will be ignored and it is a reason for banning the other party.

Yes, that is what I am trying to say. The pong alone should be necessary and sufficient.

Yes, that is what I am trying to say. The pong alone should be necessary and sufficient.

Ah ok. "goto the first message".. now we are sync. Nice bikeshedding from my end.

So yeah, we could wait only for the pong or introduce a new field on the getpeerinfo output (fSuccessfullyConnected with another name like "handshake_completed").

I think you'll have to tell maintainers if you want to address this nit, or leave it for a follow-up, otherwise they won't know whether it is fine to merge this from your side or not.

I think you'll have to tell maintainers if you want to address this nit, or leave it for a follow-up, otherwise they won't know whether it is fine to merge this from your side or not.

ok, yes. I was thinking on the second approach, but let's move forward. This will let me un-draft #27837. Happy to review any follow-up doing this.
Thanks Marko for the ping and this discussion.

AngusP

re-ACK 6629d1d

AngusP · 2024-05-21T17:16:37Z

test/functional/test_framework/test_framework.py

+            assert peer is not None, "Error: peer disconnected"
+            return peer['bytesrecv_per_msg'].pop(msg_type, 0) >= min_bytes_recv


Style nit: Mixture of ' and " strings here and in a few places, though it was already mixed before -- " seems to be the style in this file

stratospher · 2024-05-22T08:01:31Z

reACK 6629d1d.

achow101 · 2024-05-22T20:02:42Z

ACK 6629d1d

DrahtBot added the Tests label May 15, 2024

rkrux approved these changes May 16, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Show resolved Hide resolved

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

furszy commented May 16, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Show resolved Hide resolved

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

maflcko reviewed May 17, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

furszy force-pushed the 2024_test_improve_connect_nodes branch from 59ba873 to e9cb116 Compare May 18, 2024 14:02

furszy mentioned this pull request May 18, 2024

Prune Node Rescan Project Tracking #29183

Open

18 tasks

AngusP reviewed May 18, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Show resolved Hide resolved

test/functional/test_framework/test_framework.py Show resolved Hide resolved

furszy force-pushed the 2024_test_improve_connect_nodes branch from e9cb116 to f4c588c Compare May 18, 2024 22:07

AngusP approved these changes May 19, 2024

View reviewed changes

DrahtBot requested a review from rkrux May 19, 2024 10:28

stratospher reviewed May 19, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Show resolved Hide resolved

maflcko reviewed May 21, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Show resolved Hide resolved

maflcko reviewed May 21, 2024

View reviewed changes

test/functional/test_framework/test_framework.py Show resolved Hide resolved

furszy force-pushed the 2024_test_improve_connect_nodes branch from f4c588c to 6629d1d Compare May 21, 2024 13:59

maflcko approved these changes May 21, 2024

View reviewed changes

DrahtBot requested review from AngusP and stratospher May 21, 2024 15:21

AngusP approved these changes May 21, 2024

View reviewed changes

achow101 merged commit e163d86 into bitcoin:master May 23, 2024
16 checks passed

furszy deleted the 2024_test_improve_connect_nodes branch May 23, 2024 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve robustness of connect_nodes() #30118

test: improve robustness of connect_nodes() #30118

furszy commented May 15, 2024 •

edited

DrahtBot commented May 15, 2024 •

edited

rkrux left a comment •

edited

furszy left a comment

AngusP left a comment •

edited

AngusP left a comment •

edited

stratospher left a comment

maflcko commented May 21, 2024

maflcko left a comment

maflcko May 21, 2024

maflcko May 21, 2024

furszy May 22, 2024

maflcko May 22, 2024

furszy May 22, 2024

maflcko May 22, 2024

furszy May 22, 2024

maflcko May 23, 2024

furszy May 23, 2024

AngusP left a comment

AngusP May 21, 2024

stratospher commented May 22, 2024

achow101 commented May 22, 2024

	"-uacomment=testnode%d" % i, # required for subversion uniqueness across peers
	f"-uacomment=testnode{i}", # required for subversion uniqueness across peers

		assert peer is not None, "Error: peer disconnected"
		return peer['bytesrecv_per_msg'].pop(msg_type, 0) >= min_bytes_recv

test: improve robustness of connect_nodes() #30118

test: improve robustness of connect_nodes() #30118

Conversation

furszy commented May 15, 2024 • edited

DrahtBot commented May 15, 2024 • edited

Code Coverage

Reviews

Conflicts

rkrux left a comment • edited

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

AngusP left a comment • edited

Choose a reason for hiding this comment

AngusP left a comment • edited

Choose a reason for hiding this comment

stratospher left a comment

Choose a reason for hiding this comment

maflcko commented May 21, 2024

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngusP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stratospher commented May 22, 2024

achow101 commented May 22, 2024

furszy commented May 15, 2024 •

edited

DrahtBot commented May 15, 2024 •

edited

rkrux left a comment •

edited

AngusP left a comment •

edited

AngusP left a comment •

edited