Skip to content

test: fix intermittent failure in p2p_sendtxrcncl.py #26448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 4, 2022
Merged

test: fix intermittent failure in p2p_sendtxrcncl.py #26448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions test/functional/p2p_sendtxrcncl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ def run_test(self):
assert not peer.sendtxrcncl_msg_received.initiator
assert peer.sendtxrcncl_msg_received.responder
assert_equal(peer.sendtxrcncl_msg_received.version, 1)
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL should be sent before VERACK')
peer = self.nodes[0].add_p2p_connection(PeerTrackMsgOrder(), send_version=True, wait_for_verack=True)
peer.wait_for_verack()
verack_index = [i for i, msg in enumerate(peer.messages) if msg.msgtype == b'verack'][0]
sendtxrcncl_index = [i for i, msg in enumerate(peer.messages) if msg.msgtype == b'sendtxrcncl'][0]
assert(sendtxrcncl_index < verack_index)
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL on pre-WTXID version should not be sent')
peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=False, wait_for_verack=False)
Expand All @@ -94,7 +94,7 @@ def run_test(self):
peer.send_message(pre_wtxid_version_msg)
peer.wait_for_verack()
assert not peer.sendtxrcncl_msg_received
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL for fRelay=false should not be sent')
peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=False, wait_for_verack=False)
Expand All @@ -106,7 +106,7 @@ def run_test(self):
peer.send_message(no_txrelay_version_msg)
peer.wait_for_verack()
assert not peer.sendtxrcncl_msg_received
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('valid SENDTXRCNCL received')
peer = self.nodes[0].add_p2p_connection(PeerNoVerack(), send_version=True, wait_for_verack=False)
Expand Down Expand Up @@ -152,39 +152,39 @@ def run_test(self):
with self.nodes[0].assert_debug_log(['Forget txreconciliation state of peer']):
peer.send_message(create_sendtxrcncl_msg())
peer.send_message(msg_verack())
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL sent to an outbound')
peer = self.nodes[0].add_outbound_p2p_connection(
SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=1, connection_type="outbound-full-relay")
SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="outbound-full-relay")
assert peer.sendtxrcncl_msg_received
assert peer.sendtxrcncl_msg_received.initiator
assert not peer.sendtxrcncl_msg_received.responder
assert_equal(peer.sendtxrcncl_msg_received.version, 1)
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL should not be sent if block-relay-only')
peer = self.nodes[0].add_outbound_p2p_connection(
SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=2, connection_type="block-relay-only")
SendTxrcnclReceiver(), wait_for_verack=True, p2p_idx=0, connection_type="block-relay-only")
assert not peer.sendtxrcncl_msg_received
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info("SENDTXRCNCL should not be sent if feeler")
peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=2, connection_type="feeler")
peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=0, connection_type="feeler")
assert not peer.sendtxrcncl_msg_received
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL if block-relay-only triggers a disconnect')
peer = self.nodes[0].add_outbound_p2p_connection(
PeerNoVerack(), wait_for_verack=False, p2p_idx=3, connection_type="block-relay-only")
PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="block-relay-only")
with self.nodes[0].assert_debug_log(["we indicated no tx relay; disconnecting"]):
peer.send_message(create_sendtxrcncl_msg(initiator=False))
peer.wait_for_disconnect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is racy, so you'd have to call self.nodes[0].disconnect_p2ps() or use a unique index

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why? We called disconnect_p2ps() before add_outbound_p2p_connection, so are guaranteed to have no peer when doing so. Within thewith block we call wait_for_disconnect (which waits until the disconnect initiated by the node is completed), so the subsequent subtest also shouldn't be able to run before the disconnect of the current one is completed. What am I missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. As the disconnect happens on the side of the node, there shouldn't be a race where the node is not aware of the disconnect.


self.log.info('SENDTXRCNCL with initiator=1 and responder=0 from outbound triggers a disconnect')
sendtxrcncl_wrong_role = create_sendtxrcncl_msg(initiator=True)
peer = self.nodes[0].add_outbound_p2p_connection(
PeerNoVerack(), wait_for_verack=False, p2p_idx=4, connection_type="outbound-full-relay")
PeerNoVerack(), wait_for_verack=False, p2p_idx=0, connection_type="outbound-full-relay")
with self.nodes[0].assert_debug_log(["txreconciliation protocol violation"]):
peer.send_message(sendtxrcncl_wrong_role)
peer.wait_for_disconnect()
Expand All @@ -193,13 +193,13 @@ def run_test(self):
self.restart_node(0, [])
peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True)
assert not peer.sendtxrcncl_msg_received
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()

self.log.info('SENDTXRCNCL not sent if blocksonly is set')
self.restart_node(0, ["-txreconciliation", "-blocksonly"])
peer = self.nodes[0].add_p2p_connection(SendTxrcnclReceiver(), send_version=True, wait_for_verack=True)
assert not peer.sendtxrcncl_msg_received
peer.peer_disconnect()
self.nodes[0].disconnect_p2ps()


if __name__ == '__main__':
Expand Down
4 changes: 4 additions & 0 deletions test/functional/test_framework/test_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,10 @@ def add_outbound_p2p_connection(self, p2p_conn, *, wait_for_verack=True, p2p_idx
This method adds the p2p connection to the self.p2ps list and returns
the connection to the caller.
p2p_idx must be different for simultaneously connected peers. When reusing it for the next peer
after disconnecting the previous one, it is necessary to wait for the disconnect to finish to avoid
a race condition.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention that disconnect_p2ps avoids the race?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to describe it in more general terms because disconnect_p2ps can only be used if there is no other, unrelated connection that we need to keep.

"""

def addconnection_callback(address, port):
Expand Down