feat: CheckMetadataHash extension

[wischli]

Description

⚠️ Must not be merged before at least Polkadot Js Extension supports this feature, see paritytech/polkadot-sdk#4659 ⚠️

• Adds Implement CheckMetadataHash extension paritytech/polkadot-sdk#4274 to all runtimes which is necessary for the Polkadot Generic Ledger App
• Changes based on the corresponding guide.
• Increases compile time, but only in on-chain-relese-build mode
• AFAIU, the signed extension is only compiled in release mode

TODO

• Before we propose the Centrifuge Release, we need to ensure transactions can be signed properly in Polkadot Apps and Centrifuge App on Demo! cc @mustermeiszer
• Update CI to build WASMs using on-chain-release-build feature
  --> Seems to be used by srtool per default which we use for release builds.

Checklist:

• I have added Rust doc comments to structs, enums, traits and functions
• I have made corresponding changes to the documentation
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or that my feature works

[lemunozm]

👍🏻

[wischli]

@lemunozm Unfortunately, multiple of our integration tests fail with CheckMetadataHash enabled: https://github.com/centrifuge/centrifuge-chain/actions/runs/9665090255/job/26661355826

I am not fully sure why. Based on the build.rs, it is only enabled for --features=std,metadata-hash:

centrifuge-chain/runtime/altair/build.rs

Lines 1 to 6 in 2e8e44c

	#[cfg(all(feature = "std", feature = "metadata-hash"))]
	fn main() {
	substrate_wasm_builder::WasmBuilder::init_with_defaults()
	.enable_metadata_hash("AIR", 18)
	.build();
	}

Moreover, it is enabled for on-chain-release-build:

centrifuge-chain/runtime/development/Cargo.toml

Lines 450 to 457 in 2e8e44c

	# A feature that should be enabled when the runtime should be build for on-chain
	# deployment. This will disable stuff that shouldn't be part of the on-chain wasm
	# to make it smaller like logging for example.
	on-chain-release-build = [
	"sp-api/disable-logging",
	"runtime-common/on-chain-release-build",
	"metadata-hash",
	]

I tried both running integration tests with -F metadata-hash as well as enabling it for the runtimes in the integration-tests/Cargo.toml but ran into polkadot-sdk compiler issues. So it appeared to be the incorrect approach. Upon further investigating the SDK repo, I saw all their integration tests disable the metadata hash. However, this might be due to performance reasons. Apparently, the WASM needs to be built twice if the feature is enabled (see guide).

[lemunozm]

Checking this...

[lemunozm]

tried both running integration tests with -F metadata-hash as well as enabling it for the runtimes in the integration-tests/Cargo.toml but ran into polkadot-sdk compiler issues

I think this is the correct one. Nevertheless, when adding the feature, could be another missing crate from Polkadot that also requires that feature, then you get those polkadot errors. I'm going to try to overpass this

[wischli]

Yeah I also think it is some missing feature for a polkadot crate. Tried out a lot and still haven't surpassed the error

error[E0433]: failed to resolve: could not find `metadata_ir` in `__private`
   --> /Users/william/.cargo/git/checkouts/polkadot-sdk-cff69157b985ed76/c00ca35/substrate/primitives/api/src/lib.rs:797:1
    |
797 | / decl_runtime_apis! {
798 | |     /// The `Core` runtime api that every Substrate runtime needs to implement.
799 | |     #[core_trait]
800 | |     #[api_version(4)]
...   |
827 | |     }
828 | | }
    | |_^ could not find `metadata_ir` in `__private`

It can be quickly checked by trying to build any runtime with on-chain-release feature. In the Polkadot SDK this works for the template runtime. For us, it doesn't for any runtime.

[wischli]

I also don't understand why only ~20% of the integration tests fail. I couldn't detect a pattern yet as the errors occur for all runtimes as well as fudge and non-fudge envs.

[lemunozm]

Fails only the test who makes a call to submit_now() or submit_later(), because those calls sign the transaction. Other calls just mutate the state

[lemunozm]

I'm also stuck in the above error. Quite weird. It seems like frame-metadata feature is missing for sp-api. But even if you try adding it, the error still happens 🤔. It seems like who add sp-api first in the cargo tree should add the frame-metadata to it.

[wischli]

Yeah I have been there as well. Good to know I havent missed anything obvious and sad this doesnt seem to work right now for us. Will probably open an issue in the SDK then.

[lemunozm]

Yeah, Although I think this is more a "rust" problem. It seems unmaintenable that each time you add a feature to a crate, you "need" to propagate that feature to the crates that use the featured gated crate. Ideally, you need to add it even to those crates that don't require that feature. Just for the case that a possible user want to enable it.

Maybe the solution here from polkadot would be to just remove the frame-metadata feature. Include metadata_ir always. The only reason to add the feature is to avoid the double wasm compilation. But here, it could be avoided.

[lemunozm]

If we have this false, then IT tests work?

[wischli]

Yes because it disables the metadata hash check.

[lemunozm]

I think we can skip checking the metadata for IT. What knowledge gives us IT if that value is true?

[wischli]

I wanted to make the same proposal. In the end, we expect the SDK to ensure the validity of this API.

[lemunozm]

For signed-related things, we also mock the nonce and similar things for IT, so I think we're fine not checking that. It feels like out of scope (?)

[wischli]

During engineering sync, we decided to add this feature into the release due to the signature success on/with Polkadot Js, Polkadot Js Extension, Polkadot Vault, Talisman and as of last night, Centrifuge Apps.