XCMP weight metering: account for the MQ page position (#8344)

Related to #8308
Follow-up for #8021

---------

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

Author: serban300

Cargo.lock

@@ -34,6 +34,8 @@ xcm-executor = { workspace = true }
 # Cumulus
 cumulus-primitives-core = { workspace = true }
 
+# Optional import for weight accuracy testing
+approx = { workspace = true }
 # Optional import for benchmarking
 bounded-collections = { workspace = true }
 frame-benchmarking = { optional = true, workspace = true }
@@ -53,6 +55,7 @@ cumulus-pallet-parachain-system = { workspace = true, default-features = true }
 [features]
 default = ["std"]
 std = [
+	"approx/std",
 	"bounded-collections/std",
 	"bp-xcm-bridge-hub-router?/std",
 	"codec/std",

cumulus/pallets/xcmp-queue/Cargo.toml

@@ -15,7 +15,7 @@
 
 //! Benchmarking setup for cumulus-pallet-xcmp-queue
 
-use crate::*;
+use crate::{weights_ext::get_average_page_pos, *};
 
 use alloc::vec;
 use codec::DecodeAll;
@@ -107,6 +107,40 @@ mod benchmarks {
 		}
 	}
 
+	/// Add an XCMP message of 0 bytes to the message queue at the provided position
+	/// on an existing page.
+	#[benchmark]
+	fn enqueue_empty_xcmp_message_at(
+		n: Linear<0, { crate::MaxXcmpMessageLenOf::<T>::get() - 10 }>,
+	) {
+		#[cfg(test)]
+		{
+			mock::EnqueuedMessages::set(vec![]);
+		}
+
+		assert_ok!(Pallet::<T>::enqueue_xcmp_messages(
+			0.into(),
+			&[BoundedVec::try_from(vec![0; n as usize]).unwrap()],
+			&mut WeightMeter::new()
+		));
+
+		#[cfg(not(test))]
+		let fp_before = T::XcmpQueue::footprint(0.into());
+		#[block]
+		{
+			assert_ok!(Pallet::<T>::enqueue_xcmp_messages(
+				0.into(),
+				&[Default::default()],
+				&mut WeightMeter::new()
+			));
+		}
+		#[cfg(not(test))]
+		{
+			let fp_after = T::XcmpQueue::footprint(0.into());
+			assert_eq!(fp_after.ready_pages, fp_before.ready_pages);
+		}
+	}
+
 	/// Add `n` pages to the message queue.
 	///
 	/// We add one page by enqueueing a maximal size message which fills it.
@@ -157,6 +191,17 @@ mod benchmarks {
 			});
 		}
 
