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

Exclude reverted events in the new events APIs #11770

Open
Stebalien opened this issue Mar 21, 2024 · 9 comments
Open

Exclude reverted events in the new events APIs #11770

Stebalien opened this issue Mar 21, 2024 · 9 comments

Comments

@Stebalien
Copy link
Member

Stebalien commented Mar 21, 2024

We need to tell the user about reverted events when they are subscribed to events, but we don't need to tell them about historically reverted events. In the future, we may add the ability to subscribe starting at some tip set. In that case, we will want to return all events reverted between the start tip set and the first tip set with new events, but that's a future problem.

We should probably also make the reverted field optional. I'm not sure, but it would be nice to not include it in events returned from get events, only from subscriptions.

@rvagg
Copy link
Member

rvagg commented Mar 21, 2024

Here's the problem I see with it:

  1. Use a continual SubscribeActorEventsRaw to keep track of ongoing events, always building a local state map of what you care about, or just sticking them in a database and stacking them up to create that state.
  2. SubscribeActorEventsRaw will regularly disconnect—both because there's a default timeout on the connections and because of network conditions. So you're going to need to handle reconnect cases.
  3. The most sensible thing to do I think is when you re-connect to a SubscribeActorEventsRaw is go fishing for some number of tipsets before your disconnect to make sure you're in sync and didn't lose anything.

In that case, I think I'd want to know about the historical reverts because there's a good chance that some of the non-revert events I have before my disconnect were reverted in the time before re-connecting and I'll have lost state.

The alternative workflow to this is to not use historical at all on SubscribeActorEventsRaw and just start at the latest tipset and then follow-up with a single GetActorEventsRaw to fill in your gaps. This should work out the same I think.

But then why offer historical on SubscribeActorEventsRaw at all?

@Stebalien
Copy link
Member Author

So, what we want to do is return reverts for to the events you've already observed. Unfortunately, the current APIs will never return useful reverts. given

1 2 3 4 5 6
a b c d <- head before unsubscribe
  e f g h i j
        ^- resubscribe at epoch 5

We want reverts for blocks B, C, and D. But we will only get reverts for blocks H, I, and J. I could resubscribe at epoch 2 after getting the chain path between my last head and the current head, but that's racy.

I think the only option given the current API is to call GetActorEventsRaw as you described, but (a) that flow kind of sucks and (b) one usually doesn't care about reverted events when calling GetActorEventsRaw.

At a minimum, I think we should stop pre-filling reverted events in SubscribeActorEventsRaw.

@Stebalien
Copy link
Member Author

But I think a better API would be to only return reverted historical events if we specify a start tipset (both in get and subscribe).

@rvagg
Copy link
Member

rvagg commented Mar 23, 2024

Right, so if you supply a tipset key then we can at least walk back to the common ancestor of current chain and give you what you need. I'm fine with that approach, because at least then I could get better guarantees of re-subscribing where I left off and getting the things I expect.

@Stebalien
Copy link
Member Author

I'm going to suggest an alternative proposal: drop the entire concept of "reverted" events (from our custom APIs) and instead tell the user when the chain itself has reverted.

Either do this by :

  1. Grouping events by block and then occasionally returning special revert block events.
  2. Interleaving multiple types of events: Actor Event, Block Event, Revert Event.

@rvagg
Copy link
Member

rvagg commented Apr 5, 2024

That might help, because the more I try and tackle this problem as a consume of events, the more of a nightmare state management becomes, to the point where I'm considering just holding back for 900 epochs and only consuming events from there and filtering out reverted events.

@Stebalien
Copy link
Member Author

to the point where I'm considering just holding back for 900 epochs and only consuming events from there and filtering out reverted events.

That's probably not going to work for most applications. Unfortunately, we don't have F3 yet.

Although one option is to implement a finality calculator and then add an option where the user can specify that they only want finalized events. That won't make reverts impossible, but all we really need is to make reverts extremely unlikely the next 6 months.

@rjan90
Copy link
Contributor

rjan90 commented Apr 30, 2024

@rvagg Do you have a status update on this. Do you think this is worth pursuing?

@rvagg
Copy link
Member

rvagg commented May 1, 2024

@rjan90 yes, but as outlined above in #11770 (comment), which is a change to the API - "give me events since this tipset" is what we really want to provide, but it's not a small change so will need to sit in the priority queue somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants