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

Create util to verify KZG commitment by generation and comparison #1042

Merged
merged 9 commits into from
Dec 20, 2024

Conversation

litt3
Copy link
Contributor

@litt3 litt3 commented Dec 19, 2024

Why are these changes needed?

  • This PR is part of Upstream blob certificate verification from proxy #1000
  • It includes a utility to verify kzg commitments by regenerating them, and comparing vs the claimed commitment
  • The optimized version of verification using fiat shamir transformation will be included in a future PR

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
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@litt3 litt3 self-assigned this Dec 19, 2024
@litt3 litt3 marked this pull request as ready for review December 19, 2024 21:06
api/clients/verification/verification_utils_test.go Outdated Show resolved Hide resolved
api/clients/verification/verification_utils_test.go Outdated Show resolved Hide resolved
api/clients/verification/verification_utils_test.go Outdated Show resolved Hide resolved
api/clients/verification/verification_utils_test.go Outdated Show resolved Hide resolved
api/clients/verification/verification_utils_test.go Outdated Show resolved Hide resolved
api/clients/verification/verification_utils_test.go Outdated Show resolved Hide resolved
@litt3 litt3 requested a review from samlaf December 19, 2024 21:32
return fmt.Errorf("compute commitment: %w", err)
}

if claimedCommitment.X.Equal(&computedCommitment.X) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was going to say we'd need to check if claimedCommitment is on the curve but since its checked against what's computed locally, its fine.

@litt3 litt3 merged commit 290bc67 into Layr-Labs:master Dec 20, 2024
8 checks passed

// GenerateBlobCommitment computes a kzg-bn254 commitment of blob data using SRS
func GenerateBlobCommitment(
kzgVerifier *verifier.Verifier,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: why didn’t you make these methods on the verifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unconscious hesitancy to modify an existing utility, I suppose.

You're right, it's where the methods belong. I will fix this in an upcoming PR

@litt3 litt3 deleted the verification-via-regeneration branch December 20, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants