diff --git a/substrate/core/consensus/common/src/select_chain.rs b/substrate/core/consensus/common/src/select_chain.rs index cae28656a1..e696db6b87 100644 --- a/substrate/core/consensus/common/src/select_chain.rs +++ b/substrate/core/consensus/common/src/select_chain.rs @@ -42,8 +42,9 @@ pub trait SelectChain: Sync + Send + Clone { /// best chain to author new blocks upon and probably finalize. fn best_chain(&self) -> Result<::Header, Error>; - /// Get the best ancestor of `target_hash` that we should attempt - /// to finalize next. + /// Get the best descendent of `target_hash` that we should attempt to + /// finalize next, if any. It is valid to return the given `target_hash` + /// itself if no better descendent exists. fn finality_target( &self, target_hash: ::Hash, diff --git a/substrate/core/finality-grandpa/src/environment.rs b/substrate/core/finality-grandpa/src/environment.rs index 70e45848be..ee146c4608 100644 --- a/substrate/core/finality-grandpa/src/environment.rs +++ b/substrate/core/finality-grandpa/src/environment.rs @@ -52,6 +52,7 @@ use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::consensus_changes::SharedConsensusChanges; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; +use crate::voting_rule::VotingRule; use fg_primitives::{AuthorityId, AuthoritySignature, SetId, RoundNumber}; type HistoricalVotes = grandpa::HistoricalVotes< @@ -368,7 +369,7 @@ impl SharedVoterSetState { } /// The environment we run GRANDPA in. -pub(crate) struct Environment, RA, SC> { +pub(crate) struct Environment, RA, SC, VR> { pub(crate) inner: Arc>, pub(crate) select_chain: SC, pub(crate) voters: Arc>, @@ -378,9 +379,10 @@ pub(crate) struct Environment, RA, SC> { pub(crate) network: crate::communication::NetworkBridge, pub(crate) set_id: SetId, pub(crate) voter_set_state: SharedVoterSetState, + pub(crate) voting_rule: VR, } -impl, RA, SC> Environment { +impl, RA, SC, VR> Environment { /// Updates the voter set state using the given closure. The write lock is /// held during evaluation of the closure and the environment's voter set /// state is set to its result if successful. @@ -396,16 +398,18 @@ impl, RA, SC> Environment, B, E, N, RA, SC> +impl, B, E, N, RA, SC, VR> grandpa::Chain> -for Environment +for Environment where Block: 'static, B: Backend + 'static, - E: CallExecutor + 'static, + E: CallExecutor + Send + Sync + 'static, N: Network + 'static, N::In: 'static, SC: SelectChain + 'static, + VR: VotingRule>, + RA: Send + Sync, NumberFor: BlockNumberOps, { fn ancestry(&self, base: Block::Hash, block: Block::Hash) -> Result, GrandpaError> { @@ -429,7 +433,7 @@ where debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit); match self.select_chain.finality_target(block, None) { - Ok(Some(mut best_hash)) => { + Ok(Some(best_hash)) => { let base_header = self.inner.header(&BlockId::Hash(block)).ok()? .expect("Header known to exist after `best_containing` call; qed"); @@ -445,35 +449,51 @@ where } } - let mut best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()? + let best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()? .expect("Header known to exist after `best_containing` call; qed"); - // we target a vote towards 3/4 of the unfinalized chain (rounding up) - let target = { - let two = NumberFor::::one() + One::one(); - let three = two + One::one(); - let four = three + One::one(); + // check if our vote is currently being limited due to a pending change + let limit = limit.filter(|limit| limit < best_header.number()); + let target; - let diff = *best_header.number() - *base_header.number(); - let diff = ((diff * three) + two) / four; + let target_header = if let Some(target_number) = limit { + let mut target_header = best_header.clone(); - *base_header.number() + diff - }; + // walk backwards until we find the target block + loop { + if *target_header.number() < target_number { + unreachable!( + "we are traversing backwards from a known block; \ + blocks are stored contiguously; \ + qed" + ); + } - // unless our vote is currently being limited due to a pending change - let target = limit.map(|limit| limit.min(target)).unwrap_or(target); + if *target_header.number() == target_number { + break; + } - // walk backwards until we find the target block - loop { - if *best_header.number() < target { unreachable!(); } - if *best_header.number() == target { - return Some((best_hash, *best_header.number())); + target_header = self.inner.header(&BlockId::Hash(*target_header.parent_hash())).ok()? + .expect("Header known to exist after `best_containing` call; qed"); } - best_hash = *best_header.parent_hash(); - best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()? - .expect("Header known to exist after `best_containing` call; qed"); - } + target = target_header; + &target + } else { + // otherwise just use the given best as the target + &best_header + }; + + // restrict vote according to the given voting rule, if the + // voting rule doesn't restrict the vote then we keep the + // previous target. + // + // note that we pass the original `best_header`, i.e. before the + // authority set limit filter, which can be considered a + // mandatory/implicit voting rule. + self.voting_rule + .restrict_vote(&*self.inner, &base_header, &best_header, target_header) + .or(Some((target_header.hash(), *target_header.number()))) }, Ok(None) => { debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block); @@ -519,9 +539,9 @@ pub(crate) fn ancestry, E, RA>( Ok(tree_route.retracted().iter().skip(1).map(|e| e.hash).collect()) } -impl, N, RA, SC> +impl, N, RA, SC, VR> voter::Environment> -for Environment +for Environment where Block: 'static, B: Backend + 'static, @@ -530,6 +550,7 @@ where N::In: 'static + Send, RA: 'static + Send + Sync, SC: SelectChain + 'static, + VR: VotingRule>, NumberFor: BlockNumberOps, { type Timer = Box + Send>; diff --git a/substrate/core/finality-grandpa/src/lib.rs b/substrate/core/finality-grandpa/src/lib.rs index e4b71acbec..3841084d11 100644 --- a/substrate/core/finality-grandpa/src/lib.rs +++ b/substrate/core/finality-grandpa/src/lib.rs @@ -92,11 +92,15 @@ mod justification; mod light_import; mod observer; mod until_imported; +mod voting_rule; pub use communication::Network; pub use finality_proof::FinalityProofProvider; pub use light_import::light_block_import; pub use observer::run_grandpa_observer; +pub use voting_rule::{ + BeforeBestBlock, ThreeQuartersOfTheUnfinalizedChain, VotingRule, VotingRulesBuilder +}; use aux_schema::PersistentData; use environment::{Environment, VoterSetState}; @@ -466,7 +470,7 @@ fn register_finality_tracker_inherent_data_provider, N, RA, SC, X> { +pub struct GrandpaParams, N, RA, SC, VR, X> { /// Configuration for the GRANDPA service. pub config: Config, /// A link to the block import worker. @@ -479,12 +483,14 @@ pub struct GrandpaParams, N, RA, SC, X> { pub on_exit: X, /// If supplied, can be used to hook on telemetry connection established events. pub telemetry_on_connect: Option>, + /// A voting rule used to potentially restrict target votes. + pub voting_rule: VR, } /// Run a GRANDPA voter as a task. Provide configuration and a link to a /// block import worker that has already been instantiated with `block_import`. -pub fn run_grandpa_voter, N, RA, SC, X>( - grandpa_params: GrandpaParams, +pub fn run_grandpa_voter, N, RA, SC, VR, X>( + grandpa_params: GrandpaParams, ) -> client::error::Result + Send + 'static> where Block::Hash: Ord, B: Backend + 'static, @@ -492,6 +498,7 @@ pub fn run_grandpa_voter, N, RA, SC, X>( N: Network + Send + Sync + 'static, N::In: Send + 'static, SC: SelectChain + 'static, + VR: VotingRule> + Clone + 'static, NumberFor: BlockNumberOps, DigestFor: Encode, RA: Send + Sync + 'static, @@ -504,6 +511,7 @@ pub fn run_grandpa_voter, N, RA, SC, X>( inherent_data_providers, on_exit, telemetry_on_connect, + voting_rule, } = grandpa_params; let LinkHalf { @@ -556,8 +564,9 @@ pub fn run_grandpa_voter, N, RA, SC, X>( config, network, select_chain, + voting_rule, persistent_data, - voter_commands_rx + voter_commands_rx, ); let voter_work = voter_work @@ -578,13 +587,13 @@ pub fn run_grandpa_voter, N, RA, SC, X>( /// Future that powers the voter. #[must_use] -struct VoterWork, RA, SC> { +struct VoterWork, RA, SC, VR> { voter: Box>> + Send>, - env: Arc>, + env: Arc>, voter_commands_rx: mpsc::UnboundedReceiver>>, } -impl VoterWork +impl VoterWork where Block: BlockT, N: Network + Sync, @@ -594,12 +603,14 @@ where E: CallExecutor + Send + Sync + 'static, B: Backend + 'static, SC: SelectChain + 'static, + VR: VotingRule> + Clone + 'static, { fn new( client: Arc>, config: Config, network: NetworkBridge, select_chain: SC, + voting_rule: VR, persistent_data: PersistentData, voter_commands_rx: mpsc::UnboundedReceiver>>, ) -> Self { @@ -608,6 +619,7 @@ where let env = Arc::new(Environment { inner: client, select_chain, + voting_rule, voters: Arc::new(voters), config, network, @@ -731,6 +743,7 @@ where authority_set: self.env.authority_set.clone(), consensus_changes: self.env.consensus_changes.clone(), network: self.env.network.clone(), + voting_rule: self.env.voting_rule.clone(), }); self.rebuild_voter(); @@ -755,7 +768,7 @@ where } } -impl Future for VoterWork +impl Future for VoterWork where Block: BlockT, N: Network + Sync, @@ -765,6 +778,7 @@ where E: CallExecutor + Send + Sync + 'static, B: Backend + 'static, SC: SelectChain + 'static, + VR: VotingRule> + Clone + 'static, { type Item = (); type Error = Error; @@ -809,8 +823,8 @@ where } #[deprecated(since = "1.1", note = "Please switch to run_grandpa_voter.")] -pub fn run_grandpa, N, RA, SC, X>( - grandpa_params: GrandpaParams, +pub fn run_grandpa, N, RA, SC, VR, X>( + grandpa_params: GrandpaParams, ) -> ::client::error::Result + Send + 'static> where Block::Hash: Ord, B: Backend + 'static, @@ -821,6 +835,7 @@ pub fn run_grandpa, N, RA, SC, X>( NumberFor: BlockNumberOps, DigestFor: Encode, RA: Send + Sync + 'static, + VR: VotingRule> + Clone + 'static, X: Future + Clone + Send + 'static, { run_grandpa_voter(grandpa_params) diff --git a/substrate/core/finality-grandpa/src/tests.rs b/substrate/core/finality-grandpa/src/tests.rs index 1957ab5c4f..7f4a0d053a 100644 --- a/substrate/core/finality-grandpa/src/tests.rs +++ b/substrate/core/finality-grandpa/src/tests.rs @@ -385,6 +385,7 @@ fn run_to_completion_with( inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, + voting_rule: (), }; let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); @@ -516,6 +517,7 @@ fn finalize_3_voters_1_full_observer() { inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, + voting_rule: (), }; let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); @@ -676,6 +678,7 @@ fn transition_3_voters_twice_1_full_observer() { inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, + voting_rule: (), }; let voter = run_grandpa_voter(grandpa_params).expect("all in order with client and network"); @@ -861,30 +864,6 @@ fn finalizes_multiple_pending_changes_in_order() { run_to_completion(&mut runtime, 30, net.clone(), all_peers); } -#[test] -fn doesnt_vote_on_the_tip_of_the_chain() { - let mut runtime = current_thread::Runtime::new().unwrap(); - let peers_a = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; - let voters = make_ids(peers_a); - let api = TestApi::new(voters); - let mut net = GrandpaTestNet::new(api, 3); - - // add 100 blocks - net.peer(0).push_blocks(100, false); - net.block_until_sync(&mut runtime); - - for i in 0..3 { - assert_eq!(net.peer(i).client().info().chain.best_number, 100, - "Peer #{} failed to sync", i); - } - - let net = Arc::new(Mutex::new(net)); - let highest = run_to_completion(&mut runtime, 75, net.clone(), peers_a); - - // the highest block to be finalized will be 3/4 deep in the unfinalized chain - assert_eq!(highest, 75); -} - #[test] fn force_change_to_new_set() { let _ = env_logger::try_init(); @@ -1122,6 +1101,7 @@ fn voter_persists_its_votes() { inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, + voting_rule: VotingRulesBuilder::default().build(), }; let voter = run_grandpa_voter(grandpa_params) @@ -1452,6 +1432,7 @@ fn voter_catches_up_to_latest_round_when_behind() { inherent_data_providers: InherentDataProviders::new(), on_exit: Exit, telemetry_on_connect: None, + voting_rule: (), }; Box::new(run_grandpa_voter(grandpa_params).expect("all in order with client and network")) @@ -1533,3 +1514,116 @@ fn voter_catches_up_to_latest_round_when_behind() { let drive_to_completion = futures::future::poll_fn(|| { net.lock().poll(); Ok(Async::NotReady) }); let _ = runtime.block_on(test.select(drive_to_completion).map_err(|_| ())).unwrap(); } + +#[test] +fn grandpa_environment_respects_voting_rules() { + use grandpa::Chain; + use network::test::TestClient; + + let peers = &[Ed25519Keyring::Alice]; + let voters = make_ids(peers); + + let mut net = GrandpaTestNet::new(TestApi::new(voters), 1); + let peer = net.peer(0); + let network_service = peer.network_service().clone(); + let link = peer.data.lock().take().unwrap(); + + // create a voter environment with a given voting rule + let environment = |voting_rule: Box>| { + let PersistentData { + ref authority_set, + ref consensus_changes, + ref set_state, + .. + } = link.persistent_data; + + let config = Config { + gossip_duration: TEST_GOSSIP_DURATION, + justification_period: 32, + keystore: None, + name: None, + }; + + let (network, _) = NetworkBridge::new( + network_service.clone(), + config.clone(), + set_state.clone(), + Exit, + true, + ); + + Environment { + authority_set: authority_set.clone(), + config: config.clone(), + consensus_changes: consensus_changes.clone(), + inner: link.client.clone(), + select_chain: link.select_chain.clone(), + set_id: authority_set.set_id(), + voter_set_state: set_state.clone(), + voters: Arc::new(authority_set.current_authorities()), + network, + voting_rule, + } + }; + + // add 20 blocks + peer.push_blocks(20, false); + + // create an environment with no voting rule restrictions + let unrestricted_env = environment(Box::new(())); + + // another with 3/4 unfinalized chain voting rule restriction + let three_quarters_env = environment(Box::new( + voting_rule::ThreeQuartersOfTheUnfinalizedChain + )); + + // and another restricted with the default voting rules: i.e. 3/4 rule and + // always below best block + let default_env = environment(Box::new( + VotingRulesBuilder::default().build() + )); + + // the unrestricted environment should just return the best block + assert_eq!( + unrestricted_env.best_chain_containing( + peer.client().info().chain.finalized_hash + ).unwrap().1, + 20, + ); + + // both the other environments should return block 15, which is 3/4 of the + // way in the unfinalized chain + assert_eq!( + three_quarters_env.best_chain_containing( + peer.client().info().chain.finalized_hash + ).unwrap().1, + 15, + ); + + assert_eq!( + default_env.best_chain_containing( + peer.client().info().chain.finalized_hash + ).unwrap().1, + 15, + ); + + // we finalize block 19 with block 20 being the best block + peer.client().finalize_block(BlockId::Number(19), None, false).unwrap(); + + // the 3/4 environment should propose block 20 for voting + assert_eq!( + three_quarters_env.best_chain_containing( + peer.client().info().chain.finalized_hash + ).unwrap().1, + 20, + ); + + // while the default environment will always still make sure we don't vote + // on the best block + assert_eq!( + default_env.best_chain_containing( + peer.client().info().chain.finalized_hash + ).unwrap().1, + 19, + ); +} diff --git a/substrate/core/finality-grandpa/src/voting_rule.rs b/substrate/core/finality-grandpa/src/voting_rule.rs new file mode 100644 index 0000000000..355fa0cd2d --- /dev/null +++ b/substrate/core/finality-grandpa/src/voting_rule.rs @@ -0,0 +1,270 @@ +// Copyright 2018-2019 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Handling custom voting rules for GRANDPA. +//! +//! This exposes the `VotingRule` trait used to implement arbitrary voting +//! restrictions that are taken into account by the GRANDPA environment when +//! selecting a finality target to vote on. + +use std::sync::Arc; + +use client::blockchain::HeaderBackend; +use sr_primitives::generic::BlockId; +use sr_primitives::traits::{Block as BlockT, Header, NumberFor, One, Zero}; + +/// A trait for custom voting rules in GRANDPA. +pub trait VotingRule: Send + Sync where + Block: BlockT, + B: HeaderBackend, +{ + /// Restrict the given `current_target` vote, returning the block hash and + /// number of the block to vote on, and `None` in case the vote should not + /// be restricted. `base` is the block that we're basing our votes on in + /// order to pick our target (e.g. last round estimate), and `best_target` + /// is the initial best vote target before any vote rules were applied. When + /// applying multiple `VotingRule`s both `base` and `best_target` should + /// remain unchanged. + /// + /// The contract of this interface requires that when restricting a vote, the + /// returned value **must** be an ancestor of the given `current_target`, + /// this also means that a variant must be maintained throughout the + /// execution of voting rules wherein `current_target <= best_target`. + fn restrict_vote( + &self, + backend: &B, + base: &Block::Header, + best_target: &Block::Header, + current_target: &Block::Header, + ) -> Option<(Block::Hash, NumberFor)>; +} + +impl VotingRule for () where + Block: BlockT, + B: HeaderBackend, +{ + fn restrict_vote( + &self, + _backend: &B, + _base: &Block::Header, + _best_target: &Block::Header, + _current_target: &Block::Header, + ) -> Option<(Block::Hash, NumberFor)> { + None + } +} + +/// A custom voting rule that guarantees that our vote is always behind the best +/// block, in the best case exactly one block behind it. +#[derive(Clone)] +pub struct BeforeBestBlock; +impl VotingRule for BeforeBestBlock where + Block: BlockT, + B: HeaderBackend, +{ + fn restrict_vote( + &self, + _backend: &B, + _base: &Block::Header, + best_target: &Block::Header, + current_target: &Block::Header, + ) -> Option<(Block::Hash, NumberFor)> { + if current_target.number().is_zero() { + return None; + } + + if current_target.number() == best_target.number() { + return Some(( + current_target.parent_hash().clone(), + *current_target.number() - One::one(), + )); + } + + None + } +} + +/// A custom voting rule that limits votes towards 3/4 of the unfinalized chain, +/// using the given `base` and `best_target` to figure where the 3/4 target +/// should fall. +pub struct ThreeQuartersOfTheUnfinalizedChain; + +impl VotingRule for ThreeQuartersOfTheUnfinalizedChain where + Block: BlockT, + B: HeaderBackend, +{ + fn restrict_vote( + &self, + backend: &B, + base: &Block::Header, + best_target: &Block::Header, + current_target: &Block::Header, + ) -> Option<(Block::Hash, NumberFor)> { + // target a vote towards 3/4 of the unfinalized chain (rounding up) + let target_number = { + let two = NumberFor::::one() + One::one(); + let three = two + One::one(); + let four = three + One::one(); + + let diff = *best_target.number() - *base.number(); + let diff = ((diff * three) + two) / four; + + *base.number() + diff + }; + + // our current target is already lower than this rule would restrict + if target_number >= *current_target.number() { + return None; + } + + let mut target_header = current_target.clone(); + let mut target_hash = current_target.hash(); + + // walk backwards until we find the target block + loop { + if *target_header.number() < target_number { + unreachable!( + "we are traversing backwards from a known block; \ + blocks are stored contiguously; \ + qed" + ); + } + if *target_header.number() == target_number { + return Some((target_hash, target_number)); + } + + target_hash = *target_header.parent_hash(); + target_header = backend.header(BlockId::Hash(target_hash)).ok()? + .expect("Header known to exist due to the existence of one of its descendents; qed"); + } + } +} + +struct VotingRules { + rules: Arc>>>, +} + +impl Clone for VotingRules { + fn clone(&self) -> Self { + VotingRules { + rules: self.rules.clone(), + } + } +} + +impl VotingRule for VotingRules where + Block: BlockT, + B: HeaderBackend, +{ + fn restrict_vote( + &self, + backend: &B, + base: &Block::Header, + best_target: &Block::Header, + current_target: &Block::Header, + ) -> Option<(Block::Hash, NumberFor)> { + let restricted_target = self.rules.iter().fold( + current_target.clone(), + |current_target, rule| { + rule.restrict_vote( + backend, + base, + best_target, + ¤t_target, + ) + .and_then(|(hash, _)| backend.header(BlockId::Hash(hash)).ok()) + .and_then(std::convert::identity) + .unwrap_or(current_target) + }, + ); + + let restricted_hash = restricted_target.hash(); + + if restricted_hash != current_target.hash() { + Some((restricted_hash, *restricted_target.number())) + } else { + None + } + } +} + +/// A builder of a composite voting rule that applies a set of rules to +/// progressively restrict the vote. +pub struct VotingRulesBuilder { + rules: Vec>>, +} + +impl Default for VotingRulesBuilder where + Block: BlockT, + B: HeaderBackend, +{ + fn default() -> Self { + VotingRulesBuilder::new() + .add(BeforeBestBlock) + .add(ThreeQuartersOfTheUnfinalizedChain) + } +} + +impl VotingRulesBuilder where + Block: BlockT, + B: HeaderBackend, +{ + /// Return a new voting rule builder using the given backend. + pub fn new() -> Self { + VotingRulesBuilder { + rules: Vec::new(), + } + } + + /// Add a new voting rule to the builder. + pub fn add(mut self, rule: R) -> Self where + R: VotingRule + 'static, + { + self.rules.push(Box::new(rule)); + self + } + + /// Add all given voting rules to the builder. + pub fn add_all(mut self, rules: I) -> Self where + I: IntoIterator>>, + { + self.rules.extend(rules); + self + } + + /// Return a new `VotingRule` that applies all of the previously added + /// voting rules in-order. + pub fn build(self) -> impl VotingRule + Clone { + VotingRules { + rules: Arc::new(self.rules), + } + } +} + +impl VotingRule for Box> where + Block: BlockT, + B: HeaderBackend, +{ + fn restrict_vote( + &self, + backend: &B, + base: &Block::Header, + best_target: &Block::Header, + current_target: &Block::Header, + ) -> Option<(Block::Hash, NumberFor)> { + (**self).restrict_vote(backend, base, best_target, current_target) + } +} diff --git a/substrate/node-template/src/service.rs b/substrate/node-template/src/service.rs index 64a453e298..7934355812 100644 --- a/substrate/node-template/src/service.rs +++ b/substrate/node-template/src/service.rs @@ -149,6 +149,7 @@ pub fn new_full(config: Configuration