Conversation
05f41a8 to
f6bf9f1
Compare
renaynay
left a comment
There was a problem hiding this comment.
Some questions:
- what is the point of
GetBlobsto be implemented over blob over shwap -- why wouldn't it just route to namespace data over shwap? Is this just an optimisation not to send padding if it exists? If so, then what's the point of shares by namespace over shwap (besides namespace non-inclusion)? - similarly,
GetBlobs()feels to me like it should be a message at shwap protocol level for fetching multiple blobs by commitment rather than justall-- right now, we can only fetch 1 blob by commitment over shwap or fetching ALL blobs in that namespace. Is there a case that we would need that? fetching multiple blobs by commitment from the same namespace? And not to bikeshed, but similarly, would there be a need to fetch blobs from different namespaces but at the sameheightsuch that it would be more efficient to batch it in one request? - Can we DRY between Blob()/Blobs() methods between rsmt2d.go and ods.go
- Why do we need two methods on accessor? Blob + Blobs? Can we not just have 1 (Blobs) and pass in 1 or multiple there? Right now, this is probably b/c we have the binary 1 blob or ALL blobs.
- feels like missing lots of unit tests
| ctx, span := tracer.Start( | ||
| ctx, | ||
| "cascade/get-blob", | ||
| trace.WithAttributes( |
share/eds/rsmt2d.go
Outdated
| return shwap.BlobFromShares(extendedRowShares, namespace, commitment, odsSize) | ||
| } | ||
|
|
||
| func (eds *Rsmt2D) Blobs(ctx context.Context, namespace libshare.Namespace) ([]*shwap.Blob, error) { |
There was a problem hiding this comment.
where are unit tests for these methods
| commitment []byte, | ||
| ) (*shwap.Blob, error) { | ||
| var err error | ||
| ctx, span := tracer.Start(ctx, "shrex/get-blob", trace.WithAttributes( |
There was a problem hiding this comment.
we need to pull span from the trace that's passed through cascade getter rather than starting new one
There was a problem hiding this comment.
This is the problem of all methods in shrex. Probably, I missed this part during shrex refactoring. I'll prepare a PR to fix all at once.
| ns libshare.Namespace, | ||
| ) ([]*shwap.Blob, error) { | ||
| var err error | ||
| ctx, span := tracer.Start(ctx, "shrex/get-blobs", trace.WithAttributes( |
| return errors.New("nil response") | ||
| } | ||
| for _, blob := range response { | ||
| err = blob.VerifyInclusion(header.DAH.RowRoots) |
There was a problem hiding this comment.
why are we only calling VerifyInclusion here instead of top-level Verify?
There was a problem hiding this comment.
We don't know the commitments during GetBlobs. So we are interested only in inclusion.
share/shwap/blob.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func BlobsFromShares( |
There was a problem hiding this comment.
Was all of this code moved from the blob pkg down here?
|
|
||
| // BlobIDSize defines the total size of a BlobID in bytes, combining the | ||
| // size of a EdsID, the size of a Namespace and the commitment size | ||
| const BlobIDSize = EdsIDSize + libshare.NamespaceSize + commitmentSize |
There was a problem hiding this comment.
is there any test coverage for this file?
| func (blbID *BlobID) ResponseReader(ctx context.Context, acc Accessor) (io.Reader, error) { | ||
| buf := &bytes.Buffer{} | ||
|
|
||
| if !bytes.Equal(blbID.Commitment, emptyCommitment) { |
There was a problem hiding this comment.
Switch case on here to make more readable?
|
|
||
| // Make a copy of the outer slice to avoid modifying the caller's slice | ||
| // when we reassign row elements below. | ||
| shares := make([][]libshare.Share, len(extendedRowShares)) |
There was a problem hiding this comment.
can we extract into separate PR as this is a big feature PR and would rather not bundle other stuff into
There was a problem hiding this comment.
This change is necessary and imo should be a part of this PR. Previously, a single range was requested which allows us to modify the outer slice. In case of blob's retrieval it will lead to an error with indexing, out of bounds errors etc.
store/file/ods.go
Outdated
| return shwap.RangeNamespaceDataFromShares(shares, fromCoords, toCoords) | ||
| } | ||
|
|
||
| func (o *ODS) Blob(ctx context.Context, namespace libshare.Namespace, commitment []byte) (*shwap.Blob, error) { |
There was a problem hiding this comment.
already working on them
The point is to have a proof, associated with blob and not for the whole namespace in the row.
You can request multiple blobs by commitment from different peers(as GetSamples) and make requests parallel. We can add this if it will be requested.
We are not requesting samples as batches from the security standpoint. Maybe we should follow this approach with blobs as well?
Yes
Right. Thank you.
Already working on them. |
92b984c to
3c825d6
Compare
| if err != nil { | ||
| if strings.Contains(err.Error(), blob.ErrBlobNotFound.Error()) { | ||
| if strings.Contains(err.Error(), shwap.ErrBlobNotFound.Error()) { | ||
| return nil, nil //nolint:nilnil | ||
| } |
There was a problem hiding this comment.
hmm, why don't we return non-inclusion proof?
There was a problem hiding this comment.
We havent returned it previously
| // GetBlobs retrieves all blobs belonging to the given namespace from the | ||
| // Extended Data Square (EDS) referenced by the given header. | ||
| GetBlobs(_ context.Context, _ *header.ExtendedHeader, _ libshare.Namespace) ([]*Blob, error) |
There was a problem hiding this comment.
Why you added this method? It seems to duplicate blobs parsing from blob service but leaking into reader level for some reason.
|
|
||
| // Blobs retrieves blobs belonging to the given namespace from the data square. | ||
| // When commitments are provided, only blobs matching those commitments are returned. | ||
| // When no commitments are provided, all blobs in the namespace are returned. | ||
| Blobs(context.Context, libshare.Namespace, ...[]byte) ([]*shwap.Blob, error) |
There was a problem hiding this comment.
It is a bit confusing why some APIs and interfaces are requesting single blobs while others are requesting multiple. We used to have ability to request only one blob by commitment. Are there some fundamentl changes required to make this PR? Imo ability to request multiple blobs by commitment at the same time is out of scope of this change and should be discussed / implemented separetly.
There was a problem hiding this comment.
Top API level contains 2 independent methods(GetBlob/GetBlobs), while accessor has only one, since the logic is pretty the same and @renaynay suggested to have only one method.
| func (eds *Rsmt2D) extendedRowShares(ctx context.Context) ([][]libshare.Share, int, error) { | ||
| size, err := eds.Size(ctx) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| odsSize := size / 2 | ||
| shares := make([][]libshare.Share, odsSize) | ||
| for index := 0; index < odsSize; index++ { | ||
| rawShare := eds.Row(uint(index)) | ||
| sh, err := libshare.FromBytes(rawShare) | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
| shares[index] = sh | ||
| } | ||
| return shares, odsSize, nil | ||
| } |
There was a problem hiding this comment.
It is duplication. You can get extended row from just Row method.
The PR adds a support of Blob type request and response for the shrex/protocol. Please note that the PR is breaking as it breaks the proof verification logic: instead of proving the whole namespace within the Row, blob Proof now tied to the specific blob.