test: disconnect_nodes should warn if nodes were already disconnected #18890

robot-visions commented at 5:59 PM on May 5, 2020: contributor

There's no harm in calling disconnect_nodes for nodes that weren't connected (in this case it's a no-op). However, detecting this case and logging a warning can help ensure that tests are behaving as expected.

In addition, since disconnect_nodes works bidirectionally, I removed all instances of this pattern:

disconnect_nodes(self.nodes[0], 1)
disconnect_nodes(self.nodes[1], 0)

DrahtBot added the label Tests on May 5, 2020

test: warn if nodes not connected before disconnect_nodes a9bd1f9adf

test: remove redundant two-way disconnect_nodes calls e6e7abd51a

test: Remove unnecessary disconnect_nodes call in rpc_psbt.py 34e641a564

robot-visions force-pushed on May 7, 2020

in test/functional/test_framework/util.py:389 in a9bd1f9adf outdated

 385 | +    def get_peer_ids():
 386 | +        result = []
 387 | +        for peer in from_connection.getpeerinfo():
 388 | +            if "testnode{}".format(node_num) in peer['subver']:
 389 | +                result.append(peer['id'])
 390 | +        return result

amitiuttarwar commented at 7:55 PM on May 26, 2020:

is my understanding correct that there should only be one matching result, but technically multiple are possible?

MarcoFalke commented at 2:37 PM on May 27, 2020:

The tests used to spin up two connections between all nodes. (One incoming and one outgoing) I hope they don't do that anymore, but it seems ok to still support the use case here and not change behaviour.

amitiuttarwar commented at 9:35 PM on May 26, 2020: contributor

ACK 34e641a564. Thanks for this test improvement!

I read through the code, double checked that the warning would actually get thrown & carefully thought through potential sync issues with removing the two way disconnections from the tests.

some context for reviewers: this PR would have helped prevent a bug in the tests I introduced here

nice cleanup to disconnect_nodes! I didn't know about nested functions. Your cleanup makes the function much more understandable.
RE: removing redundant disconnect_nodes call / possible sync issues. I was trying to understand if there could be some difference in timing if we call disconnect_nodes on both sides of the connection from the tests VS letting one side instigate. My conclusion is that there is room for some delay, but I'd anticipate a short one (esp when running tests and the latency between nodes is negligible).
I also don't think it could cause issues in the tests where it's been removed. This is further supported by the general test pattern of calling disconnect nodes once. If we found possible issues, we'd want to encapsulate a behavior update in the disconnect_nodes function.

to share my current understanding / how I arrived to the trivial time delay conclusion -

the util.py#disconnect_nodes helper function calls the disconnectnodes RPC endpoint, which passes the info through to one of the CConnman::DisconnectNode functions (depending on arg type). This marks the peer for disconnect & then returns.
ThreadSocketHandler picks up the peers marked for fDisconnect and initiates the actual "close connection" process, involving the TCP messages sent back and forth to get the connection into the final CLOSED state.
both steps have room for delays, but I'd expect the delay to be trivial for both steps in the tests. the socket handler thread should service the peers for disconnect quickly, and the closing process should also be very quick with both nodes running on the same computer.

lots of words to say, I like this test improvement !

DrahtBot commented at 2:59 AM on June 8, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19198 (test: Check that peers with forcerelay permission are not asked to feefilter by MarcoFalke)

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.

MarcoFalke commented at 12:56 PM on June 8, 2020: member

review ACK 34e641a564531853342b03db2d9f0bf52b6e439e 👔

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash ff596bcd039e1137cd917886d4c1c72c69e5e6e01d537a557aa029722aa928df -

</details>

MarcoFalke merged this on Jun 8, 2020

MarcoFalke closed this on Jun 8, 2020

jonatack commented at 1:15 PM on June 8, 2020: member

ACK 34e641a564531853342b03db2d9f0bf52b6e439e

Nice idea. Code review and verified the warnings on master.

sidhujag referenced this in commit e48587fa64 on Jun 8, 2020

Fabcien referenced this in commit 49ccd05ff3 on May 14, 2021

DrahtBot locked this on Feb 15, 2022