Weight+size limits for bridge GRANDPA pallet calls (paritytech#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 <[email protected]>

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

Co-authored-by: Adrian Catangiu <[email protected]>

* reverted unnecessary change

* GrandpaJustification::max_reasonable_size

---------

Co-authored-by: Adrian Catangiu <[email protected]>

bin/runtime-common/src/refund_relayer_extension.rs

@@ -28,9 +28,12 @@ use codec::{Decode, Encode};
 use frame_support::{
 	dispatch::{CallableCallFor, DispatchInfo, Dispatchable, PostDispatchInfo},
 	traits::IsSubType,
+	weights::Weight,
 	CloneNoBound, DefaultNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound,
 };
-use pallet_bridge_grandpa::{CallSubType as GrandpaCallSubType, SubmitFinalityProofHelper};
+use pallet_bridge_grandpa::{
+	CallSubType as GrandpaCallSubType, SubmitFinalityProofHelper, SubmitFinalityProofInfo,
+};
 use pallet_bridge_messages::Config as MessagesConfig;
 use pallet_bridge_parachains::{
 	BoundedBridgeGrandpaConfig, CallSubType as ParachainsCallSubType, Config as ParachainsConfig,
@@ -140,7 +143,11 @@ pub struct PreDispatchData<AccountId> {
 #[derive(RuntimeDebugNoBound, PartialEq)]
 pub enum CallInfo {
 	/// Relay chain finality + parachain finality + message delivery calls.
-	AllFinalityAndDelivery(RelayBlockNumber, SubmitParachainHeadsInfo, ReceiveMessagesProofInfo),
+	AllFinalityAndDelivery(
+		SubmitFinalityProofInfo<RelayBlockNumber>,
+		SubmitParachainHeadsInfo,
+		ReceiveMessagesProofInfo,
+	),
 	/// Parachain finality + message delivery calls.
 	ParachainFinalityAndDelivery(SubmitParachainHeadsInfo, ReceiveMessagesProofInfo),
 	/// Standalone message delivery call.
@@ -149,7 +156,7 @@ pub enum CallInfo {
 
 impl CallInfo {
 	/// Returns the pre-dispatch `finality_target` sent to the `SubmitFinalityProof` call.
-	fn submit_finality_proof_info(&self) -> Option<RelayBlockNumber> {
+	fn submit_finality_proof_info(&self) -> Option<SubmitFinalityProofInfo<RelayBlockNumber>> {
 		match *self {
 			Self::AllFinalityAndDelivery(info, _, _) => Some(info),
 			_ => None,
@@ -318,6 +325,9 @@ where
 		len: usize,
 		result: &DispatchResult,
 	) -> Result<(), TransactionValidityError> {
+		let mut extra_weight = Weight::zero();
+		let mut extra_size = 0;
+
 		// We don't refund anything if the transaction has failed.
 		if result.is_err() {
 			return Ok(())
@@ -330,8 +340,10 @@ where
 		};
 
 		// check if relay chain state has been updated
-		if let Some(relay_block_number) = call_info.submit_finality_proof_info() {
-			if !SubmitFinalityProofHelper::<Runtime, Runtime::BridgesGrandpaPalletInstance>::was_successful(relay_block_number) {
+		if let Some(finality_proof_info) = call_info.submit_finality_proof_info() {
+			if !SubmitFinalityProofHelper::<Runtime, Runtime::BridgesGrandpaPalletInstance>::was_successful(
+				finality_proof_info.block_number,
+			) {
 				// we only refund relayer if all calls have updated chain state
 				return Ok(())
 			}
@@ -342,6 +354,11 @@ where
 			// `utility.batchAll` transaction always requires payment. But in both cases we'll
 			// refund relayer - either explicitly here, or using `Pays::No` if he's choosing
 			// to submit dedicated transaction.
+
+			// submitter has means to include extra weight/bytes in the `submit_finality_proof`
+			// call, so let's subtract extra weight/size to avoid refunding for this extra stuff
+			extra_weight = finality_proof_info.extra_weight;
+			extra_size = finality_proof_info.extra_size;
 		}
 
 		// check if parachain state has been updated
@@ -370,8 +387,15 @@ where
 		// cost of this attack is nothing. Hence we use zero as tip here.
 		let tip = Zero::zero();
 
+		// decrease post-dispatch weight/size using extra weight/size that we know now
+		let post_info_len = len.saturating_sub(extra_size as usize);
+		let mut post_info = *post_info;
+		post_info.actual_weight =
+			Some(post_info.actual_weight.unwrap_or(info.weight).saturating_sub(extra_weight));
+
 		// compute the relayer refund
-		let refund = Refund::compute_refund(info, post_info, len, tip);
+		let refund = Refund::compute_refund(info, &post_info, post_info_len, tip);
+
 		// finally - register refund in relayers pallet
 		RelayersPallet::<Runtime>::register_relayer_reward(Msgs::Id::get(), &relayer, refund);
 
@@ -397,9 +421,9 @@ mod tests {
 	use bp_parachains::{BestParaHeadHash, ParaInfo};
 	use bp_polkadot_core::parachains::{ParaHash, ParaHeadsProof, ParaId};
 	use bp_runtime::HeaderId;
-	use bp_test_utils::make_default_justification;
+	use bp_test_utils::{make_default_justification, test_keyring};
 	use frame_support::{assert_storage_noop, parameter_types, weights::Weight};
-	use pallet_bridge_grandpa::Call as GrandpaCall;
+	use pallet_bridge_grandpa::{Call as GrandpaCall, StoredAuthoritySet};
 	use pallet_bridge_messages::Call as MessagesCall;
 	use pallet_bridge_parachains::{Call as ParachainsCall, RelayBlockHash};
 	use sp_runtime::{
@@ -434,7 +458,11 @@ mod tests {
 		parachain_head_hash: ParaHash,
 		best_delivered_message: MessageNonce,
 	) {
+		let authorities = test_keyring().into_iter().map(|(a, w)| (a.into(), w)).collect();
 		let best_relay_header = HeaderId(best_relay_header_number, RelayBlockHash::default());
+		pallet_bridge_grandpa::CurrentAuthoritySet::<TestRuntime>::put(
+			StoredAuthoritySet::try_new(authorities, 0).unwrap(),
+		);
 		pallet_bridge_grandpa::BestFinalized::<TestRuntime>::put(best_relay_header);
 
 		let para_id = ParaId(TestParachain::get());
@@ -524,7 +552,11 @@ mod tests {
 		PreDispatchData {
 			relayer: relayer_account_at_this_chain(),
 			call_info: CallInfo::AllFinalityAndDelivery(
-				200,
+				SubmitFinalityProofInfo {
+					block_number: 200,
+					extra_weight: Weight::zero(),
+					extra_size: 0,
+				},
 				SubmitParachainHeadsInfo {
 					at_relay_block_number: 200,
 					para_id: ParaId(TestParachain::get()),
@@ -823,6 +855,54 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn post_dispatch_refunds_relayer_in_all_finality_batch_with_extra_weight() {
+		run_test(|| {
+			initialize_environment(200, 200, [1u8; 32].into(), 200);
+
+			let mut dispatch_info = dispatch_info();
+			dispatch_info.weight = Weight::from_ref_time(
+				frame_support::weights::constants::WEIGHT_REF_TIME_PER_SECOND * 2,
+			);
+
+			// without any size/weight refund: we expect regular reward
+			let pre_dispatch_data = all_finality_pre_dispatch_data();
+			let regular_reward = expected_reward();
+			run_post_dispatch(Some(pre_dispatch_data), Ok(()));
+			assert_eq!(
+				RelayersPallet::<TestRuntime>::relayer_reward(
+					relayer_account_at_this_chain(),
+					TestLaneId::get()
+				),
+				Some(regular_reward),
+			);
+
+			// now repeat the same with size+weight refund: we expect smaller reward
+			let mut pre_dispatch_data = all_finality_pre_dispatch_data();
+			match pre_dispatch_data.call_info {
+				CallInfo::AllFinalityAndDelivery(ref mut info, ..) => {
+					info.extra_weight.set_ref_time(
+						frame_support::weights::constants::WEIGHT_REF_TIME_PER_SECOND,
+					);
+					info.extra_size = 32;
+				},
+				_ => unreachable!(),
+			}
+			run_post_dispatch(Some(pre_dispatch_data), Ok(()));
+			let reward_after_two_calls = RelayersPallet::<TestRuntime>::relayer_reward(
+				relayer_account_at_this_chain(),
+				TestLaneId::get(),
+			)
+			.unwrap();
+			assert!(
+				reward_after_two_calls < 2 * regular_reward,
+				"{}  must be < 2 * {}",
+				reward_after_two_calls,
+				2 * regular_reward,
+			);
+		});
+	}
+
 	#[test]
 	fn post_dispatch_refunds_relayer_in_all_finality_batch() {
 		run_test(|| {

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);
+	}
 }