-
Notifications
You must be signed in to change notification settings - Fork 962
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
BlobSidecarsByRange rpc handler #12499
BlobSidecarsByRange rpc handler #12499
Conversation
beb95d1
to
a4678c7
Compare
7f09ac3
to
4e74b03
Compare
f66fa03
to
93249d5
Compare
} | ||
|
||
var err error | ||
rp.end, err = rp.start.SafeAdd((rp.size - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
underflow if rp.size
is 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this validation: e84e08a
146f15f
to
4c8d4ed
Compare
} | ||
totalWrites += writes | ||
// once we have written MAX_REQUEST_BLOB_SIDECARS, we're done serving the request | ||
if totalWrites >= params.BeaconNetworkConfig().MaxRequestBlobsSidecars { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I rewrote the bookkeeping code to stop streaming results as soon as we hit the limit: 8055521
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this limit apply for blobs too ? I noticed that blobs aren't to be served until a minimum threshold is reached, unlike blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
58bd57a
to
f8a1c0b
Compare
d34ce37
to
e532b40
Compare
} | ||
|
||
func blobBatchLimit() uint64 { | ||
return uint64(flags.Get().BlockBatchLimit / fieldparams.MaxBlobsPerBlock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. Shouldn't it be the maximum of the two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
8055521
to
d716dc8
Compare
b18a055
to
9df7f6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
0046681
to
99ff163
Compare
258c49d
to
107ae03
Compare
b87acf9
into
integrate-blobs-by-root
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
* 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>
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