companion for pallet order fix. #4181

thiolliere · 2021-10-30T13:39:23Z

companion for paritytech/substrate#10043

Before the hook were executed in reverse order, now with this PR we can choose the order. This PR choose to execute in declared order, we need to review if the new order is a valid implementation for polkadot.

runtime/common/src/integration_tests.rs

thiolliere · 2021-10-30T15:10:25Z

runtime/common/src/integration_tests.rs

@@ -288,12 +288,10 @@ fn run_to_block(n: u32) {
 	assert!(System::block_number() < n);
 	while System::block_number() < n {
 		let block_number = System::block_number();
-		AllPallets::on_finalize(block_number);
-		System::on_finalize(block_number);


System::on_finalize is inside AllPalletsWithSystem now

thiolliere · 2021-10-30T15:10:50Z

runtime/common/src/integration_tests.rs

 		System::set_block_number(block_number + 1);
-		System::on_initialize(block_number + 1);


System::on_initialize is now inside AllPalletsWithSystem. And it is no-op so having it after is fine.

thiolliere · 2021-10-30T16:12:28Z

note: if there is some constraint on the order of pallet or that we don't want to take any risk we can instead keep the old order and use AllPalletsReversedWithSystemFirst.
But as far as I can see, there is no constraint apart from Babe having to be before Session, which I don't know the reason yet.

…-order

thiolliere · 2021-11-17T02:17:23Z

Actually there might some more constraint in the order of the pallet:
pallet authorhsip will do note the author.
when the author is noted, staking give reward point to the author, so it must be in the correct era. and im-online note that the validator has produced a block for the current session, so it must be in the correct session.

Actually in the current order (reversed) it seems the author is noted after pallet session is initialized, thus after the session are rotated. I'm trying to understand the current relation and if the order is respected and matters.

thiolliere · 2021-11-17T02:48:59Z

I added some comment on the constraint which seems relevant to me, I guess we should write tests for them, but I'm a bit lost how.

kianenigma · 2021-11-22T09:16:21Z

runtime/kusama/src/lib.rs

@@ -1594,7 +1596,7 @@ pub type Executive = frame_executive::Executive<
 	Block,
 	frame_system::ChainContext<Runtime>,
 	Runtime,
-	AllPallets,
+	AllPalletsWithSystem,


So here we have switched from reverse order, to not-reversed order. I was expecting this to cause some items in construct_runtime to be swapped. How so?

the only constraint I have found are those I put into comment, and switching to no-reversed order actually fix them.
I believe than currently the block producer of the last block of an era doesn't get points (except if he is elected in the next era) and also the block producer of last block of a session cannot tell he was online.

because pallet-authoship is executed after pallet-session.

About babe and session AFAICT the order is not relevant, all cases are handled in their implementation

…-order

paritytech-processbot · 2021-12-01T00:59:20Z

Companion update failed: CommandFailed { cmd: "Command { std: "cargo" "update" "-v" "-p" "pallet-beefy:4.0.0-dev" "-p" "sc-state-db:0.10.0-dev" "-p" "frame-support-procedural-tools-derive:3.0.0" "-p" "sp-timestamp:4.0.0-dev" "-p" "sc-service:0.10.0-dev" "-p" "pallet-bags-list-remote-tests:4.0.0-dev" "-p" "sc-block-builder:0.10.0-dev" "-p" "sc-sync-state-rpc:0.10.0-dev" "-p" "sp-tracing:4.0.0-dev" "-p" "substrate-wasm-builder:5.0.0-dev" "-p" "sc-proposer-metrics:0.10.0-dev" "-p" "frame-support:4.0.0-dev" "-p" "pallet-nicks:4.0.0-dev" "-p" "sc-chain-spec-derive:4.0.0-dev" "-p" "sc-consensus-slots:0.10.0-dev" "-p" "frame-benchmarking-cli:4.0.0-dev" "-p" "sp-offchain:4.0.0-dev" "-p" "pallet-elections-phragmen:5.0.0-dev" "-p" "sp-maybe-compressed-blob:4.1.0-dev" "-p" "sc-executor-common:0.10.0-dev" "-p" "try-runtime-cli:0.10.0-dev" "-p" "frame-executive:4.0.0-dev" "-p" "pallet-indices:4.0.0-dev" "-p" "pallet-staking-reward-curve:4.0.0-dev" "-p" "substrate-test-client:2.0.1" "-p" "pallet-identity:4.0.0-dev" "-p" "sp-api:4.0.0-dev" "-p" "pallet-bags-list:4.0.0-dev" "-p" "sc-executor-wasmtime:0.10.0-dev" "-p" "substrate-frame-rpc-system:4.0.0-dev" "-p" "pallet-authority-discovery:4.0.0-dev" "-p" "sp-npos-elections-solution-type:4.0.0-dev" "-p" "sc-client-db:0.10.0-dev" "-p" "sp-block-builder:4.0.0-dev" "-p" "sc-keystore:4.0.0-dev" "-p" "pallet-society:4.0.0-dev" "-p" "sc-chain-spec:4.0.0-dev" "-p" "pallet-vesting:4.0.0-dev" "-p" "frame-benchmarking:4.0.0-dev" "-p" "beefy-gadget-rpc:4.0.0-dev" "-p" "frame-support-test:3.0.0" "-p" "sc-authority-discovery:0.10.0-dev" "-p" "sp-blockchain:4.0.0-dev" "-p" "pallet-beefy-mmr:4.0.0-dev" "-p" "frame-system-benchmarking:4.0.0-dev" "-p" "sp-serializer:4.0.0-dev" "-p" "pallet-sudo:4.0.0-dev" "-p" "sc-cli:0.10.0-dev" "-p" "substrate-test-utils-derive:0.10.0-dev" "-p" "pallet-mmr:4.0.0-dev" "-p" "sp-keystore:0.10.0-dev" "-p" "sp-consensus-slots:0.10.0-dev" "-p" "sc-consensus:0.10.0-dev" "-p" "sc-network-gossip:0.10.0-dev" "-p" "sc-allocator:4.1.0-dev" "-p" "sc-executor:0.10.0-dev" "-p" "pallet-mmr-primitives:4.0.0-dev" "-p" "substrate-build-script-utils:3.0.0" "-p" "sc-rpc:4.0.0-dev" "-p" "frame-system-rpc-runtime-api:4.0.0-dev" "-p" "sp-finality-grandpa:4.0.0-dev" "-p" "sp-version:4.0.0-dev" "-p" "pallet-offences-benchmarking:4.0.0-dev" "-p" "sp-core:4.0.0-dev" "-p" "sp-runtime-interface-proc-macro:4.0.0-dev" "-p" "test-runner:0.9.0" "-p" "frame-try-runtime:0.10.0-dev" "-p" "pallet-bounties:4.0.0-dev" "-p" "sp-std:4.0.0-dev" "-p" "sp-io:4.0.0-dev" "-p" "sc-basic-authorship:0.10.0-dev" "-p" "sp-debug-derive:4.0.0-dev" "-p" "pallet-transaction-payment-rpc:4.0.0-dev" "-p" "sp-version-proc-macro:4.0.0-dev" "-p" "pallet-gilt:4.0.0-dev" "-p" "pallet-grandpa:4.0.0-dev" "-p" "pallet-democracy:4.0.0-dev" "-p" "sc-finality-grandpa-rpc:0.10.0-dev" "-p" "pallet-scheduler:4.0.0-dev" "-p" "pallet-staking:4.0.0-dev" "-p" "pallet-election-provider-multi-phase:4.0.0-dev" "-p" "sc-informant:0.10.0-dev" "-p" "remote-externalities:0.10.0-dev" "-p" "sc-consensus-babe-rpc:0.10.0-dev" "-p" "pallet-tips:4.0.0-dev" "-p" "sp-storage:4.0.0-dev" "-p" "sp-transaction-storage-proof:4.0.0-dev" "-p" "sc-consensus-epochs:0.10.0-dev" "-p" "sp-session:4.0.0-dev" "-p" "sp-core-hashing-proc-macro:4.0.0-dev" "-p" "sp-externalities:0.10.0-dev" "-p" "pallet-session:4.0.0-dev" "-p" "sc-transaction-pool:4.0.0-dev" "-p" "pallet-membership:4.0.0-dev" "-p" "sp-authorship:4.0.0-dev" "-p" "sp-consensus-vrf:0.10.0-dev" "-p" "sp-npos-elections:4.0.0-dev" "-p" "sc-tracing-proc-macro:4.0.0-dev" "-p" "generate-bags:4.0.0-dev" "-p" "pallet-treasury:4.0.0-dev" "-p" "sp-runtime:4.0.0-dev" "-p" "sc-consensus-babe:0.10.0-dev" "-p" "sc-rpc-server:4.0.0-dev" "-p" "beefy-primitives:4.0.0-dev" "-p" "fork-tree:3.0.0" "-p" "frame-support-test-pallet:4.0.0-dev" "-p" "sp-arithmetic:4.0.0-dev" "-p" "frame-support-procedural-tools:4.0.0-dev" "-p" "pallet-babe:4.0.0-dev" "-p" "sc-offchain:4.0.0-dev" "-p" "sp-application-crypto:4.0.0-dev" "-p" "sp-consensus:0.10.0-dev" "-p" "sp-keyring:4.0.0-dev" "-p" "pallet-transaction-payment:4.0.0-dev" "-p" "pallet-offences:4.0.0-dev" "-p" "pallet-balances:4.0.0-dev" "-p" "sc-client-api:4.0.0-dev" "-p" "pallet-collective:4.0.0-dev" "-p" "sc-transaction-pool-api:4.0.0-dev" "-p" "pallet-im-online:4.0.0-dev" "-p" "sc-executor-wasmi:0.10.0-dev" "-p" "pallet-authorship:4.0.0-dev" "-p" "pallet-recovery:4.0.0-dev" "-p" "sp-core-hashing:4.0.0-dev" "-p" "frame-support-procedural:4.0.0-dev" "-p" "pallet-transaction-payment-rpc-runtime-api:4.0.0-dev" "-p" "sc-telemetry:4.0.0-dev" "-p" "sp-consensus-babe:0.10.0-dev" "-p" "sp-rpc:4.0.0-dev" "-p" "sp-state-machine:0.10.0-dev" "-p" "pallet-staking-reward-fn:4.0.0-dev" "-p" "frame-system:4.0.0-dev" "-p" "pallet-assets:4.0.0-dev" "-p" "sc-finality-grandpa:0.10.0-dev" "-p" "sc-tracing:4.0.0-dev" "-p" "sp-trie:4.0.0-dev" "-p" "sp-wasm-interface:4.0.0-dev" "-p" "sp-inherents:4.0.0-dev" "-p" "beefy-gadget:4.0.0-dev" "-p" "sp-api-proc-macro:4.0.0-dev" "-p" "pallet-session-benchmarking:4.0.0-dev" "-p" "sp-database:4.0.0-dev" "-p" "sp-panic-handler:4.0.0-dev" "-p" "sc-peerset:4.0.0-dev" "-p" "sp-staking:4.0.0-dev" "-p" "pallet-mmr-rpc:3.0.0" "-p" "sc-utils:4.0.0-dev" "-p" "sc-consensus-uncles:0.10.0-dev" "-p" "sc-rpc-api:0.10.0-dev" "-p" "pallet-proxy:4.0.0-dev" "-p" "pallet-timestamp:4.0.0-dev" "-p" "sp-tasks:4.0.0-dev" "-p" "sp-authority-discovery:4.0.0-dev" "-p" "sp-runtime-interface:4.0.0-dev" "-p" "sc-network:0.10.0-dev" "-p" "beefy-merkle-tree:4.0.0-dev" "-p" "frame-election-provider-support:4.0.0-dev" "-p" "pallet-multisig:4.0.0-dev" "-p" "sp-transaction-pool:4.0.0-dev" "-p" "pallet-utility:4.0.0-dev" "-p" "sc-consensus-manual-seal:0.10.0-dev" "-p" "substrate-prometheus-endpoint:0.10.0-dev" "-p" "substrate-test-utils:4.0.0-dev", kill_on_drop: false }", status_code: Some(101), err: " Updating git repository https://github.com/paritytech/substrate\n Updating crates.io index\nerror: no matching package named frame-benchmarking-cli found\nlocation searched: https://github.com/paritytech/substrate?branch=master\nrequired by package polkadot-cli v0.9.13 (/builds/polkadot/cli)\n" }

shawntabrizi · 2021-12-01T01:26:34Z

bot merge

paritytech-processbot · 2021-12-01T01:26:42Z

Waiting for commit status.

paritytech-processbot · 2021-12-01T01:34:20Z

Merge cancelled due to error. Error: Checks failed for 69a64bf

shawntabrizi · 2021-12-01T02:57:52Z

bot merge

paritytech-processbot · 2021-12-01T02:58:01Z

Waiting for commit status.

* companion * remove no-op duplicated function * fmt * add comment on constraint * Run cargo update * fix integration test Co-authored-by: Keith Yeung <kungfukeith11@gmail.com> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

companion

a230e5d

thiolliere requested review from HCastano, svyatonik and tomusdrw as code owners October 30, 2021 13:39

github-actions bot added the A0-please_review Pull request needs code review. label Oct 30, 2021

thiolliere changed the title ~~companion for https://github.com/paritytech/substrate/pull/10043~~ companion for pallet order fix. Oct 30, 2021

thiolliere mentioned this pull request Oct 30, 2021

Fix order of hook execution paritytech/substrate#10043

Merged

shawntabrizi reviewed Oct 30, 2021

View reviewed changes

runtime/common/src/integration_tests.rs Outdated Show resolved Hide resolved

remove no-op duplicated function

687ba03

thiolliere commented Oct 30, 2021

View reviewed changes

fmt

8f59d95

shawntabrizi approved these changes Oct 30, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into gui-fix-all-pallets…

357b0f9

…-order

thiolliere requested review from acatangiu, adoerr and antonio-dropulic as code owners November 17, 2021 01:56

add comment on constraint

b6cb588

kianenigma reviewed Nov 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into gui-fix-all-pallets…

f00ee26

…-order

thiolliere 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 Nov 23, 2021

shawntabrizi approved these changes Nov 30, 2021

View reviewed changes

Merge branch 'master' into gui-fix-all-pallets-order

6dcf0f9

KiChjang approved these changes Dec 1, 2021

View reviewed changes

Run cargo update

69a64bf

fix integration test

8e0588e

paritytech-processbot bot merged commit c87a220 into master Dec 1, 2021

paritytech-processbot bot deleted the gui-fix-all-pallets-order branch December 1, 2021 03:00

jasl mentioned this pull request Jan 18, 2022

Upgrade Polkadot v0.9.15 Phala-Network/khala-parachain#58

Closed

Kailai-Wang mentioned this pull request Feb 8, 2022

Double-check the collation related pallets configuration for litmus litentry/litentry-parachain#336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

companion for pallet order fix. #4181

companion for pallet order fix. #4181

thiolliere commented Oct 30, 2021 •

edited by KiChjang

Loading

thiolliere Oct 30, 2021

thiolliere Oct 30, 2021

thiolliere commented Oct 30, 2021

thiolliere commented Nov 17, 2021 •

edited

Loading

thiolliere commented Nov 17, 2021

kianenigma Nov 22, 2021

thiolliere Nov 22, 2021 •

edited

Loading

paritytech-processbot bot commented Dec 1, 2021

shawntabrizi commented Dec 1, 2021

paritytech-processbot bot commented Dec 1, 2021

paritytech-processbot bot commented Dec 1, 2021

shawntabrizi commented Dec 1, 2021

paritytech-processbot bot commented Dec 1, 2021

		System::set_block_number(block_number + 1);
		System::on_initialize(block_number + 1);

companion for pallet order fix. #4181

companion for pallet order fix. #4181

Conversation

thiolliere commented Oct 30, 2021 • edited by KiChjang Loading

thiolliere Oct 30, 2021

Choose a reason for hiding this comment

thiolliere Oct 30, 2021

Choose a reason for hiding this comment

thiolliere commented Oct 30, 2021

thiolliere commented Nov 17, 2021 • edited Loading

thiolliere commented Nov 17, 2021

kianenigma Nov 22, 2021

Choose a reason for hiding this comment

thiolliere Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

paritytech-processbot bot commented Dec 1, 2021

shawntabrizi commented Dec 1, 2021

paritytech-processbot bot commented Dec 1, 2021

paritytech-processbot bot commented Dec 1, 2021

shawntabrizi commented Dec 1, 2021

paritytech-processbot bot commented Dec 1, 2021

thiolliere commented Oct 30, 2021 •

edited by KiChjang

Loading

thiolliere commented Nov 17, 2021 •

edited

Loading

thiolliere Nov 22, 2021 •

edited

Loading