Skip `System::Events` in `TryDecodeEntireState`

[liamaharon]

@joepetrowski encountered this failed try-runtime check on one of his PRs https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4551392

This is the offending block https://rococo.subscan.io/block/0x565751cc2bfaf946216484bb34466c1e5a4f75c444235557d48f822974987048?tab=event

This is likely due to events being emitted in that block using an old storage layout. We avoid this issue with other storage items by running migrations prior to checking if state is decodable. This doesn't work for System::Events.

We should modify our decode state checks to skip System::Events.

[xlc]

can you just modify the try runtime check to call initialize first, which will remove events and bunch other storage or just ignore the events. I just don't get why we need macro magic for this.

[liamaharon]

We want this to be a general solution that doesn't make the assumption that the storage has the same characteristics as System::Events. @bkchr pointed out it's possible for downstream users have similar storage, which we can't assume will be cleared out during on_initialize.

[xlc]

Can you name an example? Otherwise I will say it is overengineering to build a solution to solve a problem that does not yet exist.

[bkchr]

Given the amount of code written, I would not say it is overengineered. Theoretically you could have values set by inherents that do not require to be migrated because you change them in every block any way. Generally, if the macro that generates the code for checking that all types can be decoded wouldn't be the same that we now extend with this option, I would not have proposed to do it this way.

Generally you could also say that having this entire machinery written in Rust is the wrong way, because you can also just write some piece of javascript that can decode the entire state using the metadata.

[xlc]

Given the amount of code written, I would not say it is overengineered.

It may not be overengineered too much, nonetheless could still be poor code in the repo.

Let's define the problem. We have a tool try-runtime that's trying to decode all the storages and ensure all of them are decodable. Right now, it decodes the stroages at end of a block, using the metadata from next block? And because metadata could change, it have false positive.

Shouldn't the solution to be using the right metadata?

For the case you want to validate a new runtime that is still able to decode all the existing data, just do like what a real runtime does. Call initialize, maybe call inherents, and then decode that data.

Generally you could also say that having this entire machinery written in Rust is the wrong way, because you can also just write some piece of javascript that can decode the entire state using the metadata.

No. I am pointing out a classic mistake of not following separation of concern. The try runtime tool is just another software, not part of FRAME. (It maybe now to some people, but it shouldn't be for many reasons) This is a bug of try runtime tool and we should fix the try runtime tool. Not modify low level FRAME code to add a new attribute that only exists to fix the bug in try runtime tool.

Image I want to add a new attribute just because it allows a tool that Acala developed to achieve something, I am sure you will have a lot of questions about it. Try runtime tool should not be an exception here.

We can do this if we have to, but I don't see reason that we have to do this way.

Another issue of this implementation is that it is only accessible via Rust code directly to my understanding. It is not exposed by metadata or any other way. This means we cannot for example using JS to implement this feature. Tell me this is not tight coupling.

There are just too many downsides. It won't work with old blocks. It requires upgrade of pokladot-sdk to work. It makes alternative try-runtime implementation miserable. It makes FRAME harder to learn (due to one more feature no body going to use but still needs to be documented).

Let's fix the problem properly, please. This is exactly why FRAME is complicated and beginner found it is hard to learn.

[liamaharon]

The try runtime tool is just another software, not part of FRAME. (It maybe now to some people, but it shouldn't be for many reasons) This is a bug of try runtime tool and we should fix the try runtime tool. Not modify low level FRAME code to add a new attribute that only exists to fix the bug in try runtime tool.

I think there is some misunderstanding here. It is important to understand the distinction between the Substrate try-runtime feature and the try-runtime-cli tool.

The first is an integral part of FRAME and Substrate, the second is an independent tool. They are seperate. The changes in the proposed solution in this PR are for the try-runtime feature (which is part of FRAME), they have nothing to do with the downstream Parity maintained cli.

If you are unhappy with or have suggestions for how to improve the general direction of the try-runtime feature, I suggest you open up dedicated discussion for that.

Image I want to add a new attribute just because it allows a tool that Acala developed to achieve something, I am sure you will have a lot of questions about it. Try runtime tool should not be an exception here.

I don't see why there would be any problem with Acala adding an improvement to try-runtime features or any other features in Substrate?

Another issue of this implementation is that it is only accessible via Rust code directly to my understanding. It is not exposed by metadata or any other way.

This change has nothing to do with which languages can call into the try-runtime runtime APIs. If there is a problem with that, please open a dedicated issue so we can discuss making them easier to access with your language of choice.

It makes alternative try-runtime implementation miserable.

I don't understand what you are getting at here. This fixes a bug for tools which use try-runtime feature gated code for checking state decoding. It doesn't impact any other tools. Please explain what you mean by "alternative try-runtime implementation" and second how this makes it "miserable".