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

Add blob_sidecar event to SSE #4790

Merged
merged 7 commits into from
Oct 12, 2023
Merged

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Sep 28, 2023

Issue Addressed

Partially address #4722 (as the issue also covers potential block event changes in Deneb, which aren't covered in this PR)

Proposed Changes

  • Introduce a BlobSidecar SSE event, and register the event when a the beacon node has received a BlobSidecar from either P2P or API)
    • For blobs received via gossip or beacon API (publish block), event are emitted for each blob that passes gossip validations.
    • For blobs received via a RPC request, events are emitted once they're received and sent over to the BeaconChain for processing.
  • An unrelated fix (could move it in a separate PR): return a success response code (202) if validation level is gossip and blob validation fails after the block has successfully broadcasted to the network (this is consistent with block validation behaviour).

@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress deneb labels Sep 28, 2023
@jimmygchen jimmygchen self-assigned this Sep 28, 2023
@jimmygchen
Copy link
Member Author

jimmygchen commented Sep 29, 2023

Had a bit of discussion with @pawanjay176 on this today, and I think I might park this for a bit, as there are some ongoing discussions on when to emit the block events, and may potentially impact blob events (on whether to emit it after gossip or after full validation).

If we're doing it after gossip, Pawan suggested

we can then do it in BeaconChain::process_gossip_blob for blobs received over gossip

This function is new in @realbigsean 's PR and hasn't landed on Deneb branch yet, so seems to make sense to wait for a bit.

On the testing side it doesn't seem too complicated, we could potentially pass a GossipVerifiedBlob to process_gossip_blob and assert that subscribers of ServerSentEventHandler receives an event.

@jimmygchen
Copy link
Member Author

I've merged latest deneb-free-blobs into this branch (given #4732 was merged), and updated the BlobSidecar event to be spec compliant.

@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed blocked work-in-progress PR is a work-in-progress labels Oct 10, 2023
Copy link
Member

@ethDreamer ethDreamer left a comment

Choose a reason for hiding this comment

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

LGTM!

.unwrap();

let sidecar_event = blob_event_receiver.try_recv().unwrap();
assert!(matches!(sidecar_event, EventKind::BlobSidecar(..)));
Copy link
Member

Choose a reason for hiding this comment

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

Nice work! You don't have to do this but since you seeded the RNG with a known value, the blob generated should be the same each time. You could check that the output matches the static SseBlobSidecar here so that this test breaks if something goes wrong in the code we use to generate it. Just an idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! Addressed in 39cc2fe. Thanks 🙏

Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Think we just need to add quotes on the JSON representation of a blob index. But nice tests! And also nice bonus fix on the blob gossip without import.

common/eth2/src/types.rs Show resolved Hide resolved
@realbigsean realbigsean added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 11, 2023
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 11, 2023
@realbigsean realbigsean merged commit 38e7172 into sigp:deneb-free-blobs Oct 12, 2023
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants