From e9ff50a2a44d94b8e77fe66731596fbf0741f4ec Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 23 May 2024 10:00:53 +0300 Subject: [PATCH] check submit_finality_proof limits before submission --- bridges/modules/grandpa/src/call_ext.rs | 56 ++++----------- bridges/primitives/header-chain/src/lib.rs | 68 ++++++++++++++++++- bridges/relays/client-substrate/src/error.rs | 7 ++ .../src/finality/target.rs | 10 +++ .../src/finality_base/engine.rs | 44 +++--------- .../src/on_demand/headers.rs | 13 ++-- 6 files changed, 111 insertions(+), 87 deletions(-) diff --git a/bridges/modules/grandpa/src/call_ext.rs b/bridges/modules/grandpa/src/call_ext.rs index 98fbeaa30bba..f08eb4c5d1ab 100644 --- a/bridges/modules/grandpa/src/call_ext.rs +++ b/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, I: 'static>( current_set_id: Option, is_free_execution_expected: bool, ) -> SubmitFinalityProofInfo> { - 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::(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::>::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::( - 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, } } diff --git a/bridges/primitives/header-chain/src/lib.rs b/bridges/primitives/header-chain/src/lib.rs index ad496012c6a3..af2afb65a26a 100644 --- a/bridges/primitives/header-chain/src/lib.rs +++ b/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 = ::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( + header: &C::Header, + proof: &justification::GrandpaJustification, +) -> 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::>::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::( + 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( is_mandatory_finality_target: bool, diff --git a/bridges/relays/client-substrate/src/error.rs b/bridges/relays/client-substrate/src/error.rs index 0b4466818188..2133c1888784 100644 --- a/bridges/relays/client-substrate/src/error.rs +++ b/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), diff --git a/bridges/relays/lib-substrate-relay/src/finality/target.rs b/bridges/relays/lib-substrate-relay/src/finality/target.rs index 0874fa53549c..52ab2462c62c 100644 --- a/bridges/relays/lib-substrate-relay/src/finality/target.rs +++ b/bridges/relays/lib-substrate-relay/src/finality/target.rs @@ -137,6 +137,16 @@ impl TargetClient: Send { @@ -129,12 +115,11 @@ pub trait Engine: Send { ) -> Result; /// 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 Engine for Grandpa { }) } - 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::( - 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::(header, proof) } /// Prepare initialization data for the GRANDPA verifier pallet. diff --git a/bridges/relays/lib-substrate-relay/src/on_demand/headers.rs b/bridges/relays/lib-substrate-relay/src/on_demand/headers.rs index 74f3a70c5e81..202f53ea4e4f 100644 --- a/bridges/relays/lib-substrate-relay/src/on_demand/headers.rs +++ b/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 OnDemandRelay