Add rpc trigger for blob sidecar event #13411
Conversation
james-prysm
added
API
| @@ -315,7 +316,12 @@ func (vs *Server) broadcastAndReceiveBlobs(ctx context.Context, sidecars []*ethp | |||
| if err != nil { | |||
| return errors.Wrap(err, "ROBlob creation failed") | |||
| } | |||
| if err := vs.BlobReceiver.ReceiveBlob(ctx, blocks.NewVerifiedROBlob(readOnlySc)); err != nil { | |||
| verifiedBlob := blocks.NewVerifiedROBlob(readOnlySc) | |||
| vs.OperationNotifier.OperationFeed().Send(&feed.Event{ | |||
There was a problem hiding this comment.
should this be under the operations feed or should I create a new notifier for blobs or use the block notifier instead?
There was a problem hiding this comment.
operation feed makes sense...as it is for all objects that go inside of blocks
There was a problem hiding this comment.
The other place we fire this event is in blobSubscriber. blobSubscriber and this function (line 324) both call ReceiveBlob. ReceiveBlob is responsible for actually writing the blob to disk and pushing it to a possibly blocked ReceiveBlock call. In terms of timing, I think it makes sense to publish the blob on the event feed after it has been saved and to prioritize the da check over the event feed, so I would suggest moving the event firing to the end of ReceiveBlob which ensures the timing is the same in both cases.
There was a problem hiding this comment.
should I create a new notifier for blobs or use the block notifier instead
I need more context - why do you want to move it to a different notifier?
There was a problem hiding this comment.
re moving the event firing to ReceiveBlob, heard from James that this requires more extensive plumbing to get the notifier into the blockchain package, so I will approve this as-is.
james-prysm
changed the title
james-prysm
dismissed stale reviews from terencechain and rauljordan
via
f3bf0be
What type of PR is this?
Feature
What does this PR do? Why is it needed?
similar to notifying blocks, when blobs are broadcasted ( triggered from rpc) we should send an event.
Which issues(s) does this PR fix?
continuing off of #13315
ethereum/beacon-APIs#348
Other notes for review