Moves Block, NodeBlock and UncheckedExtrinsic to frame_system, instead of construct_runtime

[gupnik]

Fixes #14126

Currently, one needs to define these params explicitly when using construct_runtime!

type Extrinsic = MockUncheckedExtrinsic<Runtime>;
type Block = MockBlock<Runtime>;

frame_support::construct_runtime!(
	pub struct Runtime
	where
		Block = Block,
		NodeBlock = Block,
		UncheckedExtrinsic = Extrinsic,
	{
		System: frame_system,
		Currency: pallet,
	}
);

This PR add these types as part of frame_system instead and allows the following:

frame_support::construct_runtime!(
	pub struct Runtime {
		System: frame_system,
		Currency: pallet_currency,
	}
);

Note that this change is backwards compatible. However, a warning is issued if the older syntax is still used.

Further, this PR also removes BlockNumber and Header from frame_system::Config. It instead uses Block to retrieve these using the trait HeaderProvider.

impl frame_system::Config for Runtime {
-    type Header = Header;
-    type BlockNumber = u32;
+    type Block = generic::Block<Header, UncheckedExtrinsic>;
}

Note that this is a breaking change.

Cumulus companion: paritytech/cumulus#2753
Polkadot companion: paritytech/polkadot#7392

[kianenigma]

Nice! I think now we can even further simplify the Executive struct as well to not require these generics directly, and just fetch them from System, which it already has access to.

Unfortunately, this will be a breaking change though, but I think one for the greater good.

The only unfortunate thing is that these new types are not really needed if frame_system::Config is being implemented outside of the scope of construct_runtime or without an Executive. Nowadays, even in tests, we have a construct_runtime. Nonetheless, it might be good to allow all these types to be set to () when you really really don't care about it. Although, we can already set them to frame_system::mocking types.

All in all, in my opinion, a step in the right direction. @bkchr @ggwpez thoughts?

[kianenigma]

Do we have proper types for when frame_system is not part of the runtime? given this, we are making it mandatory, and we should have proper specification about it.

[bkchr]

Do we have proper types for when frame_system is not part of the runtime? given this, we are making it mandatory, and we should have proper specification about it.

Frame system is since always mandatory for each runtime.

In general I would not move forward with the pr as stuff like NodeBlock probably can be removed in the near future. For uncheckedextrinsic we also need to see if we can do this better.

[gupnik]

In general I would not move forward with the pr as stuff like NodeBlock probably can be removed in the near future. For uncheckedextrinsic we also need to see if we can do this better.

@bkchr Could you please explain this a bit more? For UncheckedExtrinsic, what do you think could be the best way?

[kianenigma]

In general I would not move forward with the pr as stuff like NodeBlock probably can be removed in the near future. For uncheckedextrinsic we also need to see if we can do this better.

@bkchr Could you please explain this a bit more? For UncheckedExtrinsic, what do you think could be the best way?

I stumbled upon frame/support/procedural/src/construct_runtime/expand/metadata.rs, in paritytech/polkadot-sdk#198 which is one example usage of what you are looking for. In general, you can start by searching similar usages in construct_runtime to see exactly how NodeBlock, Block and Extrinsic is used.

[gupnik]

bot fmt

[command-bot]

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2970900 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-a1726420-a381-4ec1-886b-333caa1fad5f to cancel this command or bot cancel to cancel all commands in this pull request.

[gupnik]

bot fmt

[command-bot]

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3028550 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-04fb1317-bb13-4251-b43b-178e886d0d8d to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3028550 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3028550/artifacts/download.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3034706

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3034707

[bkchr]

@gupnik I'm not happy with the solution you are providing here. You are touching a lot of things that are irrelevant to the things you are doing. I looked into the cycling compile error and came up with the following workaround for the compiler bug:
c33def7

(Run cargo check -p pallet-timestamp --tests)

The shitty part of this solution is that we require to have this Wrapper type around types that use T::Block when they appear in the call enum. Otherwise we get the same error. We could also put this Wrapper into each variant in RuntimeCall. Maybe that is the better solution, up for discussion.

Another possible solution could be to keep Header and Hash in the Block trait, but forward them to the super trait HeaderProvider (this should also give a much smaller changeset).

You can also start upstreaming the changes you have done around using BlockNumberFor to reduce the size of the final pr even further.

[gupnik]

Thank you @bkchr for looking into this.

I looked into the cycling compile error and came up with the following workaround for the compiler bug:
c33def7

The shitty part of this solution is that we require to have this Wrapper type around types that use T::Block when they appear in the call enum.

Yeah, I do not particularly like this one as well. Happy to brainstorm for sure.

Another possible solution could be to keep Header and Hash in the Block trait, but forward them to the super trait HeaderProvider (this should also give a much smaller changeset).

This would mean that we would be able to access T::Header at some places while the others would still require the super-trait. Won't it make sense to choose a consistent approach throughout?

You can also start upstreaming the changes you have done around using BlockNumberFor to reduce the size of the final pr even further.

This was again done to ensure consistency. I planned to remove BlockNumberFor altogether as a follow-up.

[gupnik]

bot fmt

[command-bot]

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3037787 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 15-125f1d02-8805-4a3c-9d16-98bc79822ed5 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3037787 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3037787/artifacts/download.

[bkchr]

This would mean that we would be able to access T::Header at some places while the others would still require the super-trait. Won't it make sense to choose a consistent approach throughout?

You are breaking tons of code just to workaround a compiler bug. I don't see any reason why we should do this. Everyone using the block trait would need to change the code to a hack that we may revert at some point.

This was again done to ensure consistency. I planned to remove BlockNumberFor altogether as a follow-up.

I don't get why you want to do this? This doesn't provide any consistency, just more code to write.

[gupnik]

Thanks @bkchr.

Another possible solution could be to keep Header and Hash in the Block trait, but forward them to the super trait HeaderProvider (this should also give a much smaller changeset).

Let me try this one.

[gupnik]

@bkchr Created #14437 to use the super-trait. Let me know if I should pursue that one instead.
CC: @kianenigma @sam0x17

[gupnik]

Closing this in favour of #14437