refund extra weight in receive_messages_delivery_proof call (#2031)

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<T>,
 			proof: MessagesDeliveryProofOf<T, I>,
-			relayers_state: UnrewardedRelayersState,
-		) -> DispatchResult {
+			mut relayers_state: UnrewardedRelayersState,
+		) -> DispatchResultWithPostInfo {
 			Self::ensure_not_halted().map_err(Error::<T, I>::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::<TestRuntime>::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::<TestRuntime>::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::<TestRuntime>::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::<TestRuntime>::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));
 		});

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<AccountId> for TestDeliveryConfirmationPayment
 		messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
 		_confirmation_relayer: &AccountId,
 		received_range: &RangeInclusive<MessageNonce>,
-	) {
+	) -> 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 _
 	}
 }
 

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<bp_messages::UnrewardedRelayer<T::AccountId>>,
 		confirmation_relayer: &T::AccountId,
 		received_range: &RangeInclusive<bp_messages::MessageNonce>,
-	) {
+	) -> MessageNonce {
 		let relayers_rewards =
 			bp_messages::calc_relayers_rewards::<T::AccountId>(messages_relayers, received_range);
+		let rewarded_relayers = relayers_rewards.len();
 
 		register_relayers_rewards::<T>(
 			confirmation_relayer,
@@ -61,6 +62,8 @@ where
 			),
 			DeliveryReward::get(),
 		);
+
+		rewarded_relayers as _
 	}
 }
 

bridges/primitives/messages/src/source_chain.rs

@@ -98,12 +98,14 @@ pub trait DeliveryConfirmationPayments<AccountId> {
 	///
 	/// 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<UnrewardedRelayer<AccountId>>,
 		confirmation_relayer: &AccountId,
 		received_range: &RangeInclusive<MessageNonce>,
-	);
+	) -> MessageNonce;
 }
 
 impl<AccountId> DeliveryConfirmationPayments<AccountId> for () {
@@ -114,8 +116,9 @@ impl<AccountId> DeliveryConfirmationPayments<AccountId> for () {
 		_messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
 		_confirmation_relayer: &AccountId,
 		_received_range: &RangeInclusive<MessageNonce>,
-	) {
+	) -> MessageNonce {
 		// this implementation is not rewarding relayers at all
+		0
 	}
 }
 
@@ -202,6 +205,7 @@ impl<AccountId> DeliveryConfirmationPayments<AccountId> for ForbidOutboundMessag
 		_messages_relayers: VecDeque<UnrewardedRelayer<AccountId>>,
 		_confirmation_relayer: &AccountId,
 		_received_range: &RangeInclusive<MessageNonce>,
-	) {
+	) -> MessageNonce {
+		0
 	}
 }