Batch call for retrieving blobs from RPC #8952
Conversation
StefanBratanov
force-pushed
the
batch_call_blobs_rpc
branch
2 times, most recently
from
6d1e538 to
68fa5c2
Compare
StefanBratanov
force-pushed
the
batch_call_blobs_rpc
branch
from
6006719 to
96bf28a
Compare
| import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar; | ||
| import tech.pegasys.teku.spec.datastructures.networking.libp2p.rpc.BlobIdentifier; | ||
|
|
||
| public class FetchBlobSidecarTask extends AbstractFetchTask<BlobIdentifier, BlobSidecar> { | ||
| public class FetchBlobSidecarsTask extends AbstractFetchTask<Bytes32, List<BlobSidecar>> { |
There was a problem hiding this comment.
I have doubts here.
In principle K type should uniquely identify a task, so I think we should have a record(blockroot, List<BlobIdentifier>).
| if (allTasks.putIfAbsent(blobIdentifier, task) != null) { | ||
| final FetchBlobSidecarsTask task = | ||
| fetchTaskFactory.createFetchBlobSidecarsTask(blockRoot, requiredBlobIdentifiers); | ||
| if (allTasks.putIfAbsent(blockRoot, task) != null) { |
There was a problem hiding this comment.
so here we can have problems if we change our mind and we initiate a request with a different List<blobIdentifier> for the same block root.
There was a problem hiding this comment.
we could argue that the number of blobs identifier can only decrease with time, so we don't care if we take the initial request and cancel the new one, but:
- introduces an assumption that needs to be handled here, and it is risky (if we don't check we may cancel the task containing what we really need)
- anyway this approach breaks the design of the abstract class IMO
so if we move to the theoretical correct key we could and up requesting the same blob(s) in two different tasks, but at least we know that we will have them all executed despite the order of arrival
There was a problem hiding this comment.
so at the end:
option 1:
- change the key and allow multiple request for same blockRoot (potential duplication of blobs download)
- cancel method still based on blockRoot (looping on allTasks)
option 2:
- leave the key as blockRoot, adding some checks when adding a task for a blockRoot already present (logging an error if this is not supposed to happen, or some logic that compares the identifier list, cancel one and leave the other)
- cancel method remains as is
StefanBratanov
force-pushed
the
batch_call_blobs_rpc
branch
from
542d0a9 to
cc702fb
Compare
|
Had an internal discussion and decided to potentially revisit this idea in the future |
PR Description
When we need to retrieve blobs from RPC when following the head, we do one request per a sidecar. This PR changes this functionality to do one batch call. It will save bandwith (when used) and could help with rate limiting. Ideally, this should be merged after #8927
Fixed Issue(s)
fixes #8928
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog