try-runtime pre-post hooks not executed if defined within the pallet #13681
Comments
kianenigma
added
I3-bug
|
Did you try with |
Going to try that right now, |
|
Confirmed, this works on |
|
This makes sense, but that doesn't explain why in our case we can't get pre-runtime and post-runtime to work. (the Here is the output: (I also added logs in the https://github.com/paritytech/substrate/blob/master/frame/support/src/traits/hooks.rs#L170 for |
|
Could you give us a commit hash to try this please @crystalin ? |
|
Here is the branch https://github.com/PureStake/moonbeam/tree/crystalin-broken-try-runtime |
|
Not directly related, but I opened #13692 as the interpreted execution is not easy to debug |
|
I think that was break by #12537, this PR removed the pre/post_upgrade impl for tuples: https://github.com/paritytech/substrate/pull/12537/files#diff-1c449417be9803cb427eed15d51557931141d47d0047561407f1651d319f7103L205 And frame executive use a tuple: <(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::try_on_runtime_upgrade(...) |
|
I confirm @kianenigma , makes the pre_upgrade and post_upgrade work |
|
How do you diener moonbeam? I want to try it, since I cant reproduce it directly in the substrate kitchensink runtime. |
|
I don't know what |
|
How do you build moonbeam with a patched Substrate version? Diener can be used to patch Polkadot and Cumulus to local clones. |
|
edit the In our case we use a fork of substrate |
|
I am a bit confused by the current implementation that I have put there, it is a bit hard to argue about it -- looking further, but for now not easy to replicate a failing test: diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs
index 4e9bc32b00..5e8cb9b4d4 100644
--- a/frame/support/src/lib.rs
+++ b/frame/support/src/lib.rs
@@ -371,16 +371,14 @@ macro_rules! parameter_types {
/// Set the value of this parameter type in the storage.
///
- /// This needs to be executed in an externalities provided
- /// environment.
+ /// This needs to be executed in an externalities provided environment.
pub fn set(value: &$type) {
$crate::storage::unhashed::put(&Self::key(), value);
}
/// Returns the value of this parameter type.
///
- /// This needs to be executed in an externalities provided
- /// environment.
+ /// This needs to be executed in an externalities provided environment.
#[allow(unused)]
pub fn get() -> $type {
$crate::storage::unhashed::get(&Self::key()).unwrap_or_else(|| $value)
diff --git a/frame/support/src/traits/hooks.rs b/frame/support/src/traits/hooks.rs
index a3374d0bf5..592d9733aa 100644
--- a/frame/support/src/traits/hooks.rs
+++ b/frame/support/src/traits/hooks.rs
@@ -197,10 +197,10 @@ impl OnRuntimeUpgrade for Tuple {
weight
}
- #[cfg(feature = "try-runtime")]
/// We are executing pre- and post-checks sequentially in order to be able to test several
/// consecutive migrations for the same pallet without errors. Therefore pre and post upgrade
/// hooks for tuples are a noop.
+ #[cfg(feature = "try-runtime")]
fn try_on_runtime_upgrade(checks: bool) -> Result<Weight, &'static str> {
let mut weight = Weight::zero();
for_tuples!( #( weight = weight.saturating_add(Tuple::try_on_runtime_upgrade(checks)?); )* );
@@ -359,10 +359,64 @@ pub trait OnTimestampSet<Moment> {
#[cfg(test)]
mod tests {
use super::*;
+ use sp_io::TestExternalities;
+
+ #[test]
+ fn on_runtime_upgrade_pre_post_exeucted_tuple() {
+ crate::parameter_types! {
+ pub static Pre: Vec<&'static str> = Default::default();
+ pub static Post: Vec<&'static str> = Default::default();
+ }
+
+ macro_rules! impl_test_type {
+ ($name:ident) => {
+ struct $name;
+ impl OnRuntimeUpgrade for $name {
+ fn on_runtime_upgrade() -> Weight {
+ Default::default()
+ }
+
+ #[cfg(feature = "try-runtime")]
+ fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+ Pre::mutate(|s| s.push(stringify!($name)));
+ Ok(Vec::new())
+ }
+
+ #[cfg(feature = "try-runtime")]
+ fn post_upgrade(_: Vec<u8>) -> Result<(), &'static str> {
+ Post::mutate(|s| s.push(stringify!($name)));
+ Ok(())
+ }
+ }
+ };
+ }
+
+ impl_test_type!(Foo);
+ impl_test_type!(Bar);
+ impl_test_type!(Baz);
+
+ TestExternalities::default().execute_with(|| {
+ Foo::try_on_runtime_upgrade(true).unwrap();
+ // todo unify the API for storage parameter_types! output.
+ assert_eq!(Pre::take(), vec!["Foo"]);
+ assert_eq!(Post::take(), vec!["Foo"]);
+
+ <(Foo, Bar, Baz)>::try_on_runtime_upgrade(true).unwrap();
+ assert_eq!(Pre::take(), vec!["Foo", "Bar", "Baz"]);
+ assert_eq!(Post::take(), vec!["Foo", "Bar", "Baz"]);
+
+ <((Foo, Bar), Baz)>::try_on_runtime_upgrade(true).unwrap();
+ assert_eq!(Pre::take(), vec!["Foo", "Bar", "Baz"]);
+ assert_eq!(Post::take(), vec!["Foo", "Bar", "Baz"]);
+
+ <(Foo, (Bar, Baz))>::try_on_runtime_upgrade(true).unwrap();
+ assert_eq!(Pre::take(), vec!["Foo", "Bar", "Baz"]);
+ assert_eq!(Post::take(), vec!["Foo", "Bar", "Baz"]);
+ });
+ }
#[test]
fn on_initialize_and_on_runtime_upgrade_weight_merge_works() {
- use sp_io::TestExternalities;
struct Test;
impl OnInitialize<u8> for Test {
|
|
@kianenigma is there something else we can do to help ? @librelois identified the part (the for_tuples) that is failing for us, but I don't know actually how it works and why it fixes it. Elois can help if you need |
|
To follow up a bit on a temporary solution, I managed to run both pre and post upgrade tests by expanding what @crystalin suggested: Add the following to substrate/frame/support/src/traits/hooks.rs Line 193 in 6e81199 Which is essentially adding back what was removed in #12319 |
Hey @crystalin, I wasn't able to get this to work (it seems
I was able to get a little closer checking out my local substrate to Could you please elaborate how to get this working with a local/patched Substrate? I'm unable to reproduce this using |
|
I wonder if this actually got solved or not? perhaps this is still some bug in how This is all to some extent a consequence of the fact that we use In Parity Ops, we 99% use the former, so we rarely test the latter. Ideally, some integration tests should mimic a combination of the two. |
|
I confirm it is still NOT working in v0.9.43 |
Hey @crystalin this is on my list of things to look into. Do you have a branch I can reproduce that uses v0.9.43? |
|
This bug is due to Moonbeem using an old version of the Moonbeam's substrate/frame/executive/src/lib.rs Lines 350 to 384 in 48d4313 To prevent this silent failure again I'm going to implement Tuples for |
|
@liamaharon you are looking at a commit that is from April 2022 (more than a year late). |
|
I suspect your runtime is still trying to call the Tuples impl of Can you add some logging into your Executive and |
we have been increasingly using external migrations and have seemingly introduced a regression in how we define migrations, namely
pre-postmigration hooks within a pallet. Here's the most simple way to reproduce this:Apply this:
Then:
Logs won't be emitted, the error that I return in
pre_upgradeis also not caught. Seems like the code is not really executed.Initially tested against
polkadot-9.38branch. Probably present in master as well.The text was updated successfully, but these errors were encountered: