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

AllowKnownQueryResponses broken with new xcm router wrapper WithUniqueTopic #4868

Closed
2 tasks done
JuaniRios opened this issue Jun 24, 2024 · 3 comments
Closed
2 tasks done

AllowKnownQueryResponses broken with new xcm router wrapper WithUniqueTopic #4868

JuaniRios opened this issue Jun 24, 2024 · 3 comments
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@JuaniRios
Copy link
Contributor

JuaniRios commented Jun 24, 2024 •
edited
Loading

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

The new WithUniqueTopic wrapper on the XCM Router appends a SetTopic at the end of any xcm message sent.

The AllowKnownQueryResponses allows messages that only have 1 instruction, QueryResponse.

This means that if you submit a query through XCM, and the destination uses the WithUniqueTopic router, your barrier will reject all responses.

Proposed fix:

  • Expect either 1 message containing the response, or 2 with the last one being the topic.

I can submit a fix soon

Steps to reproduce

  • Send a message containing a query, like QueryPallet to a chain containing the WithUniqueTopic router.
  • See in the logs how your barrier is rejecting the response.
@JuaniRios JuaniRios added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Jun 24, 2024
@JuaniRios
Copy link
Contributor Author

@franciscoaguirre

@bkontur
Copy link
Contributor

bkontur commented Jun 24, 2024

@JuaniRios you need to wrap your barriers with TrailingSetTopicAsId, which is counter-part to WithUniqueTopic, you can try it e.g.:

TrailingSetTopicAsId<AllowKnownQueryResponses<..>>

There is also a test for TrailingSetTopicAsId.

We wrap all our barriers with TrailingSetTopicAsId, e.g. https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/xcm_config.rs#L266

@JuaniRios
Copy link
Contributor Author

@bkontur That's what I was looking for, thanks!

@JuaniRios JuaniRios mentioned this issue Jun 24, 2024
Merged
JuaniRios added a commit to Polimec/polimec-node that referenced this issue Jun 24, 2024
@JuaniRios
🔧 Accept trailing topics on xcm barrier (#335)
0ac2994 
## What?
- Make messages with a trailing `SetTopic` pass the barriers

## Why?
- This is a convention used by parachains. Any query we send will be responded with the response + topic, and currently our barriers don't expect that

## How?
- The wrapper checks if at the end there is a `SetTopic`, and if so removes it and sets the topic of the executor

## Testing?
No tests

## Anything Else?
Discussion on the SDK repo:
paritytech/polkadot-sdk#4868
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bkontur @JuaniRios