diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 7a064d7a6224b..f263fcc101ceb 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -32,14 +32,42 @@ use std::ops::Add; use std::sync::Arc; /// Error type returned on operations on the `AuthoritySet`. -#[derive(Debug, derive_more::Display, derive_more::From)] -pub enum Error { - #[display("Invalid authority set, either empty or with an authority weight set to 0.")] +#[derive(Debug, derive_more::Display)] +pub enum Error { + #[display(fmt = "Invalid authority set, either empty or with an authority weight set to 0.")] InvalidAuthoritySet, + #[display(fmt = "Client error during ancestry lookup: {}", _0)] + Client(E), + #[display(fmt = "Duplicate authority set change.")] + DuplicateAuthoritySetChange, + #[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 { + 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>>, @@ -111,14 +139,20 @@ pub(crate) struct AuthoritySet { /// 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. + /// 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>, } 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 +214,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, @@ -219,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, { @@ -250,16 +285,18 @@ 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)? - { - return Err(fork_tree::Error::UnfinalizedAncestor.into()); + for change in &self.pending_forced_changes { + if change.canon_hash == pending.canon_hash { + return Err(Error::DuplicateAuthoritySetChange); + } + + if is_descendent_of(&change.canon_hash, &pending.canon_hash)? { + return Err(Error::MultiplePendingForcedAuthoritySetChanges); } } @@ -293,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, { @@ -341,52 +379,92 @@ 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. 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> - where F: Fn(&H, &H) -> Result, + ) -> Result, Error> + where + F: Fn(&H, &H) -> Result, + E: std::error::Error, { let mut new_set = None; - for change in self.pending_forced_changes.iter() + 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)? { + 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 change forced at block #{:?}, due to pending standard change at block #{:?}", + change.canon_height, + standard_change.effective_number(), + ); + + return Err( + Error::ForcedAuthoritySetChangeDependencyUnsatisfied( + standard_change.effective_number() + ) + ); + } + } + // 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."), - }; - - 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(), - })); + 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(), + }, + )); break; } - - // we don't wipe forced changes until another change is - // applied } + // we don't wipe forced changes until another change is applied, hence + // why we return a new set instead of mutating. Ok(new_set) } @@ -406,7 +484,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 +508,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) } @@ -479,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, { @@ -928,7 +1007,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. @@ -937,12 +1022,7 @@ 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. + // there can only be one pending forced change per fork let change_c = PendingChange { next_authorities: set_b.clone(), delay: 3, @@ -951,37 +1031,45 @@ 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") - }); + 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::MultiplePendingForcedAuthoritySetChanges) + )); - // 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) + authorities + .apply_forced_changes("hash_a16", 16, &is_descendent_of_a, false) .unwrap() .is_none() ); - // on time -- chooses the right change. + // on time -- chooses the right change for this fork. assert_eq!( - authorities.apply_forced_changes("hash_a15", 15, &is_descendent_of_a, false) + authorities + .apply_forced_changes("hash_a15", 15, &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(), - }), + ( + 42, + AuthoritySet { + current_authorities: set_a, + set_id: 1, + pending_standard_changes: ForkTree::new(), + pending_forced_changes: Vec::new(), + }, + ) ); } @@ -1022,6 +1110,106 @@ 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, + }, + }; + + // now add a forced change on the same fork + authorities.add_pending_change(change_d, &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!(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 + .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!(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 + .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::new(), + } + ), + ); + } + #[test] fn next_change_works() { let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; @@ -1278,26 +1466,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);