Bridge: check submit_finality_proof limits before submission (paritytech#4549)

closes paritytech/parity-bridges-common#2982
closes paritytech/parity-bridges-common#2730

The main change is in the
bridges/relays/lib-substrate-relay/src/finality/target.rs, changes in
other files are just moving the code

~I haven't been able to run zn tests locally - don't know why, but it
keeps failing for me locally with: `
Error running script:
/home/svyatonik/dev/polkadot-sdk/bridges/testing/framework/js-helpers/wait-hrmp-channel-opened.js
Error: Timeout(300), "custom-js
/home/svyatonik/dev/polkadot-sdk/bridges/testing/framework/js-helpers/wait-hrmp-channel-opened.js
within 300 secs" didn't complete on time.`~ The issue was an obsolete
`polkadot-js-api` binary - did `yarn global upgrade` and it is ok now

Author: svyatonik

bridges/modules/grandpa/src/call_ext.rs

@@ -18,12 +18,8 @@ use crate::{
 	weights::WeightInfo, BestFinalized, BridgedBlockNumber, BridgedHeader, Config,
 	CurrentAuthoritySet, Error, FreeHeadersRemaining, Pallet,
 };
-use bp_header_chain::{
-	justification::GrandpaJustification, max_expected_submit_finality_proof_arguments_size,
-	ChainWithGrandpa, GrandpaConsensusLogReader,
-};
+use bp_header_chain::{justification::GrandpaJustification, submit_finality_proof_limits_extras};
 use bp_runtime::{BlockNumberOf, Chain, OwnedBridgeModule};
-use codec::Encode;
 use frame_support::{
 	dispatch::CallableCallFor,
 	traits::{Get, IsSubType},
@@ -303,53 +299,31 @@ pub(crate) fn submit_finality_proof_info_from_args<T: Config<I>, I: 'static>(
 	current_set_id: Option<SetId>,
 	is_free_execution_expected: bool,
 ) -> 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;
+	// check if call exceeds limits. In other words - whether some size or weight is included
+	// in the call
+	let extras =
+		submit_finality_proof_limits_extras::<T::BridgedChain>(finality_target, justification);
 
 	// 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_JUSTIFICATION_ANCESTRY {
-			T::WeightInfo::submit_finality_proof(precommits_len, votes_ancestries_len)
-		} else {
-			Weight::zero()
-		};
-
-	// check if the `finality_target` is a mandatory header. If so, we are ready to refund larger
-	// size
-	let is_mandatory_finality_target =
-		GrandpaConsensusLogReader::<BridgedBlockNumber<T, I>>::find_scheduled_change(
-			finality_target.digest(),
-		)
-		.is_some();
-
-	// 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_submit_finality_proof_arguments_size::<T::BridgedChain>(
-		is_mandatory_finality_target,
-		required_precommits,
-	);
-	let extra_size = actual_call_size.saturating_sub(max_expected_call_size);
+	let extra_weight = if extras.is_weight_limit_exceeded {
+		let precommits_len = justification.commit.precommits.len().saturated_into();
+		let votes_ancestries_len = justification.votes_ancestries.len().saturated_into();
+		T::WeightInfo::submit_finality_proof(precommits_len, votes_ancestries_len)
+	} else {
+		Weight::zero()
+	};
 
 	SubmitFinalityProofInfo {
-		block_number,
+		block_number: *finality_target.number(),
 		current_set_id,
-		is_mandatory: is_mandatory_finality_target,
+		is_mandatory: extras.is_mandatory_finality_target,
 		is_free_execution_expected,
 		extra_weight,
-		extra_size,
+		extra_size: extras.extra_size,
 	}
 }
 

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

