From bfa90f8b81046b2af5b9082ff641d33f2cccabd7 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 10 Apr 2023 13:43:37 +0300 Subject: [PATCH] Boost message delivery transaction priority (#2023) * reject delivery transactions with at least one obsolete message * clippy * boost priority of message delivery transactions: transaction with more messages has larger priority than the transaction with less messages * apply review suggestion * CallInfo::bundled_messages * validate_does_not_boost_priority_of_message_delivery_transactons_with_too_many_messages * clippy --- bridges/bin/millau/runtime/Cargo.toml | 2 +- bridges/bin/millau/runtime/src/lib.rs | 2 + .../runtime/src/rialto_parachain_messages.rs | 70 +++++ bridges/bin/runtime-common/Cargo.toml | 1 + bridges/bin/runtime-common/src/lib.rs | 1 + .../runtime-common/src/messages_call_ext.rs | 24 +- bridges/bin/runtime-common/src/mock.rs | 2 +- .../runtime-common/src/priority_calculator.rs | 201 +++++++++++++++ .../src/refund_relayer_extension.rs | 243 +++++++++++++----- 9 files changed, 481 insertions(+), 65 deletions(-) create mode 100644 bridges/bin/runtime-common/src/priority_calculator.rs diff --git a/bridges/bin/millau/runtime/Cargo.toml b/bridges/bin/millau/runtime/Cargo.toml index ce1484fecad94..e1a55ea6f247e 100644 --- a/bridges/bin/millau/runtime/Cargo.toml +++ b/bridges/bin/millau/runtime/Cargo.toml @@ -69,7 +69,7 @@ xcm-builder = { git = "https://github.com/paritytech/polkadot", branch = "master xcm-executor = { git = "https://github.com/paritytech/polkadot", branch = "master", default-features = false } [dev-dependencies] -bridge-runtime-common = { path = "../../runtime-common", features = ["integrity-test"] } +bridge-runtime-common = { path = "../../runtime-common", features = ["integrity-test", "std"] } env_logger = "0.10" static_assertions = "1.1" diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index deff859a81641..4e6f1e43e8c6d 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -589,11 +589,13 @@ generate_bridge_reject_obsolete_headers_and_messages! { bp_runtime::generate_static_str_provider!(BridgeRefundRialtoPara2000Lane0Msgs); /// Signed extension that refunds relayers that are delivering messages from the Rialto parachain. +pub type PriorityBoostPerMessage = ConstU64<921_900_294>; pub type BridgeRefundRialtoParachainMessages = RefundBridgedParachainMessages< Runtime, RefundableParachain, RefundableMessagesLane, ActualFeeRefund, + PriorityBoostPerMessage, StrBridgeRefundRialtoPara2000Lane0Msgs, >; diff --git a/bridges/bin/millau/runtime/src/rialto_parachain_messages.rs b/bridges/bin/millau/runtime/src/rialto_parachain_messages.rs index b3d33c1cefd70..bef8a281188e8 100644 --- a/bridges/bin/millau/runtime/src/rialto_parachain_messages.rs +++ b/bridges/bin/millau/runtime/src/rialto_parachain_messages.rs @@ -137,3 +137,73 @@ impl XcmBlobHauler for ToRialtoParachainXcmBlobHauler { XCM_LANE } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{ + PriorityBoostPerMessage, RialtoGrandpaInstance, Runtime, + WithRialtoParachainMessagesInstance, + }; + + use bridge_runtime_common::{ + assert_complete_bridge_types, + integrity::{ + assert_complete_bridge_constants, check_message_lane_weights, + AssertBridgeMessagesPalletConstants, AssertBridgePalletNames, AssertChainConstants, + AssertCompleteBridgeConstants, + }, + }; + + #[test] + fn ensure_millau_message_lane_weights_are_correct() { + check_message_lane_weights::( + bp_rialto_parachain::EXTRA_STORAGE_PROOF_SIZE, + bp_millau::MAX_UNREWARDED_RELAYERS_IN_CONFIRMATION_TX, + bp_millau::MAX_UNCONFIRMED_MESSAGES_IN_CONFIRMATION_TX, + ); + } + + #[test] + fn ensure_bridge_integrity() { + assert_complete_bridge_types!( + runtime: Runtime, + with_bridged_chain_grandpa_instance: RialtoGrandpaInstance, + with_bridged_chain_messages_instance: WithRialtoParachainMessagesInstance, + bridge: WithRialtoParachainMessageBridge, + this_chain: bp_millau::Millau, + bridged_chain: bp_rialto::Rialto, + ); + + assert_complete_bridge_constants::< + Runtime, + RialtoGrandpaInstance, + WithRialtoParachainMessagesInstance, + WithRialtoParachainMessageBridge, + >(AssertCompleteBridgeConstants { + this_chain_constants: AssertChainConstants { + block_length: bp_millau::BlockLength::get(), + block_weights: bp_millau::BlockWeights::get(), + }, + messages_pallet_constants: AssertBridgeMessagesPalletConstants { + max_unrewarded_relayers_in_bridged_confirmation_tx: + bp_rialto_parachain::MAX_UNREWARDED_RELAYERS_IN_CONFIRMATION_TX, + max_unconfirmed_messages_in_bridged_confirmation_tx: + bp_rialto_parachain::MAX_UNCONFIRMED_MESSAGES_IN_CONFIRMATION_TX, + bridged_chain_id: bp_runtime::RIALTO_PARACHAIN_CHAIN_ID, + }, + pallet_names: AssertBridgePalletNames { + with_this_chain_messages_pallet_name: bp_millau::WITH_MILLAU_MESSAGES_PALLET_NAME, + with_bridged_chain_grandpa_pallet_name: bp_rialto::WITH_RIALTO_GRANDPA_PALLET_NAME, + with_bridged_chain_messages_pallet_name: + bp_rialto_parachain::WITH_RIALTO_PARACHAIN_MESSAGES_PALLET_NAME, + }, + }); + + bridge_runtime_common::priority_calculator::ensure_priority_boost_is_sane::< + Runtime, + WithRialtoParachainMessagesInstance, + PriorityBoostPerMessage, + >(1_000_000); + } +} diff --git a/bridges/bin/runtime-common/Cargo.toml b/bridges/bin/runtime-common/Cargo.toml index 3db4ae9abca6e..9d616bf588e45 100644 --- a/bridges/bin/runtime-common/Cargo.toml +++ b/bridges/bin/runtime-common/Cargo.toml @@ -47,6 +47,7 @@ xcm-executor = { git = "https://github.com/paritytech/polkadot", branch = "maste [dev-dependencies] bp-test-utils = { path = "../../primitives/test-utils" } +bitvec = { version = "1", features = ["alloc"] } pallet-balances = { git = "https://github.com/paritytech/substrate", branch = "master" } [features] diff --git a/bridges/bin/runtime-common/src/lib.rs b/bridges/bin/runtime-common/src/lib.rs index ed486c04abccb..e8a2d2470fa19 100644 --- a/bridges/bin/runtime-common/src/lib.rs +++ b/bridges/bin/runtime-common/src/lib.rs @@ -30,6 +30,7 @@ pub mod messages_benchmarking; pub mod messages_call_ext; pub mod messages_xcm_extension; pub mod parachains_benchmarking; +pub mod priority_calculator; pub mod refund_relayer_extension; mod messages_generation; diff --git a/bridges/bin/runtime-common/src/messages_call_ext.rs b/bridges/bin/runtime-common/src/messages_call_ext.rs index b299d758a323e..8d53c4844c80f 100644 --- a/bridges/bin/runtime-common/src/messages_call_ext.rs +++ b/bridges/bin/runtime-common/src/messages_call_ext.rs @@ -104,6 +104,22 @@ pub enum CallInfo { ReceiveMessagesDeliveryProof(ReceiveMessagesDeliveryProofInfo), } +impl CallInfo { + /// Returns number of messages, bundled with this transaction. + pub fn bundled_messages(&self) -> MessageNonce { + let bundled_range = match *self { + Self::ReceiveMessagesProof(ref info) => &info.0.bundled_range, + Self::ReceiveMessagesDeliveryProof(ref info) => &info.0.bundled_range, + }; + + bundled_range + .end() + .checked_sub(*bundled_range.start()) + .map(|d| d.saturating_add(1)) + .unwrap_or(0) + } +} + /// Helper struct that provides methods for working with a call supported by `CallInfo`. pub struct CallHelper, I: 'static> { pub _phantom_data: sp_std::marker::PhantomData<(T, I)>, @@ -321,6 +337,7 @@ mod tests { TestRuntime, ThisChainRuntimeCall, }, }; + use bitvec::prelude::*; use bp_messages::{DeliveredMessages, UnrewardedRelayer, UnrewardedRelayersState}; use sp_std::ops::RangeInclusive; @@ -330,7 +347,11 @@ mod tests { for n in 0..MaxUnrewardedRelayerEntriesAtInboundLane::get() { inbound_lane_state.relayers.push_back(UnrewardedRelayer { relayer: Default::default(), - messages: DeliveredMessages { begin: n + 1, end: n + 1 }, + messages: DeliveredMessages { + begin: n + 1, + end: n + 1, + dispatch_results: bitvec![u8, Msb0; 1; 1], + }, }); } pallet_bridge_messages::InboundLanes::::insert( @@ -347,6 +368,7 @@ mod tests { messages: DeliveredMessages { begin: 1, end: MaxUnconfirmedMessagesAtInboundLane::get(), + dispatch_results: bitvec![u8, Msb0; 1; MaxUnconfirmedMessagesAtInboundLane::get() as _], }, }); pallet_bridge_messages::InboundLanes::::insert( diff --git a/bridges/bin/runtime-common/src/mock.rs b/bridges/bin/runtime-common/src/mock.rs index 967682ccbc513..036813f6fd514 100644 --- a/bridges/bin/runtime-common/src/mock.rs +++ b/bridges/bin/runtime-common/src/mock.rs @@ -127,7 +127,7 @@ parameter_types! { pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000u128); pub MaximumMultiplier: Multiplier = sp_runtime::traits::Bounded::max_value(); pub const MaxUnrewardedRelayerEntriesAtInboundLane: MessageNonce = 16; - pub const MaxUnconfirmedMessagesAtInboundLane: MessageNonce = 32; + pub const MaxUnconfirmedMessagesAtInboundLane: MessageNonce = 1_000; } impl frame_system::Config for TestRuntime { diff --git a/bridges/bin/runtime-common/src/priority_calculator.rs b/bridges/bin/runtime-common/src/priority_calculator.rs new file mode 100644 index 0000000000000..590de05fb1c66 --- /dev/null +++ b/bridges/bin/runtime-common/src/priority_calculator.rs @@ -0,0 +1,201 @@ +// Copyright 2021 Parity Technologies (UK) Ltd. +// This file is part of Parity Bridges Common. + +// Parity Bridges Common is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Parity Bridges Common is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Parity Bridges Common. If not, see . + +//! Bridge transaction priority calculator. +//! +//! We want to prioritize message delivery transactions with more messages over +//! transactions with less messages. That's because we reject delivery transactions +//! if it contains already delivered message. And if some transaction delivers +//! single message with nonce `N`, then the transaction with nonces `N..=N+100` will +//! be rejected. This can lower bridge throughput down to one message per block. + +use bp_messages::MessageNonce; +use frame_support::traits::Get; +use sp_runtime::transaction_validity::TransactionPriority; + +// reexport everything from `integrity_tests` module +pub use integrity_tests::*; + +/// Compute priority boost for message delivery transaction that delivers +/// given number of messages. +pub fn compute_priority_boost( + messages: MessageNonce, +) -> TransactionPriority +where + PriorityBoostPerMessage: Get, +{ + // we don't want any boost for transaction with single message => minus one + PriorityBoostPerMessage::get().saturating_mul(messages - 1) +} + +#[cfg(not(feature = "integrity-test"))] +mod integrity_tests {} + +#[cfg(feature = "integrity-test")] +mod integrity_tests { + use super::compute_priority_boost; + + use bp_messages::MessageNonce; + use bp_runtime::PreComputedSize; + use frame_support::{ + dispatch::{DispatchClass, DispatchInfo, Dispatchable, Pays, PostDispatchInfo}, + traits::Get, + }; + use pallet_bridge_messages::WeightInfoExt; + use pallet_transaction_payment::OnChargeTransaction; + use sp_runtime::{ + traits::{UniqueSaturatedInto, Zero}, + transaction_validity::TransactionPriority, + FixedPointOperand, SaturatedConversion, Saturating, + }; + + type BalanceOf = + <::OnChargeTransaction as OnChargeTransaction< + T, + >>::Balance; + + /// Ensures that the value of `PriorityBoostPerMessage` matches the value of + /// `tip_boost_per_message`. + /// + /// We want two transactions, `TX1` with `N` messages and `TX2` with `N+1` messages, have almost + /// the same priority if we'll add `tip_boost_per_message` tip to the `TX1`. We want to be sure + /// that if we add plain `PriorityBoostPerMessage` priority to `TX1`, the priority will be close + /// to `TX2` as well. + pub fn ensure_priority_boost_is_sane( + tip_boost_per_message: BalanceOf, + ) where + Runtime: + pallet_transaction_payment::Config + pallet_bridge_messages::Config, + MessagesInstance: 'static, + PriorityBoostPerMessage: Get, + Runtime::RuntimeCall: Dispatchable, + BalanceOf: Send + Sync + FixedPointOperand, + { + let priority_boost_per_message = PriorityBoostPerMessage::get(); + let maximal_messages_in_delivery_transaction = + Runtime::MaxUnconfirmedMessagesAtInboundLane::get(); + for messages in 1..=maximal_messages_in_delivery_transaction { + let base_priority = estimate_message_delivery_transaction_priority::< + Runtime, + MessagesInstance, + >(messages, Zero::zero()); + let priority_boost = compute_priority_boost::(messages); + let priority_with_boost = base_priority + priority_boost; + + let tip = tip_boost_per_message.saturating_mul((messages - 1).unique_saturated_into()); + let priority_with_tip = + estimate_message_delivery_transaction_priority::(1, tip); + + const ERROR_MARGIN: TransactionPriority = 5; // 5% + if priority_with_boost.abs_diff(priority_with_tip).saturating_mul(100) / + priority_with_tip > + ERROR_MARGIN + { + panic!( + "The PriorityBoostPerMessage value ({}) must be fixed to: {}", + priority_boost_per_message, + compute_priority_boost_per_message::( + tip_boost_per_message + ), + ); + } + } + } + + /// Compute priority boost that we give to message delivery transaction for additional message. + #[cfg(feature = "integrity-test")] + fn compute_priority_boost_per_message( + tip_boost_per_message: BalanceOf, + ) -> TransactionPriority + where + Runtime: + pallet_transaction_payment::Config + pallet_bridge_messages::Config, + MessagesInstance: 'static, + Runtime::RuntimeCall: Dispatchable, + BalanceOf: Send + Sync + FixedPointOperand, + { + // esimate priority of transaction that delivers one message and has large tip + let maximal_messages_in_delivery_transaction = + Runtime::MaxUnconfirmedMessagesAtInboundLane::get(); + let small_with_tip_priority = + estimate_message_delivery_transaction_priority::( + 1, + tip_boost_per_message + .saturating_mul(maximal_messages_in_delivery_transaction.saturated_into()), + ); + // estimate priority of transaction that delivers maximal number of messages, but has no tip + let large_without_tip_priority = estimate_message_delivery_transaction_priority::< + Runtime, + MessagesInstance, + >(maximal_messages_in_delivery_transaction, Zero::zero()); + + small_with_tip_priority + .saturating_sub(large_without_tip_priority) + .saturating_div(maximal_messages_in_delivery_transaction - 1) + } + + /// Estimate message delivery transaction priority. + #[cfg(feature = "integrity-test")] + fn estimate_message_delivery_transaction_priority( + messages: MessageNonce, + tip: BalanceOf, + ) -> TransactionPriority + where + Runtime: + pallet_transaction_payment::Config + pallet_bridge_messages::Config, + MessagesInstance: 'static, + Runtime::RuntimeCall: Dispatchable, + BalanceOf: Send + Sync + FixedPointOperand, + { + // just an estimation of extra transaction bytes that are added to every transaction + // (including signature, signed extensions extra and etc + in our case it includes + // all call arguments extept the proof itself) + let base_tx_size = 512; + // let's say we are relaying similar small messages and for every message we add more trie + // nodes to the proof (x0.5 because we expect some nodes to be reused) + let estimated_message_size = 512; + // let's say all our messages have the same dispatch weight + let estimated_message_dispatch_weight = + Runtime::WeightInfo::message_dispatch_weight(estimated_message_size); + // messages proof argument size is (for every message) messages size + some additional + // trie nodes. Some of them are reused by different messages, so let's take 2/3 of default + // "overhead" constant + let messages_proof_size = Runtime::WeightInfo::expected_extra_storage_proof_size() + .saturating_mul(2) + .saturating_div(3) + .saturating_add(estimated_message_size) + .saturating_mul(messages as _); + + // finally we are able to estimate transaction size and weight + let transaction_size = base_tx_size.saturating_add(messages_proof_size); + let transaction_weight = Runtime::WeightInfo::receive_messages_proof_weight( + &PreComputedSize(transaction_size as _), + messages as _, + estimated_message_dispatch_weight.saturating_mul(messages), + ); + + pallet_transaction_payment::ChargeTransactionPayment::::get_priority( + &DispatchInfo { + weight: transaction_weight, + class: DispatchClass::Normal, + pays_fee: Pays::Yes, + }, + transaction_size as _, + tip, + Zero::zero(), + ) + } +} diff --git a/bridges/bin/runtime-common/src/refund_relayer_extension.rs b/bridges/bin/runtime-common/src/refund_relayer_extension.rs index ebfc0b3346602..83eafa1a36a2d 100644 --- a/bridges/bin/runtime-common/src/refund_relayer_extension.rs +++ b/bridges/bin/runtime-common/src/refund_relayer_extension.rs @@ -46,7 +46,9 @@ use pallet_utility::{Call as UtilityCall, Config as UtilityConfig, Pallet as Uti use scale_info::TypeInfo; use sp_runtime::{ traits::{DispatchInfoOf, Get, PostDispatchInfoOf, SignedExtension, Zero}, - transaction_validity::{TransactionValidity, TransactionValidityError, ValidTransaction}, + transaction_validity::{ + TransactionPriority, TransactionValidity, TransactionValidityError, ValidTransactionBuilder, + }, DispatchResult, FixedPointOperand, }; use sp_std::{marker::PhantomData, vec, vec::Vec}; @@ -58,7 +60,7 @@ type CallOf = ::RuntimeCall; /// Trait identifying a bridged parachain. A relayer might be refunded for delivering messages /// coming from this parachain. -trait RefundableParachainId { +pub trait RefundableParachainId { /// The instance of the bridge parachains pallet. type Instance; /// The parachain Id. @@ -78,7 +80,7 @@ where /// Trait identifying a bridged messages lane. A relayer might be refunded for delivering messages /// coming from this lane. -trait RefundableMessagesLaneId { +pub trait RefundableMessagesLaneId { /// The instance of the bridge messages pallet. type Instance; /// The messages lane id. @@ -201,36 +203,75 @@ impl CallInfo { RuntimeDebugNoBound, TypeInfo, )] -#[scale_info(skip_type_params(Runtime, Para, Msgs, Refund, Id))] -pub struct RefundBridgedParachainMessages( - PhantomData<(Runtime, Para, Msgs, Refund, Id)>, +#[scale_info(skip_type_params(Runtime, Para, Msgs, Refund, Priority, Id))] +pub struct RefundBridgedParachainMessages( + PhantomData<(Runtime, Para, Msgs, Refund, Priority, Id)>, ); -impl - RefundBridgedParachainMessages +impl + RefundBridgedParachainMessages where - Runtime: UtilityConfig>, - CallOf: IsSubType, Runtime>>, + Self: 'static + Send + Sync, + Runtime: UtilityConfig> + + BoundedBridgeGrandpaConfig + + ParachainsConfig + + MessagesConfig, + Para: RefundableParachainId, + Msgs: RefundableMessagesLaneId, + CallOf: Dispatchable + + IsSubType, Runtime>> + + GrandpaCallSubType + + ParachainsCallSubType + + MessagesCallSubType, { - fn expand_call<'a>(&self, call: &'a CallOf) -> Option>> { - let calls = match call.is_sub_type() { - Some(UtilityCall::::batch_all { ref calls }) => { - if calls.len() > 3 { - return None - } - - calls.iter().collect() - }, - Some(_) => return None, + fn expand_call<'a>(&self, call: &'a CallOf) -> Vec<&'a CallOf> { + match call.is_sub_type() { + Some(UtilityCall::::batch_all { ref calls }) if calls.len() <= 3 => + calls.iter().collect(), + Some(_) => vec![], None => vec![call], - }; + } + } + + fn parse_and_check_for_obsolete_call( + &self, + call: &CallOf, + ) -> Result, TransactionValidityError> { + let calls = self.expand_call(call); + let total_calls = calls.len(); + let mut calls = calls.into_iter().map(Self::check_obsolete_call).rev(); + + let msgs_call = calls.next().transpose()?.and_then(|c| c.call_info_for(Msgs::Id::get())); + let para_finality_call = calls + .next() + .transpose()? + .and_then(|c| c.submit_parachain_heads_info_for(Para::Id::get())); + let relay_finality_call = + calls.next().transpose()?.and_then(|c| c.submit_finality_proof_info()); + + Ok(match (total_calls, relay_finality_call, para_finality_call, msgs_call) { + (3, Some(relay_finality_call), Some(para_finality_call), Some(msgs_call)) => Some( + CallInfo::AllFinalityAndMsgs(relay_finality_call, para_finality_call, msgs_call), + ), + (2, None, Some(para_finality_call), Some(msgs_call)) => + Some(CallInfo::ParachainFinalityAndMsgs(para_finality_call, msgs_call)), + (1, None, None, Some(msgs_call)) => Some(CallInfo::Msgs(msgs_call)), + _ => None, + }) + } - Some(calls) + fn check_obsolete_call( + call: &CallOf, + ) -> Result<&CallOf, TransactionValidityError> { + call.check_obsolete_submit_finality_proof()?; + call.check_obsolete_submit_parachain_heads()?; + call.check_obsolete_call()?; + Ok(call) } } -impl SignedExtension - for RefundBridgedParachainMessages +impl SignedExtension + for RefundBridgedParachainMessages where Self: 'static + Send + Sync, Runtime: UtilityConfig> @@ -241,6 +282,7 @@ where Para: RefundableParachainId, Msgs: RefundableMessagesLaneId, Refund: RefundCalculator, + Priority: Get, Id: StaticStrProvider, CallOf: Dispatchable + IsSubType, Runtime>> @@ -265,46 +307,46 @@ where _info: &DispatchInfoOf, _len: usize, ) -> TransactionValidity { - if let Some(calls) = self.expand_call(call) { - for nested_call in calls { - nested_call.check_obsolete_submit_finality_proof()?; - nested_call.check_obsolete_submit_parachain_heads()?; - nested_call.check_obsolete_call()?; + // this is the only relevant line of code for the `pre_dispatch` + // + // we're not calling `validato` from `pre_dispatch` directly because of performance + // reasons, so if you're adding some code that may fail here, please check if it needs + // to be added to the `pre_dispatch` as well + let parsed_call = self.parse_and_check_for_obsolete_call(call)?; + + // the following code just plays with transaction priority and never returns an error + let mut valid_transaction = ValidTransactionBuilder::default(); + if let Some(parsed_call) = parsed_call { + // we give delivery transactions some boost, that depends on number of messages inside + let messages_call_info = parsed_call.messages_call_info(); + if let MessagesCallInfo::ReceiveMessagesProof(_) = messages_call_info { + // compute total number of messages in transaction + let bundled_messages = messages_call_info.bundled_messages(); + + // a quick check to avoid invalid high-priority transactions + if bundled_messages <= Runtime::MaxUnconfirmedMessagesAtInboundLane::get() { + let priority_boost = crate::priority_calculator::compute_priority_boost::< + Priority, + >(bundled_messages); + valid_transaction = valid_transaction.priority(priority_boost); + } } } - Ok(ValidTransaction::default()) + valid_transaction.build() } fn pre_dispatch( self, who: &Self::AccountId, call: &Self::Call, - info: &DispatchInfoOf, - len: usize, + _info: &DispatchInfoOf, + _len: usize, ) -> Result { - // reject batch transactions with obsolete headers - self.validate(who, call, info, len).map(drop)?; - - // Try to check if the tx matches one of types we support. - let parse_call = || { - let mut calls = self.expand_call(call)?.into_iter(); - match calls.len() { - 3 => Some(CallInfo::AllFinalityAndMsgs( - calls.next()?.submit_finality_proof_info()?, - calls.next()?.submit_parachain_heads_info_for(Para::Id::get())?, - calls.next()?.call_info_for(Msgs::Id::get())?, - )), - 2 => Some(CallInfo::ParachainFinalityAndMsgs( - calls.next()?.submit_parachain_heads_info_for(Para::Id::get())?, - calls.next()?.call_info_for(Msgs::Id::get())?, - )), - 1 => Some(CallInfo::Msgs(calls.next()?.call_info_for(Msgs::Id::get())?)), - _ => None, - } - }; + // this is a relevant piece of `validate` that we need here (in `pre_dispatch`) + let parsed_call = self.parse_and_check_for_obsolete_call(call)?; - Ok(parse_call().map(|call_info| { + Ok(parsed_call.map(|call_info| { log::trace!( target: "runtime::bridge", "{} from parachain {} via {:?} parsed bridge transaction in pre-dispatch: {:?}", @@ -475,14 +517,24 @@ mod tests { use pallet_bridge_messages::Call as MessagesCall; use pallet_bridge_parachains::{Call as ParachainsCall, RelayBlockHash}; use sp_runtime::{ - traits::Header as HeaderT, transaction_validity::InvalidTransaction, DispatchError, + traits::{ConstU64, Header as HeaderT}, + transaction_validity::{InvalidTransaction, ValidTransaction}, + DispatchError, }; parameter_types! { TestParachain: u32 = 1000; pub TestLaneId: LaneId = TEST_LANE_ID; - pub MsgProofsRewardsAccount: RewardsAccountParams = RewardsAccountParams::new(TEST_LANE_ID, TEST_BRIDGED_CHAIN_ID, RewardsAccountOwner::ThisChain); - pub MsgDeliveryProofsRewardsAccount: RewardsAccountParams = RewardsAccountParams::new(TEST_LANE_ID, TEST_BRIDGED_CHAIN_ID, RewardsAccountOwner::BridgedChain); + pub MsgProofsRewardsAccount: RewardsAccountParams = RewardsAccountParams::new( + TEST_LANE_ID, + TEST_BRIDGED_CHAIN_ID, + RewardsAccountOwner::ThisChain, + ); + pub MsgDeliveryProofsRewardsAccount: RewardsAccountParams = RewardsAccountParams::new( + TEST_LANE_ID, + TEST_BRIDGED_CHAIN_ID, + RewardsAccountOwner::BridgedChain, + ); } bp_runtime::generate_static_str_provider!(TestExtension); @@ -491,6 +543,7 @@ mod tests { RefundableParachain<(), TestParachain>, RefundableMessagesLane<(), TestLaneId>, ActualFeeRefund, + ConstU64<1>, StrTestExtension, >; @@ -831,32 +884,98 @@ mod tests { ) } + #[test] + fn validate_boosts_priority_of_message_delivery_transactons() { + run_test(|| { + initialize_environment(100, 100, Default::default(), 100); + + let priority_of_100_messages_delivery = + run_validate(message_delivery_call(200)).unwrap().priority; + let priority_of_200_messages_delivery = + run_validate(message_delivery_call(300)).unwrap().priority; + assert!( + priority_of_200_messages_delivery > priority_of_100_messages_delivery, + "Invalid priorities: {} for 200 messages vs {} for 100 messages", + priority_of_200_messages_delivery, + priority_of_100_messages_delivery, + ); + + let priority_of_100_messages_confirmation = + run_validate(message_confirmation_call(200)).unwrap().priority; + let priority_of_200_messages_confirmation = + run_validate(message_confirmation_call(300)).unwrap().priority; + assert_eq!( + priority_of_100_messages_confirmation, + priority_of_200_messages_confirmation + ); + }); + } + + #[test] + fn validate_does_not_boost_priority_of_message_delivery_transactons_with_too_many_messages() { + run_test(|| { + initialize_environment(100, 100, Default::default(), 100); + + let priority_of_max_messages_delivery = run_validate(message_delivery_call( + 100 + MaxUnconfirmedMessagesAtInboundLane::get(), + )) + .unwrap() + .priority; + let priority_of_more_than_max_messages_delivery = run_validate(message_delivery_call( + 100 + MaxUnconfirmedMessagesAtInboundLane::get() + 1, + )) + .unwrap() + .priority; + + assert!( + priority_of_max_messages_delivery > priority_of_more_than_max_messages_delivery, + "Invalid priorities: {} for MAX messages vs {} for MAX+1 messages", + priority_of_max_messages_delivery, + priority_of_more_than_max_messages_delivery, + ); + }); + } + #[test] fn validate_allows_non_obsolete_transactions() { run_test(|| { initialize_environment(100, 100, Default::default(), 100); - assert_eq!(run_validate(message_delivery_call(200)), Ok(ValidTransaction::default()),); + fn run_validate_ignore_priority(call: RuntimeCall) -> TransactionValidity { + run_validate(call).map(|mut tx| { + tx.priority = 0; + tx + }) + } + assert_eq!( - run_validate(message_confirmation_call(200)), + run_validate_ignore_priority(message_delivery_call(200)), + Ok(ValidTransaction::default()), + ); + assert_eq!( + run_validate_ignore_priority(message_confirmation_call(200)), Ok(ValidTransaction::default()), ); assert_eq!( - run_validate(parachain_finality_and_delivery_batch_call(200, 200)), + run_validate_ignore_priority(parachain_finality_and_delivery_batch_call(200, 200)), Ok(ValidTransaction::default()), ); assert_eq!( - run_validate(parachain_finality_and_confirmation_batch_call(200, 200)), + run_validate_ignore_priority(parachain_finality_and_confirmation_batch_call( + 200, 200 + )), Ok(ValidTransaction::default()), ); assert_eq!( - run_validate(all_finality_and_delivery_batch_call(200, 200, 200)), + run_validate_ignore_priority(all_finality_and_delivery_batch_call(200, 200, 200)), Ok(ValidTransaction::default()), ); assert_eq!( - run_validate(all_finality_and_confirmation_batch_call(200, 200, 200)), + run_validate_ignore_priority(all_finality_and_confirmation_batch_call( + 200, 200, 200 + )), Ok(ValidTransaction::default()), ); });