From 9c80770f0aa89004ebe52cd3c1b824228f033a2e Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 15 Jan 2025 15:36:14 +0100 Subject: [PATCH 01/48] PoC: DenyInstructionsWithXcm for validating nested XCM instructions --- Cargo.lock | 1 + polkadot/xcm/xcm-builder/Cargo.toml | 5 +- polkadot/xcm/xcm-builder/src/barriers.rs | 70 +++++++++++- polkadot/xcm/xcm-builder/src/lib.rs | 4 +- .../xcm/xcm-builder/src/tests/barriers.rs | 101 ++++++++++++++++++ polkadot/xcm/xcm-executor/src/lib.rs | 2 +- 6 files changed, 178 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 55cc1721bdde..35a8c3a6df0e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28387,6 +28387,7 @@ name = "staging-xcm-builder" version = "7.0.0" dependencies = [ "assert_matches", + "environmental", "frame-support 28.0.0", "frame-system 28.0.0", "impl-trait-for-tuples", diff --git a/polkadot/xcm/xcm-builder/Cargo.toml b/polkadot/xcm/xcm-builder/Cargo.toml index f75c984c068e..bd588c97207a 100644 --- a/polkadot/xcm/xcm-builder/Cargo.toml +++ b/polkadot/xcm/xcm-builder/Cargo.toml @@ -13,6 +13,7 @@ workspace = true [dependencies] codec = { features = ["derive"], workspace = true } +environmental = { workspace = true } frame-support = { workspace = true } frame-system = { workspace = true } impl-trait-for-tuples = { workspace = true } @@ -21,6 +22,7 @@ pallet-asset-conversion = { workspace = true } pallet-transaction-payment = { workspace = true } scale-info = { features = ["derive"], workspace = true } sp-arithmetic = { workspace = true } +sp-core = { workspace = true } sp-io = { workspace = true } sp-runtime = { workspace = true } sp-weights = { workspace = true } @@ -40,7 +42,6 @@ polkadot-primitives = { workspace = true, default-features = true } polkadot-runtime-parachains = { workspace = true, default-features = true } polkadot-test-runtime = { workspace = true } primitive-types = { features = ["codec", "num-traits", "scale-info"], workspace = true } -sp-core = { workspace = true, default-features = true } [features] default = ["std"] @@ -63,6 +64,7 @@ runtime-benchmarks = [ ] std = [ "codec/std", + "environmental/std", "frame-support/std", "frame-system/std", "log/std", @@ -72,6 +74,7 @@ std = [ "primitive-types/std", "scale-info/std", "sp-arithmetic/std", + "sp-core/std", "sp-io/std", "sp-runtime/std", "sp-weights/std", diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index adba9a3ef79f..62e8dfd4ac64 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -490,7 +490,7 @@ impl ShouldExecute for DenyReserveTransferToRelayChain { if matches!(origin, Location { parents: 1, interior: Here }) => { log::warn!( - target: "xcm::barrier", + target: "xcm::barriers", "Unexpected ReserveAssetDeposited from the Relay Chain", ); Ok(ControlFlow::Continue(())) @@ -504,3 +504,71 @@ impl ShouldExecute for DenyReserveTransferToRelayChain { Ok(()) } } + +environmental::environmental!(recursion_count: u8); + +// TBD: +/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instructions. +/// +/// Note: The nested XCM is checked recursively! +pub struct DenyInstructionsWithXcm(PhantomData); +impl ShouldExecute for DenyInstructionsWithXcm { + fn should_execute( + origin: &Location, + message: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + SetAppendix(nested_xcm) | + SetErrorHandler(nested_xcm) | + ExecuteWithOrigin { xcm: nested_xcm, .. } => { + // check xcm instructions with `Inner` filter + let _ = Inner::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) + .inspect_err(|e| { + log::warn!( + target: "xcm::barriers", + "`DenyInstructionsWithXcm`'s `Inner::should_execute` did not pass for origin: {:?} and nested_xcm: {:?} with error: {:?}", + origin, + nested_xcm, + e + ); + })?; + + // Initialize the recursion count only the first time we hit this code in our + // potential recursive execution. + let _ = recursion_count::using_once(&mut 1, || { + recursion_count::with(|count| { + if *count > xcm_executor::RECURSION_LIMIT { + return Err(ProcessMessageError::StackLimitReached) + } + *count = count.saturating_add(1); + Ok(()) + }) + // This should always return `Some`, but let's play it safe. + .unwrap_or(Ok(()))?; + + // Ensure that we always decrement the counter whenever we finish processing + // the `DenyInstructionsWithXcm`. + sp_core::defer! { + recursion_count::with(|count| { + *count = count.saturating_sub(1); + }); + } + + // check recursively with `DenyInstructionsWithXcm` + Self::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) + })?; + + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + + // Permit everything else + Ok(()) + } +} diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index e23412a97ebc..ef9b0900cf93 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,8 +42,8 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, - IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, + AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyInstructionsWithXcm, DenyThenTry, + IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index d8805274d3a5..c65a7bd8ccd1 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,6 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +use core::ops::ControlFlow; use xcm_executor::traits::Properties; use super::*; @@ -532,3 +533,103 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { Ok(()), ); } + + +#[test] +fn deny_instructions_with_xcm_works() { + frame_support::__private::sp_tracing::try_init_simple(); + + // dummy filter which denies `ClearOrigin` + struct DenyClearOrigin; + impl ShouldExecute for DenyClearOrigin { + fn should_execute(_: &Location, message: &mut [Instruction], _: Weight, _: &mut Properties) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + // closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin` instruction + let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyInstructionsWithXcm::::should_execute( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + + // ok + assert_should_execute( + vec![ClearTransactStatus], + Location::parent(), + Ok(()), + ); + // ok top-level contains `ClearOrigin` + assert_should_execute( + vec![ClearOrigin], + Location::parent(), + Ok(()), + ); + // ok - SetAppendix with XCM without ClearOrigin + assert_should_execute( + vec![SetAppendix(Xcm(vec![ClearTransactStatus]))], + Location::parent(), + Ok(()), + ); + + // invalid - empty XCM + assert_should_execute( + vec![], + Location::parent(), + Err(ProcessMessageError::BadFormat), + ); + // invalid - SetAppendix with empty XCM + assert_should_execute( + vec![SetAppendix(Xcm(vec![]))], + Location::parent(), + Err(ProcessMessageError::BadFormat), + ); + // invalid SetAppendix contains `ClearOrigin` + assert_should_execute( + vec![SetAppendix(Xcm(vec![ClearOrigin]))], + Location::parent(), + Err(ProcessMessageError::Unsupported), + ); + // invalid nested SetAppendix contains `ClearOrigin` + assert_should_execute( + vec![SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ClearOrigin])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ]))], + Location::parent(), + Err(ProcessMessageError::StackLimitReached), + ); +} diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index d0f18aea1ab3..c1059a8b445f 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -60,7 +60,7 @@ pub struct FeesMode { pub jit_withdraw: bool, } -const RECURSION_LIMIT: u8 = 10; +pub const RECURSION_LIMIT: u8 = 10; environmental::environmental!(recursion_count: u8); From 6c79a06250c4d3c9840a7dc388b12cef33bb5d74 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 15 Jan 2025 15:36:14 +0100 Subject: [PATCH 02/48] PoC: DenyInstructionsWithXcm for validating nested XCM instructions --- Cargo.lock | 2 + polkadot/xcm/xcm-builder/Cargo.toml | 5 +- polkadot/xcm/xcm-builder/src/barriers.rs | 68 +++++++++++++++ polkadot/xcm/xcm-builder/src/lib.rs | 4 +- .../xcm/xcm-builder/src/tests/barriers.rs | 84 ++++++++++++++++++- polkadot/xcm/xcm-executor/src/lib.rs | 2 +- 6 files changed, 158 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d04808c0f089..ed17a441e713 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28308,6 +28308,8 @@ dependencies = [ name = "staging-xcm-builder" version = "7.0.0" dependencies = [ + "assert_matches", + "environmental", "frame-support 28.0.0", "frame-system 28.0.0", "impl-trait-for-tuples", diff --git a/polkadot/xcm/xcm-builder/Cargo.toml b/polkadot/xcm/xcm-builder/Cargo.toml index 5169f586d723..764c788f2d0f 100644 --- a/polkadot/xcm/xcm-builder/Cargo.toml +++ b/polkadot/xcm/xcm-builder/Cargo.toml @@ -13,6 +13,7 @@ workspace = true [dependencies] codec = { features = ["derive"], workspace = true } +environmental = { workspace = true } frame-support = { workspace = true } frame-system = { workspace = true } impl-trait-for-tuples = { workspace = true } @@ -21,6 +22,7 @@ pallet-asset-conversion = { workspace = true } pallet-transaction-payment = { workspace = true } scale-info = { features = ["derive"], workspace = true } sp-arithmetic = { workspace = true } +sp-core = { workspace = true } sp-io = { workspace = true } sp-runtime = { workspace = true } sp-weights = { workspace = true } @@ -39,7 +41,6 @@ polkadot-primitives = { workspace = true, default-features = true } polkadot-runtime-parachains = { workspace = true, default-features = true } polkadot-test-runtime = { workspace = true } primitive-types = { features = ["codec", "num-traits", "scale-info"], workspace = true } -sp-core = { workspace = true, default-features = true } [features] default = ["std"] @@ -62,6 +63,7 @@ runtime-benchmarks = [ ] std = [ "codec/std", + "environmental/std", "frame-support/std", "frame-system/std", "log/std", @@ -71,6 +73,7 @@ std = [ "primitive-types/std", "scale-info/std", "sp-arithmetic/std", + "sp-core/std", "sp-io/std", "sp-runtime/std", "sp-weights/std", diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 9f9d3c232122..c3b6efe6378d 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -502,3 +502,71 @@ impl DenyExecution for DenyReserveTransferToRelayChain { Ok(()) } } + +environmental::environmental!(recursion_count: u8); + +// TBD: +/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instructions. +/// +/// Note: The nested XCM is checked recursively! +pub struct DenyInstructionsWithXcm(PhantomData); +impl ShouldExecute for DenyInstructionsWithXcm { + fn should_execute( + origin: &Location, + message: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + SetAppendix(nested_xcm) | + SetErrorHandler(nested_xcm) | + ExecuteWithOrigin { xcm: nested_xcm, .. } => { + // check xcm instructions with `Inner` filter + let _ = Inner::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) + .inspect_err(|e| { + log::warn!( + target: "xcm::barriers", + "`DenyInstructionsWithXcm`'s `Inner::should_execute` did not pass for origin: {:?} and nested_xcm: {:?} with error: {:?}", + origin, + nested_xcm, + e + ); + })?; + + // Initialize the recursion count only the first time we hit this code in our + // potential recursive execution. + let _ = recursion_count::using_once(&mut 1, || { + recursion_count::with(|count| { + if *count > xcm_executor::RECURSION_LIMIT { + return Err(ProcessMessageError::StackLimitReached) + } + *count = count.saturating_add(1); + Ok(()) + }) + // This should always return `Some`, but let's play it safe. + .unwrap_or(Ok(()))?; + + // Ensure that we always decrement the counter whenever we finish processing + // the `DenyInstructionsWithXcm`. + sp_core::defer! { + recursion_count::with(|count| { + *count = count.saturating_sub(1); + }); + } + + // check recursively with `DenyInstructionsWithXcm` + Self::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) + })?; + + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + + // Permit everything else + Ok(()) + } +} diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index e23412a97ebc..ef9b0900cf93 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,8 +42,8 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, - IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, + AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyInstructionsWithXcm, DenyThenTry, + IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 2fb8e8ed0363..4c89eea32991 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,6 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +use core::ops::ControlFlow; use xcm_executor::traits::Properties; use super::*; @@ -569,12 +570,13 @@ fn deny_then_try_works() { instructions.matcher().match_next_inst_while( |_| true, |inst| match inst { - ClearOrigin { .. } => + ClearOrigin { .. } => { if origin.clone() == Here.into_location() { Err(ProcessMessageError::BadFormat) } else { Ok(ControlFlow::Continue(())) - }, + } + }, _ => Ok(ControlFlow::Continue(())), }, )?; @@ -593,7 +595,7 @@ fn deny_then_try_works() { _properties: &mut Properties, ) -> Result<(), ProcessMessageError> { if instructions.len() != 1 { - return Ok(()) + return Ok(()); } match instructions.get(0).unwrap() { UnsubscribeVersion { .. } => Err(ProcessMessageError::StackLimitReached), @@ -764,3 +766,79 @@ fn deny_reserve_transfer_to_relaychain_should_work() { // others instructions should pass assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); } + +#[test] +fn deny_instructions_with_xcm_works() { + frame_support::__private::sp_tracing::try_init_simple(); + + // dummy filter which denies `ClearOrigin` + struct DenyClearOrigin; + impl ShouldExecute for DenyClearOrigin { + fn should_execute( + _: &Location, + message: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + // closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin` instruction + let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyInstructionsWithXcm::::should_execute( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + + // ok + assert_should_execute(vec![ClearTransactStatus], Location::parent(), Ok(())); + // ok top-level contains `ClearOrigin` + assert_should_execute(vec![ClearOrigin], Location::parent(), Ok(())); + // ok - SetAppendix with XCM without ClearOrigin + assert_should_execute( + vec![SetAppendix(Xcm(vec![ClearTransactStatus]))], + Location::parent(), + Ok(()), + ); + + // invalid - empty XCM + assert_should_execute(vec![], Location::parent(), Err(ProcessMessageError::BadFormat)); + // invalid - SetAppendix with empty XCM + assert_should_execute( + vec![SetAppendix(Xcm(vec![]))], + Location::parent(), + Err(ProcessMessageError::BadFormat), + ); + // invalid SetAppendix contains `ClearOrigin` + assert_should_execute( + vec![SetAppendix(Xcm(vec![ClearOrigin]))], + Location::parent(), + Err(ProcessMessageError::Unsupported), + ); + // invalid nested SetAppendix contains `ClearOrigin` + assert_should_execute( + vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix( + Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ClearOrigin]))])), + ]))]))])), + ]))]))]))]), + )]))]))]))], + Location::parent(), + Err(ProcessMessageError::StackLimitReached), + ); +} diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index d0f18aea1ab3..c1059a8b445f 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -60,7 +60,7 @@ pub struct FeesMode { pub jit_withdraw: bool, } -const RECURSION_LIMIT: u8 = 10; +pub const RECURSION_LIMIT: u8 = 10; environmental::environmental!(recursion_count: u8); From 72bec5b3ce4c522d308ba9ae9d52852b4f257cf3 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Tue, 28 Jan 2025 09:48:28 +0000 Subject: [PATCH 03/48] Fix merge conflicts --- Cargo.lock | 1 - polkadot/xcm/xcm-builder/src/barriers.rs | 103 +++++------------------ 2 files changed, 19 insertions(+), 85 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed17a441e713..336c9411b8ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -28308,7 +28308,6 @@ dependencies = [ name = "staging-xcm-builder" version = "7.0.0" dependencies = [ - "assert_matches", "environmental", "frame-support 28.0.0", "frame-system 28.0.0", diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index d7edf84d1cf2..0afa31c983ce 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -82,21 +82,22 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFrom instructions[..end] .matcher() .match_next_inst(|inst| match inst { - WithdrawAsset(ref assets) | - ReceiveTeleportedAsset(ref assets) | - ReserveAssetDeposited(ref assets) | - ClaimAsset { ref assets, .. } => + WithdrawAsset(ref assets) + | ReceiveTeleportedAsset(ref assets) + | ReserveAssetDeposited(ref assets) + | ClaimAsset { ref assets, .. } => { if assets.len() <= MAX_ASSETS_FOR_BUY_EXECUTION { Ok(()) } else { Err(ProcessMessageError::BadFormat) - }, + } + }, _ => Err(ProcessMessageError::BadFormat), })? .skip_inst_while(|inst| { - matches!(inst, ClearOrigin | AliasOrigin(..)) || - matches!(inst, DescendOrigin(child) if child != &Here) || - matches!(inst, SetHints { .. }) + matches!(inst, ClearOrigin | AliasOrigin(..)) + || matches!(inst, DescendOrigin(child) if child != &Here) + || matches!(inst, SetHints { .. }) })? .match_next_inst(|inst| match inst { BuyExecution { weight_limit: Limited(ref mut weight), .. } @@ -198,7 +199,7 @@ impl, MaxPref }, DescendOrigin(j) => { let Ok(_) = actual_origin.append_with(j.clone()) else { - return Err(ProcessMessageError::Unsupported) + return Err(ProcessMessageError::Unsupported); }; }, _ => return Ok(ControlFlow::Break(())), @@ -371,7 +372,9 @@ impl ShouldExecute for AllowKnownQueryResponses - Ok(()), + { + Ok(()) + }, _ => Err(ProcessMessageError::BadFormat), })?; Ok(()) @@ -431,9 +434,9 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain { .matcher() .assert_remaining_insts(1)? .match_next_inst(|inst| match inst { - HrmpNewChannelOpenRequest { .. } | - HrmpChannelAccepted { .. } | - HrmpChannelClosing { .. } => Ok(()), + HrmpNewChannelOpenRequest { .. } + | HrmpChannelAccepted { .. } + | HrmpChannelClosing { .. } => Ok(()), _ => Err(ProcessMessageError::BadFormat), })?; Ok(()) @@ -478,9 +481,9 @@ impl DenyExecution for DenyReserveTransferToRelayChain { InitiateReserveWithdraw { reserve: Location { parents: 1, interior: Here }, .. - } | - DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } | - TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => { + } + | DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } + | TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => { Err(ProcessMessageError::Unsupported) // Deny }, @@ -570,71 +573,3 @@ impl ShouldExecute for DenyInstructionsWithXcm { Ok(()) } } - -environmental::environmental!(recursion_count: u8); - -// TBD: -/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instructions. -/// -/// Note: The nested XCM is checked recursively! -pub struct DenyInstructionsWithXcm(PhantomData); -impl ShouldExecute for DenyInstructionsWithXcm { - fn should_execute( - origin: &Location, - message: &mut [Instruction], - max_weight: Weight, - properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - SetAppendix(nested_xcm) | - SetErrorHandler(nested_xcm) | - ExecuteWithOrigin { xcm: nested_xcm, .. } => { - // check xcm instructions with `Inner` filter - let _ = Inner::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) - .inspect_err(|e| { - log::warn!( - target: "xcm::barriers", - "`DenyInstructionsWithXcm`'s `Inner::should_execute` did not pass for origin: {:?} and nested_xcm: {:?} with error: {:?}", - origin, - nested_xcm, - e - ); - })?; - - // Initialize the recursion count only the first time we hit this code in our - // potential recursive execution. - let _ = recursion_count::using_once(&mut 1, || { - recursion_count::with(|count| { - if *count > xcm_executor::RECURSION_LIMIT { - return Err(ProcessMessageError::StackLimitReached) - } - *count = count.saturating_add(1); - Ok(()) - }) - // This should always return `Some`, but let's play it safe. - .unwrap_or(Ok(()))?; - - // Ensure that we always decrement the counter whenever we finish processing - // the `DenyInstructionsWithXcm`. - sp_core::defer! { - recursion_count::with(|count| { - *count = count.saturating_sub(1); - }); - } - - // check recursively with `DenyInstructionsWithXcm` - Self::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) - })?; - - Ok(ControlFlow::Continue(())) - }, - _ => Ok(ControlFlow::Continue(())), - }, - )?; - - // Permit everything else - Ok(()) - } -} From 6f60579fd631042d3ec6d9bb6e8c12b4367162ae Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 29 Jan 2025 10:46:07 +0000 Subject: [PATCH 04/48] Update from raymondkfcheung running command 'fmt' --- polkadot/xcm/xcm-builder/src/barriers.rs | 36 +++++++++++------------- polkadot/xcm/xcm-builder/src/lib.rs | 6 ++-- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 0afa31c983ce..ea3fd4dfbe7b 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -82,22 +82,21 @@ impl> ShouldExecute for AllowTopLevelPaidExecutionFrom instructions[..end] .matcher() .match_next_inst(|inst| match inst { - WithdrawAsset(ref assets) - | ReceiveTeleportedAsset(ref assets) - | ReserveAssetDeposited(ref assets) - | ClaimAsset { ref assets, .. } => { + WithdrawAsset(ref assets) | + ReceiveTeleportedAsset(ref assets) | + ReserveAssetDeposited(ref assets) | + ClaimAsset { ref assets, .. } => if assets.len() <= MAX_ASSETS_FOR_BUY_EXECUTION { Ok(()) } else { Err(ProcessMessageError::BadFormat) - } - }, + }, _ => Err(ProcessMessageError::BadFormat), })? .skip_inst_while(|inst| { - matches!(inst, ClearOrigin | AliasOrigin(..)) - || matches!(inst, DescendOrigin(child) if child != &Here) - || matches!(inst, SetHints { .. }) + matches!(inst, ClearOrigin | AliasOrigin(..)) || + matches!(inst, DescendOrigin(child) if child != &Here) || + matches!(inst, SetHints { .. }) })? .match_next_inst(|inst| match inst { BuyExecution { weight_limit: Limited(ref mut weight), .. } @@ -372,9 +371,7 @@ impl ShouldExecute for AllowKnownQueryResponses - { - Ok(()) - }, + Ok(()), _ => Err(ProcessMessageError::BadFormat), })?; Ok(()) @@ -434,9 +431,9 @@ impl ShouldExecute for AllowHrmpNotificationsFromRelayChain { .matcher() .assert_remaining_insts(1)? .match_next_inst(|inst| match inst { - HrmpNewChannelOpenRequest { .. } - | HrmpChannelAccepted { .. } - | HrmpChannelClosing { .. } => Ok(()), + HrmpNewChannelOpenRequest { .. } | + HrmpChannelAccepted { .. } | + HrmpChannelClosing { .. } => Ok(()), _ => Err(ProcessMessageError::BadFormat), })?; Ok(()) @@ -481,9 +478,9 @@ impl DenyExecution for DenyReserveTransferToRelayChain { InitiateReserveWithdraw { reserve: Location { parents: 1, interior: Here }, .. - } - | DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } - | TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => { + } | + DepositReserveAsset { dest: Location { parents: 1, interior: Here }, .. } | + TransferReserveAsset { dest: Location { parents: 1, interior: Here }, .. } => { Err(ProcessMessageError::Unsupported) // Deny }, @@ -509,7 +506,8 @@ impl DenyExecution for DenyReserveTransferToRelayChain { environmental::environmental!(recursion_count: u8); // TBD: -/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instructions. +/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and +/// `ExecuteWithOrigin` instructions. /// /// Note: The nested XCM is checked recursively! pub struct DenyInstructionsWithXcm(PhantomData); diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index ef9b0900cf93..7c96ead236dd 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,9 +42,9 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyReserveTransferToRelayChain, DenyInstructionsWithXcm, DenyThenTry, - IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, - TrailingSetTopicAsId, WithComputedOrigin, + AllowUnpaidExecutionFrom, DenyInstructionsWithXcm, DenyReserveTransferToRelayChain, + DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, + RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; mod controller; From 06c7e65c5982fcc098ed07a4b9e85f1c6a98f0db Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:05:40 +0000 Subject: [PATCH 05/48] Fix merge conflicts --- polkadot/xcm/xcm-builder/src/barriers.rs | 2 +- .../xcm/xcm-builder/src/tests/barriers.rs | 332 +++++++++++++++++- 2 files changed, 332 insertions(+), 2 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index ea3fd4dfbe7b..6b7493728e24 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -198,7 +198,7 @@ impl, MaxPref }, DescendOrigin(j) => { let Ok(_) = actual_origin.append_with(j.clone()) else { - return Err(ProcessMessageError::Unsupported); + return Err(ProcessMessageError::Unsupported) }; }, _ => return Ok(ControlFlow::Break(())), diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 3afb496bcb61..19ffd5bc9878 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,7 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use core::ops::ControlFlow; use xcm_executor::traits::Properties; use super::*; @@ -533,3 +532,334 @@ fn allow_hrmp_notifications_from_relay_chain_should_work() { Ok(()), ); } + +#[test] +fn deny_then_try_works() { + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::Yield` when XCM contains + /// `ClearTransactStatus` + struct DenyClearTransactStatusAsYield; + impl DenyExecution for DenyClearTransactStatusAsYield { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTransactStatus { .. } => Err(ProcessMessageError::Yield), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::BadFormat` when XCM + /// contains `ClearOrigin` with origin location from `Here` + struct DenyClearOriginFromHereAsBadFormat; + impl DenyExecution for DenyClearOriginFromHereAsBadFormat { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin { .. } => + if origin.clone() == Here.into_location() { + Err(ProcessMessageError::BadFormat) + } else { + Ok(ControlFlow::Continue(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + /// A dummy `DenyExecution` impl which returns `ProcessMessageError::StackLimitReached` when XCM + /// contains a single `UnsubscribeVersion` + struct DenyUnsubscribeVersionAsStackLimitReached; + impl DenyExecution for DenyUnsubscribeVersionAsStackLimitReached { + fn deny_execution( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + if instructions.len() != 1 { + return Ok(()) + } + match instructions.get(0).unwrap() { + UnsubscribeVersion { .. } => Err(ProcessMessageError::StackLimitReached), + _ => Ok(()), + } + } + } + + /// 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( + _origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().assert_remaining_insts(1)?.match_next_inst( + |inst| match inst { + ClearError { .. } => Ok(()), + _ => Err(ProcessMessageError::Yield), + }, + )?; + Ok(()) + } + } + + /// 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( + origin: &Location, + instructions: &mut [Instruction], + _max_weight: Weight, + _properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + ensure!(origin.clone() == Here.into_location(), ProcessMessageError::Unsupported); + let mut found = false; + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearTopic { .. } => { + found = true; + Ok(ControlFlow::Break(())) + }, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + ensure!(found, ProcessMessageError::Unsupported); + Ok(()) + } + } + // closure for (xcm, origin) testing with `DenyThenTry` + let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + pub type Barrier = DenyThenTry< + ( + DenyClearTransactStatusAsYield, + DenyClearOriginFromHereAsBadFormat, + DenyUnsubscribeVersionAsStackLimitReached, + ), + (AllowSingleClearErrorOrYield, AllowClearTopicFromHere), + >; + assert_eq!( + Barrier::should_execute( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + + // Deny cases: + // trigger DenyClearTransactStatusAsYield + assert_should_execute( + vec![ClearTransactStatus], + Parachain(1).into_location(), + Err(ProcessMessageError::Yield), + ); + // DenyClearTransactStatusAsYield wins against AllowSingleClearErrorOrYield + assert_should_execute( + vec![ClearError, ClearTransactStatus], + Parachain(1).into_location(), + Err(ProcessMessageError::Yield), + ); + // trigger DenyClearOriginFromHereAsBadFormat + assert_should_execute( + vec![ClearOrigin], + Here.into_location(), + Err(ProcessMessageError::BadFormat), + ); + // trigger DenyUnsubscribeVersionAsStackLimitReached + assert_should_execute( + vec![UnsubscribeVersion], + Here.into_location(), + Err(ProcessMessageError::StackLimitReached), + ); + + // deny because none of the allow items match + assert_should_execute( + vec![ClearError, ClearTopic], + Parachain(1).into_location(), + Err(ProcessMessageError::Unsupported), + ); + + // ok + assert_should_execute(vec![ClearError], Parachain(1).into_location(), Ok(())); + assert_should_execute(vec![ClearTopic], Here.into(), Ok(())); + assert_should_execute(vec![ClearError, ClearTopic], Here.into_location(), Ok(())); +} + +#[test] +fn deny_reserve_transfer_to_relaychain_should_work() { + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyReserveTransferToRelayChain::deny_execution( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + // deny DepositReserveAsset to RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // deny InitiateReserveWithdraw to RelayChain + assert_deny_execution( + vec![InitiateReserveWithdraw { + assets: Wild(All), + reserve: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // deny TransferReserveAsset to RelayChain + assert_deny_execution( + vec![TransferReserveAsset { + assets: vec![].into(), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // accept DepositReserveAsset to destination other than RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Here.into_location(), + xcm: vec![].into(), + }], + Here.into_location(), + Ok(()), + ); + // others instructions should pass + assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); +} + +#[test] +fn deny_instructions_with_xcm_works() { + frame_support::__private::sp_tracing::try_init_simple(); + + // dummy filter which denies `ClearOrigin` + struct DenyClearOrigin; + impl ShouldExecute for DenyClearOrigin { + fn should_execute(_: &Location, message: &mut [Instruction], _: Weight, _: &mut Properties) -> Result<(), ProcessMessageError> { + message.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + // closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin` instruction + let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyInstructionsWithXcm::::should_execute( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + + // ok + assert_should_execute( + vec![ClearTransactStatus], + Location::parent(), + Ok(()), + ); + // ok top-level contains `ClearOrigin` + assert_should_execute( + vec![ClearOrigin], + Location::parent(), + Ok(()), + ); + // ok - SetAppendix with XCM without ClearOrigin + assert_should_execute( + vec![SetAppendix(Xcm(vec![ClearTransactStatus]))], + Location::parent(), + Ok(()), + ); + + // invalid - empty XCM + assert_should_execute( + vec![], + Location::parent(), + Err(ProcessMessageError::BadFormat), + ); + // invalid - SetAppendix with empty XCM + assert_should_execute( + vec![SetAppendix(Xcm(vec![]))], + Location::parent(), + Err(ProcessMessageError::BadFormat), + ); + // invalid SetAppendix contains `ClearOrigin` + assert_should_execute( + vec![SetAppendix(Xcm(vec![ClearOrigin]))], + Location::parent(), + Err(ProcessMessageError::Unsupported), + ); + // invalid nested SetAppendix contains `ClearOrigin` + assert_should_execute( + vec![SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![ClearOrigin])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ])) + ]))], + Location::parent(), + Err(ProcessMessageError::StackLimitReached), + ); +} From 2d3d33d98ad40440a8f0c07090dd0cae3f126f21 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 29 Jan 2025 14:42:32 +0000 Subject: [PATCH 06/48] Update from raymondkfcheung running command 'fmt' --- .../xcm/xcm-builder/src/tests/barriers.rs | 58 ++++++------------- 1 file changed, 18 insertions(+), 40 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 19ffd5bc9878..d98e9beff019 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -772,7 +772,12 @@ fn deny_instructions_with_xcm_works() { // dummy filter which denies `ClearOrigin` struct DenyClearOrigin; impl ShouldExecute for DenyClearOrigin { - fn should_execute(_: &Location, message: &mut [Instruction], _: Weight, _: &mut Properties) -> Result<(), ProcessMessageError> { + fn should_execute( + _: &Location, + message: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { message.matcher().match_next_inst_while( |_| true, |inst| match inst { @@ -784,7 +789,8 @@ fn deny_instructions_with_xcm_works() { } } - // closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin` instruction + // closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin` + // instruction let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { assert_eq!( DenyInstructionsWithXcm::::should_execute( @@ -798,17 +804,9 @@ fn deny_instructions_with_xcm_works() { }; // ok - assert_should_execute( - vec![ClearTransactStatus], - Location::parent(), - Ok(()), - ); + assert_should_execute(vec![ClearTransactStatus], Location::parent(), Ok(())); // ok top-level contains `ClearOrigin` - assert_should_execute( - vec![ClearOrigin], - Location::parent(), - Ok(()), - ); + assert_should_execute(vec![ClearOrigin], Location::parent(), Ok(())); // ok - SetAppendix with XCM without ClearOrigin assert_should_execute( vec![SetAppendix(Xcm(vec![ClearTransactStatus]))], @@ -817,11 +815,7 @@ fn deny_instructions_with_xcm_works() { ); // invalid - empty XCM - assert_should_execute( - vec![], - Location::parent(), - Err(ProcessMessageError::BadFormat), - ); + assert_should_execute(vec![], Location::parent(), Err(ProcessMessageError::BadFormat)); // invalid - SetAppendix with empty XCM assert_should_execute( vec![SetAppendix(Xcm(vec![]))], @@ -836,29 +830,13 @@ fn deny_instructions_with_xcm_works() { ); // invalid nested SetAppendix contains `ClearOrigin` assert_should_execute( - vec![SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ - SetAppendix(Xcm(vec![ClearOrigin])) - ])) - ])) - ])) - ])) - ])) - ])) - ])) - ])) - ])) - ])) - ]))], + vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix( + Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ + SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ClearOrigin]))])), + ]))]))])), + ]))]))]))]), + )]))]))]))], Location::parent(), Err(ProcessMessageError::StackLimitReached), ); From df64f5f679558b853ced1dd250d8630b25124577 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Fri, 31 Jan 2025 13:41:32 +0000 Subject: [PATCH 07/48] poc(XCM): Deny Nested XCM Barriers (#7351) Resolves (partially): #7148 Depends on: #7169, #7200 # Description For context and additional information, please refer to #7148 and #7200. # TODOs * [x] Rebase #7169 and #7200 * [x] Evaluate PoC described on #7200 | POC | Top-Level Denial | Nested Denial | Try `Allow` | Remark | |--------------------------------|--------------------|--------------------|--------------------|----------------------------------------------------------------------------------| | `DenyThenTry` | :white_check_mark: | :x: | :white_check_mark: | Blocks top-level instructions only. | | `RecursiveDenyThenTry` | :white_check_mark: | :white_check_mark: | :white_check_mark: | Blocks both top-level and nested instructions. | | `DenyInstructionsWithXcm` | :x: | :white_check_mark: | :x: | Focuses on nested instructions, requires additional checks for top-level denial. | | `DenyFirstInstructionsWithXcm` | :white_check_mark: | :white_check_mark: | :x: | Prioritises top-level denial before recursive checks. | --------- Co-authored-by: ron Co-authored-by: Branislav Kontur Co-authored-by: Francisco Aguirre Co-authored-by: command-bot <> Co-authored-by: Clara van Staden Co-authored-by: Adrian Catangiu Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- polkadot/xcm/xcm-builder/src/barriers.rs | 107 ++++++++-- polkadot/xcm/xcm-builder/src/lib.rs | 7 +- .../xcm/xcm-builder/src/tests/barriers.rs | 184 +++++++++++++++--- polkadot/xcm/xcm-builder/src/tests/mock.rs | 53 ++++- 4 files changed, 302 insertions(+), 49 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 6b7493728e24..1fa704dcac38 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -463,6 +463,47 @@ where } } +// TBD: +/// Denies execution if the XCM contains any of the denied instructions, even if nested +/// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`, +/// recursively using `DenyInstructionsWithXcm`. +/// +/// If the message passes the deny filters, it is then evaluated against the allow condition. +/// +/// Note: Ensures that restricted instructions are blocked at any depth with the XCM, enforcing +/// stricter execution policies. +pub struct RecursiveDenyThenTry(PhantomData, PhantomData) +where + Deny: DenyExecution, + Allow: ShouldExecute; + +impl ShouldExecute for RecursiveDenyThenTry +where + Deny: DenyExecution, + Allow: ShouldExecute, +{ + fn should_execute( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + // Apply the base deny filter + Deny::deny_execution(origin, instructions, max_weight, properties)?; + + // Recursively apply denial for nested XCM instructions + DenyInstructionsWithXcm::::deny_execution( + origin, + instructions, + max_weight, + properties, + )?; + + // If neither deny check fails, apply the allow filter + Allow::should_execute(origin, instructions, max_weight, properties) + } +} + // See issue pub struct DenyReserveTransferToRelayChain; impl DenyExecution for DenyReserveTransferToRelayChain { @@ -511,28 +552,27 @@ environmental::environmental!(recursion_count: u8); /// /// Note: The nested XCM is checked recursively! pub struct DenyInstructionsWithXcm(PhantomData); -impl ShouldExecute for DenyInstructionsWithXcm { - fn should_execute( + +impl DenyExecution for DenyInstructionsWithXcm { + fn deny_execution( origin: &Location, - message: &mut [Instruction], + instructions: &mut [Instruction], max_weight: Weight, properties: &mut Properties, ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( + instructions.matcher().match_next_inst_while( |_| true, |inst| match inst { SetAppendix(nested_xcm) | SetErrorHandler(nested_xcm) | ExecuteWithOrigin { xcm: nested_xcm, .. } => { - // check xcm instructions with `Inner` filter - let _ = Inner::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) + // Check xcm instructions with `Inner` filter + let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) .inspect_err(|e| { log::warn!( target: "xcm::barriers", - "`DenyInstructionsWithXcm`'s `Inner::should_execute` did not pass for origin: {:?} and nested_xcm: {:?} with error: {:?}", - origin, - nested_xcm, - e + "DenyInstructionsWithXcm::Inner denied execution, origin: {:?}, nested_xcm: {:?}, error: {:?}", + origin, nested_xcm, e ); })?; @@ -541,7 +581,13 @@ impl ShouldExecute for DenyInstructionsWithXcm { let _ = recursion_count::using_once(&mut 1, || { recursion_count::with(|count| { if *count > xcm_executor::RECURSION_LIMIT { - return Err(ProcessMessageError::StackLimitReached) + log::error!( + target: "xcm::barriers", + "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", + origin, nested_xcm + ); + + return Err(ProcessMessageError::StackLimitReached); } *count = count.saturating_add(1); Ok(()) @@ -557,8 +603,8 @@ impl ShouldExecute for DenyInstructionsWithXcm { }); } - // check recursively with `DenyInstructionsWithXcm` - Self::should_execute(origin, nested_xcm.inner_mut(), max_weight, properties) + // Check recursively with `DenyInstructionsWithXcm` + Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) })?; Ok(ControlFlow::Continue(())) @@ -571,3 +617,38 @@ impl ShouldExecute for DenyInstructionsWithXcm { Ok(()) } } + +// TBD: +/// Denies execution if the XCM contains restricted instructions, first checking at the stop level +/// and then recursively using `DenyInstructionsWithXcm`. +/// +/// Note: Ensures that restricted instructions are blocked at the top level and within nested XCM, +/// enforcing stricter execution policies. +pub struct DenyFirstInstructionsWithXcm(PhantomData); + +impl DenyExecution for DenyFirstInstructionsWithXcm { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + // First, check if the top-level message should be denied. + Inner::deny_execution(origin, instructions, max_weight, properties) + .inspect_err(|e| { + log::warn!( + target: "xcm::barriers", + "DenyFirstInstructionsWithXcm::Inner denied execution, origin: {:?}, instructions: {:?}, error: {:?}", + origin, instructions, e + ); + })?; + + // If the top-level check passes, check nested instructions recursively. + DenyInstructionsWithXcm::::deny_execution( + origin, + instructions, + max_weight, + properties, + ) + } +} diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 7c96ead236dd..6e034f0d41d8 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,9 +42,10 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyInstructionsWithXcm, DenyReserveTransferToRelayChain, - DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, - RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, + AllowUnpaidExecutionFrom, DenyFirstInstructionsWithXcm, DenyInstructionsWithXcm, + DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, IsParentsOnly, + IsSiblingSystemParachain, RecursiveDenyThenTry, RespectSuspension, TakeWeightCredit, + TrailingSetTopicAsId, WithComputedOrigin, }; mod controller; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index d98e9beff019..33e01fe0e8ca 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,9 +14,8 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use xcm_executor::traits::Properties; - use super::*; +use xcm_executor::traits::Properties; fn props(weight_credit: Weight) -> Properties { Properties { weight_credit, message_id: None } @@ -708,6 +707,75 @@ fn deny_then_try_works() { assert_should_execute(vec![ClearError, ClearTopic], Here.into_location(), Ok(())); } +#[test] +fn recursive_deny_and_try_xcm_works() { + frame_support::__private::sp_tracing::try_init_simple(); + + type Barrier = RecursiveDenyThenTry; + let xcm = Xcm::>(vec![DepositReserveAsset { + assets: Wild(All), + dest: Location::parent(), + xcm: vec![].into(), + }]); + let origin = Here.into_location(); + let max_weight = Weight::from_parts(10, 10); + let mut properties = props(Weight::zero()); + + // Should deny the original XCM + let result = + Barrier::should_execute(&origin, xcm.clone().inner_mut(), max_weight, &mut properties); + assert!(result.is_err()); + + // Should deny with `SetAppendix` + let mut message = Xcm::>(vec![SetAppendix(xcm.clone())]); + let result = + Barrier::should_execute(&origin, message.clone().inner_mut(), max_weight, &mut properties); + assert!(result.is_err()); + + // Should allow with `SetAppendix` for the original `DenyThenTry` + type OriginalBarrier = DenyThenTry; + let result = + OriginalBarrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); + assert!(result.is_ok()); + + // Should deny with `SetErrorHandler` + let mut message = Xcm::>(vec![SetErrorHandler(xcm.clone())]); + let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); + assert!(result.is_err()); + + // Should deny with `ExecuteWithOrigin` + let mut message = Xcm::>(vec![ExecuteWithOrigin { + xcm: xcm.clone(), + descendant_origin: None, + }]); + let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); + assert!(result.is_err()); + + // Should deny with more levels + let mut message = Xcm::>(vec![ExecuteWithOrigin { + xcm: vec![SetErrorHandler(vec![SetAppendix(xcm.clone())].into())].into(), + descendant_origin: None, + }]); + let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); + assert!(result.is_err()); + + // Should allow for valid XCM with `SetAppendix` + let xcm = Xcm::>(vec![DepositReserveAsset { + assets: Wild(All), + dest: Here.into_location(), + xcm: vec![].into(), + }]); + let mut message = Xcm::>(vec![SetAppendix(xcm.clone())]); + let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); + assert!(result.is_ok()); +} + +#[test] +fn recursive_deny_then_try_instructions_with_xcm_works() { + type Barrier = DenyWrapper>; + assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); +} + #[test] fn deny_reserve_transfer_to_relaychain_should_work() { let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { @@ -767,33 +835,87 @@ fn deny_reserve_transfer_to_relaychain_should_work() { #[test] fn deny_instructions_with_xcm_works() { + type Barrier = DenyInstructionsWithXcm; + assert_deny_nested_instructions_with_xcm::(Ok(())); +} + +#[test] +fn deny_first_instructions_with_xcm_works() { + type Barrier = DenyFirstInstructionsWithXcm; + assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); +} + +#[test] +fn compare_deny_filters() { frame_support::__private::sp_tracing::try_init_simple(); - // dummy filter which denies `ClearOrigin` - struct DenyClearOrigin; - impl ShouldExecute for DenyClearOrigin { - fn should_execute( - _: &Location, - message: &mut [Instruction], - _: Weight, - _: &mut Properties, - ) -> Result<(), ProcessMessageError> { - message.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ClearOrigin => Err(ProcessMessageError::Unsupported), - _ => Ok(ControlFlow::Continue(())), - }, - )?; - Ok(()) - } + type Denies = (DenyReserveTransferToRelayChain, DenyWrapper); + + fn assert_barrier( + top_level_result: Result<(), ProcessMessageError>, + nested_result: Result<(), ProcessMessageError>, + ) { + assert_deny_barrier::>(top_level_result, nested_result); } - // closure for (xcm, origin) testing with `DenyInstructionsWithXcm` which denies `ClearOrigin` + fn assert_deny_barrier( + top_level_result: Result<(), ProcessMessageError>, + nested_result: Result<(), ProcessMessageError>, + ) { + let mut xcm = Xcm::>( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Location::parent(), + xcm: vec![].into(), + }] + .into(), + ); + let mut nested_xcm = + Xcm::>(vec![SetErrorHandler(xcm.clone().into())].into()); + let origin = Here.into_location(); + let max_weight = Weight::zero(); + let mut properties = props(Weight::zero()); + + let result = Barrier::deny_execution(&origin, xcm.inner_mut(), max_weight, &mut properties); + assert_eq!(top_level_result, result); + + let result = + Barrier::deny_execution(&origin, nested_xcm.inner_mut(), max_weight, &mut properties); + assert_eq!(nested_result, result); + } + + // `DenyThenTry`: Top-level=Deny, Nested=Allow, TryAllow=Yes + assert_barrier::>(Err(ProcessMessageError::Unsupported), Ok(())); + + // `RecursiveDenyThenTry`: Top-level=Deny, Nested=Deny, TryAllow=Yes + assert_barrier::>( + Err(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), + ); + + // `DenyInstructionsWithXcm`: Top-level=Allow, Nested=Deny, TryAllow=No + assert_deny_barrier::>( + Ok(()), + Err(ProcessMessageError::Unsupported), + ); + + // `DenyFirstInstructionsWithXcm`: Top-level=Deny, Nested=Deny, TryAllow=No + assert_deny_barrier::>( + Err(ProcessMessageError::Unsupported), + Err(ProcessMessageError::Unsupported), + ); +} + +fn assert_deny_nested_instructions_with_xcm( + top_level_expected_result: Result<(), ProcessMessageError>, +) { + frame_support::__private::sp_tracing::try_init_simple(); + + // closure for (xcm, origin) testing with `Barrier` which denies `ClearOrigin` // instruction - let assert_should_execute = |mut xcm: Vec>, origin, expected_result| { + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { assert_eq!( - DenyInstructionsWithXcm::::should_execute( + Barrier::deny_execution( &origin, &mut xcm, Weight::from_parts(10, 10), @@ -804,32 +926,32 @@ fn deny_instructions_with_xcm_works() { }; // ok - assert_should_execute(vec![ClearTransactStatus], Location::parent(), Ok(())); - // ok top-level contains `ClearOrigin` - assert_should_execute(vec![ClearOrigin], Location::parent(), Ok(())); + assert_deny_execution(vec![ClearTransactStatus], Location::parent(), Ok(())); + // ok/err top-level contains `ClearOrigin` + assert_deny_execution(vec![ClearOrigin], Location::parent(), top_level_expected_result); // ok - SetAppendix with XCM without ClearOrigin - assert_should_execute( + assert_deny_execution( vec![SetAppendix(Xcm(vec![ClearTransactStatus]))], Location::parent(), Ok(()), ); // invalid - empty XCM - assert_should_execute(vec![], Location::parent(), Err(ProcessMessageError::BadFormat)); + assert_deny_execution(vec![], Location::parent(), Err(ProcessMessageError::BadFormat)); // invalid - SetAppendix with empty XCM - assert_should_execute( + assert_deny_execution( vec![SetAppendix(Xcm(vec![]))], Location::parent(), Err(ProcessMessageError::BadFormat), ); // invalid SetAppendix contains `ClearOrigin` - assert_should_execute( + assert_deny_execution( vec![SetAppendix(Xcm(vec![ClearOrigin]))], Location::parent(), Err(ProcessMessageError::Unsupported), ); // invalid nested SetAppendix contains `ClearOrigin` - assert_should_execute( + assert_deny_execution( vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix( Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index 127888104a4a..ed3046229d14 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -18,6 +18,7 @@ use crate::{ barriers::{AllowSubscriptionsFrom, RespectSuspension, TrailingSetTopicAsId}, + matcher::{CreateMatcher, MatchXcm}, test_utils::*, EnsureDecodableXcm, }; @@ -33,19 +34,20 @@ pub use core::{ fmt::Debug, ops::ControlFlow, }; -use frame_support::traits::{ContainsPair, Everything}; +use frame_support::traits::{ContainsPair, Everything, ProcessMessageError}; pub use frame_support::{ dispatch::{DispatchInfo, DispatchResultWithPostInfo, GetDispatchInfo, PostDispatchInfo}, ensure, parameter_types, sp_runtime::{traits::Dispatchable, DispatchError, DispatchErrorWithPostInfo}, traits::{Contains, Get, IsInVec}, }; +use std::marker::PhantomData; pub use xcm::latest::{prelude::*, QueryId, Weight}; pub use xcm_executor::{ traits::{ AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm, FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler, - QueryResponseStatus, TransactAsset, + QueryResponseStatus, ShouldExecute, TransactAsset, }, AssetsInHolding, Config, }; @@ -773,3 +775,50 @@ pub fn fungible_multi_asset(location: Location, amount: u128) -> Asset { pub fn fake_message_hash(message: &Xcm) -> XcmHash { message.using_encoded(sp_io::hashing::blake2_256) } + +// Dummy Barriers +// Dummy filter to allow all +pub struct AllowAll; +impl ShouldExecute for AllowAll { + fn should_execute( + _: &Location, + _: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Ok(()) + } +} + +// Dummy filter which denies `ClearOrigin` +pub struct DenyClearOrigin; +impl DenyExecution for DenyClearOrigin { + fn deny_execution( + _: &Location, + instructions: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } +} + +// Dummy filter which wraps `DenyExecution` on `ShouldExecution` +pub struct DenyWrapper(PhantomData); +impl DenyExecution for DenyWrapper { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Deny::should_execute(origin, instructions, max_weight, properties) + } +} From 72f8fe437f818b373a975a5fb2cfe67e50f8d0ec Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Fri, 31 Jan 2025 16:59:56 +0000 Subject: [PATCH 08/48] Refactor DenyInstructionsWithXcm with new NestedXcmType --- polkadot/xcm/xcm-builder/src/barriers.rs | 132 ++++++++++++++--------- 1 file changed, 84 insertions(+), 48 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 1fa704dcac38..13a36bbdf749 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -544,6 +544,27 @@ impl DenyExecution for DenyReserveTransferToRelayChain { } } +enum NestedXcmType<'a, RuntimeCall> { + Local(&'a mut RuntimeCall), + Remote(&'a mut RuntimeCall), + None, +} + +impl<'a, RuntimeCall> From> for NestedXcmType<'a, RuntimeCall> { + fn from(value: Instruction) -> Self { + match value { + // Local XCMs + ExecuteWithOrigin { mut xcm, .. } | SetAppendix(xcm) | SetErrorHandler(xcm) => + NestedXcmType::Local(&mut xcm), + // Remote XCMs + DepositReserveAsset { mut xcm, .. } | + InitiateReserveWithdraw { xcm, .. } | + TransferReserveAsset { xcm, .. } => NestedXcmType::Remote(&mut xcm), + _ => NestedXcmType::None, + } + } +} + environmental::environmental!(recursion_count: u8); // TBD: @@ -553,6 +574,60 @@ environmental::environmental!(recursion_count: u8); /// Note: The nested XCM is checked recursively! pub struct DenyInstructionsWithXcm(PhantomData); +impl DenyInstructionsWithXcm { + fn deny_recursively( + origin: &Location, + nested_xcm: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result, ProcessMessageError> { + // Check xcm instructions with `Inner` filter + let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) + .inspect_err(|e| { + log::warn!( + target: "xcm::barriers", + "DenyInstructionsWithXcm::Inner denied execution, origin: {:?}, nested_xcm: {:?}, error: {:?}", + origin, nested_xcm, e + ); + })?; + + // Initialize the recursion count only the first time we hit this code in our potential + // recursive execution. + let _ = recursion_count::using_once(&mut 1, || { + recursion_count::with(|count| { + if *count > xcm_executor::RECURSION_LIMIT { + log::error!( + target: "xcm::barriers", + "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", + origin, nested_xcm + ); + + return Err(ProcessMessageError::StackLimitReached); + } + + *count = count.saturating_add(1); + + Ok(()) + }) + // This should always return `Some`, but let's play it safe. + .unwrap_or(Ok(()))?; + + // Ensure that we always decrement the counter whenever we finish processing the + // `DenyInstructionsWithXcm`. + sp_core::defer! { + recursion_count::with(|count| { + *count = count.saturating_sub(1); + }); + } + + // Check recursively with `DenyInstructionsWithXcm` + Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) + })?; + + Ok(ControlFlow::Continue(())) + } +} + impl DenyExecution for DenyInstructionsWithXcm { fn deny_execution( origin: &Location, @@ -562,54 +637,15 @@ impl DenyExecution for DenyInstructionsWithXcm { ) -> Result<(), ProcessMessageError> { instructions.matcher().match_next_inst_while( |_| true, - |inst| match inst { - SetAppendix(nested_xcm) | - SetErrorHandler(nested_xcm) | - ExecuteWithOrigin { xcm: nested_xcm, .. } => { - // Check xcm instructions with `Inner` filter - let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) - .inspect_err(|e| { - log::warn!( - target: "xcm::barriers", - "DenyInstructionsWithXcm::Inner denied execution, origin: {:?}, nested_xcm: {:?}, error: {:?}", - origin, nested_xcm, e - ); - })?; - - // Initialize the recursion count only the first time we hit this code in our - // potential recursive execution. - let _ = recursion_count::using_once(&mut 1, || { - recursion_count::with(|count| { - if *count > xcm_executor::RECURSION_LIMIT { - log::error!( - target: "xcm::barriers", - "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", - origin, nested_xcm - ); - - return Err(ProcessMessageError::StackLimitReached); - } - *count = count.saturating_add(1); - Ok(()) - }) - // This should always return `Some`, but let's play it safe. - .unwrap_or(Ok(()))?; - - // Ensure that we always decrement the counter whenever we finish processing - // the `DenyInstructionsWithXcm`. - sp_core::defer! { - recursion_count::with(|count| { - *count = count.saturating_sub(1); - }); - } - - // Check recursively with `DenyInstructionsWithXcm` - Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) - })?; - - Ok(ControlFlow::Continue(())) - }, - _ => Ok(ControlFlow::Continue(())), + |inst| { + let nested_xcm_type: NestedXcmType = inst.into(); + match nested_xcm_type { + NestedXcmType::Local(nested_xcm) => + Self::deny_recursively(origin, nested_xcm, max_weight, properties), + NestedXcmType::Remote(nested_xcm) => + Self::deny_recursively(origin, nested_xcm, max_weight, properties), + NestedXcmType::None => Ok(ControlFlow::Continue(())), + } }, )?; From 5fe41bc63091356cbe4efe09b11de8cd8a41ceff Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 10:45:25 +0000 Subject: [PATCH 09/48] Rename to DenyNestedLocalInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 16 ++++++++-------- polkadot/xcm/xcm-builder/src/lib.rs | 2 +- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 12 ++++++------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 13a36bbdf749..28259c6df33f 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -492,7 +492,7 @@ where Deny::deny_execution(origin, instructions, max_weight, properties)?; // Recursively apply denial for nested XCM instructions - DenyInstructionsWithXcm::::deny_execution( + DenyNestedXcmInstructions::::deny_execution( origin, instructions, max_weight, @@ -572,9 +572,9 @@ environmental::environmental!(recursion_count: u8); /// `ExecuteWithOrigin` instructions. /// /// Note: The nested XCM is checked recursively! -pub struct DenyInstructionsWithXcm(PhantomData); +pub struct DenyNestedXcmInstructions(PhantomData); -impl DenyInstructionsWithXcm { +impl DenyNestedXcmInstructions { fn deny_recursively( origin: &Location, nested_xcm: &mut [Instruction], @@ -628,7 +628,7 @@ impl DenyInstructionsWithXcm { } } -impl DenyExecution for DenyInstructionsWithXcm { +impl DenyExecution for DenyNestedXcmInstructions { fn deny_execution( origin: &Location, instructions: &mut [Instruction], @@ -660,9 +660,9 @@ impl DenyExecution for DenyInstructionsWithXcm { /// /// Note: Ensures that restricted instructions are blocked at the top level and within nested XCM, /// enforcing stricter execution policies. -pub struct DenyFirstInstructionsWithXcm(PhantomData); +pub struct DenyNestedLocalInstructions(PhantomData); -impl DenyExecution for DenyFirstInstructionsWithXcm { +impl DenyExecution for DenyNestedLocalInstructions { fn deny_execution( origin: &Location, instructions: &mut [Instruction], @@ -674,13 +674,13 @@ impl DenyExecution for DenyFirstInstructionsWithXcm .inspect_err(|e| { log::warn!( target: "xcm::barriers", - "DenyFirstInstructionsWithXcm::Inner denied execution, origin: {:?}, instructions: {:?}, error: {:?}", + "DenyNestedLocalInstructions::Inner denied execution, origin: {:?}, instructions: {:?}, error: {:?}", origin, instructions, e ); })?; // If the top-level check passes, check nested instructions recursively. - DenyInstructionsWithXcm::::deny_execution( + DenyNestedXcmInstructions::::deny_execution( origin, instructions, max_weight, diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 6e034f0d41d8..7658dcf6c637 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,7 +42,7 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyFirstInstructionsWithXcm, DenyInstructionsWithXcm, + AllowUnpaidExecutionFrom, DenyNestedLocalInstructions, DenyNestedXcmInstructions, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RecursiveDenyThenTry, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 33e01fe0e8ca..46776b197eee 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -835,13 +835,13 @@ fn deny_reserve_transfer_to_relaychain_should_work() { #[test] fn deny_instructions_with_xcm_works() { - type Barrier = DenyInstructionsWithXcm; + type Barrier = DenyNestedXcmInstructions; assert_deny_nested_instructions_with_xcm::(Ok(())); } #[test] -fn deny_first_instructions_with_xcm_works() { - type Barrier = DenyFirstInstructionsWithXcm; +fn deny_nested_local_instructions_works() { + type Barrier = DenyNestedLocalInstructions; assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); } @@ -894,13 +894,13 @@ fn compare_deny_filters() { ); // `DenyInstructionsWithXcm`: Top-level=Allow, Nested=Deny, TryAllow=No - assert_deny_barrier::>( + assert_deny_barrier::>( Ok(()), Err(ProcessMessageError::Unsupported), ); - // `DenyFirstInstructionsWithXcm`: Top-level=Deny, Nested=Deny, TryAllow=No - assert_deny_barrier::>( + // `DenyNestedLocalInstructions`: Top-level=Deny, Nested=Deny, TryAllow=No + assert_deny_barrier::>( Err(ProcessMessageError::Unsupported), Err(ProcessMessageError::Unsupported), ); From 95ce08dc27a30da37aacc3e6e7ad8eda017e2264 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 10:53:25 +0000 Subject: [PATCH 10/48] Remove NestedXcmType --- polkadot/xcm/xcm-builder/src/barriers.rs | 41 ++++++++---------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 28259c6df33f..5dbe5d46482a 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -544,27 +544,6 @@ impl DenyExecution for DenyReserveTransferToRelayChain { } } -enum NestedXcmType<'a, RuntimeCall> { - Local(&'a mut RuntimeCall), - Remote(&'a mut RuntimeCall), - None, -} - -impl<'a, RuntimeCall> From> for NestedXcmType<'a, RuntimeCall> { - fn from(value: Instruction) -> Self { - match value { - // Local XCMs - ExecuteWithOrigin { mut xcm, .. } | SetAppendix(xcm) | SetErrorHandler(xcm) => - NestedXcmType::Local(&mut xcm), - // Remote XCMs - DepositReserveAsset { mut xcm, .. } | - InitiateReserveWithdraw { xcm, .. } | - TransferReserveAsset { xcm, .. } => NestedXcmType::Remote(&mut xcm), - _ => NestedXcmType::None, - } - } -} - environmental::environmental!(recursion_count: u8); // TBD: @@ -577,7 +556,7 @@ pub struct DenyNestedXcmInstructions(PhantomData); impl DenyNestedXcmInstructions { fn deny_recursively( origin: &Location, - nested_xcm: &mut [Instruction], + mut nested_xcm: Xcm, max_weight: Weight, properties: &mut Properties, ) -> Result, ProcessMessageError> { @@ -626,6 +605,15 @@ impl DenyNestedXcmInstructions { Ok(ControlFlow::Continue(())) } + + fn get_local_xcm(value: &Instruction) -> Option<&Xcm> { + match value { + // Local XCMs + ExecuteWithOrigin { xcm, .. } | SetAppendix(xcm) | SetErrorHandler(xcm) => + Some(xcm), + _ => None, + } + } } impl DenyExecution for DenyNestedXcmInstructions { @@ -638,13 +626,10 @@ impl DenyExecution for DenyNestedXcmInstructions { instructions.matcher().match_next_inst_while( |_| true, |inst| { - let nested_xcm_type: NestedXcmType = inst.into(); - match nested_xcm_type { - NestedXcmType::Local(nested_xcm) => - Self::deny_recursively(origin, nested_xcm, max_weight, properties), - NestedXcmType::Remote(nested_xcm) => + match Self::get_local_xcm(inst) { + Some(&nested_xcm) => Self::deny_recursively(origin, nested_xcm, max_weight, properties), - NestedXcmType::None => Ok(ControlFlow::Continue(())), + None => Ok(ControlFlow::Continue(())), } }, )?; From 914ba61ef9c1e1b3e3e5abbaa9b20e5e9f01eb38 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 11:09:40 +0000 Subject: [PATCH 11/48] Refactor DenyNestedXcmInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 70 ++++++++----------- .../xcm/xcm-builder/src/tests/barriers.rs | 2 +- 2 files changed, 29 insertions(+), 43 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 5dbe5d46482a..448e35652102 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -466,7 +466,7 @@ where // TBD: /// Denies execution if the XCM contains any of the denied instructions, even if nested /// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`, -/// recursively using `DenyInstructionsWithXcm`. +/// recursively using `DenyNestedXcmInstructions`. /// /// If the message passes the deny filters, it is then evaluated against the allow condition. /// @@ -553,66 +553,51 @@ environmental::environmental!(recursion_count: u8); /// Note: The nested XCM is checked recursively! pub struct DenyNestedXcmInstructions(PhantomData); + impl DenyNestedXcmInstructions { - fn deny_recursively( - origin: &Location, - mut nested_xcm: Xcm, - max_weight: Weight, - properties: &mut Properties, - ) -> Result, ProcessMessageError> { + fn deny_recursively(origin: &Location, nested_xcm: &mut Xcm, max_weight: Weight, properties: &mut Properties) -> Result, ProcessMessageError>, ProcessMessageError> { // Check xcm instructions with `Inner` filter let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) .inspect_err(|e| { log::warn!( target: "xcm::barriers", - "DenyInstructionsWithXcm::Inner denied execution, origin: {:?}, nested_xcm: {:?}, error: {:?}", + "DenyNestedXcmInstructions::Inner denied execution, origin: {:?}, nested_xcm: {:?}, error: {:?}", origin, nested_xcm, e ); })?; - // Initialize the recursion count only the first time we hit this code in our potential - // recursive execution. + // Initialize the recursion count only the first time we hit this code in our + // potential recursive execution. let _ = recursion_count::using_once(&mut 1, || { recursion_count::with(|count| { if *count > xcm_executor::RECURSION_LIMIT { log::error!( - target: "xcm::barriers", - "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", - origin, nested_xcm - ); + target: "xcm::barriers", + "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", + origin, nested_xcm + ); return Err(ProcessMessageError::StackLimitReached); } - *count = count.saturating_add(1); - Ok(()) }) - // This should always return `Some`, but let's play it safe. - .unwrap_or(Ok(()))?; + // This should always return `Some`, but let's play it safe. + .unwrap_or(Ok(()))?; - // Ensure that we always decrement the counter whenever we finish processing the - // `DenyInstructionsWithXcm`. + // Ensure that we always decrement the counter whenever we finish processing + // the `DenyNestedXcmInstructions`. sp_core::defer! { - recursion_count::with(|count| { - *count = count.saturating_sub(1); - }); - } + recursion_count::with(|count| { + *count = count.saturating_sub(1); + }); + } - // Check recursively with `DenyInstructionsWithXcm` + // Check recursively with `DenyNestedXcmInstructions` Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) })?; - Ok(ControlFlow::Continue(())) - } - - fn get_local_xcm(value: &Instruction) -> Option<&Xcm> { - match value { - // Local XCMs - ExecuteWithOrigin { xcm, .. } | SetAppendix(xcm) | SetErrorHandler(xcm) => - Some(xcm), - _ => None, - } + Ok(Ok(ControlFlow::Continue(()))) } } @@ -625,12 +610,13 @@ impl DenyExecution for DenyNestedXcmInstructions { ) -> Result<(), ProcessMessageError> { instructions.matcher().match_next_inst_while( |_| true, - |inst| { - match Self::get_local_xcm(inst) { - Some(&nested_xcm) => - Self::deny_recursively(origin, nested_xcm, max_weight, properties), - None => Ok(ControlFlow::Continue(())), - } + |inst| match inst { + SetAppendix(nested_xcm) | + SetErrorHandler(nested_xcm) | + ExecuteWithOrigin { xcm: nested_xcm, .. } => { + Self::deny_recursively(origin, nested_xcm, max_weight, properties)? + }, + _ => Ok(ControlFlow::Continue(())), }, )?; @@ -641,7 +627,7 @@ impl DenyExecution for DenyNestedXcmInstructions { // TBD: /// Denies execution if the XCM contains restricted instructions, first checking at the stop level -/// and then recursively using `DenyInstructionsWithXcm`. +/// and then recursively using `DenyNestedXcmInstructions`. /// /// Note: Ensures that restricted instructions are blocked at the top level and within nested XCM, /// enforcing stricter execution policies. diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 46776b197eee..614f42a19d88 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -893,7 +893,7 @@ fn compare_deny_filters() { Err(ProcessMessageError::Unsupported), ); - // `DenyInstructionsWithXcm`: Top-level=Allow, Nested=Deny, TryAllow=No + // `DenyNestedXcmInstructions`: Top-level=Allow, Nested=Deny, TryAllow=No assert_deny_barrier::>( Ok(()), Err(ProcessMessageError::Unsupported), From 97f50a651216645d5f816f478dd4f0d46f558b31 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 11:20:28 +0000 Subject: [PATCH 12/48] Refactor DenyNestedLocalInstructionsThenTry --- polkadot/xcm/xcm-builder/src/barriers.rs | 65 ++++++------------- polkadot/xcm/xcm-builder/src/lib.rs | 2 +- .../xcm/xcm-builder/src/tests/barriers.rs | 16 ++--- 3 files changed, 27 insertions(+), 56 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 448e35652102..d8863df6d250 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -472,37 +472,7 @@ where /// /// Note: Ensures that restricted instructions are blocked at any depth with the XCM, enforcing /// stricter execution policies. -pub struct RecursiveDenyThenTry(PhantomData, PhantomData) -where - Deny: DenyExecution, - Allow: ShouldExecute; - -impl ShouldExecute for RecursiveDenyThenTry -where - Deny: DenyExecution, - Allow: ShouldExecute, -{ - fn should_execute( - origin: &Location, - instructions: &mut [Instruction], - max_weight: Weight, - properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - // Apply the base deny filter - Deny::deny_execution(origin, instructions, max_weight, properties)?; - - // Recursively apply denial for nested XCM instructions - DenyNestedXcmInstructions::::deny_execution( - origin, - instructions, - max_weight, - properties, - )?; - - // If neither deny check fails, apply the allow filter - Allow::should_execute(origin, instructions, max_weight, properties) - } -} +pub type DenyNestedLocalInstructionsThenTry = DenyThenTry, Allow>; // See issue pub struct DenyReserveTransferToRelayChain; @@ -553,9 +523,13 @@ environmental::environmental!(recursion_count: u8); /// Note: The nested XCM is checked recursively! pub struct DenyNestedXcmInstructions(PhantomData); - impl DenyNestedXcmInstructions { - fn deny_recursively(origin: &Location, nested_xcm: &mut Xcm, max_weight: Weight, properties: &mut Properties) -> Result, ProcessMessageError>, ProcessMessageError> { + fn deny_recursively( + origin: &Location, + nested_xcm: &mut Xcm, + max_weight: Weight, + properties: &mut Properties, + ) -> Result, ProcessMessageError>, ProcessMessageError> { // Check xcm instructions with `Inner` filter let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) .inspect_err(|e| { @@ -572,26 +546,26 @@ impl DenyNestedXcmInstructions { recursion_count::with(|count| { if *count > xcm_executor::RECURSION_LIMIT { log::error!( - target: "xcm::barriers", - "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", - origin, nested_xcm - ); + target: "xcm::barriers", + "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", + origin, nested_xcm + ); return Err(ProcessMessageError::StackLimitReached); } *count = count.saturating_add(1); Ok(()) }) - // This should always return `Some`, but let's play it safe. - .unwrap_or(Ok(()))?; + // This should always return `Some`, but let's play it safe. + .unwrap_or(Ok(()))?; // Ensure that we always decrement the counter whenever we finish processing // the `DenyNestedXcmInstructions`. sp_core::defer! { - recursion_count::with(|count| { - *count = count.saturating_sub(1); - }); - } + recursion_count::with(|count| { + *count = count.saturating_sub(1); + }); + } // Check recursively with `DenyNestedXcmInstructions` Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) @@ -613,9 +587,8 @@ impl DenyExecution for DenyNestedXcmInstructions { |inst| match inst { SetAppendix(nested_xcm) | SetErrorHandler(nested_xcm) | - ExecuteWithOrigin { xcm: nested_xcm, .. } => { - Self::deny_recursively(origin, nested_xcm, max_weight, properties)? - }, + ExecuteWithOrigin { xcm: nested_xcm, .. } => + Self::deny_recursively(origin, nested_xcm, max_weight, properties)?, _ => Ok(ControlFlow::Continue(())), }, )?; diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 7658dcf6c637..36d6288d35f8 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -44,7 +44,7 @@ pub use barriers::{ AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, AllowUnpaidExecutionFrom, DenyNestedLocalInstructions, DenyNestedXcmInstructions, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, IsParentsOnly, - IsSiblingSystemParachain, RecursiveDenyThenTry, RespectSuspension, TakeWeightCredit, + IsSiblingSystemParachain, DenyNestedLocalInstructionsThenTry, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 614f42a19d88..f9f36268aa23 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -708,10 +708,10 @@ fn deny_then_try_works() { } #[test] -fn recursive_deny_and_try_xcm_works() { +fn deny_nested_local_instructions_then_try_works() { frame_support::__private::sp_tracing::try_init_simple(); - type Barrier = RecursiveDenyThenTry; + type Barrier = DenyNestedLocalInstructionsThenTry; let xcm = Xcm::>(vec![DepositReserveAsset { assets: Wild(All), dest: Location::parent(), @@ -768,12 +768,10 @@ fn recursive_deny_and_try_xcm_works() { let mut message = Xcm::>(vec![SetAppendix(xcm.clone())]); let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); assert!(result.is_ok()); -} -#[test] -fn recursive_deny_then_try_instructions_with_xcm_works() { - type Barrier = DenyWrapper>; - assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); + // Should deny recursively before allow + type BarrierDenyClearOrigin = DenyWrapper>; + assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); } #[test] @@ -887,8 +885,8 @@ fn compare_deny_filters() { // `DenyThenTry`: Top-level=Deny, Nested=Allow, TryAllow=Yes assert_barrier::>(Err(ProcessMessageError::Unsupported), Ok(())); - // `RecursiveDenyThenTry`: Top-level=Deny, Nested=Deny, TryAllow=Yes - assert_barrier::>( + // `DenyNestedLocalInstructionsThenTry`: Top-level=Deny, Nested=Deny, TryAllow=Yes + assert_barrier::>( Err(ProcessMessageError::Unsupported), Err(ProcessMessageError::Unsupported), ); From 91d954129f46c6c6cbc405c218340f8f3ed2f796 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 11:27:28 +0000 Subject: [PATCH 13/48] Make DenyNestedXcmInstructions privately --- polkadot/xcm/xcm-builder/src/barriers.rs | 5 +++-- polkadot/xcm/xcm-builder/src/lib.rs | 6 +++--- .../xcm/xcm-builder/src/tests/barriers.rs | 19 +++++-------------- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index d8863df6d250..a00810555059 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -472,7 +472,8 @@ where /// /// Note: Ensures that restricted instructions are blocked at any depth with the XCM, enforcing /// stricter execution policies. -pub type DenyNestedLocalInstructionsThenTry = DenyThenTry, Allow>; +pub type DenyNestedLocalInstructionsThenTry = + DenyThenTry, Allow>; // See issue pub struct DenyReserveTransferToRelayChain; @@ -521,7 +522,7 @@ environmental::environmental!(recursion_count: u8); /// `ExecuteWithOrigin` instructions. /// /// Note: The nested XCM is checked recursively! -pub struct DenyNestedXcmInstructions(PhantomData); +struct DenyNestedXcmInstructions(PhantomData); impl DenyNestedXcmInstructions { fn deny_recursively( diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 36d6288d35f8..729550f8db4b 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,10 +42,10 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyNestedLocalInstructions, DenyNestedXcmInstructions, + AllowUnpaidExecutionFrom, DenyNestedLocalInstructions, DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, IsParentsOnly, - IsSiblingSystemParachain, DenyNestedLocalInstructionsThenTry, RespectSuspension, TakeWeightCredit, - TrailingSetTopicAsId, WithComputedOrigin, + IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, + WithComputedOrigin, }; mod controller; diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index f9f36268aa23..8b8725fefa91 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -770,8 +770,11 @@ fn deny_nested_local_instructions_then_try_works() { assert!(result.is_ok()); // Should deny recursively before allow - type BarrierDenyClearOrigin = DenyWrapper>; - assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); + type BarrierDenyClearOrigin = + DenyWrapper>; + assert_deny_nested_instructions_with_xcm::(Err( + ProcessMessageError::Unsupported, + )); } #[test] @@ -831,12 +834,6 @@ fn deny_reserve_transfer_to_relaychain_should_work() { assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); } -#[test] -fn deny_instructions_with_xcm_works() { - type Barrier = DenyNestedXcmInstructions; - assert_deny_nested_instructions_with_xcm::(Ok(())); -} - #[test] fn deny_nested_local_instructions_works() { type Barrier = DenyNestedLocalInstructions; @@ -891,12 +888,6 @@ fn compare_deny_filters() { Err(ProcessMessageError::Unsupported), ); - // `DenyNestedXcmInstructions`: Top-level=Allow, Nested=Deny, TryAllow=No - assert_deny_barrier::>( - Ok(()), - Err(ProcessMessageError::Unsupported), - ); - // `DenyNestedLocalInstructions`: Top-level=Deny, Nested=Deny, TryAllow=No assert_deny_barrier::>( Err(ProcessMessageError::Unsupported), From 0866f2a8c72b845a59c35a1a5233612cb987e791 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 12:07:51 +0000 Subject: [PATCH 14/48] Update comments --- polkadot/xcm/xcm-builder/src/barriers.rs | 57 ++++++++++++++---------- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index a00810555059..633815af0c8f 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -463,15 +463,16 @@ where } } -// TBD: -/// Denies execution if the XCM contains any of the denied instructions, even if nested -/// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`, -/// recursively using `DenyNestedXcmInstructions`. +/// Denies execution if the XCM contains any of the denied **local** instructions, even if nested +/// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`. +/// +/// This check is applied **recursively** using `DenyNestedXcmInstructions`, ensuring that +/// instructions do not execute on the local chain at any depth. /// /// If the message passes the deny filters, it is then evaluated against the allow condition. /// -/// Note: Ensures that restricted instructions are blocked at any depth with the XCM, enforcing -/// stricter execution policies. +/// Note: This applies only to locally executed instructions. Remote parts of the XCM are expected +/// to be validated by the destination chain's barrier. pub type DenyNestedLocalInstructionsThenTry = DenyThenTry, Allow>; @@ -517,32 +518,36 @@ impl DenyExecution for DenyReserveTransferToRelayChain { environmental::environmental!(recursion_count: u8); -// TBD: -/// Applies the `Inner` filter to the nested XCM for the `SetAppendix`, `SetErrorHandler`, and -/// `ExecuteWithOrigin` instructions. +// A private helper struct that applies the `Inner` filter to nested XCMs within certain +// instructions. /// -/// Note: The nested XCM is checked recursively! +/// This struct is not meant to be used directly outside of this module. It ensures that restricted +/// local instructions within `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` are blocked +/// **recursively**, preventing unintended execution of nested XCMs. struct DenyNestedXcmInstructions(PhantomData); impl DenyNestedXcmInstructions { + // Recursively applies the deny filter to a nested XCM. + /// + /// This function ensures that restricted instructions are blocked at any depth within the XCM. + /// It maintains a **recursion counter** to prevent stack overflows due to a deep nesting. fn deny_recursively( origin: &Location, nested_xcm: &mut Xcm, max_weight: Weight, properties: &mut Properties, ) -> Result, ProcessMessageError>, ProcessMessageError> { - // Check xcm instructions with `Inner` filter + // Apply the `Inner` deny filter to the nested XCM. let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) .inspect_err(|e| { log::warn!( target: "xcm::barriers", - "DenyNestedXcmInstructions::Inner denied execution, origin: {:?}, nested_xcm: {:?}, error: {:?}", + "Execution denied by Inner filter, origin: {:?}, nested_xcm: {:?}, error: {:?}", origin, nested_xcm, e ); })?; - // Initialize the recursion count only the first time we hit this code in our - // potential recursive execution. + // Initialise the recursion count the first time we enter recursive execution. let _ = recursion_count::using_once(&mut 1, || { recursion_count::with(|count| { if *count > xcm_executor::RECURSION_LIMIT { @@ -557,18 +562,17 @@ impl DenyNestedXcmInstructions { *count = count.saturating_add(1); Ok(()) }) - // This should always return `Some`, but let's play it safe. + // Fallback safety in case of an unexpected failure. .unwrap_or(Ok(()))?; - // Ensure that we always decrement the counter whenever we finish processing - // the `DenyNestedXcmInstructions`. + // Ensure that we always decrement the counter after processing. sp_core::defer! { recursion_count::with(|count| { *count = count.saturating_sub(1); }); } - // Check recursively with `DenyNestedXcmInstructions` + // Recursively check the nested XCM instrucitons. Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) })?; @@ -577,6 +581,10 @@ impl DenyNestedXcmInstructions { } impl DenyExecution for DenyNestedXcmInstructions { + /// Denies execution of restricted nested XCM instructions. + /// + /// This checks for `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instruction + /// applying the deny filter **recursively** to any nested XCMs found. fn deny_execution( origin: &Location, instructions: &mut [Instruction], @@ -599,12 +607,15 @@ impl DenyExecution for DenyNestedXcmInstructions { } } -// TBD: -/// Denies execution if the XCM contains restricted instructions, first checking at the stop level -/// and then recursively using `DenyNestedXcmInstructions`. +/// Denies execution if the XCM contains restricted **local** instructions, first checking at the +/// top-level and then **recursively** using `DenyNestedXcmInstructions`. +/// +/// This barrier only applies to **locally executed** XCM instructions (`SetAppendix`, +/// `SetErrorHandler`, and `ExecuteWithOrigin`). Remote parts of the XCM are expected to be +/// validated by receiving chain's barrier. /// -/// Note: Ensures that restricted instructions are blocked at the top level and within nested XCM, -/// enforcing stricter execution policies. +/// Note: Ensures that restricted instructions do not execute on the local chain, enforcing stricter +/// execution policies, while allowing remote chains to enforce their own rules. pub struct DenyNestedLocalInstructions(PhantomData); impl DenyExecution for DenyNestedLocalInstructions { From b58f7a3bc9002c079825633bc3a5edb26958d152 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 12:38:41 +0000 Subject: [PATCH 15/48] Update tests --- .../xcm/xcm-builder/src/tests/barriers.rs | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 8b8725fefa91..ae82243d18c3 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -709,8 +709,6 @@ fn deny_then_try_works() { #[test] fn deny_nested_local_instructions_then_try_works() { - frame_support::__private::sp_tracing::try_init_simple(); - type Barrier = DenyNestedLocalInstructionsThenTry; let xcm = Xcm::>(vec![DepositReserveAsset { assets: Wild(All), @@ -769,6 +767,19 @@ fn deny_nested_local_instructions_then_try_works() { let result = Barrier::should_execute(&origin, message.inner_mut(), max_weight, &mut properties); assert!(result.is_ok()); + // Should ensure unrelated XCMs are not blocked + let mut unrelated_xcm = Xcm::>(vec![BuyExecution { + fees: (Parent, 100).into(), + weight_limit: Unlimited, + }]); + let result = Barrier::should_execute( + &origin, + unrelated_xcm.inner_mut(), + max_weight, + &mut properties, + ); + assert!(result.is_ok()); + // Should deny recursively before allow type BarrierDenyClearOrigin = DenyWrapper>; @@ -842,9 +853,7 @@ fn deny_nested_local_instructions_works() { #[test] fn compare_deny_filters() { - frame_support::__private::sp_tracing::try_init_simple(); - - type Denies = (DenyReserveTransferToRelayChain, DenyWrapper); + type Denies = (DenyWrapper, DenyReserveTransferToRelayChain); fn assert_barrier( top_level_result: Result<(), ProcessMessageError>, @@ -898,8 +907,6 @@ fn compare_deny_filters() { fn assert_deny_nested_instructions_with_xcm( top_level_expected_result: Result<(), ProcessMessageError>, ) { - frame_support::__private::sp_tracing::try_init_simple(); - // closure for (xcm, origin) testing with `Barrier` which denies `ClearOrigin` // instruction let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { From 255a23afcee70ba1c6d0f5eaf76c7a64d4a31391 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 12:40:34 +0000 Subject: [PATCH 16/48] Fix imports --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index ae82243d18c3..2a92314731af 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,9 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use super::*; use xcm_executor::traits::Properties; +use super::*; + fn props(weight_credit: Weight) -> Properties { Properties { weight_credit, message_id: None } } @@ -772,12 +773,8 @@ fn deny_nested_local_instructions_then_try_works() { fees: (Parent, 100).into(), weight_limit: Unlimited, }]); - let result = Barrier::should_execute( - &origin, - unrelated_xcm.inner_mut(), - max_weight, - &mut properties, - ); + let result = + Barrier::should_execute(&origin, unrelated_xcm.inner_mut(), max_weight, &mut properties); assert!(result.is_ok()); // Should deny recursively before allow From 4f8d0afbdfb279c0fa05b664b1347822950be713 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 3 Feb 2025 12:45:24 +0000 Subject: [PATCH 17/48] Update from raymondkfcheung running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_7200.prdoc | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 prdoc/pr_7200.prdoc diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc new file mode 100644 index 000000000000..e3d39c07dcac --- /dev/null +++ b/prdoc/pr_7200.prdoc @@ -0,0 +1,40 @@ +title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be + executed on the local chain' +doc: +- audience: Runtime Dev + description: "Resolves (partially): https://github.com/paritytech/polkadot-sdk/issues/7148\n\ + Depends on: https://github.com/paritytech/polkadot-sdk/pull/7169\n\n# Description\n\ + \nFor context and additional information, please refer to [_Problem 2 - Barrier\ + \ vs nested XCM validation_](https://github.com/paritytech/polkadot-sdk/issues/7148).\n\ + \n# TODO\n- [x] Evaluate PoC, more details at #7351:\n - **DenyInstructionsWithXcm**:\ + \ Keep it as it is and be explicit:\n 1. Name the Deny barriers for the top\ + \ level.\n 2. Name the Deny barrier for nested with `DenyInstructionsWithXcm`.\n\ + \ - **RecursiveDenyThenTry**: Alternatively, hard-code those three instructions\ + \ in `DenyThenTry`, so we wouldn\u2019t need `DenyInstructionsWithXcm`. However,\ + \ this approach wouldn\u2019t be as general.\n - **DenyFirstInstructionsWithXcm**:\ + \ Another possibility is to check `DenyInstructionsWithXcm::Inner` for the actual\ + \ `message`, so we don\u2019t need duplication for top-level and nested (not sure,\ + \ maybe be explicit is good thing) - see _Problem2 - example_. Instead of this:\n\ + \ ```\n DenyThenTry<\n (\n //\ + \ Deny for top level XCM program\n DenyReserveTransferToRelayChain,\n\ + \ // Dedicated barrier for nested XCM programs\n\ + \ DenyInstructionsWithXcmFor<\n \ + \ // Repeat all Deny filters here\n \ + \ DenyReserveTransferToRelayChain,\n \ + \ >\n ),\n ```\n we could just use:\n\ + \ ```\n DenyThenTry<\n (\n //\ + \ Dedicated barrier for XCM programs\n DenyInstructionsWithXcmFor<\n\ + \ // Add all `Deny` filters here\n\ + \ DenyReserveTransferToRelayChain,\n\ + \ ...\n \ + \ >\n ),\n ```\n - [POC Evaluation](https://github.com/paritytech/polkadot-sdk/pull/7200/files#r19374329150)\n\ + - [x] Consider better name `DenyInstructionsWithXcm` => `DenyNestedLocalInstructions`\ + \ and `DenyNestedLocalInstructionsThenTry`, more details at [here](https://github.com/paritytech/polkadot-sdk/pull/7200#discussion_r1933637941)\n\ + - [x] Clean-up and docs\n- [x] Merge https://github.com/paritytech/polkadot-sdk/pull/7169\ + \ or rebase this branch on the top of `yrong:fix-for-deny-then-try`\n- [ ] Set\ + \ for the runtimes where we use `DenyThenTry`\n- [ ] Schedule sec.audit" +crates: +- name: staging-xcm-builder + bump: patch +- name: staging-xcm-executor + bump: patch From 2a48a71dfd42d265148fbbd7af16daddbbe3b1a6 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 14:01:33 +0000 Subject: [PATCH 18/48] Move deny_execution logic to DenyNestedLocalInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 62 +++++++++--------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 633815af0c8f..3ce06ea7c1b6 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -466,7 +466,7 @@ where /// Denies execution if the XCM contains any of the denied **local** instructions, even if nested /// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`. /// -/// This check is applied **recursively** using `DenyNestedXcmInstructions`, ensuring that +/// This check is applied **recursively** using `DenyNestedLocalInstructions`, ensuring that /// instructions do not execute on the local chain at any depth. /// /// If the message passes the deny filters, it is then evaluated against the allow condition. @@ -522,7 +522,7 @@ environmental::environmental!(recursion_count: u8); // instructions. /// /// This struct is not meant to be used directly outside of this module. It ensures that restricted -/// local instructions within `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` are blocked +/// instructions within `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` are blocked /// **recursively**, preventing unintended execution of nested XCMs. struct DenyNestedXcmInstructions(PhantomData); @@ -531,7 +531,7 @@ impl DenyNestedXcmInstructions { /// /// This function ensures that restricted instructions are blocked at any depth within the XCM. /// It maintains a **recursion counter** to prevent stack overflows due to a deep nesting. - fn deny_recursively( + fn deny_recursively( origin: &Location, nested_xcm: &mut Xcm, max_weight: Weight, @@ -572,41 +572,14 @@ impl DenyNestedXcmInstructions { }); } - // Recursively check the nested XCM instrucitons. - Self::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) + // Recursively check the nested XCM instructions. + Deny::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) })?; Ok(Ok(ControlFlow::Continue(()))) } } -impl DenyExecution for DenyNestedXcmInstructions { - /// Denies execution of restricted nested XCM instructions. - /// - /// This checks for `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instruction - /// applying the deny filter **recursively** to any nested XCMs found. - fn deny_execution( - origin: &Location, - instructions: &mut [Instruction], - max_weight: Weight, - properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - instructions.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - SetAppendix(nested_xcm) | - SetErrorHandler(nested_xcm) | - ExecuteWithOrigin { xcm: nested_xcm, .. } => - Self::deny_recursively(origin, nested_xcm, max_weight, properties)?, - _ => Ok(ControlFlow::Continue(())), - }, - )?; - - // Permit everything else - Ok(()) - } -} - /// Denies execution if the XCM contains restricted **local** instructions, first checking at the /// top-level and then **recursively** using `DenyNestedXcmInstructions`. /// @@ -619,6 +592,10 @@ impl DenyExecution for DenyNestedXcmInstructions { pub struct DenyNestedLocalInstructions(PhantomData); impl DenyExecution for DenyNestedLocalInstructions { + /// Denies execution of restricted local nested XCM instructions. + /// + /// This checks for `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instruction + /// applying the deny filter **recursively** to any nested XCMs found. fn deny_execution( origin: &Location, instructions: &mut [Instruction], @@ -636,11 +613,20 @@ impl DenyExecution for DenyNestedLocalInstructions })?; // If the top-level check passes, check nested instructions recursively. - DenyNestedXcmInstructions::::deny_execution( - origin, - instructions, - max_weight, - properties, - ) + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + SetAppendix(nested_xcm) | + SetErrorHandler(nested_xcm) | + ExecuteWithOrigin { xcm: nested_xcm, .. } => + DenyNestedXcmInstructions::::deny_recursively::( + origin, nested_xcm, max_weight, properties, + )?, + _ => Ok(ControlFlow::Continue(())), + }, + )?; + + // Permit everything else + Ok(()) } } From 9ca64e72431a9d074859d1c038461f618c664db0 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 14:37:16 +0000 Subject: [PATCH 19/48] Update config --- .../assets/asset-hub-rococo/src/xcm_config.rs | 24 +++++++++---------- .../asset-hub-westend/src/xcm_config.rs | 24 +++++++++---------- .../bridge-hub-rococo/src/xcm_config.rs | 14 +++++------ .../bridge-hub-westend/src/xcm_config.rs | 14 +++++------ .../collectives-westend/src/xcm_config.rs | 15 ++---------- .../contracts-rococo/src/xcm_config.rs | 8 +++---- .../coretime-rococo/src/xcm_config.rs | 14 +++++------ .../coretime-westend/src/xcm_config.rs | 14 +++++------ .../people/people-rococo/src/xcm_config.rs | 10 ++++---- .../people/people-westend/src/xcm_config.rs | 10 ++++---- .../runtime/src/configs/xcm_config.rs | 8 +++---- 11 files changed, 72 insertions(+), 83 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs index 0c6ff5e4bfdd..8b59455e664b 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs @@ -51,17 +51,17 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH, WESTEND_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, - AllowTopLevelPaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, - DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, - FungibleAdapter, FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, - IsConcrete, LocalMint, MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, - NonFungiblesAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, - SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, - SignedAccountId32AsNative, SignedToAccountId32, SingleAssetExchangeAdapter, - SovereignPaidRemoteExporter, SovereignSignedViaLocation, StartsWith, - StartsWithExplicitGlobalConsensus, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, - WeightInfoBounds, WithComputedOrigin, WithLatestLocationConverter, WithUniqueTopic, - XcmFeeManagerFromComponents, + AllowTopLevelPaidExecutionFrom, DenyNestedLocalInstructionsThenTry, + DenyReserveTransferToRelayChain, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, + FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, + GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint, + MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, NonFungiblesAdapter, + ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, + SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, + SignedToAccountId32, SingleAssetExchangeAdapter, SovereignPaidRemoteExporter, + SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, + WithLatestLocationConverter, WithUniqueTopic, XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -268,7 +268,7 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs index 1ea2ce5136ab..1ecb9a3afddc 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs @@ -48,17 +48,17 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH, WESTEND_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, - AllowTopLevelPaidExecutionFrom, DenyReserveTransferToRelayChain, DenyThenTry, - DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, - FungibleAdapter, FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, - IsConcrete, LocalMint, MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, - NonFungiblesAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, - SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, - SignedAccountId32AsNative, SignedToAccountId32, SingleAssetExchangeAdapter, - SovereignPaidRemoteExporter, SovereignSignedViaLocation, StartsWith, - StartsWithExplicitGlobalConsensus, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, - WeightInfoBounds, WithComputedOrigin, WithLatestLocationConverter, WithUniqueTopic, - XcmFeeManagerFromComponents, + AllowTopLevelPaidExecutionFrom, DenyNestedLocalInstructionsThenTry, + DenyReserveTransferToRelayChain, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, + FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, + GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint, + MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, NonFungiblesAdapter, + ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, + SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, + SignedToAccountId32, SingleAssetExchangeAdapter, SovereignPaidRemoteExporter, + SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, + WithLatestLocationConverter, WithUniqueTopic, XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -280,7 +280,7 @@ impl Contains for AmbassadorEntities { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs index b37945317f6c..695851611896 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs @@ -44,12 +44,12 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HandleFee, HashedDescription, - IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, - UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HandleFee, + HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, + SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, }; use xcm_executor::{ traits::{FeeManager, FeeReason, FeeReason::Export}, @@ -130,7 +130,7 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs index befb63ef9709..f12c93c352ab 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs @@ -43,12 +43,12 @@ use xcm::latest::{prelude::*, WESTEND_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HandleFee, HashedDescription, - IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, - UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HandleFee, + HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, + SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, }; use xcm_executor::{ traits::{FeeManager, FeeReason, FeeReason::Export}, @@ -128,7 +128,7 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs index c5ab21fe8f90..92104cb4d053 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs @@ -33,18 +33,7 @@ use polkadot_parachain_primitives::primitives::Sibling; use polkadot_runtime_common::xcm_sender::ExponentialPrice; use westend_runtime_constants::xcm as xcm_constants; use xcm::latest::{prelude::*, WESTEND_GENESIS_HASH}; -use xcm_builder::{ - AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, - AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, - AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, - LocatableAssetId, OriginToPluralityVoice, ParentAsSuperuser, ParentIsPreset, - RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, - SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, - TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, - XcmFeeManagerFromComponents, -}; +use xcm_builder::{AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, LocatableAssetId, OriginToPluralityVoice, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents}; use xcm_executor::XcmExecutor; parameter_types! { @@ -145,7 +134,7 @@ impl Contains for LocalPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs index 532ad4ff4ce0..a258cc8950a5 100644 --- a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs @@ -40,9 +40,9 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor, FungibleAdapter, - HashedDescription, IsConcrete, NativeAsset, ParentAsSuperuser, ParentIsPreset, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor, + FungibleAdapter, HashedDescription, IsConcrete, NativeAsset, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic, @@ -133,7 +133,7 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs index 33ad172962a1..56baf944ce8f 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs @@ -41,12 +41,12 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, - NonFungibleAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, - UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, + HashedDescription, IsConcrete, NonFungibleAdapter, ParentAsSuperuser, ParentIsPreset, + RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -144,7 +144,7 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs index 8a4879a1506e..98c06f0218f9 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs @@ -42,12 +42,12 @@ use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, - NonFungibleAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, - UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, + HashedDescription, IsConcrete, NonFungibleAdapter, ParentAsSuperuser, ParentIsPreset, + RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, + TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -153,7 +153,7 @@ impl Contains for FellowsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs index 724d87587c6c..e1872fac211e 100644 --- a/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs @@ -38,10 +38,10 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - DescribeTerminus, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, - HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, - SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, DescribeTerminus, EnsureXcmOrigin, FrameTransactionalProcessor, + FungibleAdapter, HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, + RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents, @@ -152,7 +152,7 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs index 7eaa43c05b20..57fe1c6abf1e 100644 --- a/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs @@ -39,10 +39,10 @@ use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, - DescribeTerminus, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, - HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, - SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, DescribeTerminus, EnsureXcmOrigin, FrameTransactionalProcessor, + FungibleAdapter, HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, + RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents, @@ -161,7 +161,7 @@ impl Contains for FellowsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( // Allow local users to buy weight credit. diff --git a/templates/parachain/runtime/src/configs/xcm_config.rs b/templates/parachain/runtime/src/configs/xcm_config.rs index 3da3b711f4ff..482a767be8df 100644 --- a/templates/parachain/runtime/src/configs/xcm_config.rs +++ b/templates/parachain/runtime/src/configs/xcm_config.rs @@ -19,9 +19,9 @@ use polkadot_runtime_common::impls::ToAuthor; use xcm::latest::prelude::*; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowTopLevelPaidExecutionFrom, - DenyReserveTransferToRelayChain, DenyThenTry, EnsureXcmOrigin, FixedWeightBounds, - FrameTransactionalProcessor, FungibleAdapter, IsConcrete, NativeAsset, ParentIsPreset, - RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, EnsureXcmOrigin, + FixedWeightBounds, FrameTransactionalProcessor, FungibleAdapter, IsConcrete, NativeAsset, + ParentIsPreset, RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic, }; @@ -98,7 +98,7 @@ impl Contains for ParentOrParentsExecutivePlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< + DenyNestedLocalInstructionsThenTry< DenyReserveTransferToRelayChain, ( TakeWeightCredit, From 9601ee068a44b617ab9d7cfd9ae711edce5bf90d Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 3 Feb 2025 14:50:34 +0000 Subject: [PATCH 20/48] Update from raymondkfcheung running command 'fmt' --- .../collectives-westend/src/xcm_config.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs index 92104cb4d053..e01121feef2e 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs @@ -33,7 +33,18 @@ use polkadot_parachain_primitives::primitives::Sibling; use polkadot_runtime_common::xcm_sender::ExponentialPrice; use westend_runtime_constants::xcm as xcm_constants; use xcm::latest::{prelude::*, WESTEND_GENESIS_HASH}; -use xcm_builder::{AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, LocatableAssetId, OriginToPluralityVoice, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents}; +use xcm_builder::{ + AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, + AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, + AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, + DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, + HashedDescription, IsConcrete, LocatableAssetId, OriginToPluralityVoice, ParentAsSuperuser, + ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, + SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, + SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, + WeightInfoBounds, WithComputedOrigin, WithUniqueTopic, XcmFeeManagerFromComponents, +}; use xcm_executor::XcmExecutor; parameter_types! { From f2ff48d0f82d17aeeac5bfc251db150711efdb0c Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 14:54:55 +0000 Subject: [PATCH 21/48] Remove for regenerating --- prdoc/pr_7200.prdoc | 40 ---------------------------------------- 1 file changed, 40 deletions(-) delete mode 100644 prdoc/pr_7200.prdoc diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc deleted file mode 100644 index e3d39c07dcac..000000000000 --- a/prdoc/pr_7200.prdoc +++ /dev/null @@ -1,40 +0,0 @@ -title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be - executed on the local chain' -doc: -- audience: Runtime Dev - description: "Resolves (partially): https://github.com/paritytech/polkadot-sdk/issues/7148\n\ - Depends on: https://github.com/paritytech/polkadot-sdk/pull/7169\n\n# Description\n\ - \nFor context and additional information, please refer to [_Problem 2 - Barrier\ - \ vs nested XCM validation_](https://github.com/paritytech/polkadot-sdk/issues/7148).\n\ - \n# TODO\n- [x] Evaluate PoC, more details at #7351:\n - **DenyInstructionsWithXcm**:\ - \ Keep it as it is and be explicit:\n 1. Name the Deny barriers for the top\ - \ level.\n 2. Name the Deny barrier for nested with `DenyInstructionsWithXcm`.\n\ - \ - **RecursiveDenyThenTry**: Alternatively, hard-code those three instructions\ - \ in `DenyThenTry`, so we wouldn\u2019t need `DenyInstructionsWithXcm`. However,\ - \ this approach wouldn\u2019t be as general.\n - **DenyFirstInstructionsWithXcm**:\ - \ Another possibility is to check `DenyInstructionsWithXcm::Inner` for the actual\ - \ `message`, so we don\u2019t need duplication for top-level and nested (not sure,\ - \ maybe be explicit is good thing) - see _Problem2 - example_. Instead of this:\n\ - \ ```\n DenyThenTry<\n (\n //\ - \ Deny for top level XCM program\n DenyReserveTransferToRelayChain,\n\ - \ // Dedicated barrier for nested XCM programs\n\ - \ DenyInstructionsWithXcmFor<\n \ - \ // Repeat all Deny filters here\n \ - \ DenyReserveTransferToRelayChain,\n \ - \ >\n ),\n ```\n we could just use:\n\ - \ ```\n DenyThenTry<\n (\n //\ - \ Dedicated barrier for XCM programs\n DenyInstructionsWithXcmFor<\n\ - \ // Add all `Deny` filters here\n\ - \ DenyReserveTransferToRelayChain,\n\ - \ ...\n \ - \ >\n ),\n ```\n - [POC Evaluation](https://github.com/paritytech/polkadot-sdk/pull/7200/files#r19374329150)\n\ - - [x] Consider better name `DenyInstructionsWithXcm` => `DenyNestedLocalInstructions`\ - \ and `DenyNestedLocalInstructionsThenTry`, more details at [here](https://github.com/paritytech/polkadot-sdk/pull/7200#discussion_r1933637941)\n\ - - [x] Clean-up and docs\n- [x] Merge https://github.com/paritytech/polkadot-sdk/pull/7169\ - \ or rebase this branch on the top of `yrong:fix-for-deny-then-try`\n- [ ] Set\ - \ for the runtimes where we use `DenyThenTry`\n- [ ] Schedule sec.audit" -crates: -- name: staging-xcm-builder - bump: patch -- name: staging-xcm-executor - bump: patch From b75c40faea4e9e5564e7d582520de60db7a2bfc3 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 3 Feb 2025 14:58:59 +0000 Subject: [PATCH 22/48] Update from raymondkfcheung running command 'prdoc --audience runtime_user --bump minor' --- prdoc/pr_7200.prdoc | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 prdoc/pr_7200.prdoc diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc new file mode 100644 index 000000000000..10cee3c27c3b --- /dev/null +++ b/prdoc/pr_7200.prdoc @@ -0,0 +1,61 @@ +title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be + executed on the local chain' +doc: +- audience: Runtime User + description: "Resolves (partially): https://github.com/paritytech/polkadot-sdk/issues/7148\n\ + Depends on: https://github.com/paritytech/polkadot-sdk/pull/7169\n\n# Description\n\ + \nFor context and additional information, please refer to [_Problem 2 - Barrier\ + \ vs nested XCM validation_](https://github.com/paritytech/polkadot-sdk/issues/7148).\n\ + \n# TODO\n- [x] Evaluate PoC, more details at #7351:\n - **DenyNestedXcmInstructions**:\ + \ Keep it as it is and be explicit:\n 1. Name the Deny barriers for the top\ + \ level.\n 2. Name the Deny barrier for nested with `DenyInstructionsWithXcm`.\n\ + \ - **DenyNestedLocalInstructionsThenTry**: Alternatively, hard-code those three\ + \ instructions in `DenyThenTry`, so we wouldn\u2019t need `DenyInstructionsWithXcm`.\ + \ However, this approach wouldn\u2019t be as general.\n - **DenyNestedLocalInstructions**:\ + \ Another possibility is to check `DenyInstructionsWithXcm::Inner` for the actual\ + \ `message`, so we don\u2019t need duplication for top-level and nested (not sure,\ + \ maybe be explicit is good thing) - see _Problem2 - example_. Instead of this:\n\ + \ ```\n DenyThenTry<\n (\n //\ + \ Deny for top level XCM program\n DenyReserveTransferToRelayChain,\n\ + \ // Dedicated barrier for nested XCM programs\n\ + \ DenyInstructionsWithXcmFor<\n \ + \ // Repeat all Deny filters here\n \ + \ DenyReserveTransferToRelayChain,\n \ + \ >\n ),\n ```\n we could just use:\n\ + \ ```\n DenyThenTry<\n (\n //\ + \ Dedicated barrier for XCM programs\n DenyInstructionsWithXcmFor<\n\ + \ // Add all `Deny` filters here\n\ + \ DenyReserveTransferToRelayChain,\n\ + \ ...\n \ + \ >\n ),\n ```\n - [POC Evaluation](https://github.com/paritytech/polkadot-sdk/pull/7200/files#r19374329150)\n\ + - [x] Consider better name `DenyInstructionsWithXcm` => `DenyNestedLocalInstructions`\ + \ and `DenyNestedLocalInstructionsThenTry`, more details at [here](https://github.com/paritytech/polkadot-sdk/pull/7200#discussion_r1933637941)\n\ + - [x] Clean-up and docs\n- [x] Merge https://github.com/paritytech/polkadot-sdk/pull/7169\ + \ or rebase this branch on the top of `yrong:fix-for-deny-then-try`\n- [x] Set\ + \ for the runtimes where we use `DenyThenTry` => `DenyNestedLocalInstructionsThenTry`\n\ + - [ ] Schedule sec.audit" +crates: +- name: staging-xcm-builder + bump: minor +- name: staging-xcm-executor + bump: minor +- name: asset-hub-rococo-runtime + bump: minor +- name: asset-hub-westend-runtime + bump: minor +- name: bridge-hub-rococo-runtime + bump: minor +- name: bridge-hub-westend-runtime + bump: minor +- name: collectives-westend-runtime + bump: minor +- name: contracts-rococo-runtime + bump: minor +- name: coretime-rococo-runtime + bump: minor +- name: coretime-westend-runtime + bump: minor +- name: people-rococo-runtime + bump: minor +- name: people-westend-runtime + bump: minor From a663b4302f9af30f56056e9ab64f0d8f4b84df57 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 15:05:56 +0000 Subject: [PATCH 23/48] Update prdoc --- prdoc/pr_7200.prdoc | 41 +++++++---------------------------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 10cee3c27c3b..cb2ec82f7905 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -1,39 +1,12 @@ -title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be - executed on the local chain' +title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be executed on the local chain' doc: - audience: Runtime User - description: "Resolves (partially): https://github.com/paritytech/polkadot-sdk/issues/7148\n\ - Depends on: https://github.com/paritytech/polkadot-sdk/pull/7169\n\n# Description\n\ - \nFor context and additional information, please refer to [_Problem 2 - Barrier\ - \ vs nested XCM validation_](https://github.com/paritytech/polkadot-sdk/issues/7148).\n\ - \n# TODO\n- [x] Evaluate PoC, more details at #7351:\n - **DenyNestedXcmInstructions**:\ - \ Keep it as it is and be explicit:\n 1. Name the Deny barriers for the top\ - \ level.\n 2. Name the Deny barrier for nested with `DenyInstructionsWithXcm`.\n\ - \ - **DenyNestedLocalInstructionsThenTry**: Alternatively, hard-code those three\ - \ instructions in `DenyThenTry`, so we wouldn\u2019t need `DenyInstructionsWithXcm`.\ - \ However, this approach wouldn\u2019t be as general.\n - **DenyNestedLocalInstructions**:\ - \ Another possibility is to check `DenyInstructionsWithXcm::Inner` for the actual\ - \ `message`, so we don\u2019t need duplication for top-level and nested (not sure,\ - \ maybe be explicit is good thing) - see _Problem2 - example_. Instead of this:\n\ - \ ```\n DenyThenTry<\n (\n //\ - \ Deny for top level XCM program\n DenyReserveTransferToRelayChain,\n\ - \ // Dedicated barrier for nested XCM programs\n\ - \ DenyInstructionsWithXcmFor<\n \ - \ // Repeat all Deny filters here\n \ - \ DenyReserveTransferToRelayChain,\n \ - \ >\n ),\n ```\n we could just use:\n\ - \ ```\n DenyThenTry<\n (\n //\ - \ Dedicated barrier for XCM programs\n DenyInstructionsWithXcmFor<\n\ - \ // Add all `Deny` filters here\n\ - \ DenyReserveTransferToRelayChain,\n\ - \ ...\n \ - \ >\n ),\n ```\n - [POC Evaluation](https://github.com/paritytech/polkadot-sdk/pull/7200/files#r19374329150)\n\ - - [x] Consider better name `DenyInstructionsWithXcm` => `DenyNestedLocalInstructions`\ - \ and `DenyNestedLocalInstructionsThenTry`, more details at [here](https://github.com/paritytech/polkadot-sdk/pull/7200#discussion_r1933637941)\n\ - - [x] Clean-up and docs\n- [x] Merge https://github.com/paritytech/polkadot-sdk/pull/7169\ - \ or rebase this branch on the top of `yrong:fix-for-deny-then-try`\n- [x] Set\ - \ for the runtimes where we use `DenyThenTry` => `DenyNestedLocalInstructionsThenTry`\n\ - - [ ] Schedule sec.audit" + description: |- + This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local instructions. It + introduces two new barriers - `DenyNestedLocalInstructions` and `DenyNestedLocalInstructionsThenTry` - to provide + more refined control over instruction denial. The main change is the replacement of `DenyThenTry` with + `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested local instructions by applying allow + condition after denial. crates: - name: staging-xcm-builder bump: minor From 19e0b6f963ce08f6b76a2a7120d4eef8e18c16a8 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 15:17:14 +0000 Subject: [PATCH 24/48] Update prdoc --- prdoc/pr_7200.prdoc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index cb2ec82f7905..6b5bcc3f9c46 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -2,11 +2,11 @@ title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to b doc: - audience: Runtime User description: |- - This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local instructions. It - introduces two new barriers - `DenyNestedLocalInstructions` and `DenyNestedLocalInstructionsThenTry` - to provide - more refined control over instruction denial. The main change is the replacement of `DenyThenTry` with - `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested local instructions by applying allow - condition after denial. + This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local instructions. It + introduces two new barriers - `DenyNestedLocalInstructions` and `DenyNestedLocalInstructionsThenTry` - to provide + more refined control over instruction denial. The main change is the replacement of `DenyThenTry` with + `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested local instructions by applying allow + condition after denial. crates: - name: staging-xcm-builder bump: minor From 9f4048d1d9af1307e4f8d214fe00f225c32baf6e Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 16:00:51 +0000 Subject: [PATCH 25/48] Update prdoc --- prdoc/pr_7200.prdoc | 63 ++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 6b5bcc3f9c46..f95cecb85d44 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -1,34 +1,33 @@ -title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be executed on the local chain' +title: "XCM: Deny barrier checks for nested XCMs with specific instructions to be executed on the local chain" doc: -- audience: Runtime User - description: |- - This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local instructions. It - introduces two new barriers - `DenyNestedLocalInstructions` and `DenyNestedLocalInstructionsThenTry` - to provide - more refined control over instruction denial. The main change is the replacement of `DenyThenTry` with - `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested local instructions by applying allow - condition after denial. + - audience: Runtime User + description: "This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local \ + instructions. It introduces two new barriers - `DenyNestedLocalInstructions` and \ + `DenyNestedLocalInstructionsThenTry` - to provide more refined control over instruction denial. The main change is \ + the replacement of `DenyThenTry` with `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested \ + local instructions by applying allow condition after denial." crates: -- name: staging-xcm-builder - bump: minor -- name: staging-xcm-executor - bump: minor -- name: asset-hub-rococo-runtime - bump: minor -- name: asset-hub-westend-runtime - bump: minor -- name: bridge-hub-rococo-runtime - bump: minor -- name: bridge-hub-westend-runtime - bump: minor -- name: collectives-westend-runtime - bump: minor -- name: contracts-rococo-runtime - bump: minor -- name: coretime-rococo-runtime - bump: minor -- name: coretime-westend-runtime - bump: minor -- name: people-rococo-runtime - bump: minor -- name: people-westend-runtime - bump: minor + - name: staging-xcm-builder + bump: minor + - name: staging-xcm-executor + bump: minor + - name: asset-hub-rococo-runtime + bump: minor + - name: asset-hub-westend-runtime + bump: minor + - name: bridge-hub-rococo-runtime + bump: minor + - name: bridge-hub-westend-runtime + bump: minor + - name: collectives-westend-runtime + bump: minor + - name: contracts-rococo-runtime + bump: minor + - name: coretime-rococo-runtime + bump: minor + - name: coretime-westend-runtime + bump: minor + - name: people-rococo-runtime + bump: minor + - name: people-westend-runtime + bump: minor From 211c0d87470c1d6894fa89b1c50de0dd37f5e610 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 16:08:55 +0000 Subject: [PATCH 26/48] Update prdoc --- prdoc/pr_7200.prdoc | 55 +++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index f95cecb85d44..83691409d39f 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -1,33 +1,34 @@ -title: "XCM: Deny barrier checks for nested XCMs with specific instructions to be executed on the local chain" +title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to be + executed on the local chain' doc: - - audience: Runtime User - description: "This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local \ +- audience: Runtime User + description: "This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local \ instructions. It introduces two new barriers - `DenyNestedLocalInstructions` and \ `DenyNestedLocalInstructionsThenTry` - to provide more refined control over instruction denial. The main change is \ the replacement of `DenyThenTry` with `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested \ local instructions by applying allow condition after denial." crates: - - name: staging-xcm-builder - bump: minor - - name: staging-xcm-executor - bump: minor - - name: asset-hub-rococo-runtime - bump: minor - - name: asset-hub-westend-runtime - bump: minor - - name: bridge-hub-rococo-runtime - bump: minor - - name: bridge-hub-westend-runtime - bump: minor - - name: collectives-westend-runtime - bump: minor - - name: contracts-rococo-runtime - bump: minor - - name: coretime-rococo-runtime - bump: minor - - name: coretime-westend-runtime - bump: minor - - name: people-rococo-runtime - bump: minor - - name: people-westend-runtime - bump: minor +- name: staging-xcm-builder + bump: minor +- name: staging-xcm-executor + bump: minor +- name: asset-hub-rococo-runtime + bump: minor +- name: asset-hub-westend-runtime + bump: minor +- name: bridge-hub-rococo-runtime + bump: minor +- name: bridge-hub-westend-runtime + bump: minor +- name: collectives-westend-runtime + bump: minor +- name: contracts-rococo-runtime + bump: minor +- name: coretime-rococo-runtime + bump: minor +- name: coretime-westend-runtime + bump: minor +- name: people-rococo-runtime + bump: minor +- name: people-westend-runtime + bump: minor From f4662dae9736b83b84bda28eff111105bb8db33d Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 3 Feb 2025 16:25:18 +0000 Subject: [PATCH 27/48] Update from raymondkfcheung running command 'prdoc --audience runtime_user --bump minor --force true' --- prdoc/pr_7200.prdoc | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 83691409d39f..873d8321fd20 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -2,11 +2,44 @@ title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to b executed on the local chain' doc: - audience: Runtime User - description: "This PR addresses partially #7148 (Problem 2) and ensures the proper checking of nested local \ - instructions. It introduces two new barriers - `DenyNestedLocalInstructions` and \ - `DenyNestedLocalInstructionsThenTry` - to provide more refined control over instruction denial. The main change is \ - the replacement of `DenyThenTry` with `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested \ - local instructions by applying allow condition after denial." + description: "Resolves (partially): https://github.com/paritytech/polkadot-sdk/issues/7148\n\ + Depends on: https://github.com/paritytech/polkadot-sdk/pull/7169\n\n# Description\n\ + \nThis PR addresses partially #7148 (Problem 2) and ensures the proper checking\ + \ of nested local instructions. It introduces two new barriers - `DenyNestedLocalInstructions`\ + \ and `DenyNestedLocalInstructionsThenTry` - to provide more refined control over\ + \ instruction denial. The main change is the replacement of `DenyThenTry` with\ + \ `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested\ + \ local instructions by applying allow condition after denial.\n\nFor context\ + \ and additional information, please refer to [_Problem 2 - Barrier vs nested\ + \ XCM validation_](https://github.com/paritytech/polkadot-sdk/issues/7148).\n\n\ + # TODO\n- [x] Evaluate PoC, more details at #7351:\n - **DenyNestedXcmInstructions**:\ + \ Keep it as it is and be explicit:\n 1. Name the Deny barriers for the top\ + \ level.\n 2. Name the Deny barrier for nested with `DenyInstructionsWithXcm`.\n\ + \ - **DenyNestedLocalInstructionsThenTry**: Alternatively, hard-code those three\ + \ instructions in `DenyThenTry`, so we wouldn\u2019t need `DenyInstructionsWithXcm`.\ + \ However, this approach wouldn\u2019t be as general.\n - **DenyNestedLocalInstructions**:\ + \ Another possibility is to check `DenyInstructionsWithXcm::Inner` for the actual\ + \ `message`, so we don\u2019t need duplication for top-level and nested (not sure,\ + \ maybe be explicit is good thing) - see _Problem2 - example_. Instead of this:\n\ + \ ```\n DenyThenTry<\n (\n //\ + \ Deny for top level XCM program\n DenyReserveTransferToRelayChain,\n\ + \ // Dedicated barrier for nested XCM programs\n\ + \ DenyInstructionsWithXcmFor<\n \ + \ // Repeat all Deny filters here\n \ + \ DenyReserveTransferToRelayChain,\n \ + \ >\n ),\n ```\n we could just use:\n\ + \ ```\n DenyThenTry<\n (\n //\ + \ Dedicated barrier for XCM programs\n DenyInstructionsWithXcmFor<\n\ + \ // Add all `Deny` filters here\n\ + \ DenyReserveTransferToRelayChain,\n\ + \ ...\n \ + \ >\n ),\n ```\n - [POC Evaluation](https://github.com/paritytech/polkadot-sdk/pull/7200/files#r19374329150)\n\ + - [x] Consider better name `DenyInstructionsWithXcm` => `DenyNestedLocalInstructions`\ + \ and `DenyNestedLocalInstructionsThenTry`, more details at [here](https://github.com/paritytech/polkadot-sdk/pull/7200#discussion_r1933637941)\n\ + - [x] Clean-up and docs\n- [x] Merge https://github.com/paritytech/polkadot-sdk/pull/7169\ + \ or rebase this branch on the top of `yrong:fix-for-deny-then-try`\n- [x] Set\ + \ for the runtimes where we use `DenyThenTry` => `DenyNestedLocalInstructionsThenTry`\n\ + - [ ] Schedule sec.audit" crates: - name: staging-xcm-builder bump: minor From 1951b399c5642859e6fd2614d76e81b0b5010b40 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 16:32:47 +0000 Subject: [PATCH 28/48] Update prdoc --- prdoc/pr_7200.prdoc | 36 ++---------------------------------- 1 file changed, 2 insertions(+), 34 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 873d8321fd20..026516aa2bc1 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -2,44 +2,12 @@ title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to b executed on the local chain' doc: - audience: Runtime User - description: "Resolves (partially): https://github.com/paritytech/polkadot-sdk/issues/7148\n\ - Depends on: https://github.com/paritytech/polkadot-sdk/pull/7169\n\n# Description\n\ - \nThis PR addresses partially #7148 (Problem 2) and ensures the proper checking\ + description: "This PR addresses partially #7148 (Problem 2) and ensures the proper checking\ \ of nested local instructions. It introduces two new barriers - `DenyNestedLocalInstructions`\ \ and `DenyNestedLocalInstructionsThenTry` - to provide more refined control over\ \ instruction denial. The main change is the replacement of `DenyThenTry` with\ \ `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested\ - \ local instructions by applying allow condition after denial.\n\nFor context\ - \ and additional information, please refer to [_Problem 2 - Barrier vs nested\ - \ XCM validation_](https://github.com/paritytech/polkadot-sdk/issues/7148).\n\n\ - # TODO\n- [x] Evaluate PoC, more details at #7351:\n - **DenyNestedXcmInstructions**:\ - \ Keep it as it is and be explicit:\n 1. Name the Deny barriers for the top\ - \ level.\n 2. Name the Deny barrier for nested with `DenyInstructionsWithXcm`.\n\ - \ - **DenyNestedLocalInstructionsThenTry**: Alternatively, hard-code those three\ - \ instructions in `DenyThenTry`, so we wouldn\u2019t need `DenyInstructionsWithXcm`.\ - \ However, this approach wouldn\u2019t be as general.\n - **DenyNestedLocalInstructions**:\ - \ Another possibility is to check `DenyInstructionsWithXcm::Inner` for the actual\ - \ `message`, so we don\u2019t need duplication for top-level and nested (not sure,\ - \ maybe be explicit is good thing) - see _Problem2 - example_. Instead of this:\n\ - \ ```\n DenyThenTry<\n (\n //\ - \ Deny for top level XCM program\n DenyReserveTransferToRelayChain,\n\ - \ // Dedicated barrier for nested XCM programs\n\ - \ DenyInstructionsWithXcmFor<\n \ - \ // Repeat all Deny filters here\n \ - \ DenyReserveTransferToRelayChain,\n \ - \ >\n ),\n ```\n we could just use:\n\ - \ ```\n DenyThenTry<\n (\n //\ - \ Dedicated barrier for XCM programs\n DenyInstructionsWithXcmFor<\n\ - \ // Add all `Deny` filters here\n\ - \ DenyReserveTransferToRelayChain,\n\ - \ ...\n \ - \ >\n ),\n ```\n - [POC Evaluation](https://github.com/paritytech/polkadot-sdk/pull/7200/files#r19374329150)\n\ - - [x] Consider better name `DenyInstructionsWithXcm` => `DenyNestedLocalInstructions`\ - \ and `DenyNestedLocalInstructionsThenTry`, more details at [here](https://github.com/paritytech/polkadot-sdk/pull/7200#discussion_r1933637941)\n\ - - [x] Clean-up and docs\n- [x] Merge https://github.com/paritytech/polkadot-sdk/pull/7169\ - \ or rebase this branch on the top of `yrong:fix-for-deny-then-try`\n- [x] Set\ - \ for the runtimes where we use `DenyThenTry` => `DenyNestedLocalInstructionsThenTry`\n\ - - [ ] Schedule sec.audit" + \ local instructions by applying allow condition after denial." crates: - name: staging-xcm-builder bump: minor From c69d6c51a7925dfe4afda2548181ef23e48e8fe1 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Mon, 3 Feb 2025 16:58:43 +0000 Subject: [PATCH 29/48] Update prdoc --- prdoc/pr_7200.prdoc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 026516aa2bc1..092d92929f6f 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -2,12 +2,14 @@ title: 'XCM: Deny barrier checks for nested XCMs with specific instructions to b executed on the local chain' doc: - audience: Runtime User - description: "This PR addresses partially #7148 (Problem 2) and ensures the proper checking\ - \ of nested local instructions. It introduces two new barriers - `DenyNestedLocalInstructions`\ - \ and `DenyNestedLocalInstructionsThenTry` - to provide more refined control over\ - \ instruction denial. The main change is the replacement of `DenyThenTry` with\ - \ `DenyNestedLocalInstructionsThenTry` which handles both top-level and nested\ - \ local instructions by applying allow condition after denial." + description: |- + This PR addresses partially #7148 (Problem 2) and ensures the proper checking + of nested local instructions. It introduces two new barriers - + `DenyNestedLocalInstructions` and `DenyNestedLocalInstructionsThenTry` - to + provide more refined control over instruction denial. The main change is the + replacement of `DenyThenTry` with `DenyNestedLocalInstructionsThenTry` which + handles both top-level and nested local instructions by applying allow + condition after denial. crates: - name: staging-xcm-builder bump: minor From fb948e453d3676cfaa35643956e945ac616c2e8f Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:22:21 +0000 Subject: [PATCH 30/48] Refactor DenyNestedXcmInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 3ce06ea7c1b6..63db0c6b57f9 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -524,14 +524,21 @@ environmental::environmental!(recursion_count: u8); /// This struct is not meant to be used directly outside of this module. It ensures that restricted /// instructions within `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` are blocked /// **recursively**, preventing unintended execution of nested XCMs. -struct DenyNestedXcmInstructions(PhantomData); +struct DenyNestedXcmInstructions(PhantomData, PhantomData) +where + Deny: DenyExecution, + Inner: DenyExecution; -impl DenyNestedXcmInstructions { +impl DenyNestedXcmInstructions +where + Deny: DenyExecution, + Inner: DenyExecution, +{ // Recursively applies the deny filter to a nested XCM. /// /// This function ensures that restricted instructions are blocked at any depth within the XCM. /// It maintains a **recursion counter** to prevent stack overflows due to a deep nesting. - fn deny_recursively( + fn deny_recursively( origin: &Location, nested_xcm: &mut Xcm, max_weight: Weight, @@ -619,7 +626,7 @@ impl DenyExecution for DenyNestedLocalInstructions SetAppendix(nested_xcm) | SetErrorHandler(nested_xcm) | ExecuteWithOrigin { xcm: nested_xcm, .. } => - DenyNestedXcmInstructions::::deny_recursively::( + DenyNestedXcmInstructions::::deny_recursively::( origin, nested_xcm, max_weight, properties, )?, _ => Ok(ControlFlow::Continue(())), From f84b50ac65fbc1dfe7f1e1004f40a83b9097b49b Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:40:28 +0000 Subject: [PATCH 31/48] Add more tests --- .../xcm/xcm-builder/src/tests/barriers.rs | 46 ++++++++++++------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 2a92314731af..db1804210709 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -780,9 +780,7 @@ fn deny_nested_local_instructions_then_try_works() { // Should deny recursively before allow type BarrierDenyClearOrigin = DenyWrapper>; - assert_deny_nested_instructions_with_xcm::(Err( - ProcessMessageError::Unsupported, - )); + assert_deny_nested_instructions_with_xcm::(); } #[test] @@ -845,7 +843,7 @@ fn deny_reserve_transfer_to_relaychain_should_work() { #[test] fn deny_nested_local_instructions_works() { type Barrier = DenyNestedLocalInstructions; - assert_deny_nested_instructions_with_xcm::(Err(ProcessMessageError::Unsupported)); + assert_deny_nested_instructions_with_xcm::(); } #[test] @@ -863,23 +861,25 @@ fn compare_deny_filters() { top_level_result: Result<(), ProcessMessageError>, nested_result: Result<(), ProcessMessageError>, ) { + let origin = Here.into_location(); + let max_weight = Weight::zero(); + let mut properties = props(Weight::zero()); + + // Validate Top-Level let mut xcm = Xcm::>( vec![DepositReserveAsset { assets: Wild(All), dest: Location::parent(), - xcm: vec![].into(), + xcm: Xcm(vec![ClearOrigin]), }] .into(), ); - let mut nested_xcm = - Xcm::>(vec![SetErrorHandler(xcm.clone().into())].into()); - let origin = Here.into_location(); - let max_weight = Weight::zero(); - let mut properties = props(Weight::zero()); - - let result = Barrier::deny_execution(&origin, xcm.inner_mut(), max_weight, &mut properties); + let result = + Barrier::deny_execution(&origin, xcm.clone().inner_mut(), max_weight, &mut properties); assert_eq!(top_level_result, result); + // Validate Nested + let mut nested_xcm = Xcm::>(vec![SetErrorHandler(xcm.into())].into()); let result = Barrier::deny_execution(&origin, nested_xcm.inner_mut(), max_weight, &mut properties); assert_eq!(nested_result, result); @@ -901,9 +901,7 @@ fn compare_deny_filters() { ); } -fn assert_deny_nested_instructions_with_xcm( - top_level_expected_result: Result<(), ProcessMessageError>, -) { +fn assert_deny_nested_instructions_with_xcm() { // closure for (xcm, origin) testing with `Barrier` which denies `ClearOrigin` // instruction let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { @@ -920,14 +918,28 @@ fn assert_deny_nested_instructions_with_xcm( // ok assert_deny_execution(vec![ClearTransactStatus], Location::parent(), Ok(())); - // ok/err top-level contains `ClearOrigin` - assert_deny_execution(vec![ClearOrigin], Location::parent(), top_level_expected_result); + // invalid top-level contains `ClearOrigin` + assert_deny_execution( + vec![ClearOrigin], + Location::parent(), + Err(ProcessMessageError::Unsupported), + ); // ok - SetAppendix with XCM without ClearOrigin assert_deny_execution( vec![SetAppendix(Xcm(vec![ClearTransactStatus]))], Location::parent(), Ok(()), ); + // ok - DepositReserveAsset with XCM contains ClearOrigin + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Here.into(), + xcm: Xcm(vec![ClearOrigin]), + }], + Location::parent(), + Ok(()), + ); // invalid - empty XCM assert_deny_execution(vec![], Location::parent(), Err(ProcessMessageError::BadFormat)); From 936e56e61cbf821719317e2a4fafe78a175eeeb8 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Tue, 4 Feb 2025 10:00:57 +0000 Subject: [PATCH 32/48] Remove `mut` --- polkadot/xcm/xcm-builder/src/tests/barriers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index db1804210709..84e672c8f68a 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -866,7 +866,7 @@ fn compare_deny_filters() { let mut properties = props(Weight::zero()); // Validate Top-Level - let mut xcm = Xcm::>( + let xcm = Xcm::>( vec![DepositReserveAsset { assets: Wild(All), dest: Location::parent(), From 2994fe2f2a462143b8d1241165e96387afb0afd8 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Tue, 4 Feb 2025 10:39:46 +0000 Subject: [PATCH 33/48] Update comment for DenyNestedXcmInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 63db0c6b57f9..a5ed00420646 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -522,16 +522,16 @@ environmental::environmental!(recursion_count: u8); // instructions. /// /// This struct is not meant to be used directly outside of this module. It ensures that restricted -/// instructions within `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` are blocked -/// **recursively**, preventing unintended execution of nested XCMs. -struct DenyNestedXcmInstructions(PhantomData, PhantomData) +/// instructions within the `Outer` filter are blocked **recursively**, preventing unintended +/// execution of nested XCMs. +struct DenyNestedXcmInstructions(PhantomData, PhantomData) where - Deny: DenyExecution, + Outer: DenyExecution, Inner: DenyExecution; -impl DenyNestedXcmInstructions +impl DenyNestedXcmInstructions where - Deny: DenyExecution, + Outer: DenyExecution, Inner: DenyExecution, { // Recursively applies the deny filter to a nested XCM. @@ -580,7 +580,7 @@ where } // Recursively check the nested XCM instructions. - Deny::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) + Outer::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) })?; Ok(Ok(ControlFlow::Continue(()))) From f2f8449703c0fd1a41b7b8762191686a28f8fd84 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 14:26:35 +0000 Subject: [PATCH 34/48] Add comment for RECURSION_LIMIT --- polkadot/xcm/xcm-executor/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index c1059a8b445f..17ab8e147239 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -60,6 +60,10 @@ pub struct FeesMode { pub jit_withdraw: bool, } +/// The maximum recursion depth allowed when executing nested XCM instructions. +/// +/// Exceeding this limit results in `XcmError::ExceedsStackLimit` or +/// `ProcessMessageError::StackLimitReached`. pub const RECURSION_LIMIT: u8 = 10; environmental::environmental!(recursion_count: u8); From 3f5188942d4074ef8621f4df65ab2c21eab815a3 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 14:35:40 +0000 Subject: [PATCH 35/48] Rename to DenyLocalInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 25 +++++++++---------- polkadot/xcm/xcm-builder/src/lib.rs | 2 +- .../xcm/xcm-builder/src/tests/barriers.rs | 14 +++++------ 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index a5ed00420646..d0b14d60067e 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -466,15 +466,15 @@ where /// Denies execution if the XCM contains any of the denied **local** instructions, even if nested /// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`. /// -/// This check is applied **recursively** using `DenyNestedLocalInstructions`, ensuring that -/// instructions do not execute on the local chain at any depth. +/// This check is applied **recursively** using `DenyLocalInstructions`, ensuring that instructions +/// do not execute on the local chain at any depth. /// /// If the message passes the deny filters, it is then evaluated against the allow condition. /// /// Note: This applies only to locally executed instructions. Remote parts of the XCM are expected /// to be validated by the destination chain's barrier. pub type DenyNestedLocalInstructionsThenTry = - DenyThenTry, Allow>; + DenyThenTry, Allow>; // See issue pub struct DenyReserveTransferToRelayChain; @@ -596,9 +596,9 @@ where /// /// Note: Ensures that restricted instructions do not execute on the local chain, enforcing stricter /// execution policies, while allowing remote chains to enforce their own rules. -pub struct DenyNestedLocalInstructions(PhantomData); +pub struct DenyLocalInstructions(PhantomData); -impl DenyExecution for DenyNestedLocalInstructions { +impl DenyExecution for DenyLocalInstructions { /// Denies execution of restricted local nested XCM instructions. /// /// This checks for `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instruction @@ -610,14 +610,13 @@ impl DenyExecution for DenyNestedLocalInstructions properties: &mut Properties, ) -> Result<(), ProcessMessageError> { // First, check if the top-level message should be denied. - Inner::deny_execution(origin, instructions, max_weight, properties) - .inspect_err(|e| { - log::warn!( - target: "xcm::barriers", - "DenyNestedLocalInstructions::Inner denied execution, origin: {:?}, instructions: {:?}, error: {:?}", - origin, instructions, e - ); - })?; + Inner::deny_execution(origin, instructions, max_weight, properties).inspect_err(|e| { + log::warn!( + target: "xcm::barriers", + "DenyLocalInstructions::Inner denied execution, origin: {:?}, instructions: {:?}, error: {:?}", + origin, instructions, e + ); + })?; // If the top-level check passes, check nested instructions recursively. instructions.matcher().match_next_inst_while( diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index 729550f8db4b..c7a3fcdae7a6 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,7 +42,7 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyNestedLocalInstructions, DenyNestedLocalInstructionsThenTry, + AllowUnpaidExecutionFrom, DenyLocalInstructions, DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 84e672c8f68a..96d43472e01c 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -780,7 +780,7 @@ fn deny_nested_local_instructions_then_try_works() { // Should deny recursively before allow type BarrierDenyClearOrigin = DenyWrapper>; - assert_deny_nested_instructions_with_xcm::(); + assert_deny_instructions_recursively::(); } #[test] @@ -841,9 +841,9 @@ fn deny_reserve_transfer_to_relaychain_should_work() { } #[test] -fn deny_nested_local_instructions_works() { - type Barrier = DenyNestedLocalInstructions; - assert_deny_nested_instructions_with_xcm::(); +fn deny_local_instructions_works() { + type Barrier = DenyLocalInstructions; + assert_deny_instructions_recursively::(); } #[test] @@ -894,14 +894,14 @@ fn compare_deny_filters() { Err(ProcessMessageError::Unsupported), ); - // `DenyNestedLocalInstructions`: Top-level=Deny, Nested=Deny, TryAllow=No - assert_deny_barrier::>( + // `DenyLocalInstructions`: Top-level=Deny, Nested=Deny, TryAllow=No + assert_deny_barrier::>( Err(ProcessMessageError::Unsupported), Err(ProcessMessageError::Unsupported), ); } -fn assert_deny_nested_instructions_with_xcm() { +fn assert_deny_instructions_recursively() { // closure for (xcm, origin) testing with `Barrier` which denies `ClearOrigin` // instruction let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { From 7329db87dbafb325017283ce097372995d664dcb Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 14:40:41 +0000 Subject: [PATCH 36/48] Move dummy filters to tests/barriers.rs --- .../xcm/xcm-builder/src/tests/barriers.rs | 48 ++++++++++++++++++ polkadot/xcm/xcm-builder/src/tests/mock.rs | 50 +------------------ 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 96d43472e01c..04d4f97eca59 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -14,10 +14,58 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . +use std::marker::PhantomData; use xcm_executor::traits::Properties; use super::*; +// Dummy Barriers +// Dummy filter to allow all +struct AllowAll; +impl ShouldExecute for AllowAll { + fn should_execute( + _: &Location, + _: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Ok(()) + } +} + +// Dummy filter which denies `ClearOrigin` +struct DenyClearOrigin; +impl DenyExecution for DenyClearOrigin { + fn deny_execution( + _: &Location, + instructions: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } +} + +// Dummy filter which wraps `DenyExecution` on `ShouldExecution` +struct DenyWrapper(PhantomData); +impl DenyExecution for DenyWrapper { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Deny::should_execute(origin, instructions, max_weight, properties) + } +} + fn props(weight_credit: Weight) -> Properties { Properties { weight_credit, message_id: None } } diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index ed3046229d14..ace0d974a738 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -41,13 +41,12 @@ pub use frame_support::{ sp_runtime::{traits::Dispatchable, DispatchError, DispatchErrorWithPostInfo}, traits::{Contains, Get, IsInVec}, }; -use std::marker::PhantomData; pub use xcm::latest::{prelude::*, QueryId, Weight}; pub use xcm_executor::{ traits::{ AssetExchange, AssetLock, CheckSuspension, ConvertOrigin, DenyExecution, Enact, ExportXcm, FeeManager, FeeReason, LockError, OnResponse, Properties, QueryHandler, - QueryResponseStatus, ShouldExecute, TransactAsset, + QueryResponseStatus, TransactAsset, }, AssetsInHolding, Config, }; @@ -775,50 +774,3 @@ pub fn fungible_multi_asset(location: Location, amount: u128) -> Asset { pub fn fake_message_hash(message: &Xcm) -> XcmHash { message.using_encoded(sp_io::hashing::blake2_256) } - -// Dummy Barriers -// Dummy filter to allow all -pub struct AllowAll; -impl ShouldExecute for AllowAll { - fn should_execute( - _: &Location, - _: &mut [Instruction], - _: Weight, - _: &mut Properties, - ) -> Result<(), ProcessMessageError> { - Ok(()) - } -} - -// Dummy filter which denies `ClearOrigin` -pub struct DenyClearOrigin; -impl DenyExecution for DenyClearOrigin { - fn deny_execution( - _: &Location, - instructions: &mut [Instruction], - _: Weight, - _: &mut Properties, - ) -> Result<(), ProcessMessageError> { - instructions.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ClearOrigin => Err(ProcessMessageError::Unsupported), - _ => Ok(ControlFlow::Continue(())), - }, - )?; - Ok(()) - } -} - -// Dummy filter which wraps `DenyExecution` on `ShouldExecution` -pub struct DenyWrapper(PhantomData); -impl DenyExecution for DenyWrapper { - fn deny_execution( - origin: &Location, - instructions: &mut [Instruction], - max_weight: Weight, - properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - Deny::should_execute(origin, instructions, max_weight, properties) - } -} From c518c591cbe60c610f56be91ae18c0479407417a Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:02:41 +0000 Subject: [PATCH 37/48] Clean up tests --- .../xcm/xcm-builder/src/tests/barriers.rs | 218 +++++++++--------- polkadot/xcm/xcm-builder/src/tests/mock.rs | 3 +- 2 files changed, 110 insertions(+), 111 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/tests/barriers.rs b/polkadot/xcm/xcm-builder/src/tests/barriers.rs index 04d4f97eca59..3367745caefb 100644 --- a/polkadot/xcm/xcm-builder/src/tests/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/tests/barriers.rs @@ -19,53 +19,6 @@ use xcm_executor::traits::Properties; use super::*; -// Dummy Barriers -// Dummy filter to allow all -struct AllowAll; -impl ShouldExecute for AllowAll { - fn should_execute( - _: &Location, - _: &mut [Instruction], - _: Weight, - _: &mut Properties, - ) -> Result<(), ProcessMessageError> { - Ok(()) - } -} - -// Dummy filter which denies `ClearOrigin` -struct DenyClearOrigin; -impl DenyExecution for DenyClearOrigin { - fn deny_execution( - _: &Location, - instructions: &mut [Instruction], - _: Weight, - _: &mut Properties, - ) -> Result<(), ProcessMessageError> { - instructions.matcher().match_next_inst_while( - |_| true, - |inst| match inst { - ClearOrigin => Err(ProcessMessageError::Unsupported), - _ => Ok(ControlFlow::Continue(())), - }, - )?; - Ok(()) - } -} - -// Dummy filter which wraps `DenyExecution` on `ShouldExecution` -struct DenyWrapper(PhantomData); -impl DenyExecution for DenyWrapper { - fn deny_execution( - origin: &Location, - instructions: &mut [Instruction], - max_weight: Weight, - properties: &mut Properties, - ) -> Result<(), ProcessMessageError> { - Deny::should_execute(origin, instructions, max_weight, properties) - } -} - fn props(weight_credit: Weight) -> Properties { Properties { weight_credit, message_id: None } } @@ -757,8 +710,112 @@ fn deny_then_try_works() { } #[test] -fn deny_nested_local_instructions_then_try_works() { - type Barrier = DenyNestedLocalInstructionsThenTry; +fn deny_reserve_transfer_to_relaychain_should_work() { + let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { + assert_eq!( + DenyReserveTransferToRelayChain::deny_execution( + &origin, + &mut xcm, + Weight::from_parts(10, 10), + &mut props(Weight::zero()), + ), + expected_result + ); + }; + // deny DepositReserveAsset to RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // deny InitiateReserveWithdraw to RelayChain + assert_deny_execution( + vec![InitiateReserveWithdraw { + assets: Wild(All), + reserve: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // deny TransferReserveAsset to RelayChain + assert_deny_execution( + vec![TransferReserveAsset { + assets: vec![].into(), + dest: Location::parent(), + xcm: vec![].into(), + }], + Here.into_location(), + Err(ProcessMessageError::Unsupported), + ); + // accept DepositReserveAsset to destination other than RelayChain + assert_deny_execution( + vec![DepositReserveAsset { + assets: Wild(All), + dest: Here.into_location(), + xcm: vec![].into(), + }], + Here.into_location(), + Ok(()), + ); + // others instructions should pass + assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); +} + +// Dummy Barriers +// Dummy filter to allow all +struct AllowAll; +impl ShouldExecute for AllowAll { + fn should_execute( + _: &Location, + _: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Ok(()) + } +} + +// Dummy filter which denies `ClearOrigin` +struct DenyClearOrigin; +impl DenyExecution for DenyClearOrigin { + fn deny_execution( + _: &Location, + instructions: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } +} + +// Dummy filter which wraps `DenyExecution` on `ShouldExecution` +struct DenyWrapper(PhantomData); +impl DenyExecution for DenyWrapper { + fn deny_execution( + origin: &Location, + instructions: &mut [Instruction], + max_weight: Weight, + properties: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Deny::should_execute(origin, instructions, max_weight, properties) + } +} + +#[test] +fn deny_local_instructions_then_try_works() { + type Barrier = DenyThenTry, AllowAll>; let xcm = Xcm::>(vec![DepositReserveAsset { assets: Wild(All), dest: Location::parent(), @@ -827,67 +884,10 @@ fn deny_nested_local_instructions_then_try_works() { // Should deny recursively before allow type BarrierDenyClearOrigin = - DenyWrapper>; + DenyWrapper, AllowAll>>; assert_deny_instructions_recursively::(); } -#[test] -fn deny_reserve_transfer_to_relaychain_should_work() { - let assert_deny_execution = |mut xcm: Vec>, origin, expected_result| { - assert_eq!( - DenyReserveTransferToRelayChain::deny_execution( - &origin, - &mut xcm, - Weight::from_parts(10, 10), - &mut props(Weight::zero()), - ), - expected_result - ); - }; - // deny DepositReserveAsset to RelayChain - assert_deny_execution( - vec![DepositReserveAsset { - assets: Wild(All), - dest: Location::parent(), - xcm: vec![].into(), - }], - Here.into_location(), - Err(ProcessMessageError::Unsupported), - ); - // deny InitiateReserveWithdraw to RelayChain - assert_deny_execution( - vec![InitiateReserveWithdraw { - assets: Wild(All), - reserve: Location::parent(), - xcm: vec![].into(), - }], - Here.into_location(), - Err(ProcessMessageError::Unsupported), - ); - // deny TransferReserveAsset to RelayChain - assert_deny_execution( - vec![TransferReserveAsset { - assets: vec![].into(), - dest: Location::parent(), - xcm: vec![].into(), - }], - Here.into_location(), - Err(ProcessMessageError::Unsupported), - ); - // accept DepositReserveAsset to destination other than RelayChain - assert_deny_execution( - vec![DepositReserveAsset { - assets: Wild(All), - dest: Here.into_location(), - xcm: vec![].into(), - }], - Here.into_location(), - Ok(()), - ); - // others instructions should pass - assert_deny_execution(vec![ClearOrigin], Here.into_location(), Ok(())); -} - #[test] fn deny_local_instructions_works() { type Barrier = DenyLocalInstructions; @@ -936,8 +936,8 @@ fn compare_deny_filters() { // `DenyThenTry`: Top-level=Deny, Nested=Allow, TryAllow=Yes assert_barrier::>(Err(ProcessMessageError::Unsupported), Ok(())); - // `DenyNestedLocalInstructionsThenTry`: Top-level=Deny, Nested=Deny, TryAllow=Yes - assert_barrier::>( + // `DenyThenTry>`: Top-level=Deny, Nested=Deny, TryAllow=Yes + assert_barrier::, AllowAll>>( Err(ProcessMessageError::Unsupported), Err(ProcessMessageError::Unsupported), ); diff --git a/polkadot/xcm/xcm-builder/src/tests/mock.rs b/polkadot/xcm/xcm-builder/src/tests/mock.rs index ace0d974a738..127888104a4a 100644 --- a/polkadot/xcm/xcm-builder/src/tests/mock.rs +++ b/polkadot/xcm/xcm-builder/src/tests/mock.rs @@ -18,7 +18,6 @@ use crate::{ barriers::{AllowSubscriptionsFrom, RespectSuspension, TrailingSetTopicAsId}, - matcher::{CreateMatcher, MatchXcm}, test_utils::*, EnsureDecodableXcm, }; @@ -34,7 +33,7 @@ pub use core::{ fmt::Debug, ops::ControlFlow, }; -use frame_support::traits::{ContainsPair, Everything, ProcessMessageError}; +use frame_support::traits::{ContainsPair, Everything}; pub use frame_support::{ dispatch::{DispatchInfo, DispatchResultWithPostInfo, GetDispatchInfo, PostDispatchInfo}, ensure, parameter_types, From c5ca5e7d37131dc4ded99505deda528610507a24 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:11:31 +0000 Subject: [PATCH 38/48] Update prdoc --- prdoc/pr_7200.prdoc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 092d92929f6f..597bc38b7574 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -4,12 +4,11 @@ doc: - audience: Runtime User description: |- This PR addresses partially #7148 (Problem 2) and ensures the proper checking - of nested local instructions. It introduces two new barriers - - `DenyNestedLocalInstructions` and `DenyNestedLocalInstructionsThenTry` - to - provide more refined control over instruction denial. The main change is the - replacement of `DenyThenTry` with `DenyNestedLocalInstructionsThenTry` which - handles both top-level and nested local instructions by applying allow - condition after denial. + of nested local instructions. It introduces a new barrier - + `DenyLocalInstructions` - to provide more refined control over instruction + denial. The main change is the replacement of `DenyThenTry` with + `DenyThenTry, Allow>` which handles both top-level + and nested local instructions by applying allow condition after denial. crates: - name: staging-xcm-builder bump: minor From 24dd3e3909fb11648b869b05fde34a9e59467157 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 16:54:12 +0000 Subject: [PATCH 39/48] Remove DenyNestedLocalInstructionsThenTry --- .../assets/asset-hub-rococo/src/xcm_config.rs | 26 +++++++++---------- .../asset-hub-westend/src/xcm_config.rs | 26 +++++++++---------- .../bridge-hub-rococo/src/xcm_config.rs | 6 ++--- .../bridge-hub-westend/src/xcm_config.rs | 6 ++--- .../collectives-westend/src/xcm_config.rs | 6 ++--- .../contracts-rococo/src/xcm_config.rs | 6 ++--- .../coretime-rococo/src/xcm_config.rs | 6 ++--- .../coretime-westend/src/xcm_config.rs | 6 ++--- .../people/people-rococo/src/xcm_config.rs | 6 ++--- .../people/people-westend/src/xcm_config.rs | 6 ++--- .../runtime/src/configs/xcm_config.rs | 11 ++++---- 11 files changed, 56 insertions(+), 55 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs index 8b59455e664b..33e9f3f45663 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs @@ -51,17 +51,17 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH, WESTEND_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, - AllowTopLevelPaidExecutionFrom, DenyNestedLocalInstructionsThenTry, - DenyReserveTransferToRelayChain, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, - FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, - GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint, - MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, NonFungiblesAdapter, - ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SingleAssetExchangeAdapter, SovereignPaidRemoteExporter, - SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit, - TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, - WithLatestLocationConverter, WithUniqueTopic, XcmFeeManagerFromComponents, + AllowTopLevelPaidExecutionFrom, DenyLocalInstructions, DenyReserveTransferToRelayChain, + DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, + FungibleAdapter, FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, + IsConcrete, LocalMint, MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, + NonFungiblesAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, + SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + SignedAccountId32AsNative, SignedToAccountId32, SingleAssetExchangeAdapter, + SovereignPaidRemoteExporter, SovereignSignedViaLocation, StartsWith, + StartsWithExplicitGlobalConsensus, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, + WeightInfoBounds, WithComputedOrigin, WithLatestLocationConverter, WithUniqueTopic, + XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -268,8 +268,8 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( TakeWeightCredit, // Expected responses are OK. diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs index 1ecb9a3afddc..1a6fc7a05555 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/xcm_config.rs @@ -48,17 +48,17 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH, WESTEND_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, - AllowTopLevelPaidExecutionFrom, DenyNestedLocalInstructionsThenTry, - DenyReserveTransferToRelayChain, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, - FrameTransactionalProcessor, FungibleAdapter, FungiblesAdapter, - GlobalConsensusParachainConvertsFor, HashedDescription, IsConcrete, LocalMint, - MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, NonFungiblesAdapter, - ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SingleAssetExchangeAdapter, SovereignPaidRemoteExporter, - SovereignSignedViaLocation, StartsWith, StartsWithExplicitGlobalConsensus, TakeWeightCredit, - TrailingSetTopicAsId, UsingComponents, WeightInfoBounds, WithComputedOrigin, - WithLatestLocationConverter, WithUniqueTopic, XcmFeeManagerFromComponents, + AllowTopLevelPaidExecutionFrom, DenyLocalInstructions, DenyReserveTransferToRelayChain, + DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, + FungibleAdapter, FungiblesAdapter, GlobalConsensusParachainConvertsFor, HashedDescription, + IsConcrete, LocalMint, MatchedConvertedConcreteId, NetworkExportTableItem, NoChecking, + NonFungiblesAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, + SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, + SignedAccountId32AsNative, SignedToAccountId32, SingleAssetExchangeAdapter, + SovereignPaidRemoteExporter, SovereignSignedViaLocation, StartsWith, + StartsWithExplicitGlobalConsensus, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, + WeightInfoBounds, WithComputedOrigin, WithLatestLocationConverter, WithUniqueTopic, + XcmFeeManagerFromComponents, }; use xcm_executor::XcmExecutor; @@ -280,8 +280,8 @@ impl Contains for AmbassadorEntities { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( TakeWeightCredit, // Expected responses are OK. diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs index 695851611896..df443b359acc 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/xcm_config.rs @@ -44,7 +44,7 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HandleFee, HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -130,8 +130,8 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs index f12c93c352ab..c32e16093908 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs @@ -43,7 +43,7 @@ use xcm::latest::{prelude::*, WESTEND_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HandleFee, HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -128,8 +128,8 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs index e01121feef2e..e5f2bc7ead10 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/xcm_config.rs @@ -37,7 +37,7 @@ use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, LocatableAssetId, OriginToPluralityVoice, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, @@ -145,8 +145,8 @@ impl Contains for LocalPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs index a258cc8950a5..3439aff263d5 100644 --- a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/xcm_config.rs @@ -40,7 +40,7 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, NativeAsset, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -133,8 +133,8 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( TakeWeightCredit, // Expected responses are OK. diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs index 56baf944ce8f..386fa43bbb9d 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/xcm_config.rs @@ -41,7 +41,7 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, NonFungibleAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -144,8 +144,8 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs index 98c06f0218f9..01e3110d2a19 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/xcm_config.rs @@ -42,7 +42,7 @@ use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, NonFungibleAdapter, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -153,8 +153,8 @@ impl Contains for FellowsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs b/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs index e1872fac211e..be1667ea5e07 100644 --- a/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/people/people-rococo/src/xcm_config.rs @@ -38,7 +38,7 @@ use xcm::latest::{prelude::*, ROCOCO_GENESIS_HASH}; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, DescribeTerminus, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -152,8 +152,8 @@ impl Contains for ParentOrParentsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs b/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs index 57fe1c6abf1e..1213e9fb624d 100644 --- a/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/people/people-westend/src/xcm_config.rs @@ -39,7 +39,7 @@ use xcm_builder::{ AccountId32Aliases, AliasChildLocation, AliasOriginRootUsingFilter, AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, DescribeAllTerminal, + DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, DescribeAllTerminal, DescribeFamily, DescribeTerminus, EnsureXcmOrigin, FrameTransactionalProcessor, FungibleAdapter, HashedDescription, IsConcrete, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SendXcmFeeToAccount, SiblingParachainAsNative, SiblingParachainConvertsVia, @@ -161,8 +161,8 @@ impl Contains for FellowsPlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( // Allow local users to buy weight credit. TakeWeightCredit, diff --git a/templates/parachain/runtime/src/configs/xcm_config.rs b/templates/parachain/runtime/src/configs/xcm_config.rs index 482a767be8df..aa209d7d29d9 100644 --- a/templates/parachain/runtime/src/configs/xcm_config.rs +++ b/templates/parachain/runtime/src/configs/xcm_config.rs @@ -16,12 +16,13 @@ use frame_system::EnsureRoot; use pallet_xcm::XcmPassthrough; use polkadot_parachain_primitives::primitives::Sibling; use polkadot_runtime_common::impls::ToAuthor; +use polkadot_sdk::staging_xcm_builder::{DenyLocalInstructions, DenyThenTry}; use xcm::latest::prelude::*; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowTopLevelPaidExecutionFrom, - DenyNestedLocalInstructionsThenTry, DenyReserveTransferToRelayChain, EnsureXcmOrigin, - FixedWeightBounds, FrameTransactionalProcessor, FungibleAdapter, IsConcrete, NativeAsset, - ParentIsPreset, RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia, + DenyReserveTransferToRelayChain, EnsureXcmOrigin, FixedWeightBounds, + FrameTransactionalProcessor, FungibleAdapter, IsConcrete, NativeAsset, ParentIsPreset, + RelayChainAsNative, SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, SovereignSignedViaLocation, TakeWeightCredit, TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic, }; @@ -98,8 +99,8 @@ impl Contains for ParentOrParentsExecutivePlurality { } pub type Barrier = TrailingSetTopicAsId< - DenyNestedLocalInstructionsThenTry< - DenyReserveTransferToRelayChain, + DenyThenTry< + DenyLocalInstructions, ( TakeWeightCredit, WithComputedOrigin< From 4c4b9ec5000263f9861ecabddd54f10fa37a24ce Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 17:11:22 +0000 Subject: [PATCH 40/48] Rename to xcm --- polkadot/xcm/xcm-builder/src/barriers.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index d0b14d60067e..1f702c77b003 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -540,17 +540,17 @@ where /// It maintains a **recursion counter** to prevent stack overflows due to a deep nesting. fn deny_recursively( origin: &Location, - nested_xcm: &mut Xcm, + xcm: &mut Xcm, max_weight: Weight, properties: &mut Properties, ) -> Result, ProcessMessageError>, ProcessMessageError> { // Apply the `Inner` deny filter to the nested XCM. - let _ = Inner::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) + let _ = Inner::deny_execution(origin, xcm.inner_mut(), max_weight, properties) .inspect_err(|e| { log::warn!( target: "xcm::barriers", - "Execution denied by Inner filter, origin: {:?}, nested_xcm: {:?}, error: {:?}", - origin, nested_xcm, e + "Execution denied by Inner filter, origin: {:?}, xcm: {:?}, error: {:?}", + origin, xcm, e ); })?; @@ -561,7 +561,7 @@ where log::error!( target: "xcm::barriers", "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", - origin, nested_xcm + origin, xcm ); return Err(ProcessMessageError::StackLimitReached); @@ -580,7 +580,7 @@ where } // Recursively check the nested XCM instructions. - Outer::deny_execution(origin, nested_xcm.inner_mut(), max_weight, properties) + Outer::deny_execution(origin, xcm.inner_mut(), max_weight, properties) })?; Ok(Ok(ControlFlow::Continue(()))) From 63b0df19037066783759cc5e105b75942c3490ac Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 17:13:29 +0000 Subject: [PATCH 41/48] Remove DenyNestedLocalInstructionsThenTry --- polkadot/xcm/xcm-builder/src/barriers.rs | 13 ------------- polkadot/xcm/xcm-builder/src/lib.rs | 7 +++---- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 1f702c77b003..55e0289e40fa 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -463,19 +463,6 @@ where } } -/// Denies execution if the XCM contains any of the denied **local** instructions, even if nested -/// within `SetAppendix(xcm)`, `SetErrorHandler(xcm)`, or `ExecuteWithOrigin { xcm, ... }`. -/// -/// This check is applied **recursively** using `DenyLocalInstructions`, ensuring that instructions -/// do not execute on the local chain at any depth. -/// -/// If the message passes the deny filters, it is then evaluated against the allow condition. -/// -/// Note: This applies only to locally executed instructions. Remote parts of the XCM are expected -/// to be validated by the destination chain's barrier. -pub type DenyNestedLocalInstructionsThenTry = - DenyThenTry, Allow>; - // See issue pub struct DenyReserveTransferToRelayChain; impl DenyExecution for DenyReserveTransferToRelayChain { diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index c7a3fcdae7a6..cdc6b667b7ab 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -42,10 +42,9 @@ mod barriers; pub use barriers::{ AllowExplicitUnpaidExecutionFrom, AllowHrmpNotificationsFromRelayChain, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, - AllowUnpaidExecutionFrom, DenyLocalInstructions, DenyNestedLocalInstructionsThenTry, - DenyReserveTransferToRelayChain, DenyThenTry, IsChildSystemParachain, IsParentsOnly, - IsSiblingSystemParachain, RespectSuspension, TakeWeightCredit, TrailingSetTopicAsId, - WithComputedOrigin, + AllowUnpaidExecutionFrom, DenyLocalInstructions, DenyReserveTransferToRelayChain, DenyThenTry, + IsChildSystemParachain, IsParentsOnly, IsSiblingSystemParachain, RespectSuspension, + TakeWeightCredit, TrailingSetTopicAsId, WithComputedOrigin, }; mod controller; From 632f2b63475cda47dc4d9d01cf5cd7dc14c9a6f0 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 17:20:09 +0000 Subject: [PATCH 42/48] Remove DenyNestedXcmInstructions --- polkadot/xcm/xcm-builder/src/barriers.rs | 43 ++++++++---------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 55e0289e40fa..bffb25d25da7 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -505,22 +505,18 @@ impl DenyExecution for DenyReserveTransferToRelayChain { environmental::environmental!(recursion_count: u8); -// A private helper struct that applies the `Inner` filter to nested XCMs within certain -// instructions. +/// Denies execution if the XCM contains restricted **local** instructions, first checking at the +/// top-level and then **recursively**. /// -/// This struct is not meant to be used directly outside of this module. It ensures that restricted -/// instructions within the `Outer` filter are blocked **recursively**, preventing unintended -/// execution of nested XCMs. -struct DenyNestedXcmInstructions(PhantomData, PhantomData) -where - Outer: DenyExecution, - Inner: DenyExecution; +/// This barrier only applies to **locally executed** XCM instructions (`SetAppendix`, +/// `SetErrorHandler`, and `ExecuteWithOrigin`). Remote parts of the XCM are expected to be +/// validated by receiving chain's barrier. +/// +/// Note: Ensures that restricted instructions do not execute on the local chain, enforcing stricter +/// execution policies, while allowing remote chains to enforce their own rules. +pub struct DenyLocalInstructions(PhantomData); -impl DenyNestedXcmInstructions -where - Outer: DenyExecution, - Inner: DenyExecution, -{ +impl DenyLocalInstructions { // Recursively applies the deny filter to a nested XCM. /// /// This function ensures that restricted instructions are blocked at any depth within the XCM. @@ -556,8 +552,8 @@ where *count = count.saturating_add(1); Ok(()) }) - // Fallback safety in case of an unexpected failure. - .unwrap_or(Ok(()))?; + // Fallback safety in case of an unexpected failure. + .unwrap_or(Ok(()))?; // Ensure that we always decrement the counter after processing. sp_core::defer! { @@ -567,24 +563,13 @@ where } // Recursively check the nested XCM instructions. - Outer::deny_execution(origin, xcm.inner_mut(), max_weight, properties) + Self::deny_execution(origin, xcm.inner_mut(), max_weight, properties) })?; Ok(Ok(ControlFlow::Continue(()))) } } -/// Denies execution if the XCM contains restricted **local** instructions, first checking at the -/// top-level and then **recursively** using `DenyNestedXcmInstructions`. -/// -/// This barrier only applies to **locally executed** XCM instructions (`SetAppendix`, -/// `SetErrorHandler`, and `ExecuteWithOrigin`). Remote parts of the XCM are expected to be -/// validated by receiving chain's barrier. -/// -/// Note: Ensures that restricted instructions do not execute on the local chain, enforcing stricter -/// execution policies, while allowing remote chains to enforce their own rules. -pub struct DenyLocalInstructions(PhantomData); - impl DenyExecution for DenyLocalInstructions { /// Denies execution of restricted local nested XCM instructions. /// @@ -612,7 +597,7 @@ impl DenyExecution for DenyLocalInstructions { SetAppendix(nested_xcm) | SetErrorHandler(nested_xcm) | ExecuteWithOrigin { xcm: nested_xcm, .. } => - DenyNestedXcmInstructions::::deny_recursively::( + Self::deny_recursively::( origin, nested_xcm, max_weight, properties, )?, _ => Ok(ControlFlow::Continue(())), From 66af360f493507036d925c3a8668de560ca4c813 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 17:21:01 +0000 Subject: [PATCH 43/48] Apply fmt --- polkadot/xcm/xcm-builder/src/barriers.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index bffb25d25da7..b508336d012b 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -552,8 +552,8 @@ impl DenyLocalInstructions { *count = count.saturating_add(1); Ok(()) }) - // Fallback safety in case of an unexpected failure. - .unwrap_or(Ok(()))?; + // Fallback safety in case of an unexpected failure. + .unwrap_or(Ok(()))?; // Ensure that we always decrement the counter after processing. sp_core::defer! { @@ -596,10 +596,9 @@ impl DenyExecution for DenyLocalInstructions { |inst| match inst { SetAppendix(nested_xcm) | SetErrorHandler(nested_xcm) | - ExecuteWithOrigin { xcm: nested_xcm, .. } => - Self::deny_recursively::( - origin, nested_xcm, max_weight, properties, - )?, + ExecuteWithOrigin { xcm: nested_xcm, .. } => Self::deny_recursively::( + origin, nested_xcm, max_weight, properties, + )?, _ => Ok(ControlFlow::Continue(())), }, )?; From 758e8718f177982c0f19bdee48e1b9125f364f26 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Wed, 5 Feb 2025 17:28:33 +0000 Subject: [PATCH 44/48] Simplify the recursion --- polkadot/xcm/xcm-builder/src/barriers.rs | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index b508336d012b..7ec27b4097d6 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -516,7 +516,7 @@ environmental::environmental!(recursion_count: u8); /// execution policies, while allowing remote chains to enforce their own rules. pub struct DenyLocalInstructions(PhantomData); -impl DenyLocalInstructions { +impl DenyLocalInstructions { // Recursively applies the deny filter to a nested XCM. /// /// This function ensures that restricted instructions are blocked at any depth within the XCM. @@ -527,24 +527,14 @@ impl DenyLocalInstructions { max_weight: Weight, properties: &mut Properties, ) -> Result, ProcessMessageError>, ProcessMessageError> { - // Apply the `Inner` deny filter to the nested XCM. - let _ = Inner::deny_execution(origin, xcm.inner_mut(), max_weight, properties) - .inspect_err(|e| { - log::warn!( - target: "xcm::barriers", - "Execution denied by Inner filter, origin: {:?}, xcm: {:?}, error: {:?}", - origin, xcm, e - ); - })?; - // Initialise the recursion count the first time we enter recursive execution. let _ = recursion_count::using_once(&mut 1, || { recursion_count::with(|count| { if *count > xcm_executor::RECURSION_LIMIT { log::error!( target: "xcm::barriers", - "Recursion limit exceeded, origin: {:?}, nested_xcm: {:?}, count: {count}", - origin, xcm + "Recursion limit exceeded, origin: {:?}, xcm: {:?}, max_weight: {:?}, properties: {:?}, count: {count}", + origin, xcm, max_weight, properties ); return Err(ProcessMessageError::StackLimitReached); @@ -585,8 +575,8 @@ impl DenyExecution for DenyLocalInstructions { Inner::deny_execution(origin, instructions, max_weight, properties).inspect_err(|e| { log::warn!( target: "xcm::barriers", - "DenyLocalInstructions::Inner denied execution, origin: {:?}, instructions: {:?}, error: {:?}", - origin, instructions, e + "DenyLocalInstructions::Inner denied execution, origin: {:?}, instructions: {:?}, max_weight: {:?}, properties: {:?}, error: {:?}", + origin, instructions, max_weight, properties, e ); })?; From 5a817a8106a5a60f63b02652669f05e8a358c17d Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 5 Feb 2025 22:01:14 +0100 Subject: [PATCH 45/48] Update polkadot/xcm/xcm-builder/src/barriers.rs --- polkadot/xcm/xcm-builder/src/barriers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/xcm/xcm-builder/src/barriers.rs b/polkadot/xcm/xcm-builder/src/barriers.rs index 7ec27b4097d6..cf59cd1e1b8b 100644 --- a/polkadot/xcm/xcm-builder/src/barriers.rs +++ b/polkadot/xcm/xcm-builder/src/barriers.rs @@ -516,7 +516,7 @@ environmental::environmental!(recursion_count: u8); /// execution policies, while allowing remote chains to enforce their own rules. pub struct DenyLocalInstructions(PhantomData); -impl DenyLocalInstructions { +impl DenyLocalInstructions { // Recursively applies the deny filter to a nested XCM. /// /// This function ensures that restricted instructions are blocked at any depth within the XCM. From ede0da7b2939485757f678d89a4f19b539a32cd6 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Wed, 5 Feb 2025 22:52:47 +0100 Subject: [PATCH 46/48] Update prdoc/pr_7200.prdoc --- prdoc/pr_7200.prdoc | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 597bc38b7574..5c8b228d2ded 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -11,26 +11,26 @@ doc: and nested local instructions by applying allow condition after denial. crates: - name: staging-xcm-builder - bump: minor + bump: patch - name: staging-xcm-executor - bump: minor + bump: patch - name: asset-hub-rococo-runtime - bump: minor + bump: patch - name: asset-hub-westend-runtime - bump: minor + bump: patch - name: bridge-hub-rococo-runtime - bump: minor + bump: patch - name: bridge-hub-westend-runtime - bump: minor + bump: patch - name: collectives-westend-runtime - bump: minor + bump: patch - name: contracts-rococo-runtime - bump: minor + bump: patch - name: coretime-rococo-runtime - bump: minor + bump: patch - name: coretime-westend-runtime - bump: minor + bump: patch - name: people-rococo-runtime - bump: minor + bump: patch - name: people-westend-runtime - bump: minor + bump: patch From b625ecc94ae92edf49d100b0364ae554447f1eb9 Mon Sep 17 00:00:00 2001 From: Branislav Kontur Date: Thu, 6 Feb 2025 11:20:27 +0100 Subject: [PATCH 47/48] Update prdoc/pr_7200.prdoc --- prdoc/pr_7200.prdoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prdoc/pr_7200.prdoc b/prdoc/pr_7200.prdoc index 5c8b228d2ded..7e0d3806c6af 100644 --- a/prdoc/pr_7200.prdoc +++ b/prdoc/pr_7200.prdoc @@ -11,9 +11,9 @@ doc: and nested local instructions by applying allow condition after denial. crates: - name: staging-xcm-builder - bump: patch + bump: minor - name: staging-xcm-executor - bump: patch + bump: minor - name: asset-hub-rococo-runtime bump: patch - name: asset-hub-westend-runtime From 30d76885f696f4aa11bac6b84a420367642e5090 Mon Sep 17 00:00:00 2001 From: Raymond Cheung <178801527+raymondkfcheung@users.noreply.github.com> Date: Thu, 6 Feb 2025 12:15:40 +0000 Subject: [PATCH 48/48] Add scenario test --- polkadot/xcm/xcm-builder/tests/scenarios.rs | 98 +++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/polkadot/xcm/xcm-builder/tests/scenarios.rs b/polkadot/xcm/xcm-builder/tests/scenarios.rs index 99c14f5bba1b..78b961e53c6d 100644 --- a/polkadot/xcm/xcm-builder/tests/scenarios.rs +++ b/polkadot/xcm/xcm-builder/tests/scenarios.rs @@ -322,3 +322,101 @@ fn reserve_based_transfer_works() { ); }); } + +/// Scenario: +/// A recursive XCM that triggers itself via `SetAppendix`. +/// The execution should fail due to inner filter. +#[test] +fn recursive_xcm_execution_fail() { + use crate::mock::*; + use frame_support::traits::{Everything, Nothing, ProcessMessageError}; + use staging_xcm_builder::*; + use std::ops::ControlFlow; + use xcm::opaque::latest::prelude::*; + use xcm_executor::traits::{DenyExecution, Properties, ShouldExecute}; + + // Dummy filter to allow all + struct AllowAll; + impl ShouldExecute for AllowAll { + fn should_execute( + _: &Location, + _: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + Ok(()) + } + } + + // Dummy filter which denies `ClearOrigin` + struct DenyClearOrigin; + impl DenyExecution for DenyClearOrigin { + fn deny_execution( + _: &Location, + instructions: &mut [Instruction], + _: Weight, + _: &mut Properties, + ) -> Result<(), ProcessMessageError> { + instructions.matcher().match_next_inst_while( + |_| true, + |inst| match inst { + ClearOrigin => Err(ProcessMessageError::Unsupported), + _ => Ok(ControlFlow::Continue(())), + }, + )?; + Ok(()) + } + } + + struct XcmTestConfig; + impl xcm_executor::Config for XcmTestConfig { + type RuntimeCall = RuntimeCall; + type XcmSender = TestXcmRouter; + type AssetTransactor = LocalAssetTransactor; + type OriginConverter = (); + type IsReserve = (); + type IsTeleporter = TrustedTeleporters; + type UniversalLocation = UniversalLocation; + type Barrier = DenyThenTry, AllowAll>; + type Weigher = FixedWeightBounds; + type Trader = FixedRateOfFungible; + type ResponseHandler = XcmPallet; + type AssetTrap = XcmPallet; + type AssetLocker = (); + type AssetExchanger = (); + type AssetClaims = XcmPallet; + type SubscriptionService = XcmPallet; + type PalletInstancesInfo = AllPalletsWithSystem; + type MaxAssetsIntoHolding = MaxAssetsIntoHolding; + type FeeManager = (); + type MessageExporter = (); + type UniversalAliases = Nothing; + type CallDispatcher = RuntimeCall; + type SafeCallFilter = Everything; + type Aliasers = Nothing; + type TransactionalProcessor = (); + type HrmpNewChannelOpenRequestHandler = (); + type HrmpChannelAcceptedHandler = (); + type HrmpChannelClosingHandler = (); + type XcmRecorder = XcmPallet; + } + + let para_acc: AccountId = ParaId::from(PARA_ID).into_account_truncating(); + let balances = vec![(ALICE, INITIAL_BALANCE), (para_acc.clone(), INITIAL_BALANCE)]; + let origin = Parachain(PARA_ID); + let message = Xcm(vec![SetAppendix(Xcm(vec![SetAppendix(Xcm(vec![ClearOrigin]))]))]); + let mut hash = fake_message_hash(&message); + let weight = BaseXcmWeight::get() * 3; + + kusama_like_with_balances(balances).execute_with(|| { + let outcome = XcmExecutor::::prepare_and_execute( + origin, + message, + &mut hash, + weight, + Weight::zero(), + ); + + assert_eq!(outcome, Outcome::Error { error: XcmError::Barrier }); + }); +}