Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dynamic parameter value #4352

Closed
mversic opened this issue Mar 11, 2024 · 13 comments
Closed

dynamic parameter value #4352

mversic opened this issue Mar 11, 2024 · 13 comments
Assignees
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested
Milestone

Comments

@mversic
Copy link
Contributor

mversic commented Mar 11, 2024

          1. I think that we might want to split `***Limits` into it's constituents
  1. I think that having Numeric is sufficient because values should be loosely typed and checked in executor

Given the previous 2 points, I think that instead of ParameterValueBox we could just have StringWithJson as we do for permission tokens. This is because the value for parameters defined in executor can indeed be anything. Even though there are some hard coded, I don't think it's necessarily worth to have them strongly typed inside an enum

This is something I think should be explored in a separate PR

Originally posted by @mversic in #4305 (comment)

@mversic mversic added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Mar 11, 2024
@dima74 dima74 self-assigned this Apr 23, 2024
@mversic mversic added the question Further information is requested label Apr 24, 2024
@mversic mversic added this to the 2.0.0-rc.1 milestone Apr 24, 2024
@Erigara
Copy link
Contributor

Erigara commented Apr 25, 2024

Relevant here #4028.

We should consider that there might be arbitrary parameters used by executor and we have hard-coded parameters used by wasm engine, sumeragi.

Maybe we can define all on-chain parameters which look like smt like this:

struct Parameters {
    block_time: Option<Duration>,
    commit_time: Option<Duration>,
    wsv_asset_metadata_limits: Option<MetadataLimits>,
    // ...
    /// Can contain arbitrary key-value pairs to be used by executor
    executor_params: Map<Name, StringWithJson>,
}

What do you think @mversic @0x009922?

I should notice that executor already can kinda have arbitrary parameters by either using Parameters as they exist right now or executor can create domain and use it's metadata for storing it's state.

@mversic
Copy link
Contributor Author

mversic commented Apr 25, 2024

I should notice that executor already can kinda have arbitrary parameters by either using Parameters as they exist right now or executor can create domain and use it's metadata for storing it's state.

That's a good point. Are you saying that we can just reuse Domain::Metadata or any other metadata for custom parameters? I agree here with you, there doesn't seem to be any need for a special entity. Metadata seems like a valid place to put parameters. In this case we can just remove ParameterValueBox

@mversic
Copy link
Contributor Author

mversic commented Apr 25, 2024

We should consider that there might be arbitrary parameters used by executor and we have hard-coded parameters used by wasm engine, sumeragi.

what's the problem here?

@Erigara
Copy link
Contributor

Erigara commented Apr 25, 2024

what's the problem here?

It's not exactly a problem, but it would be nice if crucial on-chain parameters would be strongly typed with exact types (not stored as random JSON strings)

@Erigara
Copy link
Contributor

Erigara commented Apr 25, 2024

Are you saying that we can just reuse Domain::Metadata or any other metadata for custom parameters?

Yes, it can place them in any metadata, since permissions for the executor aren't checked.

@0x009922
Copy link
Contributor

I don't mind any format for executor parameters as long as WASM/Sumeragi parameters are strictly specified and typed.

@mversic
Copy link
Contributor Author

mversic commented Apr 25, 2024

what's the problem here?

It's not exactly a problem, but it would be nice if crucial on-chain parameters would be strongly typed with exact types (not stored as random JSON strings)

that's internally, no? Externally (from the client side), we should do it in the same way, i.e. as JSON string?

@Erigara
Copy link
Contributor

Erigara commented Apr 25, 2024

Not sure, i think if we expect for commit_time u64 for example that is the thing client should send us not arbitrary JSON string which would require second round of parsing.

@mversic
Copy link
Contributor Author

mversic commented Apr 25, 2024

Not sure, i think if we expect for commit_time u64 for example that is the thing client should send us not arbitrary JSON string which would require second round of parsing.

if performance is a concern, I expect updating a parameter to be really a rare operation. If the fallibility is concern, I your operation across network can fail for miriad of reasons.

I'm trying to formulate the simplest API, and not have specialized API for different edge cases unless really needed. It also complicates the code base

@mversic
Copy link
Contributor Author

mversic commented Apr 25, 2024

if we store parameters in metadata then we would have two different ways to update parameters SetKeyValue for custom and SetParameter for hard-coded. Seems confusing. That's why I'm more for having a separate storage for parameters apart from metadata

@0x009922
Copy link
Contributor

For context, what I think hard-coded configuration should look like, in a form of structures in the data model:

struct ConfigurationState {
  max_transactions_in_block: U32,
  block_time_ms: u64,
  commit_time_ms: u64,
  // metadata limits, wasm limits...
}

struct ConfigurationStatePartial {
  max_transactions_in_block: Option<u32>,
  block_time_ms: Option<u64>,
  commit_time_ms: Option<u64>,
  // ...
}

enum InstructionBox {
  // ...
  Configure(ConfigureBox)
}

struct ConfigureBox(ConfigurationStatePartial);

And also to patch the ConfigurationEvent from:

pub enum ConfigurationEvent {
    Changed(ParameterId),
    Created(ParameterId),
    Deleted(ParameterId),
}

to

pub enum ConfigurationEvent {
    Changed(ConfigurationState),
}

As for current parameters, I don't understand what is going to happen to them yet. It seems we need to clearly distinguish "static parameters" from "dynamic, custom parameters" (maybe call it "executor parameters" explicitly?). Both sets are a part of "chain wide configuration".

What are your guys opinion on the data model changes I would like to introduce, and on how we can distinguish these two sets of parameters?

@Erigara
Copy link
Contributor

Erigara commented Apr 26, 2024

What are your guys opinion on the data model changes I would like to introduce

I agree about struct for hard-coded configuration.

@0x009922 0x009922 self-assigned this Apr 29, 2024
@0x009922
Copy link
Contributor

Assigned to myself, as I see it as a natural part of #4028.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants