From 1d216a5b915b4d9702a4bb9faf625d20ecd8a00f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:02:56 +0200 Subject: [PATCH 01/19] Add RFC #13 Signed-off-by: Oliver Tale-Yazdi --- ...untime-api-block-builder-v5-and-core-v7.md | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 text/0013-runtime-api-block-builder-v5-and-core-v7.md diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-runtime-api-block-builder-v5-and-core-v7.md new file mode 100644 index 000000000..4ceb99929 --- /dev/null +++ b/text/0013-runtime-api-block-builder-v5-and-core-v7.md @@ -0,0 +1,83 @@ +# RFC-0013: Runtime API `BlockBuilder` v5 and `Core` v7 + +| | | +| --------------- | ------------------------------------------------------------------------------------------- | +| **Start Date** | July 24, 2023 | +| **Description** | Changes to the BlockBuilder v5 and Core v7 Runtime APIs. | +| **Authors** | Oliver Tale-Yazdi | + +## Summary + +Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs and bumps them to version 5 and 7 respectively. A new hook `after_inherents` is added to the `BlockBuilder` runtime API and the +`initialize_block` function of `Core` is changed to return an enum to indicate what extrinsics and hooks can be executed. + +## Motivation + +The original motivation are Multi-Block-Migrations. They require the runtime to be able to tell the block builder that it should not attempt to include transactions in the current block. Currently there is no communication possible between runtime and block builder that could achieve this. Further it is necessary to execute logic right after inherent application but still before transaction inclusion. + +This RFC proposes a way of communication between the runtime and the block builder by changing the `initialize_block` function to return a `BlockExecutiveMode` enum. Further an `after_inherents` function is introduced that runs after inherent but before transaction application. + +## Stakeholders + +- Substrate Maintainers: They will have to implement it upstream with all the imposed testing, audit and maintenance burden. +- Polkadot Runtime developers: They will have to adapt to this breaking change. +- Polkadot Parachain Teams: They also need to adapt to the breaking changes but will eventually have multi-block migrations available. + +## Explanation + +All block authors MUST respect the `BlockExecutiveMode` that is returned by `initialize_block`. Further they MUST always invoke `after_inherents` directly after inherent application. The runtime MAY reject a block that violates either of these requirements. + +Backwards compatibility with the existing runtime API SHOULD be implemented in the authorship logic to not mandate a lockstep update on the node side. + +Enum `BlockExecutiveMode` has two variants: `Normal` and `Minimal`. +- `Normal` indicates that all user transactions can be included as usual and no special detour or skipping of logic happens. +- `Minimal` indicates that only hard-deadline code (ie. inherents, `on_initialize`...) SHALL run. Transactions MUST NOT be included. Optional hooks, like `poll` and `on_idle`, MOST NOT run either; even if there is still weight available for them. + +## Drawbacks + +[…] drawbacks relating to performance, ergonomics, user experience, security, or privacy: None + +Only drawback is that the block execution logic becomes more complicated. There is more room for error. +Additionally downstream developers need to adapt their code to this breaking change. + +## Testing, Security, and Privacy + +Compliance of a block author can be tested by adding specific code to the `after_inherents` hook and checking that it always executes. The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions and optional hooks when `Minimal` is returned. + +Security: Implementations need to be well audited before merge. + +Privacy: n/a + +## Performance, Ergonomics, and Compatibility + +### Performance + +The performance overhead is minimal in the sense that no clutter was added after fulfilling the requirements. A slight performance slow-down is expected from now additionally invoking `after_inherents` once per block. + +### Ergonomics + +The new interface allows for more extensible runtime logic. In the future this will be utilized for multi-block-migrations which should be a huge ergonomic advantage for parachain developers. + +### Compatibility + +Backwards compatibility can only be considered on the node side, since the runtime cannot be backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade user performance, it is recommended to check that an updated node can still import historic blocks. + +## Prior Art and References + +The RFC is currently being implemented in [substrate#14414](https://github.com/paritytech/substrate/pull/14414) (only the name of `BlockExecutiveMode` differs). + +Related issues and merge request: +- [Simple multi block migrations](https://github.com/paritytech/substrate/pull/14275) +- [Execute a hook after inherent but before transactions](https://github.com/paritytech/substrate/issues/9210) +- [There is no module hook after inherents and before transactions](https://github.com/paritytech/substrate/issues/5757) + + +## Unresolved Questions + +Please suggest a better name for `BlockExecutiveMode`. We already tried: `RuntimeExecutiveMode`, `ExtrinsicInclusionMode`. + +## Future Directions and Related Material + +An alternative approach to this is outlined [here](https://github.com/paritytech/substrate/pull/14279#discussion_r1226289311) by using an ordering of extrinsics. In this system all inherents would have negative priority and transactions positive priority. By then enforcing an order on them, there would be no hard differentiation between inherent and transaction for the block author anymore. That approach aims more at unifying the interplay between inherents and transactions, since the problem of communicating between runtime and block author on whether transactions should be included would not be solved by it. Further it also needs to invoke the `after_inherents` hook. + +I think it can therefore be done as future refactor to improve the code clarity and simplify the runtime logic. This RFC rather tries to prepare the runtime and block authors for a simple solution to the Multi-block migrations problem. From bb6a5e4f4c9dbc19d8ee4e4fe3c966915c78d36f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:15:06 +0200 Subject: [PATCH 02/19] Grammar Signed-off-by: Oliver Tale-Yazdi --- ...untime-api-block-builder-v5-and-core-v7.md | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-runtime-api-block-builder-v5-and-core-v7.md index 4ceb99929..7901b24c2 100644 --- a/text/0013-runtime-api-block-builder-v5-and-core-v7.md +++ b/text/0013-runtime-api-block-builder-v5-and-core-v7.md @@ -13,9 +13,9 @@ Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs and bu ## Motivation -The original motivation are Multi-Block-Migrations. They require the runtime to be able to tell the block builder that it should not attempt to include transactions in the current block. Currently there is no communication possible between runtime and block builder that could achieve this. Further it is necessary to execute logic right after inherent application but still before transaction inclusion. +The original motivation is Multi-Block-Migrations. They require the runtime to be able to tell the block builder that it should not attempt to include transactions in the current block. Currently, there is no communication possible between runtime and block builder that could achieve this. Further, it is necessary to execute logic right after inherent application but still before transaction inclusion. -This RFC proposes a way of communication between the runtime and the block builder by changing the `initialize_block` function to return a `BlockExecutiveMode` enum. Further an `after_inherents` function is introduced that runs after inherent but before transaction application. +This RFC proposes a way of communication between the runtime and the block builder by changing the `initialize_block` function to return a `BlockExecutiveMode` enum. Additionally, an `after_inherents` function is introduced that runs after inherent but before transaction application. ## Stakeholders @@ -25,7 +25,7 @@ This RFC proposes a way of communication between the runtime and the block build ## Explanation -All block authors MUST respect the `BlockExecutiveMode` that is returned by `initialize_block`. Further they MUST always invoke `after_inherents` directly after inherent application. The runtime MAY reject a block that violates either of these requirements. +All block authors MUST respect the `BlockExecutiveMode` that is returned by `initialize_block`. They MUST always invoke `after_inherents` directly after inherent application. The runtime MAY reject a block that violates either of these requirements. Backwards compatibility with the existing runtime API SHOULD be implemented in the authorship logic to not mandate a lockstep update on the node side. @@ -37,14 +37,14 @@ Enum `BlockExecutiveMode` has two variants: `Normal` and `Minimal`. […] drawbacks relating to performance, ergonomics, user experience, security, or privacy: None -Only drawback is that the block execution logic becomes more complicated. There is more room for error. -Additionally downstream developers need to adapt their code to this breaking change. +The only drawback is that the block execution logic becomes more complicated. There is more room for error. +Downstream developers will also need to adapt their code to this breaking change. ## Testing, Security, and Privacy Compliance of a block author can be tested by adding specific code to the `after_inherents` hook and checking that it always executes. The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions and optional hooks when `Minimal` is returned. -Security: Implementations need to be well audited before merge. +Security: Implementations need to be well audited before merging. Privacy: n/a @@ -56,17 +56,17 @@ The performance overhead is minimal in the sense that no clutter was added after ### Ergonomics -The new interface allows for more extensible runtime logic. In the future this will be utilized for multi-block-migrations which should be a huge ergonomic advantage for parachain developers. +The new interface allows for more extensible runtime logic. In the future, this will be utilized for multi-block-migrations which should be a huge ergonomic advantage for parachain developers. ### Compatibility -Backwards compatibility can only be considered on the node side, since the runtime cannot be backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade user performance, it is recommended to check that an updated node can still import historic blocks. +Backwards compatibility can only be considered on the node side since the runtime cannot be backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade user performance, it is recommended to check that an updated node can still import historic blocks. ## Prior Art and References The RFC is currently being implemented in [substrate#14414](https://github.com/paritytech/substrate/pull/14414) (only the name of `BlockExecutiveMode` differs). -Related issues and merge request: +Related issues and merge requests: - [Simple multi block migrations](https://github.com/paritytech/substrate/pull/14275) - [Execute a hook after inherent but before transactions](https://github.com/paritytech/substrate/issues/9210) - [There is no module hook after inherents and before transactions](https://github.com/paritytech/substrate/issues/5757) @@ -78,6 +78,6 @@ Please suggest a better name for `BlockExecutiveMode`. We already tried: `Runtim ## Future Directions and Related Material -An alternative approach to this is outlined [here](https://github.com/paritytech/substrate/pull/14279#discussion_r1226289311) by using an ordering of extrinsics. In this system all inherents would have negative priority and transactions positive priority. By then enforcing an order on them, there would be no hard differentiation between inherent and transaction for the block author anymore. That approach aims more at unifying the interplay between inherents and transactions, since the problem of communicating between runtime and block author on whether transactions should be included would not be solved by it. Further it also needs to invoke the `after_inherents` hook. +An alternative approach to this is outlined [here](https://github.com/paritytech/substrate/pull/14279#discussion_r1226289311) by using an ordering of extrinsics. In this system, all inherents would have negative priority and transactions positive priority. By then enforcing an order on them, there would be no hard differentiation between inherent and transaction for the block author anymore. That approach aims more at unifying the interplay between inherents and transactions, since the problem of communicating between runtime and block author on whether transactions should be included would not be solved by it. It also needs to invoke the `after_inherents` hook. -I think it can therefore be done as future refactor to improve the code clarity and simplify the runtime logic. This RFC rather tries to prepare the runtime and block authors for a simple solution to the Multi-block migrations problem. +I think it can therefore be done as a future refactor to improve the code clarity and simplify the runtime logic. This RFC rather tries to prepare the runtime and block authors for a simple solution to the Multi-block migrations problem. From 8816365610534b0e708a8c11868d6b054c5f0fd9 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:16:13 +0200 Subject: [PATCH 03/19] Line breaks Signed-off-by: Oliver Tale-Yazdi --- ...untime-api-block-builder-v5-and-core-v7.md | 77 ++++++++++++++----- 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-runtime-api-block-builder-v5-and-core-v7.md index 7901b24c2..74f81ceb1 100644 --- a/text/0013-runtime-api-block-builder-v5-and-core-v7.md +++ b/text/0013-runtime-api-block-builder-v5-and-core-v7.md @@ -8,41 +8,61 @@ ## Summary -Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs and bumps them to version 5 and 7 respectively. A new hook `after_inherents` is added to the `BlockBuilder` runtime API and the -`initialize_block` function of `Core` is changed to return an enum to indicate what extrinsics and hooks can be executed. +Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs and bumps them to version +5 and 7 respectively. A new hook `after_inherents` is added to the `BlockBuilder` runtime API and +the `initialize_block` function of `Core` is changed to return an enum to indicate what extrinsics +and hooks can be executed. ## Motivation -The original motivation is Multi-Block-Migrations. They require the runtime to be able to tell the block builder that it should not attempt to include transactions in the current block. Currently, there is no communication possible between runtime and block builder that could achieve this. Further, it is necessary to execute logic right after inherent application but still before transaction inclusion. +The original motivation is Multi-Block-Migrations. They require the runtime to be able to tell the +block builder that it should not attempt to include transactions in the current block. Currently, +there is no communication possible between runtime and block builder that could achieve this. +Further, it is necessary to execute logic right after inherent application but still before +transaction inclusion. -This RFC proposes a way of communication between the runtime and the block builder by changing the `initialize_block` function to return a `BlockExecutiveMode` enum. Additionally, an `after_inherents` function is introduced that runs after inherent but before transaction application. +This RFC proposes a way of communication between the runtime and the block builder by changing the +`initialize_block` function to return a `BlockExecutiveMode` enum. Additionally, an +`after_inherents` function is introduced that runs after inherent but before transaction +application. ## Stakeholders -- Substrate Maintainers: They will have to implement it upstream with all the imposed testing, audit and maintenance burden. +- Substrate Maintainers: They will have to implement it upstream with all the imposed testing, audit + and maintenance burden. - Polkadot Runtime developers: They will have to adapt to this breaking change. -- Polkadot Parachain Teams: They also need to adapt to the breaking changes but will eventually have multi-block migrations available. +- Polkadot Parachain Teams: They also need to adapt to the breaking changes but will eventually have + multi-block migrations available. ## Explanation -All block authors MUST respect the `BlockExecutiveMode` that is returned by `initialize_block`. They MUST always invoke `after_inherents` directly after inherent application. The runtime MAY reject a block that violates either of these requirements. +All block authors MUST respect the `BlockExecutiveMode` that is returned by `initialize_block`. They +MUST always invoke `after_inherents` directly after inherent application. The runtime MAY reject a +block that violates either of these requirements. -Backwards compatibility with the existing runtime API SHOULD be implemented in the authorship logic to not mandate a lockstep update on the node side. +Backwards compatibility with the existing runtime API SHOULD be implemented in the authorship logic +to not mandate a lockstep update on the node side. Enum `BlockExecutiveMode` has two variants: `Normal` and `Minimal`. -- `Normal` indicates that all user transactions can be included as usual and no special detour or skipping of logic happens. -- `Minimal` indicates that only hard-deadline code (ie. inherents, `on_initialize`...) SHALL run. Transactions MUST NOT be included. Optional hooks, like `poll` and `on_idle`, MOST NOT run either; even if there is still weight available for them. +- `Normal` indicates that all user transactions can be included as usual and no special detour or + skipping of logic happens. +- `Minimal` indicates that only hard-deadline code (ie. inherents, `on_initialize`...) SHALL run. + Transactions MUST NOT be included. Optional hooks, like `poll` and `on_idle`, MOST NOT run either; + even if there is still weight available for them. ## Drawbacks […] drawbacks relating to performance, ergonomics, user experience, security, or privacy: None -The only drawback is that the block execution logic becomes more complicated. There is more room for error. +The only drawback is that the block execution logic becomes more complicated. There is more room for +error. Downstream developers will also need to adapt their code to this breaking change. ## Testing, Security, and Privacy -Compliance of a block author can be tested by adding specific code to the `after_inherents` hook and checking that it always executes. The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions and optional hooks when `Minimal` is returned. +Compliance of a block author can be tested by adding specific code to the `after_inherents` hook and +checking that it always executes. The new logic of `initialize_block` can be tested by checking that +the block-builder will skip transactions and optional hooks when `Minimal` is returned. Security: Implementations need to be well audited before merging. @@ -52,7 +72,9 @@ Privacy: n/a ### Performance -The performance overhead is minimal in the sense that no clutter was added after fulfilling the requirements. A slight performance slow-down is expected from now additionally invoking `after_inherents` once per block. +The performance overhead is minimal in the sense that no clutter was added after fulfilling the +requirements. A slight performance slow-down is expected from now additionally invoking +`after_inherents` once per block. ### Ergonomics @@ -60,16 +82,22 @@ The new interface allows for more extensible runtime logic. In the future, this ### Compatibility -Backwards compatibility can only be considered on the node side since the runtime cannot be backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade user performance, it is recommended to check that an updated node can still import historic blocks. +Backwards compatibility can only be considered on the node side since the runtime cannot be +backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade +user performance, it is recommended to check that an updated node can still import historic blocks. ## Prior Art and References -The RFC is currently being implemented in [substrate#14414](https://github.com/paritytech/substrate/pull/14414) (only the name of `BlockExecutiveMode` differs). +The RFC is currently being implemented in +[substrate#14414](https://github.com/paritytech/substrate/pull/14414) (only the name of +`BlockExecutiveMode` differs). Related issues and merge requests: - [Simple multi block migrations](https://github.com/paritytech/substrate/pull/14275) -- [Execute a hook after inherent but before transactions](https://github.com/paritytech/substrate/issues/9210) -- [There is no module hook after inherents and before transactions](https://github.com/paritytech/substrate/issues/5757) +- [Execute a hook after inherent but before + transactions](https://github.com/paritytech/substrate/issues/9210) +- [There is no module hook after inherents and before + transactions](https://github.com/paritytech/substrate/issues/5757) ## Unresolved Questions @@ -78,6 +106,15 @@ Please suggest a better name for `BlockExecutiveMode`. We already tried: `Runtim ## Future Directions and Related Material -An alternative approach to this is outlined [here](https://github.com/paritytech/substrate/pull/14279#discussion_r1226289311) by using an ordering of extrinsics. In this system, all inherents would have negative priority and transactions positive priority. By then enforcing an order on them, there would be no hard differentiation between inherent and transaction for the block author anymore. That approach aims more at unifying the interplay between inherents and transactions, since the problem of communicating between runtime and block author on whether transactions should be included would not be solved by it. It also needs to invoke the `after_inherents` hook. - -I think it can therefore be done as a future refactor to improve the code clarity and simplify the runtime logic. This RFC rather tries to prepare the runtime and block authors for a simple solution to the Multi-block migrations problem. +An alternative approach to this is outlined +[here](https://github.com/paritytech/substrate/pull/14279#discussion_r1226289311) by using an +ordering of extrinsics. In this system, all inherents would have negative priority and transactions +positive priority. By then enforcing an order on them, there would be no hard differentiation +between inherent and transaction for the block author anymore. That approach aims more at unifying +the interplay between inherents and transactions, since the problem of communicating between runtime +and block author on whether transactions should be included would not be solved by it. It also needs +to invoke the `after_inherents` hook. + +I think it can therefore be done as a future refactor to improve the code clarity and simplify the +runtime logic. This RFC rather tries to prepare the runtime and block authors for a simple solution +to the Multi-block migrations problem. From b29577cd83aec5e0b5a2900b3ebae0b6043104f1 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:16:53 +0200 Subject: [PATCH 04/19] Grammar Signed-off-by: Oliver Tale-Yazdi --- text/0013-runtime-api-block-builder-v5-and-core-v7.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-runtime-api-block-builder-v5-and-core-v7.md index 74f81ceb1..a9dd0c4bd 100644 --- a/text/0013-runtime-api-block-builder-v5-and-core-v7.md +++ b/text/0013-runtime-api-block-builder-v5-and-core-v7.md @@ -64,7 +64,7 @@ Compliance of a block author can be tested by adding specific code to the `after checking that it always executes. The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions and optional hooks when `Minimal` is returned. -Security: Implementations need to be well audited before merging. +Security: Implementations need to be well-audited before merging. Privacy: n/a From 7a1a9dc72a029f59165c3aca2f559651295109cd Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:17:38 +0200 Subject: [PATCH 05/19] Proper description Signed-off-by: Oliver Tale-Yazdi --- text/0013-runtime-api-block-builder-v5-and-core-v7.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-runtime-api-block-builder-v5-and-core-v7.md index a9dd0c4bd..ccc525f64 100644 --- a/text/0013-runtime-api-block-builder-v5-and-core-v7.md +++ b/text/0013-runtime-api-block-builder-v5-and-core-v7.md @@ -3,7 +3,7 @@ | | | | --------------- | ------------------------------------------------------------------------------------------- | | **Start Date** | July 24, 2023 | -| **Description** | Changes to the BlockBuilder v5 and Core v7 Runtime APIs. | +| **Description** | Changes to the BlockBuilder v5 and Core v7 Runtime APIs to prepare for Multi-Block-Migrations. | | **Authors** | Oliver Tale-Yazdi | ## Summary From d720ed157a45814b8746c8bfd04e51c9e1e8b2b9 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:18:36 +0200 Subject: [PATCH 06/19] Proper title Signed-off-by: Oliver Tale-Yazdi --- text/0013-runtime-api-block-builder-v5-and-core-v7.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-runtime-api-block-builder-v5-and-core-v7.md index ccc525f64..9aa00914d 100644 --- a/text/0013-runtime-api-block-builder-v5-and-core-v7.md +++ b/text/0013-runtime-api-block-builder-v5-and-core-v7.md @@ -1,9 +1,9 @@ -# RFC-0013: Runtime API `BlockBuilder` v5 and `Core` v7 +# RFC-0013: Prepare `BlockBuilder` and `Core` runtime API for MBMs | | | | --------------- | ------------------------------------------------------------------------------------------- | | **Start Date** | July 24, 2023 | -| **Description** | Changes to the BlockBuilder v5 and Core v7 Runtime APIs to prepare for Multi-Block-Migrations. | +| **Description** | Prepares the `BlockBuilder` and `Core` Runtime APIs for Multi-Block-Migrations. | | **Authors** | Oliver Tale-Yazdi | ## Summary From 9921a330ff5d4833cbdf927d43c3f1601948fff6 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:19:19 +0200 Subject: [PATCH 07/19] Adapt filename Signed-off-by: Oliver Tale-Yazdi --- ...> 0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0013-runtime-api-block-builder-v5-and-core-v7.md => 0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md} (100%) diff --git a/text/0013-runtime-api-block-builder-v5-and-core-v7.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md similarity index 100% rename from text/0013-runtime-api-block-builder-v5-and-core-v7.md rename to text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md From c152ae181208cd94221dbe11145bb15005726772 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:22:31 +0200 Subject: [PATCH 08/19] Typo Signed-off-by: Oliver Tale-Yazdi --- .../0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 9aa00914d..2f19e8645 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -84,7 +84,7 @@ The new interface allows for more extensible runtime logic. In the future, this Backwards compatibility can only be considered on the node side since the runtime cannot be backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade -user performance, it is recommended to check that an updated node can still import historic blocks. +user experience, it is recommended to check that an updated node can still import historic blocks. ## Prior Art and References From f567276fd04d3616057a0bb613c82c51e368c36b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 24 Jul 2023 18:55:33 +0200 Subject: [PATCH 09/19] Ask for better naming Signed-off-by: Oliver Tale-Yazdi --- ...013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 2f19e8645..11ac33044 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -102,7 +102,9 @@ Related issues and merge requests: ## Unresolved Questions -Please suggest a better name for `BlockExecutiveMode`. We already tried: `RuntimeExecutiveMode`, `ExtrinsicInclusionMode`. +Please suggest a better name for `BlockExecutiveMode`. We already tried: `RuntimeExecutiveMode`, +`ExtrinsicInclusionMode`. The names of the modes `Normal` and `Minimal` were also called +`AllExtrinsics` and `OnlyInherents`, so if you have naming preferences; please post them. ## Future Directions and Related Material From 3d00c47cf61fe7b1481295040c0c799d73af23ba Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 25 Jul 2023 19:38:18 +0200 Subject: [PATCH 10/19] Rename to ExtrinsicInclusionMode Signed-off-by: Oliver Tale-Yazdi --- ...kbuilder-and-core-runtime-apis-for-mbms.md | 65 ++++++++++--------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 11ac33044..ccc5c6804 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -1,54 +1,57 @@ # RFC-0013: Prepare `BlockBuilder` and `Core` runtime API for MBMs -| | | -| --------------- | ------------------------------------------------------------------------------------------- | +| | | +| --------------- | --------------------------------------------------------------------------- | | **Start Date** | July 24, 2023 | -| **Description** | Prepares the `BlockBuilder` and `Core` Runtime APIs for Multi-Block-Migrations. | +| **Description** | Prepare the `BlockBuilder` and `Core` Runtime APIs for Multi-Block-Migrations.| | **Authors** | Oliver Tale-Yazdi | ## Summary Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs and bumps them to version -5 and 7 respectively. A new hook `after_inherents` is added to the `BlockBuilder` runtime API and -the `initialize_block` function of `Core` is changed to return an enum to indicate what extrinsics -and hooks can be executed. +5 and 7 respectively. A new function `after_inherents` is added to the `BlockBuilder` runtime API +and the `initialize_block` function of `Core` is changed to return an enum to indicate what +extrinsics can be applied. ## Motivation The original motivation is Multi-Block-Migrations. They require the runtime to be able to tell the block builder that it should not attempt to include transactions in the current block. Currently, there is no communication possible between runtime and block builder that could achieve this. -Further, it is necessary to execute logic right after inherent application but still before +Further, it is necessary to execute runtime logic right after inherent application but still before transaction inclusion. This RFC proposes a way of communication between the runtime and the block builder by changing the -`initialize_block` function to return a `BlockExecutiveMode` enum. Additionally, an +`initialize_block` function to return a `ExtrinsicInclusionMode` enum. Additionally, an `after_inherents` function is introduced that runs after inherent but before transaction application. ## Stakeholders -- Substrate Maintainers: They will have to implement it upstream with all the imposed testing, audit - and maintenance burden. +- Substrate Maintainers: They will have to implement this upstream with all the testing, audit and + maintenance burden. - Polkadot Runtime developers: They will have to adapt to this breaking change. -- Polkadot Parachain Teams: They also need to adapt to the breaking changes but will eventually have +- Polkadot Parachain Teams: They also have to adapt to the breaking changes but then eventually have multi-block migrations available. ## Explanation -All block authors MUST respect the `BlockExecutiveMode` that is returned by `initialize_block`. They -MUST always invoke `after_inherents` directly after inherent application. The runtime MAY reject a -block that violates either of these requirements. +The only relevant change is on the node side in the block authoring logic. Any further preparation +for MBMs can happen entirely on the runtime side with the provided primitives. -Backwards compatibility with the existing runtime API SHOULD be implemented in the authorship logic -to not mandate a lockstep update on the node side. +All block authors MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. +They MUST always invoke `after_inherents` directly after inherent application. The runtime MAY +reject a block that violates either of those requirements. -Enum `BlockExecutiveMode` has two variants: `Normal` and `Minimal`. -- `Normal` indicates that all user transactions can be included as usual and no special detour or - skipping of logic happens. -- `Minimal` indicates that only hard-deadline code (ie. inherents, `on_initialize`...) SHALL run. - Transactions MUST NOT be included. Optional hooks, like `poll` and `on_idle`, MOST NOT run either; - even if there is still weight available for them. +Enum `ExtrinsicInclusionMode` has two variants: +- `AllExtrinsics`: All extrinsics can be applied. It is the default behaviour prior to this RFC. +- `OnlyInherents`: Only inherents SHALL be applied by the block author. This differs from the + current behaviour by omitting transactions. + +It is RECOMMENDED that block authors keep transactions in the local transaction pool (if applicable) +for as long as `initialize_block` returns `OnlyInherents`. +Backwards compatibility with the current runtime API SHOULD be implemented on the node side to not +mandate a lockstep update. ## Drawbacks @@ -78,21 +81,20 @@ requirements. A slight performance slow-down is expected from now additionally i ### Ergonomics -The new interface allows for more extensible runtime logic. In the future, this will be utilized for multi-block-migrations which should be a huge ergonomic advantage for parachain developers. +The new interface allows for more extensible runtime logic. In the future, this will be utilized for +multi-block-migrations which should be a huge ergonomic advantage for parachain developers. ### Compatibility Backwards compatibility can only be considered on the node side since the runtime cannot be backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade -user experience, it is recommended to check that an updated node can still import historic blocks. +user experience, it is recommended to ensure that an updated node can still import historic blocks. ## Prior Art and References The RFC is currently being implemented in -[substrate#14414](https://github.com/paritytech/substrate/pull/14414) (only the name of -`BlockExecutiveMode` differs). - -Related issues and merge requests: +[substrate#14414](https://github.com/paritytech/substrate/pull/14414). Related issues and merge +requests: - [Simple multi block migrations](https://github.com/paritytech/substrate/pull/14275) - [Execute a hook after inherent but before transactions](https://github.com/paritytech/substrate/issues/9210) @@ -102,9 +104,12 @@ Related issues and merge requests: ## Unresolved Questions -Please suggest a better name for `BlockExecutiveMode`. We already tried: `RuntimeExecutiveMode`, +~~Please suggest a better name for `BlockExecutiveMode`. We already tried: `RuntimeExecutiveMode`, `ExtrinsicInclusionMode`. The names of the modes `Normal` and `Minimal` were also called -`AllExtrinsics` and `OnlyInherents`, so if you have naming preferences; please post them. +`AllExtrinsics` and `OnlyInherents`, so if you have naming preferences; please post them.~~ +=> Resolved by using `ExtrinsicInclusionMode`. + +Is `post_inherents` more consistent instead of `after_inherents`? Then we should change it. ## Future Directions and Related Material From 24ca6514409d135b979c484b651e940658a75d25 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 31 Oct 2023 12:46:46 +0100 Subject: [PATCH 11/19] Clarify Signed-off-by: Oliver Tale-Yazdi --- ...kbuilder-and-core-runtime-apis-for-mbms.md | 105 +++++++++--------- 1 file changed, 52 insertions(+), 53 deletions(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index ccc5c6804..721b0c408 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -8,64 +8,74 @@ ## Summary -Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs and bumps them to version -5 and 7 respectively. A new function `after_inherents` is added to the `BlockBuilder` runtime API -and the `initialize_block` function of `Core` is changed to return an enum to indicate what -extrinsics can be applied. +Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs. +A new function `BlockBuilder::last_inherent` is introduced and the return value of `Core::initialize_block` is changed from void to an enum. +The versions of both API are bumped; `BlockBuilder` to 7 and `Core` to 5. ## Motivation -The original motivation is Multi-Block-Migrations. They require the runtime to be able to tell the -block builder that it should not attempt to include transactions in the current block. Currently, -there is no communication possible between runtime and block builder that could achieve this. -Further, it is necessary to execute runtime logic right after inherent application but still before -transaction inclusion. +There are three main motivations for this RFC: +1. Multi-Block-Migrations: These make it possible to split a migration over multiple blocks. +2. Pallet `poll` hook: Can be used to gradually replace `on_initialize`/`on_finalize` in places where the code does not need to be by a hard deadline. +3. New callback `System::PostInherents`: Can replace `on_initialize`/`on_finalize` where a hard deadline is required (complements `poll`). -This RFC proposes a way of communication between the runtime and the block builder by changing the -`initialize_block` function to return a `ExtrinsicInclusionMode` enum. Additionally, an -`after_inherents` function is introduced that runs after inherent but before transaction -application. +The three motivations can be implemented when fulfilling these two requirements: +1. The runtime can tell the block author to not include any transactions in the block. +2. The runtime can execute logic right after all pallet-provided inherents have been applied. ## Stakeholders -- Substrate Maintainers: They will have to implement this upstream with all the testing, audit and +- Substrate Maintainers: They have to implement this, including tests, audit and maintenance burden. -- Polkadot Runtime developers: They will have to adapt to this breaking change. +- Polkadot Runtime developers: They will have to adapt the runtime files to this breaking change. - Polkadot Parachain Teams: They also have to adapt to the breaking changes but then eventually have multi-block migrations available. ## Explanation -The only relevant change is on the node side in the block authoring logic. Any further preparation -for MBMs can happen entirely on the runtime side with the provided primitives. -All block authors MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. -They MUST always invoke `after_inherents` directly after inherent application. The runtime MAY -reject a block that violates either of those requirements. +### `Core::initialize_block` -Enum `ExtrinsicInclusionMode` has two variants: -- `AllExtrinsics`: All extrinsics can be applied. It is the default behaviour prior to this RFC. -- `OnlyInherents`: Only inherents SHALL be applied by the block author. This differs from the - current behaviour by omitting transactions. +This runtime API function is changed from returning `()` to `ExtrinsicInclusionMode`: +```rust +enum ExtrinsicInclusionMode { + /// All extrinsics are allowed in this block. + AllExtrinsics, + /// Only inherents are allowed in this block. + OnlyInherents, +} +``` -It is RECOMMENDED that block authors keep transactions in the local transaction pool (if applicable) +A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MAY reject blocks that violate this requirement. + +It is RECOMMENDED that block authors keep transactions in their transaction pool (if applicable) for as long as `initialize_block` returns `OnlyInherents`. -Backwards compatibility with the current runtime API SHOULD be implemented on the node side to not -mandate a lockstep update. +Backwards compatibility with the current runtime API SHOULD be implemented by the block author to not mandate a lockstep update of the authoring software. +This could be achieved by checking the runtime API version and assuming that `initialize_block` does not have a return value when the version is lower than 7. -## Drawbacks +### `BlockBuilder::last_inherent` + +A block author MUST always invoke `last_inherent` directly after applying all runtime-provided inherents. The runtime MAY reject blocks that violate this requirement. + +### Combined + +Coming back to the three main motivations and how they can be implemented with these runtime APIs changes: -[…] drawbacks relating to performance, ergonomics, user experience, security, or privacy: None +**1. Multi-Block-Migrations**: The runtime is being put into lock-down mode for the duration of the migration process by returning `OnlyInherents` from `initialize_block`. This ensures that no user provided transactions can interfere with the migration process. It is absolutely necessary to ensure this since otherwise a transaction could call into un-migrated storage and violate storage invariants. The entry-point for the MBM logic is `last_inherent`. This is a good spot since any data that is touched in inherents is not MBM-migratable anyway. It could also be done before all other inherents or at the end of the block in `finalize_block`, but there is no downside from doing it in `last_inherent` and the other two motivations are in favour of this. + +**2. `poll`** becomes possible by using `last_inherent` as entry-point. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons. First is that pallets do not have access to `AllPalletsWithSystem` that is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce the order or inherents. + +**3. `System::PostInherents`** can be done in the same manner as `poll`. + +## Drawbacks -The only drawback is that the block execution logic becomes more complicated. There is more room for -error. -Downstream developers will also need to adapt their code to this breaking change. +As noted in the review comments: this cements some assumptions about the order of inherents into the `BlockBuilder` traits. It was criticized for being to rigid in its assumptions. ## Testing, Security, and Privacy -Compliance of a block author can be tested by adding specific code to the `after_inherents` hook and +Compliance of a block author can be tested by adding specific code to the `last_inherent` hook and checking that it always executes. The new logic of `initialize_block` can be tested by checking that -the block-builder will skip transactions and optional hooks when `Minimal` is returned. +the block-builder will skip transactions and optional hooks when `OnlyInherents` is returned. Security: Implementations need to be well-audited before merging. @@ -77,7 +87,7 @@ Privacy: n/a The performance overhead is minimal in the sense that no clutter was added after fulfilling the requirements. A slight performance slow-down is expected from now additionally invoking -`after_inherents` once per block. +`last_inherent` once per block. ### Ergonomics @@ -86,14 +96,12 @@ multi-block-migrations which should be a huge ergonomic advantage for parachain ### Compatibility -Backwards compatibility can only be considered on the node side since the runtime cannot be -backwards compatible in any way. The advice here is OPTIONAL and outside of the RFC. To not degrade +The advice here is OPTIONAL and outside of the RFC. To not degrade user experience, it is recommended to ensure that an updated node can still import historic blocks. ## Prior Art and References -The RFC is currently being implemented in -[substrate#14414](https://github.com/paritytech/substrate/pull/14414). Related issues and merge +The RFC is currently being implemented in [polkadot-sdk#1781](https://github.com/paritytech/polkadot-sdk/pull/1781). Related issues and merge requests: - [Simple multi block migrations](https://github.com/paritytech/substrate/pull/14275) - [Execute a hook after inherent but before @@ -107,21 +115,12 @@ requests: ~~Please suggest a better name for `BlockExecutiveMode`. We already tried: `RuntimeExecutiveMode`, `ExtrinsicInclusionMode`. The names of the modes `Normal` and `Minimal` were also called `AllExtrinsics` and `OnlyInherents`, so if you have naming preferences; please post them.~~ -=> Resolved by using `ExtrinsicInclusionMode`. +=> renamed to `ExtrinsicInclusionMode` -Is `post_inherents` more consistent instead of `after_inherents`? Then we should change it. +~~Is `post_inherents` more consistent instead of `last_inherent`? Then we should change it.~~ +=> renamed to `last_inherent` ## Future Directions and Related Material -An alternative approach to this is outlined -[here](https://github.com/paritytech/substrate/pull/14279#discussion_r1226289311) by using an -ordering of extrinsics. In this system, all inherents would have negative priority and transactions -positive priority. By then enforcing an order on them, there would be no hard differentiation -between inherent and transaction for the block author anymore. That approach aims more at unifying -the interplay between inherents and transactions, since the problem of communicating between runtime -and block author on whether transactions should be included would not be solved by it. It also needs -to invoke the `after_inherents` hook. - -I think it can therefore be done as a future refactor to improve the code clarity and simplify the -runtime logic. This RFC rather tries to prepare the runtime and block authors for a simple solution -to the Multi-block migrations problem. +The long-term future here is to move the block building logic into the runtime. Currently there is a tight dance between the block author and the runtime; the author has to call into different runtime functions in quick succession and exact order. Any misstep causes the built block to be invalid. +This can be unified and simplified by moving both parts of the logic into the runtime. From 35522b64465626f6b7bb29b41faf721abc33d0a2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 31 Oct 2023 13:00:56 +0100 Subject: [PATCH 12/19] Typos Signed-off-by: Oliver Tale-Yazdi --- ...kbuilder-and-core-runtime-apis-for-mbms.md | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 721b0c408..a9e68e473 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -1,25 +1,25 @@ -# RFC-0013: Prepare `BlockBuilder` and `Core` runtime API for MBMs +# RFC-0013: Prepare `BlockBuilder` and `Core` runtime APIs for MBMs | | | | --------------- | --------------------------------------------------------------------------- | | **Start Date** | July 24, 2023 | -| **Description** | Prepare the `BlockBuilder` and `Core` Runtime APIs for Multi-Block-Migrations.| +| **Description** | Prepare the `BlockBuilder` and `Core` Runtime APIs for Multi-Block-Migrations. | | **Authors** | Oliver Tale-Yazdi | ## Summary Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs. -A new function `BlockBuilder::last_inherent` is introduced and the return value of `Core::initialize_block` is changed from void to an enum. -The versions of both API are bumped; `BlockBuilder` to 7 and `Core` to 5. +A new function `BlockBuilder::last_inherent` is introduced and the return value of `Core::initialize_block` is changed to an enum. +The versions of both APIs are bumped; `BlockBuilder` to 7 and `Core` to 5. ## Motivation There are three main motivations for this RFC: 1. Multi-Block-Migrations: These make it possible to split a migration over multiple blocks. -2. Pallet `poll` hook: Can be used to gradually replace `on_initialize`/`on_finalize` in places where the code does not need to be by a hard deadline. -3. New callback `System::PostInherents`: Can replace `on_initialize`/`on_finalize` where a hard deadline is required (complements `poll`). +2. Pallet `poll` hook: Can be used to gradually replace `on_initialize`/`on_finalize` in places where the code does not need to run by a hard deadline, since it is not guaranteed to execute each block. +3. New callback `System::PostInherents`: Can replace `on_initialize`/`on_finalize` where a hard deadline is required (complements `poll`). It is guaranteed to execute each block. -The three motivations can be implemented when fulfilling these two requirements: +These three features can be implemented when fulfilling these two requirements: 1. The runtime can tell the block author to not include any transactions in the block. 2. The runtime can execute logic right after all pallet-provided inherents have been applied. @@ -49,8 +49,8 @@ enum ExtrinsicInclusionMode { A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MAY reject blocks that violate this requirement. It is RECOMMENDED that block authors keep transactions in their transaction pool (if applicable) -for as long as `initialize_block` returns `OnlyInherents`. -Backwards compatibility with the current runtime API SHOULD be implemented by the block author to not mandate a lockstep update of the authoring software. +for as long as `initialize_block` returns `OnlyInherents`. The assumption is that these transactions become valid once the runtime finishes the MBM. +Backwards compatibility with the current runtime API SHOULD be implemented by block authors to not mandate a lockstep update of the authoring software. This could be achieved by checking the runtime API version and assuming that `initialize_block` does not have a return value when the version is lower than 7. ### `BlockBuilder::last_inherent` @@ -59,11 +59,11 @@ A block author MUST always invoke `last_inherent` directly after applying all ru ### Combined -Coming back to the three main motivations and how they can be implemented with these runtime APIs changes: +Coming back to the three main features and how they can be implemented with these runtime APIs changes: -**1. Multi-Block-Migrations**: The runtime is being put into lock-down mode for the duration of the migration process by returning `OnlyInherents` from `initialize_block`. This ensures that no user provided transactions can interfere with the migration process. It is absolutely necessary to ensure this since otherwise a transaction could call into un-migrated storage and violate storage invariants. The entry-point for the MBM logic is `last_inherent`. This is a good spot since any data that is touched in inherents is not MBM-migratable anyway. It could also be done before all other inherents or at the end of the block in `finalize_block`, but there is no downside from doing it in `last_inherent` and the other two motivations are in favour of this. +**1. Multi-Block-Migrations**: The runtime is being put into lock-down mode for the duration of the migration process by returning `OnlyInherents` from `initialize_block`. This ensures that no user provided transaction can interfere with the migration process. It is absolutely necessary to ensure this, since otherwise a transaction could call into un-migrated storage and violate storage invariants. The entry-point for the MBM logic is `last_inherent`. This is a good spot, because any data that is touched in inherents, is not MBM-migratable anyway. It could also be done before all other inherents or at the end of the block in `finalize_block`, but there is no downside from doing it in `last_inherent` and the other two features are in favour of this. -**2. `poll`** becomes possible by using `last_inherent` as entry-point. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons. First is that pallets do not have access to `AllPalletsWithSystem` that is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce the order or inherents. +**2. `poll`** becomes possible by using `last_inherent` as entry-point. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons. First is that pallets do not have access to `AllPalletsWithSystem` that is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce an order of inherents. **3. `System::PostInherents`** can be done in the same manner as `poll`. @@ -77,7 +77,7 @@ Compliance of a block author can be tested by adding specific code to the `last_ checking that it always executes. The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions and optional hooks when `OnlyInherents` is returned. -Security: Implementations need to be well-audited before merging. +Security: n/a Privacy: n/a @@ -86,7 +86,7 @@ Privacy: n/a ### Performance The performance overhead is minimal in the sense that no clutter was added after fulfilling the -requirements. A slight performance slow-down is expected from now additionally invoking +requirements. A slight performance penalty is expected from invoking `last_inherent` once per block. ### Ergonomics @@ -117,7 +117,7 @@ requests: `AllExtrinsics` and `OnlyInherents`, so if you have naming preferences; please post them.~~ => renamed to `ExtrinsicInclusionMode` -~~Is `post_inherents` more consistent instead of `last_inherent`? Then we should change it.~~ +~~Is `post_inherents` more consistent instead of `last_inherent`? Then we should change it.~~ => renamed to `last_inherent` ## Future Directions and Related Material From d95da32cbed64a7b3195ba7c1c83327bf3a8afec Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 31 Oct 2023 13:02:17 +0100 Subject: [PATCH 13/19] Typos Signed-off-by: Oliver Tale-Yazdi --- .../0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index a9e68e473..0374d867f 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -14,7 +14,7 @@ The versions of both APIs are bumped; `BlockBuilder` to 7 and `Core` to 5. ## Motivation -There are three main motivations for this RFC: +There are three main features that motivate for this RFC: 1. Multi-Block-Migrations: These make it possible to split a migration over multiple blocks. 2. Pallet `poll` hook: Can be used to gradually replace `on_initialize`/`on_finalize` in places where the code does not need to run by a hard deadline, since it is not guaranteed to execute each block. 3. New callback `System::PostInherents`: Can replace `on_initialize`/`on_finalize` where a hard deadline is required (complements `poll`). It is guaranteed to execute each block. From e27f40271e453342bc0851b305c29ae7276c7421 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 14 Nov 2023 13:59:35 +0100 Subject: [PATCH 14/19] Update text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- .../0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 0374d867f..89a06b2ca 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -55,7 +55,7 @@ This could be achieved by checking the runtime API version and assuming that `in ### `BlockBuilder::last_inherent` -A block author MUST always invoke `last_inherent` directly after applying all runtime-provided inherents. The runtime MAY reject blocks that violate this requirement. +A block author MUST always invoke `last_inherent` directly after applying all runtime-provided inherents. The runtime MUST reject blocks that violate this requirement. ### Combined From dbad548628fddb377070fb1041c1aa63027b88d2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 14 Nov 2023 14:06:04 +0100 Subject: [PATCH 15/19] Use MUST Signed-off-by: Oliver Tale-Yazdi --- .../0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 89a06b2ca..aa7b978a4 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -46,7 +46,7 @@ enum ExtrinsicInclusionMode { } ``` -A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MAY reject blocks that violate this requirement. +A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that do have forbidden extrinsics in them. It is RECOMMENDED that block authors keep transactions in their transaction pool (if applicable) for as long as `initialize_block` returns `OnlyInherents`. The assumption is that these transactions become valid once the runtime finishes the MBM. From 33ae12f2343e386cb653185f0faf187797ab2eed Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 14 Nov 2023 14:17:12 +0100 Subject: [PATCH 16/19] Remove paragraph Signed-off-by: Oliver Tale-Yazdi --- ...13-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index aa7b978a4..561a9a1d2 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -48,11 +48,6 @@ enum ExtrinsicInclusionMode { A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that do have forbidden extrinsics in them. -It is RECOMMENDED that block authors keep transactions in their transaction pool (if applicable) -for as long as `initialize_block` returns `OnlyInherents`. The assumption is that these transactions become valid once the runtime finishes the MBM. -Backwards compatibility with the current runtime API SHOULD be implemented by block authors to not mandate a lockstep update of the authoring software. -This could be achieved by checking the runtime API version and assuming that `initialize_block` does not have a return value when the version is lower than 7. - ### `BlockBuilder::last_inherent` A block author MUST always invoke `last_inherent` directly after applying all runtime-provided inherents. The runtime MUST reject blocks that violate this requirement. From f72c2b60886895b058ff0fc94bc37a028173b83f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 2 Feb 2024 19:18:28 +0100 Subject: [PATCH 17/19] Slim down Signed-off-by: Oliver Tale-Yazdi --- ...kbuilder-and-core-runtime-apis-for-mbms.md | 52 +++++++------------ 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index 561a9a1d2..f75f237d8 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -1,34 +1,27 @@ -# RFC-0013: Prepare `BlockBuilder` and `Core` runtime APIs for MBMs +# RFC-0013: Prepare `Core` runtime API for MBMs | | | | --------------- | --------------------------------------------------------------------------- | | **Start Date** | July 24, 2023 | -| **Description** | Prepare the `BlockBuilder` and `Core` Runtime APIs for Multi-Block-Migrations. | +| **Description** | Prepare the `Core` Runtime API for Multi-Block-Migrations | | **Authors** | Oliver Tale-Yazdi | ## Summary -Introduces breaking changes to the `BlockBuilder` and `Core` runtime APIs. -A new function `BlockBuilder::last_inherent` is introduced and the return value of `Core::initialize_block` is changed to an enum. -The versions of both APIs are bumped; `BlockBuilder` to 7 and `Core` to 5. +Introduces breaking changes to the `Core` runtime API by letting `Core::initialize_block` return an enum. The versions of `Core` is bumped from 4 to 5. ## Motivation -There are three main features that motivate for this RFC: -1. Multi-Block-Migrations: These make it possible to split a migration over multiple blocks. -2. Pallet `poll` hook: Can be used to gradually replace `on_initialize`/`on_finalize` in places where the code does not need to run by a hard deadline, since it is not guaranteed to execute each block. -3. New callback `System::PostInherents`: Can replace `on_initialize`/`on_finalize` where a hard deadline is required (complements `poll`). It is guaranteed to execute each block. - -These three features can be implemented when fulfilling these two requirements: -1. The runtime can tell the block author to not include any transactions in the block. -2. The runtime can execute logic right after all pallet-provided inherents have been applied. +The main feature that motivates this RFC are Multi-Block-Migrations (MBM); these make it possible to split a migration over multiple blocks. +Further it would be nice to not hinder the possibility of implementing a new hook `poll`, that runs at the beginning of the block when there are no MBMs and has access to `AllPalletsWithSystem`. This hook can then be used to replace the use of `on_initialize` and `on_finalize` for non-deadline critical logic. +In a similar fashion, it should not hinder the future addition of a `System::PostInherents` callback that always runs after all inherents were applied. ## Stakeholders - Substrate Maintainers: They have to implement this, including tests, audit and maintenance burden. - Polkadot Runtime developers: They will have to adapt the runtime files to this breaking change. -- Polkadot Parachain Teams: They also have to adapt to the breaking changes but then eventually have +- Polkadot Parachain Teams: They have to adapt to the breaking changes but then eventually have multi-block migrations available. ## Explanation @@ -46,31 +39,23 @@ enum ExtrinsicInclusionMode { } ``` -A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that do have forbidden extrinsics in them. - -### `BlockBuilder::last_inherent` - -A block author MUST always invoke `last_inherent` directly after applying all runtime-provided inherents. The runtime MUST reject blocks that violate this requirement. - -### Combined +A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that have extrinsics in them when `OnlyInherents` was returned. -Coming back to the three main features and how they can be implemented with these runtime APIs changes: +Coming back to the motivations and how they can be implemented with this runtime API change: -**1. Multi-Block-Migrations**: The runtime is being put into lock-down mode for the duration of the migration process by returning `OnlyInherents` from `initialize_block`. This ensures that no user provided transaction can interfere with the migration process. It is absolutely necessary to ensure this, since otherwise a transaction could call into un-migrated storage and violate storage invariants. The entry-point for the MBM logic is `last_inherent`. This is a good spot, because any data that is touched in inherents, is not MBM-migratable anyway. It could also be done before all other inherents or at the end of the block in `finalize_block`, but there is no downside from doing it in `last_inherent` and the other two features are in favour of this. +**1. Multi-Block-Migrations**: The runtime is being put into lock-down mode for the duration of the migration process by returning `OnlyInherents` from `initialize_block`. This ensures that no user provided transaction can interfere with the migration process. It is absolutely necessary to ensure this, otherwise a transaction could call into un-migrated storage and violate storage invariants. -**2. `poll`** becomes possible by using `last_inherent` as entry-point. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons. First is that pallets do not have access to `AllPalletsWithSystem` that is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce an order of inherents. +**2. `poll`** is possible by using `apply_extrinsic` as entry-point and not hindered by this approach. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons. First is that pallets do not have access to `AllPalletsWithSystem` that is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce an order of inherents. **3. `System::PostInherents`** can be done in the same manner as `poll`. ## Drawbacks -As noted in the review comments: this cements some assumptions about the order of inherents into the `BlockBuilder` traits. It was criticized for being to rigid in its assumptions. +The previous drawback of cementing the order of inherents has been addressed and removed by redesigning the approach. No further drawbacks have been identified. ## Testing, Security, and Privacy -Compliance of a block author can be tested by adding specific code to the `last_inherent` hook and -checking that it always executes. The new logic of `initialize_block` can be tested by checking that -the block-builder will skip transactions and optional hooks when `OnlyInherents` is returned. +The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions when `OnlyInherents` is returned. The runtime should also not call any optional hooks like `on_idle`. Security: n/a @@ -81,8 +66,7 @@ Privacy: n/a ### Performance The performance overhead is minimal in the sense that no clutter was added after fulfilling the -requirements. A slight performance penalty is expected from invoking -`last_inherent` once per block. +requirements. The only performance difference is that `initialize_block` also returns an enum that needs to be passed through the WASM boundary. This should be negligible. ### Ergonomics @@ -96,7 +80,7 @@ user experience, it is recommended to ensure that an updated node can still impo ## Prior Art and References -The RFC is currently being implemented in [polkadot-sdk#1781](https://github.com/paritytech/polkadot-sdk/pull/1781). Related issues and merge +The RFC is currently being implemented in [polkadot-sdk#1781](https://github.com/paritytech/polkadot-sdk/pull/1781) (formerly [substrate#14275](https://github.com/paritytech/substrate/pull/14275)). Related issues and merge requests: - [Simple multi block migrations](https://github.com/paritytech/substrate/pull/14275) - [Execute a hook after inherent but before @@ -113,9 +97,9 @@ requests: => renamed to `ExtrinsicInclusionMode` ~~Is `post_inherents` more consistent instead of `last_inherent`? Then we should change it.~~ -=> renamed to `last_inherent` +~~=> renamed to `last_inherent`~~ ## Future Directions and Related Material -The long-term future here is to move the block building logic into the runtime. Currently there is a tight dance between the block author and the runtime; the author has to call into different runtime functions in quick succession and exact order. Any misstep causes the built block to be invalid. -This can be unified and simplified by moving both parts of the logic into the runtime. +The long-term future here is to move the block building logic into the runtime. Currently there is a tight dance between the block author and the runtime; the author has to call into different runtime functions in quick succession and exact order. Any misstep causes the block to be invalid. +This can be unified and simplified by moving both parts into the runtime. From d4329c544de0ef635c2c51ce2421e16e18e22858 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 6 Feb 2024 16:53:20 +0100 Subject: [PATCH 18/19] Small fixes Signed-off-by: Oliver Tale-Yazdi --- ...lockbuilder-and-core-runtime-apis-for-mbms.md | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index f75f237d8..b8b4cc9cb 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -30,6 +30,14 @@ In a similar fashion, it should not hinder the future addition of a `System::Pos ### `Core::initialize_block` This runtime API function is changed from returning `()` to `ExtrinsicInclusionMode`: + +```patch +fn initialize_block(header: &::Header) ++ -> ExtrinsicInclusionMode; +``` + +With `ExtrinsicInclusionMode` is defined as: + ```rust enum ExtrinsicInclusionMode { /// All extrinsics are allowed in this block. @@ -39,23 +47,23 @@ enum ExtrinsicInclusionMode { } ``` -A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that have extrinsics in them when `OnlyInherents` was returned. +A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that have extrinsics in them while `OnlyInherents` was returned. Coming back to the motivations and how they can be implemented with this runtime API change: **1. Multi-Block-Migrations**: The runtime is being put into lock-down mode for the duration of the migration process by returning `OnlyInherents` from `initialize_block`. This ensures that no user provided transaction can interfere with the migration process. It is absolutely necessary to ensure this, otherwise a transaction could call into un-migrated storage and violate storage invariants. -**2. `poll`** is possible by using `apply_extrinsic` as entry-point and not hindered by this approach. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons. First is that pallets do not have access to `AllPalletsWithSystem` that is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce an order of inherents. +**2. `poll`** is possible by using `apply_extrinsic` as entry-point and not hindered by this approach. It would not be possible to use a pallet inherent like `System::last_inherent` to achieve this for two reasons: First is that pallets do not have access to `AllPalletsWithSystem` which is required to invoke the `poll` hook on all pallets. Second is that the runtime does currently not enforce an order of inherents. **3. `System::PostInherents`** can be done in the same manner as `poll`. ## Drawbacks -The previous drawback of cementing the order of inherents has been addressed and removed by redesigning the approach. No further drawbacks have been identified. +The previous drawback of cementing the order of inherents has been addressed and removed by redesigning the approach. No further drawbacks have been identified thus far. ## Testing, Security, and Privacy -The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions when `OnlyInherents` is returned. The runtime should also not call any optional hooks like `on_idle`. +The new logic of `initialize_block` can be tested by checking that the block-builder will skip transactions when `OnlyInherents` is returned. Security: n/a From e2769ade76f2622ccd9d46ff6c59938f51677447 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 16 Feb 2024 21:55:29 +0100 Subject: [PATCH 19/19] Update text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- .../0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md index b8b4cc9cb..680411cd2 100644 --- a/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md +++ b/text/0013-prepare-blockbuilder-and-core-runtime-apis-for-mbms.md @@ -47,7 +47,7 @@ enum ExtrinsicInclusionMode { } ``` -A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that have extrinsics in them while `OnlyInherents` was returned. +A block author MUST respect the `ExtrinsicInclusionMode` that is returned by `initialize_block`. The runtime MUST reject blocks that have non-inherent extrinsics in them while `OnlyInherents` was returned. Coming back to the motivations and how they can be implemented with this runtime API change: