Skip to content

Conversation

@s8sato
Copy link
Contributor

@s8sato s8sato commented Mar 16, 2025

Context

Tracking issue:

This PR introduces an abstract structure applicable to executables, events, event filters, and permissions to streamline the execution cycle.

  • No API changes are introduced at this stage.
  • The iroha_tree::*::transitional submodules demonstrate the potential to replace the current structure.
  • Partially bypasses existing logic behind the non-default prediction feature, enabling loop detection at trigger registration time (for non-Wasm executables).

Since I was unsure whether this change would be a viable atomic change until opening this PR, no RFC was written beforehand. Let's discuss it here.

Related Work

Migration Guide

  • transitional submodules serve as interfaces between new and existing structures.
  • Any future API changes will result from resolving and removing these transitional submodules.

Review Notes

classDiagram
%% note "This is a general note"
%% class Tree
%% class FuzzyTree
class StateView
class ReadSet
class ChangeSet:::prediction
class Event:::prediction
class Receptor:::prediction
class Permission
class Add
class BitOr

classDef prediction color:#f9f

%% FuzzyTree <-- Tree

%% Tree <|.. StateView
%% FuzzyTree <|.. ReadSet
%% Tree <|.. ChangeSet
%% Tree <|.. Event
%% FuzzyTree <|.. Receptor
%% FuzzyTree <|.. Permission

StateView <-- ReadSet : load()
Event <-- StateView : as_status()
Event <-- ChangeSet : as_status()
Permission <-- StateView : passes()
Permission <-- ChangeSet : passes()
Receptor <-- Event : passes()
Receptor .. Permission : same repr
ChangeSet <-- Receptor : triggers

Add <|-- ChangeSet : implements
BitOr <|-- Permission : implements

Loading

For the loop detection feature:

  • Unit test: iroha_tree::state::tests::detects_event_loop
  • Integration test: iroha::mod::triggers::not_registered_when_potential_event_loop_detected

The crate documentation provides an abstract:

iroha_tree

A transitional crate that might eventually be merged into other crates.

It aims to integrate executables, events, and event filters while enabling recursive trigger prediction in a static manner.
The prediction is based on the union of possible execution paths, enabling pessimistic event loop detection.

Additionally, to maintain performance while enabling per-transaction triggers (#4937), it consolidates:

  • Instructions into a single [ChangeSet] per transaction.
  • (Data) events into a single [Event] per transaction.
  • (Data) event filters into a single [Receptor] per trigger.
  • Permissions, roles, and ownerships into a single [Permission] per validation.

iroha_authorizer

A crate enabling user-defined logic to authorize or reject
executables and queries based on the authority’s permissions and ownerships.

This is a stripped-down version of the executor, focused solely on permission validation (#5357).
It does not define or execute instructions.

Checklist

  • I've read CONTRIBUTING.md.
  • I've written unit tests for the code changes.
  • All review comments have been resolved.
  • All CI checks pass.

@s8sato s8sato added the Enhancement New feature or request label Mar 16, 2025
@s8sato s8sato added this to the 2.0.0-rc.2.0 milestone Mar 16, 2025
@s8sato s8sato self-assigned this Mar 16, 2025
@s8sato s8sato linked an issue Mar 16, 2025 that may be closed by this pull request
@s8sato s8sato removed a link to an issue Mar 16, 2025
@s8sato s8sato marked this pull request as ready for review March 16, 2025 23:14
@0x009922 0x009922 self-assigned this Mar 24, 2025
Comment on lines +731 to +737
match inst.level {
dm::Level::TRACE => iroha_logger::trace!(target: TARGET, "{}", inst.msg),
dm::Level::DEBUG => iroha_logger::debug!(target: TARGET, "{}", inst.msg),
dm::Level::INFO => iroha_logger::info!(target: TARGET, "{}", inst.msg),
dm::Level::WARN => iroha_logger::warn!(target: TARGET, "{}", inst.msg),
dm::Level::ERROR => iroha_logger::error!(target: TARGET, "{}", inst.msg),
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side effect should probably not occur during the TryFrom conversion, which in all other ways is a pure data transformation.

Isn't critical though, but maybe it's possible to have some sort of "side-effect" node?

Comment on lines +8 to +11
/// 0. Transaction: [register Carol]
/// 0. Trigger execution: account created (Carol) -> mint roses for Carol
/// 0. Transaction: [burn one of Carol's roses] ... Depends on the previous trigger execution
/// 0. Block commit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 0. Transaction: [register Carol]
/// 0. Trigger execution: account created (Carol) -> mint roses for Carol
/// 0. Transaction: [burn one of Carol's roses] ... Depends on the previous trigger execution
/// 0. Block commit
/// 1. Transaction: [register Carol]
/// 2. Trigger execution: account created (Carol) -> mint roses for Carol
/// 3. Transaction: [burn one of Carol's roses] ... Depends on the previous trigger execution
/// 4. Block commit

@s8sato s8sato mentioned this pull request Apr 25, 2025
@s8sato s8sato marked this pull request as draft May 7, 2025 23:39
@s8sato
Copy link
Contributor Author

s8sato commented May 7, 2025

Converted to draft because the second feature commit is no longer planned (now classified as Future Consideration).

Comment on lines +141 to +145
(Trigger, TriggerK, TriggerKF: dm::TriggerId),
(Condition, ConditionK, ConditionKF: tr::ConditionId),
(Executable, ExecutableK, ExecutableKF: tr::ExecutableId),
(TriggerCondition, TriggerConditionK, TriggerConditionKF: dm::TriggerId, tr::ConditionId),
(TriggerExecutable, TriggerExecutableK, TriggerExecutableKF: dm::TriggerId, tr::ExecutableId),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This design breaks down the current all-in-one Action object into separate parts—the condition, the execution, and the mapping between them—to maximize reusability. For example, the "deposit to self" condition only needs a single global definition rather than one per trigger, and you don’t have to register the same execution component twice just because it’s invoked manually versus event-driven.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This analysis of the pros and cons of the main commit matches about 60% of my own understanding. I intend to refine this analysis and, based on it, rewrite this commit as a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants