test: Fix intermittent issue in p2p_sendtxrcncl.py #26381

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2210-test-race-🥒 changing 1 files +23 −19

maflcko commented at 2:08 PM on October 24, 2022: member

Should fix #26364

I can't reproduce this, but my guess would be that PeerNoVerack::on_version, which sends the wtxidrelay message, is executed in the event loop and thus may run after the main thread sending msg_verack.

Also, fix another bug.

Finally, add some assert_debug_log to ensure the right code branch is executed (and not some random, unrelated disconnect).
test: Fix intermittent issue in p2p_sendtxrcncl.py fa590cfaae
fanquake added the label Tests on Oct 24, 2022
DrahtBot commented at 10:23 PM on October 24, 2022: contributor



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



Conflicts

No conflicts as of last run.
in test/functional/p2p_sendtxrcncl.py:177 in fafd0204b1 outdated
```
 176 |          sendtxrcncl_wrong_role = create_sendtxrcncl_msg(initiator=True)
 177 |          peer = self.nodes[0].add_outbound_p2p_connection(
 178 |              P2PInterface(), wait_for_verack=False, p2p_idx=4, connection_type="outbound-full-relay")
 179 | -        peer.send_message(sendtxrcncl_wrong_role)
 180 | -        peer.wait_for_disconnect()
 181 | +        with self.nodes[0].assert_debug_log(["sendtxrcncl received after verack"]):
```
naumenkogs commented at 8:52 AM on October 25, 2022:

I don't think this is the log message we should expect here. I think should be txreconciliation protocol violation Does it pass because this log message happened earlier?

maflcko commented at 11:17 AM on October 25, 2022:

P2PInterface sends a verack on the version message, so this will disconnect for the verack, not for another reason. Looks like another bug in the test?
bitcoin deleted a comment on Oct 25, 2022

test: Check correct disconnect reason in p2p_sendtxrcncl.py

Previously it disconnected due to "sendtxrcncl received after verack",
now it disconnects for the correct reason.

fae0439486

test: Check debug log as well in p2p_sendtxrcncl.py fa3da8307b
in test/functional/p2p_sendtxrcncl.py:135 in fa590cfaae outdated
```
 136 | -        # sent before sendtxrcncl is sent.
 137 | -        peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
 138 | -        peer.send_and_ping(msg_verack())
 139 | -        peer.send_message(create_sendtxrcncl_msg())
 140 | -        peer.wait_for_disconnect()
 141 | +        peer = self.nodes[0].add_p2p_connection(P2PInterface())
```
naumenkogs commented at 9:11 AM on October 25, 2022:

nit: worth explicitly stating wait_for_verack=True?

maflcko commented at 11:19 AM on October 25, 2022:

I don't really like the wait_for_verack use in this test. I think all tests here should wait for the verack. However this is currently not possible because waiting for the verack will also send a ping-pong, which obviously doesn't work. Maybe there could be another optional arg to add_p2p_connection to say skip_verack_ping_pong?
maflcko force-pushed on Oct 25, 2022
maflcko commented at 11:28 AM on October 25, 2022: member

Fixed another bug in the last push
naumenkogs commented at 12:42 PM on October 25, 2022: member

ACK fa3da8307baa96ea7e51cc6ba4820aca7f93ec32
maflcko merged this on Oct 26, 2022
maflcko closed this on Oct 26, 2022
maflcko deleted the branch on Oct 26, 2022
sidhujag referenced this in commit bb7e2078c7 on Oct 27, 2022
bitcoin locked this on Oct 26, 2023

Contributors

Labels

Linked (view graph)

#26364 Intermitted failure in p2p_sendtxrcncl.py

test: Fix intermittent issue in p2p_sendtxrcncl.py #26381

Conflicts