@@ -24,8 +24,8 @@ use crate::justification::{
 	GrandpaJustification, JustificationVerificationContext, JustificationVerificationError,
 };
 use bp_runtime::{
-	BasicOperatingMode, Chain, HashOf, HasherOf, HeaderOf, RawStorageProof, StorageProofChecker,
-	StorageProofError, UnderlyingChainProvider,
+	BasicOperatingMode, BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf, RawStorageProof,
+	StorageProofChecker, StorageProofError, UnderlyingChainProvider,
 };
 use codec::{Codec, Decode, Encode, EncodeLike, MaxEncodedLen};
 use core::{clone::Clone, cmp::Eq, default::Default, fmt::Debug};
@@ -35,7 +35,7 @@ use serde::{Deserialize, Serialize};
 use sp_consensus_grandpa::{
 	AuthorityList, ConsensusLog, ScheduledChange, SetId, GRANDPA_ENGINE_ID,
 };
-use sp_runtime::{traits::Header as HeaderT, Digest, RuntimeDebug};
+use sp_runtime::{traits::Header as HeaderT, Digest, RuntimeDebug, SaturatedConversion};
 use sp_std::{boxed::Box, vec::Vec};
 
 pub mod justification;
@@ -325,6 +325,68 @@ where
 	const AVERAGE_HEADER_SIZE: u32 = <T::Chain as ChainWithGrandpa>::AVERAGE_HEADER_SIZE;
 }
 
