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.

Sign up for GitHub

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

Jump to bottom

xcm: fix for DenyThenTry Barrier #7169

Merged
merged 52 commits into from
Jan 27, 2025
Merged

xcm: fix for DenyThenTry Barrier #7169

merged 52 commits into from
Jan 27, 2025
+473 −12

Conversation

yrong
Copy link
Contributor

@yrong yrong commented Jan 15, 2025 •
edited by bkontur
Loading

Resolves (partially): #7148 (see Problem 1 - ShouldExecute tuple implementation and Deny filter tuple)

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

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

TODO

  • add unit-test for DenyReserveTransferToRelayChain
  • add test and investigate/check DenyThenTry as discussed here and update documentation if needed

@yrong yrong requested a review from a team as a code owner January 15, 2025 02:43
@yrong yrong changed the title Fix for DenyThenTry xcm: fix for DenyThenTry Barrier Jan 15, 2025
@yrong yrong mentioned this pull request Jan 15, 2025
Closed
@yrong yrong marked this pull request as draft January 15, 2025 03:36
bkontur
bkontur requested changes Jan 15, 2025
View reviewed changes
@bkontur
Copy link
Contributor

bkontur commented Jan 15, 2025

@yrong overall looks good so far, I was also thinking about another trait for that like this

@bkontur bkontur requested a review from x3c41a January 15, 2025 09:46
@yrong yrong marked this pull request as ready for review January 15, 2025 12:38
@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Jan 15, 2025
franciscoaguirre
franciscoaguirre reviewed Jan 15, 2025
View reviewed changes
@yrong yrong requested a review from bkontur January 15, 2025 16:06
@yrong yrong mentioned this pull request Jan 16, 2025
Draft
3 tasks
franciscoaguirre
franciscoaguirre approved these changes Jan 24, 2025
View reviewed changes
polkadot/xcm/xcm-executor/src/traits/should_execute.rs Outdated
instructions: &mut [Instruction<RuntimeCall>],
max_weight: Weight,
properties: &mut Properties,
) -> Option<ProcessMessageError>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it better with the Option because seeing Some(_) makes me guess it will be denied but seeing Ok(_) makes me doubt whether or not it's okay to deny or okay to pass.

To clear up all possible misunderstanding we could use a custom enum:

enum DenyResult {
  Deny,
  DontDeny,
}

Might be overkill though.

@franciscoaguirre
Copy link
Contributor

/cmd fmt

@yrong
Copy link
Contributor Author

yrong commented Jan 26, 2025

@acatangiu Please check if there is anything important that I should address for merging.

@bkontur
Copy link
Contributor

bkontur commented Jan 27, 2025

/cmd fmt

