diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index fb112d433b..f2296d442e 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -1346,10 +1346,11 @@ dependencies = [ [[package]] name = "finality-grandpa" -version = "0.11.2" +version = "0.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "024517816630be5204eba201e8d1d405042b1255a5e0e3f298b054fc24d59e1d" +checksum = "9d7907cc24468e29b5d3ea2097e78104492b6650c55f96af1f14e7915dc155ad" dependencies = [ + "either", "futures 0.3.4", "futures-timer 2.0.2", "log", @@ -6344,6 +6345,7 @@ name = "sc-finality-grandpa" version = "0.8.0-dev" dependencies = [ "assert_matches", + "derive_more", "env_logger 0.7.1", "finality-grandpa", "fork-tree", diff --git a/substrate/client/finality-grandpa/Cargo.toml b/substrate/client/finality-grandpa/Cargo.toml index a634595ed3..04c793b58e 100644 --- a/substrate/client/finality-grandpa/Cargo.toml +++ b/substrate/client/finality-grandpa/Cargo.toml @@ -14,6 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] +derive_more = "0.99.2" fork-tree = { version = "2.0.0-dev", path = "../../utils/fork-tree" } futures = "0.3.4" futures-timer = "3.0.1" @@ -41,11 +42,11 @@ sp-finality-tracker = { version = "2.0.0-dev", path = "../../primitives/finality sp-finality-grandpa = { version = "2.0.0-dev", path = "../../primitives/finality-grandpa" } prometheus-endpoint = { package = "substrate-prometheus-endpoint", path = "../../utils/prometheus", version = "0.8.0-dev"} sc-block-builder = { version = "0.8.0-dev", path = "../block-builder" } -finality-grandpa = { version = "0.11.2", features = ["derive-codec"] } +finality-grandpa = { version = "0.12.0", features = ["derive-codec"] } pin-project = "0.4.6" [dev-dependencies] -finality-grandpa = { version = "0.11.2", features = ["derive-codec", "test-helpers"] } +finality-grandpa = { version = "0.12.0", features = ["derive-codec", "test-helpers"] } sc-network = { version = "0.8.0-dev", path = "../network" } sc-network-test = { version = "0.8.0-dev", path = "../network/test" } sp-keyring = { version = "2.0.0-dev", path = "../../primitives/keyring" } diff --git a/substrate/client/finality-grandpa/src/authorities.rs b/substrate/client/finality-grandpa/src/authorities.rs index fe3f2dd19e..000c303af7 100644 --- a/substrate/client/finality-grandpa/src/authorities.rs +++ b/substrate/client/finality-grandpa/src/authorities.rs @@ -29,6 +29,15 @@ use std::fmt::Debug; 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.")] + InvalidAuthoritySet, + #[display(fmt = "Invalid operation in the pending changes tree: {}", _0)] + ForkTree(fork_tree::Error), +} + /// A shared authority set. pub(crate) struct SharedAuthoritySet { inner: Arc>>, @@ -64,7 +73,11 @@ where N: Add + Ord + Clone + Debug, /// Get the current authorities and their weights (for the current set ID). pub(crate) fn current_authorities(&self) -> VoterSet { - self.inner.read().current_authorities.iter().cloned().collect() + VoterSet::new(self.inner.read().current_authorities.iter().cloned()).expect( + "current_authorities is non-empty and weights are non-zero; \ + constructor and all mutating operations on `AuthoritySet` ensure this; \ + qed.", + ) } } @@ -88,7 +101,7 @@ pub(crate) struct Status { #[derive(Debug, Clone, Encode, Decode, PartialEq)] pub(crate) struct AuthoritySet { pub(crate) current_authorities: AuthorityList, - pub(crate) set_id: u64, + set_id: u64, // Tree of pending standard changes across forks. Standard changes are // enacted on finality and must be enacted (i.e. finalized) in-order across // a given branch @@ -96,21 +109,49 @@ pub(crate) struct AuthoritySet { // 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. - pub(crate) pending_forced_changes: Vec>, + pending_forced_changes: Vec>, } impl AuthoritySet 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 { + authorities.is_empty() || authorities.iter().any(|(_, w)| *w == 0) + } + /// Get a genesis set with given authorities. - pub(crate) fn genesis(initial: AuthorityList) -> Self { - AuthoritySet { + pub(crate) fn genesis(initial: AuthorityList) -> Option { + if Self::invalid_authority_list(&initial) { + return None; + } + + Some(AuthoritySet { current_authorities: initial, set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), + }) + } + + /// Create a new authority set. + pub(crate) fn new( + authorities: AuthorityList, + set_id: u64, + pending_standard_changes: ForkTree>, + pending_forced_changes: Vec>, + ) -> Option { + if Self::invalid_authority_list(&authorities) { + return None; } + + Some(AuthoritySet { + current_authorities: authorities, + set_id, + pending_standard_changes, + pending_forced_changes, + }) } /// Get the current set id and a reference to the current authority set. @@ -128,9 +169,9 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), fork_tree::Error> where + ) -> Result<(), Error> where F: Fn(&H, &H) -> Result, - E: std::error::Error, + E: std::error::Error, { let hash = pending.canon_hash.clone(); let number = pending.canon_height.clone(); @@ -159,15 +200,16 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), fork_tree::Error> where + ) -> Result<(), Error> where F: Fn(&H, &H) -> Result, - E: std::error::Error, + 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)? + is_descendent_of(&change.canon_hash, &pending.canon_hash) + .map_err(fork_tree::Error::Client)? { - return Err(fork_tree::Error::UnfinalizedAncestor); + return Err(fork_tree::Error::UnfinalizedAncestor.into()); } } @@ -201,10 +243,14 @@ where &mut self, pending: PendingChange, is_descendent_of: &F, - ) -> Result<(), fork_tree::Error> where + ) -> Result<(), Error> where F: Fn(&H, &H) -> Result, - E: std::error::Error, + E: std::error::Error, { + if Self::invalid_authority_list(&pending.next_authorities) { + return Err(Error::InvalidAuthoritySet); + } + match pending.delay_kind { DelayKind::Best { .. } => { self.add_forced_change(pending, is_descendent_of) @@ -309,9 +355,9 @@ where finalized_number: N, is_descendent_of: &F, initial_sync: bool, - ) -> Result, fork_tree::Error> - where F: Fn(&H, &H) -> Result, - E: std::error::Error, + ) -> Result, Error> where + F: Fn(&H, &H) -> Result, + E: std::error::Error, { let mut status = Status { changed: false, @@ -370,16 +416,16 @@ where finalized_hash: H, finalized_number: N, is_descendent_of: &F, - ) -> Result, fork_tree::Error> - where F: Fn(&H, &H) -> Result, - E: std::error::Error, + ) -> Result, Error> where + F: Fn(&H, &H) -> Result, + E: std::error::Error, { self.pending_standard_changes.finalizes_any_with_descendent_if( &finalized_hash, finalized_number.clone(), is_descendent_of, |change| change.effective_number() == finalized_number - ) + ).map_err(Error::ForkTree) } } @@ -457,8 +503,10 @@ mod tests { #[test] fn current_limit_filters_min() { + let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; + let mut authorities = AuthoritySet { - current_authorities: Vec::new(), + current_authorities: current_authorities.clone(), set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), @@ -466,7 +514,7 @@ mod tests { let change = |height| { PendingChange { - next_authorities: Vec::new(), + next_authorities: current_authorities.clone(), delay: 0, canon_height: height, canon_hash: height.to_string(), @@ -502,15 +550,17 @@ mod tests { #[test] fn changes_iterated_in_pre_order() { + let current_authorities = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; + let mut authorities = AuthoritySet { - current_authorities: Vec::new(), + current_authorities: current_authorities.clone(), set_id: 0, pending_standard_changes: ForkTree::new(), pending_forced_changes: Vec::new(), }; let change_a = PendingChange { - next_authorities: Vec::new(), + next_authorities: current_authorities.clone(), delay: 10, canon_height: 5, canon_hash: "hash_a", @@ -518,7 +568,7 @@ mod tests { }; let change_b = PendingChange { - next_authorities: Vec::new(), + next_authorities: current_authorities.clone(), delay: 0, canon_height: 5, canon_hash: "hash_b", @@ -526,7 +576,7 @@ mod tests { }; let change_c = PendingChange { - next_authorities: Vec::new(), + next_authorities: current_authorities.clone(), delay: 5, canon_height: 10, canon_hash: "hash_c", @@ -543,7 +593,7 @@ mod tests { // forced changes are iterated last let change_d = PendingChange { - next_authorities: Vec::new(), + next_authorities: current_authorities.clone(), delay: 2, canon_height: 1, canon_hash: "hash_d", @@ -551,7 +601,7 @@ mod tests { }; let change_e = PendingChange { - next_authorities: Vec::new(), + next_authorities: current_authorities.clone(), delay: 2, canon_height: 0, canon_hash: "hash_e", @@ -689,7 +739,7 @@ mod tests { // trying to finalize past `change_c` without finalizing `change_a` first assert!(matches!( authorities.apply_standard_changes("hash_d", 40, &is_descendent_of, false), - Err(fork_tree::Error::UnfinalizedAncestor) + Err(Error::ForkTree(fork_tree::Error::UnfinalizedAncestor)) )); let status = authorities.apply_standard_changes( @@ -871,4 +921,74 @@ mod tests { }), ); } + + #[test] + fn maintains_authority_list_invariants() { + // empty authority lists are invalid + assert_eq!(AuthoritySet::<(), ()>::genesis(vec![]), None); + assert_eq!( + AuthoritySet::<(), ()>::new(vec![], 0, ForkTree::new(), Vec::new()), + None, + ); + + let invalid_authorities_weight = vec![ + (AuthorityId::from_slice(&[1; 32]), 5), + (AuthorityId::from_slice(&[2; 32]), 0), + ]; + + // authority weight of zero is invalid + assert_eq!( + AuthoritySet::<(), ()>::genesis(invalid_authorities_weight.clone()), + None + ); + assert_eq!( + AuthoritySet::<(), ()>::new( + invalid_authorities_weight.clone(), + 0, + ForkTree::new(), + Vec::new() + ), + None, + ); + + let mut authority_set = + AuthoritySet::<(), u64>::genesis(vec![(AuthorityId::from_slice(&[1; 32]), 5)]).unwrap(); + + let invalid_change_empty_authorities = PendingChange { + next_authorities: vec![], + delay: 10, + canon_height: 5, + canon_hash: (), + delay_kind: DelayKind::Finalized, + }; + + // pending change contains an empty authority set + assert!(matches!( + authority_set.add_pending_change( + invalid_change_empty_authorities.clone(), + &static_is_descendent_of(false) + ), + Err(Error::InvalidAuthoritySet) + )); + + let invalid_change_authorities_weight = PendingChange { + next_authorities: invalid_authorities_weight, + delay: 10, + canon_height: 5, + canon_hash: (), + delay_kind: DelayKind::Best { + median_last_finalized: 0, + }, + }; + + // pending change contains an an authority set + // where one authority has weight of 0 + assert!(matches!( + authority_set.add_pending_change( + invalid_change_authorities_weight, + &static_is_descendent_of(false) + ), + Err(Error::InvalidAuthoritySet) + )); + } } diff --git a/substrate/client/finality-grandpa/src/aux_schema.rs b/substrate/client/finality-grandpa/src/aux_schema.rs index fe652f52fe..c217bc328a 100644 --- a/substrate/client/finality-grandpa/src/aux_schema.rs +++ b/substrate/client/finality-grandpa/src/aux_schema.rs @@ -97,12 +97,14 @@ where H: Clone + Debug + PartialEq, } } - AuthoritySet { - current_authorities: self.current_authorities, - set_id: self.set_id, - pending_forced_changes: Vec::new(), - pending_standard_changes - } + let authority_set = AuthoritySet::new( + self.current_authorities, + self.set_id, + pending_standard_changes, + Vec::new(), + ); + + authority_set.expect("current_authorities is non-empty and weights are non-zero; qed.") } } @@ -334,7 +336,8 @@ pub(crate) fn load_persistent( from genesis on what appears to be first startup."); let genesis_authorities = genesis_authorities()?; - let genesis_set = AuthoritySet::genesis(genesis_authorities.clone()); + let genesis_set = AuthoritySet::genesis(genesis_authorities.clone()) + .expect("genesis authorities is non-empty; all weights are non-zero; qed."); let state = make_genesis_round(); let base = state.prevote_ghost .expect("state is for completed round; completed rounds must have a prevote ghost; qed."); @@ -503,12 +506,12 @@ mod test { assert_eq!( *authority_set.inner().read(), - AuthoritySet { - current_authorities: authorities.clone(), - pending_standard_changes: ForkTree::new(), - pending_forced_changes: Vec::new(), + AuthoritySet::new( + authorities.clone(), set_id, - }, + ForkTree::new(), + Vec::new(), + ).unwrap(), ); let mut current_rounds = CurrentRounds::new(); @@ -547,12 +550,12 @@ mod test { }; { - let authority_set = AuthoritySet:: { - current_authorities: authorities.clone(), - pending_standard_changes: ForkTree::new(), - pending_forced_changes: Vec::new(), + let authority_set = AuthoritySet::::new( + authorities.clone(), set_id, - }; + ForkTree::new(), + Vec::new(), + ).unwrap(); let voter_set_state = V1VoterSetState::Live(round_number, round_state.clone()); @@ -593,12 +596,12 @@ mod test { assert_eq!( *authority_set.inner().read(), - AuthoritySet { - current_authorities: authorities.clone(), - pending_standard_changes: ForkTree::new(), - pending_forced_changes: Vec::new(), + AuthoritySet::new( + authorities.clone(), set_id, - }, + ForkTree::new(), + Vec::new(), + ).unwrap(), ); let mut current_rounds = CurrentRounds::new(); diff --git a/substrate/client/finality-grandpa/src/communication/gossip.rs b/substrate/client/finality-grandpa/src/communication/gossip.rs index 2d39ed7ec4..afa6581702 100644 --- a/substrate/client/finality-grandpa/src/communication/gossip.rs +++ b/substrate/client/finality-grandpa/src/communication/gossip.rs @@ -1618,7 +1618,10 @@ mod tests { use crate::environment::VoterSetState; let base = (H256::zero(), 0); - let voters = AuthoritySet::genesis(Vec::new()); + + let voters = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; + let voters = AuthoritySet::genesis(voters).unwrap(); + let set_state = VoterSetState::live( 0, &voters, diff --git a/substrate/client/finality-grandpa/src/communication/mod.rs b/substrate/client/finality-grandpa/src/communication/mod.rs index 7daa121513..c75fe251e7 100644 --- a/substrate/client/finality-grandpa/src/communication/mod.rs +++ b/substrate/client/finality-grandpa/src/communication/mod.rs @@ -258,7 +258,7 @@ impl> NetworkBridge { // is a no-op if currently in that set. self.validator.note_set( set_id, - voters.voters().iter().map(|(v, _)| v.clone()).collect(), + voters.iter().map(|(v, _)| v.clone()).collect(), |to, neighbor| self.neighbor_sender.send(to, neighbor), ); @@ -289,7 +289,7 @@ impl> NetworkBridge { let locals = local_key.and_then(|pair| { let id = pair.public(); - if voters.contains_key(&id) { + if voters.contains(&id) { Some((pair, id)) } else { None @@ -308,12 +308,12 @@ impl> NetworkBridge { } Ok(GossipMessage::Vote(msg)) => { // check signature. - if !voters.contains_key(&msg.message.id) { + if !voters.contains(&msg.message.id) { debug!(target: "afg", "Skipping message from unknown voter {}", msg.message.id); return future::ready(None); } - if voters.len() <= TELEMETRY_VOTERS_LIMIT { + if voters.len().get() <= TELEMETRY_VOTERS_LIMIT { match &msg.message.message { PrimaryPropose(propose) => { telemetry!(CONSENSUS_INFO; "afg.received_propose"; @@ -378,7 +378,7 @@ impl> NetworkBridge { ) { self.validator.note_set( set_id, - voters.voters().iter().map(|(v, _)| v.clone()).collect(), + voters.iter().map(|(v, _)| v.clone()).collect(), |to, neighbor| self.neighbor_sender.send(to, neighbor), ); @@ -476,7 +476,7 @@ fn incoming_global( gossip_validator: &Arc>, voters: &VoterSet, | { - if voters.len() <= TELEMETRY_VOTERS_LIMIT { + if voters.len().get() <= TELEMETRY_VOTERS_LIMIT { let precommits_signed_by: Vec = msg.message.auth_data.iter().map(move |(_, a)| { format!("{}", a) @@ -799,13 +799,13 @@ fn check_compact_commit( ) -> Result<(), ReputationChange> { // 4f + 1 = equivocations from f voters. let f = voters.total_weight() - voters.threshold(); - let full_threshold = voters.total_weight() + f; + let full_threshold = (f + voters.total_weight()).0; // check total weight is not out of range. let mut total_weight = 0; for (_, ref id) in &msg.auth_data { - if let Some(weight) = voters.info(id).map(|info| info.weight()) { - total_weight += weight; + if let Some(weight) = voters.get(id).map(|info| info.weight()) { + total_weight += weight.get(); if total_weight > full_threshold { return Err(cost::MALFORMED_COMMIT); } @@ -815,7 +815,7 @@ fn check_compact_commit( } } - if total_weight < voters.threshold() { + if total_weight < voters.threshold().get() { return Err(cost::MALFORMED_COMMIT); } @@ -860,7 +860,7 @@ fn check_catch_up( ) -> Result<(), ReputationChange> { // 4f + 1 = equivocations from f voters. let f = voters.total_weight() - voters.threshold(); - let full_threshold = voters.total_weight() + f; + let full_threshold = (f + voters.total_weight()).0; // check total weight is not out of range for a set of votes. fn check_weight<'a>( @@ -871,8 +871,8 @@ fn check_catch_up( let mut total_weight = 0; for id in votes { - if let Some(weight) = voters.info(&id).map(|info| info.weight()) { - total_weight += weight; + if let Some(weight) = voters.get(&id).map(|info| info.weight()) { + total_weight += weight.get(); if total_weight > full_threshold { return Err(cost::MALFORMED_CATCH_UP); } @@ -882,7 +882,7 @@ fn check_catch_up( } } - if total_weight < voters.threshold() { + if total_weight < voters.threshold().get() { return Err(cost::MALFORMED_CATCH_UP); } diff --git a/substrate/client/finality-grandpa/src/communication/tests.rs b/substrate/client/finality-grandpa/src/communication/tests.rs index ea995eff63..883fdb4f26 100644 --- a/substrate/client/finality-grandpa/src/communication/tests.rs +++ b/substrate/client/finality-grandpa/src/communication/tests.rs @@ -29,7 +29,7 @@ use std::{borrow::Cow, pin::Pin, task::{Context, Poll}}; use crate::environment::SharedVoterSetState; use sp_finality_grandpa::{AuthorityList, GRANDPA_ENGINE_ID}; use super::gossip::{self, GossipValidator}; -use super::{AuthorityId, VoterSet, Round, SetId}; +use super::{VoterSet, Round, SetId}; #[derive(Debug)] pub(crate) enum Event { @@ -142,11 +142,15 @@ fn voter_set_state() -> SharedVoterSetState { use crate::authorities::AuthoritySet; use crate::environment::VoterSetState; use finality_grandpa::round::State as RoundState; - use sp_core::H256; + use sp_core::{crypto::Public, H256}; + use sp_finality_grandpa::AuthorityId; let state = RoundState::genesis((H256::zero(), 0)); let base = state.prevote_ghost.unwrap(); - let voters = AuthoritySet::genesis(Vec::new()); + + let voters = vec![(AuthorityId::from_slice(&[1; 32]), 1)]; + let voters = AuthoritySet::genesis(voters).unwrap(); + let set_state = VoterSetState::live( 0, &voters, @@ -212,7 +216,7 @@ impl sc_network_gossip::ValidatorContext for NoopContext { fn good_commit_leads_to_relay() { let private = [Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let public = make_ids(&private[..]); - let voter_set = Arc::new(public.iter().cloned().collect::>()); + let voter_set = Arc::new(VoterSet::new(public.iter().cloned()).unwrap()); let round = 1; let set_id = 1; @@ -360,7 +364,7 @@ fn bad_commit_leads_to_report() { let _ = env_logger::try_init(); let private = [Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; let public = make_ids(&private[..]); - let voter_set = Arc::new(public.iter().cloned().collect::>()); + let voter_set = Arc::new(VoterSet::new(public.iter().cloned()).unwrap()); let round = 1; let set_id = 1; diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs index cab212333c..1c11c52468 100644 --- a/substrate/client/finality-grandpa/src/environment.rs +++ b/substrate/client/finality-grandpa/src/environment.rs @@ -579,23 +579,23 @@ where Block: 'static, B: Backend, C: crate::ClientForGrandpa + 'static, - N: NetworkT + 'static + Send, + N: NetworkT + 'static + Send + Sync, SC: SelectChain + 'static, VR: VotingRule, NumberFor: BlockNumberOps, { - type Timer = Pin> + Send>>; + type Timer = Pin> + Send + Sync>>; type Id = AuthorityId; type Signature = AuthoritySignature; // regular round message streams type In = Pin, Self::Signature, Self::Id>, Self::Error> - > + Send>>; + > + Send + Sync>>; type Out = Pin>, Error = Self::Error, - > + Send>>; + > + Send + Sync>>; type Error = CommandOrError>; diff --git a/substrate/client/finality-grandpa/src/finality_proof.rs b/substrate/client/finality-grandpa/src/finality_proof.rs index 0eef20d813..c52cc8fc90 100644 --- a/substrate/client/finality-grandpa/src/finality_proof.rs +++ b/substrate/client/finality-grandpa/src/finality_proof.rs @@ -54,6 +54,7 @@ use sc_telemetry::{telemetry, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GRANDPA_AUTHORITIES_KEY}; use crate::justification::GrandpaJustification; +use crate::VoterSet; /// Maximum number of fragments that we want to return in a single prove_finality call. const MAX_FRAGMENTS_IN_PROOF: usize = 8; @@ -588,7 +589,11 @@ impl ProvableJustification for GrandpaJustificatio NumberFor: BlockNumberOps, { fn verify(&self, set_id: u64, authorities: &[(AuthorityId, u64)]) -> ClientResult<()> { - GrandpaJustification::verify(self, set_id, &authorities.iter().cloned().collect()) + let authorities = VoterSet::new(authorities.iter().cloned()).ok_or( + ClientError::Consensus(sp_consensus::Error::InvalidAuthoritiesSet), + )?; + + GrandpaJustification::verify(self, set_id, &authorities) } } diff --git a/substrate/client/finality-grandpa/src/lib.rs b/substrate/client/finality-grandpa/src/lib.rs index 6fab89ac68..be00519f89 100644 --- a/substrate/client/finality-grandpa/src/lib.rs +++ b/substrate/client/finality-grandpa/src/lib.rs @@ -674,7 +674,7 @@ pub fn run_grandpa_voter( let events = telemetry_on_connect .for_each(move |_| { let curr = authorities.current_authorities(); - let mut auths = curr.voters().into_iter().map(|(p, _)| p); + let mut auths = curr.iter().map(|(p, _)| p); let maybe_authority_id = authority_id(&mut auths, &conf.keystore) .unwrap_or(Default::default()); @@ -682,8 +682,8 @@ pub fn run_grandpa_voter( "authority_id" => maybe_authority_id.to_string(), "authority_set_id" => ?authorities.set_id(), "authorities" => { - let authorities: Vec = curr.voters() - .iter().map(|(id, _)| id.to_string()).collect(); + let authorities: Vec = curr.iter() + .map(|(id, _)| id.to_string()).collect(); serde_json::to_string(&authorities) .expect("authorities is always at least an empty vector; elements are always of type string") } @@ -823,7 +823,7 @@ where "authority_id" => authority_id.to_string(), "authority_set_id" => ?self.env.set_id, "authorities" => { - let authorities: Vec = self.env.voters.voters() + let authorities: Vec = self.env.voters .iter().map(|(id, _)| id.to_string()).collect(); serde_json::to_string(&authorities) .expect("authorities is always at least an empty vector; elements are always of type string") @@ -894,8 +894,16 @@ where Ok(Some(set_state)) })?; + let voters = Arc::new(VoterSet::new(new.authorities.into_iter()) + .expect("new authorities come from pending change; \ + pending change comes from `AuthoritySet`; \ + `AuthoritySet` validates authorities is non-empty and weights are non-zero; \ + qed." + ) + ); + self.env = Arc::new(Environment { - voters: Arc::new(new.authorities.into_iter().collect()), + voters, set_id: new.set_id, voter_set_state: self.env.voter_set_state.clone(), // Fields below are simply transferred and not updated. @@ -1017,7 +1025,8 @@ fn is_voter( keystore: &Option, ) -> Option { match keystore { - Some(keystore) => voters.voters().iter() + Some(keystore) => voters + .iter() .find_map(|(p, _)| keystore.read().key_pair::(&p).ok()), None => None, } diff --git a/substrate/client/finality-grandpa/src/observer.rs b/substrate/client/finality-grandpa/src/observer.rs index 1e6c8ddf18..ac67675fef 100644 --- a/substrate/client/finality-grandpa/src/observer.rs +++ b/substrate/client/finality-grandpa/src/observer.rs @@ -409,11 +409,13 @@ mod tests { (Arc::new(client), backend) }; + let voters = vec![(sp_keyring::Ed25519Keyring::Alice.public().into(), 1)]; + let persistent_data = aux_schema::load_persistent( &*backend, client.info().genesis_hash, 0, - || Ok(vec![]), + || Ok(voters), ).unwrap(); let (_tx, voter_command_rx) = tracing_unbounded(""); diff --git a/substrate/client/finality-grandpa/src/tests.rs b/substrate/client/finality-grandpa/src/tests.rs index 2821737c4d..5d1c5b2c16 100644 --- a/substrate/client/finality-grandpa/src/tests.rs +++ b/substrate/client/finality-grandpa/src/tests.rs @@ -990,7 +990,6 @@ fn test_bad_justification() { #[test] fn voter_persists_its_votes() { - use std::iter::FromIterator; use std::sync::atomic::{AtomicUsize, Ordering}; use futures::future; use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver}; @@ -1145,7 +1144,7 @@ fn voter_persists_its_votes() { let (round_rx, round_tx) = network.round_communication( communication::Round(1), communication::SetId(0), - Arc::new(VoterSet::from_iter(voters)), + Arc::new(VoterSet::new(voters).unwrap()), Some(peers[1].pair().into()), HasVoted::No, ); diff --git a/substrate/client/finality-grandpa/src/until_imported.rs b/substrate/client/finality-grandpa/src/until_imported.rs index 40da7707b6..737c8c6a77 100644 --- a/substrate/client/finality-grandpa/src/until_imported.rs +++ b/substrate/client/finality-grandpa/src/until_imported.rs @@ -147,7 +147,7 @@ pub(crate) struct UntilImported, ready: VecDeque, /// Interval at which to check status of each awaited block. - check_pending: Pin> + Send>>, + check_pending: Pin> + Send + Sync>>, /// Mapping block hashes to their block number, the point in time it was /// first encountered (Instant) and a list of GRANDPA messages referencing /// the block hash.