Implement SendDataColumnSidecarsByRangeRequest and SendDataColumnSidecarsByRootRequest.
#15430
Conversation
nalepae
force-pushed
the
peerdas-send-data-columns-requests
branch
from
caf659b to
f86d723
Compare
nalepae
changed the title
SendDataColumnSidecarsByRangeRequest.SendDataColumnSidecarsByRangeRequest and `SendDataColumnSidecarsByRootRequest.
…idecarsByRootRequest`.
nalepae
force-pushed
the
peerdas-send-data-columns-requests
branch
from
f86d723 to
4d97894
Compare
nalepae
changed the title
SendDataColumnSidecarsByRangeRequest and `SendDataColumnSidecarsByRootRequest.SendDataColumnSidecarsByRangeRequest and SendDataColumnSidecarsByRootRequest.
| func isSidecarIndexRootRequested(request *p2ptypes.DataColumnsByRootIdentifiers) DataColumnResponseValidation { | ||
| columnsIndexFromRoot := make(map[[fieldparams.RootLength]byte]map[uint64]bool) | ||
|
|
||
| for _, sidecar := range *request { |
There was a problem hiding this comment.
is doing range *request safe? first time seeing someone do this in our codebase, surprised it's a pointer to an array
| // Read the data column sidecars from the stream. | ||
| roDataColumns := make([]blocks.RODataColumn, 0, totalCount) | ||
| for range totalCount { | ||
| roDataColumn, err := readChunkedDataColumnSidecar( |
There was a problem hiding this comment.
does readChunkedDataColumnSidecar handle context.Done()? if not it might be a good idea to handle it in stalled cases
There was a problem hiding this comment.
It does not. But yes it is a good idea not to loop if the context is canceled.
Fixed in 00cf5f9.
|
|
||
| // Check if we do not request too many sidecars. | ||
| columnsCount := uint64(len(request.Columns)) | ||
| totalCount := request.Count * columnsCount |
There was a problem hiding this comment.
I wonder if it ever matters to do overflow checks on these numbers from request
There was a problem hiding this comment.
I don't think so. If it starts to overflow, we'll have other bigger issues...
| // isSidecarSlotWithinBounds verifies that the slot of the data column sidecar is within the bounds of the request. | ||
| func isSidecarSlotWithinBounds(request *ethpb.DataColumnSidecarsByRangeRequest) DataColumnResponseValidation { | ||
| // endSlot is exclusive (while request.StartSlot is inclusive). | ||
| endSlot := request.StartSlot + primitives.Slot(request.Count) |
There was a problem hiding this comment.
another spot that could overflow? ( not sure if it matters, don't think we check for this elsewhere)
| func isSidecarIndexRequested(request *ethpb.DataColumnSidecarsByRangeRequest) DataColumnResponseValidation { | ||
| requestedIndices := make(map[uint64]bool) | ||
| for _, col := range request.Columns { | ||
| requestedIndices[col] = true |
There was a problem hiding this comment.
what happens if there are duplicate columns requested?
There was a problem hiding this comment.
I think they pass silently
There was a problem hiding this comment.
Yeah that's not an issue. Duplicates are ignored.
| } | ||
|
|
||
| for _, column := range sidecar.Columns { | ||
| columnsIndexFromRoot[blockRoot][column] = true |
There was a problem hiding this comment.
I think duplicates silently pass again here
There was a problem hiding this comment.
Yes, duplicates are ignored.
If the peer asks twice for the same sidecar, we will only respond once.
| } | ||
|
|
||
| // All requested sidecars were delivered by the peer. Expecting EOF. | ||
| if _, err := readChunkedDataColumnSidecar(stream, p2pApi, ctxMap); !errors.Is(err, io.EOF) { |
There was a problem hiding this comment.
I think this is possibly brittle, if the peer leaves the stream open i think this will hang.
There was a problem hiding this comment.
if the peer is oversending what about just closing the stream when we think it's done?
There was a problem hiding this comment.
I think this is possibly brittle, if the peer leaves the stream open i think this will hang.
Actually not, it will timeout if nothing is received after TTFB_TIMEOUT (default: 5 seconds).
if the peer is oversending what about just closing the stream when we think it's done?
Doing so is against the specification:
The response MUST contain no more than count * NUMBER_OF_COLUMNS data column sidecars.
…idecarsByRootRequest`. (OffchainLabs#15430) * Implement `SendDataColumnSidecarsByRangeRequest` and `SendDataColumnSidecarsByRootRequest`. * Fix James's comment. * Fix James's comment. * Fix James' comment. * Fix marshaller.
…idecarsByRootRequest`. (OffchainLabs#15430) * Implement `SendDataColumnSidecarsByRangeRequest` and `SendDataColumnSidecarsByRootRequest`. * Fix James's comment. * Fix James's comment. * Fix James' comment. * Fix marshaller.
What type of PR is this?
Feature
What does this PR do? Why is it needed?
SendDataColumnSidecarsByRangeRequestSendDataColumnSidecarsByRootRequestOther notes for review
Depending on:
Acknowledgements