Wait peers to connect before doing anything else (#7375)

There is a race condition in `NetworkBridgeRx` between registering
incoming peers and handling overseer signals. While the peer connection
was sent to `NetworkBridgeRx` first, sometimes the peer would be added to
`validation/collation_peers` with enough delay that the bridge handled
an overseer signal that was meant do send notifications to the registered
peer and as the peer was not present in the bridge, the notification was
never sent and the test would hang.

When peers are registered to `NetworkBridgeRx` using `connect_peer()`,
wait until they show up in `shared.validation/collation_peers` before
doing anything else.

Co-authored-by: parity-processbot <>

polkadot/node/network/bridge/src/rx/tests.rs

@@ -291,6 +291,27 @@ type VirtualOverseer = TestSubsystemContextHandle<NetworkBridgeRxMessage>;
 struct TestHarness {
 	network_handle: TestNetworkHandle,
 	virtual_overseer: VirtualOverseer,
+	shared: Shared,
+}
+
+// wait until all needed validation and collation peers have connected.
+async fn await_peer_connections(
+	shared: &Shared,
+	num_validation_peers: usize,
+	num_collation_peers: usize,
+) {
+	loop {
+		{
+			let shared = shared.0.lock();
+			if shared.validation_peers.len() == num_validation_peers &&
+				shared.collation_peers.len() == num_collation_peers
+			{
+				break
+			}
+		}
+
+		futures_timer::Delay::new(std::time::Duration::from_millis(100)).await;
+	}
 }
 
 fn test_harness<T: Future<Output = VirtualOverseer>>(
@@ -306,13 +327,14 @@ fn test_harness<T: Future<Output = VirtualOverseer>>(
 	let (context, virtual_overseer) =
 		polkadot_node_subsystem_test_helpers::make_subsystem_context(pool);
 	let network_stream = network.event_stream();
+	let shared = Shared::default();
 
 	let bridge = NetworkBridgeRx {
 		network_service: network,
 		authority_discovery_service: discovery,
 		metrics: Metrics(None),
 		sync_oracle,
-		shared: Shared::default(),
+		shared: shared.clone(),
 		peerset_protocol_names,
 	};
 
@@ -320,7 +342,7 @@ fn test_harness<T: Future<Output = VirtualOverseer>>(
 		.map_err(|_| panic!("subsystem execution failed"))
 		.map(|_| ());
 
-	let test_fut = test(TestHarness { network_handle, virtual_overseer });
+	let test_fut = test(TestHarness { network_handle, virtual_overseer, shared });
 
 	futures::pin_mut!(test_fut);
 	futures::pin_mut!(network_bridge);
@@ -386,7 +408,7 @@ async fn assert_sends_collation_event_to_all(
 fn send_our_view_upon_connection() {
 	let (oracle, handle) = make_sync_oracle(false);
 	test_harness(Box::new(oracle), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer = PeerId::random();
 
@@ -411,6 +433,8 @@ fn send_our_view_upon_connection() {
 			.connect_peer(peer.clone(), PeerSet::Collation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 1).await;
+
 		let view = view![head];
 		let actions = network_handle.next_network_actions(2).await;
 		assert_network_actions_contains(
@@ -437,7 +461,7 @@ fn send_our_view_upon_connection() {
 fn sends_view_updates_to_peers() {
 	let (oracle, handle) = make_sync_oracle(false);
 	test_harness(Box::new(oracle), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer_a = PeerId::random();
 		let peer_b = PeerId::random();
@@ -458,6 +482,8 @@ fn sends_view_updates_to_peers() {
 			.connect_peer(peer_b.clone(), PeerSet::Collation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 1).await;
+
 		let actions = network_handle.next_network_actions(2).await;
 		let wire_message =
 			WireMessage::<protocol_v1::ValidationProtocol>::ViewUpdate(View::default()).encode();
@@ -506,7 +532,7 @@ fn sends_view_updates_to_peers() {
 fn do_not_send_view_update_until_synced() {
 	let (oracle, handle) = make_sync_oracle(true);
 	test_harness(Box::new(oracle), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer_a = PeerId::random();
 		let peer_b = PeerId::random();
@@ -519,6 +545,8 @@ fn do_not_send_view_update_until_synced() {
 			.connect_peer(peer_b.clone(), PeerSet::Collation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 1).await;
+
 		{
 			let actions = network_handle.next_network_actions(2).await;
 			let wire_message =
@@ -600,7 +628,7 @@ fn do_not_send_view_update_until_synced() {
 #[test]
 fn do_not_send_view_update_when_only_finalized_block_changed() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer_a = PeerId::random();
 		let peer_b = PeerId::random();
@@ -612,6 +640,8 @@ fn do_not_send_view_update_when_only_finalized_block_changed() {
 			.connect_peer(peer_b.clone(), PeerSet::Validation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 2, 0).await;
+
 		let hash_a = Hash::repeat_byte(1);
 
 		virtual_overseer
@@ -660,7 +690,7 @@ fn do_not_send_view_update_when_only_finalized_block_changed() {
 #[test]
 fn peer_view_updates_sent_via_overseer() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer = PeerId::random();
 
@@ -668,6 +698,8 @@ fn peer_view_updates_sent_via_overseer() {
 			.connect_peer(peer.clone(), PeerSet::Validation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 0).await;
+
 		let view = view![Hash::repeat_byte(1)];
 
 		// bridge will inform about all connected peers.
@@ -710,7 +742,7 @@ fn peer_view_updates_sent_via_overseer() {
 #[test]
 fn peer_messages_sent_via_overseer() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer = PeerId::random();
 
@@ -718,6 +750,8 @@ fn peer_messages_sent_via_overseer() {
 			.connect_peer(peer.clone(), PeerSet::Validation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 0).await;
+
 		// bridge will inform about all connected peers.
 		{
 			assert_sends_validation_event_to_all(
@@ -782,7 +816,7 @@ fn peer_messages_sent_via_overseer() {
 #[test]
 fn peer_disconnect_from_just_one_peerset() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer = PeerId::random();
 
@@ -793,6 +827,8 @@ fn peer_disconnect_from_just_one_peerset() {
 			.connect_peer(peer.clone(), PeerSet::Collation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 1).await;
+
 		// bridge will inform about all connected peers.
 		{
 			assert_sends_validation_event_to_all(
@@ -874,7 +910,7 @@ fn peer_disconnect_from_just_one_peerset() {
 #[test]
 fn relays_collation_protocol_messages() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer_a = PeerId::random();
 		let peer_b = PeerId::random();
@@ -886,6 +922,8 @@ fn relays_collation_protocol_messages() {
 			.connect_peer(peer_b.clone(), PeerSet::Collation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 1).await;
+
 		// bridge will inform about all connected peers.
 		{
 			assert_sends_validation_event_to_all(
@@ -978,7 +1016,7 @@ fn relays_collation_protocol_messages() {
 #[test]
 fn different_views_on_different_peer_sets() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer = PeerId::random();
 
@@ -989,6 +1027,8 @@ fn different_views_on_different_peer_sets() {
 			.connect_peer(peer.clone(), PeerSet::Collation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 1).await;
+
 		// bridge will inform about all connected peers.
 		{
 			assert_sends_validation_event_to_all(
@@ -1065,7 +1105,7 @@ fn different_views_on_different_peer_sets() {
 #[test]
 fn sent_views_include_finalized_number_update() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, mut virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, mut virtual_overseer, shared } = test_harness;
 
 		let peer_a = PeerId::random();
 
@@ -1073,6 +1113,8 @@ fn sent_views_include_finalized_number_update() {
 			.connect_peer(peer_a.clone(), PeerSet::Validation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 0).await;
+
 		let hash_a = Hash::repeat_byte(1);
 		let hash_b = Hash::repeat_byte(2);
 
@@ -1110,7 +1152,7 @@ fn sent_views_include_finalized_number_update() {
 #[test]
 fn view_finalized_number_can_not_go_down() {
 	test_harness(done_syncing_oracle(), |test_harness| async move {
-		let TestHarness { mut network_handle, virtual_overseer } = test_harness;
+		let TestHarness { mut network_handle, virtual_overseer, shared } = test_harness;
 
 		let peer_a = PeerId::random();
 
@@ -1118,6 +1160,8 @@ fn view_finalized_number_can_not_go_down() {
 			.connect_peer(peer_a.clone(), PeerSet::Validation, ObservedRole::Full)
 			.await;
 
+		await_peer_connections(&shared, 1, 0).await;
+
 		network_handle
 			.peer_message(
 				peer_a.clone(),