Create a blob verification utility #1066

litt3 · 2025-01-06T16:24:36Z

Why are these changes needed?

This verification utility is needed for the v2 client

Notes

I made the decision to not add the verification contract logic into the existing reader.go, since doing so would cause a cascade of changes in unrelated files
- IMO, I don't see the advantage of piling all of this stuff into a single class, when the only thing shared is the eth client

Checks

I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy: This verification utility cannot be meaningfully unit tested. E2E integration tests will be created in a future PR.

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

api/grpc/disperser/v2/conversion_utils.go

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

epociask · 2025-01-07T16:09:30Z

api/clients/v2/verification/blob_verifier.go

+	gethcommon "github.com/ethereum/go-ethereum/common"
+)
+
+// BlobVerifier is responsible for making eth calls to verify blobs that have been received by the client


knit

Suggested change

// BlobVerifier is responsible for making eth calls to verify blobs that have been received by the client

// BlobVerifier is responsible for making eth calls against the BlobVerifier contract to ensure cryptographic and structural integrity of V2 certificates

epociask · 2025-01-07T16:09:51Z

api/clients/v2/verification/blob_verifier.go

+
+// BlobVerifier is responsible for making eth calls to verify blobs that have been received by the client
+type BlobVerifier struct {
+	// go binding around the verifyBlobV2FromSignedBatch ethereum contract


isn't the contract name BlobVerifier?

It's EigenDABlobVerifier. Fixed in ca8d9d74

epociask · 2025-01-07T16:10:31Z

api/clients/v2/verification/blob_verifier.go

+// NewBlobVerifier constructs a BlobVerifier
+func NewBlobVerifier(
+	ethClient *common.EthClient,               // the eth client, which should already be set up
+	verifyBlobV2FromSignedBatchAddress string, // the hex address of the verifyBlobV2FromSignedBatch contract


knit - why not just blobVerifierAddress?

epociask · 2025-01-07T16:12:37Z

api/clients/v2/verification/blob_verifier.go

+	}
+
+	err = v.blobVerifierCaller.VerifyBlobV2FromSignedBatch(
+		&bind.CallOpts{Context: ctx},


knit - is there any utility in allowing a configurable user timeout for simulation calls?

if there's not a default timeout used by the binding wrapper then we may wanna instate given this could result in the connection infinitely hanging

Added a comment clarifying that the caller is responsible for any timeout c752e5b1

epociask · 2025-01-07T16:40:55Z

api/clients/v2/verification/blob_verifier.go

+	}, nil
+}
+
+// VerifyBlobV2FromSignedBatch makes a call to the verifyBlobV2FromSignedBatch contract


verifyBlobV2FromSignedBatch contract?

Fixed terminology ca8d9d74

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

samlaf

@litt3 you must be a masochist asking for so many people to review your PR. As the saying goes: 10 line PR = 10 comments. 1000 line PR = LGTM. So here's my 2 comments :D

samlaf · 2025-01-08T00:35:41Z

api/clients/v2/verification/blob_verifier.go

+	gethcommon "github.com/ethereum/go-ethereum/common"
+)
+
+// BlobVerifier is responsible for making eth calls against the BlobVerifier contract to ensure cryptographic and


add link to BlobVerifier contract?

Do you mean like this? I don't like that this sort of link is instantly out of date, but I don't know of a better way to link here

eigenda/contracts/src/core/EigenDABlobVerifier.sol

Line 108 in 06e88b3

