[discussion] Align configuration-related instructions with the RFC

[0x009922]

Description

According to the Configuration Overhaul RFC (#2585), the instructions set related to setting chain-wide configuration parameters should be redefined. For more details and reasoning behind it please read the document.

The goal of this issue is to work on the design first, without implementing it. We should establish the design simultaneously with the updating of the configuration reference (hyperledger-iroha/iroha-2-docs#392). Only after the all parts of the reference are consistent and "good enough", we will start implementing it.

Set of chain-wide parameters

• sumeragi.block_time
• sumeragi.commit_time
• sumeragi.max_transactions_in_block
• wsv.transaction_limits
• wsv.identifier_length_limits
• wsv.domain_metadata_limits
• wsv.account_metadata_limits
• wsv.asset_definition_metadata_limits
• wsv.asset_metadata_limits
• wsv.wasm_runtime.fuel_limit
• wsv.wasm_runtime.memory_limit

Thoughts

Current NewParameter and SetParameter instructions accept any kind of key-value pair. There is a set of "magic keys" which are mapped into config parameters.

The core of Iroha has a limited set of parameters. Therefore, I propose to hard-code this set into the data model, and introduce an instruction to set/update parameters using this strictly defined set.

Notes:

• Remember that chain-wide parameters will be set exclusively via ISIs, i.e. they will be excluded from local peer configurations.
• Format: how such an instruction will look like?
• Genesis & Default: these parameters should be set in genesis. Will Iroha have default values for them? If not, then all parameters should be set in genesis.
• Naming & Nesting: should namespaces be flattened into a single namespace for simplicity? E.g. not wsv.wasm_runtime.fuel_limits, but just wasm_fuel_limits. Also, should the naming be aligned with naming of parameters in local configuration file? (or even in env?)
• Conveniences: according to the RFC, the config file will have some conveniences like setting durations not only as a number of millis, but also as a human-readable string like "2s 300ms". Will the instruction also support it? I think yes, for consistency.

How the instruction might look like? I don't have an idea how to do it better.

A structure might look like:

enum Instruction {
    // ...
    Configure(Parameters)
}

struct Parameters {
    block_time: Option<Duration>,
    commit_time: Option<Duration>,
    wsv_asset_metadata_limits: Option<MetadataLimits>,
    // ...
}

Or a enum?

enum Instruction {
    // ...
    Configure(Parameter)
}

enum Parameter {
    BlockTime(Duration),
    CommitTime(Duration),
    WsvAssetMetadataLimits(MetadataLimits),
    // ...
}

[6r1d]

I'm used to mapping an integer constant to some name as an argument in embedded code, but I'm not sure, which Rust feature here, struct or enum, is more convenient.
In general, I think your idea regarding a limited number of parameters would be useful.

[Erigara]

To me submitting parameters in the struct (or enum) make more sense than current approach which is unnecessary broad atm, it's aligned with the way i wanted to store parameters initially (here).

I would prefer struct with optional fields (this way we would be forward and backward compatible) and it would be possible to update whole config with just one isi, which seems convenient.

Genesis & Default: these parameters should be set in genesis. Will Iroha have default values for them? If not, then all parameters should be set in genesis.

Imo iroha should have some default values for such case.

Conveniences: according to the RFC, the config file will have some conveniences like setting durations not only as a number of millis, but also as a human-readable string like "2s 300ms". Will the instruction also support it? I think yes, for consistency.

I think we can add this later so that initially we would support only default representations, and then we can extend it with human readable representations.

[Mingela]

There is a set of "magic keys" which are mapped into config parameters. The core of Iroha has a limited set of parameters. Therefore, I propose to hard-code this set into the data model, and introduce an instruction to set/update parameters using this strictly defined set.

A very similar issue has been present for permission tokens and other literals, hope the data model will be exhaustive henceforth.

Suggested implementations are fine.

As for the defaults I'm generally fine with having ones, though there is an example when it could be confusing. Imagine there is an Iroha upgrade in the future and within the upgrade the defaults are changed. To eliminate the risk of inconsistency or blockstore corruption in that case I'd suggest the peer submitting the genesis to forcibly append an ISI (or transaction with one) with the defaults for missing config entries in the initial genesis provided by the user.

As for naming I'd support simplifying those to plain literals as shown above.

As for value formats, as @Erigara said, we don't need that flexibility in first place. That might even introduce confusion or user input errors.

[0x009922]

I'd suggest the peer submitting the genesis to forcibly append an ISI (or transaction with one) with the defaults for missing config entries in the initial genesis provided by the user.

That is a good point.

[Arjentix]

Struct or enum?

Initially I'd vote for enum, but after @Erigara's point I'm not so sure.
I'd like to defend enum's future compatibility. In future we still might have outdated fields and add new ones. Just the same as for struct. In both cases compatibility will be implemented on (de-)coding side. So the only thing to discuss here is convenience (do we want to set all parameters in one instruction or not?).

Genesis or default?

I'd prefer explicit parameters, so I vote for genesis. These parameters could be implemented not just as plain instructions, but as genesis fields which will be transmitted as instructions. Like we do for executor field.

Flat or namespace format?

I have no opinion here. Both is fine to me. I think it depends more on the end-user UX.

Human-readable time format

I'm agree with others, to me it seems to be too complicated for now.

[Erigara]

Genesis or default?

I'd prefer explicit parameters, so I vote for genesis. These parameters could be implemented not just as plain instructions, but as genesis fields which will be transmitted as instructions. Like we do for executor field.

I want to highlight that default values are still required, because we need some parameter values anyway to perform genesis round.

[0x009922]

Summarising the comments and adding some other bits from my side, here is a complete changeset:

1. Stop using NewParameter/SetParameter instructions for chain-wide hard-coded configuration
2. Introduce a new instruction, e.g. ConfigureChain. It will accept a flat structure with the known parameters aligned to a single namespace. All fields are optional.
   • Note from me as an SDK developer: since this structure with all optional fields will be in the schema, it will cause some problems for JS SDK. The current SCALE&codegen implementation there is not capable to handle such structures in a not ugly way. However, I don't think it should stop us from this design decision, it is a minor issue.
3. Introduce a new query, e.g. FindChainConfiguration (although there is nothing to find, it is just fetching the data which is always available). It will return the current configuration set in WSV, with all fields having values.
4. Add a new field to the genesis file, e.g. configuration. It will accept the same structure as the ConfigureChain instruction, and will also apply default values to absent parameters.
5. Commit initial values of all chain-wide parameters as part of the genesis block. Defaults or user-provided - doesn't matter.
6. Optionally, log setting/updating of the chain-wide parameters on DEBUG/TRACE level.

@Erigara @Arjentix @Mingela does it look satisfying and complete for you?

[Arjentix]

I want to highlight that default values are still required, because we need some parameter values anyway to perform genesis round.

Oh, that complicates my idea...

[Arjentix]

does it look satisfying and complete for you?

Yes, looks nice

[Erigara]

does it look satisfying and complete for you?

Yeah, lgtm

[0x009922]

Also, there is ConfigurationEvent (Changed, Created, Deleted)... I guess it should be split into two kinds of events:

• ParameterEvent
• ChainConfigurationEvent

[0x009922]

Also, here is how we can rename the parameters to fit them into a single flat namespace:

- sumeragi.block_time
+ block_time

- sumeragi.commit_time
+ commit_time

- sumeragi.max_transactions_in_block
+ max_transactions_in_block

- wsv.transaction_limits
+ wsv_transaction_limits

- wsv.identifier_length_limits
+ wsv_identifier_length_limits

- wsv.domain_metadata_limits
+ wsv_domain_metadata_limits

- wsv.account_metadata_limits
+ wsv_account_metadata_limits

- wsv.asset_definition_metadata_limits
+ wsv_asset_definition_metadata_limits

- wsv.asset_metadata_limits
+ wsv_asset_metadata_limits

- wsv.wasm_runtime.fuel_limit
+ wasm_runtime_fuel_limit

- wsv.wasm_runtime.memory_limit
+ wasm_runtime_memory_limit

---

upd: I forgot that this work is already done in Iroha:

https://github.com/hyperledger/iroha/blob/5578aaa16fc70cf6dbf9a11e35ffbc8a15b5b389/data_model/src/lib.rs#L233-L245

So, we can adopt it.

[Mingela]

Also, there is ConfigurationEvent (Changed, Created, Deleted)... I guess it should be split into two kinds of events:

• ParameterEvent
• ChainConfigurationEvent

Could you elaborate on the scenarios the events are gonna be thrown respectively upon?
Created and Deleted definitely does not make sense for ChainConfiguration since those are always initialized either by defaults or user input and can change over time, cannot be just removed.. Until there is a runtime upgrade changing the configuration format 🤔

[0x009922]

Could you elaborate on the scenarios the events are gonna be thrown respectively upon?

• ParameterEvent: same as current ConfigurationEvent
  • Created, payload: parameter name
  • Changed, payload: parameter name
  • Maybe Deleted, although now it never occurs
• ChainConfigurationEvent
  • Changed, payload: the whole updated on-chain configuration? a changeset with values? without values?
    upd: this event might have not any variants and be simply called ChainConfigurationChangedEvent

[Mingela]

• Changed, payload: the whole updated on-chain configuration? a changeset with values? without values?

The whole configuration state will be the easiest and the most fail-safe option I guess

[0x009922]

Overall design so far

Points:

• Terms:
  • Core Parameters: statically known chain-wide parameters, configuring consensus timings, data limits, and WASM
    limits.
  • Custom Parameters: dynamic key(String)-value(JSON) entries. Primary use case—to make use of them in the
    executor, which can have any business logic around it.
• Iroha Lifecycle:
  • Genesis transaction must set executor and all core parameters as very first instructions.
  • All core parameters must be set explicitly in the genesis transaction.
  • Unless a peer receives executor & core parameters, it is in "pending" state, where World State View is not even
    initialised. Since the state requires having some executor and core parameters, by having "pending" state, we can
    avoid having Executor::Initial and assumptions about core parameters defaults. It's more of an implementation
    detail.
• Data Model:
  • Parameter entity represents both core parameters and custom (i.e. executor) parameters, in a unified
    way. Core parameters are represented in a static way, while custom parameters are dynamic key-value pairs.

Here is an overview of the updated data model:

/// Unified representation of configuration parameter
enum Parameter {
    Core(CoreParameter),
    Custom(CustomParameter)
}

enum CoreParameter {
    BlockTimeMs(u64),
    CommitTimeMs(u64),
    // -- snip --
}

struct CustomParameter {
    id: CustomParameterId,
    payload: JsonString
}

/// Unified ID of a [`Parameter`] - needed for event filters
enum ParameterId {
    Core(CoreParameterId),
    Custom(CustomParameterId)
}

enum CoreParameterId {
    BlockTimeMs,
    CommitTimeMs,
    // -- snip --
}

struct CustomParameterId {
    name: Name
}

/// Core parameters as a structure. Convenient for internal use and
/// also as a result of [`QueryBox::FindConfiguration`]
struct CoreParametersState {
    block_time_ms: u64,
    commit_time_ms: u64
    // ...
}

// ***** ISI *****

enum InstructionBox {
    // -- snip --
    SetParameter(Parameter)
}

// ***** Query *****

enum QueryBox {
    // -- snip --
    FindConfiguration
}

struct FindConfigurationResult {
    /// A complete structure of core parameters
    core: CoreParametersState,
    custom: Vec<CustomParameter>
}

// ***** Events *****

/// Origin - [`ParameterId`]
enum ConfigurationEvent {
    ParameterSet(Parameter),
}

/// Part of the `EventFilter::Data::Configuration` -> 
struct ConfigurationEventFilter {
    id_matcher: Option<ParameterId>,
    /// if there is only one event, we don't need a set for it, do we?
    event_set: ConfigurationEventSet
}

How setting parameters would look in JSON ISI format:

[
  {
    SetParameter: {
      Core: {
        BlockTimeMs: 1000
      }
    }
  },
  {
    SetParameter: {
      Custom: {
        id: "foo",
        payload: {
          bar: 42
        }
      }
    }
  }
]

Questions:

1. Do we need to support filtering configuration events by parameter id? Implications if we do not:
   • Can remove ParameterId and CoreParameterId structs. CustomParameterId is still present.
   • Users get events regarding any parameters changes
2. From the data model standpoint, it is impossible to delete a custom parameter. Shall we support it? Implications:
   • Applies only to custom parameters, not core ones
   • New instruction: DeleteCustomParameter (in addition to SetParameter)
   • New event: ConfigurationEvent::CustomParameterDeleted
3. Alongside the SetParameter instruction, which sets a single core/custom parameter at a time, I would like to have
   an instruction which sets all core parameters at once. A specific use case for it: genesis transaction. It must
   set all core parameters anyway, and setting them via a sequence of SetParameter is clumsy. May it
   be SetCoreParameters(CoreParametersState) instruction? Implications:
   • New event: ConfigurationEvent::CoreParametersSet. Doesn't have ParameterId as an origin, hence could not be
     filtered by it.
   • It might also be useful for users, not only in genesis block.

[Mingela]

2. From the data model standpoint, it is impossible to delete a custom parameter. Shall we support it? Implications:

I guess we should provide the same interface for core and custom parameters, the special value can be defined for a custom parameter to treat that removed/reset (e.g. 0 or -1 for numerics, empty string, etc) by the parameter creator.

3. Alongside the SetParameter instruction, which sets a single core/custom parameter at a time, I would like to have
   an instruction which sets all core parameters at once. A specific use case for it: genesis transaction. It must
   set all core parameters anyway, and setting them via a sequence of SetParameter is clumsy. May it
   be SetCoreParameters(CoreParametersState) instruction? Implications:

A sequence seems fine. What about allowing an array of parameters to be expected by the ISI? That way a number of specific events can be emitted with a reasonable effort required to support such.

[mversic]

2. From the data model standpoint, it is impossible to delete a custom parameter. Shall we support it? Implications:

no, just like we don't support it for permission tokens. You can create/delete a token/parameter by upgrading executor

[mversic]

3. Alongside the SetParameter instruction, which sets a single core/custom parameter at a time, I would like to have
   an instruction which sets all core parameters at once. A specific use case for it: genesis transaction. It must
   set all core parameters anyway, and setting them via a sequence of SetParameter is clumsy. May it
   be SetCoreParameters(CoreParametersState) instruction? Implications:

I find this unconvincing. genesis.json can have any format, therefore you don't have to introduce another ISI to have API you want to have

[0x009922]

Summarizing comments & discussion:

1. Filtering config events by parameter id on the server-side - redundant. Clients can filter them on their side, and there should no be much traffic with this kind of events.
2. No, we wont introduce neither DeleteCustomParameter ISI nor CustomParameterDeleted. However, see p.4
3. No, it is extra. SetParameter is enough.
4. Like with Permission Tokens, those can be deleted only during the executor upgrade. In that case, the whole updated permission tokens schema is emitted. We decided to do the same for custom parameters: just a whole set of custom parameters will be emitted on upgrade, perhaps as a payload of the Upgrade event.

[0x009922]

Thought of an alternative, simpler design, inspired by how Custom instruction is implemented in #4645

Instead of introducing Executor Data Model concept (#4597), we can:

• Remove Parameters entirely
• Have a single Configuration structure with all core fields and a single custom: Option<String> field which Executor might read and interpret in any way it wishes
• Introduce Configure(Configuration) ISI and FindConfiguration Query

Pros:

• No need to touch Permissions and Permission Schema
• No need to introduce Executor Data model and define executor parameters there
• A single entity for chain-wide configuration

Cons:

• No atomicity in instructions/events/queries

Will have a prototype and see

[mversic]

we need Executor Data Model schema, that was a good concept. Users need a query to find out what custom permissions, parameters or instructions are available. We can discuss on how to handle parameters. Excluding parameters data model schema should look like:

struct ExecutorDataModel {
    permissions: Vec<PermissionId>,
    instructions: Vec<Name>,
    // parameters: Vec<ParameterId>
    
    schema: JsonString
}

having just one query FindExecutorDataModel instead of FindPermissionTokenSchema + FindCustomInstructionsSchema + FindParameterSchema is also fine

[mversic]

@0x009922 can we consider this ticket finished?

[0x009922]

I think so!