Check blob key from disperser against actual key #1109
Conversation
Signed-off-by: litt3 <[email protected]>
There was a problem hiding this comment.
Changes LGTM, but the DisperseBlob function is getting big and unweildy. Might be good to refactor at some point, create some functions to abstract some of the stuff out. Would need to think through best way to do this.
Your code here could clearly fit in a function though, for eg.
@samlaf Good point. Split the blob key check into a separate function 617cbd5b |
Signed-off-by: litt3 <[email protected]>
| func verifyReceivedBlobKey( | ||
| // the blob header which was constructed locally and sent to the disperser | ||
| blobHeader *corev2.BlobHeader, | ||
| // the reply received back from the disperser | ||
| disperserReply *disperser_rpc.DisperseBlobReply, | ||
| ) error { |
There was a problem hiding this comment.
nit, would a simple unit test be possible? i.e. check both a valid and invalid header
Signed-off-by: litt3 <[email protected]>
| // the blob header which was constructed locally and sent to the disperser | ||
| blobHeader *corev2.BlobHeader, | ||
| // the reply received back from the disperser | ||
| disperserReply *disperser_rpc.DisperseBlobReply, |
There was a problem hiding this comment.
why don't we just take the received blob key instead of full reply?
There was a problem hiding this comment.
The idea was that we don't immediately have either blob key (computed or received) in the correct form: they need to be constructed and error checked. Moving the logic to the helper method was intended to keep DisperseBlob as clean as possible
I don't feel super strongly about this, I can remove the helper method if you would like
| @@ -213,9 +213,47 @@ func (c *disperserClient) DisperseBlob( | |||
| return nil, [32]byte{}, err | |||
| } | |||
|
|
|||
| if verifyReceivedBlobKey(blobHeader, reply) != nil { | |||
| return nil, [32]byte{}, fmt.Errorf("verify received blob key: %w", err) | |||
There was a problem hiding this comment.
nit: "failed to verify that the reply contains locally computed blob key" or something like that..?
There was a problem hiding this comment.
The helper method does contains more specific details about what specifically failed, e.g. "blob key returned by disperser (%v) doesn't match blob which was dispersed (%v)"
(Also, this message is attempting to follow this standard that @samlaf has been rallying support behind. It prescribes omitting the "failed to..." prefix)
Happy to still make an update here if you'd like, just pointing out the rationale of the current message
Why are these changes needed?
DisperseBlobmatches what was sent #1100Checks