Move justification code to primitives crate (#640)

* Move justification module to header-chain primitives crate

* Get justification module compiling in new location

* Get justification module tests compiling

* Use justification code from `header-chain` crate

Mostly compiles, having issues with std/test feature flags across crates.

* Move some code around

* Move justification tests to integration testing crate

* Add `test-utils` crate

* Remove tests and test-helper module from justification code

* Use `test-utils` in Substrate bridge pallet tests

* Remove `sp-keyring` related code from `pallet-substrate-bridge`

* Remove `helpers` module from `pallet-substrate-bridge`

* Add some documentation

* Add more documentation

* Fix typo

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

bridges/modules/substrate/src/fork_tests.rs

@@ -54,11 +54,11 @@
 //! Import a finality proof for header 2 on fork 1. This finalty proof should fail to be imported
 //! because the header is an old header.
 
-use crate::justification::tests::*;
-use crate::mock::{helpers::*, *};
+use crate::mock::*;
 use crate::storage::{AuthoritySet, ImportedHeader};
 use crate::verifier::*;
 use crate::{BestFinalized, BestHeight, BridgeStorage, NextScheduledChange, PalletStorage};
+use bp_test_utils::{alice, authority_list, bob, make_justification_for_header};
 use codec::Encode;
 use frame_support::{IterableStorageMap, StorageValue};
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
@@ -456,8 +456,7 @@ where
 				let grandpa_round = 1;
 				let set_id = 1;
 				let authorities = authority_list();
-				let justification =
-					make_justification_for_header(&header, grandpa_round, set_id, &authorities).encode();
+				let justification = make_justification_for_header(header, grandpa_round, set_id, &authorities).encode();
 
 				let res = verifier
 					.import_finality_proof(header.hash(), justification.into())

bridges/modules/substrate/src/justification.rs

@@ -1,335 +0,0 @@
-// Copyright 2019-2020 Parity Technologies (UK) Ltd.
-// This file is part of Parity Bridges Common.
-
-// Parity Bridges Common 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.
-
-// Parity Bridges Common 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 Parity Bridges Common.  If not, see <http://www.gnu.org/licenses/>.
-
-//! Module for checking GRANDPA Finality Proofs.
-//!
-//! Adapted copy of substrate/client/finality-grandpa/src/justification.rs. If origin
-//! will ever be moved to the sp_finality_grandpa, we should reuse that implementation.
-
-use codec::Decode;
-use finality_grandpa::{voter_set::VoterSet, Chain, Error as GrandpaError};
-use frame_support::RuntimeDebug;
-use sp_finality_grandpa::{AuthorityId, AuthoritySignature, SetId};
-use sp_runtime::traits::Header as HeaderT;
-use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet};
-use sp_std::prelude::Vec;
-
-/// Justification verification error.
-#[derive(RuntimeDebug, PartialEq)]
-pub enum Error {
-	/// Failed to decode justification.
-	JustificationDecode,
-	/// Justification is finalizing unexpected header.
-	InvalidJustificationTarget,
-	/// Invalid commit in justification.
-	InvalidJustificationCommit,
-	/// Justification has invalid authority singature.
-	InvalidAuthoritySignature,
-	/// The justification has precommit for the header that has no route from the target header.
-	InvalidPrecommitAncestryProof,
-	/// The justification has 'unused' headers in its precommit ancestries.
-	InvalidPrecommitAncestries,
-}
-
-/// Decode justification target.
-pub fn decode_justification_target<Header: HeaderT>(
-	raw_justification: &[u8],
-) -> Result<(Header::Hash, Header::Number), Error> {
-	GrandpaJustification::<Header>::decode(&mut &raw_justification[..])
-		.map(|justification| (justification.commit.target_hash, justification.commit.target_number))
-		.map_err(|_| Error::JustificationDecode)
-}
-
-/// Verify that justification, that is generated by given authority set, finalizes given header.
-pub fn verify_justification<Header: HeaderT>(
-	finalized_target: (Header::Hash, Header::Number),
-	authorities_set_id: SetId,
-	authorities_set: VoterSet<AuthorityId>,
-	raw_justification: &[u8],
-) -> Result<(), Error>
-where
-	Header::Number: finality_grandpa::BlockNumberOps,
-{
-	// decode justification first
-	let justification =
-		GrandpaJustification::<Header>::decode(&mut &raw_justification[..]).map_err(|_| Error::JustificationDecode)?;
-
-	// ensure that it is justification for the expected header
-	if (justification.commit.target_hash, justification.commit.target_number) != finalized_target {
-		return Err(Error::InvalidJustificationTarget);
-	}
-
-	// validate commit of the justification (it just assumes all signatures are valid)
-	let ancestry_chain = AncestryChain::new(&justification.votes_ancestries);
-	match finality_grandpa::validate_commit(&justification.commit, &authorities_set, &ancestry_chain) {
-		Ok(ref result) if result.ghost().is_some() => {}
-		_ => return Err(Error::InvalidJustificationCommit),
-	}
-
-	// now that we know that the commit is correct, check authorities signatures
-	let mut buf = Vec::new();
-	let mut visited_hashes = BTreeSet::new();
-	for signed in &justification.commit.precommits {
-		if !sp_finality_grandpa::check_message_signature_with_buffer(
-			&finality_grandpa::Message::Precommit(signed.precommit.clone()),
-			&signed.id,
-			&signed.signature,
-			justification.round,
-			authorities_set_id,
-			&mut buf,
-		) {
-			return Err(Error::InvalidAuthoritySignature);
-		}
-
-		if justification.commit.target_hash == signed.precommit.target_hash {
-			continue;
-		}
-
-		match ancestry_chain.ancestry(justification.commit.target_hash, signed.precommit.target_hash) {
-			Ok(route) => {
-				// ancestry starts from parent hash but the precommit target hash has been visited
-				visited_hashes.insert(signed.precommit.target_hash);
-				visited_hashes.extend(route);
-			}
-			_ => {
-				// could this happen in practice? I don't think so, but original code has this check
-				return Err(Error::InvalidPrecommitAncestryProof);
-			}
-		}
-	}
-
-	let ancestry_hashes = justification
-		.votes_ancestries
-		.iter()
-		.map(|h: &Header| h.hash())
-		.collect();
-	if visited_hashes != ancestry_hashes {
-		return Err(Error::InvalidPrecommitAncestries);
-	}
-
-	Ok(())
-}
-
-/// A GRANDPA Justification is a proof that a given header was finalized
-/// at a certain height and with a certain set of authorities.
-///
-/// This particular proof is used to prove that headers on a bridged chain
-/// (so not our chain) have been finalized correctly.
-#[derive(Decode, RuntimeDebug)]
-#[cfg_attr(test, derive(codec::Encode))]
-pub(crate) struct GrandpaJustification<Header: HeaderT> {
-	round: u64,
-	commit: finality_grandpa::Commit<Header::Hash, Header::Number, AuthoritySignature, AuthorityId>,
-	votes_ancestries: Vec<Header>,
-}
-
-/// A utility trait implementing `finality_grandpa::Chain` using a given set of headers.
-#[derive(RuntimeDebug)]
-struct AncestryChain<Header: HeaderT> {
-	ancestry: BTreeMap<Header::Hash, Header::Hash>,
-}
-
-impl<Header: HeaderT> AncestryChain<Header> {
-	fn new(ancestry: &[Header]) -> AncestryChain<Header> {
-		AncestryChain {
-			ancestry: ancestry
-				.iter()
-				.map(|header| (header.hash(), *header.parent_hash()))
-				.collect(),
-		}
-	}
-}
-
-impl<Header: HeaderT> finality_grandpa::Chain<Header::Hash, Header::Number> for AncestryChain<Header>
-where
-	Header::Number: finality_grandpa::BlockNumberOps,
-{
-	fn ancestry(&self, base: Header::Hash, block: Header::Hash) -> Result<Vec<Header::Hash>, GrandpaError> {
-		let mut route = Vec::new();
-		let mut current_hash = block;
-		loop {
-			if current_hash == base {
-				break;
-			}
-			match self.ancestry.get(&current_hash).cloned() {
-				Some(parent_hash) => {
-					current_hash = parent_hash;
-					route.push(current_hash);
-				}
-				_ => return Err(GrandpaError::NotDescendent),
-			}
-		}
-		route.pop(); // remove the base
-
-		Ok(route)
-	}
-
-	fn best_chain_containing(&self, _block: Header::Hash) -> Option<(Header::Hash, Header::Number)> {
-		unreachable!("is only used during voting; qed")
-	}
-}
-
-#[cfg(test)]
-pub(crate) mod tests {
-	use super::*;
-	use crate::mock::helpers::*;
-	use codec::Encode;
-	use sp_core::H256;
-	use sp_finality_grandpa::{AuthorityId, AuthorityWeight};
-	use sp_keyring::Ed25519Keyring;
-
-	const TEST_GRANDPA_ROUND: u64 = 1;
-	const TEST_GRANDPA_SET_ID: SetId = 1;
-
-	pub(crate) fn signed_precommit(
-		signer: Ed25519Keyring,
-		target: HeaderId,
-		round: u64,
-		set_id: SetId,
-	) -> finality_grandpa::SignedPrecommit<H256, u64, AuthoritySignature, AuthorityId> {
-		let precommit = finality_grandpa::Precommit {
-			target_hash: target.0,
-			target_number: target.1,
-		};
-		let encoded = sp_finality_grandpa::localized_payload(
-			round,
-			set_id,
-			&finality_grandpa::Message::Precommit(precommit.clone()),
-		);
-		let signature = signer.sign(&encoded[..]).into();
-		finality_grandpa::SignedPrecommit {
-			precommit,
-			signature,
-			id: signer.public().into(),
-		}
-	}
-
-	pub(crate) fn make_justification_for_header(
-		header: &TestHeader,
-		round: u64,
-		set_id: SetId,
-		authorities: &[(AuthorityId, AuthorityWeight)],
-	) -> GrandpaJustification<TestHeader> {
-		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
-		// is actually related to our target header
-		let mut precommit_header = test_header(target_number + 1);
-		precommit_header.parent_hash = target_hash;
-
-		// 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 signer = extract_keyring(&id);
-			let precommit = signed_precommit(
-				signer,
-				(precommit_header.hash(), *precommit_header.number()),
-				round,
-				set_id,
-			);
-			precommits.push(precommit);
-			votes_ancestries.push(precommit_header.clone());
-		}
-
-		GrandpaJustification {
-			round,
-			commit: finality_grandpa::Commit {
-				target_hash,
-				target_number,
-				precommits,
-			},
-			votes_ancestries,
-		}
-	}
-
-	pub(crate) fn make_justification_for_header_1() -> GrandpaJustification<TestHeader> {
-		make_justification_for_header(
-			&test_header(1),
-			TEST_GRANDPA_ROUND,
-			TEST_GRANDPA_SET_ID,
-			&authority_list(),
-		)
-	}
-
-	#[test]
-	fn justification_with_invalid_encoding_rejected() {
-		assert_eq!(
-			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &[],),
-			Err(Error::JustificationDecode),
-		);
-	}
-
-	#[test]
-	fn justification_with_invalid_target_rejected() {
-		assert_eq!(
-			verify_justification::<TestHeader>(
-				header_id(2),
-				TEST_GRANDPA_SET_ID,
-				voter_set(),
-				&make_justification_for_header_1().encode(),
-			),
-			Err(Error::InvalidJustificationTarget),
-		);
-	}
-
-	#[test]
-	fn justification_with_invalid_commit_rejected() {
-		let mut justification = make_justification_for_header_1();
-		justification.commit.precommits.clear();
-
-		assert_eq!(
-			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &justification.encode(),),
-			Err(Error::InvalidJustificationCommit),
-		);
-	}
-
-	#[test]
-	fn justification_with_invalid_authority_signature_rejected() {
-		let mut justification = make_justification_for_header_1();
-		justification.commit.precommits[0].signature = Default::default();
-
-		assert_eq!(
-			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &justification.encode(),),
-			Err(Error::InvalidAuthoritySignature),
-		);
-	}
-
-	#[test]
-	fn justification_with_invalid_precommit_ancestry() {
-		let mut justification = make_justification_for_header_1();
-		justification.votes_ancestries.push(test_header(10));
-
-		assert_eq!(
-			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &justification.encode(),),
-			Err(Error::InvalidPrecommitAncestries),
-		);
-	}
-
-	#[test]
-	fn valid_justification_accepted() {
-		assert_eq!(
-			verify_justification::<TestHeader>(
-				header_id(1),
-				TEST_GRANDPA_SET_ID,
-				voter_set(),
-				&make_justification_for_header_1().encode(),
-			),
-			Ok(()),
-		);
-	}
-}

