Weight+size limits for bridge GRANDPA pallet calls (#1882)

* weight+size limits for bridge GRANDPA pallet calls

* continue

* fixed all tests

* some changes to refund computations

* post_dispatch_refunds_relayer_in_all_finality_batch_with_extra_weight

* - dup code

* do not return Pays::No if call is above weight/size limits

* relayer_pays_tx_fee_when_submitting_huge_mandatory_header and relayer_pays_tx_fee_when_submitting_justification_with_long_ancestry_votes

* clippy

* fmt

* clippy

* small change in docs

* fixed GRANDPA-limits constants for Polkadot-like chains

* clippy

* clippy + spelling

* Update primitives/polkadot-core/src/lib.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>

* Update bin/runtime-common/src/refund_relayer_extension.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>

* reverted unnecessary change

* GrandpaJustification::max_reasonable_size

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>

bridges/modules/grandpa/src/call_ext.rs

@@ -14,17 +14,46 @@
 // 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::{Config, Error, Pallet};
+use crate::{weights::WeightInfo, BridgedBlockNumber, BridgedHeader, Config, Error, Pallet};
+use bp_header_chain::{justification::GrandpaJustification, ChainWithGrandpa};
 use bp_runtime::BlockNumberOf;
-use frame_support::{dispatch::CallableCallFor, traits::IsSubType};
+use codec::Encode;
+use frame_support::{dispatch::CallableCallFor, traits::IsSubType, weights::Weight, RuntimeDebug};
 use sp_runtime::{
-	traits::Header,
+	traits::{Header, Zero},
 	transaction_validity::{InvalidTransaction, TransactionValidity, ValidTransaction},
+	SaturatedConversion,
 };
 
+/// Info about a `SubmitParachainHeads` call which tries to update a single parachain.
+#[derive(Copy, Clone, PartialEq, RuntimeDebug)]
+pub struct SubmitFinalityProofInfo<N> {
+	/// Number of the finality target.
+	pub block_number: N,
+	/// Extra weight that we assume is included in the call.
+	///
+	/// We have some assumptions about headers and justifications of the bridged chain.
+	/// We know that if our assumptions are correct, then the call must not have the
+	/// weight above some limit. The fee paid for weight above that limit, is never refunded.
+	pub extra_weight: Weight,
+	/// Extra size (in bytes) that we assume are included in the call.
+	///
+	/// We have some assumptions about headers and justifications of the bridged chain.
+	/// We know that if our assumptions are correct, then the call must not have the
+	/// weight above some limit. The fee paid for bytes above that limit, is never refunded.
+	pub extra_size: u32,
+}
+
+impl<N> SubmitFinalityProofInfo<N> {
+	/// Returns `true` if call size/weight is below our estimations for regular calls.
+	pub fn fits_limits(&self) -> bool {
+		self.extra_weight.is_zero() && self.extra_size.is_zero()
+	}
+}
+
 /// Helper struct that provides methods for working with the `SubmitFinalityProof` call.
 pub struct SubmitFinalityProofHelper<T: Config<I>, I: 'static> {
-	pub _phantom_data: sp_std::marker::PhantomData<(T, I)>,
+	_phantom_data: sp_std::marker::PhantomData<(T, I)>,
 }
 
 impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
