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

fix: add event index to transaction events #417

Conversation

janniks
Copy link
Contributor

@janniks janniks commented Sep 18, 2023

### Description

Improves how we handle a restart of `chainhook service` while predicates
are scanning/streaming. Here are the cases we now handle:
1. Predicates that were in `scanning` status when Chainhook was
terminated will resume scanning starting from their
`last_evaluated_block_height`. *Note: because we only save predicate
status every 10 scans, we could end up re-emiting matches on a resetart*
2. Predicates that were in `new` status when Chainhook was terminated
will start scanning at the predicate's `start_block`
3. Predicates that were in `streaming` status will _return_ to a
`scanning` status, starting at `last_evaluated_block_height` to catch up
on the missed blocks. Note, the `number_of_blocks_to_scan` is set to 0
for this temporary catch-up, as it's difficult to compute the number of
remaining blocks in the context of this change
4. If predicates were passed in at startup, we also register those to
begin scanning, which previously didn't happen
5. We now allow passing in a predicate at startup _and_ registering
additional predicates with the predicate registration server. This means
that if you use the same startup predicate repeatedly, it will already
be saved in redis and _not_ be reloaded.

Fixes: #298, fixes #390, fixes #402, fixes #403


---

### Checklist

- [x] All tests pass
- [ ] Tests added in this PR (if applicable)
@janniks janniks force-pushed the 387-return-event_index-in-stacks-transaction-event-metadata branch from 02945dd to 5d359ba Compare September 26, 2023 18:02
@janniks janniks marked this pull request as ready for review September 26, 2023 18:03
@MicaiahReid MicaiahReid force-pushed the 387-return-event_index-in-stacks-transaction-event-metadata branch from 5d359ba to 0578ce5 Compare October 3, 2023 16:47
Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

Thanks, @janniks!!

Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

I've discovered some ways where this may cause breakages, I need to do some investigation.

Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

Okay, it looks like it should be good!

Copy link
Contributor

@lgalabru lgalabru left a comment

Choose a reason for hiding this comment

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

Thanks @janniks! We already have the struc StacksTransactionPosition in place, indicating the position of a transaction in the block or in the microblock, could we use this instead?
Apologies for the latency on this review.

@janniks
Copy link
Contributor Author

janniks commented Oct 31, 2023

Oh yeah sure, sorry for missing that 👍

@janniks
Copy link
Contributor Author

janniks commented Nov 6, 2023

The new positions are referring to events not blocks. Should we create an enum type that mixes blocks and events (might be funky when matching). Or add an additional enum layer EventPosition (but that also sounds complected)?

@MicaiahReid
Copy link
Contributor

@lgalabru I don't know if I agree - Jannik's change is adding a struct for an event's position in a transaction, while StacksTransactionPosition represents a transaction's position within a block.

I guess we could abstract that position enum to be more general, from:

pub enum StacksTransactionPosition {
    AnchorBlock(AnchorBlockPosition),
    MicroBlock(MicroBlockPosition),
}

to:

pub enum Position {
    TransactionPositionInAnchorBlock(AnchorBlockPosition),
    TransactionPositionInMicroBlock(MicroBlockPosition),
    EventPositionInTransaction(u64)
}

but that feel's clunky. Let me know if we're misunderstanding what you meant here!

@janniks janniks requested a review from lgalabru February 12, 2024 17:37
Copy link
Contributor

@lgalabru lgalabru left a comment

Choose a reason for hiding this comment

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

My apologies for the latency on this PR, yes that makes sense to me.
Thank you for your help @janniks!

@MicaiahReid
Copy link
Contributor

Closing in favor of #495

MicaiahReid added a commit that referenced this pull request Feb 14, 2024
(this PR is just cherry-picking the relevant commits from @janniks in PR
#417, which I apparently mucked up with some bad merging)

- closes #387 
- adds a position struct to the event and renames the event to payload
(with enum type)
- updates occurences.json to include the expected serialized position
for testing

---

**Note**: Any users running a Chainhook node (i.e. `chainhook service
start ...`) will not have transaction event positions stored in the
database, so all event position will have a value of:
``` JSON
"position": {
  "index": 0
}
```
To rebuild the database with this data filled in:
 - Upgrade to the latest version of Chainhook
- Delete the `stacks.rocksdb` folder inside the `working_dir` set in
your `Chainhook.toml`
 - Rerun Chainhook

This will rebuild your Stacks database from scratch.

---------

Co-authored-by: janniks <[email protected]>
MicaiahReid added a commit that referenced this pull request Feb 14, 2024
(this PR is just cherry-picking the relevant commits from @janniks in PR
#417, which I apparently mucked up with some bad merging)

- closes #387 
- adds a position struct to the event and renames the event to payload
(with enum type)
- updates occurences.json to include the expected serialized position
for testing

---

**Note**: Any users running a Chainhook node (i.e. `chainhook service
start ...`) will not have transaction event positions stored in the
database, so all event position will have a value of:
``` JSON
"position": {
  "index": 0
}
```
To rebuild the database with this data filled in:
 - Upgrade to the latest version of Chainhook
- Delete the `stacks.rocksdb` folder inside the `working_dir` set in
your `Chainhook.toml`
 - Rerun Chainhook

This will rebuild your Stacks database from scratch.

---------

Co-authored-by: janniks <[email protected]>
github-actions bot pushed a commit that referenced this pull request Feb 14, 2024
## [1.3.1](v1.3.0...v1.3.1) (2024-02-14)

### Bug Fixes

* add event index to transaction events ([#495](#495)) ([d67d1e4](d67d1e4)), closes [#417](#417) [#387](#387)
* correctly determine PoX vs PoB block commitments ([#499](#499)) ([50dd26f](50dd26f)), closes [#496](#496)
vabanaerytk added a commit to vabanaerytk/chainhook that referenced this pull request Aug 7, 2024
## [1.3.1](hirosystems/chainhook@v1.3.0...v1.3.1) (2024-02-14)

### Bug Fixes

* add event index to transaction events ([#495](hirosystems/chainhook#495)) ([2d2760d](hirosystems/chainhook@2d2760d)), closes [#417](hirosystems/chainhook#417) [#387](hirosystems/chainhook#387)
* correctly determine PoX vs PoB block commitments ([#499](hirosystems/chainhook#499)) ([6816e76](hirosystems/chainhook@6816e76)), closes [#496](hirosystems/chainhook#496)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

return event_index in Stacks transaction event metadata
3 participants