diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index f02e257730065f72ddd5403fbb09aadf095d9098..69e993b7bb78d73dff930e54c08e915bf11f1466 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -394,6 +394,7 @@ impl pallet_bridge_messages::Config<WithRialtoMessagesInstance> for Runtime { GetDeliveryConfirmationTransactionFee, RootAccountForPayments, >; + type OnMessageAccepted = (); type OnDeliveryConfirmed = pallet_bridge_token_swap::Pallet<Runtime, WithRialtoTokenSwapInstance>; type SourceHeaderChain = crate::rialto_messages::Rialto; diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index fd3b5c94245c6589802198efd245ab9aab84e9ea..52206a8cf169391b56f709b04ef660433357b4f8 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -528,6 +528,7 @@ impl pallet_bridge_messages::Config<WithMillauMessagesInstance> for Runtime { GetDeliveryConfirmationTransactionFee, RootAccountForPayments, >; + type OnMessageAccepted = (); type OnDeliveryConfirmed = (); type SourceHeaderChain = crate::millau_messages::Millau; diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index 335cc7f9608fcdb977a81fed48118276e2faef63..6e1ce21a58509d66dc8e40454b43b2a8e39e22a6 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -48,7 +48,8 @@ use crate::weights::WeightInfo; use bp_messages::{ source_chain::{ - LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, TargetHeaderChain, + LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, OnMessageAccepted, + RelayersRewards, TargetHeaderChain, }, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, total_unrewarded_messages, DeliveredMessages, InboundLaneData, LaneId, MessageData, MessageKey, MessageNonce, @@ -160,6 +161,8 @@ pub mod pallet { Self::AccountId, Self::OutboundMessageFee, >; + /// Handler for accepted messages. + type OnMessageAccepted: OnMessageAccepted; /// Handler for delivered messages. type OnDeliveryConfirmed: OnDeliveryConfirmed; @@ -238,7 +241,7 @@ pub mod pallet { } /// Send message over lane. - #[pallet::weight(T::WeightInfo::send_message_weight(payload))] + #[pallet::weight(T::WeightInfo::send_message_weight(payload, T::DbWeight::get()))] pub fn send_message( origin: OriginFor<T>, lane_id: LaneId, @@ -557,19 +560,19 @@ pub mod pallet { Some(difference) => { log::trace!( target: "runtime::bridge-messages", - "Messages delivery callback has returned unspent weight to refund the submitter: \ + "T::OnDeliveryConfirmed callback has spent less weight than expected. Refunding: \ {} - {} = {}", preliminary_callback_overhead, actual_callback_weight, difference, ); - actual_weight -= difference; + actual_weight = actual_weight.saturating_sub(difference); } None => { - debug_assert!(false, "The delivery confirmation callback is wrong"); - log::trace!( + debug_assert!(false, "T::OnDeliveryConfirmed callback consumed too much weight."); + log::error!( target: "runtime::bridge-messages", - "Messages delivery callback has returned more weight that it may spent: \ + "T::OnDeliveryConfirmed callback has spent more weight that it is allowed to: \ {} vs {}", preliminary_callback_overhead, actual_callback_weight, @@ -859,7 +862,7 @@ fn send_message<T: Config<I>, I: 'static>( ensure_normal_operating_mode::<T, I>()?; // initially, actual (post-dispatch) weight is equal to pre-dispatch weight - let mut actual_weight = T::WeightInfo::send_message_weight(&payload); + let mut actual_weight = T::WeightInfo::send_message_weight(&payload, T::DbWeight::get()); // let's first check if message can be delivered to target chain T::TargetHeaderChain::verify_message(&payload).map_err(|err| { @@ -913,6 +916,36 @@ fn send_message<T: Config<I>, I: 'static>( payload: encoded_payload, fee: delivery_and_dispatch_fee, }); + // Guaranteed to be called outside only when the message is accepted. + // We assume that the maximum weight call back used is `single_message_callback_overhead`, so do not perform + // complex db operation in callback. If you want to, put these magic logic in outside pallet and control + // the weight there. + let single_message_callback_overhead = T::WeightInfo::single_message_callback_overhead(T::DbWeight::get()); + let actual_callback_weight = T::OnMessageAccepted::on_messages_accepted(&lane_id, &nonce); + match single_message_callback_overhead.checked_sub(actual_callback_weight) { + Some(difference) if difference == 0 => (), + Some(difference) => { + log::trace!( + target: "runtime::bridge-messages", + "T::OnMessageAccepted callback has spent less weight than expected. Refunding: \ + {} - {} = {}", + single_message_callback_overhead, + actual_callback_weight, + difference, + ); + actual_weight = actual_weight.saturating_sub(difference); + } + None => { + debug_assert!(false, "T::OnMessageAccepted callback consumed too much weight."); + log::error!( + target: "runtime::bridge-messages", + "T::OnMessageAccepted callback has spent more weight that it is allowed to: \ + {} vs {}", + single_message_callback_overhead, + actual_callback_weight, + ); + } + } // message sender pays for pruning at most `MaxMessagesToPruneAtOnce` messages // the cost of pruning every message is roughly single db write @@ -1114,7 +1147,7 @@ mod tests { use crate::mock::{ message, message_payload, run_test, unrewarded_relayer, Event as TestEvent, Origin, TestMessageDeliveryAndDispatchPayment, TestMessagesDeliveryProof, TestMessagesParameter, TestMessagesProof, - TestOnDeliveryConfirmed1, TestOnDeliveryConfirmed2, TestRuntime, TokenConversionRate, + TestOnDeliveryConfirmed1, TestOnDeliveryConfirmed2, TestOnMessageAccepted, TestRuntime, TokenConversionRate, PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_RELAYER_A, TEST_RELAYER_B, }; use bp_messages::{UnrewardedRelayer, UnrewardedRelayersState}; @@ -2259,4 +2292,40 @@ mod tests { ); }); } + + #[test] + fn message_accepted_callbacks_are_called() { + run_test(|| { + send_regular_message(); + TestOnMessageAccepted::ensure_called(&TEST_LANE_ID, &1); + }); + } + + #[test] + #[should_panic] + fn message_accepted_panics_in_debug_mode_if_callback_is_wrong() { + run_test(|| { + TestOnMessageAccepted::set_consumed_weight_per_message(crate::mock::DbWeight::get().reads_writes(2, 2)); + send_regular_message(); + }); + } + + #[test] + fn message_accepted_refunds_non_zero_weight() { + run_test(|| { + TestOnMessageAccepted::set_consumed_weight_per_message(crate::mock::DbWeight::get().writes(1)); + let actual_callback_weight = send_regular_message(); + let pre_dispatch_weight = <TestRuntime as Config>::WeightInfo::send_message_weight( + ®ULAR_PAYLOAD, + crate::mock::DbWeight::get(), + ); + let prune_weight = + crate::mock::DbWeight::get().writes(<TestRuntime as Config>::MaxMessagesToPruneAtOnce::get()); + + assert_eq!( + pre_dispatch_weight.saturating_sub(actual_callback_weight), + crate::mock::DbWeight::get().reads(1).saturating_add(prune_weight) + ); + }); + } } diff --git a/bridges/modules/messages/src/mock.rs b/bridges/modules/messages/src/mock.rs index 77a421b3411ae5988e0372bdc1f94a2584a2e785..88ec92ff03241b73a2e0031d7744f3ed41073c9b 100644 --- a/bridges/modules/messages/src/mock.rs +++ b/bridges/modules/messages/src/mock.rs @@ -22,8 +22,8 @@ use crate::Config; use bitvec::prelude::*; use bp_messages::{ source_chain::{ - LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, RelayersRewards, Sender, - TargetHeaderChain, + LaneMessageVerifier, MessageDeliveryAndDispatchPayment, OnDeliveryConfirmed, OnMessageAccepted, + RelayersRewards, Sender, TargetHeaderChain, }, target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, DeliveredMessages, InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData, @@ -178,6 +178,7 @@ impl Config for TestRuntime { type TargetHeaderChain = TestTargetHeaderChain; type LaneMessageVerifier = TestLaneMessageVerifier; type MessageDeliveryAndDispatchPayment = TestMessageDeliveryAndDispatchPayment; + type OnMessageAccepted = TestOnMessageAccepted; type OnDeliveryConfirmed = (TestOnDeliveryConfirmed1, TestOnDeliveryConfirmed2); type SourceHeaderChain = TestSourceHeaderChain; @@ -356,6 +357,35 @@ impl MessageDeliveryAndDispatchPayment<AccountId, TestMessageFee> for TestMessag } } +#[derive(Debug)] +pub struct TestOnMessageAccepted; + +impl TestOnMessageAccepted { + /// Verify that the callback has been called when the message is accepted. + pub fn ensure_called(lane: &LaneId, message: &MessageNonce) { + let key = (b"TestOnMessageAccepted", lane, message).encode(); + assert_eq!(frame_support::storage::unhashed::get(&key), Some(true)); + } + + /// Set consumed weight returned by the callback. + pub fn set_consumed_weight_per_message(weight: Weight) { + frame_support::storage::unhashed::put(b"TestOnMessageAccepted_Weight", &weight); + } + + /// Get consumed weight returned by the callback. + pub fn get_consumed_weight_per_message() -> Option<Weight> { + frame_support::storage::unhashed::get(b"TestOnMessageAccepted_Weight") + } +} + +impl OnMessageAccepted for TestOnMessageAccepted { + fn on_messages_accepted(lane: &LaneId, message: &MessageNonce) -> Weight { + let key = (b"TestOnMessageAccepted", lane, message).encode(); + frame_support::storage::unhashed::put(&key, &true); + Self::get_consumed_weight_per_message().unwrap_or_else(|| DbWeight::get().reads_writes(1, 1)) + } +} + /// First on-messages-delivered callback. #[derive(Debug)] pub struct TestOnDeliveryConfirmed1; diff --git a/bridges/modules/messages/src/weights_ext.rs b/bridges/modules/messages/src/weights_ext.rs index c88ec47d72b9df8133a3f797e21e4caa791fd1b4..297b03cfc17bb100c1bb75fc37746e6a0c8dd938 100644 --- a/bridges/modules/messages/src/weights_ext.rs +++ b/bridges/modules/messages/src/weights_ext.rs @@ -189,11 +189,14 @@ pub trait WeightInfoExt: WeightInfo { // Functions that are directly mapped to extrinsics weights. /// Weight of message send extrinsic. - fn send_message_weight(message: &impl Size) -> Weight { + fn send_message_weight(message: &impl Size, db_weight: RuntimeDbWeight) -> Weight { let transaction_overhead = Self::send_message_overhead(); let message_size_overhead = Self::send_message_size_overhead(message.size_hint()); + let call_back_overhead = Self::single_message_callback_overhead(db_weight); - transaction_overhead.saturating_add(message_size_overhead) + transaction_overhead + .saturating_add(message_size_overhead) + .saturating_add(call_back_overhead) } /// Weight of message delivery extrinsic. @@ -341,7 +344,11 @@ pub trait WeightInfoExt: WeightInfo { Self::receive_single_message_proof().saturating_sub(Self::receive_single_prepaid_message_proof()) } - /// Returns pre-dispatch weight of single message delivery callback call. + /// Returns pre-dispatch weight of single callback call. + /// + /// When benchmarking the weight please take into consideration both the `OnMessageAccepted` and + /// `OnDeliveryConfirmed` callbacks. The method should return the greater of the two, because it's + /// used to estimate the weight in both contexts. fn single_message_callback_overhead(db_weight: RuntimeDbWeight) -> Weight { db_weight.reads_writes(1, 1) } diff --git a/bridges/primitives/messages/src/source_chain.rs b/bridges/primitives/messages/src/source_chain.rs index 3b84562e9b4fde3b795745a3d92812869386471e..326ab4f0f8ccbef78f1013ddccfe07d1cb5c53f3 100644 --- a/bridges/primitives/messages/src/source_chain.rs +++ b/bridges/primitives/messages/src/source_chain.rs @@ -173,6 +173,18 @@ impl OnDeliveryConfirmed for Tuple { } } +/// Handler for messages have been accepted +pub trait OnMessageAccepted { + /// Called when a message has been accepted by message pallet. + fn on_messages_accepted(lane: &LaneId, message: &MessageNonce) -> Weight; +} + +impl OnMessageAccepted for () { + fn on_messages_accepted(_lane: &LaneId, _message: &MessageNonce) -> Weight { + 0 + } +} + /// Structure that may be used in place of `TargetHeaderChain`, `LaneMessageVerifier` and /// `MessageDeliveryAndDispatchPayment` on chains, where outbound messages are forbidden. pub struct ForbidOutboundMessages;