-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check blob key from disperser against actual key #1109
base: master
Are you sure you want to change the base?
Changes from all commits
e3302ce
8fb46b8
617cbd5
cad04c2
78c58a1
f15343a
11817be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
} | ||
|
||
return &blobStatus, corev2.BlobKey(reply.GetBlobKey()), nil | ||
} | ||
|
||
// verifyReceivedBlobKey computes the BlobKey from the BlobHeader which was sent to the disperser, and compares it with | ||
// the BlobKey which was returned by the disperser in the DisperseBlobReply | ||
// | ||
// A successful verification guarantees that the disperser didn't make any modifications to the BlobHeader that it | ||
// received from this client. | ||
// | ||
// This function returns nil if the verification succeeds, and otherwise returns an error describing the failure | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why don't we just take the received blob key instead of full reply? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I don't feel super strongly about this, I can remove the helper method if you would like |
||
) error { | ||
Comment on lines
+230
to
+235
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, would a simple unit test be possible? i.e. check both a valid and invalid header There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
actualBlobKey, err := blobHeader.BlobKey() | ||
if err != nil { | ||
// this shouldn't be possible, since the blob key has already been used when signing dispersal | ||
return fmt.Errorf("computing blob key: %w", err) | ||
} | ||
|
||
blobKeyFromDisperser, err := corev2.BytesToBlobKey(disperserReply.GetBlobKey()) | ||
if err != nil { | ||
return fmt.Errorf("converting returned bytes to blob key: %w", err) | ||
} | ||
|
||
if actualBlobKey != blobKeyFromDisperser { | ||
return fmt.Errorf( | ||
"blob key returned by disperser (%v) doesn't match blob which was dispersed (%v)", | ||
blobKeyFromDisperser, actualBlobKey) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// GetBlobStatus returns the status of a blob with the given blob key. | ||
func (c *disperserClient) GetBlobStatus(ctx context.Context, blobKey corev2.BlobKey) (*disperser_rpc.BlobStatusReply, error) { | ||
err := c.initOnceGrpcConnection() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package clients | ||
|
||
import ( | ||
"math/big" | ||
"testing" | ||
|
||
v2 "github.com/Layr-Labs/eigenda/api/grpc/disperser/v2" | ||
"github.com/Layr-Labs/eigenda/core" | ||
corev2 "github.com/Layr-Labs/eigenda/core/v2" | ||
"github.com/Layr-Labs/eigenda/encoding" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestVerifyReceivedBlobKey(t *testing.T) { | ||
blobCommitments := encoding.BlobCommitments{ | ||
Commitment: &encoding.G1Commitment{}, | ||
LengthCommitment: &encoding.G2Commitment{}, | ||
LengthProof: &encoding.LengthProof{}, | ||
Length: 4, | ||
} | ||
|
||
quorumNumbers := make([]core.QuorumID, 1) | ||
quorumNumbers[0] = 8 | ||
|
||
paymentMetadata := core.PaymentMetadata{ | ||
AccountID: "asdf", | ||
ReservationPeriod: 5, | ||
CumulativePayment: big.NewInt(6), | ||
Salt: 9, | ||
} | ||
|
||
blobHeader := &corev2.BlobHeader{ | ||
BlobVersion: 0, | ||
BlobCommitments: blobCommitments, | ||
QuorumNumbers: quorumNumbers, | ||
PaymentMetadata: paymentMetadata, | ||
} | ||
|
||
realKey, err := blobHeader.BlobKey() | ||
require.NoError(t, err) | ||
|
||
reply := v2.DisperseBlobReply{ | ||
BlobKey: realKey[:], | ||
} | ||
|
||
require.NoError(t, verifyReceivedBlobKey(blobHeader, &reply)) | ||
|
||
blobHeader.BlobVersion = 1 | ||
require.Error(t, verifyReceivedBlobKey(blobHeader, &reply), | ||
"Any modification to the header should cause verification to fail") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "failed to verify that the reply contains locally computed blob key" or something like that..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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