increase_message_fee depends on stored mesage size (#1066)

bridges/modules/messages/src/benchmarking.rs

@@ -38,7 +38,7 @@ use sp_std::{
 };
 
 /// Fee paid by submitter for single message delivery.
-pub const MESSAGE_FEE: u64 = 10_000_000_000;
+pub const MESSAGE_FEE: u64 = 100_000_000_000;
 
 const SEED: u32 = 0;
 
@@ -237,7 +237,9 @@ benchmarks_instance! {
 	// Benchmark `increase_message_fee` with following conditions:
 	// * message has maximal message;
 	// * submitter account is killed because its balance is less than ED after payment.
-	increase_message_fee {
+	//
+	// Result of this benchmark is directly used by weight formula of the call.
+	maximal_increase_message_fee {
 		let sender = account("sender", 42, SEED);
 		T::endow_account(&sender);
 
@@ -251,6 +253,25 @@ benchmarks_instance! {
 		assert_eq!(T::account_balance(&sender), 0.into());
 	}
 
+	// Benchmark `increase_message_fee` with following conditions:
+	// * message size varies from minimal to maximal;
+	// * submitter account is killed because its balance is less than ED after payment.
+	increase_message_fee {
+		let i in 0..T::maximal_message_size().try_into().unwrap_or_default();
+
+		let sender = account("sender", 42, SEED);
+		T::endow_account(&sender);
+
+		let additional_fee = T::account_balance(&sender);
+		let lane_id = T::bench_lane_id();
+		let nonce = 1;
+
+		send_regular_message_with_payload::<T, I>(vec![42u8; i as _]);
+	}: increase_message_fee(RawOrigin::Signed(sender.clone()), lane_id, nonce, additional_fee)
+	verify {
+		assert_eq!(T::account_balance(&sender), 0.into());
+	}
+
 	// Benchmark `receive_messages_proof` extrinsic with single minimal-weight message and following conditions:
 	// * proof does not include outbound lane state proof;
 	// * inbound lane already has state, so it needs to be read and decoded;

bridges/modules/messages/src/lib.rs

@@ -66,7 +66,7 @@ use frame_support::{
 };
 use frame_system::{ensure_signed, RawOrigin};
 use num_traits::{SaturatingAdd, Zero};
-use sp_runtime::{traits::BadOrigin, DispatchResult};
+use sp_runtime::traits::BadOrigin;
 use sp_std::{cell::RefCell, cmp::PartialOrd, marker::PhantomData, prelude::*};
 
 mod inbound_lane;
@@ -407,13 +407,13 @@ decl_module! {
 		}
 
 		/// Pay additional fee for the message.
-		#[weight = T::WeightInfo::increase_message_fee()]
+		#[weight = T::WeightInfo::maximal_increase_message_fee()]
 		pub fn increase_message_fee(
 			origin,
 			lane_id: LaneId,
 			nonce: MessageNonce,
 			additional_fee: T::OutboundMessageFee,
-		) -> DispatchResult {
+		) -> DispatchResultWithPostInfo {
 			ensure_not_halted::<T, I>()?;
 			// if someone tries to pay for already-delivered message, we're rejecting this intention
 			// (otherwise this additional fee will be locked forever in relayers fund)
@@ -446,7 +446,7 @@ decl_module! {
 
 			// and finally update fee in the storage
 			let message_key = MessageKey { lane_id, nonce };
-			OutboundMessages::<T, I>::mutate(message_key, |message_data| {
+			let message_size = OutboundMessages::<T, I>::mutate(message_key, |message_data| {
 				// saturating_add is fine here - overflow here means that someone controls all
 				// chain funds, which shouldn't ever happen + `pay_delivery_and_dispatch_fee`
 				// above will fail before we reach here
@@ -454,9 +454,19 @@ decl_module! {
 					.as_mut()
 					.expect("the message is sent and not yet delivered; so it is in the storage; qed");
 				message_data.fee = message_data.fee.saturating_add(&additional_fee);
+				message_data.payload.len()
 			});
 
-			Ok(())
+			// compute actual dispatch weight that depends on the stored message size
+			let actual_weight = sp_std::cmp::min(
+				T::WeightInfo::maximal_increase_message_fee(),
+				T::WeightInfo::increase_message_fee(message_size as _),
+			);
+
+			Ok(PostDispatchInfo {
+				actual_weight: Some(actual_weight),
+				pays_fee: Pays::Yes,
+			})
 		}
 
 		/// Receive messages proof from bridged chain.
@@ -2118,6 +2128,46 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn increase_message_fee_weight_depends_on_message_size() {
+		run_test(|| {
+			let mut small_payload = message_payload(0, 100);
+			let mut large_payload = message_payload(1, 100);
+			small_payload.extra = vec![1; 100];
+			large_payload.extra = vec![2; 16_384];
+
+			assert_ok!(Pallet::<TestRuntime>::send_message(
+				Origin::signed(1),
+				TEST_LANE_ID,
+				small_payload,
+				100,
+			));
+			assert_ok!(Pallet::<TestRuntime>::send_message(
+				Origin::signed(1),
+				TEST_LANE_ID,
+				large_payload,
+				100,
+			));
+
+			let small_weight = Pallet::<TestRuntime>::increase_message_fee(Origin::signed(1), TEST_LANE_ID, 1, 1)
+				.expect("increase_message_fee has failed")
+				.actual_weight
+				.expect("increase_message_fee always returns Some");
+
+			let large_weight = Pallet::<TestRuntime>::increase_message_fee(Origin::signed(1), TEST_LANE_ID, 2, 1)
+				.expect("increase_message_fee has failed")
+				.actual_weight
+				.expect("increase_message_fee always returns Some");
+
+			assert!(
+				large_weight > small_weight,
+				"Actual post-dispatch weigth for larger message {} must be larger than {} for small message",
+				large_weight,
+				small_weight,
+			);
+		});
+	}
+
 	#[test]
 	fn weight_is_refunded_for_messages_that_are_not_pruned() {
 		run_test(|| {

bridges/modules/messages/src/mock.rs

@@ -56,6 +56,8 @@ pub struct TestPayload {
 	/// Note: in correct code `dispatch_result.unspent_weight` will always be <= `declared_weight`, but for test
 	/// purposes we'll be making it larger than `declared_weight` sometimes.
 	pub dispatch_result: MessageDispatchResult,
+	/// Extra bytes that affect payload size.
+	pub extra: Vec<u8>,
 }
 pub type TestMessageFee = u64;
 pub type TestRelayer = u64;
@@ -183,7 +185,7 @@ impl Config for TestRuntime {
 
 impl Size for TestPayload {
 	fn size_hint(&self) -> u32 {
-		16
+		16 + self.extra.len() as u32
 	}
 }
 
@@ -466,6 +468,7 @@ pub const fn message_payload(id: u64, declared_weight: Weight) -> TestPayload {
 		id,
 		declared_weight,
 		dispatch_result: dispatch_result(0),
+		extra: Vec::new(),
 	}
 }
 

bridges/modules/messages/src/weights.rs

@@ -51,7 +51,8 @@ pub trait WeightInfo {
 	fn send_minimal_message_worst_case() -> Weight;
 	fn send_1_kb_message_worst_case() -> Weight;
 	fn send_16_kb_message_worst_case() -> Weight;
-	fn increase_message_fee() -> Weight;
+	fn maximal_increase_message_fee() -> Weight;
+	fn increase_message_fee(i: u32) -> Weight;
 	fn receive_single_message_proof() -> Weight;
 	fn receive_two_messages_proof() -> Weight;
 	fn receive_single_message_proof_with_outbound_lane_state() -> Weight;
@@ -88,8 +89,14 @@ impl<T: frame_system::Config> WeightInfo for RialtoWeight<T> {
 			.saturating_add(T::DbWeight::get().reads(5 as Weight))
 			.saturating_add(T::DbWeight::get().writes(12 as Weight))
 	}
-	fn increase_message_fee() -> Weight {
-		(6_709_925_000 as Weight)
+	fn maximal_increase_message_fee() -> Weight {
+		(6_781_470_000 as Weight)
+			.saturating_add(T::DbWeight::get().reads(5 as Weight))
+			.saturating_add(T::DbWeight::get().writes(3 as Weight))
+	}
+	fn increase_message_fee(i: u32) -> Weight {
+		(114_963_000 as Weight)
+			.saturating_add((6_000 as Weight).saturating_mul(i as Weight))
 			.saturating_add(T::DbWeight::get().reads(5 as Weight))
 			.saturating_add(T::DbWeight::get().writes(3 as Weight))
 	}
@@ -202,8 +209,14 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().reads(5 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(12 as Weight))
 	}
-	fn increase_message_fee() -> Weight {
-		(6_709_925_000 as Weight)
+	fn maximal_increase_message_fee() -> Weight {
+		(6_781_470_000 as Weight)
+			.saturating_add(RocksDbWeight::get().reads(5 as Weight))
+			.saturating_add(RocksDbWeight::get().writes(3 as Weight))
+	}
+	fn increase_message_fee(i: u32) -> Weight {
+		(114_963_000 as Weight)
+			.saturating_add((6_000 as Weight).saturating_mul(i as Weight))
 			.saturating_add(RocksDbWeight::get().reads(5 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(3 as Weight))
 	}