@raymondkfcheung raymondkfcheung mentioned this pull request Jan 27, 2025
Merged
2 tasks
@acatangiu acatangiu enabled auto-merge January 27, 2025 16:24
@acatangiu acatangiu added this pull request to the merge queue Jan 27, 2025
Merged via the queue into paritytech:master with commit b30aa31 Jan 27, 2025
202 of 205 checks passed
ordian added a commit that referenced this pull request Jan 30, 2025
@ordian
Merge branch 'master' into ao-migrate-zombie-past-slashing-test
722a770 
* master: (58 commits)
  [pallet-revive] pack exceeding syscall arguments into registers (#7319)
  cumulus: bump PARENT_SEARCH_DEPTH and add test for 12-core elastic scaling (#6983)
  xcm: fix for DenyThenTry Barrier (#7169)
  Migrating polkadot-runtime-common slots benchmarking to v2 (#6614)
  Add development chain-spec file for minimal/parachain templates for Omni Node compatibility (#6529)
  `Arc` definition in `TransactionPool` (#7042)
  [sync] Let new subscribers know about already connected peers (backward-compatible) (#7344)
  Removed unused dependencies (partial progress) (#7329)
  Improve debugging by using `#[track_caller]` in system `assert_last_event` and `assert_has_event` (#7142)
  `set_validation_data` register weight manually, do not use refund when the pre dispatch is zero. (#7327)
  Fix the link to the chain snapshots (#7330)
  revive: Fix compilation of `uapi` crate when `unstable-hostfn` is not set (#7318)
  [pallet-revive] eth-rpc minor fixes (#7325)
  sync-templates: enable syncing from stable release patches (#7227)
  Bridges: emulated tests small nits/improvements (#7322)
  fix(cmd bench-omni): build omni-bencher with production profile (#7299)
  Nits for collectives-westend XCM benchmarks setup (#7215)
  bench all weekly - and fix for pallet_multisig lib (#6789)
  Deprecate ParaBackingState API (#6867)
  Fix setting the image properly (#7315)
  ...
raymondkfcheung added a commit that referenced this pull request Jan 31, 2025
@raymondkfcheung @yrong
@bkontur @franciscoaguirre @claravanstaden @acatangiu @github-actions
poc(XCM): Deny Nested XCM Barriers (#7351)
df64f5f 
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 <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2025
@bkontur @raymondkfcheung
@github-actions @yrong @franciscoaguirre @claravanstaden @acatangiu
XCM: Deny barrier checks for nested XCMs with specific instructions t…
bd7cf11 
…o be executed on the local chain (#7200)

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 a new barrier -
`DenyRecursively` - to provide more refined control over instruction
denial. The main change is the replacement of `DenyThenTry<Deny, Allow>`
with `DenyThenTry<DenyRecursively<Deny>, Allow>` 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_](#7148).

# TODO
- [x] Evaluate PoC, more details at #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`.
- **DenyThenTry<DenyInstructionsWithXcm<Deny>, Allow>**: Alternatively,
hard-code those three instructions in `DenyThenTry`, so we wouldn’t need
`DenyInstructionsWithXcm`. However, this approach wouldn’t be as
general.
- **DenyInstructionsWithXcmFor**: 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](#7200 (comment))
- [x] Consider better name `DenyInstructionsWithXcm` =>
`DenyRecursively`, more details at
[here](#7200 (comment))
- [x] Clean-up and docs
- [x] Merge #7169 or
rebase this branch on the top of `yrong:fix-for-deny-then-try`
- [x] Set for the runtimes where we use `DenyThenTry<Deny, Allow>` =>
`DenyThenTry<DenyRecursively<Deny>, Allow>`
- [ ] Schedule sec.audit

---------

Co-authored-by: Raymond Cheung <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: ron <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
github-actions bot pushed a commit that referenced this pull request Mar 3, 2025
@bkontur @github-actions
XCM: Deny barrier checks for nested XCMs with specific instructions t…
79233b3 
…o be executed on the local chain (#7200)

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 a new barrier -
`DenyRecursively` - to provide more refined control over instruction
denial. The main change is the replacement of `DenyThenTry<Deny, Allow>`
with `DenyThenTry<DenyRecursively<Deny>, Allow>` 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_](#7148).

# TODO
- [x] Evaluate PoC, more details at #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`.
- **DenyThenTry<DenyInstructionsWithXcm<Deny>, Allow>**: Alternatively,
hard-code those three instructions in `DenyThenTry`, so we wouldn’t need
`DenyInstructionsWithXcm`. However, this approach wouldn’t be as
general.
- **DenyInstructionsWithXcmFor**: 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](#7200 (comment))
- [x] Consider better name `DenyInstructionsWithXcm` =>
`DenyRecursively`, more details at
[here](#7200 (comment))
- [x] Clean-up and docs
- [x] Merge #7169 or
rebase this branch on the top of `yrong:fix-for-deny-then-try`
- [x] Set for the runtimes where we use `DenyThenTry<Deny, Allow>` =>
`DenyThenTry<DenyRecursively<Deny>, Allow>`
- [ ] Schedule sec.audit

---------

Co-authored-by: Raymond Cheung <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: ron <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Clara van Staden <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
(cherry picked from commit bd7cf11)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claravanstaden claravanstaden claravanstaden left review comments

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@bkontur bkontur bkontur approved these changes

@acatangiu acatangiu acatangiu approved these changes

@raymondkfcheung raymondkfcheung raymondkfcheung approved these changes

@x3c41a x3c41a Awaiting requested review from x3c41a

Assignees
No one assigned
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
Bridges + XCM
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[XCM] Investigate better support for filtering XCM programs with Barrier
6 participants
@yrong @bkontur @franciscoaguirre @claravanstaden @acatangiu @raymondkfcheung