-
Notifications
You must be signed in to change notification settings - Fork 283
refactor(wasm): make wasm execution part of Instruction #5465
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
crates/iroha/src/client.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Since WASM execution is now at the instruction level, the runtime (fuel) should be reused from the transaction
Regardless of whether the Wasmtime instance is reused, the host can keep track of how much fuel has been consumed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #5494 . Separated because this PR is too big already.
// TODO: Can we check the number of instructions in wasm? Because we do this check | ||
// when executing wasm where we deny wasm if number of instructions exceeds the limit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counting write accesses at the static-analysis stage in a highly flexible language like Wasm—leave aside a DSL—is not realistic. A runtime approach would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
// TODO: Can we check the number of instructions in wasm? Because we do this check | ||
// when executing wasm where we deny wasm if number of instructions exceeds the limit. | ||
// | ||
// Should we allow infinite instructions in wasm? And deny only based on fuel and size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write-access limits and fee-based deterrence provide effective baseline defenses, but there are still gaps:
-
Write-access limit loophole
An attacker can spread many write requests over multiple transactions.
Mitigation: Enforce epoch-level limits by capping the total requests per account. -
Fee-based deterrence loophole
A resourceful attacker can exploit lightweight loops or simply pay high fees.
Mitigation: To be determined.
crates/iroha_data_model/src/isi.rs
Outdated
/// Execute [`WasmSmartContract`]. | ||
Smartcontract(WasmExecutable<WasmSmartContract>), | ||
/// Execute [`TriggerModule`] . | ||
Trigger(WasmExecutable<TriggerModule>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than distinguishing between instant binary build-and-run (smart contracts) and pre-registered module calls (triggers), I suggest splitting the workflow into two operations: register (build) and call (run).
- Enhanced extensibility:
Smart contracts are execution units, while triggers are mapping from conditions to execution unis. By introducing four distinct commands—register execution unit, invoke execution unit, register condition, and register mapping—we maximize reusability. This approach even integrates custom instructions. - No adverse memory impact:
The register command only stores hashes of blobs in memory. - Preserved instant execution:
Bundling register and call in a single transaction provides immediate execution, and UI can seamlessly abstract the two-step process from users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface was changed to include only the execution of the wasm binaries. It is possible to allow registering binaries and later calling them by hash. However, for the smartcontacts users already have the binaries, and for triggers, they need to provide an event
, which is not realistic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, it’s possible to provide a UX that executes the binary directly while internally splitting it into two stages—registration and invocation.
Also, since this change pertains to the executable, its concerns should be separable from triggers, and commit 89a2b59 has nearly achieved that.
If you still plan something that conflicts with the latest comment in the original issue, it should be documented in that issue, since it would overturn the assumptions of all higher-level issues.
crates/iroha/src/client.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be desirable to have a test demonstrating that multiple Wasm instructions can be used within a single transaction like a method chain.
Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
Signed-off-by: Lohachov Mykhailo <[email protected]>
Serialize, | ||
IntoSchema, | ||
)] | ||
pub enum InstructionBoxHashed { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think such redundancy across the entire InstructionBox
, including builtin instructions, is unavoidable. Wouldn’t it be enough to just add two variants, such as WasmRegister(blob)
and WasmCall(hash)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine it this way. Internally, for a trigger to be called, it needs to have:
- A binary or a hash of the binary
- A triggering action
As I understand it, WasmCall(hash)
implies the action is a "manual call." In that case, there's no real difference between WasmCall(hash)
and the already existing CallTrigger(id)
.
It's not a problem to add WasmCall(hash)
or WasmRegister(blob)
, but for a complete trigger representation, we must also include the Action
, for example, WasmCall(hash)
should become WasmCall(hash, action)
.
This would be acceptable if we add checks to discard user-submitted calls with non-manual call actions. That would significantly relax the internal representation. However, personally, I believe that if an interface allows something and later rejects it, then the interface is poorly designed, especially for strongly-typed languages.
Another concern is with the developer-facing interface. If InstructionBox
is used in place of InstructionBoxHashed
, there's no guarantee that binaries will always be hashed, and future developers may mistakenly add them directly. I dislike this duplication but don’t see a viable way to avoid it (e.g., with generics or nested enums).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we’re not on the same page about a few points. Let me clarify.
An instruction itself doesn’t dictate whether it’s run manually or automatically. That’s determined by whether the instruction is included in a user’s transaction or in a trigger execution.
As mentioned, a trigger is basically a mapping from the condition part (2. A triggering action) to the execution part (1. A binary or a hash of the binary), or a pair of them. An instruction—being a pure building block of the execution part—has nothing to do with the condition part.
Since the World
model doesn’t yet support registering condition and execution parts separately, I assume at least three new fields instead of triggers
(see https://github.com/hyperledger-iroha/iroha/pull/5355/files#r2204613082). The existing Action
structure, which no longer matches our reality, will then be dismantled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An instruction—being a pure building block of the execution part—has nothing to do with the condition part.
More precisely, the Wasm execution component will accept the triggering event as an optional input.
However, the main point here is that the Wasm instructions can be registered regardless of who will invoke them or under what conditions.
crates/iroha_data_model/src/isi.rs
Outdated
/// Execute [`WasmSmartContract`]. | ||
Smartcontract(WasmExecutable<WasmSmartContract>), | ||
/// Execute [`TriggerModule`] . | ||
Trigger(WasmExecutable<TriggerModule>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, it’s possible to provide a UX that executes the binary directly while internally splitting it into two stages—registration and invocation.
Also, since this change pertains to the executable, its concerns should be separable from triggers, and commit 89a2b59 has nearly achieved that.
If you still plan something that conflicts with the latest comment in the original issue, it should be documented in that issue, since it would overturn the assumptions of all higher-level issues.
vec![ | ||
WasmExecutable::binary(load_sample_wasm("mint_rose_smartcontract")), | ||
WasmExecutable::binary(load_sample_wasm("mint_rose_smartcontract")), | ||
WasmExecutable::binary(load_sample_wasm("mint_rose_smartcontract")), | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To demonstrate the dependencies between instructions, for example, an asset relay via Transfer would be a preferable.
use mint_rose_trigger_data_model::MintRoseArgs; | ||
|
||
#[test] | ||
fn multiple_wasm_triggers_in_transaction() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s no need to add any guarantees about the by-call trigger. It will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This topic is closely related to #5475 (comment):
In my view, by-call triggers will disappear in the near future with the resolution of #5147 or its derived issues due to the inherent contradiction in their role. A trigger is essentially a mapping from internal conditions to the execution unit, and if a user invokes the execution unit directly, it’s simply another kind of instruction.
First part of #5147 (Draft)
This PR refactors the following:
As a result:
visit_execute_wasm_smartcontract
.Breaking changes:
Executable::Wasm
toExecutable::Instruction
(WasmExecutable<WasmSmartContract>
).Instruction
in contrast toTransaction
.Issues to be addressed:
It is planned to be addressed in Unification of wasm fuel across runtimes #5494.