+/// Result of checking maximal expected submit finality proof call weight and size.
+#[derive(Debug)]
+pub struct SubmitFinalityProofCallExtras {
+	/// If true, the call weight is larger than what we have assumed.
+	///
+	/// 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 is_weight_limit_exceeded: bool,
+	/// 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,
+	/// A flag that is true if the header is the mandatory header that enacts new
+	/// authorities set.
+	pub is_mandatory_finality_target: bool,
+}
+
+/// Checks whether the given `header` and its finality `proof` fit the maximal expected
+/// call limits (size and weight). The submission may be refunded sometimes (see pallet
+/// configuration for details), but it should fit some limits. If the call has some extra
+/// weight and/or size included, though, we won't refund it or refund will be partial.
+pub fn submit_finality_proof_limits_extras<C: ChainWithGrandpa>(
+	header: &C::Header,
+	proof: &justification::GrandpaJustification<C::Header>,
+) -> SubmitFinalityProofCallExtras {
+	// 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 = proof.commit.precommits.len().saturated_into();
+	let required_precommits = precommits_len;
+
+	// the weight check is simple - we assume that there are no more than the `limit`
+	// headers in the ancestry proof
+	let votes_ancestries_len: u32 = proof.votes_ancestries.len().saturated_into();
+	let is_weight_limit_exceeded =
+		votes_ancestries_len > C::REASONABLE_HEADERS_IN_JUSTIFICATION_ANCESTRY;
+
+	// check if the `finality_target` is a mandatory header. If so, we are ready to refund larger
+	// size
+	let is_mandatory_finality_target =
+		GrandpaConsensusLogReader::<BlockNumberOf<C>>::find_scheduled_change(header.digest())
+			.is_some();
+
+	// we can estimate extra call size easily, without any additional significant overhead
+	let actual_call_size: u32 =
+		header.encoded_size().saturating_add(proof.encoded_size()).saturated_into();
+	let max_expected_call_size = max_expected_submit_finality_proof_arguments_size::<C>(
+		is_mandatory_finality_target,
+		required_precommits,
+	);
+	let extra_size = actual_call_size.saturating_sub(max_expected_call_size);
+
+	SubmitFinalityProofCallExtras {
+		is_weight_limit_exceeded,
+		extra_size,
+		is_mandatory_finality_target,
+	}
+}
+
 /// Returns maximal expected size of `submit_finality_proof` call arguments.
 pub fn max_expected_submit_finality_proof_arguments_size<C: ChainWithGrandpa>(
 	is_mandatory_finality_target: bool,

bridges/relays/client-substrate/src/error.rs

@@ -17,6 +17,7 @@
 //! Substrate node RPC errors.
 
 use crate::SimpleRuntimeVersion;
+use bp_header_chain::SubmitFinalityProofCallExtras;
 use bp_polkadot_core::parachains::ParaId;
 use jsonrpsee::core::ClientError as RpcError;
 use relay_utils::MaybeConnectionError;
@@ -129,6 +130,12 @@ pub enum Error {
 		/// Actual runtime version.
 		actual: SimpleRuntimeVersion,
 	},
+	/// Finality proof submission exceeds size and/or weight limits.
+	#[error("Finality proof submission exceeds limits: {extras:?}")]
+	FinalityProofWeightLimitExceeded {
+		/// Finality proof submission extras.
+		extras: SubmitFinalityProofCallExtras,
+	},
 	/// Custom logic error.
 	#[error("{0}")]
 	Custom(String),

bridges/relays/lib-substrate-relay/src/finality/target.rs

@@ -137,6 +137,16 @@ impl<P: SubstrateFinalitySyncPipeline> TargetClient<FinalitySyncPipelineAdapter<
 		let context =
 			P::FinalityEngine::verify_and_optimize_proof(&self.client, &header, &mut proof).await?;
 
+		// if free execution is expected, but the call size/weight exceeds hardcoded limits, the
+		// runtime may still accept the proof, but it may have some cost for relayer. Let's check
+		// it here to avoid losing relayer funds
+		if is_free_execution_expected {
+			let extras = P::FinalityEngine::check_max_expected_call_limits(&header, &proof);
+			if extras.is_weight_limit_exceeded || extras.extra_size != 0 {
+				return Err(Error::FinalityProofWeightLimitExceeded { extras })
+			}
+		}
+
 		// now we may submit optimized finality proof
 		let mortality = self.transaction_params.mortality;
 		let call = P::SubmitFinalityProofCallBuilder::build_submit_finality_proof_call(

bridges/relays/lib-substrate-relay/src/finality_base/engine.rs

@@ -23,9 +23,8 @@ use bp_header_chain::{
 		verify_and_optimize_justification, GrandpaEquivocationsFinder, GrandpaJustification,
 		JustificationVerificationContext,
 	},
-	max_expected_submit_finality_proof_arguments_size, AuthoritySet, ConsensusLogReader,
-	FinalityProof, FindEquivocations, GrandpaConsensusLogReader, HeaderFinalityInfo,
-	HeaderGrandpaInfo, StoredHeaderGrandpaInfo,
+	AuthoritySet, ConsensusLogReader, FinalityProof, FindEquivocations, GrandpaConsensusLogReader,
+	HeaderFinalityInfo, HeaderGrandpaInfo, StoredHeaderGrandpaInfo, SubmitFinalityProofCallExtras,
 };
 use bp_runtime::{BasicOperatingMode, HeaderIdProvider, OperatingMode};
 use codec::{Decode, Encode};
@@ -36,22 +35,9 @@ use relay_substrate_client::{
 };
 use sp_consensus_grandpa::{AuthorityList as GrandpaAuthoritiesSet, GRANDPA_ENGINE_ID};
 use sp_core::{storage::StorageKey, Bytes};
-use sp_runtime::{scale_info::TypeInfo, traits::Header, ConsensusEngineId, SaturatedConversion};
+use sp_runtime::{scale_info::TypeInfo, traits::Header, ConsensusEngineId};
 use std::{fmt::Debug, marker::PhantomData};
 
-/// Result of checking maximal expected call size.
-pub enum MaxExpectedCallSizeCheck {
-	/// Size is ok and call will be refunded.
-	Ok,
-	/// The call size exceeds the maximal expected and relayer will only get partial refund.
-	Exceeds {
-		/// Actual call size.
-		call_size: u32,
-		/// Maximal expected call size.
-		max_call_size: u32,
-	},
-}
-
 /// Finality engine, used by the Substrate chain.
 #[async_trait]
 pub trait Engine<C: Chain>: Send {
@@ -129,12 +115,11 @@ pub trait Engine<C: Chain>: Send {
 	) -> Result<Self::FinalityVerificationContext, SubstrateError>;
 
 	/// Checks whether the given `header` and its finality `proof` fit the maximal expected
-	/// call size limit. If result is `MaxExpectedCallSizeCheck::Exceeds { .. }`, this
-	/// submission won't be fully refunded and relayer will spend its own funds on that.
-	fn check_max_expected_call_size(
+	/// call limits (size and weight).
+	fn check_max_expected_call_limits(
 		header: &C::Header,
 		proof: &Self::FinalityProof,
-	) -> MaxExpectedCallSizeCheck;
+	) -> SubmitFinalityProofCallExtras;
 
 	/// Prepare initialization data for the finality bridge pallet.
 	async fn prepare_initialization_data(
@@ -245,22 +230,11 @@ impl<C: ChainWithGrandpa> Engine<C> for Grandpa<C> {
 		})
 	}
 
-	fn check_max_expected_call_size(
+	fn check_max_expected_call_limits(
 		header: &C::Header,
 		proof: &Self::FinalityProof,
-	) -> MaxExpectedCallSizeCheck {
-		let is_mandatory = Self::ConsensusLogReader::schedules_authorities_change(header.digest());
-		let call_size: u32 =
-			header.encoded_size().saturating_add(proof.encoded_size()).saturated_into();
-		let max_call_size = max_expected_submit_finality_proof_arguments_size::<C>(
-			is_mandatory,
-			proof.commit.precommits.len().saturated_into(),
-		);
-		if call_size > max_call_size {
-			MaxExpectedCallSizeCheck::Exceeds { call_size, max_call_size }
-		} else {
-			MaxExpectedCallSizeCheck::Ok
-		}
+	) -> SubmitFinalityProofCallExtras {
+		bp_header_chain::submit_finality_proof_limits_extras::<C>(header, proof)
 	}
 
 	/// Prepare initialization data for the GRANDPA verifier pallet.

bridges/relays/lib-substrate-relay/src/on_demand/headers.rs

@@ -16,9 +16,7 @@
 
 //! On-demand Substrate -> Substrate header finality relay.
 
-use crate::{
-	finality::SubmitFinalityProofCallBuilder, finality_base::engine::MaxExpectedCallSizeCheck,
-};
+use crate::finality::SubmitFinalityProofCallBuilder;
 
 use async_std::sync::{Arc, Mutex};
 use async_trait::async_trait;
@@ -156,22 +154,21 @@ impl<P: SubstrateFinalitySyncPipeline> OnDemandRelay<P::SourceChain, P::TargetCh
 
 			// now we have the header and its proof, but we want to minimize our losses, so let's
 			// check if we'll get the full refund for submitting this header
-			let check_result = P::FinalityEngine::check_max_expected_call_size(&header, &proof);
-			if let MaxExpectedCallSizeCheck::Exceeds { call_size, max_call_size } = check_result {
+			let check_result = P::FinalityEngine::check_max_expected_call_limits(&header, &proof);
+			if check_result.is_weight_limit_exceeded || check_result.extra_size != 0 {
 				iterations += 1;
 				current_required_header = header_id.number().saturating_add(One::one());
 				if iterations < MAX_ITERATIONS {
 					log::debug!(
 						target: "bridge",
-						"[{}] Requested to prove {} head {:?}. Selected to prove {} head {:?}. But it is too large: {} vs {}. \
+						"[{}] Requested to prove {} head {:?}. Selected to prove {} head {:?}. But it exceeds limits: {:?}. \
 						Going to select next header",
 						self.relay_task_name,
 						P::SourceChain::NAME,
 						required_header,
 						P::SourceChain::NAME,
 						header_id,
-						call_size,
-						max_call_size,
+						check_result,
 					);
 
 					continue;