[tests] sync_with_ping should assert that ping hasn't timed out #10114

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:improve_sync_with_ping changing 8 files +34 −66

jnewbery commented at 3:50 PM on March 29, 2017: member

sync_with_ping() currently returns false if the timeout expires, and it is the caller's responsibility to fail the test. However, none of the tests currently assert on sync_with_ping()'s return code. This commit adds an assert to sync_with_ping so the test will fail if the timeout expires.

This commit also removes all the duplicate implementations of sync_with_ping() from the individual tests.

~Only the second commit here needs to be reviewed. The first commit https://github.com/bitcoin/bitcoin/commit/159fe88abfadf67410578c145e631819cf50b660 is #10109.~ EDIT: #10109 is now merged.

EDIT: The first commit in this PR ("Add send_await_disconnect() method to p2p-compactblocks.py") is a test case bug fix that needs to be merged before merging the sync_with_ping() change

Should fix the intermittent failure reported here: #10073 (comment)

sync'ing and pinging @sdaftuar
sdaftuar commented at 5:50 PM on March 29, 2017: member

I think travis may have caught a bug in the p2p-compactblocks.py test, which is now throwing an error that the sync with ping failed?
jnewbery force-pushed on Mar 29, 2017
jnewbery commented at 6:22 PM on March 29, 2017: member

Oh good. We've caught our first fish.

I've pushed a commit that changes p2p-compactblock to use a new method send_await_disconnect(), which awaits a disconnect rather than a pong.

The commit adds the method to p2p-compactblocks.py, but a future commit could move it to mininode since it could be useful more generally. The commit reduces the p2p-compactblock runtime by 30 seconds.

I've added the new commit before the commit that changes sync_with_ping() so git_bisect doesn't break.
fanquake added the label Tests on Mar 30, 2017

Add send_await_disconnect() method to p2p-compactblocks.py

p2p-compactblocks was incorrectly using sync_with_ping() when sending in
invalid block. The node would disconnect us and never respond to the
ping, so the sync_with_ping would just time out after 30 seconds and
continue with the test.

This commit adds a send_await_disconnect() method that sends the
message, and then waits for the node to disconnect us. In this commit
I've added the method to p2p-compactblocks.py, but a future commit could
move it to mininode since it could be useful more generally.

This commit reduces the p2p-compactblock runtime by 30 seconds.

6426716a99

[tests] sync_with_ping should assert that ping hasn't timed out

sync_with_ping currently returns false if the timeout expires, and it is
the caller's responsibility to fail the test. However, none of the tests
currently assert on sync_with_ping()'s return code. This commit adds an
assert to sync_with_ping so the test will fail if the timeout expires.

This commit also removes all the duplicate implementations of
sync_with_ping() from the individual tests.

6a18bb9a36

jnewbery force-pushed on Mar 30, 2017
laanwj commented at 12:41 PM on March 30, 2017: member

utACK 6a18bb9
sdaftuar commented at 3:08 PM on March 30, 2017: member

utACK 6a18bb9 (also verified that the extended tests all pass for me locally, after this change)
laanwj merged this on Mar 30, 2017
laanwj closed this on Mar 30, 2017
laanwj referenced this in commit edc62c959a on Mar 30, 2017
PastaPastaPasta referenced this in commit 1d2c44a55d on Mar 15, 2019
PastaPastaPasta referenced this in commit 1068be77de on May 20, 2019
PastaPastaPasta referenced this in commit b0b1bbe90b on May 20, 2019
PastaPastaPasta referenced this in commit 2e91a65db9 on May 20, 2019
PastaPastaPasta referenced this in commit f37ef3c9b8 on May 20, 2019
PastaPastaPasta referenced this in commit a1bf76b3f8 on May 21, 2019
PastaPastaPasta referenced this in commit 7fd979cc0e on May 21, 2019
PastaPastaPasta referenced this in commit ebff8cce10 on May 21, 2019
barrystyle referenced this in commit 6cbaaf5d36 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10073 Actually run assumevalid.py #10109 Remove SingleNodeConnCB