Improve XCMP weight metering (paritytech#7963)

This PR Improves the weight metering for the `enqueue_xcmp_message()`
method, by accounting for cached reads/writes and for the size of the
enqueued messages

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: serban300

cumulus/pallets/xcmp-queue/src/benchmarking.rs

@@ -20,7 +20,7 @@ use crate::*;
 use alloc::vec;
 use codec::DecodeAll;
 use frame_benchmarking::v2::*;
-use frame_support::traits::Hooks;
+use frame_support::{assert_ok, traits::Hooks};
 use frame_system::RawOrigin;
 use xcm::MAX_INSTRUCTIONS_TO_DECODE;
 
@@ -40,15 +40,48 @@ mod benchmarks {
 		Pallet::<T>::update_resume_threshold(RawOrigin::Root, 1);
 	}
 
+	/// Add a XCMP message of `n` bytes to the message queue.
+	///
+	/// The message will be added on a new page and also, the `BookState` will be added
+	/// to the ready ring.
 	#[benchmark]
-	fn enqueue_xcmp_message() {
-		assert!(QueueConfig::<T>::get().drop_threshold * MaxXcmpMessageLenOf::<T>::get() > 1000);
-		let msg = BoundedVec::<u8, MaxXcmpMessageLenOf<T>>::default();
+	fn enqueue_n_bytes_xcmp_message(n: Linear<1, { MaxXcmpMessageLenOf::<T>::get() }>) {
+		#[cfg(test)]
+		{
+			mock::EnqueuedMessages::set(vec![]);
+		}
+
+		let msg = BoundedVec::<u8, MaxXcmpMessageLenOf<T>>::try_from(vec![0; n as usize]).unwrap();
+		let fp_before = T::XcmpQueue::footprint(0.into());
+		#[block]
+		{
+			assert_ok!(Pallet::<T>::enqueue_xcmp_message(0.into(), msg));
+		}
+		let fp_after = T::XcmpQueue::footprint(0.into());
+		assert_eq!(fp_after.ready_pages, fp_before.ready_pages + 1);
+	}
+
+	/// Add 2 XCMP message of 0 bytes to the message queue.
+	///
+	/// Only for the first message a new page will be created and the `BookState` will be added
+	/// to the ready ring.
+	#[benchmark]
+	fn enqueue_2_empty_xcmp_messages() {
+		#[cfg(test)]
+		{
+			mock::EnqueuedMessages::set(vec![]);
+		}
 
+		let msg_1 = BoundedVec::<u8, MaxXcmpMessageLenOf<T>>::default();
+		let msg_2 = BoundedVec::<u8, MaxXcmpMessageLenOf<T>>::default();
+		let fp_before = T::XcmpQueue::footprint(0.into());
 		#[block]
 		{
-			Pallet::<T>::enqueue_xcmp_message(0.into(), msg, &mut WeightMeter::new()).unwrap();
+			assert_ok!(Pallet::<T>::enqueue_xcmp_message(0.into(), msg_1));
+			assert_ok!(Pallet::<T>::enqueue_xcmp_message(0.into(), msg_2));
 		}
+		let fp_after = T::XcmpQueue::footprint(0.into());
+		assert_eq!(fp_after.ready_pages, fp_before.ready_pages + 1);
 	}
 
 	#[benchmark]
@@ -94,7 +127,7 @@ mod benchmarks {
 	/// Split a singular XCM.
 	#[benchmark]
 	fn take_first_concatenated_xcm() {
-		let max_downward_message_size = MaxXcmpMessageLenOf::<T>::get() as usize;
+		let max_message_size = MaxXcmpMessageLenOf::<T>::get() as usize;
 
 		assert!(MAX_INSTRUCTIONS_TO_DECODE as u32 > MAX_XCM_DECODE_DEPTH, "Preconditon failed");
 		let max_instrs = MAX_INSTRUCTIONS_TO_DECODE as u32 - MAX_XCM_DECODE_DEPTH;
@@ -105,7 +138,7 @@ mod benchmarks {
 		}
 
 		let data = VersionedXcm::<T>::from(xcm).encode();
-		assert!(data.len() < max_downward_message_size, "Page size is too small");
+		assert!(data.len() < max_message_size, "Page size is too small");
 		// Verify that decoding works with the exact recursion limit:
 		VersionedXcm::<T::RuntimeCall>::decode_all_with_depth_limit(
 			MAX_XCM_DECODE_DEPTH,

cumulus/pallets/xcmp-queue/src/lib.rs

@@ -48,7 +48,10 @@ mod benchmarking;
 #[cfg(feature = "bridging")]
 pub mod bridging;
 pub mod weights;
+pub mod weights_ext;
+
 pub use weights::WeightInfo;
+pub use weights_ext::WeightInfoExt;
 
 extern crate alloc;
 
@@ -164,7 +167,7 @@ pub mod pallet {
 		type PriceForSiblingDelivery: PriceForMessageDelivery<Id = ParaId>;
 
 		/// The weight information of this pallet.
-		type WeightInfo: WeightInfo;
+		type WeightInfo: WeightInfoExt;
 	}
 
 	#[pallet::call]
@@ -637,13 +640,7 @@ impl<T: Config> Pallet<T> {
 	fn enqueue_xcmp_message(
 		sender: ParaId,
 		xcm: BoundedVec<u8, MaxXcmpMessageLenOf<T>>,
-		meter: &mut WeightMeter,
 	) -> Result<(), ()> {
-		if meter.try_consume(T::WeightInfo::enqueue_xcmp_message()).is_err() {
-			defensive!("Out of weight: cannot enqueue XCMP messages; dropping msg");
-			return Err(())
-		}
-
 		let QueueConfigData { drop_threshold, .. } = <QueueConfig<T>>::get();
 		let fp = T::XcmpQueue::footprint(sender);
 		// Assume that it will not fit into the current page:
@@ -791,22 +788,46 @@ impl<T: Config> XcmpMessageHandler for Pallet<T> {
 							},
 						}
 					},
-				XcmpMessageFormat::ConcatenatedVersionedXcm =>
+				XcmpMessageFormat::ConcatenatedVersionedXcm => {
+					// We need to know if the current message is the first on the current XCMP page
+					// for weight metering accuracy.
+					let mut is_first_xcm_on_page = true;
 					while !data.is_empty() {
 						let Ok(xcm) = Self::take_first_concatenated_xcm(&mut data, &mut meter)
 						else {
 							defensive!("HRMP inbound decode stream broke; page will be dropped.",);
 							break
 						};
 
-						if let Err(()) = Self::enqueue_xcmp_message(sender, xcm, &mut meter) {
+						// For simplicity, we consider that each new XCMP page results in a new
+						// message queue page. This is not always true, but it's a good enough
+						// estimation.
+						if meter
+							.try_consume(T::WeightInfo::enqueue_xcmp_message(
+								xcm.len(),
+								is_first_xcm_on_page,
+							))
+							.is_err()
+						{
+							defensive!(
+								"Out of weight: cannot enqueue XCMP messages; dropping msg; \
+								Used weight: ",
+								meter.consumed_ratio()
+							);
+							break;
+						}
+
+						if let Err(()) = Self::enqueue_xcmp_message(sender, xcm) {
 							defensive!(
 								"Could not enqueue XCMP messages. Used weight: ",
 								meter.consumed_ratio()
 							);
 							break
 						}
-					},
+
+						is_first_xcm_on_page = false;
+					}
+				},
 				XcmpMessageFormat::ConcatenatedEncodedBlob => {
 					defensive!("Blob messages are unhandled - dropping");
 					continue

cumulus/pallets/xcmp-queue/src/mock.rs

@@ -15,7 +15,7 @@
 
 use super::*;
 use crate as xcmp_queue;
-use core::marker::PhantomData;
+use core::{cmp::max, marker::PhantomData};
 use cumulus_pallet_parachain_system::AnyRelayNumber;
 use cumulus_primitives_core::{ChannelInfo, IsSystem, ParaId};
 use frame_support::{
@@ -144,7 +144,7 @@ parameter_types! {
 pub struct EnqueueToLocalStorage<T>(PhantomData<T>);
 
 impl<T: OnQueueChanged<ParaId>> EnqueueMessage<ParaId> for EnqueueToLocalStorage<T> {
-	type MaxMessageLen = sp_core::ConstU32<65_536>;
+	type MaxMessageLen = sp_core::ConstU32<256>;
 
 	fn enqueue_message(message: BoundedSlice<u8, Self::MaxMessageLen>, origin: ParaId) {
 		let mut msgs = EnqueuedMessages::get();
@@ -179,7 +179,11 @@ impl<T: OnQueueChanged<ParaId>> EnqueueMessage<ParaId> for EnqueueToLocalStorage
 				footprint.storage.size += m.len() as u64;
 			}
 		}
-		footprint.pages = footprint.storage.size as u32 / 16; // Number does not matter
+		footprint.pages =
+			(footprint.storage.size as u32).div_ceil(<Self::MaxMessageLen as Get<u32>>::get());
+		if footprint.storage.count > 0 {
+			footprint.pages = max(footprint.pages, 1);
+		}
 		footprint.ready_pages = footprint.pages;
 		footprint
 	}
@@ -228,7 +232,7 @@ pub struct MockedChannelInfo;
 impl GetChannelInfo for MockedChannelInfo {
 	fn get_channel_status(id: ParaId) -> ChannelStatus {
 		if id == HRMP_PARA_ID.into() {
-			return ChannelStatus::Ready(usize::MAX, usize::MAX)
+			return ChannelStatus::Ready(usize::MAX, usize::MAX);
 		}
 
 		ParachainSystem::get_channel_status(id)
@@ -242,7 +246,7 @@ impl GetChannelInfo for MockedChannelInfo {
 				max_message_size: u32::MAX,
 				msg_count: 0,
 				total_size: 0,
-			})
+			});
 		}
 
 		ParachainSystem::get_channel_info(id)

cumulus/pallets/xcmp-queue/src/tests.rs

@@ -104,10 +104,16 @@ fn xcm_enqueueing_multiple_times_works() {
 #[cfg_attr(debug_assertions, should_panic = "Could not enqueue XCMP messages.")]
 fn xcm_enqueueing_starts_dropping_on_overflow() {
 	new_test_ext().execute_with(|| {
-		let xcm = VersionedXcm::<Test>::from(Xcm::<Test>(vec![ClearOrigin]));
+		let xcm = VersionedXcm::<Test>::from(Xcm::<Test>(vec![
+			ClearOrigin;
+			MAX_INSTRUCTIONS_TO_DECODE as usize
+		]));
 		let data = (ConcatenatedVersionedXcm, xcm).encode();
-		// Its possible to enqueue 256 messages at most:
-		let limit = 256;
+		// It's possible to enqueue at most `limit` messages:
+		let max_message_len: u32 =
+			<<Test as Config>::XcmpQueue as EnqueueMessage<ParaId>>::MaxMessageLen::get();
+		let drop_threshold = <QueueConfig<Test>>::get().drop_threshold;
+		let limit = max_message_len as usize / data.len() * drop_threshold as usize;
 
 		XcmpQueue::handle_xcmp_messages(
 			repeat((1000.into(), 1, data.as_slice())).take(limit * 2),