client/finality-grandpa: Add regression test for observer polling network (was #4778) (#4795)

* client/finality-grandpa: Add regression test observer polling network

Ensure `Future` implementation of `ObserverWork` is polling its
`NetworkBridge`. Regression test for bug introduced in d9837d7dd and
fixed in 504b4e89e.

When polled, `NetworkBridge` forwards reputation change requests from
the `GossipValidator` to the underlying `dyn Network`. This test
triggers a reputation change by calling `GossipValidator::validate` with
an invalid gossip message. After polling the `ObserverWork` which should
poll the `NetworkBridge`, the reputation change should be forwarded to
the test network.

* Nits

Co-authored-by: Max Inden <mail@max-inden.de>

substrate/Cargo.lock

@@ -5572,6 +5572,7 @@ dependencies = [
 name = "sc-finality-grandpa"
 version = "0.8.0"
 dependencies = [
+ "assert_matches 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "env_logger 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "finality-grandpa 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "fork-tree 2.0.0",

substrate/client/finality-grandpa/Cargo.toml

@@ -12,6 +12,7 @@ futures-timer = "2.0.2"
 log = "0.4.8"
 parking_lot = "0.9.0"
 rand = "0.7.2"
+assert_matches = "1.3.0"
 parity-scale-codec = { version = "1.0.0", features = ["derive"] }
 sp-arithmetic = { version = "2.0.0", path = "../../primitives/arithmetic" }
 sp-runtime = { version = "2.0.0", path = "../../primitives/runtime" }

substrate/client/finality-grandpa/src/communication/mod.rs

@@ -62,7 +62,7 @@ pub mod gossip;
 mod periodic;
 
 #[cfg(test)]
-mod tests;
+pub(crate) mod tests;
 
 pub use sp_finality_grandpa::GRANDPA_ENGINE_ID;
 

substrate/client/finality-grandpa/src/communication/tests.rs

@@ -31,7 +31,8 @@ use sp_finality_grandpa::{AuthorityList, GRANDPA_ENGINE_ID};
 use super::gossip::{self, GossipValidator};
 use super::{AuthorityId, VoterSet, Round, SetId};
 
-enum Event {
+#[derive(Debug)]
+pub(crate) enum Event {
 	EventStream(mpsc::UnboundedSender<NetworkEvent>),
 	WriteNotification(sc_network::PeerId, Vec<u8>),
 	Report(sc_network::PeerId, sc_network::ReputationChange),
@@ -39,7 +40,7 @@ enum Event {
 }
 
 #[derive(Clone)]
-struct TestNetwork {
+pub(crate) struct TestNetwork {
 	sender: mpsc::UnboundedSender<Event>,
 }
 
@@ -101,10 +102,10 @@ impl sc_network_gossip::ValidatorContext<Block> for TestNetwork {
 	fn send_topic(&mut self, _: &sc_network::PeerId, _: Hash, _: bool) { }
 }
 
-struct Tester {
-	net_handle: super::NetworkBridge<Block, TestNetwork>,
+pub(crate) struct Tester {
+	pub(crate) net_handle: super::NetworkBridge<Block, TestNetwork>,
 	gossip_validator: Arc<GossipValidator<Block>>,
-	events: mpsc::UnboundedReceiver<Event>,
+	pub(crate) events: mpsc::UnboundedReceiver<Event>,
 }
 
 impl Tester {
@@ -122,6 +123,14 @@ impl Tester {
 			}
 		})
 	}
+
+	pub(crate) fn trigger_gossip_validator_reputation_change(&self, p: &PeerId) {
+		self.gossip_validator.validate(
+			&mut crate::communication::tests::NoopContext,
+			p,
+			&vec![1, 2, 3],
+		);
+	}
 }
 
 // some random config (not really needed)
@@ -156,7 +165,7 @@ fn voter_set_state() -> SharedVoterSetState<Block> {
 }
 
 // needs to run in a tokio runtime.
-fn make_test_network(executor: &impl futures::task::Spawn) -> (
+pub(crate) fn make_test_network(executor: &impl futures::task::Spawn) -> (
 	impl Future<Output = Tester>,
 	TestNetwork,
 ) {

substrate/client/finality-grandpa/src/observer.rs

@@ -370,3 +370,79 @@ where
 		Future::poll(Pin::new(&mut self.network), cx)
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	use assert_matches::assert_matches;
+	use crate::{aux_schema,	communication::tests::{Event, make_test_network}};
+	use substrate_test_runtime_client::{TestClientBuilder, TestClientBuilderExt};
+	use sc_network::PeerId;
+
+	use futures::executor::{self, ThreadPool};
+
+	/// Ensure `Future` implementation of `ObserverWork` is polling its `NetworkBridge`. Regression
+	/// test for bug introduced in d4fbb897c and fixed in b7af8b339.
+	///
+	/// When polled, `NetworkBridge` forwards reputation change requests from the `GossipValidator`
+	/// to the underlying `dyn Network`. This test triggers a reputation change by calling
+	/// `GossipValidator::validate` with an invalid gossip message. After polling the `ObserverWork`
+	/// which should poll the `NetworkBridge`, the reputation change should be forwarded to the test
+	/// network.
+	#[test]
+	fn observer_work_polls_underlying_network_bridge() {
+		let thread_pool = ThreadPool::new().unwrap();
+
+		// Create a test network.
+		let (tester_fut, _network) = make_test_network(&thread_pool);
+		let mut tester = executor::block_on(tester_fut);
+
+		// Create an observer.
+		let (client, backend) = {
+			let builder = TestClientBuilder::with_default_backend();
+			let backend = builder.backend();
+			let (client, _) = builder.build_with_longest_chain();
+			(Arc::new(client), backend)
+		};
+
+		let persistent_data = aux_schema::load_persistent(
+			&*backend,
+			client.chain_info().genesis_hash,
+			0,
+			|| Ok(vec![]),
+		).unwrap();
+
+		let (_tx, voter_command_rx) = mpsc::unbounded();
+		let observer = ObserverWork::new(
+			client,
+			tester.net_handle.clone(),
+			persistent_data,
+			None,
+			voter_command_rx,
+		);
+
+		// Trigger a reputation change through the gossip validator.
+		let peer_id = PeerId::random();
+		tester.trigger_gossip_validator_reputation_change(&peer_id);
+
+		executor::block_on(async move {
+			// Ignore initial event stream request by gossip engine.
+			match tester.events.next().now_or_never() {
+				Some(Some(Event::EventStream(_))) => {},
+				_ => panic!("expected event stream request"),
+			};
+
+			assert!(
+				tester.events.next().now_or_never().is_none(),
+				"expect no further network events",
+			);
+
+			// Poll the observer once and have it forward the reputation change from the gossip
+			// validator to the test network.
+			assert!(observer.now_or_never().is_none());
+
+			assert_matches!(tester.events.next().now_or_never(), Some(Some(Event::Report(_, _))));
+		});
+	}
+}