small improvements to clarify logic in the functional tests
rpc_net.py match run order of the testconnect_nodes calls that are redundant with the automatic test setup executed by the test frameworkNoticed when I was trying to debug a test for #19725. Small changes but imo very helpful, because they initially confused me.
93+ assert_equal(peer_info[0][0]['minfeefilter'], Decimal("0.00000500"))
94+ assert_equal(peer_info[1][0]['minfeefilter'], Decimal("0.00001000"))
95+ # check the `servicesnames` field
96+ for info in peer_info:
97+ assert_net_servicesnames(int(info[0]["services"], 0x10), info[0]["servicesnames"])
98+
test_getpeerinfo function declaration 75 lines upwards if you haven’t modified it? It just creates unnecessary diffs for your commit.
Yeah, reasonable question!
Concept ACK, thanks for clarifying! After trying to trace the code, I agree with you that addnode should add connections in both directions.
wait_for_helper calls in the connect_nodes implementation wait until the node has received a version and a verack. However, it doesn’t wait until the peer has received a verack. Could this introduce some sort of extremely rare race condition?51@@ -52,9 +52,12 @@ def set_test_params(self):
52 def run_test(self):
53 # Get out of IBD for the minfeefilter and getpeerinfo tests.
54 self.nodes[0].generate(101)
55- # Connect nodes both ways.
56+
57+ # By default, the test framework sets up an addnode connection from
58+ # node 1 --> node0. By connecting node0 --> node 1, this leaves us with
By connecting node0 --> node 1, **we're left** with
Concept ACK
As @robot-dreams mentioned, these changes could be applied in L147.
Also, similar behaviour can be seen in tests\functional\p2p_disconnect_ban.py L21 and L80, e.g.
0 def run_test(self):
1 self.log.info("Connect nodes both way")
2 connect_nodes(self.nodes[0], 1)
3 connect_nodes(self.nodes[1], 0)
4 ...
so updating code there as well could be an option.
As a reference, original change was introduced in this PR: https://github.com/bitcoin/bitcoin/pull/16898/files
thanks for the reviews @ipriver, @robot-dreams & @epson121 !
updated to improve another “connect nodes both ways” instance in p2p_disconnect_ban.py
PR is ready for review!
thoughts around review suggestions / questions :
Would it also make sense to change the “Connect nodes both ways” on line 147 below?
good suggestion but I’ve looked into it & have found that those connections are important to leave in. Here is why: at the start of the test, the test framework creates the initial node connection behind the scenes, so we only need one additional connect_nodes call to get a two way connection. However in test_getnetworkinfo it sets the networkactive flag to false, which disconnects everything. So, we need to explicitly connect_nodes both ways in order to restore the two way connection between nodes 0 & 1.
Let me know if this doesn’t make sense / if there are further questions. You can test it out by commenting out either/both of the lines & seeing the following assertions fail. @robot-dreams
The wait_for_helper calls in the connect_nodes implementation wait until the node has received a version and a verack. However, it doesn’t wait until the peer has received a verack. Could this introduce some sort of extremely rare race condition?
hm, good observation. let’s think this through…
VERACK we’ve sent, could it mean a deeper problem where all messages aren’t being processed? or taking forever or something..I think overall, if a test wanted to do something like “send another message, check that message was received”, it would want to use a wait_until for that assertion to be reliable, which should allow enough flexibility in any delay processing the VERACK. What do you think?
@epson121
Also, similar behaviour can be seen in tests\functional\p2p_disconnect_ban.py L21 and L80, e.g.
nice catch! I’ve updated the setup on L21. but with L80, we need to leave it in for a similar reason to what I explained earlier for the rpc_net.py test.
Concept ACK.
Are there any edge cases where the P2P behavior of a one-sided connection is different to a two-sided connection? Do we want to test both or is a two-sided connection sufficient?
ACK 00ea3fc8e4d2f545c10970d83ef24648468deb0b
Thanks for clarifying! I previously had a brain error—for some reason I thought a single connect_nodes call adds connections in both directions. But that’s not the case.
I tested locally to make sure that the connect_nodes calls you removed were indeed no-ops:
In addition, the connect_nodes call you kept (on line 147) was actually doing something:
As for my verack question, thanks for thinking this through! Your suggestions seem very reasonable, and in retrospect I’d be very surprised if the timing actually causes test flakiness. Although I don’t fully understand what’s going on here, I do think further discussion is out of scope for this PR.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Since the test framework automatically sets up a connection between the nodes,
the second connect_nodes call was a no-op. Remove the redundant call & add
comments to explain the expected topology.
