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.

Sign up for GitHub

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

Jump to bottom

Event monitor: Batch together all events from all transactions included in a block #958

Merged
merged 8 commits into from
May 21, 2021
Merged

Event monitor: Batch together all events from all transactions included in a block #958

merged 8 commits into from
May 21, 2021

Conversation

romac
Copy link
Member

@romac romac commented May 19, 2021 •
edited
Loading

Closes: #957

Description

Bulk events from all transactions included in a block by grouping all the IBC events extracted from the subscription stream by height.

This allows the supervisor to process all events emitted at the same height at once, and substantially improves performance (eg. relaying 250 packets now take ~4 seconds instead of a dozen minutes).

For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@romac
Copy link
Member Author

romac commented May 19, 2021 •
edited
Loading

We could improve a few things here, but I figured we can address those in a follow-up PR.

  • Statically express that group_while returns a vector of non-empty vectors.
  • Rewrite group_while as struct + Stream instance to avoid pulling in the async-stream crate.

@adizere adizere mentioned this pull request May 19, 2021
Closed
4 tasks
@adizere
Copy link
Member

adizere commented May 19, 2021

We could improve a few things here, but I figured we can address those in a follow-up PR.

Took the liberty to open #959 for capturing this.

adizere
adizere approved these changes May 19, 2021
View reviewed changes
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Looks good! I didn't catch any problems after multiple rounds of testing. I did catch a bug, however (#960) but it has nothing to do with the present PR.

LGTM!

@adizere adizere mentioned this pull request May 19, 2021
Merged
5 tasks
ancazamfir
ancazamfir requested changes May 20, 2021
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

For the record, as we discussed, filtering is broken, i.e. hermes listen ibc-1 -e Tx does not work anymore.
There is also a weird issue with re-ordering of events where NewBlock is interleaved with the Tx events. I believed not caused by this PR but found it while testing on this branch.

@ancazamfir ancazamfir mentioned this pull request May 20, 2021
Closed
5 tasks
@romac
Copy link
Member Author

romac commented May 20, 2021

For the record, as we discussed, filtering is broken, i.e. hermes listen ibc-1 -e Tx does not work anymore.
There is also a weird issue with re-ordering of events where NewBlock is interleaved with the Tx events. I believed not caused by this PR but found it while testing on this branch.

Fixed in 122954b

ancazamfir
ancazamfir approved these changes May 20, 2021
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Thanks Romain!!

@romac romac merged commit c84c352 into master May 21, 2021
@romac romac deleted the romac/batch-events branch May 21, 2021 07:10
@romac romac changed the title Event monitor: Bulk events from all transactions included in a block Event monitor: Batch together all events from all transactions included in a block May 21, 2021
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@romac
Event monitor: Bulk events from all transactions included in a block (i…
4f2f2d1 
…nformalsystems#958)

* event monitor: Bulk events from all transactions included in a block

* Update changelog

* Improve unit test to ensure items are not re-ordered

* Cleanup

* Ensure NewBlock event is always first in the event batch

* Re-enable events filtering in `listen` command

* Only print event batch header if there are matching events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ancazamfir ancazamfir ancazamfir approved these changes

@adizere adizere adizere approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

hermes does not bulk events from all transactions included in a block
3 participants
@romac @adizere @ancazamfir