bridges/modules/substrate/src/lib.rs

@@ -45,10 +45,8 @@ use sp_trie::StorageProof;
 // Re-export since the node uses these when configuring genesis
 pub use storage::{AuthoritySet, InitializationData, ScheduledChange};
 
-pub use justification::decode_justification_target;
 pub use storage_proof::StorageProofChecker;
 
-mod justification;
 mod storage;
 mod storage_proof;
 mod verifier;
@@ -622,8 +620,8 @@ impl<T: Config> BridgeStorage for PalletStorage<T> {
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::mock::helpers::{authority_list, test_header, unfinalized_header};
-	use crate::mock::{run_test, Origin, TestRuntime};
+	use crate::mock::{run_test, test_header, unfinalized_header, Origin, TestRuntime};
+	use bp_test_utils::authority_list;
 	use frame_support::{assert_noop, assert_ok};
 	use sp_runtime::DispatchError;
 

bridges/modules/substrate/src/mock.rs

@@ -16,11 +16,11 @@
 
 //! Mock Runtime for Substrate Pallet Testing.
 //!
-//! Includes some useful testing utilities in the `helpers` module.
+//! Includes some useful testing types and functions.
 
 #![cfg(test)]
 
-use crate::Config;
+use crate::{BridgedBlockHash, BridgedBlockNumber, BridgedHeader, Config};
 use bp_runtime::Chain;
 use frame_support::{impl_outer_origin, parameter_types, weights::Weight};
 use sp_runtime::{
@@ -30,6 +30,9 @@ use sp_runtime::{
 };
 
 pub type AccountId = u64;
+pub type TestHeader = BridgedHeader<TestRuntime>;
+pub type TestNumber = BridgedBlockNumber<TestRuntime>;
+pub type TestHash = BridgedBlockHash<TestRuntime>;
 
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub struct TestRuntime;
@@ -88,66 +91,16 @@ pub fn run_test<T>(test: impl FnOnce() -> T) -> T {
 	sp_io::TestExternalities::new(Default::default()).execute_with(test)
 }
 
-pub mod helpers {
-	use super::*;
-	use crate::storage::ImportedHeader;
-	use crate::{BridgedBlockHash, BridgedBlockNumber, BridgedHeader};
-	use finality_grandpa::voter_set::VoterSet;
-	use sp_finality_grandpa::{AuthorityId, AuthorityList};
-	use sp_keyring::Ed25519Keyring;
+pub fn test_header(num: TestNumber) -> TestHeader {
+	// We wrap the call to avoid explicit type annotations in our tests
+	bp_test_utils::test_header(num)
+}
 
-	pub type TestHeader = BridgedHeader<TestRuntime>;
-	pub type TestNumber = BridgedBlockNumber<TestRuntime>;
-	pub type TestHash = BridgedBlockHash<TestRuntime>;
-	pub type HeaderId = (TestHash, TestNumber);
-
-	pub fn test_header(num: TestNumber) -> TestHeader {
-		let mut header = TestHeader::new_from_number(num);
-		header.parent_hash = if num == 0 {
-			Default::default()
-		} else {
-			test_header(num - 1).hash()
-		};
-
-		header
-	}
-
-	pub fn unfinalized_header(num: u64) -> ImportedHeader<TestHeader> {
-		ImportedHeader {
-			header: test_header(num),
-			requires_justification: false,
-			is_finalized: false,
-			signal_hash: None,
-		}
-	}
-
-	pub fn header_id(index: u8) -> HeaderId {
-		(test_header(index.into()).hash(), index as _)
-	}
-
-	pub fn extract_keyring(id: &AuthorityId) -> Ed25519Keyring {
-		let mut raw_public = [0; 32];
-		raw_public.copy_from_slice(id.as_ref());
-		Ed25519Keyring::from_raw_public(raw_public).unwrap()
-	}
-
-	pub fn voter_set() -> VoterSet<AuthorityId> {
-		VoterSet::new(authority_list()).unwrap()
-	}
-
-	pub fn authority_list() -> AuthorityList {
-		vec![(alice(), 1), (bob(), 1), (charlie(), 1)]
-	}
-
-	pub fn alice() -> AuthorityId {
-		Ed25519Keyring::Alice.public().into()
-	}
-
-	pub fn bob() -> AuthorityId {
-		Ed25519Keyring::Bob.public().into()
-	}
-
-	pub fn charlie() -> AuthorityId {
-		Ed25519Keyring::Charlie.public().into()
+pub fn unfinalized_header(num: u64) -> crate::storage::ImportedHeader<TestHeader> {
+	crate::storage::ImportedHeader {
+		header: test_header(num),
+		requires_justification: false,
+		is_finalized: false,
+		signal_hash: None,
 	}
 }

bridges/modules/substrate/src/verifier.rs

@@ -22,9 +22,9 @@
 //! has been signed off by the correct GRANDPA authorities, and also enact any authority set changes
 //! if required.
 
-use crate::justification::verify_justification;
 use crate::storage::{AuthoritySet, ImportedHeader, ScheduledChange};
 use crate::BridgeStorage;
+use bp_header_chain::justification::verify_justification;
 use finality_grandpa::voter_set::VoterSet;
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
 use sp_runtime::generic::OpaqueDigestItemId;
@@ -350,10 +350,9 @@ fn find_scheduled_change<H: HeaderT>(header: &H) -> Option<sp_finality_grandpa::
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use crate::justification::tests::*;
-	use crate::mock::helpers::*;
 	use crate::mock::*;
 	use crate::{BestFinalized, BestHeight, HeaderId, ImportedHeaders, PalletStorage};
+	use bp_test_utils::{alice, authority_list, bob, make_justification_for_header};
 	use codec::Encode;
 	use frame_support::{assert_err, assert_ok};
 	use frame_support::{StorageMap, StorageValue};