Configuration Test for Runtime

[kianenigma]

Here are a number of cases in pallet configs (via their Trait) that we pretty much rely on comments and the author to not make mistakes.

substrate/bin/node/runtime/src/lib.rs

Lines 260 to 261 in 8c18580

	// for a sane configuration, this should always be less than `AvailableBlockRatio`.
	pub const TargetBlockFullness: Perbill = Perbill::from_percent(25);

substrate/bin/node/runtime/src/lib.rs

Line 340 in 8c18580

pub const ElectionLookahead: BlockNumber = 25; // 10 minutes per session => 100 block.

^^ must be less than the length of one session per block. (although we cannot know this from just Trait and system::Trait)

All the SomethingBonds must be above MinimumBalance.

And these are only ones that I have seen. We probably have more that I don't know.

We can do better. A module can expose some tests that generally depend on <T: Trait + system::Trait> in their own source code. a call to construct_runtime!{} will check that this function won't panic with the created Runtime, hence we can know upon initialisation of a runtime if it has been configured correctly or not.

[kianenigma]

Might be unrelated, but see what happens here: paritytech/polkadot#1021

Perhaps there should have been a way fro staking to know and assert at compile time/ genesis that it should be included in a runtime for which its ValidateUnsigned is enabled.

[kianenigma]

Another one: https://github.com/paritytech/substrate/pull/5802/files#r421555192

The colective::Members must be the same as Elections::Members (in normal conditions).

[kianenigma]

This is practically blocked on stabel const fn as well rust-lang/rust#57563

[gui1117]

one implementation could be adding yet another optional function in decl_module similarly to on_runtime_upgrade. we would have runtime_test and this function will be used to implement tests by construct_runtime.

But not sure I'm in favor of it. Would be good to move on_initialize/on_finalize/on_runtime_ugprade to some trait implementation with default implementation being no-op.

[kianenigma]

Would be good to move on_initialize/on_finalize/on_runtime_ugprade to some trait implementation with default implementation being no-op.

I see your preference to have them default to noop but even if we add them to decl_module the default is already a noop, so this doesn't make a big difference as far as I can see.

I also considered duplicating the pattern of how we do on_runtime_upgrade and it is indeed one way to do it. But I don't think it has any added value compared to add_extra_genesis. It has a different purpose and name, but I think you can achieve it with add_extra_genesis as well, so no point in adding a on_runtime_construction or whatever.

[bkchr]

I actually already started to add something similar to decl_module!. But did not get that far, because other stuff came in.

I imagined it like this:

trait IntegrityTest {
    fn integrity_test();
}

decl_module! {

  fn integrity_test() {
      // Check stuff
  }
}

construct_runtime! would generate a test that calls all these integrity_test functions. So, a normal cargo test would execute them and make sure that everything is set up correctly.

[kianenigma]

sounds good to me. This is actually better than add_extra_genesis because it is hooked into cargo test.

[kianenigma]

There are a small number of tests that I'd still like to add, and they are not super important, but they are also blocked on specilaization and/or some kind of downcasting that we still don't seem to have.

The basic requirement is like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=03c5e0698e6cdaad28cc2431eee94b7e

Check from pallet X, if T has other::Trait, and then do some checks. But this is as far as I have tried and asked around impossible to do, and if so it would 100% require the unstable specialization feature.

Examples of such tests are:

1. Staking::ElectionLookahead must be less than Babe::EpochDuration.
2. Collective::Members and Elections::Members should technically match.

These are not really important tests, so I don't see this as a priority issue, but I do want to note that we still don't have a way to express or test these (other than sprinkling const_assert in the runtime which is not a solution).

One way that came out of a discussion with @thiolliere was that we could have a new module in pallet/integrity-tests that is only for inter-pallet tests. Yet, even this won't be trivial. Say we implement the first one for staking and babe. Ideally, we want the construct_runtime to include this test iff Runtime: staking::Trait + babe::Trait. This still seems like a lot of hacking for a handful of tests though.

Either way, I am writing this down for the future. In case the list above of inter-pallet tests become more extensive, I think we should add ma standard machinery for it.