-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Index Actor Events by ID #11594
Comments
@masih I had a look at my code and it wasn't quite as terrible as I imagined. I've tidied it up a little and made it all pass with the latest v1.26 @ #11694. It's just a draft and shouldn't necessarily be the way this ticket gets done. I have some other things to get done in the meantime so I've removed myself from assignee here but could come back to this if you don't happen to get to it in the course of what you're working on. I started looking at the migration problem but didn't get too far. I really want good tests for migrations and there are none in there yet. |
@Stebalien Question: What is the correct thing to do when migrating the reverted events |
But, honestly, I'd just delete the reverted events from the database in this case. I'm not quite sure why we record them anyways given that, unless I'm mistaken, we only return reverted events from the "subscribe" API. I guess we should add "remove reverted events from the database" to the TODO list... |
Since we are iterating over the events as part of the migration for this issue anyway, I can add reverted event removal right there. It should be straightforward |
On second thought, the event collection also needs to be updated to remove storing reverted events. This is a big-ish enough work to make a reasonable size PR on its own. Let's do that in a separate PR. |
@aarshkshah1992 was this address in the new chainindexer api by any chance? |
Having some discussion in https://github.com/filecoin-project/lotus/pull/12421/files#r1756198386, but because GitHub's linking into inline comments in PRs is kind of impermanent and often doesn't take you to a discussion, here's a screenshot: I would like a resolution on this one and am concerned about going forward without it in ChainIndexer if we believe this is the right way ahead. I'm just not sure now that it is the right thing to do. |
Being closed finally in #12421 by storing both the actor id and the |
Currently, we index actor events by the emitter's f4 address. This will have to change in #11540 to accommodate non-EVM actors, but really, we simply shouldn't be resolving addresses when we index events.
Instead:
Unlike the current system, this will work correctly with every address type, will result in more performant indexing, and will reduce the size of the database.
Unfortunately, the correct way to do this involves a migration.
The text was updated successfully, but these errors were encountered: