Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix order of hook execution #10043

Merged
merged 16 commits into from
Dec 1, 2021
Merged

Fix order of hook execution #10043

merged 16 commits into from
Dec 1, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Oct 18, 2021 •
edited
Loading

Fix #6280
Fix #9105

polkadot companion: paritytech/polkadot#4181
cumulus companion: paritytech/cumulus#711

Breaking Change:

  • AllPallets is deprecated in favor of explicit: AllPalletsWithSystem, AllPalletsWithoutSystem, AllPalletsWithSystemReversed, AllPalletsWithoutSystemReversed. Before this PR AllPallets represented all pallets without system in reversed order (i.e. same as AllPalletsWithoutSystemReversed, it now represent AllPalletsWithSystem as it is a more sane expectation.
  • frame_executive::Executive requires as generic all pallets with system. Before this PR the type required as generic all pallets without system and always executed system pallet hook before all pallets hook. But this requires duplicate works and error prone implementation thus it now requires all pallets with system. The previous order was to execute the hook on system pallet first and then AllPallets. It is recomended to use one of AllPalletsWithSystem or AllPalletsWithSystemReversed or AllPalletsReversedWithSystemFirst.

Because before the PR all pallets represented all pallets in reversed, and system was executed first then if someone want to keep the same behavior as before they should use the type AllPalletsReversedWithSystemFirst when using the type frame_executive::Executive.
Though it is recommended to be careful about the order of definition of the pallet in construct_runtime. and probably reorganize and use AllPalletsWithSystem, so the order is the expected normal order and not in reversed (as before this PR).

NOTE: frame_executive::Executive will execute the hooks on_initialize, on_idle, on_runtime_upgrade, on_finalize, offchain_worker on the generic AllPallets, so the order should be reviewed for those execution.

Upgrade guide:

  1. use AllPalletsReversedWithSystemFirst when instancing executive type, that mean the execution order is not changed from before this PR, thus no need to change anything in construct_runtime.

  2. Use the new AllPalletsWithSystem, but this would mean that they might need to reorder some stuff in construct_runtime.

  3. review other usage of AllPallets and change to AllPalletsWithoutSystemReversed in case the old order must be kept, or change to AllPalletsWithoutSystem to get the new definition with pallets defined in the order of their declaration in construct_runtime.

@gui1117 gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 18, 2021
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 18, 2021
@gui1117 gui1117 mentioned this pull request Oct 18, 2021
Closed
gui1117 and others added 2 commits October 18, 2021 11:05
@gui1117 @apopiak
Update frame/support/procedural/src/construct_runtime/mod.rs
8486f82 
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
@gui1117
format
7a1e047
@gui1117
Copy link
Contributor Author

gui1117 commented Oct 18, 2021

for the polkadot It seems we don't need any changes:
maybe scheduler should be put last instead of 2nd. So that the execution of scheduled call happens after other pallet initialization ?

@gui1117 gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Oct 19, 2021
@gui1117 gui1117 mentioned this pull request Oct 19, 2021
Merged
@gui1117 gui1117 mentioned this pull request Oct 30, 2021
Closed
This was referenced Oct 30, 2021
Merged
Merged
shawntabrizi
shawntabrizi reviewed Oct 30, 2021
View reviewed changes
frame/executive/src/lib.rs Outdated
OnRuntimeUpgrade
>::pre_upgrade().unwrap();

<(COnRuntimeUpgrade, AllPallets) as OnRuntimeUpgrade>::pre_upgrade().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is also a noteworthy change. Here system pallet upgrades after the runtime upgrades.

Dunno actually if this can cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for pre_upgrade/post_upgrade the order wasn't so important as it is just checks.
But the order of on_runtime_upgrade is important. That said the order was: custom, then system then pallets.
I will precise this in the note

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try_on_runtime_upgrade should always imitate what the real on_runtime_upgrade would do, which is respected here, so no issues.

@ferrell-code ferrell-code mentioned this pull request Dec 28, 2021
Closed
This was referenced Jan 5, 2022
Merged
Merged
brenzi pushed a commit to integritee-network/integritee-node that referenced this pull request Jan 6, 2022
@gaudenzkessler @gkessler-scs
Update substrate to b9cafba (#108)
d9454f9 
- The substrate verion for sp-core was updated to 4.1.0-dev.
- The toolchain was updated to nightly-2021-12-10 due to incompatibilities with the toolchain nightly-2021-12-22
- "AllPallets" was replaces by AllPalletsReversedWithSystemFirst according to paritytech/substrate#10043

Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>
brenzi pushed a commit to integritee-network/sgx-runtime that referenced this pull request Jan 6, 2022
@gaudenzkessler @gkessler-scs
Update substrate to b9cafba (#44)
e998d6e 
- The substrate verion for sp-core was updated to 4.1.0-dev.
- "AllPallets" was replaces by AllPalletsReversedWithSystemFirst according to Fix order of hook execution paritytech/substrate#10043

Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>
chevdor pushed a commit that referenced this pull request Jan 11, 2022
@gui1117 @apopiak @chevdor
Fix order of hook execution (#10043)
d36550a 
* fix order

* Update frame/support/procedural/src/construct_runtime/mod.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* format

* more accurate description

* format

* better explicit types

* fix tests

* address feedback

* add a type to ease non breaking implementation

* add comment about constraint

* fix test

* add test for generated types

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
This was referenced Jan 12, 2022
Closed
Closed
Closed
Closed
Closed
chevdor added a commit that referenced this pull request Jan 14, 2022
@chevdor
Revert "Fix order of hook execution (#10043)"
a50dbbf 
This reverts commit d36550a.
@chevdor chevdor mentioned this pull request Jan 14, 2022
Closed
chevdor added a commit that referenced this pull request Jan 15, 2022
@chevdor
Revert "Fix order of hook execution (#10043)"
e6fbbd5 
This reverts commit d36550a.
jasl added a commit to Phala-Network/khala-parachain that referenced this pull request Jan 17, 2022
@jasl
Refactor as paritytech/substrate#10043 suggests
19f9862
@Dengjianping Dengjianping mentioned this pull request Jan 21, 2022
Merged
12 tasks
@mrq1911 mrq1911 mentioned this pull request Jan 25, 2022
Closed
3 tasks
@wischli wischli mentioned this pull request Feb 1, 2022
Merged
14 tasks
@dutterbutter dutterbutter mentioned this pull request Feb 3, 2022
Merged
@ghzlatarev ghzlatarev mentioned this pull request Feb 3, 2022
Open
@librelois librelois mentioned this pull request Feb 7, 2022
Closed
@Kailai-Wang Kailai-Wang mentioned this pull request Feb 8, 2022
Closed
@nanocryk nanocryk mentioned this pull request Feb 9, 2022
Merged
2 tasks
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
@gui1117 @apopiak @grishasobol
Fix order of hook execution (paritytech#10043)
98c8c4e 
* fix order

* Update frame/support/procedural/src/construct_runtime/mod.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* format

* more accurate description

* format

* better explicit types

* fix tests

* address feedback

* add a type to ease non breaking implementation

* add comment about constraint

* fix test

* add test for generated types

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
haerdib added a commit to ajuna-network/sgx-runtime that referenced this pull request May 6, 2022
@haerdib @suinuj
@gaudenzkessler @gkessler-scs @echevrier
Upstream merge (#5)
3458223 
* Add secp256k1 ecdsa recover (integritee-network#42)

* implement secp256k1_ecdsa_recover

* fix sgx compilation error

* reorder libsecp256k1 in .toml

Co-authored-by: suinuj <junius@litentry.com>

* Update substrate to b9cafba (integritee-network#44)

- The substrate verion for sp-core was updated to 4.1.0-dev.
- "AllPallets" was replaces by AllPalletsReversedWithSystemFirst according to Fix order of hook execution paritytech/substrate#10043

Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>

* Add runtime-api `get_metadata` (integritee-network#47)

Co-authored-by: echevrier <edith.chevrier@scs.ch>

* Bump substrate to commit 7c63420  (integritee-network#48)

* update .tomls to match new substrate version

* update rust toolchain

* cargo update

* revert unnecessary cargo update..

* updae sp-io version

* redo toolchain bump - teaclave..

* fix version changes

* add missing sp-io functions (integritee-network#49)

* update sp-io trie

* add root version 2

* add storage::root version2

* Bump substrate to commit f5f286db0da99761792b69bf4a997535dcc8f260 (integritee-network#50)

Co-authored-by: echevrier <edith.chevrier@scs.ch>

* Bump to polkadot 0.9.19 and switch to polkadot branches (integritee-network#53)

Remove TransactionByteFee
Apply WeightToFeePolynomials to pallet_transaction_payment

Co-authored-by: echevrier <edith.chevrier@scs.ch>

* fix cargo.lock

* add some pubs

* add one more pub

Co-authored-by: suinuj <junius@litentry.com>
Co-authored-by: gaudenzkessler <92718752+gaudenzkessler@users.noreply.github.com>
Co-authored-by: Gaudenz Kessler <gaudenz.kessler@scs.ch>
Co-authored-by: echevrier <84318241+echevrier@users.noreply.github.com>
Co-authored-by: echevrier <edith.chevrier@scs.ch>
AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 8, 2022
@AurevoirXavier
Companion of paritytech/substrate#10043, !
640216b
@wischli wischli mentioned this pull request Oct 5, 2022
Merged
6 tasks
@mrq1911 mrq1911 mentioned this pull request Nov 3, 2022
Merged
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@gui1117 @apopiak @ark0f
Fix order of hook execution (paritytech#10043)
c1fdf16 
* fix order

* Update frame/support/procedural/src/construct_runtime/mod.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* format

* more accurate description

* format

* better explicit types

* fix tests

* address feedback

* add a type to ease non breaking implementation

* add comment about constraint

* fix test

* add test for generated types

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apopiak apopiak apopiak approved these changes

@kianenigma kianenigma kianenigma approved these changes

@KiChjang KiChjang KiChjang approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Milestone
No milestone
5 participants
@gui1117 @shawntabrizi @KiChjang @kianenigma @apopiak