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 :laughing:

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?

[gavofyork]

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.

Isn't that really dirty? In general, software shouldn't be systemically lying to itself. If the block still has weight left but there is another reason why transactions cannot be pushed, then that is the information which should be reported to the block builder. Pretending anything else is going on is just asking for trouble down the line when some other coder comes to it and expects to be able to rely on the accuracy of what is being reported to the block builder.

[gavofyork]

@ggwpez There's some terminology issues here. Inherents and transactions are both kinds of extrinsic. So you can't say "ExtrinsicsForbidden" add still allow inherents.

Block := BlockHeader ++ Extrinsics. Extrinsics := Inherents (from the system via block-builder) and Transactions (from OCW and users).

In general I think it's good to make things explicit between the runtime and the block builder. I would add that we probably want to ensure that the pre-inherents API allows for the possibility of two modes - one which is "regular" operation and does what happens now, and another "minimal" in which only strictly mandatory inherents may be provided into the runtime (and where the runtime is allowed to panic if inherents not marked as being "optional" are pushed from the builder).

[bkchr]

Isn't that really dirty? In general, software shouldn't be systemically lying to itself. If the block still has weight left but there is another reason why transactions cannot be pushed, then that is the information which should be reported to the block builder. Pretending anything else is going on is just asking for trouble down the line when some other coder comes to it and expects to be able to rely on the accuracy of what is being reported to the block builder.

I mean I would assume that when there is some operation that prevents extrinsics to be included, that it already used all weight (like some migration). But yeah, we can be more explicit by returning some kind of "no further extrinsics".

[ggwpez]

After recent discussions I would then start implementing this instead.
The RuntimeExecutiveMode will decide on the logic to run in a block. It has two variants: Normal and Minimal.

PS: Updated graph.

[gavofyork]

As far as I know there's no such thing as "Mandatory Extrinsics" - they would be "Mandatory Inherents" and are already expressed in the diagram. There are "Operational Transactions", but these are not mandatory.

Inherents are a kind of Extrinsic. Transactions are the other kind. "Extrinsic" just means information whose origin is external to the chain.

[bkchr]

What are optional inherents? If they are optional, they should not be inherents and could just be an unsigned transaction. Or what do I miss?

[bkchr]

(Yeah, I know that we currently don't enforce that inherents are always present on the Executive level. However, I would like to do this to open up utility::batch for None origin again)

[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