support upgrade hooks to directly pass data

[NingLin-P]

Attempt to fix #10824

polkadot address: 1CWyso9Zw46QdR54sTvKJXTUdmZznYZa2aJfNHdG6xQ6L71

[NingLin-P]

This is because almost all crate's try-runtime feature didn't contain frame-system/try-runtime, and there is no default implement for PreStateDigest and pre_upgrade, thus when compiling these crates errors like frame-system missing trait items will return.

[NingLin-P]

I had tried to provide a default implement here by PreStateDigest: Default and return Ok(Self:: PreStateDigest::default()). But it failed to implement OnRuntimeUpgrade for Tuple because it has PreStateDigest: (P1, P2, ..) and rust only implement Default for tuples up to 12 elements, also #11813 had changed the nested tuple to a flatten one.

[kianenigma]

Oops, that is one issue that we may have intorduced with flatten tuples.

[bkchr]

I think this is fine. It just shows that the current code is "wrong"

[kianenigma]

Can't we make this type be two different generics that FullCodec in each function, as the issue recommended? Having to define this type also increases the footprint of this PR a lot.

Something like:

fn pre_upgrade<I: Encode>() -> Result<I, ..> {}
fn post_upgrade<I: Decode>(input: I) {}

and we encode and pass the data between the hooks automatically.

This is more or less what we do now with OnRuntimeUpgradeHelpersExt, and if we do this we can remove this extension trait + make sure the state root is not changing in the pre/post migrate hooks.

[NingLin-P]

I think we can't do that, the type parameters of a generic function are instantiated when calling the function not when we implement traits (defining the function).

Using concrete types like:

fn pre_upgrade() -> Result<Vec<u8>, ..> {}
fn post_upgrade(input: Vec<u8>) {}

may work though, but requiring to do encode/decode within pre_upgrade/post_upgrade

[kianenigma]

I think we can't do that, the type parameters of a generic function are instantiated when calling the function not when we implement traits (defining the function).

Not sure if this is an issue, can you give it a try?

All in all, I don't want to pollute the code too much for this feature. In rare cases we might need to pass data between the two hooks. Let's just make it Vec<u8> (or T: Codec) and move on without making a big change.

Making sure that the hooks don't alter storage is a good change though, let's keep that part.

[NingLin-P]

Yes, I had tried,

impl OnRuntimeUpgrade for SomeType {
    fn pre_upgrade<u32>() -> Result<u32, &'static str> {
        ...
    }
}

the compiler will recognize u32 as a type parameter instead of a concrete type.

I will try to it Vec<u8>.

[NingLin-P]

I will try to it Vec.

I really made progress on this approach, except for one (perhaps final) issue that OnRuntimeUpgrade for Tuple failed due to Codec did not implement for tuple with elements more than 18. This may be related to https://github.com/paritytech/parity-scale-codec/blob/46859b803c94f091e3ca644d8468bfadfbd2c539/src/max_encoded_len.rs#L79-L81

It work now, PTAL

[kianenigma]

Looks in the right direction, but in general turned out a bit more complicated than anticipated.

[NingLin-P]

Looks in the right direction, but in general turned out a bit more complicated than anticipated.

Yes, it is a bit complicated, especially impl OnRuntimeUpgrade for Tuple. This PR could be trivial if we can use the unstable associated_type_defaults feature and the nested tuple is still in use. Then we can provide a total default implement for the OnRuntimeUpgrade trait and also make the tuple implement working, thus without introducing any redundant code.

[kianenigma]

Yes, it is a bit complicated, especially impl OnRuntimeUpgrade for Tuple. This PR could be trivial if we can use the unstable associated_type_defaults feature and the nested tuple is still in use. Then we can provide a total default implement for the OnRuntimeUpgrade trait and also make the tuple implement working, thus without introducing any redundant code.

I think the current state of the PR is good though, not too much complexity, we can allow a Vec<u8> to be passed in and out, guard is there, and not much complexity is added.

WDYT @NingLin-P? I will do a last review later.

[NingLin-P]

I think the current state of the PR is good though, not too much complexity, we can allow a Vec<u8> to be passed in and out, guard is there, and not much complexity is added.

WDYT @NingLin-P? I will do a last review later.

Yes, I agree.

[kianenigma]

LGTM, Thanks!

[kianenigma]

Polkadot address?

[BustaNit]

Quick nit. Otherwise LGTM

[sam0x17]

Looks good, would like to see some more detailed docs as mentioned by other reviewers ;)

[kianenigma]

bot merge

[kianenigma]

/tip small

[substrate-tip-bot]

@kianenigma A small tip was successfully submitted for NingLin-P (1CWyso9Zw46QdR54sTvKJXTUdmZznYZa2aJfNHdG6xQ6L71 on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips