Stop Importing Full Header Chain (#707)

* Make AncestryProof type more flexible

* Only import single finalized header instead of a chain

* Fix unchecked header import tests

* Add option for limiting ancestry proof size

* Update finality verifier Config in runtimes

* Update some documentation

* Fix Clippy warning

* Allow AncestryChecker to return proof size

Stops us from abusing the `Size` trait

* Remove Size impl for Vec<T>

* Remove size contraints for ancestry proofs

With different proof types its unclear how to "size" should be interpreted,
so we remove this requirement all together to avoid confusion.

bridges/bin/millau/runtime/src/lib.rs

@@ -300,17 +300,11 @@ impl pallet_substrate_bridge::Config for Runtime {
 	type BridgedChain = bp_rialto::Rialto;
 }
 
-parameter_types! {
-	// We'll use the length of a session on the bridged chain as our bound since GRANDPA is
-	// guaranteed to produce a justification every session.
-	pub const MaxHeadersInSingleProof: bp_rialto::BlockNumber = bp_rialto::SESSION_LENGTH;
-}
-
 impl pallet_finality_verifier::Config for Runtime {
 	type BridgedChain = bp_rialto::Rialto;
 	type HeaderChain = pallet_substrate_bridge::Module<Runtime>;
+	type AncestryProof = Vec<bp_rialto::Header>;
 	type AncestryChecker = bp_header_chain::LinearAncestryChecker;
-	type MaxHeadersInSingleProof = MaxHeadersInSingleProof;
 }
 
 impl pallet_shift_session_manager::Config for Runtime {}

bridges/bin/rialto/runtime/src/lib.rs

@@ -407,17 +407,11 @@ impl pallet_substrate_bridge::Config for Runtime {
 	type BridgedChain = bp_millau::Millau;
 }
 
-parameter_types! {
-	// We'll use the length of a session on the bridged chain as our bound since GRANDPA is
-	// guaranteed to produce a justification every session.
-	pub const MaxHeadersInSingleProof: bp_millau::BlockNumber = bp_millau::SESSION_LENGTH;
-}
-
 impl pallet_finality_verifier::Config for Runtime {
 	type BridgedChain = bp_millau::Millau;
 	type HeaderChain = pallet_substrate_bridge::Module<Runtime>;
+	type AncestryProof = Vec<bp_millau::Header>;
 	type AncestryChecker = bp_header_chain::LinearAncestryChecker;
-	type MaxHeadersInSingleProof = MaxHeadersInSingleProof;
 }
 
 impl pallet_shift_session_manager::Config for Runtime {}

bridges/modules/finality-verifier/Cargo.toml

@@ -11,7 +11,6 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0"
 codec = { package = "parity-scale-codec", version = "1.3.1", default-features = false }
 finality-grandpa = { version = "0.12.3", default-features = false }
 serde = { version = "1.0", optional = true }
-num-traits = { version = "0.2", default-features = false }
 
 # Bridge Dependencies
 
@@ -40,7 +39,6 @@ std = [
 	"finality-grandpa/std",
 	"frame-support/std",
 	"frame-system/std",
-	"num-traits/std",
 	"serde",
 	"sp-runtime/std",
 	"sp-std/std",

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

@@ -35,9 +35,8 @@
 use bp_header_chain::{justification::verify_justification, AncestryChecker, HeaderChain};
 use bp_runtime::{Chain, HeaderOf};
 use finality_grandpa::voter_set::VoterSet;
-use frame_support::{dispatch::DispatchError, ensure, traits::Get};
+use frame_support::{dispatch::DispatchError, ensure};
 use frame_system::ensure_signed;
-use num_traits::AsPrimitive;
 use sp_runtime::traits::Header as HeaderT;
 use sp_std::vec::Vec;
 
@@ -64,17 +63,15 @@ pub mod pallet {
 		/// The pallet which we will use as our underlying storage mechanism.
 		type HeaderChain: HeaderChain<<Self::BridgedChain as Chain>::Header, DispatchError>;
 
+		/// The type of ancestry proof used by the pallet.
+		///
+		/// Will be used by the ancestry checker to verify that the header being finalized is
+		/// related to the best finalized header in storage.
+		type AncestryProof: Parameter;
+
 		/// The type through which we will verify that a given header is related to the last
 		/// finalized header in our storage pallet.
-		type AncestryChecker: AncestryChecker<
-			<Self::BridgedChain as Chain>::Header,
-			Vec<<Self::BridgedChain as Chain>::Header>,
-		>;
-
-		/// The maximum length of headers we can have in a single ancestry proof. This prevents
-		/// unbounded iteration when verifying proofs.
-		#[pallet::constant]
-		type MaxHeadersInSingleProof: Get<<Self::BridgedChain as Chain>::BlockNumber>;
+		type AncestryChecker: AncestryChecker<<Self::BridgedChain as Chain>::Header, Self::AncestryProof>;
 	}
 
 	#[pallet::pallet]
@@ -87,45 +84,33 @@ pub mod pallet {
 	impl<T: Config> Pallet<T> {
 		/// Verify a target header is finalized according to the given finality proof.
 		///
-		/// Will use the underlying storage pallet to fetch information about the current
+		/// It will use the underlying storage pallet to fetch information about the current
 		/// authorities and best finalized header in order to verify that the header is finalized.
 		///
-		/// If successful in verification, it will write the header as well as its ancestors (from
-		/// the given `ancestry_proof`) to the underlying storage pallet.
-		///
-		/// Note that the expected format for `ancestry_proof` is a continguous list of finalized
-		/// headers containing (current_best_finalized_header, finality_target]
+		/// If successful in verification, it will write the target header to the underlying storage
+		/// pallet.
 		#[pallet::weight(0)]
 		pub fn submit_finality_proof(
 			origin: OriginFor<T>,
 			finality_target: BridgedHeader<T>,
 			justification: Vec<u8>,
-			ancestry_proof: Vec<BridgedHeader<T>>,
+			ancestry_proof: T::AncestryProof,
 		) -> DispatchResultWithPostInfo {
 			let _ = ensure_signed(origin)?;
 
 			frame_support::debug::trace!("Going to try and finalize header {:?}", finality_target);
-			frame_support::debug::trace!("Got ancestry proof of length {}", ancestry_proof.len());
-
-			ensure!(
-				ancestry_proof.len() <= T::MaxHeadersInSingleProof::get().as_(),
-				<Error<T>>::OversizedAncestryProof
-			);
 
 			let authority_set = T::HeaderChain::authority_set();
 			let voter_set = VoterSet::new(authority_set.authorities).ok_or(<Error<T>>::InvalidAuthoritySet)?;
 			let set_id = authority_set.set_id;
 
-			verify_justification::<BridgedHeader<T>>(
-				(finality_target.hash(), *finality_target.number()),
-				set_id,
-				voter_set,
-				&justification,
-			)
-			.map_err(|e| {
-				frame_support::debug::error!("Received invalid justification for {:?}: {:?}", finality_target, e);
-				<Error<T>>::InvalidJustification
-			})?;
+			let (hash, number) = (finality_target.hash(), *finality_target.number());
+			verify_justification::<BridgedHeader<T>>((hash, number), set_id, voter_set, &justification).map_err(
+				|e| {
+					frame_support::debug::error!("Received invalid justification for {:?}: {:?}", finality_target, e);
+					<Error<T>>::InvalidJustification
+				},
+			)?;
 
 			let best_finalized = T::HeaderChain::best_finalized();
 			frame_support::debug::trace!("Checking ancestry against best finalized header: {:?}", &best_finalized);
@@ -135,17 +120,8 @@ pub mod pallet {
 				<Error<T>>::InvalidAncestryProof
 			);
 
-			// Note that this won't work if we ever change the `ancestry_proof` format to be
-			// sparse since this expects a contiguous set of finalized headers.
-			let _ = T::HeaderChain::append_finalized_chain(ancestry_proof).map_err(|_| {
-				frame_support::debug::error!("Failed to append finalized header chain.");
-				<Error<T>>::FailedToWriteHeader
-			})?;
-
-			frame_support::debug::info!(
-				"Succesfully imported finalized header chain for target header {:?}!",
-				finality_target
-			);
+			T::HeaderChain::append_header(finality_target);
+			frame_support::debug::info!("Succesfully imported finalized header with hash {:?}!", hash);
 
 			Ok(().into())
 		}
@@ -162,8 +138,6 @@ pub mod pallet {
 		InvalidAuthoritySet,
 		/// Failed to write a header to the underlying header chain.
 		FailedToWriteHeader,
-		/// The given ancestry proof is too large to be verified in a single transaction.
-		OversizedAncestryProof,
 	}
 }
 
@@ -283,27 +257,6 @@ mod tests {
 		})
 	}
 
-	#[test]
-	fn disallows_ancestry_proofs_which_are_too_large() {
-		run_test(|| {
-			initialize_substrate_bridge();
-
-			let header = test_header(1);
-			let justification = [1u8; 32].encode();
-
-			let mut ancestry_proof = vec![];
-			let max_len = <TestRuntime as Config>::MaxHeadersInSingleProof::get();
-			for i in 1..=max_len + 1 {
-				ancestry_proof.push(test_header(i as u64));
-			}
-
-			assert_err!(
-				Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification, ancestry_proof,),
-				<Error<TestRuntime>>::OversizedAncestryProof
-			);
-		})
-	}
-
 	#[test]
 	fn disallows_invalid_authority_set() {
 		run_test(|| {

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

@@ -71,14 +71,14 @@ impl pallet_substrate_bridge::Config for TestRuntime {
 }
 
 parameter_types! {
-	pub const MaxHeadersInSingleProof: u8 = 5;
+	pub const MaxElementsInSingleProof: Option<u32> = Some(5);
 }
 
 impl crate::pallet::Config for TestRuntime {
 	type BridgedChain = TestBridgedChain;
 	type HeaderChain = pallet_substrate_bridge::Module<TestRuntime>;
-	type AncestryChecker = Checker<<Self::BridgedChain as Chain>::Header, Vec<<Self::BridgedChain as Chain>::Header>>;
-	type MaxHeadersInSingleProof = MaxHeadersInSingleProof;
+	type AncestryProof = Vec<<Self::BridgedChain as Chain>::Header>;
+	type AncestryChecker = Checker<<Self::BridgedChain as Chain>::Header, Self::AncestryProof>;
 }
 
 #[derive(Debug)]

bridges/modules/substrate/src/lib.rs

@@ -375,25 +375,8 @@ impl<T: Config> bp_header_chain::HeaderChain<BridgedHeader<T>, sp_runtime::Dispa
 		PalletStorage::<T>::new().current_authority_set()
 	}
 
-	fn append_finalized_chain(
-		headers: impl IntoIterator<Item = BridgedHeader<T>>,
-	) -> Result<(), sp_runtime::DispatchError> {
-		let mut storage = PalletStorage::<T>::new();
-
-		let mut header_iter = headers.into_iter().peekable();
-		let first_header = header_iter.peek().ok_or(Error::<T>::NotDescendant)?;
-
-		// Quick ancestry check to make sure we're not writing complete nonsense to storage
-		ensure!(
-			<BestFinalized<T>>::get() == *first_header.parent_hash(),
-			Error::<T>::NotDescendant,
-		);
-
-		for header in header_iter {
-			import_header_unchecked::<_, T>(&mut storage, header);
-		}
-
-		Ok(())
+	fn append_header(header: BridgedHeader<T>) {
+		import_header_unchecked::<_, T>(&mut PalletStorage::<T>::new(), header);
 	}
 }
 
@@ -923,36 +906,15 @@ mod tests {
 			init_with_origin(Origin::root()).unwrap();
 			let storage = PalletStorage::<TestRuntime>::new();
 
-			let child = test_header(2);
-			let header = test_header(3);
+			let header = test_header(2);
+			Module::<TestRuntime>::append_header(header.clone());
 
-			let header_chain = vec![child.clone(), header.clone()];
-			assert_ok!(Module::<TestRuntime>::append_finalized_chain(header_chain));
-
-			assert!(storage.header_by_hash(child.hash()).unwrap().is_finalized);
 			assert!(storage.header_by_hash(header.hash()).unwrap().is_finalized);
-
 			assert_eq!(storage.best_finalized_header().header, header);
 			assert_eq!(storage.best_headers()[0].hash, header.hash());
 		})
 	}
 
