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

Dry run xcms on bridge hubs to get forwarded xcms across the W<>R bridge #6002

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

Conversation

franciscoaguirre
Copy link
Contributor

@franciscoaguirre franciscoaguirre commented Oct 9, 2024

Addresses #5708
Closes: #5878

The DryRunApi did not return any forwarded xcms when used on bridge hubs.
This PR completes the story of dry running an XCM across the W<>R bridge.

The tests dry_run_transfer_to_westend_sends_xcm_to_bridge_hub and dry_run_transfer_to_rococo_sends_xcm_to_bridge_hub show how you can start from Asset Hub on one side of the bridge and get the message that will be executed on the Asset Hub on the other side.
These two tests use the test_dry_run_transfer_across_pk_bridge macro.

The most important implementation is InspectMessageQueues for pallet bridge messages.
With this new implementation we can implement this on bridge hub polkadot and kusama for the P<>K bridge.

I also added a call to clear_messages in dry_run_xcm which was missing (it was previously only added to dry_run_call).

TODO

  • remove DescendOrigin when dispatching on BH and validate UniversalOrigin in the XCM message when doing dispatch on BH
  • dry-run on the source BH should return final XCM as executed on destination AH + destination should be target AH

@franciscoaguirre franciscoaguirre added the T6-XCM This PR/Issue is related to XCM. label Oct 9, 2024
@franciscoaguirre franciscoaguirre self-assigned this Oct 9, 2024
@franciscoaguirre franciscoaguirre requested a review from a team as a code owner October 9, 2024 21:06
@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 9, 2024 21:07
@franciscoaguirre franciscoaguirre changed the title Dry run xcms on bridge hubs to get forwarded xcms across the P<>K bridge Dry run xcms on bridge hubs to get forwarded xcms across the W<>R bridge Oct 9, 2024
@franciscoaguirre
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Oct 9, 2024

@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7548088 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 29-a382d81c-ceb6-40ad-87d8-982dbec44101 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Oct 9, 2024

@franciscoaguirre Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7548088 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7548088/artifacts/download.

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

nice!

@acatangiu acatangiu added the A4-needs-backport Pull request must be backported to all maintained releases. label Oct 10, 2024
Base automatically changed from xcm-dry-run-redundant-forwarded-xcms to master October 10, 2024 09:23
@acatangiu acatangiu requested a review from serban300 October 10, 2024 14:00
bridges/modules/messages/src/lib.rs Outdated Show resolved Hide resolved
bridges/modules/messages/src/lib.rs Outdated Show resolved Hide resolved
bridges/modules/messages/src/lib.rs Outdated Show resolved Hide resolved
remote_message = messages[0].clone();
});

