From 594e3c0ead8f4ddf0b5af4c1b975558665ebd18a Mon Sep 17 00:00:00 2001 From: bear Date: Mon, 13 Jun 2022 16:32:59 +0800 Subject: [PATCH] Enhance dispatch module (#121) --- bin/runtime-common/src/messages.rs | 540 +++++++++++++++++++------ modules/dispatch/src/lib.rs | 143 ++----- primitives/message-dispatch/src/lib.rs | 50 +-- 3 files changed, 462 insertions(+), 271 deletions(-) diff --git a/bin/runtime-common/src/messages.rs b/bin/runtime-common/src/messages.rs index 2df1cb4c9..f4d184a05 100644 --- a/bin/runtime-common/src/messages.rs +++ b/bin/runtime-common/src/messages.rs @@ -26,18 +26,21 @@ use bp_messages::{ target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages}, InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData, }; -use bp_runtime::{messages::MessageDispatchResult, ChainId, Size, StorageProofChecker}; +use bp_runtime::{ + messages::{DispatchFeePayment, MessageDispatchResult}, + ChainId, Size, StorageProofChecker, +}; use codec::{Decode, DecodeLimit, Encode}; use frame_support::{ traits::{Currency, ExistenceRequirement}, - weights::{Weight, WeightToFee}, + weights::{Weight, WeightToFeePolynomial}, RuntimeDebug, }; use hash_db::Hasher; use scale_info::TypeInfo; use sp_runtime::{ - traits::{CheckedAdd, CheckedDiv, CheckedMul, Saturating, Zero}, - FixedPointNumber, FixedPointOperand, + traits::{AtLeast32BitUnsigned, CheckedAdd, CheckedDiv, CheckedMul, Saturating, Zero}, + FixedPointNumber, FixedPointOperand, FixedU128, }; use sp_std::{ cmp::PartialOrd, convert::TryFrom, fmt::Debug, marker::PhantomData, ops::RangeInclusive, @@ -63,6 +66,12 @@ pub trait MessageBridge { type ThisChain: ThisChainWithMessages; /// Bridged chain in context of message bridge. type BridgedChain: BridgedChainWithMessages; + + /// Convert Bridged chain balance into This chain balance. + fn bridged_balance_to_this_balance( + bridged_balance: BalanceOf>, + bridged_to_this_conversion_rate_override: Option, + ) -> BalanceOf>; } /// Chain that has `pallet-bridge-messages` and `dispatch` modules. @@ -91,6 +100,15 @@ pub trait ChainWithMessages { + Copy; } +/// Message related transaction parameters estimation. +#[derive(RuntimeDebug)] +pub struct MessageTransaction { + /// The estimated dispatch weight of the transaction. + pub dispatch_weight: Weight, + /// The estimated size of the encoded transaction. + pub size: u32, +} + /// This chain that has `pallet-bridge-messages` and `dispatch` modules. pub trait ThisChainWithMessages: ChainWithMessages { /// Call origin on the chain. @@ -105,6 +123,12 @@ pub trait ThisChainWithMessages: ChainWithMessages { /// /// Any messages over this limit, will be rejected. fn maximal_pending_messages_at_outbound_lane() -> MessageNonce; + + /// Estimate size and weight of single message delivery confirmation transaction at This chain. + fn estimate_delivery_confirmation_transaction() -> MessageTransaction>; + + /// Returns minimal transaction fee that must be paid for given transaction at This chain. + fn transaction_payment(transaction: MessageTransaction>) -> BalanceOf; } /// Bridged chain that has `pallet-bridge-messages` and `dispatch` modules. @@ -122,6 +146,17 @@ pub trait BridgedChainWithMessages: ChainWithMessages { /// already accounted by the `weight_of_delivery_transaction`. So this function should /// return pure call dispatch weights range. fn message_weight_limits(message_payload: &[u8]) -> RangeInclusive; + + /// Estimate size and weight of single message delivery transaction at the Bridged chain. + fn estimate_delivery_transaction( + message_payload: &[u8], + include_pay_dispatch_fee_cost: bool, + message_dispatch_weight: WeightOf, + ) -> MessageTransaction>; + + /// Returns minimal transaction fee that must be paid for given transaction at the Bridged + /// chain. + fn transaction_payment(transaction: MessageTransaction>) -> BalanceOf; } /// This chain in context of message bridge. @@ -148,6 +183,33 @@ pub type CallOf = ::Call; /// Raw storage proof type (just raw trie nodes). pub type RawStorageProof = Vec>; +/// Compute fee of transaction at runtime where regular transaction payment pallet is being used. +/// +/// The value of `multiplier` parameter is the expected value of +/// `pallet_transaction_payment::NextFeeMultiplier` at the moment when transaction is submitted. If +/// you're charging this payment in advance (and that's what happens with delivery and confirmation +/// transaction in this crate), then there's a chance that the actual fee will be larger than what +/// is paid in advance. So the value must be chosen carefully. +pub fn transaction_payment( + base_extrinsic_weight: Weight, + per_byte_fee: Balance, + multiplier: FixedU128, + weight_to_fee: impl Fn(Weight) -> Balance, + transaction: MessageTransaction, +) -> Balance { + // base fee is charged for every tx + let base_fee = weight_to_fee(base_extrinsic_weight); + + // non-adjustable per-byte fee + let len_fee = per_byte_fee.saturating_mul(Balance::from(transaction.size)); + + // the adjustable part of the fee + let unadjusted_weight_fee = weight_to_fee(transaction.dispatch_weight); + let adjusted_weight_fee = multiplier.saturating_mul_int(unadjusted_weight_fee); + + base_fee.saturating_add(len_fee).saturating_add(adjusted_weight_fee) +} + /// Sub-module that is declaring types required for processing This -> Bridged chain messages. pub mod source { use super::*; @@ -207,7 +269,7 @@ pub mod source { /// dispatch origin; /// - check that the sender has paid enough funds for both message delivery and dispatch. #[derive(RuntimeDebug)] - pub struct FromThisChainMessageVerifier(PhantomData<(B, F, I)>); + pub struct FromThisChainMessageVerifier(PhantomData); /// The error message returned from LaneMessageVerifier when outbound lane is disabled. pub const MESSAGE_REJECTED_BY_OUTBOUND_LANE: &str = @@ -220,27 +282,23 @@ pub mod source { /// The error message returned from LaneMessageVerifier when the message fee is too low. pub const TOO_LOW_FEE: &str = "Provided fee is below minimal threshold required by the lane."; - impl + impl LaneMessageVerifier< OriginOf>, AccountIdOf>, FromThisChainMessagePayload, BalanceOf>, - > for FromThisChainMessageVerifier + > for FromThisChainMessageVerifier where B: MessageBridge, - F: pallet_fee_market::Config, - I: 'static, // matches requirements from the `frame_system::Config::Origin` OriginOf>: Clone + Into>>, OriginOf>>>, AccountIdOf>: PartialEq + Clone, - pallet_fee_market::BalanceOf: From>>, { type Error = &'static str; #[allow(clippy::single_match)] - #[cfg(not(feature = "runtime-benchmarks"))] fn verify_message( submitter: &OriginOf>, delivery_and_dispatch_fee: &BalanceOf>, @@ -280,35 +338,19 @@ pub mod source { // is valid, or not. }; - // Do the delivery_and_dispatch_fee. We assume that the delivery and dispatch fee always - // greater than the fee market provided fee. - if let Some(market_fee) = pallet_fee_market::Pallet::::market_fee() { - let message_fee: pallet_fee_market::BalanceOf = - (*delivery_and_dispatch_fee).into(); + let minimal_fee_in_this_tokens = estimate_message_dispatch_and_delivery_fee::( + payload, + B::RELAYER_FEE_PERCENT, + None, + )?; - // compare with actual fee paid - if message_fee < market_fee { - return Err(TOO_LOW_FEE); - } - } else { - const NO_MARKET_FEE: &str = "The fee market are not ready for accepting messages."; - - return Err(NO_MARKET_FEE); + // compare with actual fee paid + if *delivery_and_dispatch_fee < minimal_fee_in_this_tokens { + return Err(TOO_LOW_FEE); } Ok(()) } - - #[cfg(feature = "runtime-benchmarks")] - fn verify_message( - _submitter: &OriginOf>, - _delivery_and_dispatch_fee: &BalanceOf>, - _lane: &LaneId, - _lane_outbound_data: &OutboundLaneData, - _payload: &FromThisChainMessagePayload, - ) -> Result<(), Self::Error> { - Ok(()) - } } /// Return maximal message size of This -> Bridged chain message. @@ -346,6 +388,54 @@ pub mod source { Ok(()) } + /// Estimate delivery and dispatch fee that must be paid for delivering a message to the Bridged + /// chain. + /// + /// The fee is paid in This chain Balance, but we use Bridged chain balance to avoid additional + /// conversions. Returns `None` if overflow has happened. + pub fn estimate_message_dispatch_and_delivery_fee( + payload: &FromThisChainMessagePayload, + relayer_fee_percent: u32, + bridged_to_this_conversion_rate: Option, + ) -> Result>, &'static str> { + // the fee (in Bridged tokens) of all transactions that are made on the Bridged chain + // + // if we're going to pay dispatch fee at the target chain, then we don't include weight + // of the message dispatch in the delivery transaction cost + let pay_dispatch_fee_at_target_chain = + payload.dispatch_fee_payment == DispatchFeePayment::AtTargetChain; + let delivery_transaction = BridgedChain::::estimate_delivery_transaction( + &payload.encode(), + pay_dispatch_fee_at_target_chain, + if pay_dispatch_fee_at_target_chain { 0.into() } else { payload.weight.into() }, + ); + let delivery_transaction_fee = BridgedChain::::transaction_payment(delivery_transaction); + + // the fee (in This tokens) of all transactions that are made on This chain + let confirmation_transaction = ThisChain::::estimate_delivery_confirmation_transaction(); + let confirmation_transaction_fee = + ThisChain::::transaction_payment(confirmation_transaction); + + // minimal fee (in This tokens) is a sum of all required fees + let minimal_fee = B::bridged_balance_to_this_balance( + delivery_transaction_fee, + bridged_to_this_conversion_rate, + ) + .checked_add(&confirmation_transaction_fee); + + // before returning, add extra fee that is paid to the relayer (relayer interest) + minimal_fee + .and_then(|fee| + // having message with fee that is near the `Balance::MAX_VALUE` of the chain is + // unlikely and should be treated as an error + // => let's do multiplication first + fee + .checked_mul(&relayer_fee_percent.into()) + .and_then(|interest| interest.checked_div(&100u32.into())) + .and_then(|interest| fee.checked_add(&interest))) + .ok_or("Overflow when computing minimal required message delivery and dispatch fee") + } + /// Verify proof of This -> Bridged chain messages delivery. pub fn verify_messages_delivery_proof( proof: FromBridgedChainMessagesDeliveryProof>>, @@ -433,7 +523,7 @@ pub mod target { /// /// Our Call is opaque (`Vec`) for Bridged chain. So it is encoded, prefixed with /// vector length. Custom decode implementation here is exactly to deal with this. - #[derive(Decode, Encode, Clone, RuntimeDebug, PartialEq)] + #[derive(Decode, Encode, RuntimeDebug, PartialEq)] pub struct FromBridgedChainEncodedMessageCall { encoded_call: Vec, _marker: PhantomData, @@ -446,87 +536,6 @@ pub mod target { } } - /// Dispatching Bridged -> This chain messages. - #[derive(RuntimeDebug, Clone, Copy)] - pub struct FromBridgedChainMessageDispatch { - _marker: PhantomData<(B, ThisRuntime, ThisCurrency, ThisDispatchInstance)>, - } - - impl - MessageDispatch>, BalanceOf>> - for FromBridgedChainMessageDispatch - where - BalanceOf>: Saturating + FixedPointOperand, - ThisDispatchInstance: 'static, - ThisRuntime: pallet_bridge_dispatch::Config< - ThisDispatchInstance, - BridgeMessageId = (LaneId, MessageNonce), - > + pallet_transaction_payment::Config, - ::OnChargeTransaction: - pallet_transaction_payment::OnChargeTransaction< - ThisRuntime, - Balance = BalanceOf>, - >, - ThisCurrency: Currency>, Balance = BalanceOf>>, - pallet_bridge_dispatch::Pallet: - bp_message_dispatch::MessageDispatch< - AccountIdOf>, - (LaneId, MessageNonce), - Message = FromBridgedChainMessagePayload, - >, - { - type DispatchPayload = FromBridgedChainMessagePayload; - - fn dispatch_weight( - message: &DispatchMessage>>, - ) -> frame_support::weights::Weight { - message.data.payload.as_ref().map(|payload| payload.weight).unwrap_or(0) - } - - fn pre_dispatch( - relayer_account: &AccountIdOf>, - message: &DispatchMessage>>, - ) -> Result<(), &'static str> { - pallet_bridge_dispatch::Pallet::::pre_dispatch( - relayer_account, - message.data.payload.as_ref().map_err(drop), - ) - } - - fn dispatch( - relayer_account: &AccountIdOf>, - message: DispatchMessage>>, - ) -> MessageDispatchResult { - let message_id = (message.key.lane_id, message.key.nonce); - pallet_bridge_dispatch::Pallet::::dispatch( - B::BRIDGED_CHAIN_ID, - B::THIS_CHAIN_ID, - relayer_account, - message_id, - message.data.payload.map_err(drop), - |dispatch_origin, dispatch_weight| { - let unadjusted_weight_fee = - ThisRuntime::WeightToFee::weight_to_fee(&dispatch_weight); - let fee_multiplier = - pallet_transaction_payment::Pallet::::next_fee_multiplier(); - let adjusted_weight_fee = - fee_multiplier.saturating_mul_int(unadjusted_weight_fee); - if !adjusted_weight_fee.is_zero() { - ThisCurrency::transfer( - dispatch_origin, - relayer_account, - adjusted_weight_fee, - ExistenceRequirement::AllowDeath, - ) - .map_err(drop) - } else { - Ok(()) - } - }, - ) - } - } - impl From> for Result { @@ -723,11 +732,15 @@ pub mod target { #[cfg(test)] mod tests { use super::*; - use bp_runtime::messages::DispatchFeePayment; use codec::{Decode, Encode}; use frame_support::weights::Weight; use std::ops::RangeInclusive; + const DELIVERY_TRANSACTION_WEIGHT: Weight = 100; + const DELIVERY_CONFIRMATION_TRANSACTION_WEIGHT: Weight = 100; + const THIS_CHAIN_WEIGHT_TO_BALANCE_RATE: Weight = 2; + const BRIDGED_CHAIN_WEIGHT_TO_BALANCE_RATE: Weight = 4; + const BRIDGED_CHAIN_TO_THIS_CHAIN_BALANCE_RATE: u32 = 6; const BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT: Weight = 2048; const BRIDGED_CHAIN_MAX_EXTRINSIC_SIZE: u32 = 1024; @@ -744,6 +757,16 @@ mod tests { const BRIDGED_MESSAGES_PALLET_NAME: &'static str = ""; const RELAYER_FEE_PERCENT: u32 = 10; const THIS_CHAIN_ID: ChainId = *b"this"; + + fn bridged_balance_to_this_balance( + bridged_balance: BridgedChainBalance, + bridged_to_this_conversion_rate_override: Option, + ) -> ThisChainBalance { + let conversion_rate = bridged_to_this_conversion_rate_override + .map(|r| r.to_float() as u32) + .unwrap_or(BRIDGED_CHAIN_TO_THIS_CHAIN_BALANCE_RATE); + ThisChainBalance(bridged_balance.0 * conversion_rate) + } } /// Bridge that is deployed on BridgedChain and allows sending/receiving messages to/from @@ -901,6 +924,19 @@ mod tests { fn maximal_pending_messages_at_outbound_lane() -> MessageNonce { MAXIMAL_PENDING_MESSAGES_AT_TEST_LANE } + + fn estimate_delivery_confirmation_transaction() -> MessageTransaction> { + MessageTransaction { + dispatch_weight: DELIVERY_CONFIRMATION_TRANSACTION_WEIGHT, + size: 0, + } + } + + fn transaction_payment(transaction: MessageTransaction>) -> BalanceOf { + ThisChainBalance( + transaction.dispatch_weight as u32 * THIS_CHAIN_WEIGHT_TO_BALANCE_RATE as u32, + ) + } } impl BridgedChainWithMessages for ThisChain { @@ -911,6 +947,20 @@ mod tests { fn message_weight_limits(_message_payload: &[u8]) -> RangeInclusive { unreachable!() } + + fn estimate_delivery_transaction( + _message_payload: &[u8], + _include_pay_dispatch_fee_cost: bool, + _message_dispatch_weight: WeightOf, + ) -> MessageTransaction> { + unreachable!() + } + + fn transaction_payment( + _transaction: MessageTransaction>, + ) -> BalanceOf { + unreachable!() + } } struct BridgedChain; @@ -935,6 +985,16 @@ mod tests { fn maximal_pending_messages_at_outbound_lane() -> MessageNonce { unreachable!() } + + fn estimate_delivery_confirmation_transaction() -> MessageTransaction> { + unreachable!() + } + + fn transaction_payment( + _transaction: MessageTransaction>, + ) -> BalanceOf { + unreachable!() + } } impl BridgedChainWithMessages for BridgedChain { @@ -947,6 +1007,27 @@ mod tests { std::cmp::min(BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT, message_payload.len() as Weight); begin..=BRIDGED_CHAIN_MAX_EXTRINSIC_WEIGHT } + + fn estimate_delivery_transaction( + _message_payload: &[u8], + _include_pay_dispatch_fee_cost: bool, + message_dispatch_weight: WeightOf, + ) -> MessageTransaction> { + MessageTransaction { + dispatch_weight: DELIVERY_TRANSACTION_WEIGHT + message_dispatch_weight, + size: 0, + } + } + + fn transaction_payment(transaction: MessageTransaction>) -> BalanceOf { + BridgedChainBalance( + transaction.dispatch_weight as u32 * BRIDGED_CHAIN_WEIGHT_TO_BALANCE_RATE as u32, + ) + } + } + + fn test_lane_outbound_data() -> OutboundLaneData { + OutboundLaneData::default() } #[test] @@ -986,6 +1067,180 @@ mod tests { const TEST_LANE_ID: &LaneId = b"test"; const MAXIMAL_PENDING_MESSAGES_AT_TEST_LANE: MessageNonce = 32; + fn regular_outbound_message_payload() -> source::FromThisChainMessagePayload + { + source::FromThisChainMessagePayload:: { + spec_version: 1, + weight: 100, + origin: bp_message_dispatch::CallOrigin::SourceRoot, + dispatch_fee_payment: DispatchFeePayment::AtSourceChain, + call: vec![42], + } + } + + #[test] + fn message_fee_is_checked_by_verifier() { + const EXPECTED_MINIMAL_FEE: u32 = 5500; + + // payload of the This -> Bridged chain message + let payload = regular_outbound_message_payload(); + + // let's check if estimation matching hardcoded value + assert_eq!( + source::estimate_message_dispatch_and_delivery_fee::( + &payload, + OnThisChainBridge::RELAYER_FEE_PERCENT, + None, + ), + Ok(ThisChainBalance(EXPECTED_MINIMAL_FEE)), + ); + + // let's check if estimation is less than hardcoded, if dispatch is paid at target chain + let mut payload_with_pay_on_target = regular_outbound_message_payload(); + payload_with_pay_on_target.dispatch_fee_payment = DispatchFeePayment::AtTargetChain; + let fee_at_source = + source::estimate_message_dispatch_and_delivery_fee::( + &payload_with_pay_on_target, + OnThisChainBridge::RELAYER_FEE_PERCENT, + None, + ) + .expect( + "estimate_message_dispatch_and_delivery_fee failed for pay-at-target-chain message", + ); + assert!( + fee_at_source < EXPECTED_MINIMAL_FEE.into(), + "Computed fee {:?} without prepaid dispatch must be less than the fee with prepaid dispatch {}", + fee_at_source, + EXPECTED_MINIMAL_FEE, + ); + + // and now check that the verifier checks the fee + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), + &ThisChainBalance(1), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ), + Err(source::TOO_LOW_FEE) + ); + assert!(source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ) + .is_ok(),); + } + + #[test] + fn should_disallow_root_calls_from_regular_accounts() { + // payload of the This -> Bridged chain message + let payload = source::FromThisChainMessagePayload:: { + spec_version: 1, + weight: 100, + origin: bp_message_dispatch::CallOrigin::SourceRoot, + dispatch_fee_payment: DispatchFeePayment::AtSourceChain, + call: vec![42], + }; + + // and now check that the verifier checks the fee + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Signed(ThisChainAccountId(0)))), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ), + Err(source::BAD_ORIGIN) + ); + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::None)), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ), + Err(source::BAD_ORIGIN) + ); + assert!(source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ) + .is_ok(),); + } + + #[test] + fn should_verify_source_and_target_origin_matching() { + // payload of the This -> Bridged chain message + let payload = source::FromThisChainMessagePayload:: { + spec_version: 1, + weight: 100, + origin: bp_message_dispatch::CallOrigin::SourceAccount(ThisChainAccountId(1)), + dispatch_fee_payment: DispatchFeePayment::AtSourceChain, + call: vec![42], + }; + + // and now check that the verifier checks the fee + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Signed(ThisChainAccountId(0)))), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ), + Err(source::BAD_ORIGIN) + ); + assert!(source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Signed(ThisChainAccountId(1)))), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &test_lane_outbound_data(), + &payload, + ) + .is_ok(),); + } + + #[test] + fn message_is_rejected_when_sent_using_disabled_lane() { + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), + &ThisChainBalance(1_000_000), + b"dsbl", + &test_lane_outbound_data(), + ®ular_outbound_message_payload(), + ), + Err(source::MESSAGE_REJECTED_BY_OUTBOUND_LANE) + ); + } + + #[test] + fn message_is_rejected_when_there_are_too_many_pending_messages_at_outbound_lane() { + assert_eq!( + source::FromThisChainMessageVerifier::::verify_message( + &ThisChainOrigin(Ok(frame_system::RawOrigin::Root)), + &ThisChainBalance(1_000_000), + TEST_LANE_ID, + &OutboundLaneData { + latest_received_nonce: 100, + latest_generated_nonce: 100 + MAXIMAL_PENDING_MESSAGES_AT_TEST_LANE + 1, + ..Default::default() + }, + ®ular_outbound_message_payload(), + ), + Err(source::TOO_MANY_PENDING_MESSAGES) + ); + } + #[test] fn verify_chain_message_rejects_message_with_too_small_declared_weight() { assert!(source::verify_chain_message::( @@ -1282,4 +1537,53 @@ mod tests { Err(target::MessageProofError::MessagesCountMismatch), ); } + + #[test] + fn transaction_payment_works_with_zero_multiplier() { + use sp_runtime::traits::Zero; + + assert_eq!( + transaction_payment( + 100, + 10, + FixedU128::zero(), + |weight| weight, + MessageTransaction { size: 50, dispatch_weight: 777 }, + ), + 100 + 50 * 10, + ); + } + + #[test] + fn transaction_payment_works_with_non_zero_multiplier() { + use sp_runtime::traits::One; + + assert_eq!( + transaction_payment( + 100, + 10, + FixedU128::one(), + |weight| weight, + MessageTransaction { size: 50, dispatch_weight: 777 }, + ), + 100 + 50 * 10 + 777, + ); + } + + #[test] + fn conversion_rate_override_works() { + let payload = regular_outbound_message_payload(); + let regular_fee = source::estimate_message_dispatch_and_delivery_fee::( + &payload, + OnThisChainBridge::RELAYER_FEE_PERCENT, + None, + ); + let overrided_fee = source::estimate_message_dispatch_and_delivery_fee::( + &payload, + OnThisChainBridge::RELAYER_FEE_PERCENT, + Some(FixedU128::from_float((BRIDGED_CHAIN_TO_THIS_CHAIN_BALANCE_RATE * 2) as f64)), + ); + + assert!(regular_fee < overrided_fee); + } } diff --git a/modules/dispatch/src/lib.rs b/modules/dispatch/src/lib.rs index d0f061cef..252894ff9 100644 --- a/modules/dispatch/src/lib.rs +++ b/modules/dispatch/src/lib.rs @@ -26,7 +26,7 @@ #![allow(clippy::unused_unit)] use bp_message_dispatch::{ - CallOrigin, CallValidate, IntoDispatchOrigin, MessageDispatch, MessagePayload, SpecVersion, + CallFilter, CallOrigin, IntoDispatchOrigin, MessageDispatch, MessagePayload, SpecVersion, }; use bp_runtime::{ derive_account_id, @@ -35,14 +35,13 @@ use bp_runtime::{ }; use codec::Encode; use frame_support::{ - dispatch::{DispatchInfo, DispatchResultWithPostInfo, Dispatchable, Weight}, + dispatch::Dispatchable, ensure, - pallet_prelude::Pays, traits::Get, - weights::GetDispatchInfo, + weights::{extract_actual_weight, GetDispatchInfo}, }; use frame_system::RawOrigin; -use sp_runtime::traits::{BadOrigin, Convert, IdentifyAccount, MaybeDisplay, Verify, Zero}; +use sp_runtime::traits::{BadOrigin, Convert, IdentifyAccount, MaybeDisplay, Verify}; use sp_std::{fmt::Debug, prelude::*}; pub use pallet::*; @@ -80,18 +79,18 @@ pub mod pallet { Origin = ::Origin, PostInfo = frame_support::dispatch::PostDispatchInfo, >; - /// Pre-dispatch validation for incoming calls. + /// Pre-dispatch filter for incoming calls. /// - /// The pallet will validate all incoming calls right before they're dispatched. If this - /// validator rejects the call, special event (`Event::MessageCallRejected`) is emitted. - type CallValidator: CallValidate>::Call>; + /// The pallet will filter all incoming calls right before they're dispatched. If this + /// filter rejects the call, special event (`Event::MessageCallRejected`) is emitted. + type CallFilter: CallFilter>::Call>; /// The type that is used to wrap the `Self::Call` when it is moved over bridge. /// /// The idea behind this is to avoid `Call` conversion/decoding until we'll be sure /// that all other stuff (like `spec_version`) is ok. If we would try to decode /// `Call` which has been encoded using previous `spec_version`, then we might end /// up with decoding error, instead of `MessageVersionSpecMismatch`. - type EncodedCall: Decode + Encode + Into>::Call, ()>> + Clone; + type EncodedCall: Decode + Encode + Into>::Call, ()>>; /// A type which can be turned into an AccountId from a 256-bit hash. /// /// Used when deriving target chain AccountIds from source chain AccountIds. @@ -147,7 +146,9 @@ pub mod pallet { } } -impl, I: 'static> MessageDispatch for Pallet { +impl, I: 'static> + MessageDispatch>::Call> for Pallet +{ type Message = MessagePayload< T::SourceChainAccountId, T::TargetChainAccountPublic, @@ -159,33 +160,9 @@ impl, I: 'static> MessageDispatch message.weight } - fn pre_dispatch( - relayer_account: &T::AccountId, - message: Result<&Self::Message, ()>, - ) -> Result<(), &'static str> { - match message { - Ok(raw_message) => - if let Ok(call) = raw_message.clone().call.into() { - return T::CallValidator::check_receiving_before_dispatch( - relayer_account, - &call, - ); - }, - Err(_) => { - log::trace!( - target: "runtime::bridge-dispatch", - "Message will be rejected in dispatch, still Ok here", - ); - }, - } - - Ok(()) - } - - fn dispatch Result<(), ()>>( + fn dispatch>::Call) -> Result<(), ()>>( source_chain: ChainId, target_chain: ChainId, - relayer_account: &T::AccountId, id: T::BridgeMessageId, message: Result, pay_dispatch_fee: P, @@ -297,8 +274,8 @@ impl, I: 'static> MessageDispatch let dispatch_origin = T::IntoDispatchOrigin::into_dispatch_origin(&origin_derived_account, &call); - // validate the call - if let Err(_e) = T::CallValidator::call_validate(relayer_account, &dispatch_origin, &call) { + // filter the call + if !T::CallFilter::contains(&dispatch_origin, &call) { log::trace!( target: "runtime::bridge-dispatch", "Message {:?}/{:?}: the call ({:?}) is rejected by filter", @@ -306,7 +283,6 @@ impl, I: 'static> MessageDispatch id, call, ); - // TODO: https://github.com/paritytech/substrate/pull/11599 Self::deposit_event(Event::MessageCallRejected(source_chain, id)); return dispatch_result; } @@ -337,9 +313,7 @@ impl, I: 'static> MessageDispatch // pay dispatch fee right before dispatch let pay_dispatch_fee_at_target_chain = message.dispatch_fee_payment == DispatchFeePayment::AtTargetChain; - if pay_dispatch_fee_at_target_chain - && pay_dispatch_fee(&origin_derived_account, message.weight).is_err() - { + if pay_dispatch_fee_at_target_chain && pay_dispatch_fee(&dispatch_origin, &call).is_err() { log::trace!( target: "runtime::bridge-dispatch", "Failed to pay dispatch fee for dispatching message {:?}/{:?} with weight {}", @@ -384,17 +358,6 @@ impl, I: 'static> MessageDispatch } } -fn extract_actual_weight(result: &DispatchResultWithPostInfo, info: &DispatchInfo) -> Weight { - let post_info = match result { - Ok(post_info) => &post_info, - Err(err) => &err.post_info, - }; - match post_info.pays_fee { - Pays::Yes => post_info.calc_actual_weight(info), - Pays::No => Weight::zero(), - } -} - /// Check if the message is allowed to be dispatched on the target chain given the sender's origin /// on the source chain. /// @@ -479,7 +442,6 @@ mod tests { use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, - transaction_validity::{InvalidTransaction, TransactionValidityError}, Perbill, }; @@ -573,7 +535,7 @@ mod tests { type AccountIdConverter = AccountIdConverter; type BridgeMessageId = BridgeMessageId; type Call = Call; - type CallValidator = CallValidator; + type CallFilter = TestCallFilter; type EncodedCall = EncodedCall; type Event = Event; type IntoDispatchOrigin = TestIntoDispatchOrigin; @@ -582,7 +544,7 @@ mod tests { type TargetChainSignature = TestSignature; } - #[derive(Decode, Encode, Clone)] + #[derive(Decode, Encode)] pub struct EncodedCall(Vec); impl From for Result { @@ -591,25 +553,11 @@ mod tests { } } - pub struct CallValidator; - impl CallValidate for CallValidator { - fn check_receiving_before_dispatch( - _relayer_account: &AccountId, - _call: &Call, - ) -> Result<(), &'static str> { - Ok(()) - } + pub struct TestCallFilter; - fn call_validate( - _relayer_account: &AccountId, - _origin: &Origin, - call: &Call, - ) -> Result<(), TransactionValidityError> { - match call { - Call::System(frame_system::Call::fill_block { .. }) => - Err(TransactionValidityError::Invalid(InvalidTransaction::Call)), - _ => Ok(()), - } + impl CallFilter for TestCallFilter { + fn contains(_origin: &Origin, call: &Call) -> bool { + !matches!(*call, Call::System(frame_system::Call::fill_block { .. })) } } @@ -633,8 +581,9 @@ mod tests { origin: CallOrigin, call: Call, ) -> as MessageDispatch< - AccountId, + ::Origin, ::BridgeMessageId, + ::Call, >>::Message { MessagePayload { spec_version: TEST_SPEC_VERSION, @@ -648,8 +597,9 @@ mod tests { fn prepare_root_message( call: Call, ) -> as MessageDispatch< - AccountId, + ::Origin, ::BridgeMessageId, + ::Call, >>::Message { prepare_message(CallOrigin::SourceRoot, call) } @@ -657,8 +607,9 @@ mod tests { fn prepare_target_message( call: Call, ) -> as MessageDispatch< - AccountId, + ::Origin, ::BridgeMessageId, + ::Call, >>::Message { let origin = CallOrigin::TargetAccount(1, TestAccountPublic(1), TestSignature(1)); prepare_message(origin, call) @@ -667,8 +618,9 @@ mod tests { fn prepare_source_message( call: Call, ) -> as MessageDispatch< - AccountId, + ::Origin, ::BridgeMessageId, + ::Call, >>::Message { let origin = CallOrigin::SourceAccount(1); prepare_message(origin, call) @@ -678,7 +630,6 @@ mod tests { fn should_fail_on_spec_version_mismatch() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; const BAD_SPEC_VERSION: SpecVersion = 99; let mut message = prepare_root_message(Call::System(frame_system::Call::remark { @@ -691,7 +642,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -721,7 +671,6 @@ mod tests { fn should_fail_on_weight_mismatch() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let call = Call::System(frame_system::Call::set_heap_pages { pages: 42 }); let call_weight = call.get_dispatch_info().weight; let mut message = prepare_root_message(call); @@ -732,7 +681,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -762,7 +710,6 @@ mod tests { fn should_fail_on_signature_mismatch() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let call_origin = CallOrigin::TargetAccount(1, TestAccountPublic(1), TestSignature(99)); let message = prepare_message( @@ -775,7 +722,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -803,13 +749,11 @@ mod tests { fn should_emit_event_for_rejected_messages() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; System::set_block_number(1); Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Err(()), |_, _| unreachable!(), @@ -833,7 +777,6 @@ mod tests { fn should_fail_on_call_decode() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let mut message = prepare_root_message(Call::System(frame_system::Call::remark { remark: vec![1, 2, 3], @@ -845,7 +788,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -873,7 +815,6 @@ mod tests { fn should_emit_event_for_rejected_calls() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let call = Call::System(frame_system::Call::fill_block { ratio: Perbill::from_percent(75) }); @@ -885,7 +826,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -913,7 +853,6 @@ mod tests { fn should_emit_event_for_unpaid_calls() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let mut message = prepare_root_message(Call::System(frame_system::Call::remark { remark: vec![1, 2, 3], @@ -922,14 +861,10 @@ mod tests { message.dispatch_fee_payment = DispatchFeePayment::AtTargetChain; System::set_block_number(1); - let result = Dispatch::dispatch( - SOURCE_CHAIN_ID, - TARGET_CHAIN_ID, - &relayer_account, - id, - Ok(message), - |_, _| Err(()), - ); + let result = + Dispatch::dispatch(SOURCE_CHAIN_ID, TARGET_CHAIN_ID, id, Ok(message), |_, _| { + Err(()) + }); assert_eq!(result.unspent_weight, weight); assert!(!result.dispatch_result); @@ -958,7 +893,6 @@ mod tests { fn should_dispatch_calls_paid_at_target_chain() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let mut message = prepare_root_message(Call::System(frame_system::Call::remark { remark: vec![1, 2, 3], @@ -969,7 +903,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| Ok(()), @@ -996,7 +929,6 @@ mod tests { fn should_return_dispatch_failed_flag_if_dispatch_happened_but_failed() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let call = Call::System(frame_system::Call::set_heap_pages { pages: 1 }); let message = prepare_target_message(call); @@ -1005,7 +937,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -1032,7 +963,6 @@ mod tests { fn should_dispatch_bridge_message_from_root_origin() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let message = prepare_root_message(Call::System(frame_system::Call::remark { remark: vec![1, 2, 3], })); @@ -1041,7 +971,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -1068,7 +997,6 @@ mod tests { fn should_dispatch_bridge_message_from_target_origin() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let call = Call::System(frame_system::Call::remark { remark: vec![] }); let message = prepare_target_message(call); @@ -1077,7 +1005,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), @@ -1104,7 +1031,6 @@ mod tests { fn should_dispatch_bridge_message_from_source_origin() { new_test_ext().execute_with(|| { let id = [0; 4]; - let relayer_account = 1; let call = Call::System(frame_system::Call::remark { remark: vec![] }); let message = prepare_source_message(call); @@ -1113,7 +1039,6 @@ mod tests { let result = Dispatch::dispatch( SOURCE_CHAIN_ID, TARGET_CHAIN_ID, - &relayer_account, id, Ok(message), |_, _| unreachable!(), diff --git a/primitives/message-dispatch/src/lib.rs b/primitives/message-dispatch/src/lib.rs index 7401dd468..2e69d7881 100644 --- a/primitives/message-dispatch/src/lib.rs +++ b/primitives/message-dispatch/src/lib.rs @@ -26,7 +26,6 @@ use bp_runtime::{ use codec::{Decode, Encode}; use frame_support::RuntimeDebug; use scale_info::TypeInfo; -use sp_runtime::transaction_validity::TransactionValidityError; use sp_std::prelude::*; /// Message dispatch weight. @@ -36,7 +35,7 @@ pub type Weight = u64; pub type SpecVersion = u32; /// A generic trait to dispatch arbitrary messages delivered over the bridge. -pub trait MessageDispatch { +pub trait MessageDispatch { /// A type of the message to be dispatched. type Message: codec::Decode; @@ -46,15 +45,6 @@ pub trait MessageDispatch { /// of dispatch weight. fn dispatch_weight(message: &Self::Message) -> Weight; - /// Checking in message receiving step before dispatch - /// - /// This will be called before the call enter dispatch phase. If failed, the message(call) will - /// be not be processed by this relayer, latter relayers can still continue process it. - fn pre_dispatch( - relayer_account: &AccountId, - message: Result<&Self::Message, ()>, - ) -> Result<(), &'static str>; - /// Dispatches the message internally. /// /// `source_chain` indicates the chain where the message came from. @@ -68,10 +58,9 @@ pub trait MessageDispatch { /// the whole message). /// /// Returns unspent dispatch weight. - fn dispatch Result<(), ()>>( + fn dispatch Result<(), ()>>( source_chain: ChainId, target_chain: ChainId, - relayer_account: &AccountId, id: BridgeMessageId, message: Result, pay_dispatch_fee: P, @@ -161,35 +150,8 @@ pub trait IntoDispatchOrigin { fn into_dispatch_origin(id: &AccountId, call: &Call) -> Origin; } -/// A generic trait to validate message before dispatch. -pub trait CallValidate { - /// Checking in message receiving step before dispatch - /// - /// This will be called before the call enter dispatch phase. If failed, the message(call) will - /// be not be processed by this relayer, latter relayers can still continue process it. - fn check_receiving_before_dispatch( - relayer_account: &AccountId, - call: &Call, - ) -> Result<(), &'static str>; - /// In-dispatch call validation - /// - /// This will be called in the dispatch process, If failed, return message dispatch errors. - fn call_validate( - relayer_account: &AccountId, - origin: &Origin, - call: &Call, - ) -> Result<(), TransactionValidityError>; -} - -/// CallValidate's default implementation, no additional validation -pub enum Everything {} - -impl CallValidate for Everything { - fn check_receiving_before_dispatch(_: &AccountId, _: &Call) -> Result<(), &'static str> { - Ok(()) - } - - fn call_validate(_: &AccountId, _: &Origin, _: &Call) -> Result<(), TransactionValidityError> { - Ok(()) - } +/// A generic trait to filter calls that are allowed to be dispatched. +pub trait CallFilter { + /// Filter the call, you might need origin to in the filter. return false, if not allowed. + fn contains(origin: &Origin, call: &Call) -> bool; }