feat: Custom instructions in executor

[dima74]

Description

Added ISI for custom instructions, and two tests demonstrating possible use:

• Test with one custom instruction MintAssetForAllAccounts
• Test with simple expression system

Linked issue

Closes #4599

Benefits

Checklist

• I've read CONTRIBUTING.md
• I've used the standard signed-off commit format (or will squash just before merging)
• All applicable CI checks pass (or I promised to make them pass later)
• (optional) I've written unit tests for the code changes
• I replied to all comments after code review, marking all implemented changes with thumbs up

[0x009922]

So, the approach of custom instruction in this PR is the following, as I understand:

• In Executor, users can define whatever parsing logic around Custom { payload: String } instruction
• On client side, users must know how to serialize the Custom instruction, and there is no way to know it via Iroha API

This goes against the current design. We had a discussion with @mversic, and the idea is that all executor-defined types must be defined in some schema and clients should be able to retrieve it and use it.

So, I guess this PR should came after #4597 and extend ExecutorDataModel with a set of instruction types, and then to have Custom instruction somehow this way:

struct Custom {
    id: CustomInstructionId, // or just Name?
    payload: JsonString
}

[mversic]

great work

[mversic]

So, I guess this PR should came after #4597 and extend ExecutorDataModel with a set of instruction types, and then to have Custom instruction somehow this way:

struct Custom {
    id: CustomInstructionId, // or just Name?
    payload: JsonString
}

I think you should make ExecutorDataModel a separate pr. And yeah, I was thinking that Custom should have an id field as you present here, but maybe there are some drawbacks?

[0x009922]

I think you should make ExecutorDataModel a separate pr.

Created #4658

And yeah, I was thinking that Custom should have an id field as you present here, but maybe there are some drawbacks?

IMO, technically, there is no need to have an id. This would suffice:

struct ExecutorDataModel {
  schema: String,

  // -- snip --

  // type id in the schema
  custom_instruction_definition_id: Option<Name>
}

enum Instruction {
  // -- snip --
  Custom(JsonString)
}

Clients will know how to serialize Custom if they look into the data model.

[mversic]

IMO, technically, there is no need to have an id. This would suffice:

there is obviously no need in this PR, but I still maintain it might be useful. Maybe for migrations. Will iroha_core ever have to know that two custom instructions are of the same type, i.e. will it have to compare them by id. When Custom instruction contains only payload it's not possible to do so without invoking executor to evaluate the whole instruction. I think id brings a little more flexibility and makes it more future proof

@Erigara do you have opinion on the matter?

[0x009922]

I think id brings a little more flexibility and makes it more future proof

I see your point, though, it sounds vague and this possible future-proof solution doesn't sound very robust to me. Due to that, I would prefer a minimal implementation now, as we are not 100% sure that having id would help in future.

[mversic]

I think id brings a little more flexibility and makes it more future proof

I see your point, though, it sounds vague and this possible future-proof solution doesn't sound very robust to me. Due to that, I would prefer a minimal implementation now, as we are not 100% sure that having id would help in future.

I don't have strong arguments for adding instruction id. Except that we could have nicer Display/Debug implementation, without dumping the whole payload. If we decide that instructions won't have ids, then permissions shouldn't either

I'd like some more opinions here: @Erigara @DCNick3

[mversic]

I see your point, though, it sounds vague and this possible future-proof solution doesn't sound very robust to me. Due to that, I would prefer a minimal implementation now, as we are not 100% sure that having id would help in future.

I didn't want to focus on future-proofing. It was more about iroha_core being able to categorize instructions. Because now there is no way for it to know whether 2 Custom instructions are of the same type nominally. Custom instructions are completely opaque

[Erigara]

Looks cool, give ability to implement custom instructions without too much changes.

One concern i can think of is that custom instructions work more like an extension, so executor can't for example completely replace isi with own DSL for example.

Alternative design i was thinking about was to make transaction payload opaque to the core iroha (just bytes essentially).
So executor has complete freedom in how to interpret this payload.
But this would involve much more work and i'm not sure we need such extensibility in the first place.

[Erigara]

It was more about iroha_core being able to categorize instructions.

Do we need this ability since whole custom isi execution is on the executor?
In general i don't see strong need in adding id.

[Erigara]

moreover, remove Permission::id in a separate PR to be consistent with this one here

having ids for permissions might be still helpful because we do lookups on then which is not the case for instructions

[mversic]

moreover, remove Permission::id in a separate PR to be consistent with this one here

having ids for permissions might be still helpful because we do lookups on then which is not the case for instructions

but the lookups are completely unnecessary. They only happen because we keep list of permission ids in wsv. There is no need for that

[mversic]

moreover, remove Permission::id in a separate PR to be consistent with this one here

having ids for permissions might be still helpful because we do lookups on then which is not the case for instructions

but the lookups are completely unnecessary. They only happen because we keep list of permission ids in wsv. There is no need for that

created #4702 for further discussion, but I don't see any reason to keep Permission::id if we don't have it for instructions