From 637c4d91adec4d31e2808457cbd169648ae7e776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Mon, 12 Oct 2020 23:45:40 +0100 Subject: [PATCH 1/5] grandpa: fix early enactment of forced authority set changes --- client/finality-grandpa/src/authorities.rs | 182 ++++++++++++++++----- 1 file changed, 141 insertions(+), 41 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 7a064d7a6224b..483e4932c704d 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -32,14 +32,36 @@ use std::ops::Add; use std::sync::Arc; /// Error type returned on operations on the `AuthoritySet`. -#[derive(Debug, derive_more::Display, derive_more::From)] +#[derive(Debug, derive_more::Display)] pub enum Error { #[display("Invalid authority set, either empty or with an authority weight set to 0.")] InvalidAuthoritySet, + #[display("Client error during ancestry lookup: {}", _0)] + Client(E), + #[display("Duplicate authority set change.")] + DuplicateAuthoritySetChange, + #[display("Authority set change overlaps with existing change.")] + OverlappingAuthoritySetChange, #[display(fmt = "Invalid operation in the pending changes tree: {}", _0)] ForkTree(fork_tree::Error), } +impl From> for Error { + fn from(err: fork_tree::Error) -> Error { + match err { + fork_tree::Error::Client(err) => Error::Client(err), + fork_tree::Error::Duplicate => Error::DuplicateAuthoritySetChange, + err => Error::ForkTree(err), + } + } +} + +impl From for Error { + fn from(err: E) -> Error { + Error::Client(err) + } +} + /// A shared authority set. pub struct SharedAuthoritySet { inner: Arc>>, @@ -110,15 +132,20 @@ pub(crate) struct AuthoritySet { /// enacted on finality and must be enacted (i.e. finalized) in-order across /// a given branch pub(crate) pending_standard_changes: ForkTree>, - /// Pending forced changes across different forks (at most one per fork). - /// Forced changes are enacted on block depth (not finality), for this reason - /// only one forced change should exist per fork. + /// Pending forced changes across different forks. Forced changes are + /// enacted on block depth (not finality) and must be applied in-order + /// across a given branch. When trying to apply forced changes we keep track + /// of any pending standard changes that they may depend on, this is done by + /// making sure that any pending change that is an ancestor of the forced + /// changed and its effective block number is lower than the last finalized + /// block (as signaled in the forced change) must be applied beforehand. pending_forced_changes: Vec>, } impl AuthoritySet -where H: PartialEq, - N: Ord, +where + H: PartialEq, + N: Ord, { // authority sets must be non-empty and all weights must be greater than 0 fn invalid_authority_list(authorities: &AuthorityList) -> bool { @@ -180,7 +207,7 @@ where &self, best_hash: &H, is_descendent_of: &F, - ) -> Result, fork_tree::Error> + ) -> Result, Error> where F: Fn(&H, &H) -> Result, E: std::error::Error, @@ -250,16 +277,20 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), Error> where + ) -> Result<(), Error> + where F: Fn(&H, &H) -> Result, E: std::error::Error, { - for change in self.pending_forced_changes.iter() { - if change.canon_hash == pending.canon_hash || - is_descendent_of(&change.canon_hash, &pending.canon_hash) - .map_err(fork_tree::Error::Client)? + for change in &self.pending_forced_changes { + if change.canon_hash == pending.canon_hash { + return Err(Error::DuplicateAuthoritySetChange); + } + + if change.effective_number() >= pending.canon_height + && is_descendent_of(&change.canon_hash, &pending.canon_hash)? { - return Err(fork_tree::Error::UnfinalizedAncestor.into()); + return Err(Error::OverlappingAuthoritySetChange); } } @@ -341,6 +372,15 @@ where /// /// These transitions are always forced and do not lead to justifications /// which light clients can follow. + /// + /// Forced changes can only be applied after all pending standard changes + /// that it depends on have been applied. If any pending standard change + /// exists that is an ancestor of a given forced changed and which effective + /// block number is lower than the last finalized block (as defined by the + /// forced change), then the forced change cannot be applied. + /// + /// This method might apply multiple forced changes in one call (if any were + /// previously delayed by dependencies on pending standard changes). pub(crate) fn apply_forced_changes( &self, best_hash: H, @@ -348,46 +388,106 @@ where is_descendent_of: &F, initial_sync: bool, ) -> Result, E> - where F: Fn(&H, &H) -> Result, + where + F: Fn(&H, &H) -> Result, { - let mut new_set = None; + let mut state: Option<(H, N, Self)> = None; - for change in self.pending_forced_changes.iter() - .take_while(|c| c.effective_number() <= best_number) // to prevent iterating too far - .filter(|c| c.effective_number() == best_number) - { + let apply_change = |set_id, change: &PendingChange| { // check if the given best block is in the same branch as // the block that signaled the change. if change.canon_hash == best_hash || is_descendent_of(&change.canon_hash, &best_hash)? { + let median_last_finalized = match change.delay_kind { + DelayKind::Best { + ref median_last_finalized, + } => median_last_finalized.clone(), + _ => unreachable!( + "pending_forced_changes only contains forced changes; forced changes have delay kind Best; qed." + ), + }; + + // check if there's any pending standard change that we depend on + for (_, _, standard_change) in self.pending_standard_changes.roots() { + if standard_change.effective_number() <= median_last_finalized + && is_descendent_of(&standard_change.canon_hash, &change.canon_hash)? + { + log::info!(target: "afg", + "Not applying authority set changed forced at block #{:?}, due to pending standard change at block #{:?}", + change.canon_height, + standard_change.effective_number(), + ); + + return Ok(None); + } + } + // apply this change: make the set canonical - afg_log!(initial_sync, + afg_log!( + initial_sync, "👴 Applying authority set change forced at block #{:?}", change.canon_height, ); - telemetry!(CONSENSUS_INFO; "afg.applying_forced_authority_set_change"; + + telemetry!( + CONSENSUS_INFO; + "afg.applying_forced_authority_set_change"; "block" => ?change.canon_height ); - let median_last_finalized = match change.delay_kind { - DelayKind::Best { ref median_last_finalized } => median_last_finalized.clone(), - _ => unreachable!("pending_forced_changes only contains forced changes; forced changes have delay kind Best; qed."), - }; + return Ok(Some(( + change.canon_hash.clone(), + median_last_finalized, + AuthoritySet { + current_authorities: change.next_authorities.clone(), + set_id: set_id + 1, + pending_standard_changes: ForkTree::new(), // new set, new changes. + pending_forced_changes: Vec::new(), + }, + ))); + } - new_set = Some((median_last_finalized, AuthoritySet { - current_authorities: change.next_authorities.clone(), - set_id: self.set_id + 1, - pending_standard_changes: ForkTree::new(), // new set, new changes. - pending_forced_changes: Vec::new(), - })); + // nothing to apply + Ok(None) + }; - break; - } + for change in &self.pending_forced_changes { + // we are still looking for any viable forced changes + if change.effective_number() <= best_number { + // we might have already found a change to apply, + // in which case we need to its set_id + let set_id = if let Some((_, _, ref new_set)) = state { + new_set.set_id + } else { + self.set_id + }; - // we don't wipe forced changes until another change is - // applied + let new_state = apply_change(set_id, &change)?; + if new_state.is_some() { + state = new_state; + } + } else { + // we already found a change to apply previously, now we + // need to figure out if there's any remaining changes we + // should keep + if let Some((ref applied_change_canon_hash, _, ref mut new_set)) = state { + // keep any further changes that are descendents of the one we previously + // found. + if is_descendent_of(applied_change_canon_hash, &change.canon_hash)? { + new_set.pending_forced_changes.push(change.clone()); + } + } else { + // there are no more viable changes and we didn't apply + // any change previously so there's nothing left to do. + break; + } + } } - Ok(new_set) + // we don't wipe forced changes until another change is applied, hence + // why we return a new set instead of mutating. + Ok(state.map(|(_, median_last_finalized, authority_set)| { + (median_last_finalized, authority_set) + })) } /// Apply or prune any pending transitions based on a finality trigger. This @@ -406,7 +506,8 @@ where finalized_number: N, is_descendent_of: &F, initial_sync: bool, - ) -> Result, Error> where + ) -> Result, Error> + where F: Fn(&H, &H) -> Result, E: std::error::Error, { @@ -429,12 +530,11 @@ where Vec::new(), ); - // we will keep all forced change for any later blocks and that are a - // descendent of the finalized block (i.e. they are from this fork). + // we will keep all forced changes for any later blocks and that are a + // descendent of the finalized block (i.e. they are part of this branch). for change in pending_forced_changes { if change.effective_number() > finalized_number && - is_descendent_of(&finalized_hash, &change.canon_hash) - .map_err(fork_tree::Error::Client)? + is_descendent_of(&finalized_hash, &change.canon_hash)? { self.pending_forced_changes.push(change) } From 9b5279878d39b6b787a3c3876aa7770b309c8334 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 13 Oct 2020 00:41:38 +0100 Subject: [PATCH 2/5] grandpa: add test for early enactment of forced changes --- client/finality-grandpa/src/authorities.rs | 225 +++++++++++++++++---- 1 file changed, 184 insertions(+), 41 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 483e4932c704d..bf60cb0948073 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -1028,7 +1028,13 @@ mod tests { }; authorities.add_pending_change(change_a, &static_is_descendent_of(false)).unwrap(); - authorities.add_pending_change(change_b, &static_is_descendent_of(false)).unwrap(); + authorities.add_pending_change(change_b.clone(), &static_is_descendent_of(false)).unwrap(); + + // no duplicates are allowed + assert!(matches!( + authorities.add_pending_change(change_b, &static_is_descendent_of(false)), + Err(Error::DuplicateAuthoritySetChange) + )); // there's an effective change triggered at block 15 but not a standard one. // so this should do nothing. @@ -1037,12 +1043,9 @@ mod tests { None, ); - // throw a standard change into the mix to prove that it's discarded - // for being on the same fork. - // - // NOTE: after https://github.com/paritytech/substrate/issues/1861 - // this should still be rejected based on the "span" rule -- it overlaps - // with another change on the same fork. + let is_descendent_of_a = is_descendent_of(|base: &&str, _| base.starts_with("hash_a")); + + // we can add multiple forced changes on the same fork, but they must not overlap. let change_c = PendingChange { next_authorities: set_b.clone(), delay: 3, @@ -1051,37 +1054,46 @@ mod tests { delay_kind: DelayKind::Best { median_last_finalized: 0 }, }; - let is_descendent_of_a = is_descendent_of(|base: &&str, _| { - base.starts_with("hash_a") - }); - - assert!(authorities.add_pending_change(change_c, &is_descendent_of_a).is_err()); + assert!(matches!( + authorities.add_pending_change(change_c, &is_descendent_of_a), + Err(Error::OverlappingAuthoritySetChange) + )); - // too early. + // let's try and apply the forced changes. + // too early and there's no forced changes to apply. assert!( - authorities.apply_forced_changes("hash_a10", 10, &static_is_descendent_of(true), false) + authorities + .apply_forced_changes("hash_a10", 10, &static_is_descendent_of(true), false) .unwrap() .is_none() ); - // too late. - assert!( - authorities.apply_forced_changes("hash_a16", 16, &static_is_descendent_of(true), false) + let expected = ( + 42, + AuthoritySet { + current_authorities: set_a, + set_id: 1, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }, + ); + + // on time -- chooses the right change for this fork. + assert_eq!( + authorities + .apply_forced_changes("hash_a15", 15, &is_descendent_of_a, false) .unwrap() - .is_none() + .unwrap(), + expected, ); - // on time -- chooses the right change. + // also works on any later child block if it wasn't previously applied. assert_eq!( - authorities.apply_forced_changes("hash_a15", 15, &is_descendent_of_a, false) + authorities + .apply_forced_changes("hash_a16", 16, &is_descendent_of_a, false) .unwrap() .unwrap(), - (42, AuthoritySet { - current_authorities: set_a, - set_id: 1, - pending_standard_changes: ForkTree::new(), - pending_forced_changes: Vec::new(), - }), + expected, ); } @@ -1122,6 +1134,152 @@ mod tests { ); } + #[test] + fn forced_changes_blocked_by_standard_changes() { + let set_a = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; + + let mut authorities = AuthoritySet { + current_authorities: set_a.clone(), + set_id: 0, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }; + + // effective at #15 + let change_a = PendingChange { + next_authorities: set_a.clone(), + delay: 5, + canon_height: 10, + canon_hash: "hash_a", + delay_kind: DelayKind::Finalized, + }; + + // effective #20 + let change_b = PendingChange { + next_authorities: set_a.clone(), + delay: 0, + canon_height: 20, + canon_hash: "hash_b", + delay_kind: DelayKind::Finalized, + }; + + // effective at #35 + let change_c = PendingChange { + next_authorities: set_a.clone(), + delay: 5, + canon_height: 30, + canon_hash: "hash_c", + delay_kind: DelayKind::Finalized, + }; + + // add some pending standard changes all on the same fork + authorities.add_pending_change(change_a, &static_is_descendent_of(true)).unwrap(); + authorities.add_pending_change(change_b, &static_is_descendent_of(true)).unwrap(); + authorities.add_pending_change(change_c, &static_is_descendent_of(true)).unwrap(); + + // effective at #45 + let change_d = PendingChange { + next_authorities: set_a.clone(), + delay: 5, + canon_height: 40, + canon_hash: "hash_d", + delay_kind: DelayKind::Best { + median_last_finalized: 31, + }, + }; + + // effective at #55 + let change_e = PendingChange { + next_authorities: set_a.clone(), + delay: 5, + canon_height: 50, + canon_hash: "hash_e", + delay_kind: DelayKind::Best { + median_last_finalized: 31, + }, + }; + + // effective at #65 + let change_f = PendingChange { + next_authorities: set_a.clone(), + delay: 5, + canon_height: 60, + canon_hash: "hash_f", + delay_kind: DelayKind::Best { + median_last_finalized: 31, + }, + }; + + // now we add multiple forced changes on the same fork + authorities.add_pending_change(change_d, &static_is_descendent_of(true)).unwrap(); + authorities.add_pending_change(change_e.clone(), &static_is_descendent_of(true)).unwrap(); + authorities.add_pending_change(change_f.clone(), &static_is_descendent_of(true)).unwrap(); + + // the forced change cannot be applied since the pending changes it depends on + // have not been applied yet. + assert!( + authorities + .apply_forced_changes("hash_d45", 45, &static_is_descendent_of(true), false) + .unwrap() + .is_none() + ); + + // we apply the first pending standard change at #15 + authorities + .apply_standard_changes("hash_a15", 15, &static_is_descendent_of(true), false) + .unwrap(); + + // but the forced change still depends on the next standard change + assert!( + authorities + .apply_forced_changes("hash_d", 45, &static_is_descendent_of(true), false) + .unwrap() + .is_none() + ); + + // we apply the pending standard change at #20 + authorities + .apply_standard_changes("hash_b", 20, &static_is_descendent_of(true), false) + .unwrap(); + + // afterwards the forced change at #45 can already be applied since it signals + // that finality stalled at #31, and the next pending standard change is effective + // at #35. subsequent forced changes on the same branch must be kept + assert_eq!( + authorities + .apply_forced_changes("hash_d", 45, &static_is_descendent_of(true), false) + .unwrap() + .unwrap(), + ( + 31, + AuthoritySet { + current_authorities: set_a.clone(), + set_id: 3, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: vec![change_e, change_f.clone()], + } + ), + ); + + // it's also possible that multiple pending forced changes are applied at once, + // in case they were previously delayed due to any pending standard changes. + assert_eq!( + authorities + .apply_forced_changes("hash_e56", 56, &static_is_descendent_of(true), false) + .unwrap() + .unwrap(), + ( + 31, + AuthoritySet { + current_authorities: set_a, + set_id: 4, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: vec![change_f], + } + ), + ); + } + #[test] fn next_change_works() { let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; @@ -1378,26 +1536,11 @@ mod tests { add_pending_change(15, "C3", true); add_pending_change(20, "D", true); - println!( - "pending_changes: {:?}", - authorities - .pending_changes() - .map(|c| c.canon_hash) - .collect::>() - ); - // applying the standard change at A should not prune anything // other then the change that was applied authorities .apply_standard_changes("A", 5, &is_descendent_of, false) .unwrap(); - println!( - "pending_changes: {:?}", - authorities - .pending_changes() - .map(|c| c.canon_hash) - .collect::>() - ); assert_eq!(authorities.pending_changes().count(), 6); From 8a2acdb6f5e0239dc9ea52503beab1e6250525a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 13 Oct 2020 01:36:37 +0100 Subject: [PATCH 3/5] grandpa: fix typo in log message --- client/finality-grandpa/src/authorities.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index bf60cb0948073..b61c8ee183e9d 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -412,7 +412,7 @@ where && is_descendent_of(&standard_change.canon_hash, &change.canon_hash)? { log::info!(target: "afg", - "Not applying authority set changed forced at block #{:?}, due to pending standard change at block #{:?}", + "Not applying authority set change forced at block #{:?}, due to pending standard change at block #{:?}", change.canon_height, standard_change.effective_number(), ); From fa02c7946a31f34fb4532974704de49b596b50fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 14 Oct 2020 11:08:54 +0100 Subject: [PATCH 4/5] grandpa: only allow one pending forced change per fork --- client/finality-grandpa/src/authorities.rs | 135 +++++++++------------ 1 file changed, 57 insertions(+), 78 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index b61c8ee183e9d..bc8a2cc7953f1 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -33,21 +33,27 @@ use std::sync::Arc; /// Error type returned on operations on the `AuthoritySet`. #[derive(Debug, derive_more::Display)] -pub enum Error { - #[display("Invalid authority set, either empty or with an authority weight set to 0.")] +pub enum Error { + #[display(fmt = "Invalid authority set, either empty or with an authority weight set to 0.")] InvalidAuthoritySet, - #[display("Client error during ancestry lookup: {}", _0)] + #[display(fmt = "Client error during ancestry lookup: {}", _0)] Client(E), - #[display("Duplicate authority set change.")] + #[display(fmt = "Duplicate authority set change.")] DuplicateAuthoritySetChange, - #[display("Authority set change overlaps with existing change.")] - OverlappingAuthoritySetChange, + #[display(fmt = "Multiple pending forced authority set changes are not allowed.")] + MultiplePendingForcedAuthoritySetChanges, + #[display( + fmt = "A pending forced authority set change could not be applied since it must be applied after \ + the pending standard change at #{}", + _0 + )] + ForcedAuthoritySetChangeDependencyUnsatisfied(N), #[display(fmt = "Invalid operation in the pending changes tree: {}", _0)] ForkTree(fork_tree::Error), } -impl From> for Error { - fn from(err: fork_tree::Error) -> Error { +impl From> for Error { + fn from(err: fork_tree::Error) -> Error { match err { fork_tree::Error::Client(err) => Error::Client(err), fork_tree::Error::Duplicate => Error::DuplicateAuthoritySetChange, @@ -56,8 +62,8 @@ impl From> for Error { } } -impl From for Error { - fn from(err: E) -> Error { +impl From for Error { + fn from(err: E) -> Error { Error::Client(err) } } @@ -132,13 +138,14 @@ pub(crate) struct AuthoritySet { /// enacted on finality and must be enacted (i.e. finalized) in-order across /// a given branch pub(crate) pending_standard_changes: ForkTree>, - /// Pending forced changes across different forks. Forced changes are - /// enacted on block depth (not finality) and must be applied in-order - /// across a given branch. When trying to apply forced changes we keep track - /// of any pending standard changes that they may depend on, this is done by - /// making sure that any pending change that is an ancestor of the forced - /// changed and its effective block number is lower than the last finalized - /// block (as signaled in the forced change) must be applied beforehand. + /// Pending forced changes across different forks (at most one per fork). + /// Forced changes are enacted on block depth (not finality), for this + /// reason only one forced change should exist per fork. When trying to + /// apply forced changes we keep track of any pending standard changes that + /// they may depend on, this is done by making sure that any pending change + /// that is an ancestor of the forced changed and its effective block number + /// is lower than the last finalized block (as signaled in the forced + /// change) must be applied beforehand. pending_forced_changes: Vec>, } @@ -207,7 +214,7 @@ where &self, best_hash: &H, is_descendent_of: &F, - ) -> Result, Error> + ) -> Result, Error> where F: Fn(&H, &H) -> Result, E: std::error::Error, @@ -246,7 +253,8 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), Error> where + ) -> Result<(), Error> + where F: Fn(&H, &H) -> Result, E: std::error::Error, { @@ -277,7 +285,7 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), Error> + ) -> Result<(), Error> where F: Fn(&H, &H) -> Result, E: std::error::Error, @@ -287,10 +295,8 @@ where return Err(Error::DuplicateAuthoritySetChange); } - if change.effective_number() >= pending.canon_height - && is_descendent_of(&change.canon_hash, &pending.canon_hash)? - { - return Err(Error::OverlappingAuthoritySetChange); + if is_descendent_of(&change.canon_hash, &pending.canon_hash)? { + return Err(Error::MultiplePendingForcedAuthoritySetChanges); } } @@ -324,7 +330,8 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), Error> where + ) -> Result<(), Error> + where F: Fn(&H, &H) -> Result, E: std::error::Error, { @@ -377,23 +384,27 @@ where /// that it depends on have been applied. If any pending standard change /// exists that is an ancestor of a given forced changed and which effective /// block number is lower than the last finalized block (as defined by the - /// forced change), then the forced change cannot be applied. - /// - /// This method might apply multiple forced changes in one call (if any were - /// previously delayed by dependencies on pending standard changes). + /// forced change), then the forced change cannot be applied. An error will + /// be returned in that case which will prevent block import. pub(crate) fn apply_forced_changes( &self, best_hash: H, best_number: N, is_descendent_of: &F, initial_sync: bool, - ) -> Result, E> + ) -> Result, Error> where F: Fn(&H, &H) -> Result, + E: std::error::Error, { - let mut state: Option<(H, N, Self)> = None; - - let apply_change = |set_id, change: &PendingChange| { + let mut new_set = None; + + for change in self + .pending_forced_changes + .iter() + .take_while(|c| c.effective_number() <= best_number) // to prevent iterating too far + .filter(|c| c.effective_number() == best_number) + { // check if the given best block is in the same branch as // the block that signaled the change. if change.canon_hash == best_hash || is_descendent_of(&change.canon_hash, &best_hash)? { @@ -417,7 +428,11 @@ where standard_change.effective_number(), ); - return Ok(None); + return Err( + Error::ForcedAuthoritySetChangeDependencyUnsatisfied( + standard_change.effective_number() + ) + ); } } @@ -434,60 +449,23 @@ where "block" => ?change.canon_height ); - return Ok(Some(( - change.canon_hash.clone(), + new_set = Some(( median_last_finalized, AuthoritySet { current_authorities: change.next_authorities.clone(), - set_id: set_id + 1, + set_id: self.set_id + 1, pending_standard_changes: ForkTree::new(), // new set, new changes. pending_forced_changes: Vec::new(), }, - ))); - } - - // nothing to apply - Ok(None) - }; - - for change in &self.pending_forced_changes { - // we are still looking for any viable forced changes - if change.effective_number() <= best_number { - // we might have already found a change to apply, - // in which case we need to its set_id - let set_id = if let Some((_, _, ref new_set)) = state { - new_set.set_id - } else { - self.set_id - }; + )); - let new_state = apply_change(set_id, &change)?; - if new_state.is_some() { - state = new_state; - } - } else { - // we already found a change to apply previously, now we - // need to figure out if there's any remaining changes we - // should keep - if let Some((ref applied_change_canon_hash, _, ref mut new_set)) = state { - // keep any further changes that are descendents of the one we previously - // found. - if is_descendent_of(applied_change_canon_hash, &change.canon_hash)? { - new_set.pending_forced_changes.push(change.clone()); - } - } else { - // there are no more viable changes and we didn't apply - // any change previously so there's nothing left to do. - break; - } + break; } } // we don't wipe forced changes until another change is applied, hence // why we return a new set instead of mutating. - Ok(state.map(|(_, median_last_finalized, authority_set)| { - (median_last_finalized, authority_set) - })) + Ok(new_set) } /// Apply or prune any pending transitions based on a finality trigger. This @@ -506,7 +484,7 @@ where finalized_number: N, is_descendent_of: &F, initial_sync: bool, - ) -> Result, Error> + ) -> Result, Error> where F: Fn(&H, &H) -> Result, E: std::error::Error, @@ -579,7 +557,8 @@ where finalized_hash: H, finalized_number: N, is_descendent_of: &F, - ) -> Result, Error> where + ) -> Result, Error> + where F: Fn(&H, &H) -> Result, E: std::error::Error, { From 23a90e1279921bd3f60b5bb08f312e9e9c1d2a80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 14 Oct 2020 11:44:42 +0100 Subject: [PATCH 5/5] grandpa: fix tests --- client/finality-grandpa/src/authorities.rs | 107 ++++++--------------- 1 file changed, 29 insertions(+), 78 deletions(-) diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index bc8a2cc7953f1..f263fcc101ceb 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -1022,9 +1022,7 @@ mod tests { None, ); - let is_descendent_of_a = is_descendent_of(|base: &&str, _| base.starts_with("hash_a")); - - // we can add multiple forced changes on the same fork, but they must not overlap. + // there can only be one pending forced change per fork let change_c = PendingChange { next_authorities: set_b.clone(), delay: 3, @@ -1033,9 +1031,11 @@ mod tests { delay_kind: DelayKind::Best { median_last_finalized: 0 }, }; + let is_descendent_of_a = is_descendent_of(|base: &&str, _| base.starts_with("hash_a")); + assert!(matches!( authorities.add_pending_change(change_c, &is_descendent_of_a), - Err(Error::OverlappingAuthoritySetChange) + Err(Error::MultiplePendingForcedAuthoritySetChanges) )); // let's try and apply the forced changes. @@ -1047,32 +1047,29 @@ mod tests { .is_none() ); - let expected = ( - 42, - AuthoritySet { - current_authorities: set_a, - set_id: 1, - pending_standard_changes: ForkTree::new(), - pending_forced_changes: Vec::new(), - }, - ); - - // on time -- chooses the right change for this fork. - assert_eq!( + // too late. + assert!( authorities - .apply_forced_changes("hash_a15", 15, &is_descendent_of_a, false) + .apply_forced_changes("hash_a16", 16, &is_descendent_of_a, false) .unwrap() - .unwrap(), - expected, + .is_none() ); - // also works on any later child block if it wasn't previously applied. + // on time -- chooses the right change for this fork. assert_eq!( authorities - .apply_forced_changes("hash_a16", 16, &is_descendent_of_a, false) + .apply_forced_changes("hash_a15", 15, &is_descendent_of_a, false) .unwrap() .unwrap(), - expected, + ( + 42, + AuthoritySet { + current_authorities: set_a, + set_id: 1, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }, + ) ); } @@ -1167,41 +1164,15 @@ mod tests { }, }; - // effective at #55 - let change_e = PendingChange { - next_authorities: set_a.clone(), - delay: 5, - canon_height: 50, - canon_hash: "hash_e", - delay_kind: DelayKind::Best { - median_last_finalized: 31, - }, - }; - - // effective at #65 - let change_f = PendingChange { - next_authorities: set_a.clone(), - delay: 5, - canon_height: 60, - canon_hash: "hash_f", - delay_kind: DelayKind::Best { - median_last_finalized: 31, - }, - }; - - // now we add multiple forced changes on the same fork + // now add a forced change on the same fork authorities.add_pending_change(change_d, &static_is_descendent_of(true)).unwrap(); - authorities.add_pending_change(change_e.clone(), &static_is_descendent_of(true)).unwrap(); - authorities.add_pending_change(change_f.clone(), &static_is_descendent_of(true)).unwrap(); // the forced change cannot be applied since the pending changes it depends on // have not been applied yet. - assert!( - authorities - .apply_forced_changes("hash_d45", 45, &static_is_descendent_of(true), false) - .unwrap() - .is_none() - ); + assert!(matches!( + authorities.apply_forced_changes("hash_d45", 45, &static_is_descendent_of(true), false), + Err(Error::ForcedAuthoritySetChangeDependencyUnsatisfied(15)) + )); // we apply the first pending standard change at #15 authorities @@ -1209,12 +1180,10 @@ mod tests { .unwrap(); // but the forced change still depends on the next standard change - assert!( - authorities - .apply_forced_changes("hash_d", 45, &static_is_descendent_of(true), false) - .unwrap() - .is_none() - ); + assert!(matches!( + authorities.apply_forced_changes("hash_d", 45, &static_is_descendent_of(true), false), + Err(Error::ForcedAuthoritySetChangeDependencyUnsatisfied(20)) + )); // we apply the pending standard change at #20 authorities @@ -1235,25 +1204,7 @@ mod tests { current_authorities: set_a.clone(), set_id: 3, pending_standard_changes: ForkTree::new(), - pending_forced_changes: vec![change_e, change_f.clone()], - } - ), - ); - - // it's also possible that multiple pending forced changes are applied at once, - // in case they were previously delayed due to any pending standard changes. - assert_eq!( - authorities - .apply_forced_changes("hash_e56", 56, &static_is_descendent_of(true), false) - .unwrap() - .unwrap(), - ( - 31, - AuthoritySet { - current_authorities: set_a, - set_id: 4, - pending_standard_changes: ForkTree::new(), - pending_forced_changes: vec![change_f], + pending_forced_changes: Vec::new(), } ), );