Fixes #28585.
Fixes a few races within the assumeutxo tests:
-stopatheight can’t be used with connect_nodes safely because the latter performs blocking assertions that are racy with the stopatheight triggering.normal after background validation, accept the final height from either chainstate.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | MarcoFalke, ryanofsky, fjahr, achow101 |
| Concept ACK | hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Code review ACK 5bd2010f024b5bcccf1d57bae6fc36c53f5facc5
I had just written the same fix essentially. The stopatheight node shuts down before the list of wait_until checks in connect_nodes succeeds. I guess in the python execution is slower in CI but the node and sync is still comparatively fast.
129@@ -130,8 +130,8 @@ def no_sync():
130
131 monitor = n1.getchainstates()
132 assert_equal(monitor['normal']['blocks'], START_HEIGHT)
133- assert_equal(monitor['snapshot']['blocks'], SNAPSHOT_BASE_HEIGHT)
134- assert_equal(monitor['snapshot']['snapshot_blockhash'], dump_output['base_hash'])
135+ assert_equal(monitor.get('snapshot', {}).get('blocks'), SNAPSHOT_BASE_HEIGHT)
136+ assert_equal(monitor.get('snapshot', {}).get('snapshot_blockhash'), dump_output['base_hash'])
Even with latest HEAD, still seeing an apparent race. Still investigating.
02023-10-04T15:24:47.476000Z TestFramework (INFO): Restarted node before snapshot validation completed, reloading...
12023-10-04T15:24:47.782000Z TestFramework (INFO): Ensuring snapshot chain syncs to tip. (399)
22023-10-04T15:25:47.828000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
3 lambda: n1.getchainstates().get('snapshot', {}).get('blocks') == FINAL_HEIGHT)
4'''
52023-10-04T15:25:47.828000Z TestFramework (ERROR): Assertion failed
6Traceback (most recent call last):
7 File "/home/james/src/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
8 self.run_test()
9 File "/home/james/src/bitcoin/./test/functional/feature_assumeutxo.py", line 162, in run_test
10 wait_until_helper(
11 File "/home/james/src/bitcoin/test/functional/test_framework/util.py", line 276, in wait_until_helper
12 raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
13AssertionError: Predicate ''''
14 lambda: n1.getchainstates().get('snapshot', {}).get('blocks') == FINAL_HEIGHT)
15''' not true after 60.0 seconds
162023-10-04T15:25:47.879000Z TestFramework (INFO): Stopping nodes
Alright, I’m guessing we’re fixed for real this time; https://github.com/bitcoin/bitcoin/pull/28589/commits/7e4003226030a04a19c718a4b1b83b4ca40ca33f accounts for the late change that we made on @ryanofsky’s recommendation to list what was the snapshot chainstate as the normal chainstate once background validation completes. There was a race in the test during the first time we check for the final height, which may be done after background validation completed.
I’m going to keep running the test in a loop on my end and will post back in a bit to confirm that I think we’re good to go.
Code review ACK 7e4003226030a04a19c718a4b1b83b4ca40ca33f
(#28590 would allow simplifying the check in 7e4003226030a04a19c718a4b1b83b4ca40ca33f as well)
ACK 7e4003226030a04a19c718a4b1b83b4ca40ca33f
Gonna let this run in a loop for a bit before merging.