Add event for showing the hash of an UMP sent message #1228

girazoki · 2022-05-02T09:39:46Z

Adds Event showing Hash of message sent to parent through UMP

The goal is to match this ID with the one that the UMP queue shows in the event: https://github.com/paritytech/polkadot/blob/0fca26fb0a5bdf00bde70a9bcc31805dad61b2a9/runtime/parachains/src/ump.rs#L139

bkchr · 2022-05-03T11:04:41Z

pallets/parachain-system/src/lib.rs

 		/// \[ weight_used, result_mqc_head \]
 		DownwardMessagesProcessed(Weight, relay_chain::Hash),
+		/// An Upward message was sent to the parent.
+		UpwardMessageSent(Option<MessageId>),


Why don't you use T::Hash here as well?

I just tried to mimic as much as possible what ump shows so that both ids match. But I am happy to change it if I find no issues doing so

However, now that I think about it; the reason for proposing this PR is to help potential cross-chain explorers identify and match which messages have been sent from a chain and executed in another chain. With this goal in mind, shouldnt we ensure we mimic exactly what ump is doing to calculate the id?

T::Hash and T::Hashing can be configured in the runtime and they might have different hashing implementations than that of the ump queue.

So I am hesitant about using the more generic T::Hash and T::Hashing, what do you think?

This really should be done as part of XCMv3, since there is a concept of XCM hashes there, and we can use it here as well.

I can PR the xvm V3 branch if prefered

(Keith pointed me out this SE answer when I was sniffing around for something similar: https://substrate.stackexchange.com/questions/1893/is-there-an-id-for-xcm-message/1906#1906 )

Yes I agree with what Gav said. And although the message hash being different is something each chain can work on indepentently, I think to help explorers identify these hashes the ump queue should emit an event with this hash. I will re-work this PR to go into xcm V3, as it seems we already have the notion of "hash of the message" there

girazoki · 2022-05-09T15:54:57Z

Back to draft because I need to test it with cargo override

gilescope · 2022-05-18T09:03:19Z

any news?

girazoki · 2022-05-18T09:28:32Z

I think I can make it ready for review, as I tested it against the xcmv3 branch back on its day

girazoki · 2022-05-18T09:33:17Z

For some reason, I obtained different results using message.using_encoded(sp_io::hashing::blake2_256) vs using sp_io::hashing::blake2_256(message). I can investigate further, but I chose using the later as it is what the ump queue outputs too https://github.com/paritytech/polkadot/blob/ae53f58f9975a03c6d6b282e51b9d46f5a92f991/runtime/parachains/src/ump.rs#L92

girazoki · 2022-05-18T09:52:41Z

Alright using_encoded yields a different slice (because it prepends a first byte indicating the lenght). Since we want to mimic the ump event I think we should stay with sp_io::hashing::blake2_256(message)

albertov19 · 2022-05-27T09:46:24Z

This will not get merged into XCM the current XCM implementation? Having a hash to match a XCM message sent from a parachain to a relay chain (or the other way around) is key to improve the UX of XCM in general.

We have been offering a XCM explorer bounty for two hackathons in a row, but without this feature, it is not possible to have a straightforward way to link XCM messages between chains. Note that from parachain to parachain this is not true, and it is possible to link XCM message sent on one chain, to XCM message executed in the other chain.

…ump-message-hash-event

girazoki · 2022-10-31T11:05:48Z

Hi! Just wanted to know whether there is any plan of incorporating this or it conflicts with paritytech/polkadot-sdk#489

CC: @KiChjang @joepetrowski

joepetrowski · 2022-10-31T15:41:07Z

I don't see how it conflicts, unless 6161 implies the removal of send_upward_message from Parachain System? Nonetheless, the event will still be useful for indexing services.

KiChjang · 2022-11-01T11:50:03Z

I don't think this conflicts, but will need to double-check whether XCM v3 already includes an XCM hash in the event. My guess is no, in which case this PR does not conflict with anything at all.

gilescope · 2022-11-12T09:08:17Z

Personally I would love to see this land.

…

On Tue, 1 Nov 2022 at 11:50, Keith Yeung ***@***.***> wrote: I don't think this conflicts, but will need to double-check whether XCM v3 already includes an XCM hash in the event. My guess is no, in which case this PR does not conflict with anything at all. — Reply to this email directly, view it on GitHub <#1228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEJCGGA6G2MBSTB2AAVF3WGD7XLANCNFSM5U3PNUIA> . You are receiving this because you commented.Message ID: ***@***.***>

bkontur · 2022-11-14T09:23:17Z

pallets/parachain-system/src/lib.rs

+
+		// The relay ump does not use using_encoded
+		// We apply the same this to use the same hash
+		// TODO: shall we change both ump and this to


what about this TODO?

It seems paritytech/polkadot#6271 will still use sp_io::hashing::blake2_256(message). I guess we can leave it as is (and remove the todo), as it is the only way hashes will match

bkchr · 2022-12-30T22:35:22Z

@KiChjang do we want this merged?

KiChjang · 2023-01-02T12:15:39Z

Yes, but not at this current state as it's failing CI.

bkchr · 2023-01-02T22:50:29Z

@girazoki can you make the CI working? :)

girazoki · 2023-01-03T08:17:51Z

