xcm: fix for DenyThenTry Barrier

cumulus/parachains/runtimes/bridge-hubs/common/Cargo.toml

@@ -19,6 +19,8 @@ sp-core = { workspace = true }
 sp-runtime = { workspace = true }
 sp-std = { workspace = true }
 xcm = { workspace = true }
+xcm-builder = { workspace = true }
+xcm-executor = { workspace = true }
 
 [features]
 default = ["std"]
@@ -32,6 +34,8 @@ std = [
 	"sp-core/std",
 	"sp-runtime/std",
 	"sp-std/std",
+	"xcm-builder/std",
+	"xcm-executor/std",
 	"xcm/std",
 ]
 
@@ -41,5 +45,7 @@ runtime-benchmarks = [
 	"pallet-message-queue/runtime-benchmarks",
 	"snowbridge-core/runtime-benchmarks",
 	"sp-runtime/runtime-benchmarks",
+	"xcm-builder/runtime-benchmarks",
+	"xcm-executor/runtime-benchmarks",
 	"xcm/runtime-benchmarks",
 ]

cumulus/parachains/runtimes/bridge-hubs/common/src/barriers.rs

@@ -0,0 +1,59 @@
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use core::{marker::PhantomData, ops::ControlFlow};
+use cumulus_primitives_core::Weight;
+use frame_support::traits::{Contains, ProcessMessageError};
+use xcm::prelude::{ExportMessage, Instruction, Location, NetworkId};
+
+use xcm_builder::{CreateMatcher, MatchXcm};
+use xcm_executor::traits::{DenyExecution, Properties};
+
+/// Deny execution if the message contains instruction `ExportMessage` with
+/// a. origin is contained in `FromOrigin` (i.e.`FromOrigin::Contains(origin)`)
+/// b. network is contained in `ToGlobalConsensus`, (i.e. `ToGlobalConsensus::contains(network)`)
+pub struct DenyExportMessageFrom<FromOrigin, ToGlobalConsensus>(
+	PhantomData<(FromOrigin, ToGlobalConsensus)>,
+);
+
+impl<FromOrigin, ToGlobalConsensus> DenyExecution
+	for DenyExportMessageFrom<FromOrigin, ToGlobalConsensus>
+where
+	FromOrigin: Contains<Location>,
+	ToGlobalConsensus: Contains<NetworkId>,
+{
+	fn deny_execution<RuntimeCall>(
+		origin: &Location,
+		message: &mut [Instruction<RuntimeCall>],
+		_max_weight: Weight,
+		_properties: &mut Properties,
+	) -> Option<ProcessMessageError> {
+		match message.matcher().match_next_inst_while(
+			|_| true,
+			|inst| match inst {
+				ExportMessage { network, .. } =>
+					if ToGlobalConsensus::contains(network) && FromOrigin::contains(origin) {
+						return Err(ProcessMessageError::Unsupported)
+					} else {
+						Ok(ControlFlow::Continue(()))
+					},
+				_ => Ok(ControlFlow::Continue(())),
+			},
+		) {
+			Ok(_) => None,
+			Err(error) => Some(error),
+		}
+	}
+}

cumulus/parachains/runtimes/bridge-hubs/common/src/lib.rs

@@ -14,9 +14,11 @@
 // limitations under the License.
 #![cfg_attr(not(feature = "std"), no_std)]
 
+pub mod barriers;
 pub mod digest_item;
 pub mod message_queue;
 pub mod xcm_version;
 
+pub use barriers::DenyExportMessageFrom;
 pub use digest_item::CustomDigestItem;
 pub use message_queue::{AggregateMessageOrigin, BridgeHubMessageRouter};

cumulus/parachains/runtimes/bridge-hubs/common/tests/tests.rs

