xcm: fix for DenyThenTry Barrier (paritytech#7169)

Resolves (partially):
paritytech#7148 (see _Problem 1 -
`ShouldExecute` tuple implementation and `Deny` filter tuple_)

This PR changes the behavior of `DenyThenTry` from the pattern
`DenyIfAllMatch` to `DenyIfAnyMatch` for the tuple.

I would expect the latter is the right behavior so make the fix in
place, but we can also add a dedicated Impl with the legacy one
untouched.

## TODO
- [x] add unit-test for `DenyReserveTransferToRelayChain`
- [x] add test and investigate/check `DenyThenTry` as discussed
[here](paritytech#6838 (comment))
and update documentation if needed

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>

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,57 @@
+// 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,
+	) -> Result<(), ProcessMessageError> {
+		// This barrier only cares about messages with `origin` matching `FromOrigin`.
+		if !FromOrigin::contains(origin) {
+			return Ok(())
+		}
+		message.matcher().match_next_inst_while(
+			|_| true,
+			|inst| match inst {
+				ExportMessage { network, .. } if ToGlobalConsensus::contains(network) =>
+					Err(ProcessMessageError::Unsupported),
+				_ => Ok(ControlFlow::Continue(())),
+			},
+		)?;
+		Ok(())
+	}
+}

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,85 @@
+// 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::{
+	parameter_types,
+	traits::{Equals, EverythingBut, ProcessMessageError::Unsupported},
+};
+use xcm::prelude::{
+	AliasOrigin, ByGenesis, ExportMessage, Here, Instruction, Location, NetworkId, Parachain,
+	Weight,
+};
+use xcm_executor::traits::{DenyExecution, Properties};
+
+#[test]
+fn test_deny_export_message_from() {
+	parameter_types! {
+		pub Source1: Location = Location::new(1, Parachain(1));
+		pub Source2: Location = Location::new(1, Parachain(2));
+		pub Remote1: NetworkId = ByGenesis([1;32]);
+		pub Remote2: NetworkId = ByGenesis([2;32]);
+	}
+
+	// Deny ExportMessage when both of the conditions met:
+	// 1: source != Source1
+	// 2: network == Remote1
+	pub type Denied = DenyExportMessageFrom<EverythingBut<Equals<Source1>>, Equals<Remote1>>;
+
+	let assert_deny_execution = |mut xcm: Vec<Instruction<()>>, origin, expected_result| {
+		assert_eq!(
+			Denied::deny_execution(
+				&origin,
+				&mut xcm,
+				Weight::zero(),
+				&mut Properties { weight_credit: Weight::zero(), message_id: None }
+			),
+			expected_result
+		);
+	};
+
+	// A message without an `ExportMessage` should pass
+	assert_deny_execution(vec![AliasOrigin(Here.into())], Source1::get(), Ok(()));
+
+	// `ExportMessage` from source1 and remote1 should pass
+	assert_deny_execution(
+		vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }],
+		Source1::get(),
+		Ok(()),
+	);
+
+	// `ExportMessage` from source1 and remote2 should pass
+	assert_deny_execution(
+		vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }],
+		Source1::get(),
+		Ok(()),
+	);
+
+	// `ExportMessage` from source2 and remote2 should pass
+	assert_deny_execution(
+		vec![ExportMessage { network: Remote2::get(), destination: Here, xcm: Default::default() }],
+		Source2::get(),
+		Ok(()),
+	);
+
+	// `ExportMessage` from source2 and remote1 should be banned
+	assert_deny_execution(
+		vec![ExportMessage { network: Remote1::get(), destination: Here, xcm: Default::default() }],
+		Source2::get(),
+		Err(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,15 +458,15 @@ where
 		max_weight: Weight,
 		properties: &mut Properties,
 	) -> Result<(), ProcessMessageError> {
-		Deny::should_execute(origin, message, max_weight, properties)?;
+		Deny::deny_execution(origin, message, max_weight, properties)?;
 		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,
@@ -499,8 +499,6 @@ impl ShouldExecute for DenyReserveTransferToRelayChain {
 				_ => Ok(ControlFlow::Continue(())),
 			},
 		)?;
-
-		// Permit everything else
 		Ok(())
 	}
 }