I fixed the fmt checks, but the build jobs won't pass because this is going to the xcmv3 branch and the CI does not pass there. If you prefer, I can make it go to master, but then the xcmv3 branch will need to be updated

paritytech-cicd-pr · 2023-01-03T08:22:03Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/cumulus/-/jobs/2215233

bkchr · 2023-01-03T09:12:40Z

error[E0412]: cannot find type `XcmHash` in this scope
   --> primitives/core/src/lib.rs:100:61
    |
96  | pub trait UpwardMessageSender {
    |                              - help: you might be missing a type parameter: `<XcmHash>`
...
100 |     fn send_upward_message(msg: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError>;
    |                                                                ^^^^^^^ not found in this scope
error[E0412]: cannot find type `XcmHash` in this scope
   --> primitives/core/src/lib.rs:103:62
    |
102 | impl UpwardMessageSender for () {
    |     - help: you might be missing a type parameter: `<XcmHash>`
103 |     fn send_upward_message(_msg: UpwardMessage) -> Result<(u32, XcmHash), MessageSendError> {
    |                                                                 ^^^^^^^ not found in this scope

But these messages look valid?

girazoki · 2023-01-03T09:26:12Z

I think this is because the branch still is pointing at the master branch of polkadot, which does not contain the xcmHash type. The xcmHash type was only added in the xcmv3 branch in polkadot. I see similar errors on the cumulus companion for xcmv3 :

error[E0412]: cannot find type `XcmHash` in this scope
   --> pallets/dmp-queue/src/lib.rs:479:11
    |
479 |             _hash: XcmHash,
    |                    ^^^^^^^ not found in this scope

bkchr · 2023-01-03T09:27:33Z

Ahh okay!

@KiChjang please verify that this branch compiles for you and then let's merge it.

KiChjang · 2023-01-03T09:49:48Z

Ah okay, let me just merge this as-is.

girazoki marked this pull request as draft May 2, 2022 10:29

girazoki closed this May 2, 2022

girazoki reopened this May 2, 2022

girazoki marked this pull request as ready for review May 2, 2022 16:00

bkchr reviewed May 3, 2022

View reviewed changes

Add UpwardMessageSent event in parachain-system

fae4dd5

girazoki force-pushed the girazoki-add-ump-message-hash-event branch from 3d6357c to fae4dd5 Compare May 9, 2022 15:53

girazoki changed the base branch from master to gav-xcm-v3 May 9, 2022 15:54

girazoki marked this pull request as draft May 9, 2022 15:54

girazoki added 2 commits May 10, 2022 12:38

additional fixes

6eebc1a

Message Id

cd5db5c

girazoki marked this pull request as ready for review May 18, 2022 09:28

girazoki requested review from KiChjang and bkchr May 18, 2022 09:30

jamesbayly mentioned this pull request May 21, 2022

[$20,000] XCM UI Tool (with SubQuery, Moonbeam, Acala, and HydraDX) subquery/developer-guild#26

Closed

girazoki added 2 commits October 31, 2022 12:00

Merge remote-tracking branch 'upstream/gav-xcm-v3' into girazoki-add-…

d6049fb

…ump-message-hash-event

Fix errors from merge

c49f379

KiChjang approved these changes Nov 14, 2022

View reviewed changes

bkontur reviewed Nov 14, 2022

View reviewed changes

girazoki added 5 commits January 3, 2023 08:56

Merge remote-tracking branch 'origin/gav-xcm-v3' into HEAD

484cab9

fmt

7daad07

more fmt

ab2a95f

Remove todo

d9ab982

more formatting

effbc81

KiChjang merged commit e2a3354 into paritytech:gav-xcm-v3 Jan 3, 2023

mkchungs mentioned this pull request Apr 19, 2023

[Feature Request] Add Event to Show DMP XCMHash After Enqueued/Attempted #2468

Open

XY-Wang mentioned this pull request Oct 17, 2023

Emit New Event with Message Hash and ID on DMP Message Sent paritytech/polkadot-sdk#1905

Open

2 tasks

Add event for showing the hash of an UMP sent message #1228

Add event for showing the hash of an UMP sent message #1228

Uh oh!

Conversation

girazoki commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

girazoki May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

girazoki commented May 9, 2022

Uh oh!

gilescope commented May 18, 2022

Uh oh!

girazoki commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

girazoki commented May 18, 2022

Uh oh!

girazoki commented May 18, 2022

Uh oh!

albertov19 commented May 27, 2022

Uh oh!

girazoki commented Oct 31, 2022

Uh oh!

joepetrowski commented Oct 31, 2022

Uh oh!

KiChjang commented Nov 1, 2022

Uh oh!

gilescope commented Nov 12, 2022 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Dec 30, 2022

Uh oh!

KiChjang commented Jan 2, 2023

Uh oh!

bkchr commented Jan 2, 2023

Uh oh!

girazoki commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paritytech-cicd-pr commented Jan 3, 2023

Uh oh!

bkchr commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

girazoki commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Jan 3, 2023

Uh oh!

KiChjang commented Jan 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

girazoki commented May 2, 2022 •

edited

Loading

girazoki May 3, 2022 •

edited

Loading

girazoki commented May 18, 2022 •

edited

Loading

girazoki commented Jan 3, 2023 •

edited

Loading

bkchr commented Jan 3, 2023 •

edited

Loading

girazoki commented Jan 3, 2023 •

edited

Loading