Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XCM: Deny barrier checks for nested XCMs with specific instructions to be executed on the local chain #7200

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Jan 16, 2025

Resolves (partially): #7148
Depends on: #7169

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.

For context and additional information, please refer to Problem 2 - Barrier vs nested XCM validation.

TODO

  • Evaluate PoC, more details at poc(XCM): Deny Nested XCM Barriers #7351:
    • DenyNestedXcmInstructions: Keep it as it is and be explicit:
      1. Name the Deny barriers for the top level.
      2. Name the Deny barrier for nested with DenyInstructionsWithXcm.
    • DenyNestedLocalInstructionsThenTry: Alternatively, hard-code those three instructions in DenyThenTry, so we wouldn’t need DenyInstructionsWithXcm. However, this approach wouldn’t be as general.
    • DenyNestedLocalInstructions: Another possibility is to check DenyInstructionsWithXcm::Inner for the actual message, so we don’t need duplication for top-level and nested (not sure, maybe be explicit is good thing) - see Problem2 - example. Instead of this:
    DenyThenTry<
                 (
                                // Deny for top level XCM program 
                                DenyReserveTransferToRelayChain,
                                // Dedicated barrier for nested XCM programs
                                DenyInstructionsWithXcmFor<
                                                 // Repeat all Deny filters here 
                                                 DenyReserveTransferToRelayChain,
                                 >
                 ),
    
    we could just use:
    DenyThenTry<
                 (
                                // Dedicated barrier for XCM programs
                                DenyInstructionsWithXcmFor<
                                                 // Add all `Deny` filters here 
                                                 DenyReserveTransferToRelayChain,
                                                 ...
                                 >
                 ),
    
  • POC Evaluation
  • Consider better name DenyInstructionsWithXcm => DenyNestedLocalInstructions and DenyNestedLocalInstructionsThenTry, more details at here
  • Clean-up and docs
  • Merge xcm: fix for DenyThenTry Barrier #7169 or rebase this branch on the top of yrong:fix-for-deny-then-try
  • Set for the runtimes where we use DenyThenTry => DenyNestedLocalInstructionsThenTry
  • Schedule sec.audit

@bkontur bkontur added T6-XCM This PR/Issue is related to XCM. C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. labels Jan 16, 2025
@bkontur bkontur force-pushed the bko-deny-nested-xcm-barrier branch from 2c6325f to 9c80770 Compare January 22, 2025 09:31
@bkontur bkontur added the C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. label Jan 22, 2025
@bkontur bkontur assigned bkontur and unassigned bkontur Jan 22, 2025
@raymondkfcheung raymondkfcheung self-assigned this Jan 23, 2025
/// 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<Inner>(PhantomData<Inner>);
Copy link

@raymondkfcheung raymondkfcheung Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the naming, maybe we can use DenyNestedXcmInstructions, which may be better to state clearly with nested XCMs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think DenyNestedXcmInstructions is a better name, but we still need to ensure it's explicitly used only for the following instructions: SetAppendix, SetErrorHandler, and ExecuteWithOrigin—which are meant to be executed on the local chain.

This is important because there are other instructions with nested XCM, such as DepositReserveAsset { xcm: Xcm<()>, ... } and InitiateReserveWithdraw { xcm: Xcm<()>, ... }, where the inner xcm is executed on a remote chain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! If we want to check all nested XCM instructions recursively (including those executed remotely), we could rename it to DenyNestedXcmInstructions and adjust the logic accordingly.

However, since this implementation currently applies only to instructions executed on the local chain (SetAppendix, SetErrorHandler, and ExecuteWithOrigin), we could instead introduce two separate types for clarity:

  • DenyNestedLocalInstructions: Covers only local execution cases (current behavior).
  • DenyNestedRemoteInstructions: Specifically targets instructions like DepositReserveAsset and InitiateReserveWithdraw, ensuring remote execution filtering.

Would this separation make sense, or do you think a more unified approach is preferable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DenyNestedRemoteInstructions: Specifically targets instructions like DepositReserveAsset and InitiateReserveWithdraw, ensuring remote execution filtering.

Generally, a local chain should not assume rules about other chains' rules/barriers.

The design is that each chain only enforces its own local rules.

It is the job of the offchain component (wallet/ui/app) building the XCM to validate (e.g. through XCM dry-run APIs) that the XCM they build will pass barriers on all involved chains.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've defined a new NestedXcmType when performing the deny execution. This categorises nested XCM calls as either Local or Remote, ensuring we handle them separately.

Right now, both types go through deny_recursively, but I can implement two different denial strategies (DenyNestedLocalInstructions and DenyNestedRemoteInstructions) if needed. Let me know if you see any difference in behavior, or if you'd prefer keeping a single DenyNestedXcmInstructions that applies to both.

Or shall I use origin: &Location to determine whether it's Local or Remote? If it's Remote, then I could deny all location-based instructions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not care to validate "remote" parts - only what is locally executed - the remote part will be validated by the barrier on the remote chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondkfcheung yes, agree with Adrian, other words, I think we need just DenyNestedLocalInstructions (for those: SetAppendix, SetErrorHandler, ExecuteWithOrigin). The barrier should handle just incoming XCM and check whether to execute or not on the local chain.

