Open 0x009922 opened 8 months ago
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.
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.
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.
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.
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?).
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 have no opinion here. Both is fine to me. I think it depends more on the end-user UX.
I'm agree with others, to me it seems to be too complicated for now.
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.
Summarising the comments and adding some other bits from my side, here is a complete changeset:
NewParameter
/SetParameter
instructions for chain-wide hard-coded configurationConfigureChain
. It will accept a flat structure with the known parameters aligned to a single namespace. All fields are optional.
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.configuration
. It will accept the same structure as the ConfigureChain
instruction, and will also apply default values to absent parameters.@Erigara @Arjentix @Mingela does it look satisfying and complete for you?
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...
does it look satisfying and complete for you?
Yes, looks nice
does it look satisfying and complete for you?
Yeah, lgtm
Also, there is ConfigurationEvent
(Changed
, Created
, Deleted
)... I guess it should be split into two kinds of events:
ParameterEvent
ChainConfigurationEvent
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:
So, we can adopt it.
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 🤔
Could you elaborate on the scenarios the events are gonna be thrown respectively upon?
ParameterEvent
: same as current ConfigurationEvent
Created
, payload: parameter nameChanged
, payload: parameter nameDeleted
, although now it never occursChainConfigurationEvent
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
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
Points:
Executor::Initial
and assumptions about core parameters defaults. It's more of an implementation
detail.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:
ParameterId
and CoreParameterId
structs. CustomParameterId
is still present.DeleteCustomParameter
(in addition to SetParameter
)ConfigurationEvent::CustomParameterDeleted
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:
ConfigurationEvent::CoreParametersSet
. Doesn't have ParameterId
as an origin, hence could not be
filtered by it.
- 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.
- 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 ofSetParameter
is clumsy. May it beSetCoreParameters(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.
- 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
- 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
Summarizing comments & discussion:
DeleteCustomParameter
ISI nor CustomParameterDeleted
. However, see p.4SetParameter
is enough.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:
Configuration
structure with all core fields and a single custom: Option<String>
field which Executor might read and interpret in any way it wishesConfigure(Configuration)
ISI and FindConfiguration
QueryPros:
Cons:
Will have a prototype and see
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
@0x009922 can we consider this ticket finished?
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 (https://github.com/hyperledger/iroha-2-docs/issues/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
andSetParameter
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:
wsv.wasm_runtime.fuel_limits
, but justwasm_fuel_limits
. Also, should the naming be aligned with naming of parameters in local configuration file? (or even in env?)"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:
Or a enum?