diff --git a/substrate/client/finality-grandpa/src/communication/gossip.rs b/substrate/client/finality-grandpa/src/communication/gossip.rs index 7fef47867f..77c3543aae 100644 --- a/substrate/client/finality-grandpa/src/communication/gossip.rs +++ b/substrate/client/finality-grandpa/src/communication/gossip.rs @@ -147,14 +147,6 @@ impl Default for View { } impl View { - /// Update the set ID. implies a reset to round 0. - fn update_set(&mut self, set_id: SetId) { - if set_id != self.set_id { - self.set_id = set_id; - self.round = Round(1); - } - } - /// Consider a round and set ID combination under a current view. fn consider_vote(&self, round: Round, set_id: SetId) -> Consider { // only from current set @@ -188,6 +180,39 @@ impl View { } } +/// A local view of protocol state. Only differs from `View` in that we also +/// track the round and set id at which the last commit was observed. +struct LocalView { + round: Round, + set_id: SetId, + last_commit: Option<(N, Round, SetId)>, +} + +impl LocalView { + /// Converts the local view to a `View` discarding round and set id + /// information about the last commit. + fn as_view(&self) -> View<&N> { + View { + round: self.round, + set_id: self.set_id, + last_commit: self.last_commit_height(), + } + } + + /// Update the set ID. implies a reset to round 1. + fn update_set(&mut self, set_id: SetId) { + if set_id != self.set_id { + self.set_id = set_id; + self.round = Round(1); + } + } + + /// Returns the height of the block that the last observed commit finalizes. + fn last_commit_height(&self) -> Option<&N> { + self.last_commit.as_ref().map(|(number, _, _)| number) + } +} + const KEEP_RECENT_ROUNDS: usize = 3; /// Tracks gossip topics that we are keeping messages for. We keep topics of: @@ -614,7 +639,7 @@ impl CatchUpConfig { } struct Inner { - local_view: Option>>, + local_view: Option>>, peers: Peers>, live_topics: KeepTopics, round_start: Instant, @@ -690,7 +715,7 @@ impl Inner { fn note_set(&mut self, set_id: SetId, authorities: Vec) -> MaybeMessage { { let local_view = match self.local_view { - ref mut x @ None => x.get_or_insert(View { + ref mut x @ None => x.get_or_insert(LocalView { round: Round(1), set_id, last_commit: None, @@ -710,12 +735,17 @@ impl Inner { } /// Note that we've imported a commit finalizing a given block. - fn note_commit_finalized(&mut self, finalized: NumberFor) -> MaybeMessage { + fn note_commit_finalized( + &mut self, + round: Round, + set_id: SetId, + finalized: NumberFor, + ) -> MaybeMessage { { match self.local_view { None => return None, - Some(ref mut v) => if v.last_commit.as_ref() < Some(&finalized) { - v.last_commit = Some(finalized); + Some(ref mut v) => if v.last_commit_height() < Some(&finalized) { + v.last_commit = Some((finalized, round, set_id)); } else { return None }, @@ -726,12 +756,16 @@ impl Inner { } fn consider_vote(&self, round: Round, set_id: SetId) -> Consider { - self.local_view.as_ref().map(|v| v.consider_vote(round, set_id)) + self.local_view.as_ref() + .map(LocalView::as_view) + .map(|v| v.consider_vote(round, set_id)) .unwrap_or(Consider::RejectOutOfScope) } fn consider_global(&self, set_id: SetId, number: NumberFor) -> Consider { - self.local_view.as_ref().map(|v| v.consider_global(set_id, number)) + self.local_view.as_ref() + .map(LocalView::as_view) + .map(|v| v.consider_global(set_id, &number)) .unwrap_or(Consider::RejectOutOfScope) } @@ -1012,7 +1046,7 @@ impl Inner { let packet = NeighborPacket { round: local_view.round, set_id: local_view.set_id, - commit_finalized_height: local_view.last_commit.unwrap_or(Zero::zero()), + commit_finalized_height: *local_view.last_commit_height().unwrap_or(&Zero::zero()), }; let peers = self.peers.inner.keys().cloned().collect(); @@ -1210,10 +1244,21 @@ impl GossipValidator { } /// Note that we've imported a commit finalizing a given block. - pub(super) fn note_commit_finalized(&self, finalized: NumberFor, send_neighbor: F) + pub(super) fn note_commit_finalized( + &self, + round: Round, + set_id: SetId, + finalized: NumberFor, + send_neighbor: F, + ) where F: FnOnce(Vec, NeighborPacket>) { - let maybe_msg = self.inner.write().note_commit_finalized(finalized); + let maybe_msg = self.inner.write().note_commit_finalized( + round, + set_id, + finalized, + ); + if let Some((to, msg)) = maybe_msg { send_neighbor(to, msg); } @@ -1289,7 +1334,7 @@ impl sc_network_gossip::Validator for GossipValidator sc_network_gossip::Validator for GossipValidator return false, // cannot evaluate until we have a local view. }; - let our_best_commit = local_view.last_commit; - let peer_best_commit = peer.view.last_commit; - match GossipMessage::::decode(&mut data) { Err(_) => false, Ok(GossipMessage::Commit(full)) => { - // we only broadcast our best commit and only if it's - // better than last received by peer. - Some(full.message.target_number) == our_best_commit && - Some(full.message.target_number) > peer_best_commit + // we only broadcast commit messages if they're for the same + // set the peer is in and if the commit is better than the + // last received by peer, additionally we make sure to only + // broadcast our best commit. + peer.view.consider_global(set_id, full.message.target_number) == Consider::Accept && + Some(&full.message.target_number) == local_view.last_commit_height() } Ok(GossipMessage::Neighbor(_)) => false, Ok(GossipMessage::CatchUpRequest(_)) => false, @@ -1432,12 +1476,17 @@ impl sc_network_gossip::Validator for GossipValidator::decode(&mut data) { Err(_) => true, - Ok(GossipMessage::Commit(full)) - => Some(full.message.target_number) != best_commit, + Ok(GossipMessage::Commit(full)) => match local_view.last_commit { + Some((number, round, set_id)) => + // we expire any commit message that doesn't target the same block + // as our best commit or isn't from the same round and set id + !(full.message.target_number == number && + full.round == round && + full.set_id == set_id), + None => true, + }, Ok(_) => true, } }) @@ -2306,4 +2355,133 @@ mod tests { } } } + + #[test] + fn only_gossip_commits_to_peers_on_same_set() { + let (val, _) = GossipValidator::::new(config(), voter_set_state()); + + // the validator start at set id 1 + val.note_set(SetId(1), Vec::new(), |_, _| {}); + + // add a new peer at set id 1 + let peer1 = PeerId::random(); + + val.inner + .write() + .peers + .new_peer(peer1.clone(), Roles::AUTHORITY); + + val.inner + .write() + .peers + .update_peer_state( + &peer1, + NeighborPacket { + round: Round(1), + set_id: SetId(1), + commit_finalized_height: 1, + }, + ) + .unwrap(); + + // peer2 will default to set id 0 + let peer2 = PeerId::random(); + val.inner + .write() + .peers + .new_peer(peer2.clone(), Roles::AUTHORITY); + + // create a commit for round 1 of set id 1 + // targeting a block at height 2 + let commit = { + let commit = finality_grandpa::CompactCommit { + target_hash: H256::random(), + target_number: 2, + precommits: Vec::new(), + auth_data: Vec::new(), + }; + + crate::communication::gossip::GossipMessage::::Commit( + crate::communication::gossip::FullCommitMessage { + round: Round(1), + set_id: SetId(1), + message: commit, + }, + ) + .encode() + }; + + // note the commit in the validator + val.note_commit_finalized(Round(1), SetId(1), 2, |_, _| {}); + + let mut message_allowed = val.message_allowed(); + + // the commit should be allowed to peer 1 + assert!(message_allowed( + &peer1, + MessageIntent::Broadcast, + &crate::communication::global_topic::(1), + &commit, + )); + + // but disallowed to peer 2 since the peer is on set id 0 + // the commit should be allowed to peer 1 + assert!(!message_allowed( + &peer2, + MessageIntent::Broadcast, + &crate::communication::global_topic::(1), + &commit, + )); + } + + #[test] + fn expire_commits_from_older_rounds() { + let (val, _) = GossipValidator::::new(config(), voter_set_state()); + + let commit = |round, set_id, target_number| { + let commit = finality_grandpa::CompactCommit { + target_hash: H256::random(), + target_number, + precommits: Vec::new(), + auth_data: Vec::new(), + }; + + crate::communication::gossip::GossipMessage::::Commit( + crate::communication::gossip::FullCommitMessage { + round: Round(round), + set_id: SetId(set_id), + message: commit, + }, + ) + .encode() + }; + + // note the beginning of a new set with id 1 + val.note_set(SetId(1), Vec::new(), |_, _| {}); + + // note a commit for round 1 in the validator + // finalizing a block at height 2 + val.note_commit_finalized(Round(1), SetId(1), 2, |_, _| {}); + + let mut message_expired = val.message_expired(); + + // a commit message for round 1 that finalizes the same height as we + // have observed previously should not be expired + assert!(!message_expired( + crate::communication::global_topic::(1), + &commit(1, 1, 2), + )); + + // it should be expired if it is for a lower block + assert!(message_expired( + crate::communication::global_topic::(1), + &commit(1, 1, 1), + )); + + // or the same block height but from the previous round + assert!(message_expired( + crate::communication::global_topic::(1), + &commit(0, 1, 2), + )); + } } diff --git a/substrate/client/finality-grandpa/src/communication/mod.rs b/substrate/client/finality-grandpa/src/communication/mod.rs index 7525c44f15..d496f305fc 100644 --- a/substrate/client/finality-grandpa/src/communication/mod.rs +++ b/substrate/client/finality-grandpa/src/communication/mod.rs @@ -497,7 +497,8 @@ fn incoming_global( return None; } - let round = msg.round.0; + let round = msg.round; + let set_id = msg.set_id; let commit = msg.message; let finalized_number = commit.target_number; let gossip_validator = gossip_validator.clone(); @@ -509,6 +510,8 @@ fn incoming_global( // any discrepancy between the actual ghost and the claimed // finalized number. gossip_validator.note_commit_finalized( + round, + set_id, finalized_number, |to, neighbor| neighbor_sender.send(to, neighbor), ); @@ -525,7 +528,7 @@ fn incoming_global( let cb = voter::Callback::Work(Box::new(cb)); - Some(voter::CommunicationIn::Commit(round, commit, cb)) + Some(voter::CommunicationIn::Commit(round.0, commit, cb)) }; let process_catch_up = move | @@ -1015,7 +1018,7 @@ impl Sink<(RoundNumber, Commit)> for CommitsOut { }; let message = GossipMessage::Commit(FullCommitMessage:: { - round: round, + round, set_id: self.set_id, message: compact_commit, }); @@ -1025,6 +1028,8 @@ impl Sink<(RoundNumber, Commit)> for CommitsOut { // the gossip validator needs to be made aware of the best commit-height we know of // before gossiping self.gossip_validator.note_commit_finalized( + round, + self.set_id, commit.target_number, |to, neighbor| self.neighbor_sender.send(to, neighbor), ); diff --git a/substrate/client/finality-grandpa/src/communication/tests.rs b/substrate/client/finality-grandpa/src/communication/tests.rs index 96761a2f3c..fb2c0f12f4 100644 --- a/substrate/client/finality-grandpa/src/communication/tests.rs +++ b/substrate/client/finality-grandpa/src/communication/tests.rs @@ -292,12 +292,32 @@ fn good_commit_leads_to_relay() { }); // Add a random peer which will be the recipient of this message + let receiver_id = sc_network::PeerId::random(); let _ = sender.unbounded_send(NetworkEvent::NotificationStreamOpened { - remote: sc_network::PeerId::random(), + remote: receiver_id.clone(), engine_id: GRANDPA_ENGINE_ID, roles: Roles::FULL, }); + // Announce its local set has being on the current set id through a neighbor + // packet, otherwise it won't be eligible to receive the commit + let _ = { + let update = gossip::VersionedNeighborPacket::V1( + gossip::NeighborPacket { + round: Round(round), + set_id: SetId(set_id), + commit_finalized_height: 1, + } + ); + + let msg = gossip::GossipMessage::::Neighbor(update); + + sender.unbounded_send(NetworkEvent::NotificationsReceived { + remote: receiver_id, + messages: vec![(GRANDPA_ENGINE_ID, msg.encode().into())], + }) + }; + true } _ => false,