grandpa: generic voting rule for backing off from best block (#4635)

* grandpa: generic voting rule for backing off from best block

* grandpa: fix tests

substrate/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",

substrate/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" }

substrate/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;

substrate/client/finality-grandpa/src/tests.rs

@@ -1694,8 +1694,8 @@ fn grandpa_environment_respects_voting_rules() {
 		}
 	};
 
-	// add 20 blocks
+	// add 21 blocks
-	peer.push_blocks(20, false);
+	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
+	// we finalize block 21 with block 21 being the best block
-	peer.client().finalize_block(BlockId::Number(20), None, false).unwrap();
+	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,
 	);
 }
 

substrate/client/finality-grandpa/src/voting_rule.rs

@@ -70,30 +70,38 @@ impl<Block, B> VotingRule<Block, B> 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;
+pub struct BeforeBestBlockBy<N>(N);
-impl<Block, B> VotingRule<Block, B> for BeforeBestBlock where
+impl<Block, B> VotingRule<Block, B> for BeforeBestBlockBy<NumberFor<Block>> where
 	Block: BlockT,
 	B: HeaderBackend<Block>,
 {
 	fn restrict_vote(
 		&self,
-		_backend: &B,
+		backend: &B,
 		_base: &Block::Header,
 		best_target: &Block::Header,
 		current_target: &Block::Header,
 	) -> Option<(Block::Hash, NumberFor<Block>)> {
+		use sp_arithmetic::traits::Saturating;
+
 		if current_target.number().is_zero() {
 			return None;
 		}
 
-		if current_target.number() == best_target.number() {
+		// find the target number restricted by this rule
-			return Some((
+		let target_number = best_target.number().saturating_sub(self.0);
-				current_target.parent_hash().clone(),
+
-				*current_target.number() - One::one(),
+		// 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<Block, B> VotingRule<Block, B> for ThreeQuartersOfTheUnfinalizedChain where
 			return None;
 		}
 
-		let mut target_header = current_target.clone();
+		// find the block at the given target height
-		let mut target_hash = current_target.hash();
+		find_target(
+			backend,
+			target_number,
+			current_target,
+		)
+	}
+}
 
-		// walk backwards until we find the target block
+// walk backwards until we find the target block
-		loop {
+fn find_target<Block, B>(
-			if *target_header.number() < target_number {
+	backend: &B,
-				unreachable!(
+	target_number: NumberFor<Block>,
-					"we are traversing backwards from a known block; \
+	current_header: &Block::Header,
-					 blocks are stored contiguously; \
+) -> Option<(Block::Hash, NumberFor<Block>)> where
-					 qed"
+	Block: BlockT,
-				);
+	B: HeaderBackend<Block>,
-			}
+{
-			if *target_header.number() == target_number {
+	let mut target_hash = current_header.hash();
-				return Some((target_hash, target_number));
+	let mut target_header = current_header.clone();
-			}
 
-			target_hash = *target_header.parent_hash();
+	loop {
-			target_header = backend.header(BlockId::Hash(target_hash)).ok()?
+		if *target_header.number() < target_number {
-				.expect("Header known to exist due to the existence of one of its descendents; qed");
+			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<Block, B> Default for VotingRulesBuilder<Block, B> where
 {
 	fn default() -> Self {
 		VotingRulesBuilder::new()
-			.add(BeforeBestBlock)
+			.add(BeforeBestBlockBy(2.into()))
 			.add(ThreeQuartersOfTheUnfinalizedChain)
 	}
 }