Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduced events for dmp DownwardMessageSent + refactor to common MessageId for events (ump, dmp) #7161

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

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented May 2, 2023

This PR is trying to:

Open questions:

  • Extract type MessageId / fn message_id from ump/dmp somewhere, e.g. polkadot-core-primitives or polkadot-parachain or polkadot-runtime-parachains?
  • Reuse the same MessageId in Cumulus

Cumulus companion: paritytech/cumulus#2504


Further suggestions/improvements (beyond the scope of this PR):

  • Add uniqueId for uniquess tracking
    • because now MessageId is generated from message content, so the same message means same id, it is not possible to exactly tract
  • Add some contextId / processId to group several events (maybe with this feature, we dont need uniqueId)
    • e.g. for tracking all events which were fired from the same extrinsic call
    • e.g. unique/contextId could be maybe generated based on something like: origin_multilocation + para_id + block_number + message_id + random_nonce
  • Add possibility to track events "over bridge", useful e.g. for some dedicated UI/indexer (this is separated issue):
    • one parachain fires let em_message = xcm::ExportMesage(dest, inner_message);, so we can now track em_message_id (ump/dmp)
    • on BridgeHub ExportMesage is unwrapped and inner_message is sent over bridge, so on the other side we can track just inner_message_id
      • (em_message_id, em_message) -> (inner_message_id, inner_message) -> | BRIDGE | -> (inner_message_id, inner_message)
    • possible solutions are several:
    • e.g. when unwrapping fire event with mapping em_message_id -> inner_message_id (but neither id is unique so this is not exact solution)
    • e.g. add some "unique context id", so we could track flow:
      • (contextId, em_message_id, em_message) -> (contextId, inner_message_id, inner_message) -> | BRIDGE | -> (contextId, inner_message_id, inner_message)
    • e.g. maybe to extend events with XcmContext or use topic somehow?

@paritytech-ci paritytech-ci requested review from a team May 2, 2023 13:26
@bkontur bkontur added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels May 2, 2023
runtime/parachains/src/dmp.rs Outdated Show resolved Hide resolved
runtime/parachains/src/dmp.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team May 4, 2023 08:45
@gavofyork
Copy link
Member

gavofyork commented May 15, 2023

This will probably be helped a lot with a facility from System pallet which provides an intra-block universally unique ID, XcmHash can just return this from anything which can assume frame_system exists. Here it is: paritytech/substrate#14149

I would consider introducing a barrier to allow for - and eventually perhaps even require - a single SetTopic to sit atop any message. A new middleware SendXcm impl could take the current SendXcm impls (typically in a tuple pub type XcmRouter = (...)) and prepend a SetTopic with this unique hash. Block explorers and the like can inspect the messages and the first instruction should generally be SetTopic. If this does indeed become a requirement across the ecosystem then it should constitute a full solution.

One alternative would be to introduce versioning into the channel and send this unique-hash along with each message. The problem here is that we would need to alter the wire-formats of the various MPQ protocols, which is non-trivial and would likely take longer to get merged.

Another alternative would be to alter the XCM format itself to include a message ID, basically making:

mod xcm { mod v4 {
pub struct Xcm {
  unique_id: XcmHash,
  instructions: Vec<Instruction>,
}
} }

This requires messages to provide an ID (which means always using these 32 bytes, but gives a little more certainty and reduces XCM configuration complexity), but the big downside is that XCMv4 would need to be released, which may not be for some months.

@gavofyork
Copy link
Member

#7234 implements the first option, and the description includes further ideas for extending the unique ID system over into bridged chains.

@gavofyork
Copy link
Member

@bkontur is the underlying issue sorted with the merge of #7234 ?

@bkontur
Copy link
Contributor Author

bkontur commented May 29, 2023

@bkontur is the underlying issue sorted with the merge of #7234 ?

@gavofyork yes, I think so, cool, thank you, but I would leave open and re-validate this PR/issue when paritytech/cumulus#2157 is merged