+		assert_ok!(Pallet::<T>::enqueue_xcmp_messages(
+			0.into(),
+			&[BoundedVec::try_from(vec![
+				0;
+				get_average_page_pos(MaxXcmpMessageLenOf::<T>::get())
+					as usize
+			])
+			.unwrap()],
+			&mut WeightMeter::new()
+		));
+
 		let mut msgs = vec![];
 		for _i in 0..1000 {
 			msgs.push(BoundedVec::try_from(vec![0; 3]).unwrap());
@@ -175,7 +220,7 @@ mod benchmarks {
 		#[cfg(not(test))]
 		{
 			let fp_after = T::XcmpQueue::footprint(0.into());
-			assert_eq!(fp_after.ready_pages, fp_before.ready_pages + 1);
+			assert_eq!(fp_after.ready_pages, fp_before.ready_pages);
 		}
 	}
 

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

@@ -51,8 +51,6 @@ pub mod weights;
 pub mod weights_ext;
 
 pub use weights::WeightInfo;
-#[cfg(feature = "std")]
-pub use weights_ext::check_weight_info_ext_accuracy;
 pub use weights_ext::WeightInfoExt;
 
 extern crate alloc;
@@ -68,8 +66,8 @@ use cumulus_primitives_core::{
 use frame_support::{
 	defensive, defensive_assert,
 	traits::{
-		BatchFootprint, Defensive, EnqueueMessage, EnsureOrigin, Get, QueueFootprint,
-		QueueFootprintQuery, QueuePausedQuery,
+		Defensive, EnqueueMessage, EnsureOrigin, Get, QueueFootprint, QueueFootprintQuery,
+		QueuePausedQuery,
 	},
 	weights::{Weight, WeightMeter},
 	BoundedVec,
@@ -79,7 +77,7 @@ use polkadot_runtime_common::xcm_sender::PriceForMessageDelivery;
 use polkadot_runtime_parachains::{FeeTracker, GetMinFeeFactor};
 use scale_info::TypeInfo;
 use sp_core::MAX_POSSIBLE_ALLOCATION;
-use sp_runtime::{FixedU128, RuntimeDebug, WeakBoundedVec};
+use sp_runtime::{FixedU128, RuntimeDebug, SaturatedConversion, WeakBoundedVec};
 use xcm::{latest::prelude::*, VersionedLocation, VersionedXcm, WrapVersion, MAX_XCM_DECODE_DEPTH};
 use xcm_builder::InspectMessageQueues;
 use xcm_executor::traits::ConvertOrigin;
@@ -264,9 +262,13 @@ pub mod pallet {
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
 		fn integrity_test() {
+			assert!(!T::MaxPageSize::get().is_zero(), "MaxPageSize too low");
+
 			let w = Self::on_idle_weight();
 			assert!(w != Weight::zero());
 			assert!(w.all_lte(T::BlockWeights::get().max_block));
+
+			<T::WeightInfo as WeightInfoExt>::check_accuracy::<MaxXcmpMessageLenOf<T>>(0.15);
 		}
 
 		fn on_idle(_block: BlockNumberFor<T>, limit: Weight) -> Weight {
@@ -643,39 +645,21 @@ impl<T: Config> Pallet<T> {
 			drop_threshold,
 		);
 
-		// `batches_footprints[n]` contains the footprint of the batch `xcms[0..n]`,
-		// so as `n` increases `batches_footprints[n]` contains the footprint of a bigger batch.
-		let best_batch_idx = batches_footprints.binary_search_by(|batch_info| {
+		let best_batch_footprint = batches_footprints.search_best_by(|batch_info| {
 			let required_weight = T::WeightInfo::enqueue_xcmp_messages(
-				batch_info.new_pages_count,
-				batch_info.msgs_count,
-				batch_info.size_in_bytes,
+				batches_footprints.first_page_pos.saturated_into(),
+				batch_info,
 			);
 
 			match meter.can_consume(required_weight) {
 				true => core::cmp::Ordering::Less,
 				false => core::cmp::Ordering::Greater,
 			}
 		});
-		let best_batch_idx = match best_batch_idx {
-			Ok(last_ok_idx) => {
-				// We should never reach this branch since we never return `Ordering::Equal`.
-				defensive!("Unexpected best_batch_idx found: Ok({})", last_ok_idx);
-				Some(last_ok_idx)
-			},
-			Err(first_err_idx) => first_err_idx.checked_sub(1),
-		};
-		let best_batch_footprint = match best_batch_idx {
-			Some(best_batch_idx) => batches_footprints.get(best_batch_idx).ok_or_else(|| {
-				defensive!("Invalid best_batch_idx: {}", best_batch_idx);
-			})?,
-			None => &BatchFootprint { msgs_count: 0, size_in_bytes: 0, new_pages_count: 0 },
-		};
 
 		meter.consume(T::WeightInfo::enqueue_xcmp_messages(
-			best_batch_footprint.new_pages_count,
-			best_batch_footprint.msgs_count,
-			best_batch_footprint.size_in_bytes,
+			batches_footprints.first_page_pos.saturated_into(),
+			best_batch_footprint,
 		));
 		T::XcmpQueue::enqueue_messages(
 			xcms.iter()

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

@@ -20,7 +20,7 @@ use cumulus_pallet_parachain_system::AnyRelayNumber;
 use cumulus_primitives_core::{ChannelInfo, IsSystem, ParaId};
 use frame_support::{
 	derive_impl, parameter_types,
-	traits::{ConstU32, Everything, OriginTrait},
+	traits::{BatchesFootprints, ConstU32, Everything, OriginTrait},
 	BoundedSlice,
 };
 use frame_system::EnsureRoot;
@@ -193,24 +193,16 @@ impl<T: OnQueueChanged<ParaId>> QueueFootprintQuery<ParaId> for EnqueueToLocalSt
 		origin: ParaId,
 		msgs: impl Iterator<Item = BoundedSlice<'a, u8, Self::MaxMessageLen>>,
 		total_pages_limit: u32,
-	) -> Vec<BatchFootprint> {
+	) -> BatchesFootprints {
 		// Let's consider that we add one message per page
 		let footprint = Self::footprint(origin);
-		let mut batches_footprints = vec![];
-		let mut new_pages_count = 0;
-		let mut total_size = 0;
+		let mut batches_footprints = BatchesFootprints::default();
 		for (idx, msg) in msgs.enumerate() {
-			new_pages_count += 1;
-			if footprint.pages + new_pages_count > total_pages_limit {
+			if footprint.pages + idx as u32 + 1 > total_pages_limit {
 				break;
 			}
 
-			total_size += msg.len();
-			batches_footprints.push(BatchFootprint {
-				msgs_count: idx + 1,
-				size_in_bytes: total_size,
-				new_pages_count,
-			})
+			batches_footprints.push(msg.into(), true);
 		}
 		batches_footprints
 	}

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

@@ -22,7 +22,8 @@ use XcmpMessageFormat::*;
 use codec::Input;
 use cumulus_primitives_core::{ParaId, XcmpMessageHandler};
 use frame_support::{
-	assert_err, assert_noop, assert_ok, assert_storage_noop, hypothetically, traits::Hooks,
+	assert_err, assert_noop, assert_ok, assert_storage_noop, hypothetically,
+	traits::{BatchFootprint, Hooks},
 	StorageNoopGuard,
 };
 use mock::{new_test_ext, ParachainSystem, RuntimeOrigin as Origin, Test, XcmpQueue};
@@ -210,9 +211,12 @@ fn xcm_enqueueing_starts_dropping_on_out_of_weight() {
 
 			total_size += xcm.len();
 			let required_weight = <<Test as Config>::WeightInfo>::enqueue_xcmp_messages(
-				idx as u32 + 1,
-				idx + 1,
-				total_size,
+				0,
+				&BatchFootprint {
+					msgs_count: idx + 1,
+					size_in_bytes: total_size,
+					new_pages_count: idx as u32 + 1,
+				},
 			);
 
 			let mut weight_meter = WeightMeter::with_limit(required_weight);