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

[feature] #1619: Introduce event-based triggers #1874

Merged

Conversation

appetrosyan
Copy link
Contributor

@appetrosyan appetrosyan commented Feb 7, 2022

Description of the Change

Add TriggerSet and carry trigger recommendations through the lifecycle of a block.

TriggerSet is stored in WSV.

Issue

Closes #1619
Closes #1859
Related to #1771

Benefits

Event triggers

Possible Drawbacks

Prototype implementation, so both UX and reliability are untested.

Event processing is quadratic.

Opens #1889
Opens #1890
Opens #1891
Opens #1892

@appetrosyan appetrosyan self-assigned this Feb 7, 2022
@appetrosyan appetrosyan requested review from mversic and s8sato February 7, 2022 05:17
@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Feb 7, 2022
@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #1874 (d00ceeb) into iroha2-dev (7f51a10) will decrease coverage by 0.77%.
The diff coverage is 46.12%.

Impacted file tree graph

@@              Coverage Diff               @@
##           iroha2-dev    #1874      +/-   ##
==============================================
- Coverage       79.02%   78.24%   -0.78%     
==============================================
  Files             163      164       +1     
  Lines           22243    22528     +285     
==============================================
+ Hits            17577    17627      +50     
- Misses           4666     4901     +235     
Impacted Files Coverage Δ
core/src/lib.rs 65.29% <ø> (ø)
core/src/smartcontracts/isi/asset.rs 68.12% <ø> (ø)
core/src/smartcontracts/isi/mod.rs 74.32% <ø> (ø)
core/src/tx.rs 97.87% <ø> (ø)
data_model/src/transaction.rs 66.89% <ø> (ø)
data_model/src/lib.rs 49.42% <5.06%> (-19.47%) ⬇️
core/src/triggers.rs 8.33% <8.33%> (ø)
data_model/src/events/data.rs 35.46% <9.37%> (-12.76%) ⬇️
data_model/src/events/mod.rs 44.23% <25.00%> (-6.99%) ⬇️
data_model/src/events/pipeline.rs 90.07% <39.13%> (-9.93%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f51a10...d00ceeb. Read the comment docs.

@appetrosyan appetrosyan force-pushed the i2-trigger-events branch 2 times, most recently from be488e7 to 7c706f3 Compare February 7, 2022 18:22
@appetrosyan appetrosyan force-pushed the i2-trigger-events branch 4 times, most recently from 60c5053 to 6c4fb22 Compare February 9, 2022 08:30
@appetrosyan appetrosyan marked this pull request as ready for review February 9, 2022 08:36
@appetrosyan appetrosyan requested a review from Arjentix as a code owner February 9, 2022 08:36
@appetrosyan appetrosyan requested a review from s8sato February 9, 2022 10:07
/// Find all actions which match the current events.
pub fn actions_matching(&self, events: &[Event]) -> Vec<Action> {
let mut result = Vec::new();
for event in events {
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 is quadratic, because DashMap is keyed in the trigger's name.

A solution would be to keep a few DashMaps with specialised filters, and custom logic. I'd leave that for a much later stage if possible.

@appetrosyan appetrosyan force-pushed the i2-trigger-events branch 2 times, most recently from 21cd85f to 38f5885 Compare February 10, 2022 08:12
@appetrosyan appetrosyan marked this pull request as draft February 10, 2022 08:13
@appetrosyan appetrosyan requested a review from mversic February 10, 2022 17:28
@appetrosyan appetrosyan marked this pull request as ready for review February 10, 2022 17:28
@@ -312,11 +334,14 @@ impl ChainedBlock {
.map(VersionedRejectedTransaction::hash)
.collect::<MerkleTree<_>>()
.root_hash();
let trigger_recommendations = self.trigger_recommendations;
// TODO: Validate trigger recommendations somehow?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still under considering about how to validate the recommendations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used to think that the technical accounts would be the solution. But the problem with them, is that you either don't attach a signature, store the private key on-chain or in a node, either of which defeats the point of validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is left to #1891, right?

Comment on lines +369 to +370
// TODO: This should properly process triggers
let trigger_recommendations = Vec::new();
Copy link
Contributor

@s8sato s8sato Feb 11, 2022

Choose a reason for hiding this comment

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

From what this PR is suggesting,
wsv.triggers.recommendations works as a temporary store of the recommendations
which is based on events emitted by previous block application, right?
Then Sumeragi somehow can receive the recommendations from WSV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

I'm wondering if there's more kinds of validation that we could do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you create an issue for this TODO?

@appetrosyan appetrosyan force-pushed the i2-trigger-events branch 2 times, most recently from 26686a7 to 8e33775 Compare February 11, 2022 07:02
mversic
mversic previously approved these changes Feb 11, 2022
@appetrosyan appetrosyan requested a review from s8sato February 11, 2022 11:59
@appetrosyan appetrosyan requested a review from mversic February 11, 2022 12:49
@appetrosyan appetrosyan added the Enhancement New feature or request label Feb 12, 2022
@appetrosyan appetrosyan force-pushed the i2-trigger-events branch 5 times, most recently from b684b60 to 86b0bc7 Compare February 17, 2022 14:10
@@ -312,11 +334,14 @@ impl ChainedBlock {
.map(VersionedRejectedTransaction::hash)
.collect::<MerkleTree<_>>()
.root_hash();
let trigger_recommendations = self.trigger_recommendations;
// TODO: Validate trigger recommendations somehow?
Copy link
Contributor

Choose a reason for hiding this comment

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

This is left to #1891, right?

Comment on lines +369 to +370
// TODO: This should properly process triggers
let trigger_recommendations = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you create an issue for this TODO?

@appetrosyan appetrosyan merged commit 2102df4 into hyperledger-iroha:iroha2-dev Feb 18, 2022
@appetrosyan appetrosyan deleted the i2-trigger-events branch February 18, 2022 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants