diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 33c21027f8..4913043373 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -688,6 +688,7 @@ mod tests { bp_millau::ADDITIONAL_MESSAGE_BYTE_DELIVERY_WEIGHT, bp_millau::MAX_SINGLE_MESSAGE_DELIVERY_CONFIRMATION_TX_WEIGHT, bp_millau::PAY_INBOUND_DISPATCH_FEE_WEIGHT, + DbWeight::get(), ); let max_incoming_message_proof_size = bp_rialto::EXTRA_STORAGE_PROOF_SIZE.saturating_add( @@ -712,6 +713,7 @@ mod tests { max_incoming_inbound_lane_data_proof_size, bp_rialto::MAX_UNREWARDED_RELAYER_ENTRIES_AT_INBOUND_LANE, bp_rialto::MAX_UNCONFIRMED_MESSAGES_AT_INBOUND_LANE, + DbWeight::get(), ); } } diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index 219feda646..e53beed389 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -1125,6 +1125,7 @@ mod tests { bp_rialto::ADDITIONAL_MESSAGE_BYTE_DELIVERY_WEIGHT, bp_rialto::MAX_SINGLE_MESSAGE_DELIVERY_CONFIRMATION_TX_WEIGHT, bp_rialto::PAY_INBOUND_DISPATCH_FEE_WEIGHT, + DbWeight::get(), ); let max_incoming_message_proof_size = bp_millau::EXTRA_STORAGE_PROOF_SIZE.saturating_add( @@ -1149,6 +1150,7 @@ mod tests { max_incoming_inbound_lane_data_proof_size, bp_millau::MAX_UNREWARDED_RELAYER_ENTRIES_AT_INBOUND_LANE, bp_millau::MAX_UNCONFIRMED_MESSAGES_AT_INBOUND_LANE, + DbWeight::get(), ); } diff --git a/bridges/modules/messages/README.md b/bridges/modules/messages/README.md index be25b3c37f..062a966fad 100644 --- a/bridges/modules/messages/README.md +++ b/bridges/modules/messages/README.md @@ -354,7 +354,7 @@ Both conditions are verified by `pallet_bridge_messages::ensure_weights_are_corr `pallet_bridge_messages::ensure_able_to_receive_messages` functions, which must be called from every runtime's tests. -### Post-dispatch weight refunds of the `receive_messages_proof` call +#### Post-dispatch weight refunds of the `receive_messages_proof` call Weight formula of the `receive_messages_proof` call assumes that the dispatch fee of every message is paid at the target chain (where call is executed), that every message will be dispatched and that @@ -388,6 +388,7 @@ The weight formula is: Weight = BaseWeight + MessagesCount * MessageConfirmationWeight + RelayersCount * RelayerRewardWeight + Max(0, ActualProofSize - ExpectedProofSize) * ProofByteDeliveryWeight + + MessagesCount * (DbReadWeight + DbWriteWeight) ``` Where: @@ -403,6 +404,15 @@ Where: | `ExpectedProofSize` | `EXTRA_STORAGE_PROOF_SIZE` | Size of proof that we are expecting | | `ProofByteDeliveryWeight` | `(receive_single_message_proof_16_kb - receive_single_message_proof_1_kb) / (15 * 1024)` | Weight of processing every additional proof byte over `ExpectedProofSize` limit. We're using the same formula, as for message delivery, because proof mechanism is assumed to be the same in both cases | +#### Post-dispatch weight refunds of the `receive_messages_delivery_proof` call + +Weight formula of the `receive_messages_delivery_proof` call assumes that all messages in the proof +are actually delivered (so there are no already confirmed messages) and every messages is processed +by the `OnDeliveryConfirmed` callback. This means that for every message, we're adding single db read +weight and single db write weight. If, by some reason, messages are not processed by the +`OnDeliveryConfirmed` callback, or their processing is faster than that additional weight, the +difference is refunded to the submitter. + #### Why we're always able to craft `receive_messages_delivery_proof` transaction? There can be at most `::MaxUnconfirmedMessagesAtInboundLane` diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index a5f94c1eda..c1d4fa223a 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -594,14 +594,35 @@ decl_module! { } /// Receive messages delivery proof from bridged chain. - #[weight = T::WeightInfo::receive_messages_delivery_proof_weight(proof, relayers_state)] + #[weight = T::WeightInfo::receive_messages_delivery_proof_weight( + proof, + relayers_state, + T::DbWeight::get(), + )] pub fn receive_messages_delivery_proof( origin, proof: MessagesDeliveryProofOf, relayers_state: UnrewardedRelayersState, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { ensure_not_halted::()?; + // why do we need to know the weight of this (`receive_messages_delivery_proof`) call? Because + // we may want to return some funds for messages that are not processed by the delivery callback, + // or if their actual processing weight is less than accounted by weight formula. + // So to refund relayer, we need to: + // + // ActualWeight = DeclaredWeight - UnspentCallbackWeight + // + // The DeclaredWeight is exactly what's computed here. Unfortunately it is impossible + // to get pre-computed value (and it has been already computed by the executive). + let single_message_callback_overhead = T::WeightInfo::single_message_callback_overhead(T::DbWeight::get()); + let declared_weight = T::WeightInfo::receive_messages_delivery_proof_weight( + &proof, + &relayers_state, + T::DbWeight::get(), + ); + let mut actual_weight = declared_weight; + let confirmation_relayer = ensure_signed(origin)?; let (lane_id, lane_data) = T::TargetHeaderChain::verify_messages_delivery_proof(proof).map_err(|err| { log::trace!( @@ -641,7 +662,37 @@ decl_module! { }; if let Some(confirmed_messages) = confirmed_messages { // handle messages delivery confirmation - T::OnDeliveryConfirmed::on_messages_delivered(&lane_id, &confirmed_messages); + let preliminary_callback_overhead = relayers_state.total_messages.saturating_mul( + single_message_callback_overhead + ); + let actual_callback_weight = T::OnDeliveryConfirmed::on_messages_delivered( + &lane_id, + &confirmed_messages, + ); + match preliminary_callback_overhead.checked_sub(actual_callback_weight) { + Some(difference) if difference == 0 => (), + Some(difference) => { + log::trace!( + target: "runtime::bridge-messages", + "Messages delivery callback has returned unspent weight to refund the submitter: \ + {} - {} = {}", + preliminary_callback_overhead, + actual_callback_weight, + difference, + ); + actual_weight -= difference; + }, + None => { + debug_assert!(false, "The delivery confirmation callback is wrong"); + log::trace!( + target: "runtime::bridge-messages", + "Messages delivery callback has returned more weight that it may spent: \ + {} vs {}", + preliminary_callback_overhead, + actual_callback_weight, + ); + } + } // emit 'delivered' event let received_range = confirmed_messages.begin..=confirmed_messages.end; @@ -684,7 +735,10 @@ decl_module! { lane_id, ); - Ok(()) + Ok(PostDispatchInfo { + actual_weight: Some(actual_weight), + pays_fee: Pays::Yes, + }) } } } @@ -962,8 +1016,8 @@ mod tests { use crate::mock::{ message, message_payload, run_test, unrewarded_relayer, Event as TestEvent, Origin, TestMessageDeliveryAndDispatchPayment, TestMessagesDeliveryProof, TestMessagesParameter, TestMessagesProof, - TestRuntime, TokenConversionRate, PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, - TEST_RELAYER_A, TEST_RELAYER_B, + TestOnDeliveryConfirmed1, TestOnDeliveryConfirmed2, TestRuntime, TokenConversionRate, + PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_RELAYER_A, TEST_RELAYER_B, }; use bp_messages::{UnrewardedRelayer, UnrewardedRelayersState}; use frame_support::{assert_noop, assert_ok}; @@ -1260,10 +1314,14 @@ mod tests { TEST_LANE_ID, InboundLaneData { last_confirmed_nonce: 1, - ..Default::default() + relayers: vec![unrewarded_relayer(1, 1, TEST_RELAYER_A)].into_iter().collect(), }, ))), - Default::default(), + UnrewardedRelayersState { + unrewarded_relayer_entries: 1, + messages_in_oldest_entry: 1, + total_messages: 1, + }, ), Error::::Halted, ); @@ -1309,10 +1367,14 @@ mod tests { TEST_LANE_ID, InboundLaneData { last_confirmed_nonce: 1, - ..Default::default() + relayers: vec![unrewarded_relayer(1, 1, TEST_RELAYER_A)].into_iter().collect(), }, ))), - Default::default(), + UnrewardedRelayersState { + unrewarded_relayer_entries: 1, + messages_in_oldest_entry: 1, + total_messages: 1, + }, )); }); } @@ -1923,10 +1985,70 @@ mod tests { )); // ensure that both callbacks have been called twice: for 1+2, then for 3 - crate::mock::TestOnDeliveryConfirmed1::ensure_called(&TEST_LANE_ID, &delivered_messages_1_and_2); - crate::mock::TestOnDeliveryConfirmed1::ensure_called(&TEST_LANE_ID, &delivered_message_3); - crate::mock::TestOnDeliveryConfirmed2::ensure_called(&TEST_LANE_ID, &delivered_messages_1_and_2); - crate::mock::TestOnDeliveryConfirmed2::ensure_called(&TEST_LANE_ID, &delivered_message_3); + TestOnDeliveryConfirmed1::ensure_called(&TEST_LANE_ID, &delivered_messages_1_and_2); + TestOnDeliveryConfirmed1::ensure_called(&TEST_LANE_ID, &delivered_message_3); + TestOnDeliveryConfirmed2::ensure_called(&TEST_LANE_ID, &delivered_messages_1_and_2); + TestOnDeliveryConfirmed2::ensure_called(&TEST_LANE_ID, &delivered_message_3); + }); + } + + fn confirm_3_messages_delivery() -> (Weight, Weight) { + send_regular_message(); + send_regular_message(); + send_regular_message(); + + let proof = TestMessagesDeliveryProof(Ok(( + TEST_LANE_ID, + InboundLaneData { + last_confirmed_nonce: 0, + relayers: vec![unrewarded_relayer(1, 3, TEST_RELAYER_A)].into_iter().collect(), + }, + ))); + let relayers_state = UnrewardedRelayersState { + unrewarded_relayer_entries: 1, + total_messages: 3, + ..Default::default() + }; + let pre_dispatch_weight = ::WeightInfo::receive_messages_delivery_proof_weight( + &proof, + &relayers_state, + crate::mock::DbWeight::get(), + ); + let post_dispatch_weight = + Pallet::::receive_messages_delivery_proof(Origin::signed(1), proof, relayers_state) + .expect("confirmation has failed") + .actual_weight + .expect("receive_messages_delivery_proof always returns Some"); + (pre_dispatch_weight, post_dispatch_weight) + } + + #[test] + fn receive_messages_delivery_proof_refunds_zero_weight() { + run_test(|| { + let (pre_dispatch_weight, post_dispatch_weight) = confirm_3_messages_delivery(); + assert_eq!(pre_dispatch_weight, post_dispatch_weight); + }); + } + + #[test] + fn receive_messages_delivery_proof_refunds_non_zero_weight() { + run_test(|| { + TestOnDeliveryConfirmed1::set_consumed_weight_per_message(crate::mock::DbWeight::get().writes(1)); + + let (pre_dispatch_weight, post_dispatch_weight) = confirm_3_messages_delivery(); + assert_eq!( + pre_dispatch_weight.saturating_sub(post_dispatch_weight), + crate::mock::DbWeight::get().reads(1) * 3 + ); + }); + } + + #[test] + #[should_panic] + fn receive_messages_panics_in_debug_mode_if_callback_is_wrong() { + run_test(|| { + TestOnDeliveryConfirmed1::set_consumed_weight_per_message(crate::mock::DbWeight::get().reads_writes(2, 2)); + confirm_3_messages_delivery() }); } } diff --git a/bridges/modules/messages/src/mock.rs b/bridges/modules/messages/src/mock.rs index 2e184dda15..9d361b53cf 100644 --- a/bridges/modules/messages/src/mock.rs +++ b/bridges/modules/messages/src/mock.rs @@ -31,7 +31,10 @@ use bp_messages::{ }; use bp_runtime::{messages::MessageDispatchResult, Size}; use codec::{Decode, Encode}; -use frame_support::{parameter_types, weights::Weight}; +use frame_support::{ + parameter_types, + weights::{RuntimeDbWeight, Weight}, +}; use sp_core::H256; use sp_runtime::{ testing::Header as SubstrateHeader, @@ -87,6 +90,7 @@ parameter_types! { pub const MaximumBlockWeight: Weight = 1024; pub const MaximumBlockLength: u32 = 2 * 1024; pub const AvailableBlockRatio: Perbill = Perbill::one(); + pub const DbWeight: RuntimeDbWeight = RuntimeDbWeight { read: 1, write: 2 }; } impl frame_system::Config for TestRuntime { @@ -110,7 +114,7 @@ impl frame_system::Config for TestRuntime { type SystemWeightInfo = (); type BlockWeights = (); type BlockLength = (); - type DbWeight = (); + type DbWeight = DbWeight; type SS58Prefix = (); type OnSetCode = (); } @@ -358,12 +362,25 @@ impl TestOnDeliveryConfirmed1 { let key = (b"TestOnDeliveryConfirmed1", lane, messages).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"TestOnDeliveryConfirmed1_Weight", &weight); + } + + /// Get consumed weight returned by the callback. + pub fn get_consumed_weight_per_message() -> Option { + frame_support::storage::unhashed::get(b"TestOnDeliveryConfirmed1_Weight") + } } impl OnDeliveryConfirmed for TestOnDeliveryConfirmed1 { - fn on_messages_delivered(lane: &LaneId, messages: &DeliveredMessages) { + fn on_messages_delivered(lane: &LaneId, messages: &DeliveredMessages) -> Weight { let key = (b"TestOnDeliveryConfirmed1", lane, messages).encode(); frame_support::storage::unhashed::put(&key, &true); + Self::get_consumed_weight_per_message() + .unwrap_or_else(|| DbWeight::get().reads_writes(1, 1)) + .saturating_mul(messages.total_messages()) } } @@ -380,9 +397,10 @@ impl TestOnDeliveryConfirmed2 { } impl OnDeliveryConfirmed for TestOnDeliveryConfirmed2 { - fn on_messages_delivered(lane: &LaneId, messages: &DeliveredMessages) { + fn on_messages_delivered(lane: &LaneId, messages: &DeliveredMessages) -> Weight { let key = (b"TestOnDeliveryConfirmed2", lane, messages).encode(); frame_support::storage::unhashed::put(&key, &true); + 0 } } diff --git a/bridges/modules/messages/src/weights_ext.rs b/bridges/modules/messages/src/weights_ext.rs index be440174b4..e1b8c7e885 100644 --- a/bridges/modules/messages/src/weights_ext.rs +++ b/bridges/modules/messages/src/weights_ext.rs @@ -20,7 +20,7 @@ use crate::weights::WeightInfo; use bp_messages::{MessageNonce, UnrewardedRelayersState}; use bp_runtime::{PreComputedSize, Size}; -use frame_support::weights::Weight; +use frame_support::weights::{RuntimeDbWeight, Weight}; /// Size of the message being delivered in benchmarks. pub const EXPECTED_DEFAULT_MESSAGE_LENGTH: u32 = 128; @@ -35,6 +35,7 @@ pub fn ensure_weights_are_correct( expected_additional_byte_delivery_weight: Weight, expected_messages_delivery_confirmation_tx_weight: Weight, expected_pay_inbound_dispatch_fee_weight: Weight, + db_weight: RuntimeDbWeight, ) { // verify `send_message` weight components assert_ne!(W::send_message_overhead(), 0); @@ -82,6 +83,7 @@ pub fn ensure_weights_are_correct( total_messages: 1, ..Default::default() }, + db_weight, ); assert!( actual_messages_delivery_confirmation_tx_weight <= expected_messages_delivery_confirmation_tx_weight, @@ -138,6 +140,7 @@ pub fn ensure_able_to_receive_confirmation( max_inbound_lane_data_proof_size_from_peer_chain: u32, max_unrewarded_relayer_entries_at_peer_inbound_lane: MessageNonce, max_unconfirmed_messages_at_inbound_lane: MessageNonce, + db_weight: RuntimeDbWeight, ) { // verify that we're able to receive confirmation of maximal-size let max_confirmation_transaction_size = @@ -158,6 +161,7 @@ pub fn ensure_able_to_receive_confirmation( total_messages: max_unconfirmed_messages_at_inbound_lane, ..Default::default() }, + db_weight, ); assert!( max_confirmation_transaction_dispatch_weight <= max_extrinsic_weight, @@ -212,7 +216,11 @@ pub trait WeightInfoExt: WeightInfo { } /// Weight of confirmation delivery extrinsic. - fn receive_messages_delivery_proof_weight(proof: &impl Size, relayers_state: &UnrewardedRelayersState) -> Weight { + fn receive_messages_delivery_proof_weight( + proof: &impl Size, + relayers_state: &UnrewardedRelayersState, + db_weight: RuntimeDbWeight, + ) -> Weight { // basic components of extrinsic weight let transaction_overhead = Self::receive_messages_delivery_proof_overhead(); let messages_overhead = Self::receive_messages_delivery_proof_messages_overhead(relayers_state.total_messages); @@ -225,10 +233,16 @@ pub trait WeightInfoExt: WeightInfo { let proof_size_overhead = Self::storage_proof_size_overhead(actual_proof_size.saturating_sub(expected_proof_size)); + // and cost of calling `OnDeliveryConfirmed::on_messages_delivered()` for every confirmed message + let callback_overhead = relayers_state + .total_messages + .saturating_mul(Self::single_message_callback_overhead(db_weight)); + transaction_overhead .saturating_add(messages_overhead) .saturating_add(relayers_overhead) .saturating_add(proof_size_overhead) + .saturating_add(callback_overhead) } // Functions that are used by extrinsics weights formulas. @@ -321,6 +335,11 @@ pub trait WeightInfoExt: WeightInfo { fn pay_inbound_dispatch_fee_overhead() -> Weight { Self::receive_single_message_proof().saturating_sub(Self::receive_single_prepaid_message_proof()) } + + /// Returns pre-dispatch weight of single message delivery callback call. + fn single_message_callback_overhead(db_weight: RuntimeDbWeight) -> Weight { + db_weight.reads_writes(1, 1) + } } impl WeightInfoExt for () { diff --git a/bridges/primitives/messages/src/lib.rs b/bridges/primitives/messages/src/lib.rs index 963543ec32..4ec7c1f9d2 100644 --- a/bridges/primitives/messages/src/lib.rs +++ b/bridges/primitives/messages/src/lib.rs @@ -226,6 +226,15 @@ impl DeliveredMessages { } } + /// Return total count of delivered messages. + pub fn total_messages(&self) -> MessageNonce { + if self.end >= self.begin { + self.end - self.begin + 1 + } else { + 0 + } + } + /// Note new dispatched message. pub fn note_dispatched_message(&mut self, dispatch_result: bool) { self.end += 1; diff --git a/bridges/primitives/messages/src/source_chain.rs b/bridges/primitives/messages/src/source_chain.rs index 392331eda6..ed69f39c76 100644 --- a/bridges/primitives/messages/src/source_chain.rs +++ b/bridges/primitives/messages/src/source_chain.rs @@ -19,7 +19,7 @@ use crate::{DeliveredMessages, InboundLaneData, LaneId, MessageNonce, OutboundLaneData}; use bp_runtime::Size; -use frame_support::{Parameter, RuntimeDebug}; +use frame_support::{weights::Weight, Parameter, RuntimeDebug}; use sp_std::{collections::btree_map::BTreeMap, fmt::Debug}; /// The sender of the message on the source chain. @@ -136,12 +136,33 @@ pub trait MessageDeliveryAndDispatchPayment { } /// Handler for messages delivery confirmation. -#[impl_trait_for_tuples::impl_for_tuples(30)] pub trait OnDeliveryConfirmed { /// Called when we receive confirmation that our messages have been delivered to the /// target chain. The confirmation also has single bit dispatch result for every - /// confirmed message (see `DeliveredMessages` for details). - fn on_messages_delivered(_lane: &LaneId, _messages: &DeliveredMessages) {} + /// confirmed message (see `DeliveredMessages` for details). Guaranteed to be called + /// only when at least one message is delivered. + /// + /// Should return total weight consumed by the call. + /// + /// NOTE: messages pallet assumes that maximal weight that may be spent on processing + /// single message is single DB read + single DB write. So this function shall never + /// return weight that is larger than total number of messages * (db read + db write). + /// If your pallet needs more time for processing single message, please do it + /// from `on_initialize` call(s) of the next block(s). + fn on_messages_delivered(_lane: &LaneId, _messages: &DeliveredMessages) -> Weight; +} + +#[impl_trait_for_tuples::impl_for_tuples(30)] +impl OnDeliveryConfirmed for Tuple { + fn on_messages_delivered(lane: &LaneId, messages: &DeliveredMessages) -> Weight { + let mut total_weight: Weight = 0; + for_tuples!( + #( + total_weight = total_weight.saturating_add(Tuple::on_messages_delivered(lane, messages)); + )* + ); + total_weight + } } /// Structure that may be used in place of `TargetHeaderChain`, `LaneMessageVerifier` and