-
Notifications
You must be signed in to change notification settings - Fork 254
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
Support notify first shred received in geyser #3030
Support notify first shred received in geyser #3030
Conversation
Thank you! Is it also possible to add |
Yes. Will be done in follow-on PRs to cut down one PR size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Left a few comments
@@ -319,6 +319,9 @@ pub enum SlotStatus { | |||
|
|||
/// The highest slot that has been voted on by supermajority of the cluster, ie. is confirmed. | |||
Confirmed, | |||
|
|||
/// First Shred Received |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the comments for the other SlotStatus enums wrong? Almost seems like they were copy pasted from confirmation levels.. We send theses notifications out each time a slot transitions to the new state, right (assuming it makes it to that state)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- they were largely copied. Which one is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the part explaining this is "The highest slot" makes more sense in the context of confirmation levels.
I would probably rewrite them like this:
/// First shred has been received for this slot.
FirstShredReceived,
/// This slot has been processed.
Processed,
/// This slot has been optimistically confirmed by a supermajority of the cluster.
Confirmed,
/// This slot has been confirmed and reached max vote lockout on the canonical fork.
Rooted,
I acknowledge this is sort of out of scope for this PR - I just noticed it because your comment is written more like how I would expect for a slot status notification but doesn't quite match the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is the plan to build out more of the supporting functions in a follow-up PR? I'm thinking things like these:
https://github.com/anza-xyz/agave/blob/master/geyser-plugin-manager/src/slot_status_notifier.rs#L28-L40
https://github.com/anza-xyz/agave/blob/master/geyser-plugin-interface/src/geyser_plugin_interface.rs#L324-L332
3e4f03d
to
11a1fba
Compare
* support notify first shred received in geyser
Problem
This is the first part to support more slot status notification type in geyser. FirstShredReceived.
Summary of Changes
Link SlotStatusNotifier in retransmit stage on FirstShredReceived.
Tests: tested change with Postgres geyser plugin and confirming the FirstShredReceived being received.
Fixes #
#2957