Add submit_finality_proof_ex call to the GRANDPA pallet (#2821)

* depreacte submit_finality_proof and introduce submit_finality_proof_ex instead

* propagate changes to the rest of runtime crates

* tests for new call

* suppress deprecation warning

* revert changes to benchmarks to avoid unnecessary compilation issues when integrating to other repos

modules/grandpa/README.md

@@ -38,11 +38,11 @@ There are two main things in GRANDPA that help building light clients:
 
 ## Pallet Operations
 
-The main entrypoint of the pallet is the `submit_finality_proof` call. It has two arguments - the finalized
-headers and associated GRANDPA justification. The call simply verifies the justification using current
-validators set and checks if header is better than the previous best header. If both checks are passed, the
-header (only its useful fields) is inserted into the runtime storage and may be used by other pallets to verify
-storage proofs.
+The main entrypoint of the pallet is the `submit_finality_proof_ex` call. It has three arguments - the finalized
+headers, associated GRANDPA justification and ID of the authority set that has generated this justification. The
+call simply verifies the justification using current validators set and checks if header is better than the
+previous best header. If both checks are passed, the header (only its useful fields) is inserted into the runtime
+storage and may be used by other pallets to verify storage proofs.
 
 The submitter pays regular fee for submitting all headers, except for the mandatory header. Since it is
 required for the pallet operations, submitting such header is free. So if you're ok with session-length

modules/grandpa/src/benchmarking.rs

@@ -16,8 +16,9 @@
 
 //! Benchmarks for the GRANDPA Pallet.
 //!
-//! The main dispatchable for the GRANDPA pallet is `submit_finality_proof`, so these benchmarks are
-//! based around that. There are to main factors which affect finality proof verification:
+//! The main dispatchable for the GRANDPA pallet is `submit_finality_proof_ex`. Our benchmarks
+//! are based around `submit_finality_proof`, though - from weight PoV they are the same calls.
+//! There are to main factors which affect finality proof verification:
 //!
 //! 1. The number of `votes-ancestries` in the justification
 //! 2. The number of `pre-commits` in the justification

modules/grandpa/src/call_ext.rs

@@ -14,14 +14,18 @@
 // 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/>.
 
-use crate::{weights::WeightInfo, BridgedBlockNumber, BridgedHeader, Config, Error, Pallet};
+use crate::{
+	weights::WeightInfo, BridgedBlockNumber, BridgedHeader, Config, CurrentAuthoritySet, Error,
+	Pallet,
+};
 use bp_header_chain::{
 	justification::GrandpaJustification, max_expected_submit_finality_proof_arguments_size,
 	ChainWithGrandpa, GrandpaConsensusLogReader, SubmitFinalityProofInfo,
 };
 use bp_runtime::{BlockNumberOf, OwnedBridgeModule};
 use codec::Encode;
 use frame_support::{dispatch::CallableCallFor, traits::IsSubType, weights::Weight};
+use sp_consensus_grandpa::SetId;
 use sp_runtime::{
 	traits::Header,
 	transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction},
@@ -35,9 +39,11 @@ pub struct SubmitFinalityProofHelper<T: Config<I>, I: 'static> {
 
 impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
 	/// Check that the GRANDPA head provided by the `SubmitFinalityProof` is better than the best
-	/// one we know.
+	/// one we know. Additionally, checks if `current_set_id` matches the current authority set
+	/// id, if specified.
 	pub fn check_obsolete(
 		finality_target: BlockNumberOf<T::BridgedChain>,
+		current_set_id: Option<SetId>,
 	) -> Result<(), Error<T, I>> {
 		let best_finalized = crate::BestFinalized::<T, I>::get().ok_or_else(|| {
 			log::trace!(
@@ -59,6 +65,20 @@ impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
 			return Err(Error::<T, I>::OldHeader)
 		}
 
+		if let Some(current_set_id) = current_set_id {
+			let actual_set_id = <CurrentAuthoritySet<T, I>>::get().set_id;
+			if current_set_id != actual_set_id {
+				log::trace!(
+					target: crate::LOG_TARGET,
+					"Cannot finalize header signed by unknown authority set: bundled {:?}, best {:?}",
+					current_set_id,
+					actual_set_id,
+				);
+
+				return Err(Error::<T, I>::InvalidAuthoritySetId)
+			}
+		}
+
 		Ok(())
 	}
 
@@ -85,6 +105,18 @@ pub trait CallSubType<T: Config<I, RuntimeCall = Self>, I: 'static>:
 			return Some(submit_finality_proof_info_from_args::<T, I>(
 				finality_target,
 				justification,
+				None,
+			))
+		} else if let Some(crate::Call::<T, I>::submit_finality_proof_ex {
+			finality_target,
+			justification,
+			current_set_id,
+		}) = self.is_sub_type()
+		{
+			return Some(submit_finality_proof_info_from_args::<T, I>(
+				finality_target,
+				justification,
+				Some(*current_set_id),
 			))
 		}
 
@@ -107,7 +139,10 @@ pub trait CallSubType<T: Config<I, RuntimeCall = Self>, I: 'static>:
 			return InvalidTransaction::Call.into()
 		}
 
-		match SubmitFinalityProofHelper::<T, I>::check_obsolete(finality_target.block_number) {
+		match SubmitFinalityProofHelper::<T, I>::check_obsolete(
+			finality_target.block_number,
+			finality_target.current_set_id,
+		) {
 			Ok(_) => Ok(ValidTransaction::default()),
 			Err(Error::<T, I>::OldHeader) => InvalidTransaction::Stale.into(),
 			Err(_) => InvalidTransaction::Call.into(),
@@ -124,6 +159,7 @@ impl<T: Config<I>, I: 'static> CallSubType<T, I> for T::RuntimeCall where
 pub(crate) fn submit_finality_proof_info_from_args<T: Config<I>, I: 'static>(
 	finality_target: &BridgedHeader<T, I>,
 	justification: &GrandpaJustification<BridgedHeader<T, I>>,
+	current_set_id: Option<SetId>,
 ) -> SubmitFinalityProofInfo<BridgedBlockNumber<T, I>> {
 	let block_number = *finality_target.number();
 
@@ -165,28 +201,32 @@ pub(crate) fn submit_finality_proof_info_from_args<T: Config<I>, I: 'static>(
 	);
 	let extra_size = actual_call_size.saturating_sub(max_expected_call_size);
 
-	SubmitFinalityProofInfo { block_number, extra_weight, extra_size }
+	SubmitFinalityProofInfo { block_number, current_set_id, extra_weight, extra_size }
 }
 
 #[cfg(test)]
 mod tests {
 	use crate::{
 		call_ext::CallSubType,
 		mock::{run_test, test_header, RuntimeCall, TestBridgedChain, TestNumber, TestRuntime},
-		BestFinalized, Config, PalletOperatingMode, WeightInfo,
+		BestFinalized, Config, CurrentAuthoritySet, PalletOperatingMode, StoredAuthoritySet,
+		WeightInfo,
 	};
-	use bp_header_chain::ChainWithGrandpa;
+	use bp_header_chain::{ChainWithGrandpa, SubmitFinalityProofInfo};
 	use bp_runtime::{BasicOperatingMode, HeaderId};
 	use bp_test_utils::{
 		make_default_justification, make_justification_for_header, JustificationGeneratorParams,
+		TEST_GRANDPA_SET_ID,
 	};
 	use frame_support::weights::Weight;
 	use sp_runtime::{testing::DigestItem, traits::Header as _, SaturatedConversion};
 
 	fn validate_block_submit(num: TestNumber) -> bool {
-		let bridge_grandpa_call = crate::Call::<TestRuntime, ()>::submit_finality_proof {
+		let bridge_grandpa_call = crate::Call::<TestRuntime, ()>::submit_finality_proof_ex {
 			finality_target: Box::new(test_header(num)),
 			justification: make_default_justification(&test_header(num)),
+			// not initialized => zero
+			current_set_id: 0,
 		};
 		RuntimeCall::check_obsolete_submit_finality_proof(&RuntimeCall::Grandpa(
 			bridge_grandpa_call,
@@ -230,6 +270,18 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn extension_rejects_new_header_if_set_id_is_invalid() {
+		run_test(|| {
+			// when set id is different from the passed one => tx is rejected
+			sync_to_header_10();
+			let next_set = StoredAuthoritySet::<TestRuntime, ()>::try_new(vec![], 0x42).unwrap();
+			CurrentAuthoritySet::<TestRuntime, ()>::put(next_set);
+
+			assert!(!validate_block_submit(15));
+		});
+	}
+
 	#[test]
 	fn extension_accepts_new_header() {
 		run_test(|| {
@@ -240,6 +292,42 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn submit_finality_proof_info_is_parsed() {
+		// when `submit_finality_proof` is used, `current_set_id` is set to `None`
+		let deprecated_call =
+			RuntimeCall::Grandpa(crate::Call::<TestRuntime, ()>::submit_finality_proof {
+				finality_target: Box::new(test_header(42)),
+				justification: make_default_justification(&test_header(42)),
+			});
+		assert_eq!(
+			deprecated_call.submit_finality_proof_info(),
+			Some(SubmitFinalityProofInfo {
+				block_number: 42,
+				current_set_id: None,
+				extra_weight: Weight::zero(),
+				extra_size: 0,
+			})
+		);
+
+		// when `submit_finality_proof_ex` is used, `current_set_id` is set to `Some`
+		let deprecated_call =
+			RuntimeCall::Grandpa(crate::Call::<TestRuntime, ()>::submit_finality_proof_ex {
+				finality_target: Box::new(test_header(42)),
+				justification: make_default_justification(&test_header(42)),
+				current_set_id: 777,
+			});
+		assert_eq!(
+			deprecated_call.submit_finality_proof_info(),
+			Some(SubmitFinalityProofInfo {
+				block_number: 42,
+				current_set_id: Some(777),
+				extra_weight: Weight::zero(),
+				extra_size: 0,
+			})
+		);
+	}
+
 	#[test]
 	fn extension_returns_correct_extra_size_if_call_arguments_are_too_large() {
 		// when call arguments are below our limit => no refund
@@ -249,9 +337,10 @@ mod tests {
 			..Default::default()
 		};
 		let small_justification = make_justification_for_header(justification_params);
-		let small_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+		let small_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex {
 			finality_target: Box::new(small_finality_target),
 			justification: small_justification,
+			current_set_id: TEST_GRANDPA_SET_ID,
 		});
 		assert_eq!(small_call.submit_finality_proof_info().unwrap().extra_size, 0);
 
@@ -265,9 +354,10 @@ mod tests {
 			..Default::default()
 		};
 		let large_justification = make_justification_for_header(justification_params);
-		let large_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+		let large_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex {
 			finality_target: Box::new(large_finality_target),
 			justification: large_justification,
+			current_set_id: TEST_GRANDPA_SET_ID,
 		});
 		assert_ne!(large_call.submit_finality_proof_info().unwrap().extra_size, 0);
 	}
@@ -283,9 +373,10 @@ mod tests {
 
 		// when there are `REASONABLE_HEADERS_IN_JUSTIFICATON_ANCESTRY` headers => no refund
 		let justification = make_justification_for_header(justification_params.clone());
-		let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+		let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex {
 			finality_target: Box::new(finality_target.clone()),
 			justification,
+			current_set_id: TEST_GRANDPA_SET_ID,
 		});
 		assert_eq!(call.submit_finality_proof_info().unwrap().extra_weight, Weight::zero());
 
@@ -296,9 +387,10 @@ mod tests {
 			justification.commit.precommits.len().saturated_into(),
 			justification.votes_ancestries.len().saturated_into(),
 		);
-		let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+		let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof_ex {
 			finality_target: Box::new(finality_target),
 			justification,
+			current_set_id: TEST_GRANDPA_SET_ID,
 		});
 		assert_eq!(call.submit_finality_proof_info().unwrap().extra_weight, call_weight);
 	}