Move commit to on_idle with weight check (paritytech#758)

* Move to on_idle with weight check

* fix breaking tests

* fix tests

* Add comment about weight estimation

* Fix weight consumed

* Fix tests

parachain/pallets/basic-channel/src/outbound/benchmarking.rs

@@ -14,9 +14,9 @@ benchmarks! {
 	}
 	// Benchmark `on_initialize` under worst case conditions, i.e. messages
 	// in queue are committed.
-	on_initialize {
+	on_commit {
 		let m in 1 .. T::MaxMessagesPerCommit::get();
-		let p in 0 .. T::MaxMessagePayloadSize::get();
+		let p in 0 .. T::MaxMessagePayloadSize::get()-1;
 
 		for _ in 0 .. m {
 			let payload: Vec<u8> = (0..).take(p as usize).collect();
@@ -32,34 +32,14 @@ benchmarks! {
 
 		let block_number = Interval::<T>::get();
 
-	}: { BasicOutboundChannel::<T>::on_initialize(block_number) }
+	}: { BasicOutboundChannel::<T>::commit(Weight::MAX) }
 	verify {
 		assert_eq!(<MessageQueue<T>>::get().len(), 0);
 	}
 
-	// Benchmark 'on_initialize` for the best case, i.e. nothing is done
-	// because it's not a commitment interval.
-	on_initialize_non_interval {
-		<MessageQueue<T>>::try_append(EnqueuedMessage {
-			account: account("", 0, 0),
-			message: Message {
-				id: 0u64,
-				target: H160::zero(),
-				payload: vec![1u8; T::MaxMessagePayloadSize::get() as usize].try_into().unwrap(),
-			}
-		}).unwrap();
-
-		Interval::<T>::put::<T::BlockNumber>(10u32.into());
-		let block_number: T::BlockNumber = 11u32.into();
-
-	}: { BasicOutboundChannel::<T>::on_initialize(block_number) }
-	verify {
-		assert_eq!(<MessageQueue<T>>::get().len(), 1);
-	}
-
 	// Benchmark 'on_initialize` for the case where it is a commitment interval
 	// but there are no messages in the queue.
-	on_initialize_no_messages {
+	on_commit_no_messages {
 		<MessageQueue<T>>::kill();
 
 		let block_number = Interval::<T>::get();

parachain/pallets/basic-channel/src/outbound/mod.rs

@@ -9,12 +9,12 @@ mod test;
 use codec::{Decode, Encode, MaxEncodedLen};
 use ethabi::{self, Token};
 use frame_support::{
-	dispatch::DispatchResult, ensure, traits::Get, BoundedVec, CloneNoBound, PartialEqNoBound,
-	RuntimeDebugNoBound,
+	dispatch::DispatchResult, ensure, traits::Get, weights::Weight, BoundedVec, CloneNoBound,
+	PartialEqNoBound, RuntimeDebugNoBound,
 };
 use scale_info::TypeInfo;
 use sp_core::{H160, H256};
-use sp_runtime::traits::{Hash, Zero};
+use sp_runtime::traits::Hash;
 
 use sp_std::{collections::btree_map::BTreeMap, fmt::Debug, prelude::*};
 
@@ -103,6 +103,13 @@ pub type MessageBundleOf<T> = MessageBundle<
 	<T as Config>::MaxMessagesPerCommit,
 >;
 
+// base_weight=(0.75*0.5)*(10**12)=375_000_000_000
+// we leave the extra 10_000_000_000/375_000_000_000=2.66% as margin
+// so we can use at most 365000000000 for the commit call
+// need to rerun benchmarks later to get weight based on the worst case:
+// MaxMessagesPerCommit=20 and MaxMessagePayloadSize=256
+pub const MINIMUM_WEIGHT_REMAIN_IN_BLOCK: Weight = Weight::from_ref_time(10_000_000_000);
+
 pub use pallet::*;
 
 #[frame_support::pallet]
@@ -190,16 +197,18 @@ pub mod pallet {
 	where
 		T::AccountId: AsRef<[u8]>,
 	{
-		// Generate a message commitment every [`Interval`] blocks.
-		//
+		// Generate a message commitment when the chain is idle with enough remaining weight
 		// The commitment hash is included in an [`AuxiliaryDigestItem`] in the block header,
 		// with the corresponding commitment is persisted offchain.
-		fn on_initialize(now: T::BlockNumber) -> Weight {
-			if (now % Self::interval()).is_zero() {
-				Self::commit()
-			} else {
-				T::WeightInfo::on_initialize_non_interval()
+		fn on_idle(_n: T::BlockNumber, total_weight: Weight) -> Weight {
+			let weight_remaining = total_weight.saturating_sub(T::WeightInfo::on_commit(
+				T::MaxMessagesPerCommit::get(),
+				T::MaxMessagePayloadSize::get(),
+			));
+			if weight_remaining.ref_time() <= MINIMUM_WEIGHT_REMAIN_IN_BLOCK.ref_time() {
+				return total_weight
 			}
+			Self::commit(total_weight)
 		}
 	}
 
@@ -251,15 +260,15 @@ pub mod pallet {
 		/// - Emit an event with the commitment hash and SCALE-encoded message bundles for a
 		/// relayer to read.
 		/// - Persist the ethabi-encoded message bundles to off-chain storage.
-		fn commit() -> Weight {
+		pub fn commit(_total_weight: Weight) -> Weight {
 			// TODO: SNO-310 consider using mutate here. If some part of emitting message bundles
 			// fails, we don't want the MessageQueue to be empty.
 			let message_queue = <MessageQueue<T>>::take();
 			if message_queue.is_empty() {
-				return T::WeightInfo::on_initialize_no_messages()
+				return T::WeightInfo::on_commit_no_messages()
 			}
 
-			// Store these for the on_initialize call at the end
+			// Store these for the on_commit call at the end
 			let message_count = message_queue.len() as u32;
 			let average_payload_size = Self::average_payload_size(&message_queue);
 
@@ -284,7 +293,7 @@ pub mod pallet {
 
 			set(commitment_hash.as_bytes(), &eth_message_bundles.encode());
 
-			T::WeightInfo::on_initialize(message_count, average_payload_size)
+			return T::WeightInfo::on_commit(message_count, average_payload_size)
 		}
 
 		fn make_message_bundles(

parachain/pallets/basic-channel/src/outbound/test.rs

@@ -64,8 +64,8 @@ impl frame_system::Config for Test {
 }
 
 parameter_types! {
-	pub const MaxMessagePayloadSize: u32 = 128;
-	pub const MaxMessagesPerCommit: u32 = 5;
+	pub const MaxMessagePayloadSize: u32 = 256;
+	pub const MaxMessagesPerCommit: u32 = 20;
 }
 
 impl basic_outbound_channel::Config for Test {
@@ -134,8 +134,12 @@ fn test_submit_exceeds_payload_limit() {
 		let target = H160::zero();
 		let who: &AccountId = &Keyring::Bob.into();
 
-		let max_payload_bytes = MaxMessagePayloadSize::get();
-		let payload: Vec<u8> = (0..).take(max_payload_bytes as usize + 1).collect();
+		let max_payload_bytes = MaxMessagePayloadSize::get() - 1;
+
+		let mut payload: Vec<u8> = (0..).take(max_payload_bytes as usize).collect();
+		// Make payload oversize
+		payload.push(5);
+		payload.push(10);
 
 		assert_noop!(
 			BasicOutboundChannel::submit(who, target, payload.as_slice()),
@@ -152,6 +156,7 @@ fn test_commit_single_user() {
 
 		assert_ok!(BasicOutboundChannel::submit(who, target, &vec![0, 1, 2]));
 		run_to_block(2);
+		BasicOutboundChannel::commit(Weight::MAX);
 
 		assert_eq!(<NextId<Test>>::get(), 1);
 		assert_eq!(<Nonce<Test>>::get(who), 1);
@@ -169,6 +174,7 @@ fn test_commit_multi_user() {
 		assert_ok!(BasicOutboundChannel::submit(alice, target, &vec![0, 1, 2]));
 		assert_ok!(BasicOutboundChannel::submit(bob, target, &vec![0, 1, 2]));
 		run_to_block(2);
+		BasicOutboundChannel::commit(Weight::MAX);
 
 		assert_eq!(<NextId<Test>>::get(), 2);
 		assert_eq!(<Nonce<Test>>::get(alice), 1);

parachain/pallets/basic-channel/src/outbound/weights.rs

@@ -37,50 +37,41 @@ use sp_std::marker::PhantomData;
 
 /// Weight functions needed for basic_channel::outbound.
 pub trait WeightInfo {
-	fn on_initialize(m: u32, p: u32, ) -> Weight;
-	fn on_initialize_non_interval() -> Weight;
-	fn on_initialize_no_messages() -> Weight;
+	fn on_commit_no_messages() -> Weight;
+	fn on_commit(m: u32, p: u32, ) -> Weight;
 }
 
 /// Weights for basic_channel::outbound using the Snowbridge node and recommended hardware.
 pub struct SnowbridgeWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SnowbridgeWeight<T> {
-	fn on_initialize(m: u32, p: u32, ) -> Weight {
-		Weight::from_ref_time(0 as u64)
+	fn on_commit_no_messages() -> Weight {
+		Weight::from_ref_time(5_228_000 as u64)
+			.saturating_add(T::DbWeight::get().reads(2))
+	}
+	fn on_commit(m: u32, p: u32, ) -> Weight {
+		Weight::from_ref_time(3_294_000 as u64)
 			// Standard Error: 31_000
-			.saturating_add(Weight::from_ref_time(10_849_000 as u64).saturating_mul(m as u64))
+			.saturating_add(Weight::from_ref_time(100_849_000 as u64).saturating_mul(m as u64))
 			// Standard Error: 1_000
-			.saturating_add(Weight::from_ref_time(388_000 as u64).saturating_mul(p as u64))
+			.saturating_add(Weight::from_ref_time(3_880_000 as u64).saturating_mul(p as u64))
 			.saturating_add(T::DbWeight::get().reads(3 as u64))
 			.saturating_add(T::DbWeight::get().writes(2 as u64))
 	}
-	fn on_initialize_non_interval() -> Weight {
-		Weight::from_ref_time(3_294_000 as u64)
-			.saturating_add(T::DbWeight::get().reads(1))
-	}
-	fn on_initialize_no_messages() -> Weight {
-		Weight::from_ref_time(5_228_000 as u64)
-			.saturating_add(T::DbWeight::get().reads(2))
-	}
 }
 
 // For backwards compatibility and tests
 impl WeightInfo for () {
-	fn on_initialize(m: u32, p: u32, ) -> Weight {
+	fn on_commit_no_messages() -> Weight {
+		Weight::from_ref_time(5_228_000 as u64)
+			.saturating_add(RocksDbWeight::get().reads(2))
+	}
+	fn on_commit(m: u32, p: u32, ) -> Weight {
 		Weight::from_ref_time(0 as u64)
 			// Standard Error: 31_000
-			.saturating_add(Weight::from_ref_time(10_849_000 as u64).saturating_mul(m as u64))
+			.saturating_add(Weight::from_ref_time(100_849_000 as u64).saturating_mul(m as u64))
 			// Standard Error: 1_000
-			.saturating_add(Weight::from_ref_time(388_000 as u64).saturating_mul(p as u64))
+			.saturating_add(Weight::from_ref_time(3_880_000 as u64).saturating_mul(p as u64))
 			.saturating_add(RocksDbWeight::get().reads(3 as u64))
 			.saturating_add(RocksDbWeight::get().writes(2 as u64))
 	}
-	fn on_initialize_non_interval() -> Weight {
-		Weight::from_ref_time(3_294_000 as u64)
-			.saturating_add(RocksDbWeight::get().reads(1))
-	}
-	fn on_initialize_no_messages() -> Weight {
-		Weight::from_ref_time(5_228_000 as u64)
-			.saturating_add(RocksDbWeight::get().reads(2))
-	}
 }