@@ -0,0 +1,140 @@
+// Copyright (C) Parity Technologies (UK) Ltd.
+// This file is part of Cumulus.
+
+// Cumulus is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Cumulus is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Cumulus.  If not, see <http://www.gnu.org/licenses/>.
+
+#![cfg(test)]
+use bridge_hub_common::DenyExportMessageFrom;
+use frame_support::{
+	assert_err, assert_ok, parameter_types,
+	traits::{Equals, Everything, EverythingBut, ProcessMessageError},
+};
+use xcm::prelude::{
+	AliasOrigin, All, AssetFilter, DepositReserveAsset, Ethereum, ExportMessage, Here, Instruction,
+	Location, NetworkId, NetworkId::Polkadot, Parachain, Weight, Wild,
+};
+use xcm_builder::{DenyReserveTransferToRelayChain, DenyThenTry, TakeWeightCredit};
+use xcm_executor::traits::{Properties, ShouldExecute};
+
+parameter_types! {
+	pub AssetHubLocation: Location = Location::new(1, Parachain(1000));
+	pub ParachainLocation: Location = Location::new(1, Parachain(2000));
+	pub EthereumNetwork: NetworkId = Ethereum { chain_id: 1 };
+}
+
+#[test]
+fn deny_export_message_from_source_other_than_asset_hub_should_work() {
+	pub type Barrier = DenyThenTry<
+		(
+			DenyReserveTransferToRelayChain,
+			DenyExportMessageFrom<EverythingBut<Equals<AssetHubLocation>>, Equals<EthereumNetwork>>,
+		),
+		TakeWeightCredit,
+	>;
+
+	let mut xcm: Vec<Instruction<()>> = vec![
+		AliasOrigin(Here.into()),
+		ExportMessage {
+			network: EthereumNetwork::get(),
+			destination: Here,
+			xcm: Default::default(),
+		},
+	];
+
+	let result = Barrier::should_execute(
+		&ParachainLocation::get(),
+		&mut xcm,
+		Weight::zero(),
+		&mut Properties { weight_credit: Weight::zero(), message_id: None },
+	);
+
+	assert_err!(result, ProcessMessageError::Unsupported);
+}
+
+#[test]
+fn allow_export_message_from_asset_hub_should_work() {
+	pub type Barrier = DenyThenTry<
+		(
+			DenyReserveTransferToRelayChain,
+			DenyExportMessageFrom<EverythingBut<Equals<AssetHubLocation>>, Equals<EthereumNetwork>>,
+		),
+		TakeWeightCredit,
+	>;
+
+	let mut xcm: Vec<Instruction<()>> = vec![
+		AliasOrigin(Here.into()),
+		ExportMessage {
+			network: EthereumNetwork::get(),
+			destination: Here,
+			xcm: Default::default(),
+		},
+	];
+
+	let result = Barrier::should_execute(
+		&AssetHubLocation::get(),
+		&mut xcm,
+		Weight::zero(),
+		&mut Properties { weight_credit: Weight::zero(), message_id: None },
+	);
+
+	assert_ok!(result);
+}
+
+#[test]
+fn allow_export_message_from_source_other_than_asset_hub_if_destination_other_than_ethereum() {
+	pub type Barrier = DenyThenTry<
+		(
+			DenyReserveTransferToRelayChain,
+			DenyExportMessageFrom<EverythingBut<Equals<AssetHubLocation>>, Equals<EthereumNetwork>>,
+		),
+		TakeWeightCredit,
+	>;
+
+	let mut xcm: Vec<Instruction<()>> = vec![
+		AliasOrigin(Here.into()),
+		ExportMessage { network: Polkadot, destination: Here, xcm: Default::default() },
+	];
+
+	let result = Barrier::should_execute(
+		&ParachainLocation::get(),
+		&mut xcm,
+		Weight::zero(),
+		&mut Properties { weight_credit: Weight::zero(), message_id: None },
+	);
+
+	assert_ok!(result);
+}
+
+#[test]
+fn deny_reserver_transfer_to_relaychain_does_not_break() {
+	pub type Barrier = DenyThenTry<
+		(DenyReserveTransferToRelayChain, DenyExportMessageFrom<Everything, Everything>),
+		TakeWeightCredit,
+	>;
+
+	let mut xcm: Vec<Instruction<()>> = vec![DepositReserveAsset {
+		assets: AssetFilter::try_from(Wild(All)).unwrap(),
+		dest: Location { parents: 1, interior: Here },
+		xcm: Default::default(),
+	}];
+
+	let result = Barrier::should_execute(
+		&Here.into(),
+		&mut xcm,
+		Weight::zero(),
+		&mut Properties { weight_credit: Weight::zero(), message_id: None },
+	);
+
+	assert_err!(result, ProcessMessageError::Unsupported);
+}

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

@@ -24,7 +24,7 @@ use frame_support::{
 };
 use polkadot_parachain_primitives::primitives::IsSystem;
 use xcm::prelude::*;
-use xcm_executor::traits::{CheckSuspension, OnResponse, Properties, ShouldExecute};
+use xcm_executor::traits::{CheckSuspension, DenyExecution, OnResponse, Properties, ShouldExecute};
 
 /// Execution barrier that just takes `max_weight` from `properties.weight_credit`.
 ///
