grandpa: only send catch-up requests to authorities (#3499)

* grandpa: only send catch-up requests to authorities

* grandpa: fix gossip tests

* grandpa: test sending catch-up requests only to authorities

* grandpa: fix catch-up test

* grandpa: apply suggestions from code review

substrate/core/finality-grandpa/src/communication/gossip.rs

@@ -386,12 +386,14 @@ impl Misbehavior {
 
 struct PeerInfo<N> {
 	view: View<N>,
+	roles: Roles,
 }
 
 impl<N> PeerInfo<N> {
-	fn new() -> Self {
+	fn new(roles: Roles) -> Self {
 		PeerInfo {
 			view: View::default(),
+			roles,
 		}
 	}
 }
@@ -408,8 +410,8 @@ impl<N> Default for Peers<N> {
 }
 
 impl<N: Ord> Peers<N> {
-	fn new_peer(&mut self, who: PeerId) {
-		self.inner.insert(who, PeerInfo::new());
+	fn new_peer(&mut self, who: PeerId, roles: Roles) {
+		self.inner.insert(who, PeerInfo::new(roles));
 	}
 
 	fn peer_disconnected(&mut self, who: &PeerId) {
@@ -815,9 +817,12 @@ impl<Block: BlockT> Inner<Block> {
 
 		// if the peer is on the same set and ahead of us by a margin bigger
 		// than `CATCH_UP_THRESHOLD` then we should ask it for a catch up
-		// message.
+		// message. we only send catch-up requests to authorities, observers
+		// won't be able to reply since they don't follow the full GRANDPA
+		// protocol and therefore might not have the vote data available.
 		if let (Some(peer), Some(local_view)) = (self.peers.peer(who), &self.local_view) {
-			if peer.view.set_id == local_view.set_id &&
+			if peer.roles.is_authority() &&
+				peer.view.set_id == local_view.set_id &&
 				peer.view.round.0.saturating_sub(CATCH_UP_THRESHOLD) > local_view.round.0
 			{
 				// send catch up request if allowed
@@ -1033,10 +1038,10 @@ impl<Block: BlockT> GossipValidator<Block> {
 }
 
 impl<Block: BlockT> network_gossip::Validator<Block> for GossipValidator<Block> {
-	fn new_peer(&self, context: &mut dyn ValidatorContext<Block>, who: &PeerId, _roles: Roles) {
+	fn new_peer(&self, context: &mut dyn ValidatorContext<Block>, who: &PeerId, roles: Roles) {
 		let packet = {
 			let mut inner = self.inner.write();
-			inner.peers.new_peer(who.clone());
+			inner.peers.new_peer(who.clone(), roles);
 
 			inner.local_view.as_ref().map(|v| {
 				NeighborPacket {
@@ -1324,7 +1329,7 @@ mod tests {
 		assert!(res.unwrap().is_none());
 
 		// connect & disconnect.
-		peers.new_peer(id.clone());
+		peers.new_peer(id.clone(), Roles::AUTHORITY);
 		peers.peer_disconnected(&id);
 
 		let res = peers.update_peer_state(&id, update.clone());
@@ -1360,7 +1365,7 @@ mod tests {
 		let mut peers = Peers::default();
 		let id = PeerId::random();
 
-		peers.new_peer(id.clone());
+		peers.new_peer(id.clone(), Roles::AUTHORITY);
 
 		let mut check_update = move |update: NeighborPacket<_>| {
 			let view = peers.update_peer_state(&id, update.clone()).unwrap().unwrap();
@@ -1380,7 +1385,7 @@ mod tests {
 		let mut peers = Peers::default();
 
 		let id = PeerId::random();
-		peers.new_peer(id.clone());
+		peers.new_peer(id.clone(), Roles::AUTHORITY);
 
 		peers.update_peer_state(&id, NeighborPacket {
 			round: Round(10),
@@ -1581,7 +1586,7 @@ mod tests {
 		// add the peer making the request to the validator,
 		// otherwise it is discarded
 		let mut inner = val.inner.write();
-		inner.peers.new_peer(peer.clone());
+		inner.peers.new_peer(peer.clone(), Roles::AUTHORITY);
 
 		let res = inner.handle_catch_up_request(
 			&peer,
@@ -1632,7 +1637,7 @@ mod tests {
 		// add the peer making the request to the validator,
 		// otherwise it is discarded
 		let peer = PeerId::random();
-		val.inner.write().peers.new_peer(peer.clone());
+		val.inner.write().peers.new_peer(peer.clone(), Roles::AUTHORITY);
 
 		let send_request = |set_id, round| {
 			let mut inner = val.inner.write();
@@ -1712,7 +1717,7 @@ mod tests {
 		// add the peer making the request to the validator,
 		// otherwise it is discarded.
 		let peer = PeerId::random();
-		val.inner.write().peers.new_peer(peer.clone());
+		val.inner.write().peers.new_peer(peer.clone(), Roles::AUTHORITY);
 
 		let import_neighbor_message = |set_id, round| {
 			let (_, _, catch_up_request, _) = val.inner.write().import_neighbor_message(
@@ -1775,7 +1780,7 @@ mod tests {
 		// add the peer making the request to the validator,
 		// otherwise it is discarded.
 		let peer = PeerId::random();
-		val.inner.write().peers.new_peer(peer.clone());
+		val.inner.write().peers.new_peer(peer.clone(), Roles::AUTHORITY);
 
 		// importing a neighbor message from a peer in the same set in a later
 		// round should lead to a catch up request but since they're disabled
@@ -1794,4 +1799,54 @@ mod tests {
 			_ => panic!("expected no catch up message"),
 		}
 	}
+
+	#[test]
+	fn doesnt_send_catch_up_requests_to_non_authorities() {
+		let (val, _) = GossipValidator::<Block>::new(
+			config(),
+			voter_set_state(),
+			true,
+		);
+
+		// the validator starts at set id 1.
+		val.note_set(SetId(1), Vec::new(), |_, _| {});
+
+		// add the peers making the requests to the validator,
+		// otherwise it is discarded.
+		let peer_authority = PeerId::random();
+		let peer_full = PeerId::random();
+
+		val.inner.write().peers.new_peer(peer_authority.clone(), Roles::AUTHORITY);
+		val.inner.write().peers.new_peer(peer_full.clone(), Roles::FULL);
+
+		let import_neighbor_message = |peer| {
+			let (_, _, catch_up_request, _) = val.inner.write().import_neighbor_message(
+				&peer,
+				NeighborPacket {
+					round: Round(42),
+					set_id: SetId(1),
+					commit_finalized_height: 50,
+				},
+			);
+
+			catch_up_request
+		};
+
+		// importing a neighbor message from a peer in the same set in a later
+		// round should lead to a catch up request but since the node is not an
+		// authority we should get `None`.
+		if import_neighbor_message(peer_full).is_some() {
+			panic!("expected no catch up message");
+		}
+
+		// importing the same neighbor message from a peer who is an authority
+		// should lead to a catch up request.
+		match import_neighbor_message(peer_authority) {
+			Some(GossipMessage::CatchUpRequest(request)) => {
+				assert_eq!(request.set_id, SetId(1));
+				assert_eq!(request.round, Round(41));
+			},
+			_ => panic!("expected catch up message"),
+		}
+	}
 }

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

@@ -448,8 +448,8 @@ fn peer_with_higher_view_leads_to_catch_up_request() {
 	let (tester, mut net) = make_test_network();
 	let test = tester
 		.and_then(move |tester| {
-			// register a peer.
-			tester.gossip_validator.new_peer(&mut NoopContext, &id, network::config::Roles::FULL);
+			// register a peer with authority role.
+			tester.gossip_validator.new_peer(&mut NoopContext, &id, network::config::Roles::AUTHORITY);
 			Ok((tester, id))
 		})
 		.and_then(move |(tester, id)| {