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.

Sign up for GitHub

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

Jump to bottom

Add rpc trigger for blob sidecar event #13411

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Add rpc trigger for blob sidecar event #13411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a29a7f7
adding missed rpc trigger for blob sidecar event
james-prysm Jan 3, 2024
8df12b0
fixing unit tests
james-prysm Jan 3, 2024
576af36
Merge branch 'develop' into rpc-blob-sidecar-event
james-prysm Jan 3, 2024
f3bf0be
moving event feed to after receive blob call to prioritize db
james-prysm Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion beacon-chain/rpc/prysm/v1alpha1/validator/proposer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/prysmaticlabs/prysm/v4/beacon-chain/cache"
"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/feed"
blockfeed "github.com/prysmaticlabs/prysm/v4/beacon-chain/core/feed/block"
"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/feed/operation"
"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/helpers"
"github.com/prysmaticlabs/prysm/v4/beacon-chain/core/transition"
"github.com/prysmaticlabs/prysm/v4/beacon-chain/db/kv"
Expand Down Expand Up @@ -315,9 +316,14 @@ 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)
if err := vs.BlobReceiver.ReceiveBlob(ctx, verifiedBlob); err != nil {
return errors.Wrap(err, "receive blob failed")
}
vs.OperationNotifier.OperationFeed().Send(&feed.Event{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be under the operations feed or should I create a new notifier for blobs or use the block notifier instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

operation feed makes sense...as it is for all objects that go inside of blocks

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kasey kasey Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Type: operation.BlobSidecarReceived,
Data: &operation.BlobSidecarReceivedData{Blob: &verifiedBlob},
})
}
return nil
}
Expand Down
5 changes: 3 additions & 2 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,8 +799,9 @@ func TestProposer_ProposeBlock_OK(t *testing.T) {
P2P: mockp2p.NewTestP2P(t),
BlockBuilder: &builderTest.MockBuilderService{HasConfigured: tt.useBuilder, PayloadCapella: emptyPayloadCapella(), PayloadDeneb: emptyPayloadDeneb(),
BlobBundle: &enginev1.BlobsBundle{KzgCommitments: [][]byte{bytesutil.PadTo([]byte{0x01}, 48)}, Proofs: [][]byte{{0x02}}, Blobs: [][]byte{{0x03}}}},
BeaconDB: db,
BlobReceiver: c,
BeaconDB: db,
BlobReceiver: c,
OperationNotifier: c.OperationNotifier(),
}
blockToPropose := tt.block(bsRoot)
res, err := proposerServer.ProposeBeaconBlock(context.Background(), blockToPropose)
Expand Down
Loading