Expose WASM extensions in executor semantics

[s0me0ne-unkn0wn]

Description

During a recent LLVM incident, we discovered that enabling and disabling WASM extensions is not only a matter of our will. Teams working on compilers and WASM runtimes stabilize things independently, and we need a mechanism to enable those extensions deterministically on a per-session basis.

Thus, paritytech/polkadot#6918 was raised. To enable the underlying execution logic to control the extensions, the Substrate has to expose corresponding handles through Wasmtime executor semantics. This PR solves that. The controlling logic will be implemented in a separate PR in the Polkadot repo.

Related issues

Closes #13809
Unblocks paritytech/polkadot#6918

Companion PRs

Polkadot companion: paritytech/polkadot#6998

[koute]

Note: this will also need either a) changes in parity-wasm or b) nuking parity-wasm and migrating to wasmparser + wasm-encoder to actually work (which I have started work on, but it's on a backburner for now)

[bkchr]

During a recent LLVM incident, we discovered that enabling and disabling WASM extensions is not only a matter of our will.

This makes no sense! This pr would not solve anything when there is a compiler bug enabling some features that we don't support. The reasoning makes no sense. You are right that we need a way to enable these features in a sensible way when the time has come. However, we currently have these features disabled and will not randomly enable them. We should not add features for stuff that we don't need. Or do we want to enable any of these features in the near future?

[s0me0ne-unkn0wn]

This makes no sense! This pr would not solve anything when there is a compiler bug enabling some features that we don't support. The reasoning makes no sense.

I agree that the LLVM incident was an outstanding event, and this PR doesn't defend us against breaking changes in the compiler. Nothing can protect us from breaking changes in the compiler, tbh. I hope they've learned the lesson, and next time we'll see some announcements like "we're enabling multi-value in two months". And when we see it, we'll be prepared.

You are right that we need a way to enable these features in a sensible way when the time has come. However, we currently have these features disabled and will not randomly enable them. We should not add features for stuff that we don't need.

And when the time comes, should we do it in panic mode again, releasing hotfixes? Right now, we don't have the means to enable WASM features deterministically. So when we need them, they're better to be there already.
Besides, bringing those features to the semantics control structure is not a complication; it's a natural flow. I'd understand your concerns if it involved complex logic and breaking changes. But it's just three booleans that were hardcoded and now become configurable. Nothing really changes (until we need to change it, as you mentioned).

Or do we want to enable any of these features in the near future?

Well, I would've enabled SIMD already if it were solely my decision 😁 But hopefully not, we're not enabling anything out of the blue. It's just about preparedness; better safe than sorry.

[bkchr]

And when the time comes, should we do it in panic mode again, releasing hotfixes? Right now, we don't have the means to enable WASM features deterministically. So when we need them, they're better to be there already.

In what world should we ever require to release a new language feature in a hurry? Nothing will happen and require that we enable SIMD tomorrow without any preparation. Even if Rust decides to enable the feature tomorrow, you can still continue using the old compiler until we have done all the changes and released them. With PVF pre-checking any PVF that will want to enable a not supported feature will just be rejected.

But it's just three booleans that were hardcoded and now become configurable. Nothing really changes (until we need to change it, as you mentioned).

This is only the case for Substrate, you will also need to add code to Polkadot and then write some migration (because the current executor params are not designed to be easily extended) etc.

[s0me0ne-unkn0wn]

Even if Rust decides to enable the feature tomorrow, you can still continue using the old compiler until we have done all the changes and released them.

Yes, until we do the changes, then release them, then wait for the supermajority to upgrade the node software, etc etc etc. Instead of governance just turning the switch, "we want this feature enabled beginning from session 10453". The latter seems to be the more natural path of evolution of the network, isn't it?

With PVF pre-checking any PVF that will want to enable a not supported feature will just be rejected.

And with this change, we can have PVFs with the feature enabled before the network enables it to be rejected and the PVFs with the feature enabled after the network enables it to be accepted. That sounds like network-wide determinism to me!

This is only the case for Substrate, you will also need to add code to Polkadot and then write some migration (because the current executor params are not designed to be easily extended)

The code is trivial, and I don't think the migration is needed. Those features are just new variants of the enum. We should start using them after the validators get upgraded. Those who didn't wouldn't be able to decode a new enum variant and wouldn't be able to process new PVFs (and probably would be slashed for not upgrading in time, I'm not sure).

All in all, I cannot get your concern about this PR. When we were implementing execution environment parameters themselves, a monstrous PR introducing somewhat breaking features that are not supposed to be used shortly, you weren't against it. Can you please explain why this little change matters so much? Maybe I could rework it to make it more acceptable.

[bkchr]

(and probably would be slashed for not upgrading in time, I'm not sure).

There is no slashing for not upgrading.

All in all, I cannot get your concern about this PR. When we were implementing execution environment parameters themselves, a monstrous PR introducing somewhat breaking features that are not supposed to be used shortly, you weren't against it. Can you please explain why this little change matters so much? Maybe I could rework it to make it more acceptable.

If you go back and look at what I complained about it was "feature creep", aka adding configurations for things that we don't need to configure. Here we again try to introduce configuration options for stuff that we may need at some point or that we may never enable.

[koute]

@bkchr In general I don't like adding stuff we won't immediately need/use right now, but I was under impression that @s0me0ne-unkn0wn 's going to work on this right now:

I've just started working on this

which is why I OK'd this. And at least we do want to enable the bulk memory feature (since it improves performance significantly), and the other features are part of the core WASM spec at this point so they are somewhat reasonable to maybe experiment with. But I guess you're right that for the rest of them we don't have any immediate plans/need to enable them.

[bkchr]

@bkchr In general I don't like adding stuff we won't immediately need/use right now, but I was under impression that @s0me0ne-unkn0wn 's going to work on this right now:

But this says that this feature is already enabled since 3 years?

And at least we do want to enable the bulk memory feature (since it improves performance significantly),

But given this, I'm going to release my block here. Still, we should not add configuration options for all the things, when we don't need them. Removing them otherwise will be almost impossible. In Polkadot please only start with exposing the bulk memory as executor param @s0me0ne-unkn0wn. And a refactor of executor param to support unknown variants (by ignoring them) should also be done. This can be achieved by putting the length of every variant in front of it when encoding it.

[koute]

But this says that this feature is already enabled since 3 years?

The signed ops feature, yes, that is enabled in wasmtime and for that there's no feature knob. But this doesn't matter in practice as any runtime which uses it won't load anyway as parity-wasm will choke on it.

But you do have a good point, this does remind me, @s0me0ne-unkn0wn currently the fact that wasmtime supports e.g. signed ops but we don't load such runtimes is entirely accidental and we should fix this ASAP. That is, we should probably:

1. enable the sign_ext feature in parity-wasm,
2. explicitly detect runtimes which use these instructions reject them, (instead of relying on the feature flag in parity-wasm not being enabled and only implicitly rejecting those runtimes, which brings in the risk of it being accidentally enabled through a transitive dependency without us noticing)
3. (optionally, in a later step) consider enabling the sign_ext, which would entail adding a new feature flag like in this PR which would disable the check that the runtime doesn't use the feature.

[bkchr]

@koute please create an issue for this.

[koute]

@koute please create an issue for this.

Right, good idea! Done: paritytech/polkadot-sdk#15

[eskimor]

And a refactor of executor param to support unknown variants (by ignoring them) should also be done.

This is not a good idea. Those executor params are meant to make it possible to change semantics in a consensus preserving way. If some validators just ignore the value, because they don't know it, then we don't have consensus on semantics. For validators it is better to fail decoding (and punt on participation) than to happily decode, ignore and then dispute a candidate because it used the wrong parameters.

[bkchr]

And a refactor of executor param to support unknown variants (by ignoring them) should also be done.

This is not a good idea. Those executor params are meant to make it possible to change semantics in a consensus preserving way. If some validators just ignore the value, because they don't know it, then we don't have consensus on semantics. For validators it is better to fail decoding (and punt on participation) than to happily decode, ignore and then dispute a candidate because it used the wrong parameters.

Valid point. I would still find it better to not fail on decoding and just stay away from validating a candidate if we found an unknown variant.

[s0me0ne-unkn0wn]

bot merge

[mrcnski]

Why are these not exposed?

[s0me0ne-unkn0wn]

#13811 (comment)

[mrcnski]

Thanks. A comment would have been nice, but it's too late now for my nits. 😛

[eskimor]

Valid point. I would still find it better to not fail on decoding and just stay away from validating a candidate if we found an unknown variant.

Wouldn't the end result be the same? The decoding should only fail if an unknown variant was actually used. So those two should be equivalent. Considering other cases for decoding errors (some corruption), the treatment should also be the same.

Not really arguing that having an explicit error "unknown variant found" would not be better, but maybe decoding could actually tell us that? What I am pushing against a bit in particular is buying this "we can decode whatever" at the expense of type safety.