$sender_bridge_hub::execute_with(|| {
Copy link
Contributor

Choose a reason for hiding this comment

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

@franciscoaguirre
I think we should be able to test or dry-run the full route: AHK -> BHK ... BHP -> AHP.

This way, in the end, we'll be able to write or run a periodic TypeScript/PAPI/whatever script that tests/simulates/dry-runs an asset transfer from AHK to AHP live, without the need to manually intervene (no chopsticks required).

I think the only missing piece is the DescendOrigin I mentioned here. We need to trigger some pre-dispatch, fake-message-proof-delivery, or whatever runtime API on the bridged BridgeHub, that will ultimately append the DescendOrigin. Let me think about it more.

Copy link
Contributor

@acatangiu acatangiu Oct 15, 2024

Choose a reason for hiding this comment

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

doesn't the dispatch (and DescendOrigin appending) happen before sending to outbound HRMP queue? I mean outbound HRMP Q of BH will contain full XCM exactly as it goes to AH, so I don't see the problem

Copy link
Contributor

Choose a reason for hiding this comment

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

well, not exactly, bridged/target BridgeHub appends to the xcm DescendOrigin when dispatching: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-builder/src/universal_exports.rs#L433-L439

so the XCM in outbound Q on sender BridgeHub is not exactly the same as it comes to the target AH

Copy link
Contributor

Choose a reason for hiding this comment

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

but the appending DescendOrigin we can do "offchain" (in the test, PAPI call), so this PR should be enough probably

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'm thinking of adding chopsticks tests for this under integration-tests/chopsticks/ but in another PR

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

The mechanism here is actually incomplete, and the tests only test the source side of the bridge.

We need to support the full path across all hops all the way to final destination.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 15, 2024 12:54
@bkontur bkontur assigned bkontur and unassigned franciscoaguirre Oct 18, 2024
@bkontur
Copy link
Contributor

bkontur commented Nov 12, 2024

@franciscoaguirre

  1. part - easy changes should be like:
  1. part - important changes:
  • now we need to validate and check decoded XCM somewhere on the BridgeHub in the BridgeBlobDispatcher, which does two things: 1. decoding from BridgeMessage and 2. send_xcm::<Router>, that xcm "contains" expected UniversalOrigin instruction.

Proposed solutions

This is how it looks now:

/// Dispatches received XCM messages from other bridge
type FromWestendMessageBlobDispatcher =
	BridgeBlobDispatcher<XcmRouter, UniversalLocation, BridgeRococoToWestendMessagesPalletInstance>;
1. Add filter capabilities to the *BridgeBlobDispatcher:

One possible solution would be to add some barrier/filter-like configurable capabilities to the BridgeBlobDispatcher interface/signature to allow filtering decoded XCM, e.g. Filter: Contains<Xcm<()> or maybe ShouldExecute. Something like:

pub struct BridgeBlobDispatcher<Router, OurPlace, OurPlaceBridgeInstance, Filter: Contains<Xcm<()>>(
or 
pub struct BridgeBlobDispatcher<Router, OurPlace, OurPlaceBridgeInstance, Filter: ShouldExecute>(
2. Proposed wrapper implementation with checking XCM:

There is also an easy solution, we can easily add another SendXcm wrapper implementation over Router. Something like this:

/// Dispatches received XCM messages from other bridge
type FromWestendMessageBlobDispatcher =
	BridgeBlobDispatcher<
		EnsureUniversalOrigin<XcmRouter, WestendGlobalConsensusWithNetworId>,
		UniversalLocation,
		()
	>;
// TODO: place somewhere in the xcm-builder:
pub struct EnsureUniversalOrigin;
impl SendXcm<XcmRouter: SendXcm, ExpectedUniversalOrigin: Get<Junction>> for EnsureUniversalOrigin {
	type Ticket = XcmRouter::Ticket;

	fn validate(destination: &mut Option<Location>, message: &mut Option<Xcm<()>>) -> SendResult<Self::Ticket> {

		// TODO: some validation for: ExpectedUniversalOrigin::get() and message
                // TODO: not sure how to validate `UniversalOrigin`:
                //  - check if message starts with required first instruction with expected UO?
                //  - or check recursively all the XCM messages? This could goes tricky, probably not the best solution at all.

		XcmRouter::valdiate(...)
	}

	fn deliver(ticket: Self::Ticket) -> Result<XcmHash, SendError> {
		XcmRouter::deliver(ticket)
	}
}

or maybe EnsureUniversalOrigin could be replace with some generic stuff:

pub struct FilteredSendXcm;
impl SendXcm<XcmRouter: SendXcm, Filter: Contains<Xcm>> for FilteredSendXcm {
	type Ticket = XcmRouter::Ticket;

	fn validate(destination: &mut Option<Location>, message: &mut Option<Xcm<()>>) -> SendResult<Self::Ticket> {
		
                // TODO: do some filtering
                ensure!(Filter::contains(message));

		XcmRouter::valdiate(...)
	}

        fn deliver(ticket: Self::Ticket) -> Result<XcmHash, SendError> {
		XcmRouter::deliver(ticket)
	}
}
3. other solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T6-XCM This PR/Issue is related to XCM.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

dryRunApi.dryRunCall: redundant items in forwardedXcms
3 participants