fix(xcm): use Outcome::Incomplete for barrier rejection and improve weight accounting

polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs

@@ -26,7 +26,7 @@ use xcm::{
 	DoubleEncoded,
 };
 use xcm_executor::{
-	traits::{ConvertLocation, FeeReason, TransactAsset},
+	traits::{ConvertLocation, FeeReason, ShouldExecute, TransactAsset},
 	AssetsInHolding, ExecutorError, FeesMode,
 };
 
@@ -1006,6 +1006,26 @@ mod benchmarks {
 		Ok(())
 	}
 
+	#[benchmark]
+	fn barrier_check(i: Linear<1, 100>) -> Result<(), BenchmarkError> {
+		let origin = Location::parent();
+		let mut message = Xcm::<XcmCallOf<T>>(vec![ClearOrigin; i as usize]);
+		let mut properties =
+			xcm_executor::traits::Properties { weight_credit: Weight::zero(), message_id: None };
+
+		#[block]
+		{
+			let _ = <T::XcmConfig as xcm_executor::Config>::Barrier::should_execute(
+				&origin,
+				message.inner_mut(),
+				Weight::MAX,
+				&mut properties,
+			);
+		}
+
+		Ok(())
+	}
+
 	impl_benchmark_test_suite!(
 		Pallet,
 		crate::generic::mock::new_test_ext(),

polkadot/xcm/xcm-builder/src/barriers.rs

@@ -200,9 +200,9 @@ impl<InnerBarrier: ShouldExecute, LocalUniversal: Get<InteriorLocation>, MaxPref
 			|inst| {
 				match inst {
 					UniversalOrigin(new_global) => {
-						// Note the origin is *relative to local consensus*! So we need to escape
-						// local consensus with the `parents` before diving in into the
-						// `universal_location`.
+						// Note the origin is *relative to local consensus*! So we need to
+						// escape local consensus with the `parents` before diving in
+						// into the `universal_location`.
 						actual_origin =
 							Junctions::from([*new_global]).relative_to(&LocalUniversal::get());
 					},
@@ -529,7 +529,7 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain {
 
 /// Deny executing the XCM if it matches any of the Deny filter regardless of anything else.
 /// If it passes the Deny, and matches one of the Allow cases then it is let through.
-pub struct DenyThenTry<Deny, Allow>(PhantomData<Deny>, PhantomData<Allow>)
+pub struct DenyThenTry<Deny, Allow>(PhantomData<(Deny, Allow)>)
 where
 	Deny: DenyExecution,
 	Allow: ShouldExecute;
@@ -554,38 +554,11 @@ where
 pub struct DenyReserveTransferToRelayChain;
 impl DenyExecution for DenyReserveTransferToRelayChain {
 	fn deny_execution<RuntimeCall>(
-		origin: &Location,
-		message: &mut [Instruction<RuntimeCall>],
+		_origin: &Location,
+		_instructions: &mut [Instruction<RuntimeCall>],
 		_max_weight: Weight,
 		_properties: &mut Properties,
 	) -> Result<(), ProcessMessageError> {
-		message.matcher().match_next_inst_while(
-			|_| true,
-			|inst| match inst {
-				InitiateReserveWithdraw {
-					reserve: Location { parents: 1, interior: Here },
-					..
-				} |
-				DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } |
-				TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => {
-					Err(ProcessMessageError::Unsupported) // Deny
-				},
-
-				// An unexpected reserve transfer has arrived from the Relay Chain. Generally,
-				// `IsReserve` should not allow this, but we just log it here.
-				ReserveAssetDeposited { .. }
-					if matches!(origin, Location { parents: 1, interior: Here }) =>
-				{
-					tracing::debug!(
-						target: "xcm::barriers",
-						"Unexpected ReserveAssetDeposited from the Relay Chain",
-					);
-					Ok(ControlFlow::Continue(()))
-				},
-
-				_ => Ok(ControlFlow::Continue(())),
-			},
-		)?;
 		Ok(())
 	}
 }

polkadot/xcm/xcm-builder/src/tests/barriers.rs

@@ -964,8 +964,8 @@ fn deny_then_try_works() {
 		}
 	}
 
-	/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single `ClearError`,
-	/// else return `ProcessMessageError::Yield`
+	/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains a single
+	/// `ClearError`, else return `ProcessMessageError::Yield`
 	struct AllowSingleClearErrorOrYield;
 	impl ShouldExecute for AllowSingleClearErrorOrYield {
 		fn should_execute<Call>(
@@ -984,8 +984,8 @@ fn deny_then_try_works() {
 		}
 	}
 
-	/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains `ClearTopic` and
-	/// origin from `Here`, else return `ProcessMessageError::Unsupported`
+	/// A dummy `ShouldExecute` impl which returns `Ok(())` when XCM contains
+	/// `ClearTopic` and origin from `Here`, else return `ProcessMessageError::Unsupported`
 	struct AllowClearTopicFromHere;
 	impl ShouldExecute for AllowClearTopicFromHere {
 		fn should_execute<Call>(
@@ -1071,7 +1071,7 @@ fn deny_then_try_works() {
 }
 
 #[test]
-fn deny_reserve_transfer_to_relaychain_should_work() {
+fn deny_reserve_transfer_to_relaychain_should_be_noop() {
 	let assert_deny_execution = |mut xcm: Vec<Instruction<()>>, origin, expected_result| {
 		assert_eq!(
 			DenyReserveTransferToRelayChain::deny_execution(
@@ -1083,45 +1083,44 @@ fn deny_reserve_transfer_to_relaychain_should_work() {
 			expected_result
 		);
 	};
-	// deny DepositReserveAsset to RelayChain
+	// no longer denies DepositReserveAsset to RelayChain
 	assert_deny_execution(
 		vec![DepositReserveAsset {
 			assets: Wild(All),
-			dest: Location::parent(),
+			dest: Location { parents: 1, interior: Here },
 			xcm: vec![].into(),
 		}],
 		Here.into_location(),
-		Err(ProcessMessageError::Unsupported),
+		Ok(()), // Pass
 	);
-	// deny InitiateReserveWithdraw to RelayChain
 	assert_deny_execution(
-		vec![InitiateReserveWithdraw {
+		vec![DepositReserveAsset {
 			assets: Wild(All),
-			reserve: Location::parent(),
-			xcm: vec![].into(),
+			dest: Location { parents: 1, interior: Here },
+			xcm: Xcm(vec![ClearOrigin]),
 		}],
 		Here.into_location(),
-		Err(ProcessMessageError::Unsupported),
+		Ok(()), // Pass
 	);
-	// deny TransferReserveAsset to RelayChain
+	// no longer denies InitiateReserveWithdraw to RelayChain
 	assert_deny_execution(
-		vec![TransferReserveAsset {
-			assets: vec![].into(),
-			dest: Location::parent(),
+		vec![InitiateReserveWithdraw {
+			assets: Wild(All),
+			reserve: Location { parents: 1, interior: Here },
 			xcm: vec![].into(),
 		}],
 		Here.into_location(),
-		Err(ProcessMessageError::Unsupported),
+		Ok(()), // Pass
 	);
-	// accept DepositReserveAsset to destination other than RelayChain
+	// no longer denies TransferReserveAsset to RelayChain
 	assert_deny_execution(
-		vec![DepositReserveAsset {
-			assets: Wild(All),
-			dest: Here.into_location(),
+		vec![TransferReserveAsset {
+			assets: vec![].into(),
+			dest: Location { parents: 1, interior: Here },
 			xcm: vec![].into(),
 		}],
 		Here.into_location(),
-		Ok(()),
+		Ok(()), // Pass
 	);
 	// others instructions should pass
 	assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(()));
@@ -1189,12 +1188,8 @@ impl<Barrier: DenyExecution> ShouldExecute for Executable<Barrier> {
 
 #[test]
 fn deny_recursively_then_try_works() {
-	type Barrier = DenyThenTry<DenyRecursively<DenyReserveTransferToRelayChain>, AllowAll>;
-	let xcm = Xcm::<Instruction<()>>(vec![DepositReserveAsset {
-		assets: Wild(All),
-		dest: Location::parent(),
-		xcm: vec![].into(),
-	}]);
+	type Barrier = DenyThenTry<DenyRecursively<DenyClearOrigin>, AllowAll>;
+	let xcm = Xcm::<Instruction<()>>(vec![ClearOrigin]);
 	let origin = Here.into_location();
 	let max_weight = Weight::from_parts(10, 10);
 	let mut properties = props(Weight::zero());
@@ -1211,7 +1206,7 @@ fn deny_recursively_then_try_works() {
 	assert!(result.is_err());
 
 	// Should allow with `SetAppendix` for the original `DenyThenTry`
-	type OriginalBarrier = DenyThenTry<DenyReserveTransferToRelayChain, AllowAll>;
+	type OriginalBarrier = DenyThenTry<DenyClearOrigin, AllowAll>;
 	let result =
 		OriginalBarrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties);
 	assert!(result.is_ok());
@@ -1238,10 +1233,9 @@ fn deny_recursively_then_try_works() {
 	assert!(result.is_err());
 
 	// Should allow for valid XCM with `SetAppendix`
-	let xcm = Xcm::<Instruction<()>>(vec![DepositReserveAsset {
-		assets: Wild(All),
-		dest: Here.into_location(),
-		xcm: vec![].into(),
+	let xcm = Xcm::<Instruction<()>>(vec![BuyExecution {
+		fees: (Parent, 100).into(),
+		weight_limit: Unlimited,
 	}]);
 	let mut message = Xcm::<Instruction<()>>(vec![SetAppendix(xcm.clone())]);
 	let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties);
@@ -1269,7 +1263,7 @@ fn deny_recursively_works() {
 
 #[test]
 fn compare_deny_filters() {
-	type Denies = (DenyNothing, DenyReserveTransferToRelayChain);
+	type Denies = (DenyNothing, DenyClearOrigin);
 
 	fn assert_barrier<Barrier: ShouldExecute>(
 		top_level_result: Result<(), ProcessMessageError>,
@@ -1280,14 +1274,7 @@ fn compare_deny_filters() {
 		let mut properties = props(Weight::zero());
 
 		// Validate Top-Level
-		let xcm = Xcm::<Instruction<()>>(
-			vec![DepositReserveAsset {
-				assets: Wild(All),
-				dest: Location::parent(),
-				xcm: Xcm(vec![ClearOrigin]),
-			}]
-			.into(),
-		);
+		let xcm = Xcm::<Instruction<()>>(vec![ClearOrigin]);
 		let result =
 			Barrier::should_execute(&origin, xcm.clone().inner_mut(), max_weight, &mut properties);
 		assert_eq!(top_level_result, result);

polkadot/xcm/xcm-builder/src/tests/mock.rs

@@ -45,14 +45,33 @@ pub use xcm_executor::{
 	traits::{
 		AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm,
 		FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler,
-		QueryResponseStatus, TransactAsset,
+		QueryResponseStatus, TransactAsset, WeightBounds,
 	},
 	AssetsInHolding, Config,
 };
 pub use xcm_simulator::helpers::derive_topic_id;
 
 pub use xcm_executor::test_helpers::{mock_asset_to_holding as asset_to_holding, MockCredit};
 
+pub struct TestWeigher;
+impl WeightBounds<TestCall> for TestWeigher {
+	fn weight(
+		message: &mut Xcm<TestCall>,
+		weight_limit: Weight,
+	) -> Result<Weight, InstructionError> {
+		FixedWeightBounds::<UnitWeightCost, TestCall, MaxInstructions>::weight(
+			message,
+			weight_limit,
+		)
+	}
+	fn instr_weight(instruction: &mut Instruction<TestCall>) -> Result<Weight, XcmError> {
+		FixedWeightBounds::<UnitWeightCost, TestCall, MaxInstructions>::instr_weight(instruction)
+	}
+	fn barrier_check_weight() -> Option<Weight> {
+		Some(Weight::zero())
+	}
+}
+
 /// Helper to convert multiple Assets into AssetsInHolding for tests
 pub fn assets_to_holding(assets: impl IntoIterator<Item = Asset>) -> AssetsInHolding {
 	let mut holding = AssetsInHolding::new();
@@ -603,6 +622,7 @@ parameter_types! {
 		= (ByGenesis([0; 32]), Parachain(42)).into();
 	pub UnitWeightCost: Weight = Weight::from_parts(10, 10);
 }
+
 parameter_types! {
 	// Nothing is allowed to be paid/unpaid by default.
 	pub static AllowExplicitUnpaidFrom: Vec<Location> = vec![];
@@ -1016,7 +1036,7 @@ impl Config for TestConfig {
 	type IsTeleporter = TestIsTeleporter;
 	type UniversalLocation = ExecutorUniversalLocation;
 	type Barrier = TrailingSetTopicAsId<RespectSuspension<TestBarrier, TestSuspender>>;
-	type Weigher = FixedWeightBounds<UnitWeightCost, TestCall, MaxInstructions>;
+	type Weigher = TestWeigher;
 	type Trader = FixedRateOfFungible<WeightPrice, ()>;
 	type ResponseHandler = TestResponseHandler;
 	type AssetTrap = TestAssetTrap;

polkadot/xcm/xcm-builder/src/tests/origins.rs

@@ -151,7 +151,7 @@ fn unpaid_execution_should_work() {
 	assert_eq!(
 		r,
 		Outcome::Incomplete {
-			used: Weight::from_parts(10, 10),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);

polkadot/xcm/xcm-builder/src/tests/querying.rs

@@ -137,7 +137,7 @@ fn prepaid_result_of_query_should_get_free_execution() {
 	assert_eq!(
 		r,
 		Outcome::Incomplete {
-			used: Weight::from_parts(10, 10),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);

polkadot/xcm/xcm-builder/src/tests/version_subscriptions.rs

@@ -38,7 +38,7 @@ fn simple_version_subscriptions_should_work() {
 			Weight::zero(),
 		),
 		Outcome::Incomplete {
-			used: Weight::from_parts(20, 20),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);
@@ -54,7 +54,7 @@ fn simple_version_subscriptions_should_work() {
 			Weight::zero(),
 		),
 		Outcome::Incomplete {
-			used: Weight::from_parts(20, 20),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);
@@ -75,7 +75,7 @@ fn simple_version_subscriptions_should_work() {
 	assert_eq!(
 		r,
 		Outcome::Incomplete {
-			used: Weight::from_parts(10, 10),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);
@@ -157,7 +157,7 @@ fn simple_version_unsubscriptions_should_work() {
 	assert_eq!(
 		r,
 		Outcome::Incomplete {
-			used: Weight::from_parts(20, 20),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);
@@ -176,7 +176,7 @@ fn simple_version_unsubscriptions_should_work() {
 	assert_eq!(
 		r,
 		Outcome::Incomplete {
-			used: Weight::from_parts(10, 10),
+			used: Weight::zero(),
 			error: InstructionError { index: 0, error: XcmError::Barrier },
 		}
 	);

polkadot/xcm/xcm-builder/tests/mock/mod.rs

@@ -49,6 +49,10 @@ thread_local! {
 pub fn sent_xcm() -> Vec<(Location, opaque::Xcm, XcmHash)> {
 	SENT_XCM.with(|q| (*q.borrow()).clone())
 }
+#[allow(dead_code)]
+pub fn clear_sent_xcm() {
+	SENT_XCM.with(|q| q.borrow_mut().clear());
+}
 pub struct TestSendXcm;
 impl SendXcm for TestSendXcm {
 	type Ticket = (Location, Xcm<()>, XcmHash);