Enable changing executor params through governance #6934

s0me0ne-unkn0wn · 2023-03-21T17:22:39Z

Add a pallet call to session_info which sets new executor parameters. The new parameter set is stored as a separate storage value and becomes effective starting from the next session.

s0me0ne-unkn0wn · 2023-03-21T17:24:09Z

Currently, I'm not sure I'm doing the right thing. Feel free to guide me on the right path.

runtime/parachains/src/session_info.rs

rphmeier

Is there a reason why the executor params can't just go into the host configuration via the Configuration pallet?

s0me0ne-unkn0wn · 2023-03-21T23:50:40Z

Is there a reason why the executor params can't just go into the host configuration via the Configuration pallet?

I believe there are no technical obstacles, but I have two concerns about that:

Currently, the configuration only contains values of primitive types, like u32 and BlockNumber. Is it okay to bring the whole ExecutorParams with all its derivatives to it?
Do ExecutorParams fit the HostConfiguration concept at all?

Beyond that, I have no other arguments against moving them to the configuration pallet altogether (but storage migration will be needed then).

slumber · 2023-03-22T14:02:44Z

I'd say moving it to configuration makes sense within this PR.

ordian · 2023-03-22T14:51:05Z

Is there a reason why the executor params can't just go into the host configuration via the Configuration pallet?

I believe there are no technical obstacles, but I have two concerns about that:
1. Currently, the `configuration` only contains values of primitive types, like `u32` and `BlockNumber`. Is it okay to bring the whole `ExecutorParams` with all its derivatives to it?

2. Do `ExecutorParams` fit the `HostConfiguration` concept at all?
Beyond that, I have no other arguments against moving them to the configuration pallet altogether (but storage migration will be needed then).

To me it feels like HostConfiguration has become a God object of sorts where many unrelated knobs are being put. For example, there are things only relevant for runtime, or things used by both validators and parachain runtime (via AbridgedHostConfiguration) or things used only on the client side (e.g. needed_approvals).

