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

feat(eigen-client-extra-features): Verifier #326

Merged
merged 28 commits into from
Nov 5, 2024

Conversation

gianbelinche
Copy link

What ❔

This PR implements verifier for EigenDA

Since config was changed, recompile zkstack:
cargo install --path zkstack_cli/crates/zkstack --force --locked

Why ❔

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zkstack dev fmt and zkstack dev lint.

Copy link

@juan518munoz juan518munoz left a comment

Choose a reason for hiding this comment

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

LGTM. Be sure to remove all unused imports before merging!

pub path_to_points: String,
}

#[derive(Debug, Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

can you add some docs to this struct?

})
}

fn commit(&self, blob: Vec<u8>) -> Result<G1Affine, VerificationError> {
Copy link
Member

Choose a reason for hiding this comment

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

Add docs

Ok(())
}

pub async fn verify_certificate(&self, cert: BlobInfo) -> Result<(), VerificationError> {
Copy link
Member

Choose a reason for hiding this comment

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

add docs

Copy link
Member

@juanbono juanbono left a comment

Choose a reason for hiding this comment

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

Add docs

@gianbelinche
Copy link
Author

Add docs

Done

let blob_info = blob_info::BlobInfo::try_from(blob_info)
.map_err(|e| anyhow::anyhow!("Failed to convert blob info: {}", e))?;
self.verifier
.verify_commitment(blob_info.blob_header.commitment.clone(), data)
Copy link

Choose a reason for hiding this comment

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

In this code, I was looking at this file. Where does eigenda fit in the match (self.mode, self.pubdata_da) result ?

Copy link

Choose a reason for hiding this comment

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

To clarify, is this computing the commitment locally to see if the DA has computed the same commitment ?

Copy link
Author

Choose a reason for hiding this comment

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

We fit in (L1BatchCommitmentMode::Validium, PubdataSendingMode::Custom)
The verifier.verifyCommitment function is the one that generates a commitment based on the blob and compares it to the one given by EigenDA.
The code you were looking at is the one that defines what would be sent to the L1, once we make the PR with full M0, that part will send the EigenDA commitment.

&format!("{}{}", cfg.path_to_points, "/g1.point"),
"",
&format!("{}{}", cfg.path_to_points, "/g2.point.powerOf2"),
268435456, // 2 ^ 32
Copy link

Choose a reason for hiding this comment

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

small correction, its 2 ^ 28

let expected_commitment = G1Affine::new_unchecked(
Fq::from(num_bigint::BigUint::from_bytes_be(&expected_commitment.x)),
Fq::from(num_bigint::BigUint::from_bytes_be(&expected_commitment.y)),
);
Copy link

@anupsv anupsv Nov 5, 2024

Choose a reason for hiding this comment

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

We should add a isInSubgroup and IsOnCurve checks. Good to catch it even though DA also does it on its end.

) -> Result<Vec<u8>, VerificationError> {
let mut index = index;
if proof.len() == 0 || proof.len() % 32 != 0 {
return Err(VerificationError::WrongProof);
Copy link

Choose a reason for hiding this comment

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

Would be good to differentiate the errors since the VerificationError::WrongProof is used in many places.

Base automatically changed from eigen-client-memstore to eigen-client-extra-features November 5, 2024 16:10
Copy link
Member

@juanbono juanbono left a comment

Choose a reason for hiding this comment

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

LGTM

@gianbelinche gianbelinche merged commit 9c06b40 into eigen-client-extra-features Nov 5, 2024
12 of 24 checks passed
@gianbelinche gianbelinche deleted the eigen-client-verifier branch November 5, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants