joystream icon indicating copy to clipboard operation
joystream copied to clipboard

Reorganizing parameters: mutable, configurable and unified

Open bedeho opened this issue 2 years ago • 4 comments

Background

We have a large number of literal constants across

  • the runtime
  • the node
  • pallets

littered across the code base which impact business logic. To be clear, by this I mean a literal value in the Rust code which itself can only be changed or removed with a runtime upgrade or a fresh network. I am here not talking purely about numerical constants, although they constitute the majority, I really mean any literal value. There are a few problems with them being organised this way.

  • Disorganised: They are scattered all over the final runtime in a way which makes it quite difficult to understand what the full set of exogenous parameters that shape the runtime functioning are.
  • Not configurable: Often we may want to easily build versions of a pallet with different values, for example for specific testing environments, or even just for unit testing purposes. This is currently really bad, with duplicate inlined specifications for staging and production environments :cry: :cry: :cry:
  • Static: Changing any value in a running system requires a runtime upgrade, which a more risky process, not only technically, but also because its harder for a non-technical person to adjudicate exactly whether a new final runtime WASM file corresponds exactly to a desired parameter change, as issues pertaining to build configurations and Rust have to be carefully reviewed.
  • Documentation: When each pallet author or subsequent contributor makes one off constants here and there, its harder to enforce a rigorous and expansive documentation norm for such values.

Proposal

  • Remove every literal value in all pallets, and turn them into module trait values that can be specified at runtime composition, and where it should be possible for the value to be dynamic.
  • Remove all literal constants in the runtime which can be changed safely in production without migration!!!, and replace with mutable storage values in a new pallet, called parameters.
    • Make sure to introduce a Root origin get/set pallet method which allows the proposal system to read/write value.
    • make all such pallet storage values configurable, so that it's practical to build the node to run under very different circumstances.
  • Somehow make all literal constants in the runtime which are not safely updatable in production at least configurable at compile time in a type safe way.

A very attractive part of making parameter values configurable is that for some it may be important to harmonize or synchronize the way they are set with other values in GenesisConfig for some pallet, and making all of them originate from the chain spec building process makes that much easier to enforce properly.

┆Issue is synchronized with this Asana task by Unito

bedeho avatar Aug 28 '22 13:08 bedeho

Before we start this, it may be wise to actually map out across all pallet values which ones are safe to mutate, and which ones are not, in particular for built in pallets. This may take substantial amount of time.

bedeho avatar Sep 01 '22 12:09 bedeho

I am wondering whether this is actually too risky and complex to do now before release, there may be too many edge cases, and its not a must now, it mostly is a must for #3626, and otherwise is just a nice cleanup making it easy to launch new chains, which won't be important until next runtime upgrade at least. Here is one problem that popped up.

It may be that some pallets have "constants" defined through pallet:constants, and even if the business logic of the pallet allows it, its [unclear if its wise or safe to actually mutate such values in the absence of runtime upgrades](https://substrate.stackexchange.com/questions/4556/are-palletconstants-really-constant, e.g. a proposal. If it is not then this would be a second family of values which could not be mutated. So now we have these two groups of pallet values (mutation not safe + mutation not possible due to pallet:constants), and they may both need to have values which are coordinated by other exogenous values, such as implied market cap or something else. Now, when you update the exogenous values, you will be automatically updating lots of pallet values which work fine, but you will have this other category of values which will be stuck with stale inputs so to speak. So now you have to remember that whenever certain parameter upgrade proposals are made, you have to do runtime upgrades very quickly after to make sure all values are in harmony across all pallets. This is bound to go wrong.

If this analysis applies even to a few cases, then it may make this whole approach infeasible, or at least, we have to be very careful and systematic about what exact values can be made mutable.

bedeho avatar Sep 01 '22 12:09 bedeho

Update on mutability of pallet::constants

While in principle they can change, and this change will even be reflected in the metadata across RPC calls, despite no runtime upgrade, folks at Parity are adamant that we don't do this, so we should not. The implication here is that we pretty much have to abandon mutable parameters for all pallets we dont control, because otherwise we would have to fork them all and change the parameter to no longer be a pallet::constant. Perhaps that could make sense in the future, but it would be a risky undertaking, so it would have to be done very carefully, and also a very detailed examination of what parameters are truly safe to mutate anyway, which is very time intensive to determine in many cases. It also implies we should in general avoid using pallet::constant unless we know the value has to actually be constant in a pallet.

bedeho avatar Sep 03 '22 05:09 bedeho

Removing from mainnet scope, too many unknowns here.

bedeho avatar Oct 08 '22 12:10 bedeho