Integrate Grandpa Proof Checker into Substrate Pallet (#375)

* Remove the Substrate primitives crate

The types here were only used in one place, the pallet itself. If other
components start using these types we can considering moving them back
into a standalone crate.

* Start trying to integrate justification module

* Make Substrate blocks configurable in Pallet

* WIP: Try and generalize justification test helpers

* Fix tests which use "real" justifications

* Put common test helpers alongside mock code

* Use common helper for creating headers

* Remove usage of UintAuthorityId

This change favours the use of the Ed25519Keyring authorities
in order to keep things consistent with the tests.

* Add documentation around config trait types

* Make test header, hash, and number types consistent

* Update modules/substrate/src/verifier.rs

Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>

* Update modules/substrate/src/lib.rs

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

* Update modules/substrate/Cargo.toml

Co-authored-by: Svyatoslav Nikolsky <svyatonik@gmail.com>

* Derive `RuntimeDebug` instead of `Debug`

* Add `Paramter` as a trait constraint on config types

Since we use these types as part of the dispatchable functions
we should explicitly require this.

* Enforce that hasher output matches expected hash type

* Accept headers over indexes when making test justifications

* Check that authority sets are valid

* Make Clippy happy

* Apply correct Clippy fix

* Move justification code into primitives module

* Use new module in verifier code

* Add primitives module for Substrate test helpers

* WIP

* Move justification generation into test_helpers

* Revert commits which move `justification` into primitives

This reverts commit 03a381f0bc4a8dbe4785c30d42ab252a06ba876c.

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

bridges/modules/substrate/Cargo.toml

@@ -9,10 +9,10 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0"
 
 [dependencies]
 bp-header-chain = { path = "../../primitives/header-chain", default-features = false }
-bp-substrate = { path = "../../primitives/substrate", default-features = false }
 finality-grandpa = { version = "0.12.3", default-features = false }
 hash-db = { version = "0.15.2", default-features = false }
 serde = { version = "1.0", optional = true }
+num-traits = { version = "0.2", default-features = false }
 
 [dependencies.codec]
 package = "parity-scale-codec"
@@ -89,6 +89,7 @@ std = [
 	"finality-grandpa/std",
 	"frame-support/std",
 	"frame-system/std",
+	"num-traits/std",
 	"serde",
 	"sp-finality-grandpa/std",
 	"sp-runtime/std",

bridges/modules/substrate/src/justification.rs

@@ -14,12 +14,11 @@
 // 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.
 
-// TODO: remove on actual use
-#![allow(dead_code)]
-
 use codec::Decode;
 use finality_grandpa::{voter_set::VoterSet, Chain, Error as GrandpaError};
 use frame_support::RuntimeDebug;
@@ -115,16 +114,21 @@ where
 	Ok(())
 }
 
-/// GRANDPA justification of the bridged chain
+/// 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))]
-struct GrandpaJustification<Header: HeaderT> {
+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>,
 }
@@ -170,56 +174,30 @@ where
 }
 
 #[cfg(test)]