@@ -444,12 +444,12 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain {
 /// 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>)
 where
-	Deny: ShouldExecute,
+	Deny: DenyExecution,
 	Allow: ShouldExecute;
 
 impl<Deny, Allow> ShouldExecute for DenyThenTry<Deny, Allow>
 where
-	Deny: ShouldExecute,
+	Deny: DenyExecution,
 	Allow: ShouldExecute,
 {
 	fn should_execute<RuntimeCall>(
@@ -458,21 +458,23 @@ where
 		max_weight: Weight,
 		properties: &mut Properties,
 	) -> Result<(), ProcessMessageError> {
-		Deny::should_execute(origin, message, max_weight, properties)?;
-		Allow::should_execute(origin, message, max_weight, properties)
+		match Deny::deny_execution(origin, message, max_weight, properties) {
+			Some(err) => Err(err),
+			None => Allow::should_execute(origin, message, max_weight, properties),
+		}
 	}
 }
 
 // See issue <https://github.com/paritytech/polkadot/issues/5233>
 pub struct DenyReserveTransferToRelayChain;
-impl ShouldExecute for DenyReserveTransferToRelayChain {
-	fn should_execute<RuntimeCall>(
+impl DenyExecution for DenyReserveTransferToRelayChain {
+	fn deny_execution<RuntimeCall>(
 		origin: &Location,
 		message: &mut [Instruction<RuntimeCall>],
 		_max_weight: Weight,
 		_properties: &mut Properties,
-	) -> Result<(), ProcessMessageError> {
-		message.matcher().match_next_inst_while(
+	) -> Option<ProcessMessageError> {
+		match message.matcher().match_next_inst_while(
 			|_| true,
 			|inst| match inst {
 				InitiateReserveWithdraw {
@@ -498,9 +500,9 @@ impl ShouldExecute for DenyReserveTransferToRelayChain {
 
 				_ => Ok(ControlFlow::Continue(())),
 			},
-		)?;
-
-		// Permit everything else
-		Ok(())
+		) {
+			Ok(_) => None,
+			Err(err) => Some(err),
+		}
 	}
 }

polkadot/xcm/xcm-executor/src/traits/mod.rs

@@ -42,7 +42,7 @@ pub use on_response::{OnResponse, QueryHandler, QueryResponseStatus, VersionChan
 mod process_transaction;
 pub use process_transaction::ProcessTransaction;
 mod should_execute;
-pub use should_execute::{CheckSuspension, Properties, ShouldExecute};
+pub use should_execute::{CheckSuspension, DenyExecution, Properties, ShouldExecute};
 mod transact_asset;
 pub use transact_asset::TransactAsset;
 mod hrmp;

polkadot/xcm/xcm-executor/src/traits/should_execute.rs

@@ -127,3 +127,66 @@ impl CheckSuspension for Tuple {
 		false
 	}
 }
+
+/// Trait to determine whether the execution engine should not execute a given XCM.
+///
+/// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns
+/// `Err(())`, the execution stops. Else, `Ok(_)` is returned if all elements accept the message.
+pub trait DenyExecution {
+	/// Returns `Ok(())` means there is no reason not to execute the message
+	/// while Err(e) indicates there is a reason not to execute.
+	///
+	/// - `origin`: The origin (sender) of the message.
+	/// - `instructions`: The message itself.
+	/// - `max_weight`: The (possibly over-) estimation of the weight of execution of the message.
+	/// - `properties`: Various pre-established properties of the message which may be mutated by
+	///   this API.
+	fn deny_execution<RuntimeCall>(
+		origin: &Location,
+		instructions: &mut [Instruction<RuntimeCall>],
+		max_weight: Weight,
+		properties: &mut Properties,
+	) -> Option<ProcessMessageError>;
+}
+
+#[impl_trait_for_tuples::impl_for_tuples(30)]
+impl DenyExecution for Tuple {
+	fn deny_execution<RuntimeCall>(
+		origin: &Location,
+		instructions: &mut [Instruction<RuntimeCall>],
+		max_weight: Weight,
+		properties: &mut Properties,
+	) -> Option<ProcessMessageError> {
+		for_tuples!( #(
+            let barrier = core::any::type_name::<Tuple>();
+            match Tuple::deny_execution(origin, instructions, max_weight, properties) {
+                Some(error) => {
+                    tracing::trace!(
+                        target: "xcm::should_execute",
+                        ?origin,
+                        ?instructions,
+                        ?max_weight,
+                        ?properties,
+                        ?error,
+                        %barrier,
+                        "did not pass barrier",
+                    );
+                    return Some(error);
+                },
+				 None => {
+                    tracing::trace!(
+                        target: "xcm::should_execute",
+                        ?origin,
+                        ?instructions,
+                        ?max_weight,
+                        ?properties,
+                        %barrier,
+                        "pass barrier",
+                    );
+                },
+            }
+        )* );
+
+		None
+	}
+}