function verifyBlobV2FromSignedBatch(

Yes did mean like this. We can also just link to the contract itself, in the master branch, so that it doesn't get out of date instantly.

Added 8bba429a

api/clients/v2/verification/blob_verifier.go

bxue-l2 · 2025-01-08T04:20:22Z

contracts/bindings/EigenDABlobVerifier/conversion_utils.go

+
+func bytesToBN254G2Point(bytes []byte) (*BN254G2Point, error) {
+	var g2Point bn254.G2Affine
+	_, err := g2Point.SetBytes(bytes)


maybe add a comment to say, this makes sure bytes is in the subgroup
https://pkg.go.dev/github.com/consensys/gnark-crypto/ecc/bn254#G2Affine.SetBytes

Done f31e4a6b

bxue-l2 · 2025-01-08T04:22:40Z

One thing that looks missing to me is the test. Though it looks difficult to test, unless we can mock the eth call, and prepare all the consistent data

litt3 · 2025-01-08T14:34:25Z

@litt3 you must be a masochist asking for so many people to review your PR. As the saying goes: 10 line PR = 10 comments. 1000 line PR = LGTM. So here's my 2 comments :D

@samlaf Unfortunately github has no concept of "optional reviewer"... better lots of comments now than lots of bugs tomorrow

litt3 · 2025-01-08T14:42:42Z

One thing that looks missing to me is the test. Though it looks difficult to test, unless we can mock the eth call, and prepare all the consistent data

@bxue-l2 I considered mocking the eth call, but it would be a lot of boilerplate to really only test the simple logic of whether errors are returned. This seems to fall into the "99% coverage" bucket.

I wouldn't have a problem with a 99% coverage policy, if that's what we want to go for across the board. Thoughts?

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

bxue-l2 · 2025-01-08T17:52:27Z

One thing that looks missing to me is the test. Though it looks difficult to test, unless we can mock the eth call, and prepare all the consistent data

@bxue-l2 I considered mocking the eth call, but it would be a lot of boilerplate to really only test the simple logic of whether errors are returned. This seems to fall into the "99% coverage" bucket.

I wouldn't have a problem with a 99% coverage policy, if that's what we want to go for across the board. Thoughts?

Got it, I am not trying to impose a high coverage policy, The code looks good to me.

ian-shim · 2025-01-08T21:59:44Z

contracts/bindings/EigenDABlobVerifier/conversion_utils.go

+	}
+
+	var x, y [2]*big.Int
+	x[0] = g2Point.X.A0.BigInt(new(big.Int))


Has this been tested?
I'm pretty sure the ordering should be reversed, i.e. X = {X.A1, X.A0}

Has this been tested?

Not yet, integration tests are forthcoming

Good catch, thank you. Fixed

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 added 2 commits January 3, 2025 17:10

Draft blob verification

a0d7808

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Try rearranging conversion methods

dcbcc07

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 self-assigned this Jan 6, 2025

litt3 added 8 commits January 6, 2025 13:18

Fix style

2e99455

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into blob-verify-util

30bf639

Pass in eth client instead of connecting in constructor

90e3e43

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Sort imports

7c6984e

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Remove unnecessary member

ec56c39

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Remove incorrect comment

b44b119

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into blob-verify-util

4db5acc

Clean up comment

8777294

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 requested review from samlaf, epociask, 0x0aa0, cody-littley and bxue-l2 January 7, 2025 15:52

litt3 marked this pull request as ready for review January 7, 2025 15:53

cody-littley approved these changes Jan 7, 2025

View reviewed changes

cody-littley reviewed Jan 7, 2025

View reviewed changes

api/grpc/disperser/v2/conversion_utils.go Outdated Show resolved Hide resolved

Move utility as suggested in review

2e79dc6

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

litt3 requested review from cody-littley and anupsv January 7, 2025 16:43

cody-littley approved these changes Jan 7, 2025

View reviewed changes

epociask reviewed Jan 7, 2025

View reviewed changes

Fix doc terminology

ca8d9d7

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

anupsv approved these changes Jan 7, 2025

View reviewed changes

litt3 added 2 commits January 7, 2025 16:05

Mention timeout responsiblity

c752e5b

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into blob-verify-util

257264c

litt3 requested a review from epociask January 7, 2025 21:48

samlaf reviewed Jan 8, 2025

View reviewed changes

bxue-l2 reviewed Jan 8, 2025

View reviewed changes

litt3 added 2 commits January 8, 2025 09:44

Add additional comment

f43c7ce

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into blob-verify-util

f31e4a6

0x0aa0 approved these changes Jan 8, 2025

View reviewed changes

litt3 requested a review from samlaf January 8, 2025 19:48

ian-shim reviewed Jan 8, 2025

View reviewed changes

litt3 added 3 commits January 9, 2025 11:00

Fix int ordering for G2 points

66249bc

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Merge branch 'master' into blob-verify-util

62d7476

Add link to verifier contract

8bba429

Signed-off-by: litt3 <102969658+litt3@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a blob verification utility #1066

Create a blob verification utility #1066

litt3 commented Jan 6, 2025 •

edited

Loading

epociask Jan 7, 2025

litt3 Jan 7, 2025

epociask Jan 7, 2025

litt3 Jan 7, 2025 •

edited

Loading

epociask Jan 7, 2025

litt3 Jan 7, 2025

epociask Jan 7, 2025

epociask Jan 7, 2025

litt3 Jan 7, 2025

epociask Jan 7, 2025

litt3 Jan 7, 2025

samlaf left a comment

samlaf Jan 8, 2025

litt3 Jan 8, 2025

samlaf Jan 9, 2025

litt3 Jan 9, 2025

bxue-l2 Jan 8, 2025

litt3 Jan 8, 2025

bxue-l2 commented Jan 8, 2025 •

edited

Loading

litt3 commented Jan 8, 2025 •

edited

Loading

litt3 commented Jan 8, 2025 •

edited

Loading

bxue-l2 commented Jan 8, 2025

ian-shim Jan 8, 2025 •

edited

Loading

litt3 Jan 9, 2025

	// BlobVerifier is responsible for making eth calls to verify blobs that have been received by the client
	// BlobVerifier is responsible for making eth calls against the BlobVerifier contract to ensure cryptographic and structural integrity of V2 certificates

Create a blob verification utility #1066

Are you sure you want to change the base?

Create a blob verification utility #1066

Conversation

litt3 commented Jan 6, 2025 • edited Loading

Why are these changes needed?

Notes

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litt3 Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bxue-l2 commented Jan 8, 2025 • edited Loading

litt3 commented Jan 8, 2025 • edited Loading

litt3 commented Jan 8, 2025 • edited Loading

bxue-l2 commented Jan 8, 2025

ian-shim Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litt3 commented Jan 6, 2025 •

edited

Loading

litt3 Jan 7, 2025 •

edited

Loading

bxue-l2 commented Jan 8, 2025 •

edited

Loading

litt3 commented Jan 8, 2025 •

edited

Loading

litt3 commented Jan 8, 2025 •

edited

Loading

ian-shim Jan 8, 2025 •

edited

Loading