Another concern is tying breaking changes and migration between the two (although I don't expect many breaking changes to ExecutorParams).

Don't have any other objection.

runtime/parachains/src/session_info.rs

ggwpez · 2023-03-22T18:49:08Z

runtime/parachains/src/session_info.rs

+		))]
+		pub fn set_executor_params(origin: OriginFor<T>, new: ExecutorParams) -> DispatchResult {
+			ensure_root(origin)?;
+			<PendingExecutorParams<T>>::put(Some(new));


And probably emit an event, so wallets and indexers can be notified.

Do they need to know about execution parameters change? That's a validators' business iiuc.

I dont know of the implications of this - events are good when the outside world us supposed to be notified.
Your call.

runtime/parachains/src/session_info.rs

bkchr · 2023-03-23T09:40:54Z

To me it feels like HostConfiguration has become a God object of sorts where many unrelated knobs are being put.

I think the main problem here being that we always need to decode the entire HostConfiguration for just accessing some small detail of the configuration. Maybe with the ExecutorParams it is a good start to put these into their own Configuration storage item. Nevertheless, the configuration pallet should be re-used to host this.

s0me0ne-unkn0wn · 2023-03-23T11:36:19Z

@bkchr, one more concern is that we're accessing executor params on the session base, not on the state base, and we're maintaining a sliding window of different executor params for the sessions inside the dispute period. The session_info pallet maintains some infrastructure to support that (similar to SessionInfo storage), like keeping the earliest stored session number, etc. Moving the whole thing to the configuration pallet would introduce calls from the configuration pallet to the storage of the session_info pallet. Are we okay with that?

bkchr · 2023-03-23T11:55:25Z

etc. Moving the whole thing to the configuration pallet would introduce calls from the configuration pallet to the storage of the session_info pallet. Are we okay with that?

Isn't the session info not just caching certain parameters per session and also currently caches things that are already part of the host configuration?

slumber · 2023-03-23T12:09:06Z

@s0me0ne-unkn0wn configuration change is always buffered to be applied in a couple of sessions, if this is somethings you're worried about

s0me0ne-unkn0wn · 2023-03-23T13:41:33Z

Isn't the session info not just caching certain parameters per session and also currently caches things that are already part of the host configuration?

In principle, yes. But

It maintains a sliding window of those caches to make them available when referred by session index, not by relay parent
It works the other way round: session_info gets some information from configuration and then caches it, and that's quite natural. But I'm not sure how natural it is for the configuration to request from session_info (I didn't check yet, but at least I'm afraid to get a pallet initialization order conflict)

@s0me0ne-unkn0wn configuration change is always buffered to be applied in a couple of sessions, if this is somethings you're worried about

It currently only does for the parameters encapsulated into the HostConfiguration struct, which is stored and updated as a whole; if we're talking about taking ExecutorParams out of it, we can implement any different logic as needed, I believe.

s0me0ne-unkn0wn · 2023-03-29T17:15:52Z

Moved pending changes to the configuration pallet. Now the whole thing looks much more logical and straightforward to me.

mrcnski · 2023-03-30T08:45:27Z

Moved pending changes to the configuration pallet. Now the whole thing looks much more logical and straightforward to me.

+1. To me it definitely makes sense to have executor params reflected in the
session info, as we need to keep them per session. But the pending value
should be present in some external configuration which feeds the session info,
and this other configuration can be governanced.

…rams-governed

s0me0ne-unkn0wn · 2023-04-07T10:59:03Z

I'm a little bit stuck with the configuration pallet migration.

If I add something to HostConfiguration I have to add a migration that translates storage. But HostConfiguation is not a versioned primitive, and we have an active storage migration from v4 to v5. It refers to the current version of HostConfiguration as a translation target structure, so if I change anything in HostConfiguration, I have to change the migration.

And here I'm not sure, if that migration to v5 is over now, and the code may be safely removed and replaced with similar migration from v5 to v6, or if I should add my changes to the existing migration from v4 to v5. How do I determine that?

eskimor · 2023-04-07T11:29:48Z

I'm a little bit stuck with the configuration pallet migration.

If I add something to HostConfiguration I have to add a migration that translates storage. But HostConfiguation is not a versioned primitive, and we have an active storage migration from v4 to v5. It refers to the current version of HostConfiguration as a translation target structure, so if I change anything in HostConfiguration, I have to change the migration.

And here I'm not sure, if that migration to v5 is over now, and the code may be safely removed and replaced with similar migration from v5 to v6, or if I should add my changes to the existing migration from v4 to v5. How do I determine that?

If the v5 migration has been released already, we have to do a v6. Not sure how well this works with runtime releases being skipped, but it is the only sensible way.

s0me0ne-unkn0wn · 2023-04-07T12:42:21Z

If the v5 migration has been released already

I've checked that the latest release branch (release-v0.9.41) does not include the v5 migration, so it's not been released yet? How to know for sure?

slumber · 2023-04-07T12:43:39Z

If the v5 migration has been released already

I've checked that the latest release branch (release-v0.9.41) does not include the v5 migration, so it's not been released yet? How to know for sure?

@coderobe

…chains::configuration

eskimor · 2023-04-07T17:15:43Z

If the v5 migration has been released already

I've checked that the latest release branch (release-v0.9.41) does not include the v5 migration, so it's not been released yet? How to know for sure?

That's how you are sure. If it is not yet on the latest release branch, It cannot be released. I mean in theory things could be backported, would still be very weird (and a bug) to not have that backport in the next release as well.

ordian · 2023-04-07T17:25:55Z

I think the idea was to keep previous migrations for a few releases at least. Since each migration does version check (if on_chain_version == expected), it's safe to keep outdated migrations, they just need to be applied in the right order. Whereas, if we remove them prematurely, it will cause problems for us, potentially corrupting the storage.

s0me0ne-unkn0wn · 2023-04-07T20:18:22Z

Whereas, if we remove them prematurely, it will cause problems for us, potentially corrupting the storage.

I'm much more concerned about if it is safe to add my changes to the existing migration :) If it's not been released, then it's safe, right?

…ains::configuration

…hains::configuration

…rams-governed

coderobe · 2023-04-13T15:53:47Z

If the v5 migration has been released already

I've checked that the latest release branch (release-v0.9.41) does not include the v5 migration, so it's not been released yet? How to know for sure?

@coderobe

parachains_configuration::migration::v5::MigrateToV5<Runtime> is unreleased and can still be modified until 0.9.42 is cut, yes

s0me0ne-unkn0wn · 2023-04-14T11:21:04Z

bot merge

Add a pallet call to change executor params

b738984

s0me0ne-unkn0wn commented Mar 21, 2023

View reviewed changes

s0me0ne-unkn0wn requested review from sandreim, ordian, eskimor and slumber March 21, 2023 17:31

rphmeier reviewed Mar 21, 2023

View reviewed changes

ggwpez reviewed Mar 22, 2023

View reviewed changes

s0me0ne-unkn0wn added 3 commits March 29, 2023 10:59

Use OptionQuery; Avoid runtime panic

cfdde42

Move pending executor params to configuration

2b35a4c

Merge branch 'master' into s0me0ne/executor-params-governed

f56ff95

s0me0ne-unkn0wn marked this pull request as ready for review March 29, 2023 17:16

s0me0ne-unkn0wn removed the T4-parachains_engineering This PR/Issue is related to Parachains performance, stability, maintenance. label Mar 29, 2023

s0me0ne-unkn0wn added 2 commits April 7, 2023 11:43

Move ExecutorParams to HostConfiguration structure

10c6fb1

Merge remote-tracking branch 'origin/master' into s0me0ne/executor-pa…

ef04730

…rams-governed

s0me0ne-unkn0wn and others added 3 commits April 7, 2023 15:02

Add executor params to the v5 migration

91eb230

Add an ExecutorParams benchmark

8746cc4

".git/.scripts/commands/bench/bench.sh" runtime polkadot runtime_para…

ef255d0

…chains::configuration

s0me0ne-unkn0wn and others added 5 commits April 7, 2023 21:34

Add to WeightInfo

2ff1199

Add dummy weights to other networks

f4ac516

".git/.scripts/commands/bench/bench.sh" runtime kusama runtime_parach…

2ba9117

…ains::configuration

".git/.scripts/commands/bench/bench.sh" runtime rococo runtime_parach…

7ca3fd2

…ains::configuration

".git/.scripts/commands/bench/bench.sh" runtime westend runtime_parac…

6ce9028

…hains::configuration

s0me0ne-unkn0wn requested a review from eskimor April 13, 2023 11:15

s0me0ne-unkn0wn added 3 commits April 13, 2023 13:19

Use real weight

b932f11

Merge remote-tracking branch 'origin/master' into s0me0ne/executor-pa…

54e4bc9

…rams-governed

Fix comment

b41a1c7

slumber approved these changes Apr 13, 2023

View reviewed changes

ordian approved these changes Apr 14, 2023

View reviewed changes

paritytech-processbot bot merged commit df61c0b into master Apr 14, 2023

paritytech-processbot bot deleted the s0me0ne/executor-params-governed branch April 14, 2023 11:21

s0me0ne-unkn0wn mentioned this pull request Aug 24, 2023

Executor parameters change may affect PVF validity and executability retrospectively paritytech/polkadot-sdk#694

Open

eskimor removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label May 8, 2023

github-actions bot mentioned this pull request Jun 15, 2023

Update substrate/polkadot/cumulus from v0.9.40 to v0.9.43 moonbeam-foundation/moonbeam#2354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable changing executor params through governance #6934

Enable changing executor params through governance #6934

s0me0ne-unkn0wn commented Mar 21, 2023

s0me0ne-unkn0wn commented Mar 21, 2023

rphmeier left a comment

s0me0ne-unkn0wn commented Mar 21, 2023

slumber commented Mar 22, 2023

ordian commented Mar 22, 2023

ggwpez Mar 22, 2023

s0me0ne-unkn0wn Mar 23, 2023

ggwpez Mar 30, 2023

bkchr commented Mar 23, 2023

s0me0ne-unkn0wn commented Mar 23, 2023

bkchr commented Mar 23, 2023

slumber commented Mar 23, 2023

s0me0ne-unkn0wn commented Mar 23, 2023 •

edited

Loading

s0me0ne-unkn0wn commented Mar 29, 2023

mrcnski commented Mar 30, 2023

s0me0ne-unkn0wn commented Apr 7, 2023

eskimor commented Apr 7, 2023

s0me0ne-unkn0wn commented Apr 7, 2023

slumber commented Apr 7, 2023

eskimor commented Apr 7, 2023

ordian commented Apr 7, 2023 •

edited

Loading

s0me0ne-unkn0wn commented Apr 7, 2023

coderobe commented Apr 13, 2023

s0me0ne-unkn0wn commented Apr 14, 2023

Enable changing executor params through governance #6934

Enable changing executor params through governance #6934

Conversation

s0me0ne-unkn0wn commented Mar 21, 2023

s0me0ne-unkn0wn commented Mar 21, 2023

rphmeier left a comment

Choose a reason for hiding this comment

s0me0ne-unkn0wn commented Mar 21, 2023

slumber commented Mar 22, 2023

ordian commented Mar 22, 2023

ggwpez Mar 22, 2023

Choose a reason for hiding this comment

s0me0ne-unkn0wn Mar 23, 2023

Choose a reason for hiding this comment

ggwpez Mar 30, 2023

Choose a reason for hiding this comment

bkchr commented Mar 23, 2023

s0me0ne-unkn0wn commented Mar 23, 2023

bkchr commented Mar 23, 2023

slumber commented Mar 23, 2023

s0me0ne-unkn0wn commented Mar 23, 2023 • edited Loading

s0me0ne-unkn0wn commented Mar 29, 2023

mrcnski commented Mar 30, 2023

s0me0ne-unkn0wn commented Apr 7, 2023

eskimor commented Apr 7, 2023

s0me0ne-unkn0wn commented Apr 7, 2023

slumber commented Apr 7, 2023

eskimor commented Apr 7, 2023

ordian commented Apr 7, 2023 • edited Loading

s0me0ne-unkn0wn commented Apr 7, 2023

coderobe commented Apr 13, 2023

s0me0ne-unkn0wn commented Apr 14, 2023

s0me0ne-unkn0wn commented Mar 23, 2023 •

edited

Loading

ordian commented Apr 7, 2023 •

edited

Loading