@@ -69,12 +98,17 @@ impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
 pub trait CallSubType<T: Config<I, RuntimeCall = Self>, I: 'static>:
 	IsSubType<CallableCallFor<Pallet<T, I>, T>>
 {
-	/// Extract the finality target from a `SubmitParachainHeads` call.
-	fn submit_finality_proof_info(&self) -> Option<BlockNumberOf<T::BridgedChain>> {
-		if let Some(crate::Call::<T, I>::submit_finality_proof { finality_target, .. }) =
+	/// Extract finality proof info from a runtime call.
+	fn submit_finality_proof_info(
+		&self,
+	) -> Option<SubmitFinalityProofInfo<BridgedBlockNumber<T, I>>> {
+		if let Some(crate::Call::<T, I>::submit_finality_proof { finality_target, justification }) =
 			self.is_sub_type()
 		{
-			return Some(*finality_target.number())
+			return Some(submit_finality_proof_info_from_args::<T, I>(
+				finality_target,
+				justification,
+			))
 		}
 
 		None
@@ -92,7 +126,7 @@ pub trait CallSubType<T: Config<I, RuntimeCall = Self>, I: 'static>:
 			_ => return Ok(ValidTransaction::default()),
 		};
 
-		match SubmitFinalityProofHelper::<T, I>::check_obsolete(finality_target) {
+		match SubmitFinalityProofHelper::<T, I>::check_obsolete(finality_target.block_number) {
 			Ok(_) => Ok(ValidTransaction::default()),
 			Err(Error::<T, I>::OldHeader) => InvalidTransaction::Stale.into(),
 			Err(_) => InvalidTransaction::Call.into(),
@@ -105,15 +139,66 @@ impl<T: Config<I>, I: 'static> CallSubType<T, I> for T::RuntimeCall where
 {
 }
 
+/// Extract finality proof info from the submitted header and justification.
+pub(crate) fn submit_finality_proof_info_from_args<T: Config<I>, I: 'static>(
+	finality_target: &BridgedHeader<T, I>,
+	justification: &GrandpaJustification<BridgedHeader<T, I>>,
+) -> SubmitFinalityProofInfo<BridgedBlockNumber<T, I>> {
+	let block_number = *finality_target.number();
+
+	// the `submit_finality_proof` call will reject justifications with invalid, duplicate,
+	// unknown and extra signatures. It'll also reject justifications with less than necessary
+	// signatures. So we do not care about extra weight because of additional signatures here.
+	let precommits_len = justification.commit.precommits.len().saturated_into();
+	let required_precommits = precommits_len;
+
+	// We do care about extra weight because of more-than-expected headers in the votes
+	// ancestries. But we have problems computing extra weight for additional headers (weight of
+	// additional header is too small, so that our benchmarks aren't detecting that). So if there
+	// are more than expected headers in votes ancestries, we will treat the whole call weight
+	// as an extra weight.
+	let votes_ancestries_len = justification.votes_ancestries.len().saturated_into();
+	let extra_weight =
+		if votes_ancestries_len > T::BridgedChain::REASONABLE_HEADERS_IN_JUSTIFICATON_ANCESTRY {
+			T::WeightInfo::submit_finality_proof(precommits_len, votes_ancestries_len)
+		} else {
+			Weight::zero()
+		};
+
+	// we can estimate extra call size easily, without any additional significant overhead
+	let actual_call_size: u32 = finality_target
+		.encoded_size()
+		.saturating_add(justification.encoded_size())
+		.saturated_into();
+	let max_expected_call_size = max_expected_call_size::<T, I>(required_precommits);
+	let extra_size = actual_call_size.saturating_sub(max_expected_call_size);
+
+	SubmitFinalityProofInfo { block_number, extra_weight, extra_size }
+}
+
+/// Returns maximal expected size of `submit_finality_proof` call arguments.
+fn max_expected_call_size<T: Config<I>, I: 'static>(required_precommits: u32) -> u32 {
+	let max_expected_justification_size =
+		GrandpaJustification::max_reasonable_size::<T::BridgedChain>(required_precommits);
+
+	// call arguments are header and justification
+	T::BridgedChain::MAX_HEADER_SIZE.saturating_add(max_expected_justification_size)
+}
+
 #[cfg(test)]
 mod tests {
 	use crate::{
 		call_ext::CallSubType,
-		mock::{run_test, test_header, RuntimeCall, TestNumber, TestRuntime},
-		BestFinalized,
+		mock::{run_test, test_header, RuntimeCall, TestBridgedChain, TestNumber, TestRuntime},
+		BestFinalized, Config, WeightInfo,
 	};
+	use bp_header_chain::ChainWithGrandpa;
 	use bp_runtime::HeaderId;
-	use bp_test_utils::make_default_justification;
+	use bp_test_utils::{
+		make_default_justification, make_justification_for_header, JustificationGeneratorParams,
+	};
+	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 {
@@ -160,4 +245,67 @@ mod tests {
 			assert!(validate_block_submit(15));
 		});
 	}
+
+	#[test]
+	fn extension_returns_correct_extra_size_if_call_arguments_are_too_large() {
+		// when call arguments are below our limit => no refund
+		let small_finality_target = test_header(1);
+		let justification_params = JustificationGeneratorParams {
+			header: small_finality_target.clone(),
+			..Default::default()
+		};
+		let small_justification = make_justification_for_header(justification_params);
+		let small_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+			finality_target: Box::new(small_finality_target),
+			justification: small_justification,
+		});
+		assert_eq!(small_call.submit_finality_proof_info().unwrap().extra_size, 0);
+
+		// when call arguments are too large => partial refund
+		let mut large_finality_target = test_header(1);
+		large_finality_target
+			.digest_mut()
+			.push(DigestItem::Other(vec![42u8; 1024 * 1024]));
+		let justification_params = JustificationGeneratorParams {
+			header: large_finality_target.clone(),
+			..Default::default()
+		};
+		let large_justification = make_justification_for_header(justification_params);
+		let large_call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+			finality_target: Box::new(large_finality_target),
+			justification: large_justification,
+		});
+		assert_ne!(large_call.submit_finality_proof_info().unwrap().extra_size, 0);
+	}
+
+	#[test]
+	fn extension_returns_correct_extra_weight_if_there_are_too_many_headers_in_votes_ancestry() {
+		let finality_target = test_header(1);
+		let mut justification_params = JustificationGeneratorParams {
+			header: finality_target.clone(),
+			ancestors: TestBridgedChain::REASONABLE_HEADERS_IN_JUSTIFICATON_ANCESTRY,
+			..Default::default()
+		};
+
+		// 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 {
+			finality_target: Box::new(finality_target.clone()),
+			justification,
+		});
+		assert_eq!(call.submit_finality_proof_info().unwrap().extra_weight, Weight::zero());
+
+		// when there are `REASONABLE_HEADERS_IN_JUSTIFICATON_ANCESTRY + 1` headers => full refund
+		justification_params.ancestors += 1;
+		let justification = make_justification_for_header(justification_params);
+		let call_weight = <TestRuntime as Config>::WeightInfo::submit_finality_proof(
+			justification.commit.precommits.len().saturated_into(),
+			justification.votes_ancestries.len().saturated_into(),
+		);
+		let call = RuntimeCall::Grandpa(crate::Call::submit_finality_proof {
+			finality_target: Box::new(finality_target),
+			justification,
+		});
+		assert_eq!(call.submit_finality_proof_info().unwrap().extra_weight, call_weight);
+	}
 }

