fix: ensure extrinsic types are collected before pallet types

[carlosala]

CheckMetadataHash depends on an implementation detail of frame-metadata: the lookup order.
Therefore, we need to ensure that the collection of relevant types in the registry stays stable across different versions. These types are, essentially, the extrinsic types. Collecting them before anything else ensures the order is the same.
Note: modifying v14 is not necessary, since CheckMetadataHash only supports v15 and ahead.

References: polkadot-fellows/RFCs#148 (comment) TalismanSociety/talisman#2180 polkadot-js/api#6227 paseo-network/runtimes#309

cc @bkchr @jsdw

[carlosala]

This should be a patch for frame-metadata (it is only an implementation detail), and updating in polkadot-sdk is very straightforward.

[Copilot]

Pull request overview

This PR fixes metadata hash stability for CheckMetadataHash by ensuring extrinsic types are collected before pallet types in the metadata registry. The hash depends on the type lookup order, so this change maintains stability across different metadata versions (v15 and v16).

Key Changes:

• Reorder type collection in RuntimeMetadataV15::new() and RuntimeMetadataV16::new() to register extrinsic types before pallets
• Reorder field initialization in ExtrinsicMetadata::into_portable() for v15 to align type registration order with v16
• Add explanatory comments documenting the requirement for stable collection order

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
frame-metadata/src/v15.rs	Moved extrinsic type collection before pallets in RuntimeMetadataV15::new(); reordered ExtrinsicMetadata::into_portable() to register extensions before extra_ty for consistency with v16
frame-metadata/src/v16.rs	Moved extrinsic type collection before pallets in RuntimeMetadataV16::new(); added explanatory comments for both constructor and ExtrinsicMetadata::into_portable()

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[carlosala]

Pinging people who built clients for Polkadot. You shouldn't rely on the ordering of the lookup, since it is an implementation. Nevertheless, does anyone think this can be a problem?

@TarikGul @leonardocustodio @sinzii

[leonardocustodio]

Let me check this with @justkawal on Polkadart. But I totally agree that the order should not affect the metadata hash. I believe this should work similarly to dcbor spec, which sorts data objects; there is a predefined sorting order that makes it deterministic regardless of the order you give the objects/maps/json/data...

[leonardocustodio]

Would you like to comment @Lohann ?

[carlosala]

which sorts data objects; there is a predefined sorting order

I agree, for next version of CheckMetadataHash this will be a requirement. Nevertheless, for CheckMetadataHash V1 we are stuck with this implementation detail.

[Lohann]

Hey there, I'm not the maintainer of this repo, but I visit it a lot when I need to parse frame-metadata in another language like what we did in Polkadart's substrate_metadata, and one really annoying thing is the absence of unit tests and examples, there's not a single test for v15.rs and v16.rs, the test_data only contains examples up to v14...

Is really difficult for anyone having to implement a compliant frame-metadata encoder/encoder in another language to validade if their implementation is correct, and is also hard for anyone working in this repo to avoid introducing breaking changes.

[Lohann]

IMO any PR that fixes an issue, must have a test reproducing the issue it fixes, otherwise we must blindly trust it, or spend a lot of time to get the context on how merkleized-metadata is implemented, to understand what is causing the issue, etc...

[Lohann]

But I totally agree that the order should not affect the metadata hash

About this topic specifically, I believe the source of this is because frame-metadata's types aren't associative, this PR is an band-aid fix, but this issue will happen again because internally substrate uses an intermediate MetadataIR, which is converted to the respective frame-metadata version, any changes there or here can trigger this issue again, and there are multiple ways to create an equivalent metadata simply reordering or duplicating things.

Have multiple representations of the same frame-metadata wasn't an issue before RFC: 0078-merkleized-metadata, but now it is and I don't think this is taken into account in the design.

One way to make a JSON deterministic for example is sorting all attributes, the same principle can be applied in future frame-metadata versions, basically assuring there's just one valid deterministic representation for a given metadata, for example using hashes instead numeric ids to refer to types, but then the tricky part will be ciclic types.

[leonardocustodio]

As @carlosala said, the PR is a remediation to CheckMetadataHash V1.
It is just an improvement that will not require us to bump the version and break backwards compatibility.
But yeah, as commented and as the dcbor spec specifies, sorting the attributes would be a nice thing to make for V2.

[Lohann]

the dcbor spec specifies, sorting the attributes would be a nice thing to make for V2

The tricky part is that unlike JSON or dcbor, frame-metadata allows ciclic references, more likely a graph instead a tree, example:

struct A {
    value: Option<Box<B>>
}
struct B {
    value: Option<Box<A>>
}

It can get worse in some enums where we find multiple cicles, like:

<type> -> [<dependencies>]
A -> [B]
B -> [C]
C -> [D, E]
D -> [A, B]
E -> []

Currently the order is arbitrary, the precedence is determined by the type id which by itself is order dependent, but for a truly unique metatadata some deterministic non-ambiguous rule is needed to determine the precedence in those cases.

[josepot]

this PR is an band-aid fix

This is, indeed, the case. As @carlosala already pointed out:

CheckMetadataHash depends on an implementation detail of frame-metadata: the lookup order.

The goal of this PR is to provide a temporary patch. A new version of the CheckMetadataHash signed-extension is probably needed, yes.

Currently the order is arbitrary, the precedence is determined by the type id which by itself is order dependent, but for a truly unique metatadata some deterministic non-ambiguous rule is needed to determine the precedence in those cases.

We (the PAPI team) solved these issues a while ago.

The solution that we created is TS specific, but it could be generalized for any other language. We will try to find some time to make a detailed blog-post about this topic, because it's certainly fascinating.

However, please know that this has very little to do do with the work done on this PR.

[Lohann]

We (the PAPI team) solved polkadot-api/polkadot-api#448.

Interesting, is this the same thing that Polkadot's Ledger App does? I remember having issues to sign transactions using ledger plugin + polkadot-api at the beginning of this year.
UPDATE: After look the code I got it know, this hash is for uniquely identify generated types, very nice, but I missed a more detailed description. I noticed it's using Tarjan's String Connected Components for grouping, but this algorithm may generate different results based on the starting node, so is not completely order independent... this is not an requirement if you expect the types to have the same order, but is an issue for an order indepedent merkleized-metadata, but I haven't read the whole code to understand if this was fixed somewhere else.

However, please know that this has very little to do do with the work done on this PR.

Yep I got it, my last message was about the long-term solution not this specific PR. The absence of unit tests also is an issue of this repo, not of this particular PR.

[bkchr]

One way to make a JSON deterministic for example is sorting all attributes, the same principle can be applied in future frame-metadata versions, basically assuring there's just one valid deterministic representation for a given metadata, for example using hashes instead numeric ids to refer to types, but then the tricky part will be ciclic types.

I tried to do exactly this in the spec with the IDs. I also told @carlosala that I forgot to mention that this is only build for V15. The runtime side is always only using metadata v15 and not anything else to build the hash. In the future we will improve this.

[bkchr]

I also know that my description of the algorithm was very confusing in the old spec. Also something that needs to be improved with the new one.

[bkchr]

@jsdw please release these changes as a patch release.