From 712ccbb742a87b37d3a764346ab095201cd2d3d6 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 28 Jul 2021 15:44:19 +0300 Subject: [PATCH] increase_message_fee depends on stored mesage size (#1066) --- bridges/modules/messages/src/benchmarking.rs | 25 +++++++- bridges/modules/messages/src/lib.rs | 60 ++++++++++++++++++-- bridges/modules/messages/src/mock.rs | 5 +- bridges/modules/messages/src/weights.rs | 23 ++++++-- 4 files changed, 100 insertions(+), 13 deletions(-) diff --git a/bridges/modules/messages/src/benchmarking.rs b/bridges/modules/messages/src/benchmarking.rs index cab2704f40..e2a3237eb0 100644 --- a/bridges/modules/messages/src/benchmarking.rs +++ b/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::(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; diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index 12d8ab342b..3019129e03 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/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::()?; // 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::::mutate(message_key, |message_data| { + let message_size = OutboundMessages::::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::::send_message( + Origin::signed(1), + TEST_LANE_ID, + small_payload, + 100, + )); + assert_ok!(Pallet::::send_message( + Origin::signed(1), + TEST_LANE_ID, + large_payload, + 100, + )); + + let small_weight = Pallet::::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::::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(|| { diff --git a/bridges/modules/messages/src/mock.rs b/bridges/modules/messages/src/mock.rs index 9d361b53cf..063bbfaa5a 100644 --- a/bridges/modules/messages/src/mock.rs +++ b/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, } 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(), } } diff --git a/bridges/modules/messages/src/weights.rs b/bridges/modules/messages/src/weights.rs index 9b65c8217a..b3028c8fcc 100644 --- a/bridges/modules/messages/src/weights.rs +++ b/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 WeightInfo for RialtoWeight { .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)) }