-	#[test]
-	fn prevents_unchecked_header_import_if_headers_are_unrelated() {
-		run_test(|| {
-			init_with_origin(Origin::root()).unwrap();
-
-			// Pallet is expecting test_header(2) as the child
-			let not_a_child = test_header(3);
-			let header_chain = vec![not_a_child];
-
-			assert_noop!(
-				Module::<TestRuntime>::append_finalized_chain(header_chain),
-				Error::<TestRuntime>::NotDescendant,
-			);
-		})
-	}
-
 	#[test]
 	fn importing_unchecked_headers_enacts_new_authority_set() {
 		run_test(|| {
@@ -968,7 +930,7 @@ mod tests {
 			header.digest = fork_tests::change_log(0);
 
 			// Let's import our test header
-			assert_ok!(Module::<TestRuntime>::append_finalized_chain(vec![header.clone()]));
+			Module::<TestRuntime>::append_header(header.clone());
 
 			// Make sure that our header is the best finalized
 			assert_eq!(storage.best_finalized_header().header, header);
@@ -998,8 +960,8 @@ mod tests {
 			let header = test_header(3);
 
 			// Let's import our test headers
-			let header_chain = vec![schedules_change, header.clone()];
-			assert_ok!(Module::<TestRuntime>::append_finalized_chain(header_chain));
+			Module::<TestRuntime>::append_header(schedules_change);
+			Module::<TestRuntime>::append_header(header.clone());
 
 			// Make sure that our header is the best finalized
 			assert_eq!(storage.best_finalized_header().header, header);
@@ -1039,8 +1001,7 @@ mod tests {
 
 			// We are expecting an authority set change at height 2, so this header should enact
 			// that upon being imported.
-			let header_chain = vec![test_header(2)];
-			assert_ok!(Module::<TestRuntime>::append_finalized_chain(header_chain));
+			Module::<TestRuntime>::append_header(test_header(2));
 
 			// Make sure that the authority set actually changed upon importing our header
 			assert_eq!(

bridges/primitives/header-chain/src/lib.rs

@@ -69,7 +69,7 @@ pub trait InclusionProofVerifier {
 	fn verify_transaction_inclusion_proof(proof: &Self::TransactionInclusionProof) -> Option<Self::Transaction>;
 }
 
-/// A base trait for pallets which want to keep track of a full set of headers from a bridged chain.
+/// A trait for pallets which want to keep track of finalized headers from a bridged chain.
 pub trait HeaderChain<H, E> {
 	/// Get the best finalized header known to the header chain.
 	fn best_finalized() -> H;
@@ -77,14 +77,8 @@ pub trait HeaderChain<H, E> {
 	/// Get the best authority set known to the header chain.
 	fn authority_set() -> AuthoritySet;
 
-	/// Write a finalized chain of headers to the underlying pallet storage.
-	///
-	/// It is assumed that each header in this chain been finalized, and that the given headers are
-	/// in order (e.g vec![header_1, header_2, ..., header_n]).
-	///
-	/// This function should fail if the first header is not a child of the current best finalized
-	/// header known to the underlying pallet storage.
-	fn append_finalized_chain(headers: impl IntoIterator<Item = H>) -> Result<(), E>;
+	/// Write a header finalized by GRANDPA to the underlying pallet storage.
+	fn append_header(header: H);
 }
 
 impl<H: Default, E> HeaderChain<H, E> for () {
@@ -96,9 +90,7 @@ impl<H: Default, E> HeaderChain<H, E> for () {
 		AuthoritySet::default()
 	}
 
-	fn append_finalized_chain(_headers: impl IntoIterator<Item = H>) -> Result<(), E> {
-		Ok(())
-	}
+	fn append_header(_header: H) {}
 }
 
 /// A trait for checking if a given child header is a direct descendant of an ancestor.