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

Snowbridge v2 - Outbound Queue #6706

Closed
wants to merge 115 commits into from
Closed

Conversation

yrong
Copy link
Contributor

@yrong yrong commented Nov 29, 2024

Context

This PR is for implement the design in SnowbridgeV2, essentially moving from ordered messaging system to an unordered one.

It focus more on outbound changes, inbound changes will be addressed in another PR.

The point is that we want to make V2 work side by side with V1, so add new routes/pallets specific for V2 and leave legacy codes untouched is important.

This PR is also based on XCM V5 with new instructions like AliasOrigin, InitiateTransfer in place.

Initial PR yrong#4 has been internally reviewed, companion changes required for other components of the bridge including:

Workflow

  1. on AH we add a custom exporter returns a lower fee(without the execution cost on Ethereum including), essentially predict the route by content of XCM, more details in Predicate route table by Xcm #6074 and thanks for the review from @bkontur

  2. On BH also add a new exporter to handle V2 specific xcm, convert the xcm to a Message structure which will be executed on Ethereum.

  3. In OutboundQueue store the message into a Merkle tree as V1 before, and add the pending message with the fee attached into PendingOrder storage. Code here for detail. The off-chain relayer should check if relaying the message with the fee attached is profitable.

  4. In OutboundQueue add a new extrinsic submit_delivery_proof in which to verify the Message has been delivered to Ethereum, only after that add the fee to RewardLeger which can be redeemed by the relayer later, more details in Relayer rewards paid to specified location account #6578

Review notes

Simulated test is the entry point how to build XCM for V2, also a good candidate to dig deep into the process.

cargo test -p bridge-hub-westend-integration-tests --lib tests::snowbridge_v2_outbound -- --nocapture

@franciscoaguirre franciscoaguirre added the T15-bridges This PR/Issue is related to bridges. label Jan 27, 2025
})
.collect();

let message = InboundMessage {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this called InboundMessage? It's going from Polkadot to Ethereum, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a bit confusing, InboundMessage here is in the context of Ethereum, the receiver end.

Maybe renaming it to OutboundMessage makes more sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Obsolete, this API has been removed in #7402

bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs Outdated Show resolved Hide resolved
bridges/snowbridge/pallets/system-frontend/src/lib.rs Outdated Show resolved Hide resolved

let command = Command::CreateAgent {};

Self::send(origin_location.clone(), command, fee)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this command fail? What happens if the storage has the agent but the actual command fails?

Copy link
Contributor Author

@yrong yrong Jan 30, 2025

Choose a reason for hiding this comment

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

We just push the command to the message queue. I would assume if the send call fails, then it will also revert the agent storage change, so it might not be a problem.

let _ = self.next();
}

// Try to get ENA again if it is after PNA
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it first ENA then PNA and then ENA again? Why can't it be PNA again afterwards?

Copy link
Contributor Author

@yrong yrong Jan 30, 2025

Choose a reason for hiding this comment

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

@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 30, 2025 01:16
@vgeddes vgeddes mentioned this pull request Jan 30, 2025
@@ -0,0 +1,3 @@
# Ethereum Outbound Queue

Sends messages from an origin in the Polkadot ecosystem to Ethereum.
Copy link
Contributor

Choose a reason for hiding this comment

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

please more detailed docs: design, APIs, usage, etc

could also be done as code docs (example) instead of README

Copy link
Contributor

Choose a reason for hiding this comment

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

I would move the contents of this to bridges/snowbridge/pallets/outbound-queue-v2/runtime-api/src/lib.rs as high-level code docs: example

/// `sp_runtime::generic::DigestItem::Other`
fn prove_message(leaf_index: u64) -> Option<MerkleProof>;

fn dry_run(xcm: Xcm<()>) -> Result<(OutboundMessage,Balance),DryRunError>;
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs an origin too to work properly, no? or does the impl always assume origin is AH?

Copy link
Contributor Author

@yrong yrong Feb 2, 2025

Choose a reason for hiding this comment

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

The origin is derived from AliasOrigin here

Btw, the dry_run here is used only to convert the xcm to an abi-encoded message in order to estimate the cost on Ethereum later.

Comment on lines +17 to +28
/// An inbound message that has had its outer envelope decoded.
#[derive(Clone, RuntimeDebug)]
pub struct Envelope<T: Config> {
/// The address of the outbound queue on Ethereum that emitted this message as an event log
pub gateway: H160,
/// A nonce for enforcing replay protection and ordering.
pub nonce: u64,
/// Delivery status
pub success: bool,
/// The reward address
pub reward_address: T::AccountId,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Without having read all the code and figuring this out, just by looking at these types and their documentation I am utterly confused.

Is this "inbound" in what context? inbound for BH or inbound for Ethereum? it's on the BH outbound queue crate so I am guessing it's envelope for BH->Ethereum message, but then the outer comment just says "inbound" instead of "outbound", and the inner gateway says outbound q on Ethereum that emitted this message, meaning it IS in fact inbound to BH (Ethereum->BH). But then I'm confused why there are different envelopes (one in inbound-q-v2 and one here) for Ethereum->BH messages?

all in all, needs much more detailed/clearer docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

meaning it IS in fact inbound to BH (Ethereum->BH). why there are different envelopes (one in inbound-q-v2 and one here) for Ethereum->BH messages?

Exactly. The inbound message here is actually the delivery proof sent from Ethereum to BH. Workflow and some details I added in #6706 (comment).

Sorry, it's a bit confusion here. We'll try to improve the docs/address the comments in #7402 Vincent added recently, we think it may be better to review the V2 changes in a single PR at this moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest also explicit naming like MessageEnvelope and DeliveryProofEnvelope

Copy link
Contributor

Choose a reason for hiding this comment

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

//! 2. The message is then enqueued for later processing via the implementation for
//! [`snowbridge_outbound_primitives::v2::SendMessage::deliver`]
//! 3. The underlying message queue is implemented by [`Config::MessageQueue`]
//! 4. The message queue delivers messages back to this pallet via the implementation for
Copy link
Contributor

Choose a reason for hiding this comment

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

they are not really coming "back", no?

Suggested change
//! 4. The message queue delivers messages back to this pallet via the implementation for
//! 4. The message queue delivers messages to this pallet via the implementation for

//! [`frame_support::traits::ProcessMessage::process_message`]
//! 5. The message is processed in `Pallet::do_process_message`: a. Assigned a nonce b. ABI-encoded,
//! hashed, and stored in the `MessageLeaves` vector
//! 6. At the end of the block, a merkle root is constructed from all the leaves in `MessageLeaves`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//! 6. At the end of the block, a merkle root is constructed from all the leaves in `MessageLeaves`.
//! 6. At the end of the block, a merkle root is constructed from all the leaves in `MessageLeaves`,
//! then `MessageLeaves` is dropped so that it is never committed to storage or included in PoV.

@yrong
Copy link
Contributor Author

yrong commented Feb 14, 2025

Closed in favor of #7402

We'll address all left comments there.

@yrong yrong closed this Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants