test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure #30174

maflcko commented at 8:09 AM on May 25, 2024: member

Otherwise, the test may fail on slow hardware when running in valgrind.

Also, use named args for the absolute timepoint, while touching this file.

in test/functional/p2p_disconnect_ban.py:107 in fa4f0decb9 outdated

 101 | @@ -102,7 +102,9 @@ def run_test(self):
 102 |                  assert_equal(ban["ban_duration"], 120)
 103 |                  assert_equal(ban["time_remaining"], 117)
 104 |  
 105 | -        self.restart_node(1)
 106 | +        # Keep mocktime, to avoid ban expiry when restart takes longer than
 107 | +        # time_remaining
 108 | +        self.restart_node(1, extra_args=[f"-mocktime={old_time+4}"])

tdb3 commented at 1:18 AM on May 27, 2024:

At first glance, this seems like it may allow for node 0 and node 1 to diverge in time (node 1's time is forced to a point potentially in the past, while node 0 isn't). Had an initial thought about the ramifications of allowing this to happen. Since this test is exercising banning rather than block generation, I'm not sure this would matter.

AngusP commented at 7:23 PM on June 4, 2024:

There's the "maximum 2-hour block timestamp difference" acceptance rule, but I don't think Valgrind is that slow 😆

maflcko commented at 7:50 PM on June 4, 2024:

Yeah, I think it shouldn't matter here. An alternative would be to increase the magic number 120 sufficiently (maybe by a factor of 10 or so).

tdb3 commented at 1:26 AM on May 27, 2024: contributor

Concept ACK

Thank you. It's nice to make tests more robust even in slower environments/configurations. Did you run into this specific failure scenario, or simply being proactive?

nit: If another push happens, these look like typos:

https://github.com/bitcoin/bitcoin/blob/fa4f0decb9106b5046fda76baf2f7ed44a5d3c62/test/functional/p2p_disconnect_ban.py#L21

diff --git a/test/functional/p2p_disconnect_ban.py b/test/functional/p2p_disconnect_ban.py
index e50fc78056d..e47f9c732bf 100755
--- a/test/functional/p2p_disconnect_ban.py
+++ b/test/functional/p2p_disconnect_ban.py
@@ -18,7 +18,7 @@ class DisconnectBanTest(BitcoinTestFramework):
         self.supports_cli = False
 
     def run_test(self):
-        self.log.info("Connect nodes both way")
+        self.log.info("Connect nodes both ways")
         # By default, the test framework sets up an addnode connection from
         # node 1 --> node0. By connecting node0 --> node 1, we're left with
         # the two nodes being connected both ways.
@@ -115,7 +115,7 @@ class DisconnectBanTest(BitcoinTestFramework):
 
         # Clear ban lists
         self.nodes[1].clearbanned()
-        self.log.info("Connect nodes both way")
+        self.log.info("Connect nodes both ways")
         self.connect_nodes(0, 1)
         self.connect_nodes(1, 0)

Or maybe update to something like connect nodes bidirectionally.

DrahtBot commented at 1:26 AM on May 27, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, AngusP

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

AngusP approved

AngusP commented at 7:28 PM on June 4, 2024: contributor

Untested ACK fa4f0decb9106b5046fda76baf2f7ed44a5d3c62

DrahtBot requested review from tdb3 on Jun 4, 2024

test: Fix typos and use names args fa6aa4027c

test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure 4444de152f

maflcko force-pushed on Jun 4, 2024

maflcko commented at 8:30 PM on June 4, 2024: member

nit: If another push happens, these look like typos:

Thanks, fixed.

Did you run into this specific failure scenario, or simply being proactive?

It happened once with valgrind, which sometimes takes some time to load. You can test with:

diff --git a/test/functional/p2p_disconnect_ban.py b/test/functional/p2p_disconnect_ban.py
index 72a42a658e..aa4ae7ea27 100755
--- a/test/functional/p2p_disconnect_ban.py
+++ b/test/functional/p2p_disconnect_ban.py
@@ -103,6 +103,7 @@ class DisconnectBanTest(BitcoinTestFramework):
                 assert_equal(ban["time_remaining"], 117)
 
         self.restart_node(1)
+        time.sleep(121);
 
         listAfterShutdown = self.nodes[1].listbanned()
         assert_equal("127.0.0.0/24", listAfterShutdown[0]['address'])

tdb3 approved

tdb3 commented at 10:35 PM on June 4, 2024: contributor

ACK for 4444de152f01368e603f2b089679a86eae02e34a

DrahtBot requested review from AngusP on Jun 4, 2024

AngusP approved

AngusP commented at 9:06 AM on June 5, 2024: contributor

re-ACK 4444de152f01368e603f2b089679a86eae02e34a

fanquake merged this on Jun 5, 2024

fanquake closed this on Jun 5, 2024

maflcko deleted the branch on Jun 5, 2024

bitcoin locked this on Jun 5, 2025