Add ability for try runtime hooks to directly pass data

[emostov]

Currently, if a user of the hooks fn pre_upgrade() -> Result<(), &'static str> {} & fn post_upgrade() -> Result<(), &'static str> {} wants to persist data between the two functions they must use runtime storage (see https://paritytech.github.io/substrate/master/frame_support/traits/trait.OnRuntimeUpgradeHelpersExt.html#method.get_temp_storage). This approach is suboptimal for users who may want to ensure that the storage root has not changed during the execution of either hook (ref: paritytech/polkadot#4772 (comment)).

Instead, a user should be able to persist some information to help verify a migration has executed successfully without modifying the runtime state. A proposed approach is that the functions be modified to output and accept some data. For example the new hook signatures could something look like:

trait OnRuntimeUpgrade {
    ...
    fn pre_upgrade<O>() -> Result<O, &'static str> {}
    fn post_upgrade<I>(_input: I) -> Result<(), &'static str> {}
}

[ayevbeosa]

I am interested in working on this. Where would you suggest would be a good place to start.

[emostov]

I would start by modifying the trait functions signatures and docs here

substrate/frame/support/src/traits/hooks.rs

Lines 176 to 187 in 66ff3a5

	#[cfg(feature = "try-runtime")]
	fn pre_upgrade() -> Result<(), &'static str> {
	Ok(())
	}
	/// Execute some post-checks after a runtime upgrade.
	///
	/// This hook is never meant to be executed on-chain but is meant to be used by testing tools.
	#[cfg(feature = "try-runtime")]
	fn post_upgrade() -> Result<(), &'static str> {
	Ok(())
	}

and here

substrate/frame/support/src/traits/hooks.rs

Lines 263 to 274 in 66ff3a5

	#[cfg(feature = "try-runtime")]
	fn pre_upgrade() -> Result<(), &'static str> {
	Ok(())
	}
	/// Execute some post-checks after a runtime upgrade.
	///
	/// This hook is never meant to be executed on-chain but is meant to be used by testing tools.
	#[cfg(feature = "try-runtime")]
	fn post_upgrade() -> Result<(), &'static str> {
	Ok(())
	}

to roughly:

    fn pre_upgrade<O>() -> Result<O, &'static str> {}
    fn post_upgrade<I>(_input: I) -> Result<(), &'static str> {}

To prove to yourself it works, in the tests mod of hooks.rs you could impl the trait for a dummy struct and write a simple test showing the intended behavior. This isn't necessary though and probably wouldn't get merged in, just recommending if you feel your understanding is shaky.

Then you can go through pallets that have impls for pre_upgrade and post_upgrade and update their function signatures. For all pre-existing cases the signature should look like something like pre_upgrade() -> Result<(), &'static str> and post_upgrade<I>(_: I) -> Result<(), &'static str>.

Once that is done you will also need to create polkadot companion that updates the method signatures in polkadot. See here for instructions: https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well

[emostov]

Let me know if this sounds ok and you intend to start working on this so I can assign the issue to you.

[ayevbeosa]

This sounds okay to me and I intend to start working on it. Thank you very much for the explanation, I have a clearer idea on what to do now.

[ayevbeosa]

This is looking more complicated than I anticipated, I am considering dropping it.

[daanschutte]

Hey @emostov, I'd like to give this one a go.

[emostov]

Hey @emostov, I'd like to give this one a go.

@daanschutte Sounds good, let me know if you have any questions

[daanschutte]

This one is turning out to be lot more complicated than I anticipated, and unfortunately I just don't have the time available at the moment to dedicate finding a solution. I'll keep following it since I'd really like to see the solution here but if someone else wants to pick it up in the meantime it may be resolved sooner.

[kianenigma]

additionally, as @shawntabrizi pointed out in other mediums, we should assert that pre_upgrade and post_upgrade are actually storage_noop!. A pre_upgrade should never alter the storage, because then it can, in principle, affect the on_runtime_upgrade itself.

[NingLin-P]

Here come the next challenger :) , I'm going to have a try.