diff --git a/Cargo.lock b/Cargo.lock index d5f71b9b30115..df45454de0b7f 100644 --- a/Cargo.lock +++ b/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", diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index e1fe3f0361b27..68efa2e644e76 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/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" } diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 42f26aa77e2ca..227ecaa3707eb 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/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; diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index a5435bdfdce55..e10d24a16a264 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/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), WriteNotification(sc_network::PeerId, Vec), Report(sc_network::PeerId, sc_network::ReputationChange), @@ -39,7 +40,7 @@ enum Event { } #[derive(Clone)] -struct TestNetwork { +pub(crate) struct TestNetwork { sender: mpsc::UnboundedSender, } @@ -101,10 +102,10 @@ impl sc_network_gossip::ValidatorContext for TestNetwork { fn send_topic(&mut self, _: &sc_network::PeerId, _: Hash, _: bool) { } } -struct Tester { - net_handle: super::NetworkBridge, +pub(crate) struct Tester { + pub(crate) net_handle: super::NetworkBridge, gossip_validator: Arc>, - events: mpsc::UnboundedReceiver, + pub(crate) events: mpsc::UnboundedReceiver, } 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 { } // 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, TestNetwork, ) { diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index ef85bede1c1d9..7e6d1e7793a81 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/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(_, _)))); + }); + } +}