-mod tests {
+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;
-	use sp_runtime::traits::BlakeTwo256;
 
 	const TEST_GRANDPA_ROUND: u64 = 1;
 	const TEST_GRANDPA_SET_ID: SetId = 1;
 
-	type TestHeader = sp_runtime::generic::Header<u64, BlakeTwo256>;
+	pub(crate) fn signed_precommit(
-
-	fn header(index: u8) -> TestHeader {
-		TestHeader::new(
-			index as _,
-			Default::default(),
-			Default::default(),
-			if index == 0 {
-				Default::default()
-			} else {
-				header(index - 1).hash()
-			},
-			Default::default(),
-		)
-	}
-
-	fn header_id(index: u8) -> (H256, u64) {
-		(header(index).hash(), index as _)
-	}
-
-	fn authorities_set() -> VoterSet<AuthorityId> {
-		VoterSet::new(vec![
-			(Ed25519Keyring::Alice.public().into(), 1),
-			(Ed25519Keyring::Bob.public().into(), 1),
-			(Ed25519Keyring::Charlie.public().into(), 1),
-		])
-		.unwrap()
-	}
-
-	fn signed_precommit(
 		signer: Ed25519Keyring,
-		target: (H256, u64),
+		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(
-			TEST_GRANDPA_ROUND,
+			round,
-			TEST_GRANDPA_SET_ID,
+			set_id,
 			&finality_grandpa::Message::Precommit(precommit.clone()),
 		);
 		let signature = signer.sign(&encoded[..]).into();
@@ -230,26 +208,59 @@ mod tests {
 		}
 	}
 
-	fn make_justification_for_header_1() -> GrandpaJustification<TestHeader> {
+	pub(crate) fn make_justification_for_header(
-		GrandpaJustification {
+		header: &TestHeader,
-			round: TEST_GRANDPA_ROUND,
+		round: u64,
-			commit: finality_grandpa::Commit {
+		set_id: SetId,
-				target_hash: header_id(1).0,
+		authorities: &[(AuthorityId, AuthorityWeight)],
-				target_number: header_id(1).1,
+	) -> GrandpaJustification<TestHeader> {
-				precommits: vec![
+		let (target_hash, target_number) = (header.hash(), *header.number());
-					signed_precommit(Ed25519Keyring::Alice, header_id(2)),
+		let mut precommits = vec![];
-					signed_precommit(Ed25519Keyring::Bob, header_id(3)),
+		let mut votes_ancestries = vec![];
-					signed_precommit(Ed25519Keyring::Charlie, header_id(4)),
+
-				],
+		// We want to make sure that the header included in the vote ancestries
-			},
+		// is actually related to our target header
-			votes_ancestries: vec![header(2), header(3), header(4)],
+		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, authorities_set(), &[],),
+			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &[],),
 			Err(Error::JustificationDecode),
 		);
 	}
@@ -260,7 +271,7 @@ mod tests {
 			verify_justification::<TestHeader>(
 				header_id(2),
 				TEST_GRANDPA_SET_ID,
-				authorities_set(),
+				voter_set(),
 				&make_justification_for_header_1().encode(),
 			),
 			Err(Error::InvalidJustificationTarget),
@@ -273,12 +284,7 @@ mod tests {
 		justification.commit.precommits.clear();
 
 		assert_eq!(
-			verify_justification::<TestHeader>(
+			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &justification.encode(),),
-				header_id(1),
-				TEST_GRANDPA_SET_ID,
-				authorities_set(),
-				&justification.encode(),
-			),
 			Err(Error::InvalidJustificationCommit),
 		);
 	}
@@ -289,12 +295,7 @@ mod tests {
 		justification.commit.precommits[0].signature = Default::default();
 
 		assert_eq!(
-			verify_justification::<TestHeader>(
+			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &justification.encode(),),
-				header_id(1),
-				TEST_GRANDPA_SET_ID,
-				authorities_set(),
-				&justification.encode(),
-			),
 			Err(Error::InvalidAuthoritySignature),
 		);
 	}
