BlobSidecarsByRange rpc handler by kasey · Pull Request #12499 · OffchainLabs/prysm

kasey · 2023-06-06T19:28:31Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR adds support for the deneb BlobSidecarsByRange RPC

terencechain · 2023-06-07T19:53:56Z

beacon-chain/sync/rpc_blob_sidecars_by_range.go

+	}
+
+	var err error
+	rp.end, err = rp.start.SafeAdd((rp.size - 1))


underflow if rp.size is 0?

added this validation: e84e08a

nisdas · 2023-06-09T02:47:13Z

beacon-chain/sync/rpc_blob_sidecars_by_range.go

+		}
+		totalWrites += writes
+		// once we have written MAX_REQUEST_BLOB_SIDECARS, we're done serving the request
+		if totalWrites >= params.BeaconNetworkConfig().MaxRequestBlobsSidecars {


I noticed this is checked after we have streamed the batch, would it be an issue if we streamed an excessive number of sidecars to the peer (beyond the limit).

Good catch. I rewrote the bookkeeping code to stop streaming results as soon as we hit the limit: 8055521

There was a compile error in the above commit because hadn't updated this branch after changing the signature of readChunkEncodedBlobs - added a validator for range requests here: b18a055

nisdas · 2023-06-09T02:50:10Z

beacon-chain/sync/rpc_blob_sidecars_by_range.go

+	maxRequest := params.BeaconNetworkConfig().MaxRequestBlocksDeneb
+	// Allow some wiggle room, up to double the MaxRequestBlocks past the current slot,
+	// to give nodes syncing close to the head of the chain some margin for error.
+	maxStart, err := current.SafeAdd(maxRequest * 2)


does this limit apply for blobs too ? I noticed that blobs aren't to be served until a minimum threshold is reached, unlike blocks.

Updating this discussion from our chat elsewhere - the reasoning here is that we should use the same start slot validation logic as BeaconBlocksByRange so that we don't penalize clients who try to sync blobs in parallel with blocks (and potentially overshoot current slot for both). Ultimately we won't serve any results when the start slot is > current, but we also won't treat a request like this as an error and penalize clients for aggressively seeking past this point, within the maxRequest * 2 margin of error.

prestonvanloon · 2023-06-12T23:02:03Z

beacon-chain/sync/rpc_blob_sidecars_by_range.go

+}
+
+func blobBatchLimit() uint64 {
+	return uint64(flags.Get().BlockBatchLimit / fieldparams.MaxBlobsPerBlock)


This doesn't seem right. Shouldn't it be the maximum of the two?

Ultimately this value controls how many blocks we'll process in a batch, ie this is rate limiting blocks/sec that we query and stream. In the context of BeaconBlocksByRange, we stream batch.size values (1 blocks for each item in the batch), but for BlobSidecarsByRange, we'll stream up to batch.size * MaxBlobsPerBlock values (blobs). We want both requests to serve roughly the same number of values per cycle, so we divide the block limit by the number of blobs per block to arrive at roughly the same number of values written to the wire per batch / same interval of time. I should add a comment to better explain what this is for.

nisdas

LGTM

* 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 June 6, 2023 19:28

kasey requested review from james-prysm, prestonvanloon and saolyn and removed request for a team, james-prysm, prestonvanloon and saolyn June 6, 2023 19:28

kasey force-pushed the integrate-blobs-by-range branch 2 times, most recently from beb95d1 to a4678c7 Compare June 7, 2023 00:11

kasey force-pushed the integrate-blobs-by-root branch from 7f09ac3 to 4e74b03 Compare June 7, 2023 18:03

kasey force-pushed the integrate-blobs-by-range branch 2 times, most recently from f66fa03 to 93249d5 Compare June 7, 2023 19:15

terencechain reviewed Jun 7, 2023

View reviewed changes

kasey force-pushed the integrate-blobs-by-range branch from 146f15f to 4c8d4ed Compare June 8, 2023 15:39

kasey changed the title ~~WIP BlobSidecarsByRange rpc handler~~ BlobSidecarsByRange rpc handler Jun 8, 2023

nisdas reviewed Jun 9, 2023

View reviewed changes

kasey force-pushed the integrate-blobs-by-root branch from 58bd57a to f8a1c0b Compare June 10, 2023 15:59

kasey force-pushed the integrate-blobs-by-range branch 2 times, most recently from d34ce37 to e532b40 Compare June 12, 2023 21:41

prestonvanloon reviewed Jun 12, 2023

View reviewed changes

kasey force-pushed the integrate-blobs-by-root branch from 8055521 to d716dc8 Compare June 12, 2023 23:59

kasey force-pushed the integrate-blobs-by-range branch from b18a055 to 9df7f6b Compare June 12, 2023 23:59

nisdas approved these changes Jun 13, 2023

View reviewed changes

kasey force-pushed the integrate-blobs-by-range branch 5 times, most recently from 0046681 to 99ff163 Compare June 14, 2023 22:06

prestonvanloon approved these changes Jun 15, 2023

View reviewed changes

kasey force-pushed the integrate-blobs-by-range branch from 258c49d to 107ae03 Compare June 15, 2023 19:30

kasey added the OK to merge label Jun 15, 2023

prylabs-bulldozer bot merged commit b87acf9 into integrate-blobs-by-root Jun 15, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BlobSidecarsByRange rpc handler#12499

BlobSidecarsByRange rpc handler#12499
prylabs-bulldozer[bot] merged 1 commit intointegrate-blobs-by-rootfrom
integrate-blobs-by-range

kasey commented Jun 6, 2023

Uh oh!

terencechain Jun 7, 2023

Uh oh!

kasey Jun 8, 2023

Uh oh!

nisdas Jun 9, 2023

Uh oh!

kasey Jun 12, 2023

Uh oh!

kasey Jun 12, 2023

Uh oh!

nisdas Jun 9, 2023

Uh oh!

kasey Jun 12, 2023

Uh oh!

prestonvanloon Jun 12, 2023

Uh oh!

kasey Jun 13, 2023

Uh oh!

nisdas left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kasey commented Jun 6, 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!

nisdas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants