Tunable Justification Generator (#835)

* Add way to create tunable GRANDPA justifications

* Use new function in tests

* Allow multiple authorities on a single fork

* Only store pre-commit targets instead of full ancestry chains

* Rename precommit_header to be more generic

* Push new digest item instead of overriding entire digest

* Ensure that we generate chains with non-zero length

* Extract justification creation parameters into struct

* Appease Clippy

bridges/modules/finality-verifier/src/lib.rs

@@ -522,7 +522,7 @@ mod tests {
 	use super::*;
 	use crate::mock::{run_test, test_header, Origin, TestHash, TestHeader, TestNumber, TestRuntime};
 	use bp_test_utils::{
-		authority_list, keyring, make_justification_for_header,
+		authority_list, make_default_justification, make_justification_for_header, JustificationGeneratorParams,
 		Keyring::{Alice, Bob},
 	};
 	use codec::Encode;
@@ -551,10 +551,7 @@ mod tests {
 
 	fn submit_finality_proof(header: u8) -> frame_support::dispatch::DispatchResultWithPostInfo {
 		let header = test_header(header.into());
-
+		let justification = make_default_justification(&header).encode();
-		let set_id = 1;
-		let grandpa_round = 1;
-		let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 		Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification)
 	}
@@ -726,9 +723,11 @@ mod tests {
 
 			let header = test_header(1);
 
-			let set_id = 2;
+			let params = JustificationGeneratorParams::<TestHeader> {
-			let grandpa_round = 1;
+				set_id: 2,
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
+				..Default::default()
+			};
+			let justification = make_justification_for_header(params).encode();
 
 			assert_err!(
 				Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification,),
@@ -802,9 +801,7 @@ mod tests {
 			header.digest = change_log(0);
 
 			// Create a valid justification for the header
-			let set_id = 1;
+			let justification = make_default_justification(&header).encode();
-			let grandpa_round = 1;
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 			// Let's import our test header
 			assert_ok!(Module::<TestRuntime>::submit_finality_proof(
@@ -836,9 +833,7 @@ mod tests {
 			header.digest = change_log(1);
 
 			// Create a valid justification for the header
-			let set_id = 1;
+			let justification = make_default_justification(&header).encode();
-			let grandpa_round = 1;
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 			// Should not be allowed to import this header
 			assert_err!(
@@ -859,9 +854,7 @@ mod tests {
 			header.digest = forced_change_log(0);
 
 			// Create a valid justification for the header
-			let set_id = 1;
+			let justification = make_default_justification(&header).encode();
-			let grandpa_round = 1;
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 			// Should not be allowed to import this header
 			assert_err!(

bridges/modules/substrate/src/fork_tests.rs

@@ -60,7 +60,7 @@ use crate::verifier::*;
 use crate::{BestFinalized, BestHeight, BridgeStorage, NextScheduledChange, PalletStorage};
 use bp_header_chain::AuthoritySet;
 use bp_test_utils::{
-	authority_list, keyring, make_justification_for_header,
+	authority_list, make_default_justification,
 	Keyring::{Alice, Bob},
 };
 use codec::Encode;
@@ -457,9 +457,7 @@ where
 				// `grandpa_round`).
 				//
 				// See for more: https://github.com/paritytech/parity-bridges-common/issues/430
-				let grandpa_round = 1;
+				let justification = make_default_justification(header).encode();
-				let set_id = 1;
-				let justification = make_justification_for_header(header, grandpa_round, set_id, &keyring()).encode();
 
 				let res = verifier
 					.import_finality_proof(header.hash(), justification.into())

bridges/modules/substrate/src/verifier.rs

@@ -343,7 +343,7 @@ mod tests {
 	use crate::mock::*;
 	use crate::{BestFinalized, BestHeight, HeaderId, ImportedHeaders, PalletStorage};
 	use bp_test_utils::{
-		authority_list, keyring, make_justification_for_header,
+		authority_list, make_default_justification,
 		Keyring::{Alice, Bob},
 	};
 	use codec::Encode;
@@ -605,8 +605,7 @@ mod tests {
 			assert_eq!(storage.best_headers().len(), 1);
 
 			// Now lets finalize our best header
-			let grandpa_round = 1;
+			let justification = make_default_justification(&header).encode();
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 			assert_ok!(verifier.import_finality_proof(header.hash(), justification.into()));
 
 			// Our best header should only appear once in the list of best headers
@@ -729,8 +728,7 @@ mod tests {
 			storage.update_current_authority_set(authority_set);
 
 			// We'll need this justification to finalize the header
-			let grandpa_round = 1;
+			let justification = make_default_justification(&header).encode();
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 			let mut verifier = Verifier {
 				storage: storage.clone(),
@@ -754,8 +752,7 @@ mod tests {
 			let authority_set = AuthoritySet { authorities, set_id };
 			storage.update_current_authority_set(authority_set);
 
-			let grandpa_round = 1;
+			let justification = make_default_justification(&header).encode();
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 			let mut verifier = Verifier {
 				storage: storage.clone(),
@@ -798,8 +795,7 @@ mod tests {
 			// This header enacts an authority set change upon finalization
 			let header = test_header(2);
 
-			let grandpa_round = 1;
+			let justification = make_default_justification(&header).encode();
-			let justification = make_justification_for_header(&header, grandpa_round, set_id, &keyring()).encode();
 
 			// Schedule a change at the height of our header
 			let set_id = 2;

bridges/primitives/header-chain/tests/justification.rs

@@ -16,14 +16,55 @@
 
 //! Tests for Grandpa Justification code.
 
-use bp_header_chain::justification::{verify_justification, Error, GrandpaJustification};
+use bp_header_chain::justification::{verify_justification, Error};
+use bp_test_utils::Keyring::*;
 use bp_test_utils::*;
 use codec::Encode;
 
 type TestHeader = sp_runtime::testing::Header;
 
-fn make_justification_for_header_1() -> GrandpaJustification<TestHeader> {
+#[test]
-	make_justification_for_header(&test_header(1), TEST_GRANDPA_ROUND, TEST_GRANDPA_SET_ID, &keyring())
+fn valid_justification_accepted() {
+	let params = JustificationGeneratorParams {
+		header: test_header(1),
+		round: TEST_GRANDPA_ROUND,
+		set_id: TEST_GRANDPA_SET_ID,
+		authorities: vec![(Alice, 1), (Bob, 1), (Charlie, 1), (Dave, 1), (Eve, 1)],
+		depth: 5,
+		forks: 5,
+	};
+
+	assert_eq!(
+		verify_justification::<TestHeader>(
+			header_id::<TestHeader>(1),
+			TEST_GRANDPA_SET_ID,
+			&voter_set(),
+			&make_justification_for_header::<TestHeader>(params).encode()
+		),
+		Ok(()),
+	);
+}
+
+#[test]
+fn valid_justification_accepted_with_single_fork() {
+	let params = JustificationGeneratorParams {
+		header: test_header(1),
+		round: TEST_GRANDPA_ROUND,
+		set_id: TEST_GRANDPA_SET_ID,
+		authorities: vec![(Alice, 1), (Bob, 1), (Charlie, 1), (Dave, 1), (Eve, 1)],
+		depth: 5,
+		forks: 1,
+	};
+
+	assert_eq!(
+		verify_justification::<TestHeader>(
+			header_id::<TestHeader>(1),
+			TEST_GRANDPA_SET_ID,
+			&voter_set(),
+			&make_justification_for_header::<TestHeader>(params).encode()
+		),
+		Ok(()),
+	);
 }
 
 #[test]
@@ -41,7 +82,7 @@ fn justification_with_invalid_target_rejected() {
 			header_id::<TestHeader>(2),
 			TEST_GRANDPA_SET_ID,
 			&voter_set(),
-			&make_justification_for_header_1().encode(),
+			&make_default_justification::<TestHeader>(&test_header(1)).encode(),
 		),
 		Err(Error::InvalidJustificationTarget),
 	);
@@ -49,7 +90,7 @@ fn justification_with_invalid_target_rejected() {
 
 #[test]
 fn justification_with_invalid_commit_rejected() {
-	let mut justification = make_justification_for_header_1();
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
 	justification.commit.precommits.clear();
 
 	assert_eq!(
@@ -65,7 +106,7 @@ fn justification_with_invalid_commit_rejected() {
 
 #[test]
 fn justification_with_invalid_authority_signature_rejected() {
-	let mut justification = make_justification_for_header_1();
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
 	justification.commit.precommits[0].signature = Default::default();
 
 	assert_eq!(
@@ -81,7 +122,7 @@ fn justification_with_invalid_authority_signature_rejected() {
 
 #[test]
 fn justification_with_invalid_precommit_ancestry() {
-	let mut justification = make_justification_for_header_1();
+	let mut justification = make_default_justification::<TestHeader>(&test_header(1));
 	justification.votes_ancestries.push(test_header(10));
 
 	assert_eq!(
@@ -96,14 +137,24 @@ fn justification_with_invalid_precommit_ancestry() {
 }
 
 #[test]
-fn valid_justification_accepted() {
+fn justification_is_invalid_if_we_dont_meet_threshold() {
+	// Need at least three authorities to sign off or else the voter set threshold can't be reached
+	let params = JustificationGeneratorParams {
+		header: test_header(1),
+		round: TEST_GRANDPA_ROUND,
+		set_id: TEST_GRANDPA_SET_ID,
+		authorities: vec![(Alice, 1), (Bob, 1)],
+		depth: 2,
+		forks: 2,
+	};
+
 	assert_eq!(
 		verify_justification::<TestHeader>(
 			header_id::<TestHeader>(1),
 			TEST_GRANDPA_SET_ID,
 			&voter_set(),
-			&make_justification_for_header_1().encode(),
+			&make_justification_for_header::<TestHeader>(params).encode()
 		),
-		Ok(()),
+		Err(Error::InvalidJustificationCommit),
 	);
 }

bridges/primitives/test-utils/src/lib.rs

@@ -31,29 +31,95 @@ use sp_std::prelude::*;
 pub const TEST_GRANDPA_ROUND: u64 = 1;
 pub const TEST_GRANDPA_SET_ID: SetId = 1;
 
-/// Get a valid Grandpa justification for a header given a Grandpa round, authority set ID, and
+/// Configuration parameters when generating test GRANDPA justifications.
-/// authority list.
+pub struct JustificationGeneratorParams<H> {
-pub fn make_justification_for_header<H: HeaderT>(
+	/// The header which we want to finalize.
-	header: &H,
+	pub header: H,
-	round: u64,
+	/// The GRANDPA round number for the current authority set.
-	set_id: SetId,
+	pub round: u64,
-	authorities: &[(Keyring, AuthorityWeight)],
+	/// The current authority set ID.
-) -> GrandpaJustification<H> {
+	pub set_id: SetId,
+	/// The current GRANDPA authority set.
+	pub authorities: Vec<(Keyring, AuthorityWeight)>,
+	/// The number of headers included in our justification's vote ancestries.
+	pub depth: u32,
+	/// The number of forks, and thus the number of pre-commits in our justification.
+	pub forks: u32,
+}
+
+impl<H: HeaderT> Default for JustificationGeneratorParams<H> {
+	fn default() -> Self {
+		Self {
+			header: test_header(One::one()),
+			round: TEST_GRANDPA_ROUND,
+			set_id: TEST_GRANDPA_SET_ID,
+			authorities: keyring(),
+			depth: 2,
+			forks: 1,
+		}
+	}
+}
+
+/// Make a valid GRANDPA justification with sensible defaults
+pub fn make_default_justification<H: HeaderT>(header: &H) -> GrandpaJustification<H> {
+	let params = JustificationGeneratorParams::<H> {
+		header: header.clone(),
+		..Default::default()
+	};
+
+	make_justification_for_header(params)
+}
+
+/// Generate justifications in a way where we are able to tune the number of pre-commits
+/// and vote ancestries which are included in the justification.
+///
+/// This is useful for benchmarkings where we want to generate valid justifications with
+/// a specific number of pre-commits (tuned with the "forks" parameter) and/or a specific
+/// number of vote ancestries (tuned with the "depth" parameter).
+///
+/// Note: This needs at least three authorities or else the verifier will complain about
+/// being given an invalid commit.
+pub fn make_justification_for_header<H: HeaderT>(params: JustificationGeneratorParams<H>) -> GrandpaJustification<H> {
+	let JustificationGeneratorParams {
+		header,
+		round,
+		set_id,
+		authorities,
+		depth,
+		forks,
+	} = params;
+
 	let (target_hash, target_number) = (header.hash(), *header.number());
 	let mut precommits = vec![];
 	let mut votes_ancestries = vec![];
 
-	// We want to make sure that the header included in the vote ancestries
+	assert!(depth != 0, "Can't have a chain of zero length.");
-	// is actually related to our target header
+	assert!(
-	let mut precommit_header = test_header::<H>(target_number + One::one());
+		forks as usize <= authorities.len(),
-	precommit_header.set_parent_hash(target_hash);
+		"If we have more forks than authorities we can't create valid pre-commits for all the forks."
+	);
+
+	let mut unsigned_precommits = vec![];
+	for i in 0..forks {
+		let chain = generate_chain(i as u8, depth, &header);
+
+		// We don't include our finality target header in the vote ancestries
+		for child in &chain[1..] {
+			votes_ancestries.push(child.clone());
+		}
+
+		// The header we need to use when pre-commiting is the one at the highest height
+		// on our chain.
+		let precommit_candidate = chain.last().map(|h| (h.hash(), *h.number())).unwrap();
+		unsigned_precommits.push(precommit_candidate);
+	}
+
+	for (i, (id, _weight)) in authorities.iter().enumerate() {
+		// Assign authorities to sign pre-commits in a round-robin fashion
+		let target = unsigned_precommits[i % forks as usize];
+		let precommit = signed_precommit::<H>(&id, target, round, set_id);
 
-	// I'm using the same header for all the voters since it doesn't matter as long
-	// as they all vote on blocks _ahead_ of the one we're interested in finalizing
-	for (id, _weight) in authorities.iter() {
-		let precommit = signed_precommit::<H>(id, (precommit_header.hash(), *precommit_header.number()), round, set_id);
 		precommits.push(precommit);
-		votes_ancestries.push(precommit_header.clone());
 	}
 
 	GrandpaJustification {
@@ -67,6 +133,29 @@ pub fn make_justification_for_header<H: HeaderT>(
 	}
 }
 
+fn generate_chain<H: HeaderT>(fork_id: u8, depth: u32, ancestor: &H) -> Vec<H> {
+	let mut headers = vec![ancestor.clone()];
+
+	for i in 1..depth {
+		let parent = &headers[(i - 1) as usize];
+		let (hash, num) = (parent.hash(), *parent.number());
+
+		let mut header = test_header::<H>(num + One::one());
+		header.set_parent_hash(hash);
+
+		// Modifying the digest so headers at the same height but in different forks have different
+		// hashes
+		header
+			.digest_mut()
+			.logs
+			.push(sp_runtime::DigestItem::Other(vec![fork_id]));
+
+		headers.push(header);
+	}
+
+	headers
+}
+
 fn signed_precommit<H: HeaderT>(
 	signer: &Keyring,
 	target: (H::Hash, H::Number),