From 660c882cd3ead34b55367b07ee5d6e848af5bed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 7 Jan 2020 22:34:54 +0000 Subject: [PATCH] grandpa: guarantee that vote limit is never lower than vote base (#4563) --- .../finality-grandpa/src/authorities.rs | 59 +++++++++++++++++-- .../finality-grandpa/src/environment.rs | 30 ++++------ 2 files changed, 66 insertions(+), 23 deletions(-) diff --git a/substrate/client/finality-grandpa/src/authorities.rs b/substrate/client/finality-grandpa/src/authorities.rs index 683ff7e764..5e295d0bae 100644 --- a/substrate/client/finality-grandpa/src/authorities.rs +++ b/substrate/client/finality-grandpa/src/authorities.rs @@ -51,9 +51,10 @@ impl SharedAuthoritySet where N: Add + Ord + Clone + Debug, H: Clone + Debug { - /// Get the earliest limit-block number, if any. - pub(crate) fn current_limit(&self) -> Option { - self.inner.read().current_limit() + /// Get the earliest limit-block number that's higher or equal to the given + /// min number, if any. + pub(crate) fn current_limit(&self, min: N) -> Option { + self.inner.read().current_limit(min) } /// Get the current set ID. This is incremented every time the set changes. @@ -224,10 +225,13 @@ where /// Get the earliest limit-block number, if any. If there are pending changes across /// different forks, this method will return the earliest effective number (across the - /// different branches). Only standard changes are taken into account for the current + /// different branches) that is higher or equal to the given min number. + /// + /// Only standard changes are taken into account for the current /// limit, since any existing forced change should preclude the voter from voting. - pub(crate) fn current_limit(&self) -> Option { + pub(crate) fn current_limit(&self, min: N) -> Option { self.pending_standard_changes.roots() + .filter(|&(_, _, c)| c.effective_number() >= min) .min_by_key(|&(_, _, c)| c.effective_number()) .map(|(_, _, c)| c.effective_number()) } @@ -445,6 +449,51 @@ mod tests { move |base, hash| Ok(f(base, hash)) } + #[test] + fn current_limit_filters_min() { + let mut authorities = AuthoritySet { + current_authorities: Vec::new(), + set_id: 0, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }; + + let change = |height| { + PendingChange { + next_authorities: Vec::new(), + delay: 0, + canon_height: height, + canon_hash: height.to_string(), + delay_kind: DelayKind::Finalized, + } + }; + + let is_descendent_of = static_is_descendent_of(false); + + authorities.add_pending_change(change(1), &is_descendent_of).unwrap(); + authorities.add_pending_change(change(2), &is_descendent_of).unwrap(); + + assert_eq!( + authorities.current_limit(0), + Some(1), + ); + + assert_eq!( + authorities.current_limit(1), + Some(1), + ); + + assert_eq!( + authorities.current_limit(2), + Some(2), + ); + + assert_eq!( + authorities.current_limit(3), + None, + ); + } + #[test] fn changes_iterated_in_pre_order() { let mut authorities = AuthoritySet { diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs index e47a17e2a5..ac01d5294f 100644 --- a/substrate/client/finality-grandpa/src/environment.rs +++ b/substrate/client/finality-grandpa/src/environment.rs @@ -432,32 +432,26 @@ where return None; } + let base_header = match self.client.header(&BlockId::Hash(block)).ok()? { + Some(h) => h, + None => { + debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find base block", block); + return None; + } + }; + // we refuse to vote beyond the current limit number where transitions are scheduled to // occur. // once blocks are finalized that make that transition irrelevant or activate it, // we will proceed onwards. most of the time there will be no pending transition. - let limit = self.authority_set.current_limit(); + // the limit, if any, is guaranteed to be higher than or equal to the given base number. + let limit = self.authority_set.current_limit(*base_header.number()); debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit); match self.select_chain.finality_target(block, None) { Ok(Some(best_hash)) => { - let base_header = self.client.header(&BlockId::Hash(block)).ok()? - .expect("Header known to exist after `best_containing` call; qed"); - - if let Some(limit) = limit { - // this is a rare case which might cause issues, - // might be better to return the header itself. - if *base_header.number() > limit { - debug!(target: "afg", "Encountered error finding best chain containing {:?} with limit {:?}: target block is after limit", - block, - limit, - ); - return None; - } - } - let best_header = self.client.header(&BlockId::Hash(best_hash)).ok()? - .expect("Header known to exist after `best_containing` call; qed"); + .expect("Header known to exist after `finality_target` call; qed"); // check if our vote is currently being limited due to a pending change let limit = limit.filter(|limit| limit < best_header.number()); @@ -481,7 +475,7 @@ where } target_header = self.client.header(&BlockId::Hash(*target_header.parent_hash())).ok()? - .expect("Header known to exist after `best_containing` call; qed"); + .expect("Header known to exist after `finality_target` call; qed"); } target = target_header;