bridges/modules/grandpa/src/lib.rs

@@ -36,7 +36,7 @@
 // Runtime-generated enums
 #![allow(clippy::large_enum_variant)]
 
-use storage_types::StoredAuthoritySet;
+pub use storage_types::StoredAuthoritySet;
 
 use bp_header_chain::{
 	justification::GrandpaJustification, ChainWithGrandpa, HeaderChain, InitializationData,
@@ -180,6 +180,9 @@ pub mod pallet {
 
 			let is_authorities_change_enacted =
 				try_enact_authority_change::<T, I>(&finality_target, set_id)?;
+			let may_refund_call_fee = is_authorities_change_enacted &&
+				submit_finality_proof_info_from_args::<T, I>(&*finality_target, &justification)
+					.fits_limits();
 			<RequestCount<T, I>>::mutate(|count| *count += 1);
 			insert_header::<T, I>(*finality_target, hash);
 			log::info!(
@@ -193,8 +196,10 @@ pub mod pallet {
 			//
 			// We don't want to charge extra costs for mandatory operations. So relayer is not
 			// paying fee for mandatory headers import transactions.
-			let is_mandatory_header = is_authorities_change_enacted;
-			let pays_fee = if is_mandatory_header { Pays::No } else { Pays::Yes };
+			//
+			// If size/weight of the call is exceeds our estimated limits, the relayer still needs
+			// to pay for the transaction.
+			let pays_fee = if may_refund_call_fee { Pays::No } else { Pays::Yes };
 
 			// the proof size component of the call weight assumes that there are
 			// `MaxBridgedAuthorities` in the `CurrentAuthoritySet` (we use `MaxEncodedLen`
@@ -313,7 +318,7 @@ pub mod pallet {
 
 	/// The current GRANDPA Authority set.
 	#[pallet::storage]
-	pub(super) type CurrentAuthoritySet<T: Config<I>, I: 'static = ()> =
+	pub type CurrentAuthoritySet<T: Config<I>, I: 'static = ()> =
 		StorageValue<_, StoredAuthoritySet<T, I>, ValueQuery>;
 
 	/// Optional pallet owner.
@@ -504,7 +509,7 @@ pub mod pallet {
 			init_params;
 		let authority_set_length = authority_list.len();
 		let authority_set = StoredAuthoritySet::<T, I>::try_new(authority_list, set_id)
-			.map_err(|_| {
+			.map_err(|e| {
 				log::error!(
 					target: LOG_TARGET,
 					"Failed to initialize bridge. Number of authorities in the set {} is larger than the configured value {}",
@@ -512,7 +517,7 @@ pub mod pallet {
 					T::BridgedChain::MAX_AUTHORITIES_COUNT,
 				);
 
-				Error::TooManyAuthoritiesInSet
+				e
 			})?;
 		let initial_hash = header.hash();
 
@@ -630,8 +635,8 @@ pub fn initialize_for_benchmarks<T: Config<I>, I: 'static>(header: BridgedHeader
 mod tests {
 	use super::*;
 	use crate::mock::{
-		run_test, test_header, RuntimeOrigin, TestHeader, TestNumber, TestRuntime,
-		MAX_BRIDGED_AUTHORITIES,
+		run_test, test_header, RuntimeOrigin, TestBridgedChain, TestHeader, TestNumber,
+		TestRuntime, MAX_BRIDGED_AUTHORITIES,
 	};
 	use bp_header_chain::BridgeGrandpaCall;
 	use bp_runtime::BasicOperatingMode;
@@ -965,6 +970,64 @@ mod tests {
 		})
 	}
 
+	#[test]
+	fn relayer_pays_tx_fee_when_submitting_huge_mandatory_header() {
+		run_test(|| {
+			initialize_substrate_bridge();
+
+			// let's prepare a huge authorities change header, which is definitely above size limits
+			let mut header = test_header(2);
+			header.digest = change_log(0);
+			header.digest.push(DigestItem::Other(vec![42u8; 1024 * 1024]));
+			let justification = make_default_justification(&header);
+
+			// without large digest item ^^^ the relayer would have paid zero transaction fee
+			// (`Pays::No`)
+			let result = Pallet::<TestRuntime>::submit_finality_proof(
+				RuntimeOrigin::signed(1),
+				Box::new(header.clone()),
+				justification,
+			);
+			assert_ok!(result);
+			assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes);
+
+			// Make sure that our header is the best finalized
+			assert_eq!(<BestFinalized<TestRuntime>>::get().unwrap().1, header.hash());
+			assert!(<ImportedHeaders<TestRuntime>>::contains_key(header.hash()));
+		})
+	}
+
+	#[test]
+	fn relayer_pays_tx_fee_when_submitting_justification_with_long_ancestry_votes() {
+		run_test(|| {
+			initialize_substrate_bridge();
+
+			// let's prepare a huge authorities change header, which is definitely above weight
+			// limits
+			let mut header = test_header(2);
+			header.digest = change_log(0);
+			let justification = make_justification_for_header(JustificationGeneratorParams {
+				header: header.clone(),
+				ancestors: TestBridgedChain::REASONABLE_HEADERS_IN_JUSTIFICATON_ANCESTRY + 1,
+				..Default::default()
+			});
+
+			// without many headers in votes ancestries ^^^ the relayer would have paid zero
+			// transaction fee (`Pays::No`)
+			let result = Pallet::<TestRuntime>::submit_finality_proof(
+				RuntimeOrigin::signed(1),
+				Box::new(header.clone()),
+				justification,
+			);
+			assert_ok!(result);
+			assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes);
+
+			// Make sure that our header is the best finalized
+			assert_eq!(<BestFinalized<TestRuntime>>::get().unwrap().1, header.hash());
+			assert!(<ImportedHeaders<TestRuntime>>::contains_key(header.hash()));
+		})
+	}
+
 	#[test]
 	fn importing_header_rejects_header_with_scheduled_change_delay() {
 		run_test(|| {

bridges/modules/grandpa/src/storage_types.rs

@@ -16,7 +16,7 @@
 
 //! Wrappers for public types that are implementing `MaxEncodedLen`
 
-use crate::Config;
+use crate::{Config, Error};
 
 use bp_header_chain::{AuthoritySet, ChainWithGrandpa};
 use codec::{Decode, Encode, MaxEncodedLen};
@@ -52,8 +52,12 @@ impl<T: Config<I>, I: 'static> StoredAuthoritySet<T, I> {
 	/// Try to create a new bounded GRANDPA Authority Set from unbounded list.
 	///
 	/// Returns error if number of authorities in the provided list is too large.
-	pub fn try_new(authorities: AuthorityList, set_id: SetId) -> Result<Self, ()> {
-		Ok(Self { authorities: TryFrom::try_from(authorities).map_err(drop)?, set_id })
+	pub fn try_new(authorities: AuthorityList, set_id: SetId) -> Result<Self, Error<T, I>> {
+		Ok(Self {
+			authorities: TryFrom::try_from(authorities)
+				.map_err(|_| Error::TooManyAuthoritiesInSet)?,
+			set_id,
+		})
 	}
 
 	/// Returns number of bytes that may be subtracted from the PoV component of