@@ -302,15 +303,10 @@ mod tests {
 	#[test]
 	fn justification_with_invalid_precommit_ancestry() {
 		let mut justification = make_justification_for_header_1();
-		justification.votes_ancestries.push(header(10));
+		justification.votes_ancestries.push(test_header(10));
 
 		assert_eq!(
-			verify_justification::<TestHeader>(
+			verify_justification::<TestHeader>(header_id(1), TEST_GRANDPA_SET_ID, voter_set(), &justification.encode(),),
-				header_id(1),
-				TEST_GRANDPA_SET_ID,
-				authorities_set(),
-				&justification.encode(),
-			),
 			Err(Error::InvalidPrecommitAncestries),
 		);
 	}
@@ -321,7 +317,7 @@ mod tests {
 			verify_justification::<TestHeader>(
 				header_id(1),
 				TEST_GRANDPA_SET_ID,
-				authorities_set(),
+				voter_set(),
 				&make_justification_for_header_1().encode(),
 			),
 			Ok(()),

bridges/modules/substrate/src/lib.rs

@@ -31,43 +31,100 @@
 // Runtime-generated enums
 #![allow(clippy::large_enum_variant)]
 
-use bp_substrate::{AuthoritySet, ImportedHeader, ScheduledChange};
+use crate::storage::{AuthoritySet, ImportedHeader, ScheduledChange};
-use frame_support::{decl_error, decl_module, decl_storage, dispatch};
+use codec::{Codec, EncodeLike};
+use frame_support::{
+	decl_error, decl_module, decl_storage,
+	dispatch::{DispatchResult, Parameter},
+};
 use frame_system::ensure_signed;
-use sp_runtime::traits::Header as HeaderT;
+use num_traits::AsPrimitive;
-use sp_std::{marker::PhantomData, prelude::*};
+use sp_runtime::traits::{
+	AtLeast32BitUnsigned, Hash as HashT, Header as HeaderT, MaybeDisplay, MaybeMallocSizeOf, MaybeSerializeDeserialize,
+	Member, SimpleBitOps,
+};
+use sp_std::{fmt::Debug, marker::PhantomData, prelude::*, str::FromStr};
 
 mod justification;
+mod storage;
 mod storage_proof;
 mod verifier;
 
 #[cfg(test)]
 mod mock;
 
-type Hash<T> = <T as HeaderT>::Hash;
+pub trait Trait: frame_system::Trait {
-type Number<T> = <T as HeaderT>::Number;
+	/// A type that fulfills the abstract idea of what a Substrate header is.
+	// See here for more info:
+	// https://crates.parity.io/sp_runtime/traits/trait.Header.html
+	type BridgedHeader: Parameter + HeaderT<Number = Self::BridgedBlockNumber, Hash = Self::BridgedBlockHash>;
 
-pub trait Trait: frame_system::Trait {}
+	/// A type that fulfills the abstract idea of what a Substrate block number is.
+	// Constraits come from the associated Number type of `sp_runtime::traits::Header`
+	// See here for more info:
+	// https://crates.parity.io/sp_runtime/traits/trait.Header.html#associatedtype.Number
+	//
+	// Note that the `AsPrimitive<usize>` trait is required by the Grandpa justification
+	// verifier, and is not usually part of a Substrate Header's Number type.
+	type BridgedBlockNumber: Parameter
+		+ Member
+		+ MaybeSerializeDeserialize
+		+ Debug
+		+ sp_std::hash::Hash
+		+ Copy
+		+ MaybeDisplay
+		+ AtLeast32BitUnsigned
+		+ Codec
+		+ FromStr
+		+ MaybeMallocSizeOf
+		+ AsPrimitive<usize>;
+
+	/// A type that fulfills the abstract idea of what a Substrate hash is.
+	// Constraits come from the associated Hash type of `sp_runtime::traits::Header`
+	// See here for more info:
+	// https://crates.parity.io/sp_runtime/traits/trait.Header.html#associatedtype.Hash
+	type BridgedBlockHash: Parameter
+		+ Member
+		+ MaybeSerializeDeserialize
+		+ Debug
+		+ sp_std::hash::Hash
+		+ Ord
+		+ Copy
+		+ MaybeDisplay
+		+ Default
+		+ SimpleBitOps
+		+ Codec
+		+ AsRef<[u8]>
+		+ AsMut<[u8]>
+		+ MaybeMallocSizeOf
+		+ EncodeLike;
+
+	/// A type that fulfills the abstract idea of what a Substrate hasher (a type
+	/// that produces hashes) is.
+	// Constraits come from the associated Hashing type of `sp_runtime::traits::Header`
+	// See here for more info:
+	// https://crates.parity.io/sp_runtime/traits/trait.Header.html#associatedtype.Hashing
+	type BridgedBlockHasher: HashT<Output = Self::BridgedBlockHash>;
+}
 
 decl_storage! {
 	trait Store for Module<T: Trait> as SubstrateBridge {
 		/// Hash of the best finalized header.
-		BestFinalized: T::Hash;
+		BestFinalized: T::BridgedBlockHash;
 		/// Headers which have been imported into the pallet.
-		ImportedHeaders: map hasher(identity) T::Hash => Option<ImportedHeader<T::Header>>;
+		ImportedHeaders: map hasher(identity) T::BridgedBlockHash => Option<ImportedHeader<T::BridgedHeader>>;
 		/// The current Grandpa Authority set.
 		CurrentAuthoritySet: AuthoritySet;
 		/// The next scheduled authority set change.
-		///
 		// Grandpa doesn't require there to always be a pending change. In fact, most of the time
 		// there will be no pending change available.
-		NextScheduledChange: Option<ScheduledChange<Number<T::Header>>>;
+		NextScheduledChange: Option<ScheduledChange<T::BridgedBlockNumber>>;
 	}
 	add_extra_genesis {
-		config(initial_header): Option<T::Header>;
+		config(initial_header): Option<T::BridgedHeader>;
 		config(initial_authority_list): sp_finality_grandpa::AuthorityList;
 		config(initial_set_id): sp_finality_grandpa::SetId;
-		config(first_scheduled_change): Option<ScheduledChange<Number<T::Header>>>;
+		config(first_scheduled_change): Option<ScheduledChange<T::BridgedBlockNumber>>;
 		build(|config| {
 			assert!(
 				!config.initial_authority_list.is_empty(),
@@ -122,8 +179,8 @@ decl_module! {
 		#[weight = 0]
 		pub fn import_signed_header(
 			origin,
-			header: T::Header,
+			header: T::BridgedHeader,
-		) -> dispatch::DispatchResult {
+		) -> DispatchResult {
 			let _ = ensure_signed(origin)?;
 			frame_support::debug::trace!(target: "sub-bridge", "Got header {:?}", header);
 
@@ -147,9 +204,9 @@ decl_module! {
 		#[weight = 0]
 		pub fn finalize_header(
 			origin,
-			hash: Hash<T::Header>,
+			hash: T::BridgedBlockHash,
 			finality_proof: Vec<u8>,
-		) -> dispatch::DispatchResult {
+		) -> DispatchResult {
 			let _ = ensure_signed(origin)?;
 			frame_support::debug::trace!(target: "sub-bridge", "Got header hash {:?}", hash);
 
@@ -220,28 +277,28 @@ impl<T> PalletStorage<T> {
 }
 
 impl<T: Trait> BridgeStorage for PalletStorage<T> {
-	type Header = T::Header;
+	type Header = T::BridgedHeader;
 
-	fn write_header(&mut self, header: &ImportedHeader<T::Header>) {
+	fn write_header(&mut self, header: &ImportedHeader<T::BridgedHeader>) {
 		let hash = header.header.hash();
 		<ImportedHeaders<T>>::insert(hash, header);
 	}
 
-	fn best_finalized_header(&self) -> ImportedHeader<T::Header> {
+	fn best_finalized_header(&self) -> ImportedHeader<T::BridgedHeader> {
 		let hash = <BestFinalized<T>>::get();
 		self.header_by_hash(hash)
 			.expect("A finalized header was added at genesis, therefore this must always exist")
 	}
 
-	fn update_best_finalized(&self, hash: Hash<T::Header>) {
+	fn update_best_finalized(&self, hash: T::BridgedBlockHash) {
 		<BestFinalized<T>>::put(hash)
 	}
 
-	fn header_exists(&self, hash: Hash<T::Header>) -> bool {
+	fn header_exists(&self, hash: T::BridgedBlockHash) -> bool {
 		<ImportedHeaders<T>>::contains_key(hash)
 	}
 
-	fn header_by_hash(&self, hash: Hash<T::Header>) -> Option<ImportedHeader<T::Header>> {
+	fn header_by_hash(&self, hash: T::BridgedBlockHash) -> Option<ImportedHeader<T::BridgedHeader>> {
 		<ImportedHeaders<T>>::get(hash)
 	}
 
@@ -266,11 +323,11 @@ impl<T: Trait> BridgeStorage for PalletStorage<T> {
 		}
 	}
 
-	fn scheduled_set_change(&self) -> Option<ScheduledChange<Number<T::Header>>> {
+	fn scheduled_set_change(&self) -> Option<ScheduledChange<T::BridgedBlockNumber>> {
 		<NextScheduledChange<T>>::get()
 	}
 
-	fn schedule_next_set_change(&self, next_change: ScheduledChange<Number<T::Header>>) {
+	fn schedule_next_set_change(&self, next_change: ScheduledChange<T::BridgedBlockNumber>) {
 		<NextScheduledChange<T>>::put(next_change)
 	}
 }

bridges/modules/substrate/src/mock.rs

@@ -14,6 +14,10 @@
 // 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/>.
 
+//! Mock Runtime for Substrate Pallet Testing.
+//!
+//! Includes some useful testing utilities in the `helpers` module.
+
 #![cfg(test)]
 
 use crate::Trait;
@@ -68,8 +72,66 @@ impl frame_system::Trait for TestRuntime {
 	type SystemWeightInfo = ();
 }
 
-impl Trait for TestRuntime {}
+impl Trait for TestRuntime {
+	type BridgedHeader = <Self as frame_system::Trait>::Header;
+	type BridgedBlockNumber = <Self as frame_system::Trait>::BlockNumber;
+	type BridgedBlockHash = <Self as frame_system::Trait>::Hash;
+	type BridgedBlockHasher = <Self as frame_system::Trait>::Hashing;
+}
 
 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 finality_grandpa::voter_set::VoterSet;
+	use sp_finality_grandpa::{AuthorityId, AuthorityList};
+	use sp_keyring::Ed25519Keyring;
+
+	pub type TestHeader = <TestRuntime as Trait>::BridgedHeader;
+	pub type TestNumber = <TestRuntime as Trait>::BridgedBlockNumber;
+	pub type TestHash = <TestRuntime as Trait>::BridgedBlockHash;
+	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 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()
+	}
+}

bridges/modules/substrate/src/storage.rs

@@ -14,13 +14,10 @@
 // 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/>.
 
-//! Primitives for the Substrate light client (a.k.a bridge) pallet.
+//! Storage primitives for the Substrate light client (a.k.a bridge) pallet.
-
-#![warn(missing_docs)]
-#![cfg_attr(not(feature = "std"), no_std)]
 
+use codec::{Decode, Encode};
 use core::default::Default;
-use parity_scale_codec::{Decode, Encode};
 #[cfg(feature = "std")]
 use serde::{Deserialize, Serialize};
 use sp_finality_grandpa::{AuthorityList, SetId};
@@ -75,8 +72,3 @@ impl<H> core::ops::Deref for ImportedHeader<H> {
 		&self.header
 	}
 }
-
-/// Prove that the given header was finalized by the given authority set.
-pub fn check_finality_proof<H>(_header: &H, _set: &AuthoritySet, _justification: &[u8]) -> bool {
-	true
-}

bridges/modules/substrate/src/verifier.rs

@@ -22,17 +22,21 @@
 //! 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_substrate::{check_finality_proof, AuthoritySet, ImportedHeader, ScheduledChange};
+use finality_grandpa::voter_set::VoterSet;
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
 use sp_runtime::generic::OpaqueDigestItemId;
 use sp_runtime::traits::{CheckedAdd, Header as HeaderT, One};
+use sp_runtime::RuntimeDebug;
 use sp_std::{prelude::Vec, vec};
 
 /// The finality proof used by the pallet.
 ///
 /// For a Substrate based chain using Grandpa this will
 /// be an encoded Grandpa Justification.
+#[derive(RuntimeDebug)]
 pub struct FinalityProof(Vec<u8>);
 
 impl From<&[u8]> for FinalityProof {
@@ -48,7 +52,7 @@ impl From<Vec<u8>> for FinalityProof {
 }
 
 /// Errors which can happen while importing a header.
-#[derive(Debug, PartialEq)]
+#[derive(RuntimeDebug, PartialEq)]
 pub enum ImportError {
 	/// This header is older than our latest finalized block, thus not useful.
 	OldHeader,
@@ -60,25 +64,33 @@ pub enum ImportError {
 	InvalidChildNumber,
 	/// The height of the next authority set change overflowed.
 	ScheduledHeightOverflow,
+	/// Received an authority set which was invalid in some way, such as
+	/// the authority weights being empty or overflowing the `AuthorityWeight`
+	/// type.
+	InvalidAuthoritySet,
 }
 
 /// Errors which can happen while verifying a headers finality.
-#[derive(Debug, PartialEq)]
+#[derive(RuntimeDebug, PartialEq)]
 pub enum FinalizationError {
 	/// This header has never been imported by the pallet.
 	UnknownHeader,
-	/// We were unable to prove finality for this header.
-	UnfinalizedHeader,
 	/// Trying to prematurely import a justification
 	PrematureJustification,
 	/// We failed to verify this header's ancestry.
 	AncestryCheckFailed,
 	/// This header is older than our latest finalized block, thus not useful.
 	OldHeader,
+	/// The given justification was not able to finalize the given header.
+	///
+	/// There are several reasons why this might happen, such as the justification being
+	/// signed by the wrong authority set, being given alongside an unexpected header,
+	/// or failing ancestry checks.
+	InvalidJustification,
 }
 
 /// Used to verify imported headers and their finality status.
-#[derive(Debug)]
+#[derive(RuntimeDebug)]
 pub struct Verifier<S> {
 	pub storage: S,
 }
@@ -87,6 +99,7 @@ impl<S, H> Verifier<S>
 where
 	S: BridgeStorage<Header = H>,
 	H: HeaderT,
+	H::Number: finality_grandpa::BlockNumberOps,
 {
 	/// Import a header to the pallet.
 	///
@@ -126,6 +139,21 @@ where
 			// Since we don't currently have a pending authority set change let's check if the header
 			// contains a log indicating when the next change should be.
 			if let Some(change) = find_scheduled_change(&header) {
+				let mut total_weight = 0u64;
+
+				// We need to make sure that we don't overflow the `AuthorityWeight` type.
+				for (_id, weight) in &change.next_authorities {
+					total_weight = total_weight
+						.checked_add(*weight)
+						.ok_or(ImportError::InvalidAuthoritySet)?;
+				}
+
+				// If none of the authorities have a weight associated with them the
+				// set is essentially empty. We don't want that.
+				if total_weight == 0 {
+					return Err(ImportError::InvalidAuthoritySet);
+				}
+
 				let next_set = AuthoritySet {
 					authorities: change.next_authorities,
 					set_id: self.storage.current_authority_set().set_id + 1,
@@ -176,10 +204,18 @@ where
 		}
 
 		let current_authority_set = self.storage.current_authority_set();
-		let is_finalized = check_finality_proof(&header, &current_authority_set, &proof.0);
+		let voter_set = VoterSet::new(current_authority_set.authorities).expect(
-		if !is_finalized {
+			"This only fails if we have an invalid list of authorities. Since we
-			return Err(FinalizationError::UnfinalizedHeader);
+			got this from storage it should always be valid, otherwise we have a bug.",
-		}
+		);
+		verify_justification::<H>(
+			(hash, *header.number()),
+			current_authority_set.set_id,
+			voter_set,
+			&proof.0,
+		)
+		.map_err(|_| FinalizationError::InvalidJustification)?;
+		frame_support::debug::trace!(target: "sub-bridge", "Received valid justification for {:?}", header);
 
 		frame_support::debug::trace!(target: "sub-bridge", "Checking ancestry for headers between {:?} and {:?}", last_finalized, header);
 		let mut finalized_headers =
@@ -279,37 +315,29 @@ 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, ImportedHeaders, PalletStorage};
+	use codec::Encode;
 	use frame_support::{assert_err, assert_ok};
 	use frame_support::{StorageMap, StorageValue};
-	use sp_finality_grandpa::{AuthorityId, AuthorityList};
+	use sp_finality_grandpa::{AuthorityId, SetId};
-	use sp_runtime::testing::UintAuthorityId;
-
-	type TestHeader = <TestRuntime as frame_system::Trait>::Header;
-	type TestNumber = <TestHeader as HeaderT>::Number;
 
 	fn unfinalized_header(num: u64) -> ImportedHeader<TestHeader> {
 		ImportedHeader {
-			header: TestHeader::new_from_number(num),
+			header: test_header(num),
 			requires_justification: false,
 			is_finalized: false,
 		}
 	}
 
-	fn get_authorities(authorities: Vec<(u64, u64)>) -> AuthorityList {
-		authorities
-			.iter()
-			.map(|(id, weight)| (UintAuthorityId(*id).to_public_key::<AuthorityId>(), *weight))
-			.collect()
-	}
-
 	fn schedule_next_change(
-		authorities: Vec<(u64, u64)>,
+		authorities: Vec<AuthorityId>,
-		set_id: u64,
+		set_id: SetId,
 		height: TestNumber,
 	) -> ScheduledChange<TestNumber> {
-		let authorities = get_authorities(authorities);
+		let authorities = authorities.into_iter().map(|id| (id, 1u64)).collect();
 		let authority_set = AuthoritySet::new(authorities, set_id);
 		ScheduledChange { authority_set, height }
 	}
@@ -321,7 +349,7 @@ mod tests {
 	) -> Vec<ImportedHeader<TestHeader>> {
 		let mut imported_headers = vec![];
 		let genesis = ImportedHeader {
-			header: TestHeader::new_from_number(0),
+			header: test_header(0),
 			requires_justification: false,
 			is_finalized: true,
 		};
@@ -331,11 +359,8 @@ mod tests {
 		imported_headers.push(genesis);
 
 		for (num, requires_justification, is_finalized) in headers {
-			let mut h = TestHeader::new_from_number(num);
-			h.parent_hash = imported_headers.last().unwrap().hash();
-
 			let header = ImportedHeader {
-				header: h,
+				header: test_header(num),
 				requires_justification,
 				is_finalized,
 			};
@@ -355,7 +380,7 @@ mod tests {
 			storage.write_header(&parent);
 			storage.update_best_finalized(parent.hash());
 
-			let header = TestHeader::new_from_number(1);
+			let header = test_header(1);
 			let mut verifier = Verifier { storage };
 			assert_err!(verifier.import_header(header), ImportError::OldHeader);
 		})
@@ -381,7 +406,7 @@ mod tests {
 	fn fails_to_import_header_twice() {
 		run_test(|| {
 			let storage = PalletStorage::<TestRuntime>::new();
-			let header = TestHeader::new_from_number(1);
+			let header = test_header(1);
 			<BestFinalized<TestRuntime>>::put(header.hash());
 
 			let imported_header = ImportedHeader {
@@ -400,7 +425,7 @@ mod tests {
 	fn succesfully_imports_valid_but_unfinalized_header() {
 		run_test(|| {
 			let storage = PalletStorage::<TestRuntime>::new();
-			let parent = TestHeader::new_from_number(1);
+			let parent = test_header(1);
 			let parent_hash = parent.hash();
 			<BestFinalized<TestRuntime>>::put(parent.hash());
 
@@ -411,8 +436,7 @@ mod tests {
 			};
 			<ImportedHeaders<TestRuntime>>::insert(parent_hash, &imported_header);
 
-			let mut header = TestHeader::new_from_number(2);
+			let header = test_header(2);
-			header.parent_hash = parent_hash;
 			let mut verifier = Verifier {
 				storage: storage.clone(),
 			};
@@ -458,7 +482,7 @@ mod tests {
 
 			// Need to give it a different parent_hash or else it'll be
 			// related to our test genesis header
-			let mut bad_ancestor = TestHeader::new_from_number(0);
+			let mut bad_ancestor = test_header(0);
 			bad_ancestor.parent_hash = [1u8; 32].into();
 			let bad_ancestor = ImportedHeader {
 				header: bad_ancestor,
@@ -484,7 +508,7 @@ mod tests {
 			}
 
 			// What if we have an "ancestor" that's newer than child?
-			let new_ancestor = TestHeader::new_from_number(5);
+			let new_ancestor = test_header(5);
 			let new_ancestor = ImportedHeader {
 				header: new_ancestor,
 				requires_justification: false,
@@ -497,24 +521,62 @@ mod tests {
 		})
 	}
 
+	#[test]
+	fn doesnt_import_header_which_schedules_change_with_invalid_authority_set() {
+		use sp_runtime::{Digest, DigestItem};
+
+		run_test(|| {
+			let mut storage = PalletStorage::<TestRuntime>::new();
+			let headers = vec![(1, false, false)];
+			let _imported_headers = write_headers(&mut storage, headers);
+			let mut header = test_header(2);
+
+			// This is an *invalid* authority set because the combined weight of the
+			// authorities is greater than `u64::MAX`
+			let consensus_log = ConsensusLog::<TestNumber>::ScheduledChange(sp_finality_grandpa::ScheduledChange {
+				next_authorities: vec![(alice(), u64::MAX), (bob(), u64::MAX)],
+				delay: 0,
+			});
+
+			header.digest = Digest::<TestHash> {
+				logs: vec![DigestItem::Consensus(GRANDPA_ENGINE_ID, consensus_log.encode())],
+			};
+
+			let mut verifier = Verifier { storage };
+
+			assert_eq!(
+				verifier.import_header(header).unwrap_err(),
+				ImportError::InvalidAuthoritySet
+			);
+		})
+	}
+
 	#[test]
 	fn finalizes_header_which_doesnt_enact_or_schedule_a_new_authority_set() {
 		run_test(|| {
 			let mut storage = PalletStorage::<TestRuntime>::new();
 			let headers = vec![(1, false, false)];
-			let imported_headers = write_headers(&mut storage, headers);
+			let _imported_headers = write_headers(&mut storage, headers);
 
 			// Nothing special about this header, yet Grandpa may have created a justification
 			// for it since it does that periodically
-			let mut header = TestHeader::new_from_number(2);
+			let header = test_header(2);
-			header.parent_hash = imported_headers[1].hash();
+
+			let set_id = 1;
+			let authorities = authority_list();
+			let authority_set = AuthoritySet::new(authorities.clone(), set_id);
+			storage.update_current_authority_set(authority_set);
+
+			// We'll need this justification to finalize the header
+			let grandpa_round = 1;
+			let justification = make_justification_for_header(&header, grandpa_round, set_id, &authorities).encode();
 
 			let mut verifier = Verifier {
 				storage: storage.clone(),
 			};
 
 			assert_ok!(verifier.import_header(header.clone()));
-			assert_ok!(verifier.import_finality_proof(header.hash(), vec![4, 2].into()));
+			assert_ok!(verifier.import_finality_proof(header.hash(), justification.into()));
 			assert_eq!(storage.best_finalized_header().header, header);
 		})
 	}
@@ -525,15 +587,26 @@ mod tests {
 			let mut storage = PalletStorage::<TestRuntime>::new();
 			let headers = vec![(1, false, false), (2, false, false)];
 			let imported_headers = write_headers(&mut storage, headers);
+			let header = test_header(3);
 
-			let mut header = TestHeader::new_from_number(3);
+			let set_id = 1;
-			header.parent_hash = imported_headers[2].hash();
+			let authorities = authority_list();
+			let authority_set = AuthoritySet {
+				authorities: authorities.clone(),
+				set_id,
+			};
+			storage.update_current_authority_set(authority_set);
+
+			let grandpa_round = 1;
+			let justification = make_justification_for_header(&header, grandpa_round, set_id, &authorities).encode();
 
 			let mut verifier = Verifier {
 				storage: storage.clone(),
 			};
 			assert!(verifier.import_header(header.clone()).is_ok());
-			assert!(verifier.import_finality_proof(header.hash(), vec![4, 2].into()).is_ok());
+			assert!(verifier
+				.import_finality_proof(header.hash(), justification.into())
+				.is_ok());
 
 			// Make sure we marked the our headers as finalized
 			assert!(storage.header_by_hash(imported_headers[1].hash()).unwrap().is_finalized);
@@ -550,21 +623,23 @@ mod tests {
 		run_test(|| {
 			let mut storage = PalletStorage::<TestRuntime>::new();
 			let headers = vec![(1, false, false)];
-			let imported_headers = write_headers(&mut storage, headers);
+			let _imported_headers = write_headers(&mut storage, headers);
 
-			let set_id = 0;
+			let set_id = 1;
-			let authorities = get_authorities(vec![(1, 1)]);
+			let authorities = authority_list();
-			let initial_authority_set = AuthoritySet::new(authorities, set_id);
+			let initial_authority_set = AuthoritySet::new(authorities.clone(), set_id);
 			storage.update_current_authority_set(initial_authority_set);
 
 			// This header enacts an authority set change upon finalization
-			let mut header = TestHeader::new_from_number(2);
+			let header = test_header(2);
-			header.parent_hash = imported_headers[1].hash();
+
+			let grandpa_round = 1;
+			let justification = make_justification_for_header(&header, grandpa_round, set_id, &authorities).encode();
 
 			// Schedule a change at the height of our header
-			let set_id = 1;
+			let set_id = 2;
 			let height = *header.number();
-			let authorities = vec![(2, 1)];
+			let authorities = vec![alice()];
 			let change = schedule_next_change(authorities, set_id, height);
 			storage.schedule_next_set_change(change.clone());
 
@@ -573,7 +648,7 @@ mod tests {
 			};
 
 			assert_ok!(verifier.import_header(header.clone()));
-			assert_ok!(verifier.import_finality_proof(header.hash(), vec![4, 2].into()));
+			assert_ok!(verifier.import_finality_proof(header.hash(), justification.into()));
 			assert_eq!(storage.best_finalized_header().header, header);
 
 			// Make sure that we have updated the set now that we've finalized our header
@@ -585,7 +660,7 @@ mod tests {
 	fn importing_finality_proof_for_already_finalized_header_doesnt_work() {
 		run_test(|| {
 			let mut storage = PalletStorage::<TestRuntime>::new();
-			let genesis = TestHeader::new_from_number(0);
+			let genesis = test_header(0);
 
 			let genesis = ImportedHeader {
 				header: genesis,
@@ -627,14 +702,23 @@ mod tests {
 			let headers = vec![(1, false, false)];
 			let imported_headers = write_headers(&mut storage, headers);
 
+			// Set up our initial authority set
+			let set_id = 1;
+			let authorities = authority_list();
+			let initial_authority_set = AuthoritySet::new(authorities.clone(), set_id);
+			storage.update_current_authority_set(initial_authority_set);
+
 			// This is header N
-			let mut header = TestHeader::new_from_number(2);
+			let header = test_header(2);
-			header.parent_hash = imported_headers[1].hash();
+
+			// Since we want to finalize N we need a justification for it
+			let grandpa_round = 1;
+			let justification = make_justification_for_header(&header, grandpa_round, set_id, &authorities).encode();
 
 			// Schedule a change at height N
-			let set_id = 1;
+			let set_id = 2;
 			let height = *header.number();
-			let authorities = vec![(1, 1)];
+			let authorities = vec![alice()];
 			let change = schedule_next_change(authorities, set_id, height);
 			storage.schedule_next_set_change(change.clone());
 
@@ -651,17 +735,17 @@ mod tests {
 			);
 
 			// Now we want to import some headers which are past N
-			let mut child = TestHeader::new_from_number(*header.number() + 1);
+			let child = test_header(*header.number() + 1);
-			child.parent_hash = header.hash();
 			assert!(verifier.import_header(child.clone()).is_ok());
 
-			let mut grandchild = TestHeader::new_from_number(*child.number() + 1);
+			let grandchild = test_header(*child.number() + 1);
-			grandchild.parent_hash = child.hash();
 			assert!(verifier.import_header(grandchild).is_ok());
 
 			// Even though we're a few headers ahead we should still be able to import
 			// a justification for header N
-			assert!(verifier.import_finality_proof(header.hash(), vec![4, 2].into()).is_ok());
+			assert!(verifier
+				.import_finality_proof(header.hash(), justification.into())
+				.is_ok());
 
 			// Some checks to make sure that our header has been correctly finalized
 			let finalized_header = storage.header_by_hash(header.hash()).unwrap();