Macro or tests to ensure storage elements consistency

[mustermeiszer]

Currently, we have a few rather complex storage types. E.g. the permissions storage and the tranches in the pools. My main concern is, that changes to dependent structs might slip through unnoticed and result in runtime errors upon decoding.

In order to mitigate this risk, we need some logic that ensures storage elements are invariant over time and if an invariant is broken, we get an error (either compilation or through tests).

Needs

• Checks for ALL storage elements we define and use
• Checks on a per runtime basis

Implementation

Generally, I am not in favor of doing this in any specific way, but in order to make this ticket approachable more easily, I draw two "possible" (might also not work) ways to realize what we want.

Proposal A

We write a macro that automatically implements a trait for our storage elements. Best through derive. The trait could look something like:

trait StorageInvariant {
   type Hasher: Hasher; // Something that can hash ^^
   
   /// Implementation that checks whether a default version of 
   /// self, when being scale encoded and then hashed is equal 
   /// to the given hash. 
   fn invariant(hash: Hasher::Hash) -> bool; 
   
   /// Can be used to retrieve the current up-to-date
   /// hash of the default version of self
   fn hash() -> Hasher::Hash;
}

• I made the trait static, as we should probably not test against a specific self.
• we could also check the actual mem-layout of our structs without instantiating it, but I guess with rust unstable ABI this might break even without changes, and I am not sure if wasm is an issue here too.

Finally, we would need to have a test section in the integration-tests that for each runtime has an assert for each of our storage elements of kind:

assert!(StorageElement::invariant("A_HASH_WE_NEED_TO_UPDATE_WHEN_WE_CHANGE_THINGS"));

We could also try to bring into another trait into substrate as a PR that all storage elements get an additional bound that checks their invariance. But I am 99% sure, that this won't happen as it is a serious breaking change for all consumers of substrate.

Proposal B

This proposal is more simple and more work:

• Recreate all used storage elements as "copies" in the runtime-integration tests
• Check whether the copies match the actual types defined in the pallets

Proposal C

I know that parity is doing some crazy stuff within their macros when setting up their polkadot nodes. Basically, they check, whether the given code initializes all fields of a struct during compilation and return a compilation error, if this is not the case.

I guess we could use a similar approach here and check during compilation if the hashes match. But is more of interest from a rust-nerdy perspective than from a solution-wise one.

[wischli]

I was wondering whether the following proposal could also be a sufficient solution?

Proposal D

Add an empty OnRuntimeUpgrade migration which just executes the post_upgrade hook guarded by the `try-runtim~. feature to all runtimes. The hook should iterate over all pallets and their storage items and check whether the latter can be decoded.

Then, the execution of the try-runtime hook needs to be added to CI. This would also come in handy for future migrations which would automatically be checked against live chains via CI.

[mustermeiszer]

Agreed with you. Parity is also going for your approach paritytech/polkadot-sdk#241 (PR)and they already have a check for migrations in the CI via the try-state and fetch state from the live chains.

• CI needs to check both Altair and Centrifuge states