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

chore(sequencer): index all ABCI events #1786

Merged
merged 7 commits into from
Nov 15, 2024

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Nov 4, 2024

Summary

Instructs CometBFT to index all ABCI events emitted by Sequencer.

Background

This change instructs CometBFT to index all events emitted by Sequencer by setting their field index: true.

This allows programatically determining stuck IBC transactions to automatically trigger a timeout. Previously, the height that a given IBC packet was sent at had to be found manually before it could be passed to the Hermes CLI (Astria's IBC relayer of choice).

The indexing field is described in the CometBFT ABCI spec [1].

1: https://github.com/cometbft/cometbft/blob/16b1ed87c4b10118df10eb7175412db066bd09d5/spec/abci/abci%2B%2B_basic_concepts.md?plain=1#L318-L319

Changes

  • Index all event attributes before returning in app::execute_transaction

Testing

  • Added unit test to ensure all event attributes are indexed for the events we create (unable to unit test an IBC action).

Changelogs

Changelog updated

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Nov 4, 2024
@ethanoroshiba ethanoroshiba force-pushed the eoroshiba/index_all_send_packet_events branch from e599795 to 527eff1 Compare November 4, 2024 21:04
@ethanoroshiba ethanoroshiba changed the base branch from noot/ibc-fix to main November 4, 2024 21:04
@ethanoroshiba ethanoroshiba marked this pull request as ready for review November 4, 2024 21:10
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner November 4, 2024 21:10
@ethanoroshiba ethanoroshiba force-pushed the eoroshiba/index_all_send_packet_events branch from 0b67513 to 94416b1 Compare November 4, 2024 22:15
Copy link
Collaborator

@noot noot left a comment

Choose a reason for hiding this comment

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

the title and changelog should be updated to say all events are now indexed, not just send_packet ones. otherwise looks good!

@ethanoroshiba ethanoroshiba changed the title fix: index all sent packet event attributes fix: index all ABCI event attributes Nov 5, 2024
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Couple notes:

  1. If abci events are toggled on every execution, then this should be in execute_transactions and the code not duplicated in every fn execute_transactions_* method.
  2. Please expand the background section, right now it contains a placeholder.
  3. These changes are observable by providing a unit test on execute_transactions. The test section right now is equivalent to saying "this is not tested".
  4. This should further be observable by spinning up a sequencer network and checking the provided ABCI events.

@SuperFluffy SuperFluffy changed the title fix: index all ABCI event attributes chore(sequencer): index all ABCI events Nov 15, 2024
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

I have updated the patch title and description a bit (arguably, this is more chore-like or feat-like than a fix for sequencer proper).

I have also linked the relevant entry in the cometbft repository (preferable over a website that is more likely to change).

Please address @Fraser999's comment re the unnecessary filter adapter when enabled all indexing.

@ethanoroshiba ethanoroshiba added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@ethanoroshiba ethanoroshiba added the docker-build used to trigger docker builds on PRs label Nov 15, 2024
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit 740d9e7 Nov 15, 2024
67 checks passed
@ethanoroshiba ethanoroshiba deleted the eoroshiba/index_all_send_packet_events branch November 15, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker-build used to trigger docker builds on PRs sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants