diff --git a/substrate/core/finality-grandpa/src/communication/gossip.rs b/substrate/core/finality-grandpa/src/communication/gossip.rs index e0db1f0ba2..831be7ad14 100644 --- a/substrate/core/finality-grandpa/src/communication/gossip.rs +++ b/substrate/core/finality-grandpa/src/communication/gossip.rs @@ -386,12 +386,14 @@ impl Misbehavior { struct PeerInfo { view: View, + roles: Roles, } impl PeerInfo { - fn new() -> Self { + fn new(roles: Roles) -> Self { PeerInfo { view: View::default(), + roles, } } } @@ -408,8 +410,8 @@ impl Default for Peers { } impl Peers { - 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 Inner { // 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 GossipValidator { } impl network_gossip::Validator for GossipValidator { - fn new_peer(&self, context: &mut dyn ValidatorContext, who: &PeerId, _roles: Roles) { + fn new_peer(&self, context: &mut dyn ValidatorContext, 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::::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"), + } + } } diff --git a/substrate/core/finality-grandpa/src/communication/tests.rs b/substrate/core/finality-grandpa/src/communication/tests.rs index 3ec3bc8dd4..b537c29dca 100644 --- a/substrate/core/finality-grandpa/src/communication/tests.rs +++ b/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)| {