Add after_inherents

[ggwpez]

Adds an after_inherents function to the BlockBuilder runtime API. The function is not exposed as hook to the pallets. (Preparation for the MBMs, as requested here)

after_inherents is invoked after Inherents and before any Transaction processing.
It returns RuntimeExecutiveMode which decides whether Transactions can be included in this block. The BlockBuilder respects this return value and includes only inherents otherwise.
frame-executive refuses to import a block when after_inherents returns TransactionsForbidden but transactions are present. This is done by changing ensure_inherents_are_first to return the number of inherents and comparing that with the number of extrinsics in the block.

Changes:

• 🚨 Add after_inherents to Runtime API BlockBuilder.
• 🚨 Change initialize_block from Runtime API Core to return RuntimeExecutiveMode.
• Add variant TransactionsForbidden to EndProposingReason.
• Move frame-executive tests to own file.
• Let ensure_inherents_are_first return the number of inherents.
• Renamed execute_extrinsics_with_book_keeping to apply_extrinsic and add a apply_extrinsic_with_mode variant.

Integration

Add it to your runtime implementation of Core and BlockBuilder:

diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs
@@ impl_runtime_apis! {
	impl sp_block_builder::BlockBuilder<Block> for Runtime {
 		...
+
+		fn after_inherents() {
+			Executive::after_inherents()
+		}

		...
	}
	impl sp_block_builder::Core<Block> for Runtime {
-		fn initialize_block(header: &<Block as BlockT>::Header) {
+		fn initialize_block(header: &<Block as BlockT>::Header) -> RuntimeExecutiveMode {
			Executive::initialize_block(header)
		}

		...
	}

TODO

• Check that try-runtime still works
• Ensure backwards compatibility with old Runtime APIs

[kianenigma]

The only thing that was not possible to address is the inclusion of Mandatory extrinsics when after_inherents forbids extrinsics. The Block builder is unaware of the concept of a DispatchClass, so it cannot make distinctions by it. Any idea how to fix it in a not-too-ugly way? Or are we fine with the current state?

From what I can say, we must be fine with it as there is no other way, unless if we want to leak the concept of DispatchClass or something similar to the client side.

I don't think it is particularly horrible to do that. Not suggesting we leak DispatchClass as-is, but we can consider introducing a new primitive that would be like a u8 indicating the significance of each extrinsic to the block builder (something similar to this). after_inherents can then return something like 128 which indicates that any extrinsic who's significance is higher than 128 should be permitted.

Or even more broadly, the runtime would return a significance: u8 in response to initialize_block, and the block builder would know that it should only add extrinsics of such significance or more into the block.

The client would indeed need to know the significance: u8 of each extrinsic, which can be part of the validate_transaction call, or yet a new API. The runtime should preferably make sure significance is more or less static, in order to not fool the block builder.

This can also help relax the strict need that Inherents must always be first (can't find the discussion, but was talked about recently -- cc @thiolliere). They can appear at any point in the block, and as long as their significance is higher than what initialize_block has dictated, it is acceptable.

All of this can also be backwards compatible, as by returning 255 or 0 you can block all or nothing, which is basically the current behavior.

---

Truthfully, I don't think you should block this PR or MBM for what I am saying, but I hope to give you ideas to refactor the API to something a bit clearer in the long run. Right now, it seems like we are just adding APIs that fix the problem right in front of us and nothing more.

[ggwpez]

I don't think it is particularly horrible to do that. Not suggesting we leak DispatchClass as-is, but we can consider introducing a new primitive that would be like a u8 indicating the significance of each extrinsic to the block builder (something similar to this). after_inherents can then return something like 128 which indicates that any extrinsic who's significance is higher than 128 should be permitted.

Okay now i understood how that should work, because i did not from the other comment alone, thanks 😆

Nothing to add so far. Hopefully it is possible in validate_transaction to avoid another runtime API.

[bkchr]

In general I don't see the need why after_inherents needs to return anything at all? The block builder is just dumb and tries to push transactions until the runtime shouts "stop", not sure why we need to change this here? With MBM we would set the block weight to 100% after we have executed them in a block and then we would get this "stop adding new extrinsics" for free?

This can also help relax the strict need that Inherents must always be first (can't find the discussion, but was talked about recently -- cc @thiolliere). They can appear at any point in the block, and as long as their significance is higher than what initialize_block has dictated, it is acceptable.

Not sure why you want this? Finally we start coming up with a proper way of ensuring that certain operations are not executed before inherents are applied and then you want to revert this again?

[bkchr]

Yeah, I don't really see a need to let after_inherents return some value. We should just handle this by "artificially" using all weight of the block. Then there is no need to add all these extra checks that you have introduced.

[muharem]

looks correct to me.
without approve solely to draw in engineers who are more experienced in this topic.

[muharem]

I do like Normal and Minimal (or could be System). it's not only about non-inherents inclusion, but also about hooks and could be more in the future. More generic names with documentation looks like more flexible approach here. My five cents.

[kianenigma]

Suggested change

	pub fn apply_extrinsic_with_mode(
	fn apply_extrinsic_with_mode(

might help the readability of this file if we only make the things used in the runtime public, and even name them such that it is clear in which API they are meant to be used eg pub fn block_builder_finalze_block()

[ggwpez]

I will make them private otherwise, yes. Not sure if we should do the renaming though. WDYT @bkchr?

[kianenigma]

Why? imagine we are running MBMs, and there is some non-mandatory inherent (of which we currently don't have an example, but it could exist).

It seems like ExtrinsicInclusionMode::OnlyInherents is actually ExtrinsicInclusionMode::OnlyMandatoryInherents? 🙈

[ggwpez]

There could still be optional inherents, the blockbuilder would just omit them.

Mandatory has two meanings that can get mixed up here: Mandatory in the sense of being infallible and Mandatory in the sense that it needs to execute in every block. We can still have optional inherents that are non-mandatory in the second sense by letting the blockbuilder omit them.
But having an inherents that can fail is not quite useful i think. LMK if you meant something else 😄

Not sure about the OnlyMandatoryInherents, maybe but since there are no non-mandatory it is not adding any information, or?

[kianenigma]

Code is solid, but I think the namings we are using here are prone to becoming a puzzle for our future selves, or even worse, others to understand this code.

If no better option exists, I suggest putting some extra documentation about the known behavior of this entire system once the MBM system is final.

[JoshOrndorff]

If you are breaking the core and blockbuilder apis anyway, maybe this is a good time to move initialize_block to its more natural place in the block builder api.

See https://substrate.stackexchange.com/questions/3128/why-is-initialize-block-in-the-core-runtime-api-as-opposed-to-blockbuilder for details.

[ggwpez]

If you are breaking the core and blockbuilder apis anyway, maybe this is a good time to move initialize_block to its more natural place in the block builder api.

See https://substrate.stackexchange.com/questions/3128/why-is-initialize-block-in-the-core-runtime-api-as-opposed-to-blockbuilder for details.

Yes i actually did that in the MR, but then reverted it since we cannot keep backwards compatibility by completely removing a runtime API function. Normally we have this changed_in attribute to indicate change and keep the old version accessible, but there is no removed_in... and even if there were one: then we would have the function in the runtime API twice (which is also ugly).

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3339614