DenyNestedRemoteInstructions could be a potentially "good" addition, but we don't have that scenario and probably don't want to support that either. For example, for filtering/denying outgoing XCMs, we could/would add a custom XCM router - some wrapper implementation of SendXcm trait with filtering capabilities.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, we can focus to implement, DenyNestedLocalInstructions. If it's required, then we can adjust for remote instructions. I'll adjust the PR accordingly.

@raymondkfcheung
Copy link

/cmd fmt

@raymondkfcheung
Copy link

/cmd fmt

raymondkfcheung and others added 2 commits January 31, 2025 13:41
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` | ✅ | ❌ | ✅ | Blocks
top-level instructions only. |
| `RecursiveDenyThenTry` | ✅ | ✅ |
✅ | Blocks both top-level and nested instructions. |
| `DenyInstructionsWithXcm` | ❌ | ✅ | ❌ | Focuses
on nested instructions, requires additional checks for top-level denial.
|
| `DenyFirstInstructionsWithXcm` | ✅ |
✅ | ❌ | Prioritises top-level denial before recursive
checks. |

---------

Co-authored-by: ron <yrong1997@gmail.com>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: command-bot <>
Co-authored-by: Clara van Staden <claravanstaden64@gmail.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Comment on lines 887 to 906
// `DenyThenTry`: Top-level=Deny, Nested=Allow, TryAllow=Yes
assert_barrier::<DenyThenTry<Denies, AllowAll>>(Err(ProcessMessageError::Unsupported), Ok(()));

// `RecursiveDenyThenTry`: Top-level=Deny, Nested=Deny, TryAllow=Yes
assert_barrier::<RecursiveDenyThenTry<Denies, AllowAll>>(
Err(ProcessMessageError::Unsupported),
Err(ProcessMessageError::Unsupported),
);

// `DenyInstructionsWithXcm`: Top-level=Allow, Nested=Deny, TryAllow=No
assert_deny_barrier::<DenyInstructionsWithXcm<Denies>>(
Ok(()),
Err(ProcessMessageError::Unsupported),
);

// `DenyFirstInstructionsWithXcm`: Top-level=Deny, Nested=Deny, TryAllow=No
assert_deny_barrier::<DenyFirstInstructionsWithXcm<Denies>>(
Err(ProcessMessageError::Unsupported),
Err(ProcessMessageError::Unsupported),
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POC Evaluation

  • Redefine DenyThenTry as RecursiveDenyThenTry: blocks both top-level and nested instructions, before trying to allow execution.
  • Create a new DenyFirstInstructionsWithXcm: blocks both top-level and nested instructions.
  • Establish a helper DenyInstructionsWithXcm, handles nested denies.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated:

PoC Top-Level Denial Nested Denial Try Allow Remark
DenyThenTry Blocks only top-level instructions, allowing nested ones unless denied elsewhere.
DenyNestedLocalInstructionsThenTry Blocks both top-level and nested local instructions, then applies the allow condition.
DenyNestedXcmInstructions Internal helper; focuses only on nested instructions, requiring additional checks for top-level denial.
DenyNestedLocalInstructions Blocks both top-level and nested local instructions, without attempting to allow execution.

@raymondkfcheung
Copy link

/cmd prdoc --audience runtime_user --bump minor

Copy link
Contributor

github-actions bot commented Feb 3, 2025

Command "prdoc --audience runtime_user --bump minor" has failed ❌! See logs here

@raymondkfcheung
Copy link

/cmd fmt

@raymondkfcheung
Copy link

/cmd prdoc --audience runtime_user --bump minor

@raymondkfcheung raymondkfcheung marked this pull request as ready for review February 3, 2025 16:02
@raymondkfcheung raymondkfcheung requested a review from a team as a code owner February 3, 2025 16:02
@raymondkfcheung
Copy link

/cmd prdoc --audience runtime_user --bump minor --force true

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13132547786
Failed job name: cargo-clippy

@@ -60,7 +60,7 @@ pub struct FeesMode {
pub jit_withdraw: bool,
}

const RECURSION_LIMIT: u8 = 10;
pub const RECURSION_LIMIT: u8 = 10;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it is public now, we should add here some description, when it is used

Comment on lines +778 to +824

// Dummy Barriers
// Dummy filter to allow all
pub struct AllowAll;
impl ShouldExecute for AllowAll {
fn should_execute<RuntimeCall>(
_: &Location,
_: &mut [Instruction<RuntimeCall>],
_: Weight,
_: &mut Properties,
) -> Result<(), ProcessMessageError> {
Ok(())
}
}

// Dummy filter which denies `ClearOrigin`
pub struct DenyClearOrigin;
impl DenyExecution for DenyClearOrigin {
fn deny_execution<RuntimeCall>(
_: &Location,
instructions: &mut [Instruction<RuntimeCall>],
_: 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<Deny: ShouldExecute>(PhantomData<Deny>);
impl<Deny: ShouldExecute> DenyExecution for DenyWrapper<Deny> {
fn deny_execution<RuntimeCall>(
origin: &Location,
instructions: &mut [Instruction<RuntimeCall>],
max_weight: Weight,
properties: &mut Properties,
) -> Result<(), ProcessMessageError> {
Deny::should_execute(origin, instructions, max_weight, properties)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's better not expose all of these as pub and move them directly where needed to the tests/barriers.rs, similar as struct DenyClearTransactStatusAsYield;, we don't need them elsewhere

Comment on lines +599 to +606
pub struct DenyNestedLocalInstructions<Inner>(PhantomData<Inner>);

impl<Inner: DenyExecution> DenyExecution for DenyNestedLocalInstructions<Inner> {
/// 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<RuntimeCall>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename DenyNestedLocalInstructions -> DenyLocalInstructions, because it does top-level + nested

Suggested change
pub struct DenyNestedLocalInstructions<Inner>(PhantomData<Inner>);
impl<Inner: DenyExecution> DenyExecution for DenyNestedLocalInstructions<Inner> {
/// 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<RuntimeCall>(
pub struct DenyLocalInstructions<Inner>(PhantomData<Inner>);
impl<Inner: DenyExecution> DenyExecution for DenyLocalInstructions<Inner> {
/// Denies execution of restricted local XCM instructions, including nested XCM within the `SetAppendix`, `SetErrorHandler`, and `ExecuteWithOrigin` instructions. Applies the `Inner` deny filter **recursively** to any nested XCMs found.
fn deny_execution<RuntimeCall>(

Comment on lines +466 to +478
/// 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.
///
/// 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<Deny, Allow> =
DenyThenTry<DenyNestedLocalInstructions<Deny>, Allow>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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.
///
/// 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<Deny, Allow> =
DenyThenTry<DenyNestedLocalInstructions<Deny>, Allow>;

I think we don't need this type alias, just one DenyThenTry is enough.

so instead:

pub type Barrier = TrailingSetTopicAsId<
	DenyNestedLocalInstructionsThenTry<
		DenyReserveTransferToRelayChain,

I would prefer this:

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
		DenyLocalInstructions<
			DenyReserveTransferToRelayChain,
		>,
		(
			// Allow local users to buy weight credit.
			...

I think this is more readable and easier to follow. If somebody will need to add another Denies, it will be easier to follow.:

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
		DenyLocalInstructions<
			DenyReserveTransferToRelayChain,
+                      DenyXyz1, 
		>,
		(
			// Allow local users to buy weight credit.
			...

or

pub type Barrier = TrailingSetTopicAsId<
	DenyThenTry<
		(
			DenyLocalInstructions<
				DenyReserveTransferToRelayChain,
+                      	DenyXyz1, 
			>,
+                      DenyWhateverNonNestedInstructions<>,
		),
		(
			// Allow local users to buy weight credit.
			...

@@ -144,7 +144,7 @@ impl Contains<Location> for ParentOrParentsPlurality {
}

pub type Barrier = TrailingSetTopicAsId<
DenyThenTry<
DenyNestedLocalInstructionsThenTry<
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I wrote before, we don't need type alias DenyNestedLocalInstructionsThenTry

Suggested change
DenyNestedLocalInstructionsThenTry<
DenyThenTry<
DenyLocalInstructions<

/// It maintains a **recursion counter** to prevent stack overflows due to a deep nesting.
fn deny_recursively<RuntimeCall>(
origin: &Location,
nested_xcm: &mut Xcm<RuntimeCall>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nested_xcm: &mut Xcm<RuntimeCall>,
xcm: &mut Xcm<RuntimeCall>,

SetAppendix(nested_xcm) |
SetErrorHandler(nested_xcm) |
ExecuteWithOrigin { xcm: nested_xcm, .. } =>
DenyNestedXcmInstructions::<Self, Inner>::deny_recursively::<RuntimeCall>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I think that DenyNestedXcmInstructions is kind of unnecessary (and for me little bit hard to follow with this Outer/Inner stuff :) )

DenyNestedXcmInstructions is private, so I think it would be enough just to have some private helper function:

fn deny_recursively<Deny: DenyExecution>(xcm: Xcm) {

         recursion_count::using_once(
             // TODO: all that recursion stuff

            // here just apply recursively
            Deny::deny_execution(xcm, ...)
         )
}

and then use here:

Suggested change
DenyNestedXcmInstructions::<Self, Inner>::deny_recursively::<RuntimeCall>(
// apply `Self` to the `nested_xcm`
deny_recursively::<Self>::deny_recursively(nested_xcm, ...)

Could this be easier? Could this work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. T6-XCM This PR/Issue is related to XCM.
Projects
Status: In-Review
Development

Successfully merging this pull request may close these issues.

[XCM] Investigate better support for filtering XCM programs with Barrier
3 participants