BlobSidecarsByRoot by kasey · Pull Request #12420 · OffchainLabs/prysm

kasey · 2023-05-17T20:31:52Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This implements the BlobSidecarsByRoot p2p RPC handler according to deneb/p2p-interface.md.

terencechain · 2023-05-17T21:29:50Z

config/fieldparams/mainnet.go

 	SyncAggregateSyncCommitteeBytesLength = 64            // SyncAggregateSyncCommitteeBytesLength defines the length of sync committee bytes in a sync aggregate.
 	MaxWithdrawalsPerPayload              = 16            // MaxWithdrawalsPerPayloadLength defines the maximum number of withdrawals that can be included in a payload.
+	BlobSize                              = 131072        // defined to match blob.size in bazel ssz codegen
+	MaxBlobsPerBlock                      = 4             // MAX_BLOBS_PER_BLOCK


I originally had MaxBlobsPerBlock under beacon config. Looking at this, defining in fieldparam is a better place than config. It is a preset and preset is closer to fieldparams than beacon config

terencechain · 2023-05-17T21:30:55Z

config/params/mainnet_config.go

 	c.AltairForkVersion = make([]byte, fieldparams.VersionLength)
 	c.BellatrixForkVersion = make([]byte, fieldparams.VersionLength)
 	c.CapellaForkVersion = make([]byte, fieldparams.VersionLength)
+	c.DenebForkVersion = make([]byte, fieldparams.VersionLength)


Thanks for taking care all the configs!

TODO: I think if a user-supplied yaml config doesn't specify a deneb fork version, we could fail to load it, because the zero-value for the version could conflict. I need to test this.

prestonvanloon · 2023-05-22T20:04:33Z

beacon-chain/p2p/types/types.go

