From d38b63d9c0bcc0a6328aa3e9f046d03e5d534aed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 15 Jan 2020 17:57:50 +0000 Subject: [PATCH 1/2] grandpa: generic voting rule for backing off from best block --- Cargo.lock | 1 + client/finality-grandpa/Cargo.toml | 1 + client/finality-grandpa/src/lib.rs | 2 +- client/finality-grandpa/src/voting_rule.rs | 83 ++++++++++++++-------- 4 files changed, 57 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac34452e31516..c51b04e6bc952 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5482,6 +5482,7 @@ dependencies = [ "sc-telemetry 2.0.0", "serde_json 1.0.44 (registry+https://github.com/rust-lang/crates.io-index)", "sp-api 2.0.0", + "sp-arithmetic 2.0.0", "sp-blockchain 2.0.0", "sp-consensus 0.8.0", "sp-consensus-babe 0.8.0", diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index ac0501c3c31d9..46268ee44ab0d 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -13,6 +13,7 @@ log = "0.4.8" parking_lot = "0.9.0" rand = "0.7.2" parity-scale-codec = { version = "1.0.0", features = ["derive"] } +sp-arithmetic = { version = "2.0.0", path = "../../primitives/arithmetic" } sp-runtime = { version = "2.0.0", path = "../../primitives/runtime" } sp-consensus = { version = "0.8", path = "../../primitives/consensus/common" } sp-core = { version = "2.0.0", path = "../../primitives/core" } diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index dbecd9c9a4b7f..2d0edc14f7015 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -96,7 +96,7 @@ pub use justification::GrandpaJustification; pub use light_import::light_block_import; pub use observer::run_grandpa_observer; pub use voting_rule::{ - BeforeBestBlock, ThreeQuartersOfTheUnfinalizedChain, VotingRule, VotingRulesBuilder + BeforeBestBlockBy, ThreeQuartersOfTheUnfinalizedChain, VotingRule, VotingRulesBuilder }; use aux_schema::PersistentData; diff --git a/client/finality-grandpa/src/voting_rule.rs b/client/finality-grandpa/src/voting_rule.rs index 8ba52f30f7b0f..523a1b05cd488 100644 --- a/client/finality-grandpa/src/voting_rule.rs +++ b/client/finality-grandpa/src/voting_rule.rs @@ -70,30 +70,38 @@ impl VotingRule for () where /// 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 +pub struct BeforeBestBlockBy(N); +impl VotingRule for BeforeBestBlockBy> where Block: BlockT, B: HeaderBackend, { fn restrict_vote( &self, - _backend: &B, + backend: &B, _base: &Block::Header, best_target: &Block::Header, current_target: &Block::Header, ) -> Option<(Block::Hash, NumberFor)> { + use sp_arithmetic::traits::Saturating; + 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(), - )); + // find the target number restricted by this rule + let target_number = best_target.number().saturating_sub(self.0); + + // our current target is already lower than this rule would restrict + if target_number >= *current_target.number() { + return None; } - None + // find the block at the given target height + find_target( + backend, + target_number, + current_target, + ) } } @@ -130,26 +138,43 @@ impl VotingRule for ThreeQuartersOfTheUnfinalizedChain where 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"); + // find the block at the given target height + find_target( + backend, + target_number, + current_target, + ) + } +} + +// walk backwards until we find the target block +fn find_target( + backend: &B, + target_number: NumberFor, + current_header: &Block::Header, +) -> Option<(Block::Hash, NumberFor)> where + Block: BlockT, + B: HeaderBackend, +{ + let mut target_hash = current_header.hash(); + let mut target_header = current_header.clone(); + + 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"); } } @@ -213,7 +238,7 @@ impl Default for VotingRulesBuilder where { fn default() -> Self { VotingRulesBuilder::new() - .add(BeforeBestBlock) + .add(BeforeBestBlockBy(2.into())) .add(ThreeQuartersOfTheUnfinalizedChain) } } From 53cd7b836d0d4c65e4f27aede0d8cd496213385d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 15 Jan 2020 17:58:27 +0000 Subject: [PATCH 2/2] grandpa: fix tests --- client/finality-grandpa/src/tests.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 4ad08c8868f27..05b8f9689bbd5 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1694,8 +1694,8 @@ fn grandpa_environment_respects_voting_rules() { } }; - // add 20 blocks - peer.push_blocks(20, false); + // add 21 blocks + peer.push_blocks(21, false); // create an environment with no voting rule restrictions let unrestricted_env = environment(Box::new(())); @@ -1716,38 +1716,38 @@ fn grandpa_environment_respects_voting_rules() { unrestricted_env.best_chain_containing( peer.client().info().finalized_hash ).unwrap().1, - 20, + 21, ); - // both the other environments should return block 15, which is 3/4 of the + // both the other environments should return block 16, which is 3/4 of the // way in the unfinalized chain assert_eq!( three_quarters_env.best_chain_containing( peer.client().info().finalized_hash ).unwrap().1, - 15, + 16, ); assert_eq!( default_env.best_chain_containing( peer.client().info().finalized_hash ).unwrap().1, - 15, + 16, ); - // we finalize block 19 with block 20 being the best block + // we finalize block 19 with block 21 being the best block peer.client().finalize_block(BlockId::Number(19), None, false).unwrap(); - // the 3/4 environment should propose block 20 for voting + // the 3/4 environment should propose block 21 for voting assert_eq!( three_quarters_env.best_chain_containing( peer.client().info().finalized_hash ).unwrap().1, - 20, + 21, ); // while the default environment will always still make sure we don't vote - // on the best block + // on the best block (2 behind) assert_eq!( default_env.best_chain_containing( peer.client().info().finalized_hash @@ -1755,17 +1755,17 @@ fn grandpa_environment_respects_voting_rules() { 19, ); - // we finalize block 20 with block 20 being the best block - peer.client().finalize_block(BlockId::Number(20), None, false).unwrap(); + // we finalize block 21 with block 21 being the best block + peer.client().finalize_block(BlockId::Number(21), None, false).unwrap(); // even though the default environment will always try to not vote on the // best block, there's a hard rule that we can't cast any votes lower than - // the given base (#20). + // the given base (#21). assert_eq!( default_env.best_chain_containing( peer.client().info().finalized_hash ).unwrap().1, - 20, + 21, ); }