From 8e140b54c5838f787da0b2a227cd3921f3fab3be Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 12 Apr 2023 09:52:55 +0300 Subject: [PATCH] refund extra weight in receive_messages_delivery_proof call (#2031) --- bridges/modules/messages/src/lib.rs | 120 ++++++++++++------ bridges/modules/messages/src/mock.rs | 10 +- .../modules/relayers/src/payment_adapter.rs | 7 +- .../primitives/messages/src/source_chain.rs | 10 +- 4 files changed, 104 insertions(+), 43 deletions(-) diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index bf456b1c0d2c..3404ba0b219c 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -63,7 +63,7 @@ use bp_messages::{ MessageKey, MessageNonce, MessagePayload, MessagesOperatingMode, OutboundLaneData, OutboundMessageDetails, UnrewardedRelayersState, }; -use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, Size}; +use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize, Size}; use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{dispatch::PostDispatchInfo, ensure, fail, traits::Get}; use sp_runtime::{traits::UniqueSaturatedFrom, SaturatedConversion}; @@ -423,10 +423,11 @@ pub mod pallet { pub fn receive_messages_delivery_proof( origin: OriginFor, proof: MessagesDeliveryProofOf, - relayers_state: UnrewardedRelayersState, - ) -> DispatchResult { + mut relayers_state: UnrewardedRelayersState, + ) -> DispatchResultWithPostInfo { Self::ensure_not_halted().map_err(Error::::BridgeModule)?; + let proof_size = proof.size(); let confirmation_relayer = ensure_signed(origin)?; let (lane_id, lane_data) = T::TargetHeaderChain::verify_messages_delivery_proof(proof) .map_err(|err| { @@ -499,13 +500,27 @@ pub mod pallet { }); // if some new messages have been confirmed, reward relayers - T::DeliveryConfirmationPayments::pay_reward( + let actually_rewarded_relayers = T::DeliveryConfirmationPayments::pay_reward( lane_id, lane_data.relayers, &confirmation_relayer, &received_range, ); - } + + // update relayers state with actual numbers to compute actual weight below + relayers_state.unrewarded_relayer_entries = sp_std::cmp::min( + relayers_state.unrewarded_relayer_entries, + actually_rewarded_relayers, + ); + relayers_state.total_messages = sp_std::cmp::min( + relayers_state.total_messages, + received_range + .end() + .checked_sub(*received_range.start()) + .and_then(|x| x.checked_add(1)) + .unwrap_or(MessageNonce::MAX), + ); + }; log::trace!( target: LOG_TARGET, @@ -514,11 +529,15 @@ pub mod pallet { lane_id, ); - // TODO: https://github.com/paritytech/parity-bridges-common/issues/2020 - // we need to refund unused weight (because the inbound lane state may contain - // already confirmed messages and already rewarded relayer entries) + // because of lags, the inbound lane state (`lane_data`) may have entries for + // already rewarded relayers and messages (if all entries are duplicated, then + // this transaction must be filtered out by our signed extension) + let actual_weight = T::WeightInfo::receive_messages_delivery_proof_weight( + &PreComputedSize(proof_size as usize), + &relayers_state, + ); - Ok(()) + Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes }) } } @@ -940,8 +959,9 @@ mod tests { message, message_payload, run_test, unrewarded_relayer, AccountId, DbWeight, RuntimeEvent as TestEvent, RuntimeOrigin, TestDeliveryConfirmationPayments, TestDeliveryPayments, TestMessagesDeliveryProof, TestMessagesProof, TestRelayer, - TestRuntime, MAX_OUTBOUND_PAYLOAD_SIZE, PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, - TEST_LANE_ID, TEST_LANE_ID_2, TEST_LANE_ID_3, TEST_RELAYER_A, TEST_RELAYER_B, + TestRuntime, TestWeightInfo, MAX_OUTBOUND_PAYLOAD_SIZE, PAYLOAD_REJECTED_BY_TARGET_CHAIN, + REGULAR_PAYLOAD, TEST_LANE_ID, TEST_LANE_ID_2, TEST_LANE_ID_3, TEST_RELAYER_A, + TEST_RELAYER_B, }; use bp_messages::{BridgeMessagesCall, UnrewardedRelayer, UnrewardedRelayersState}; use bp_test_utils::generate_owned_bridge_module_tests; @@ -1398,50 +1418,78 @@ mod tests { )); // this reports delivery of message 1 => reward is paid to TEST_RELAYER_A - assert_ok!(Pallet::::receive_messages_delivery_proof( + let single_message_delivery_proof = TestMessagesDeliveryProof(Ok(( + TEST_LANE_ID, + InboundLaneData { + relayers: vec![unrewarded_relayer(1, 1, TEST_RELAYER_A)].into_iter().collect(), + ..Default::default() + }, + ))); + let single_message_delivery_proof_size = single_message_delivery_proof.size(); + let result = Pallet::::receive_messages_delivery_proof( RuntimeOrigin::signed(1), - TestMessagesDeliveryProof(Ok(( - TEST_LANE_ID, - InboundLaneData { - relayers: vec![unrewarded_relayer(1, 1, TEST_RELAYER_A)] - .into_iter() - .collect(), - ..Default::default() - } - ))), + single_message_delivery_proof, UnrewardedRelayersState { unrewarded_relayer_entries: 1, total_messages: 1, last_delivered_nonce: 1, ..Default::default() }, - )); + ); + assert_ok!(result); + assert_eq!( + result.unwrap().actual_weight.unwrap(), + TestWeightInfo::receive_messages_delivery_proof_weight( + &PreComputedSize(single_message_delivery_proof_size as _), + &UnrewardedRelayersState { + unrewarded_relayer_entries: 1, + total_messages: 1, + ..Default::default() + }, + ) + ); assert!(TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_A, 1)); assert!(!TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_B, 1)); // this reports delivery of both message 1 and message 2 => reward is paid only to // TEST_RELAYER_B - assert_ok!(Pallet::::receive_messages_delivery_proof( + let two_messages_delivery_proof = TestMessagesDeliveryProof(Ok(( + TEST_LANE_ID, + InboundLaneData { + relayers: vec![ + unrewarded_relayer(1, 1, TEST_RELAYER_A), + unrewarded_relayer(2, 2, TEST_RELAYER_B), + ] + .into_iter() + .collect(), + ..Default::default() + }, + ))); + let two_messages_delivery_proof_size = two_messages_delivery_proof.size(); + let result = Pallet::::receive_messages_delivery_proof( RuntimeOrigin::signed(1), - TestMessagesDeliveryProof(Ok(( - TEST_LANE_ID, - InboundLaneData { - relayers: vec![ - unrewarded_relayer(1, 1, TEST_RELAYER_A), - unrewarded_relayer(2, 2, TEST_RELAYER_B) - ] - .into_iter() - .collect(), - ..Default::default() - } - ))), + two_messages_delivery_proof, UnrewardedRelayersState { unrewarded_relayer_entries: 2, total_messages: 2, last_delivered_nonce: 2, ..Default::default() }, - )); + ); + assert_ok!(result); + // even though the pre-dispatch weight was for two messages, the actual weight is + // for single message only + assert_eq!( + result.unwrap().actual_weight.unwrap(), + TestWeightInfo::receive_messages_delivery_proof_weight( + &PreComputedSize(two_messages_delivery_proof_size as _), + &UnrewardedRelayersState { + unrewarded_relayer_entries: 1, + total_messages: 1, + ..Default::default() + }, + ) + ); assert!(!TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_A, 1)); assert!(TestDeliveryConfirmationPayments::is_reward_paid(TEST_RELAYER_B, 1)); }); diff --git a/bridges/modules/messages/src/mock.rs b/bridges/modules/messages/src/mock.rs index be4072f9278f..d16fd4138fc3 100644 --- a/bridges/modules/messages/src/mock.rs +++ b/bridges/modules/messages/src/mock.rs @@ -148,9 +148,12 @@ parameter_types! { pub const ActiveOutboundLanes: &'static [LaneId] = &[TEST_LANE_ID, TEST_LANE_ID_2]; } +/// weights of messages pallet calls we use in tests. +pub type TestWeightInfo = (); + impl Config for TestRuntime { type RuntimeEvent = RuntimeEvent; - type WeightInfo = (); + type WeightInfo = TestWeightInfo; type ActiveOutboundLanes = ActiveOutboundLanes; type MaxUnrewardedRelayerEntriesAtInboundLane = MaxUnrewardedRelayerEntriesAtInboundLane; type MaxUnconfirmedMessagesAtInboundLane = MaxUnconfirmedMessagesAtInboundLane; @@ -381,12 +384,15 @@ impl DeliveryConfirmationPayments for TestDeliveryConfirmationPayment messages_relayers: VecDeque>, _confirmation_relayer: &AccountId, received_range: &RangeInclusive, - ) { + ) -> MessageNonce { let relayers_rewards = calc_relayers_rewards(messages_relayers, received_range); + let rewarded_relayers = relayers_rewards.len(); for (relayer, reward) in &relayers_rewards { let key = (b":relayer-reward:", relayer, reward).encode(); frame_support::storage::unhashed::put(&key, &true); } + + rewarded_relayers as _ } } diff --git a/bridges/modules/relayers/src/payment_adapter.rs b/bridges/modules/relayers/src/payment_adapter.rs index 2752044958b4..a9536cfc0275 100644 --- a/bridges/modules/relayers/src/payment_adapter.rs +++ b/bridges/modules/relayers/src/payment_adapter.rs @@ -20,7 +20,7 @@ use crate::{Config, Pallet}; use bp_messages::{ source_chain::{DeliveryConfirmationPayments, RelayersRewards}, - LaneId, + LaneId, MessageNonce, }; use bp_relayers::{RewardsAccountOwner, RewardsAccountParams}; use frame_support::{sp_runtime::SaturatedConversion, traits::Get}; @@ -47,9 +47,10 @@ where messages_relayers: VecDeque>, confirmation_relayer: &T::AccountId, received_range: &RangeInclusive, - ) { + ) -> MessageNonce { let relayers_rewards = bp_messages::calc_relayers_rewards::(messages_relayers, received_range); + let rewarded_relayers = relayers_rewards.len(); register_relayers_rewards::( confirmation_relayer, @@ -61,6 +62,8 @@ where ), DeliveryReward::get(), ); + + rewarded_relayers as _ } } diff --git a/bridges/primitives/messages/src/source_chain.rs b/bridges/primitives/messages/src/source_chain.rs index 83f27843d638..394a934171fe 100644 --- a/bridges/primitives/messages/src/source_chain.rs +++ b/bridges/primitives/messages/src/source_chain.rs @@ -98,12 +98,14 @@ pub trait DeliveryConfirmationPayments { /// /// The implementation may also choose to pay reward to the `confirmation_relayer`, which is /// a relayer that has submitted delivery confirmation transaction. + /// + /// Returns number of actually rewarded relayers. fn pay_reward( lane_id: LaneId, messages_relayers: VecDeque>, confirmation_relayer: &AccountId, received_range: &RangeInclusive, - ); + ) -> MessageNonce; } impl DeliveryConfirmationPayments for () { @@ -114,8 +116,9 @@ impl DeliveryConfirmationPayments for () { _messages_relayers: VecDeque>, _confirmation_relayer: &AccountId, _received_range: &RangeInclusive, - ) { + ) -> MessageNonce { // this implementation is not rewarding relayers at all + 0 } } @@ -202,6 +205,7 @@ impl DeliveryConfirmationPayments for ForbidOutboundMessag _messages_relayers: VecDeque>, _confirmation_relayer: &AccountId, _received_range: &RangeInclusive, - ) { + ) -> MessageNonce { + 0 } }