[test] clarify rpc_net & p2p_disconnect_ban functional tests #19877

amitiuttarwar · 2020-09-04T20:44:55Z

small improvements to clarify logic in the functional tests

have test logic in rpc_net.py match run order of the test
remove connect_nodes calls that are redundant with the automatic test setup executed by the test framework

Noticed when I was trying to debug a test for #19725. Small changes but imo very helpful, because they initially confused me.

test/functional/rpc_net.py

robot-dreams · 2020-09-05T20:15:47Z

Concept ACK, thanks for clarifying! After trying to trace the code, I agree with you that addnode should add connections in both directions.

Would it also make sense to change the "Connect nodes both ways" on line 147 below?
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?

epson121

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.

    def run_test(self):
        self.log.info("Connect nodes both way")
        connect_nodes(self.nodes[0], 1)
        connect_nodes(self.nodes[1], 0)
        ...

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

test/functional/rpc_net.py

amitiuttarwar · 2020-09-11T20:22:41Z

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

as one data point, this function is used a lot (every time we set up a test with more than one node) so I'd imagine we'd see some sporadic failures in misc tests if this was the case. that said, we do have some spurious test failures on CI, so insufficient to rule out entirely.
I'm trying to think of what kind of problems this could cause. message processing in bitcoin core is single threaded, so if the peer isn't processing the VERACK we've sent, could it mean a deeper problem where all messages aren't being processed? or taking forever or something..
which leads me to wonder.. what could cause an issue?.. latency should be low, esp since the nodes are local. what are other elements that could cause long time lags?

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.

michaelfolkson · 2020-09-15T11:02:09Z

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?

amitiuttarwar · 2020-09-16T20:00:11Z

@michaelfolkson not sure what you mean... I think you're asking about a subset of the tests?

robot-dreams · 2020-09-16T21:51:58Z

ACK 00ea3fc

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.

DrahtBot · 2020-09-19T13:56:36Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

amitiuttarwar · 2020-09-28T23:46:27Z

rebased

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.

amitiuttarwar · 2020-10-28T02:24:09Z

rebased

laanwj · 2020-10-28T08:45:29Z

ACK 47ff509

…tional tests 47ff509 [test] Clarify setup of node topology. (Amiti Uttarwar) 0672522 [move-only, test]: Match test order with run order (Amiti Uttarwar) Pull request description: small improvements to clarify logic in the functional tests 1. have test logic in `rpc_net.py` match run order of the test 2. remove `connect_nodes` calls that are redundant with the automatic test setup executed by the test framework Noticed when I was trying to debug a test for bitcoin#19725. Small changes but imo very helpful, because they initially confused me. ACKs for top commit: laanwj: ACK 47ff509 Tree-SHA512: 2843da2c0b4f06b2600b3adb97900a62be7bb2228770abd67d86f2a65c58079af22c7c20957474a98c17da85f40a958a6f05cb8198aa0c56a58adc1c31100492

DrahtBot added the Tests label Sep 4, 2020

ipriver reviewed Sep 5, 2020

View reviewed changes

test/functional/rpc_net.py Show resolved Hide resolved

epson121 reviewed Sep 8, 2020

View reviewed changes

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

amitiuttarwar force-pushed the 2020-09-rpcnet-fixes branch from 9bd90db to 00ea3fc Compare September 11, 2020 20:18

amitiuttarwar changed the title ~~[test] Clarify rpc_net functional test~~ [test] clarify rpc_net & p2p_disconnect_ban functional tests Sep 11, 2020

fjahr mentioned this pull request Sep 13, 2020

"Good First Review" label #19941

Closed

This was referenced Sep 19, 2020

test: Replace (dis)?connect_nodes globals with TestFramework methods #19967

Merged

Remove connect_nodes global and Replace connect_nodes(self.nodes[a], b) with self.connect_nodes(a, b) #19945

Closed

net, rpc: expose connection type in getpeerinfo #19883

Closed

DrahtBot mentioned this pull request Sep 22, 2020

[RPC] Add connection type to getpeerinfo, improve logs #19725

Merged

DrahtBot added the Needs rebase label Sep 26, 2020

amitiuttarwar force-pushed the 2020-09-rpcnet-fixes branch from 00ea3fc to c069690 Compare September 28, 2020 23:43

DrahtBot removed the Needs rebase label Sep 29, 2020

DrahtBot added the Needs rebase label Oct 21, 2020

[move-only, test]: Match test order with run order

0672522

amitiuttarwar force-pushed the 2020-09-rpcnet-fixes branch from c069690 to 686283d Compare October 27, 2020 20:57

DrahtBot removed the Needs rebase label Oct 27, 2020

[test] Clarify setup of node topology.

47ff509

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.

amitiuttarwar force-pushed the 2020-09-rpcnet-fixes branch from 686283d to 47ff509 Compare October 28, 2020 02:23

laanwj merged commit db26eeb into bitcoin:master Oct 28, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[test] clarify rpc_net & p2p_disconnect_ban functional tests #19877

[test] clarify rpc_net & p2p_disconnect_ban functional tests #19877

Uh oh!

amitiuttarwar commented Sep 4, 2020 •

edited

Loading

Uh oh!

Uh oh!

robot-dreams commented Sep 5, 2020

Uh oh!

epson121 left a comment

Uh oh!

Uh oh!

amitiuttarwar commented Sep 11, 2020

Uh oh!

michaelfolkson commented Sep 15, 2020

Uh oh!

amitiuttarwar commented Sep 16, 2020

Uh oh!

robot-dreams commented Sep 16, 2020

Uh oh!

DrahtBot commented Sep 19, 2020 •

edited

Loading

Uh oh!

amitiuttarwar commented Sep 28, 2020

Uh oh!

amitiuttarwar commented Oct 28, 2020

Uh oh!

laanwj commented Oct 28, 2020

Uh oh!

Uh oh!

[test] clarify rpc_net & p2p_disconnect_ban functional tests #19877

[test] clarify rpc_net & p2p_disconnect_ban functional tests #19877

Uh oh!

Conversation

amitiuttarwar commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

robot-dreams commented Sep 5, 2020

Uh oh!

epson121 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amitiuttarwar commented Sep 11, 2020

Uh oh!

michaelfolkson commented Sep 15, 2020

Uh oh!

amitiuttarwar commented Sep 16, 2020

Uh oh!

robot-dreams commented Sep 16, 2020

Uh oh!

DrahtBot commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

amitiuttarwar commented Sep 28, 2020

Uh oh!

amitiuttarwar commented Oct 28, 2020

Uh oh!

laanwj commented Oct 28, 2020

Uh oh!

Uh oh!

amitiuttarwar commented Sep 4, 2020 •

edited

Loading

DrahtBot commented Sep 19, 2020 •

edited

Loading