+	bufLen := len(buf)
+	maxLength := int(params.BeaconNetworkConfig().MaxRequestBlobsSidecars) * blobIdSize
+	if bufLen > maxLength {
+		return errors.Errorf("expected buffer with length of upto %d but received length %d", maxLength, bufLen)


Suggested change

return errors.Errorf("expected buffer with length of upto %d but received length %d", maxLength, bufLen)

return errors.Errorf("expected buffer with length of up to %d but received length %d", maxLength, bufLen)

Looks un-updated, Could you please check?

you are correct, fixed here 0a62357

prestonvanloon · 2023-05-23T12:59:50Z

beacon-chain/sync/blobs_test.go

+	code, _, err := ReadStatusCode(stream, encoding)
+	require.NoError(t, err)
+	require.Equal(t, r.code, code, "unexpected response code")
+	// require.Equal(t, r.message, msg, "unexpected error message")


Want to delete this?

Suggested change

// require.Equal(t, r.message, msg, "unexpected error message")

prestonvanloon · 2023-05-23T13:15:12Z

beacon-chain/sync/rpc_blob_sidecars_by_root.go

+	blobIdents := *ref
+	for i := range blobIdents {


Before you start work processing this list, should you validate that it is within the max bounds?

No more than MAX_REQUEST_BLOBS_SIDECARS * MAX_BLOBS_PER_BLOCK may be requested at a time.

definitely, thanks for pointing that out, I'm surprised there is no check at all for this, good catch.

Speaking of which, I am going to do some additional validation and processing on the request. The way this code is written, we'll call the DB lookup multiple times for each index for a given root. I'm going to add an intermediate type for this to loop over so that it naturally batches by root.

updated -- wound up sorting request elements to avoid call the DB lookup multiple times for each index for a given root.

prestonvanloon · 2023-05-23T13:17:32Z

beacon-chain/sync/rpc_blob_sidecars_by_root.go

+	blobIdents := *ref
+	for i := range blobIdents {


It might also be a good idea to add a ctx expired check at the start of the loop routine.

Suggested change

blobIdents := *ref

for i := range blobIdents {

blobIdents := *ref

for i := range blobIdents {

if err := ctx.Err(); err != nil {

return err

}

prestonvanloon · 2023-05-23T13:18:36Z

beacon-chain/sync/rpc_blob_sidecars_by_root.go

+		}
+		s.rateLimiter.add(stream, 1)
+	}
+	closeStream(stream, log)


Shouldn't this be a defer as soon as the stream is opened? Seems like any of the early terminations on error wouldnt close the stream.

Also, wouldn't the caller be responsible for closing the stream? Perhaps you could add some commentary or something to explain why this is here in the way that it is.

If you look at other RPC handlers, they all do the same thing. The reason is that the error handling functions independently call closeStream, so calling it in a defer would cause the stream to be closed twice. I want to refactor the dispatch code to take care of this after the handler completes and remove the close from the error handlers, since we should always close the stream in both error and successful paths.

I'm going to do some work to fix this for all RPCs in develop. In the meantime the context check adds another place where we need to explicitly call closeStream. I thought this was cleaner than my other idea, which was to defer with an enclosed bool to track whether an error method had been called. Once I get the other PR into develop and we rebase, I'll update this code to defer the stream close. Hopefully that doesn't block this PR.

prestonvanloon · 2023-05-23T13:25:25Z

beacon-chain/sync/rpc_blob_sidecars_by_root.go

+		if chunkErr := WriteBlobSidecarChunk(stream, s.cfg.chain, s.cfg.p2p.Encoding(), sc); chunkErr != nil {
+			log.WithError(chunkErr).Debug("Could not send a chunked response")
+			s.writeErrorResponseToStream(responseCodeServerError, types.ErrGeneric.Error(), stream)
+			tracing.AnnotateError(span, chunkErr)
+			return chunkErr
+		}
+		s.rateLimiter.add(stream, 1)


Want to increment this before you write the chunk so that it is incremented whether or not the response was successful?

Suggestion below moves s.rateLimiter.add(stream, 1) to the line before the if statement.

Suggested change

if chunkErr := WriteBlobSidecarChunk(stream, s.cfg.chain, s.cfg.p2p.Encoding(), sc); chunkErr != nil {

log.WithError(chunkErr).Debug("Could not send a chunked response")

s.writeErrorResponseToStream(responseCodeServerError, types.ErrGeneric.Error(), stream)

tracing.AnnotateError(span, chunkErr)

return chunkErr

}

s.rateLimiter.add(stream, 1)

s.rateLimiter.add(stream, 1)

if chunkErr := WriteBlobSidecarChunk(stream, s.cfg.chain, s.cfg.p2p.Encoding(), sc); chunkErr != nil {

log.WithError(chunkErr).Debug("Could not send a chunked response")

s.writeErrorResponseToStream(responseCodeServerError, types.ErrGeneric.Error(), stream)

tracing.AnnotateError(span, chunkErr)

return chunkErr

}

yeah that makes sense, actually I will move this to right after the DB lookup.

Yeap, was about to also add that suggestion too ^

beacon-chain/p2p/rpc_topic_mappings.go

nisdas · 2023-05-24T09:00:56Z

beacon-chain/sync/rate_limiter.go

 	blockCollectorV2 := leakybucket.NewCollector(allowedBlocksPerSecond, allowedBlocksBurst, blockBucketPeriod, false /* deleteEmptyBuckets */)

+	// for BlobSidecarsByRoot and BlobSidecarsByRange
+	blobCollector := leakybucket.NewCollector(allowedBlocksPerSecond, allowedBlocksBurst, blockBucketPeriod, false)


is it ok to assume 1-1 mapping for blob rate limiter params( same as for a block).

Good point, it's probably better for this to have its own set of params that can be tuned independently.

added these params: b2c8f09

I'm always a little on the fence about adding excessive CLI flags, but in order for them to be settable in the same way as blocks I guess this is the pattern. Also note that I picked a lower value for blob batch size, corresponding to the larger size of blob objects. I probably went a little too low but we can tweak the flag default, let me know if you have a suggestion.

nisdas · 2023-05-24T09:27:21Z

beacon-chain/sync/rpc_send_request.go

+		if !found {
+			return nil, errors.Wrapf(ErrBlobUnmarshal, fmt.Sprintf("unrecognized fork digest %#x", ctxb))
+		}
+		if v != version.Deneb {


this would break for any forks after deneb

This was intentional, because this function returns a deneb-specific type. I thought it was premature to add an interface type for blobs, so it seemed natural to make this break in an obvious way for future forks, so that we'll know we need to update this code to handle multiple forks.

nisdas · 2023-05-24T09:29:35Z

config/params/network_config.go

 	BlobsidecarSubnetCount           uint64           `yaml:"BLOBSIDECAR_SUBNET_COUNT"`              // BlobsidecarSubnetCount is the number of blobsidecar subnets used in the gossipsub protocol.
 	AttestationPropagationSlotRange  primitives.Slot  `yaml:"ATTESTATION_PROPAGATION_SLOT_RANGE"`    // AttestationPropagationSlotRange is the maximum number of slots during which an attestation can be propagated.
 	MaxRequestBlocks                 uint64           `yaml:"MAX_REQUEST_BLOCKS"`                    // MaxRequestBlocks is the maximum number of blocks in a single request.
+	MaxRequestBlocksDeneb            uint64           `yaml:"MAX_REQUEST_BLOCKS_DENEB"`              // MaxRequestBlocksDeneb is the maximum number of blocks in a single request after the Deneb fork.


why do we need a new param after deneb

It's a configuration modification in deneb: https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/p2p-interface.md#configuration (decreasing from 1024 down to 128). As to why this changed in deneb, I suppose it's possible this is leftover from block+blob coupling - maybe we should check on that, propose getting rid of this config variable and set it back to 1024?

nisdas · 2023-05-24T09:31:47Z

beacon-chain/sync/rpc_send_request.go

+		if err := decode(stream, sc); err != nil {
+			return nil, errors.Wrap(err, "failed to decode the protobuf-encoded BlobSidecar message from RPC chunk stream")
+		}
+		sidecars = append(sidecars, sc)


there seems to be no validation on the blob sidecars, how do we verify that the blobs being provided are the ones we want ?

The response may not include all requested values:

Clients MUST respond with at least one sidecar, if they have it. Clients MAY limit the number of blocks and sidecars in the response.

Since the caller of this function may need to formulate a new request to get the missing values, I thought it was better to leave that kind of logic to the user of the function. What do you think?

I think that is fine, but is there any correctness check that we can do to verify that the sidecars provided by the peer are at least a subset of what we requested ? Thinking about how to protect us in the event of malicious peer just returning us nonsense

That makes sense - I added this check, please let me know what you think: f463646

nisdas · 2023-05-24T09:46:43Z

beacon-chain/sync/rpc_blob_sidecars_by_root.go

+		if chunkErr := WriteBlobSidecarChunk(stream, s.cfg.chain, s.cfg.p2p.Encoding(), sc); chunkErr != nil {
+			log.WithError(chunkErr).Debug("Could not send a chunked response")
+			s.writeErrorResponseToStream(responseCodeServerError, types.ErrGeneric.Error(), stream)
+			tracing.AnnotateError(span, chunkErr)
+			return chunkErr
+		}
+		s.rateLimiter.add(stream, 1)


Yeap, was about to also add that suggestion too ^

beacon-chain/p2p/types/types.go

terencechain · 2023-06-06T22:21:03Z

beacon-chain/p2p/types/types.go

+func init() {
+	sizer := &eth.BlobIdentifier{}
+	blobIdSize = sizer.SizeSSZ()
+}


should we put init() at the start of the file?

This is consistent with how I've use init elsewhere, always at the end of the file. It's not a strongly held opinion, but keeping it at the end would be consistent. My reasoning is that it's usually not the most interesting thing in the file, it only runs once to do basic initialization of values that tend to be declared at the top, and it is evaluated after everything else in the file (for instance, any other package scoped var declarations).

terencechain · 2023-06-06T22:22:37Z

beacon-chain/sync/blobs_test.go

+		Timestamp:     0,
+		ExtraData:     make([]byte, 0),
+		BaseFeePerGas: bytesutil.PadTo([]byte("baseFeePerGas"), fieldparams.RootLength),
+		ExcessDataGas: bytesutil.PadTo([]byte("excessDataGas"), fieldparams.RootLength),


excess data gas is uint64 now and there's a new field data gas used

terencechain · 2023-06-06T22:23:01Z

beacon-chain/sync/blobs_test.go

+	cleanup := func() {
+		require.NoError(t, undo())
+	}
+	maxBlobs := int(params.BeaconConfig().MaxBlobsPerBlock)


MaxBlobsPerBlock moved to field params

cmd/beacon-chain/flags/base.go

Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>

* BlobSidecarsByRoot RPC handler * BlobSidecarsByRange rpc handler (#12499) Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com> --------- Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>

kasey requested a review from a team as a code owner May 17, 2023 20:31

kasey requested review from potuz, rkapka and terencechain and removed request for a team May 17, 2023 20:31

kasey force-pushed the integrate-blobs-by-root branch from efb9391 to 0700b8a Compare May 17, 2023 20:42

terencechain reviewed May 17, 2023

View reviewed changes

terencechain force-pushed the deneb-integration branch from 6e9f6fd to ce4a937 Compare May 18, 2023 17:09

terencechain requested a review from prestonvanloon as a code owner May 18, 2023 17:09

kasey force-pushed the deneb-integration branch from ce4a937 to ca11e4a Compare May 19, 2023 17:26

kasey force-pushed the integrate-blobs-by-root branch from 0700b8a to c5677b2 Compare May 19, 2023 17:33

kasey force-pushed the deneb-integration branch from ca11e4a to 220beca Compare May 22, 2023 16:43

kasey force-pushed the integrate-blobs-by-root branch from c5677b2 to c0f950b Compare May 22, 2023 17:19

kasey changed the title ~~WIP BlobSidecarsByRoot~~ BlobSidecarsByRoot May 22, 2023

prestonvanloon reviewed May 23, 2023

View reviewed changes

terencechain force-pushed the deneb-integration branch from 0166a25 to 35f2e33 Compare May 23, 2023 17:51

kasey force-pushed the integrate-blobs-by-root branch from c0f950b to 391355f Compare May 24, 2023 02:38

nisdas reviewed May 24, 2023

View reviewed changes

terencechain force-pushed the deneb-integration branch 2 times, most recently from f42b326 to 0c7f3cc Compare May 31, 2023 18:39

kasey force-pushed the integrate-blobs-by-root branch from 85c2cc7 to 2c9d58a Compare June 5, 2023 18:51

terencechain reviewed Jun 6, 2023

View reviewed changes

beacon-chain/p2p/types/types.go Outdated Show resolved Hide resolved

terencechain reviewed Jun 6, 2023

View reviewed changes

beacon-chain/p2p/types/types.go Outdated Show resolved Hide resolved

terencechain reviewed Jun 6, 2023

View reviewed changes

beacon-chain/p2p/types/types.go Outdated Show resolved Hide resolved

terencechain reviewed Jun 6, 2023

View reviewed changes

cmd/beacon-chain/flags/base.go Show resolved Hide resolved

terencechain reviewed Jun 6, 2023

View reviewed changes

cmd/beacon-chain/flags/base.go Outdated Show resolved Hide resolved

kasey force-pushed the integrate-blobs-by-root branch from 8e48d48 to 3b9af97 Compare June 15, 2023 19:29

BlobSidecarsByRange rpc handler (#12499)

b87acf9

Co-authored-by: Kasey Kirkham <kasey@users.noreply.github.com>

kasey added the OK to merge label Jun 15, 2023

prylabs-bulldozer bot merged commit 4f7db3d into deneb-integration Jun 15, 2023

prylabs-bulldozer bot deleted the integrate-blobs-by-root branch June 15, 2023 19:38

	return errors.Errorf("expected buffer with length of upto %d but received length %d", maxLength, bufLen)
	return errors.Errorf("expected buffer with length of up to %d but received length %d", maxLength, bufLen)

Conversation

kasey commented May 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment