From 5b9952ef1c6ad91b1f7f335f5ea2d116a1631028 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 18 Oct 2019 00:25:18 +0100 Subject: [PATCH] grandpa: don't expire next round messages (#3845) * grandpa: don't expire next round messages * grandpa: fix test * grandpa: first round in set is 1 * grandpa: fix test * grandpa: update doc --- .../src/communication/gossip.rs | 81 ++++++++++++++----- .../src/communication/tests.rs | 4 +- 2 files changed, 62 insertions(+), 23 deletions(-) diff --git a/substrate/core/finality-grandpa/src/communication/gossip.rs b/substrate/core/finality-grandpa/src/communication/gossip.rs index 831be7ad14..1f9d78c867 100644 --- a/substrate/core/finality-grandpa/src/communication/gossip.rs +++ b/substrate/core/finality-grandpa/src/communication/gossip.rs @@ -132,7 +132,7 @@ struct View { impl Default for View { fn default() -> Self { View { - round: Round(0), + round: Round(1), set_id: SetId(0), last_commit: None, } @@ -144,7 +144,7 @@ impl View { fn update_set(&mut self, set_id: SetId) { if set_id != self.set_id { self.set_id = set_id; - self.round = Round(0); + self.round = Round(1); } } @@ -194,21 +194,30 @@ impl KeepTopics { fn new() -> Self { KeepTopics { current_set: SetId(0), - rounds: VecDeque::with_capacity(KEEP_RECENT_ROUNDS + 1), + rounds: VecDeque::with_capacity(KEEP_RECENT_ROUNDS + 2), reverse_map: HashMap::new(), } } fn push(&mut self, round: Round, set_id: SetId) { self.current_set = std::cmp::max(self.current_set, set_id); - self.rounds.push_back((round, set_id)); - // the 1 is for the current round. - while self.rounds.len() > KEEP_RECENT_ROUNDS + 1 { + // under normal operation the given round is already tracked (since we + // track one round ahead). if we skip rounds (with a catch up) the given + // round topic might not be tracked yet. + if !self.rounds.contains(&(round, set_id)) { + self.rounds.push_back((round, set_id)); + } + + // we also accept messages for the next round + self.rounds.push_back((Round(round.0.saturating_add(1)), set_id)); + + // the 2 is for the current and next round. + while self.rounds.len() > KEEP_RECENT_ROUNDS + 2 { let _ = self.rounds.pop_front(); } - let mut map = HashMap::with_capacity(KEEP_RECENT_ROUNDS + 2); + let mut map = HashMap::with_capacity(KEEP_RECENT_ROUNDS + 3); map.insert(super::global_topic::(self.current_set.0), (None, self.current_set)); for &(round, set) in &self.rounds { @@ -554,7 +563,7 @@ impl Inner { { let local_view = match self.local_view { ref mut x @ None => x.get_or_insert(View { - round: Round(0), + round: Round(1), set_id, last_commit: None, }), @@ -566,7 +575,7 @@ impl Inner { }; local_view.update_set(set_id); - self.live_topics.push(Round(0), set_id); + self.live_topics.push(Round(1), set_id); self.authorities = authorities; } self.multicast_neighbor_packet() @@ -1466,11 +1475,11 @@ mod tests { let peer = PeerId::random(); val.note_set(SetId(set_id), vec![auth.clone()], |_, _| {}); - val.note_round(Round(0), |_, _| {}); + val.note_round(Round(1), |_, _| {}); let inner = val.inner.read(); let unknown_voter = inner.validate_round_message(&peer, &VoteOrPrecommitMessage { - round: Round(0), + round: Round(1), set_id: SetId(set_id), message: SignedMessage:: { message: grandpa::Message::Prevote(grandpa::Prevote { @@ -1483,7 +1492,7 @@ mod tests { }); let bad_sig = inner.validate_round_message(&peer, &VoteOrPrecommitMessage { - round: Round(0), + round: Round(1), set_id: SetId(set_id), message: SignedMessage:: { message: grandpa::Message::Prevote(grandpa::Prevote { @@ -1512,7 +1521,7 @@ mod tests { let peer = PeerId::random(); val.note_set(SetId(set_id), vec![auth.clone()], |_, _| {}); - val.note_round(Round(0), |_, _| {}); + val.note_round(Round(1), |_, _| {}); let validate_catch_up = || { let mut inner = val.inner.write(); @@ -1548,19 +1557,19 @@ mod tests { #[test] fn unanswerable_catch_up_requests_discarded() { - // create voter set state with round 1 completed + // create voter set state with round 2 completed let set_state: SharedVoterSetState = { let mut completed_rounds = voter_set_state().read().completed_rounds(); completed_rounds.push(environment::CompletedRound { - number: 1, + number: 2, state: grandpa::round::State::genesis(Default::default()), base: Default::default(), votes: Default::default(), }); let mut current_rounds = environment::CurrentRounds::new(); - current_rounds.insert(2, environment::HasVoted::No); + current_rounds.insert(3, environment::HasVoted::No); let set_state = environment::VoterSetState::::Live { completed_rounds, @@ -1581,7 +1590,7 @@ mod tests { let peer = PeerId::random(); val.note_set(SetId(set_id), vec![auth.clone()], |_, _| {}); - val.note_round(Round(2), |_, _| {}); + val.note_round(Round(3), |_, _| {}); // add the peer making the request to the validator, // otherwise it is discarded @@ -1597,7 +1606,7 @@ mod tests { &set_state, ); - // we're at round 2, a catch up request for round 10 is out of scope + // we're at round 3, a catch up request for round 10 is out of scope assert!(res.0.is_none()); assert_eq!(res.1, Action::Discard(cost::OUT_OF_SCOPE_MESSAGE)); @@ -1605,16 +1614,16 @@ mod tests { &peer, CatchUpRequestMessage { set_id: SetId(set_id), - round: Round(1), + round: Round(2), }, &set_state, ); - // a catch up request for round 1 should be answered successfully + // a catch up request for round 2 should be answered successfully match res.0.unwrap() { GossipMessage::CatchUp(catch_up) => { assert_eq!(catch_up.set_id, SetId(set_id)); - assert_eq!(catch_up.message.round_number, 1); + assert_eq!(catch_up.message.round_number, 2); assert_eq!(res.1, Action::Discard(cost::CATCH_UP_REPLY)); }, @@ -1849,4 +1858,34 @@ mod tests { _ => panic!("expected catch up message"), } } + + #[test] + fn doesnt_expire_next_round_messages() { + // NOTE: this is a regression test + let (val, _) = GossipValidator::::new( + config(), + voter_set_state(), + true, + ); + + // the validator starts at set id 1. + val.note_set(SetId(1), Vec::new(), |_, _| {}); + + // we are at round 10 + val.note_round(Round(9), |_, _| {}); + val.note_round(Round(10), |_, _| {}); + + let mut is_expired = val.message_expired(); + + // we accept messages from rounds 9, 10 and 11 + // therefore neither of those should be considered expired + for round in &[9, 10, 11] { + assert!( + !is_expired( + crate::communication::round_topic::(*round, 1), + &[], + ) + ) + } + } } diff --git a/substrate/core/finality-grandpa/src/communication/tests.rs b/substrate/core/finality-grandpa/src/communication/tests.rs index 6215e30b80..14e54511fb 100644 --- a/substrate/core/finality-grandpa/src/communication/tests.rs +++ b/substrate/core/finality-grandpa/src/communication/tests.rs @@ -217,7 +217,7 @@ fn good_commit_leads_to_relay() { let public = make_ids(&private[..]); let voter_set = Arc::new(public.iter().cloned().collect::>()); - let round = 0; + let round = 1; let set_id = 1; let commit = { @@ -332,7 +332,7 @@ fn bad_commit_leads_to_report() { let public = make_ids(&private[..]); let voter_set = Arc::new(public.iter().cloned().collect::>()); - let round = 0; + let round = 1; let set_id = 1; let commit = {