Fix flaky reorg tests#972
Conversation
Use available listener ports and tolerate intermediate force-close balances so the property test exercises reorg behavior instead of setup timing. Co-Authored-By: HAL 9000
Wait for the replacement chain to confirm close and sweep outputs before checking final balances after the reorg. Co-Authored-By: HAL 9000
Stop all reorg test nodes on helper threads before leaving each property case so shutdown does not park the Tokio worker. Co-Authored-By: HAL 9000
Wait for bitcoind to reach the generated height before polling electrs for the corresponding block header. Co-Authored-By: HAL 9000
|
👋 Thanks for assigning @joostjager as a reviewer! |
Were you able to reliably repro the flakes and see that this PR fixed them? |
|
|
||
| pub(crate) fn generate_listening_addresses() -> Vec<SocketAddress> { | ||
| let port = NEXT_PORT.fetch_add(2, Ordering::Relaxed); | ||
| let listener_a = TcpListener::bind(("127.0.0.1", 0)).expect("available listener port"); |
There was a problem hiding this comment.
The PR doesn't give more info on the nature of the flakes. Was it port collisions again?
| let listener_a = TcpListener::bind(("127.0.0.1", 0)).expect("available listener port"); | ||
| let listener_b = TcpListener::bind(("127.0.0.1", 0)).expect("available listener port"); | ||
| let port_a = listener_a.local_addr().expect("listener address").port(); | ||
| let port_b = listener_b.local_addr().expect("listener address").port(); |
There was a problem hiding this comment.
The problem here is also again that the listener is dropped before the node binds, leaving a window where it can be re-allocated in another thread.
| } => { | ||
| let cur_height = node.status().current_best_block.height; | ||
| let blocks_to_go = confirmation_height - cur_height; | ||
| if force_close { |
There was a problem hiding this comment.
This commit seems to combine two unrelated fixes.
| }; | ||
| found_claimable_balance = true; | ||
|
|
||
| let cur_height = node.status().current_best_block.height; |
There was a problem hiding this comment.
Seems quite a few things are changing in this diff hunk. Is it really minimal without unnecessary restructuring?
| assert_eq!(node.next_event(), None); | ||
| }); | ||
|
|
||
| stop_nodes_concurrently(nodes).await; |
There was a problem hiding this comment.
[P2] tests/reorg_test.rs:240 only stops nodes on the success path. All assertions and waits above it can still panic or time out, and then nodes is dropped during unwinding before stop_nodes_concurrently(nodes).await runs. Since Node::drop calls stop(), failing proptest cases can still hit the exact shutdown-on-runtime path this commit is trying to avoid. That matters especially for proptest, because failures need to unwind cleanly for shrinking and diagnostics.
| .into_iter() | ||
| .map(|node| { | ||
| let (stop_sender, stop_receiver) = tokio::sync::oneshot::channel(); | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
Add comment why we need to do it concurrently? And how it would otherwise block the tokio worker?
| loop { | ||
| if header.height >= min_height { | ||
| break; | ||
| if electrs.block_header(min_height).is_ok() { |
There was a problem hiding this comment.
[P2] tests/common/mod.rs:709-714 makes wait_for_block return on a stale reorged-out header. The two remaining callers use it after invalidate_blocks and manual generateblock calls, for example tests/integration_tests_rust.rs:746-756 and tests/integration_tests_rust.rs:1995-2005. At original_height + 1, electrs may already have the old block header from before the invalidation, so block_header(min_height).is_ok() can return immediately before electrs has switched to the replacement chain. Then the following wallet sync can still observe the pre-reorg chain, preserving the flake this helper is supposed to avoid. These callers need to wait for the replacement block hash, not just any header at that height.
| assert!(tries < 120, "electrs did not serve block header {} within 60 seconds", min_height); | ||
| tries += 1; | ||
| tokio::time::sleep(delay).await; | ||
| if delay.as_millis() < 512 { |
There was a problem hiding this comment.
why not the exponential backoff anymore?
| electrs.block_headers_subscribe().expect("failed to subscribe to block headers") | ||
| }, | ||
| }; | ||
| let mut delay = Duration::from_millis(64); |
There was a problem hiding this comment.
What is the reason this method is touched?
|
|
||
| async fn wait_for_bitcoind_block_in_electrs<E: ElectrumApi>( | ||
| bitcoind: &BitcoindClient, electrs: &E, min_height: usize, | ||
| mut expected_block_hash: Option<BlockHash>, |
There was a problem hiding this comment.
min_height and expected_block_hash and their interaction might benefit from a comment explaining how to use it.
3 participants
Previously, the reorg tests could be somewhat flaky. Here we attempt to stabilize them.