Allow bytes from relay to be of 0 length #1295
Conversation
Signed-off-by: litt3 <[email protected]>
| err = verification.CheckBlobLength(blob, blobCommitments.Length) | ||
| if err != nil { | ||
| pr.log.Warn("check blob length", "blobKey", blobKey.Hex(), "relayKey", relayKey, "error", err) | ||
| if uint(len(blob)) > blobCommitments.Length*encoding.BYTES_PER_SYMBOL { |
There was a problem hiding this comment.
this is the kind of code that I'm trying to avoid by forcing unit suffixes everywhere :
It's basically impossible for me to know whether this check is correct without an IDE, which I don't like that much.
There was a problem hiding this comment.
EDIT: I actually even checkout out the code on vscode and still didn't have enough context to verify this inequality. Can you add comments to BlobCommitments struct (
Line 27 in 9f4d8be
There was a problem hiding this comment.
Also is it normal that blob is an array of bytes? Is this something that your new type system fixes? Aren't blobs arrays of Fr.Elements now?
There was a problem hiding this comment.
Is this something that your new type system fixes?
Correct. Now that the type system PR has merged, I will be putting up a PR which updates these clients to use the new types.
There was a problem hiding this comment.
this is the kind of code that I'm trying to avoid by forcing unit suffixes everywhere
💯 agree. I'm on board with adopting this standard, so definitely point out if you see me forgetting and generating any new code without a proper suffix
Can you add comments to BlobCommitments struct
Sure, I will add some comments there
There was a problem hiding this comment.
comment added to BlobCommitments about blob length ea0c0538, to resolve the issue with units.
I didn't add comments to the other fields, since I'm not equipped to provide a nuanced description of how the elements relate to each other. This could definitely use some attention from someone able to write such a description
There was a problem hiding this comment.
Awesome thanks for the updates. For the BlobCommitment comments, you can look at the proto comments, they should prob just match: https://github.com/Layr-Labs/eigenda/blob/master/api/proto/common/common.proto#L13-L27
The state of comment drift across proto/backend/contract is one thing that I really had a way to solve. I think CUElang has some really interesting approach to this, which I want to look deeper into. But otherwise for now, perhaps we can just add a link to proto file from the backend struct to prevent comment drift?
There was a problem hiding this comment.
Yeah, I think it would be really good to have a way to keep comments consistent. Next time I touch this area of code, I'll add a link as suggested
Signed-off-by: litt3 <[email protected]>
Signed-off-by: litt3 <[email protected]>
| "blobKey", blobKey.Hex(), | ||
| "relayKey", relayKey, | ||
| "receivedLengthBytes", len(blob), | ||
| "claimedLengthBytes", blobCommitments.Length*encoding.BYTES_PER_SYMBOL) | ||
| continue |
There was a problem hiding this comment.
It's fine to skip over because the disperser returns the length which includes any "0" value elements at the end correct?
There was a problem hiding this comment.
Right. If a relay returns anything > blob length in the commitment, we know reflexively that the bytes cannot be correct
Why are these changes needed?
Checks