Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions api/clients/v2/disperser_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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..?

Copy link
Contributor Author

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

}

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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 DisperseBlob as clean as possible

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
Expand Down
51 changes: 51 additions & 0 deletions api/clients/v2/disperser_client_test.go
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")
}
Loading