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.

Sign up for GitHub

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

Jump to bottom

CertVerifier updates #1207

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

CertVerifier updates #1207

wants to merge 7 commits into from

Conversation

0x0aa0
Copy link
Contributor

@0x0aa0 0x0aa0 commented Feb 3, 2025 •
edited
Loading

Fixes edge case when blob quorums are subset of batch quorums

  • Signature check within v2 cert verification now uses the batch level quorum numbers ("signedQuorums") specified in the Attestation of the SignedBatch instead of blob level quorum numbers
  • getNonSignerStakesAndSignatures now returns NonSignerStakesAndSignatures and corresponding batch level signedQuroums
  • signedQuorums must now be passed to verifyCertV2() and the corresponding ZK wrapper
  • remove verifyDACertV2ForQuorumsForThresholds as it is not used

@bxue-l2 bxue-l2 requested a review from samlaf February 3, 2025 22:30
bxue-l2
bxue-l2 reviewed Feb 4, 2025
View reviewed changes
contracts/src/core/EigenDACertVerifier.sol
*/
function verifyDACertV2ForZKProof(
BatchHeaderV2 calldata batchHeader,
BlobInclusionInfo calldata blobInclusionInfo,
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature,
bytes memory signedQuorumNumbers
Copy link
Contributor

Choose a reason for hiding this comment

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

intention is to let proxy to set those value for now, and later pulled from rollup config?

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it is not the case, the signedQuorumNumbers is actually the all quorum for all blobs a batch, I think it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what it currently is, but it doesn't NEED to be. As we discussed with Robert the GetBlobStatus endpoint could eventually be optimized to return a different signature for each blob request. I vote to rename signedQuorumNumbers to sigmaQuorumNumbers so that the sigma name matches the sigma inside the NonSignerStakesAndSignature (sigma = signature)

samlaf
samlaf reviewed Feb 4, 2025
View reviewed changes
Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

Let's discuss during standup whether this approach is best for what we are trying to achieve. It's still not clear to me what exactly the problem is.

Feels like a problem with our DACert struct. Would prefer to modify that instead of forcing another ad-hoc parameter (batchSignedQuorums) to be passed for verification.
image

When looking at this diagram, which information is missing, and why? Where do we get it from? e.g. how is a call to verifyDACertV2 supposed to be made now that it requires an extra signedQuorumNumbers param? Where is that supposed to be fetched from?

contracts/src/interfaces/IEigenDACertVerifier.sol
/**
* @notice Returns the nonSignerStakesAndSignature for a given blob cert and signed batch
* @param signedBatch The signed batch to get the nonSignerStakesAndSignature for
*/
function getNonSignerStakesAndSignature(
SignedBatch calldata signedBatch
) external view returns (NonSignerStakesAndSignature memory);
) external view returns (NonSignerStakesAndSignature memory, bytes memory);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you document what the second bytes param is being returned is

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally by giving it a name here (can we do this?)

contracts/src/libraries/EigenDACertVerificationUtils.sol
@@ -184,7 +185,7 @@ library EigenDACertVerificationUtils {
bytes32 signatoryRecordHash
) = signatureVerifier.checkSignatures(
EigenDAHasher.hashBatchHeaderV2(batchHeader),
blobInclusionInfo.blobCertificate.blobHeader.quorumNumbers,
signedQuorumNumbers,
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the issue exactly when quorumNumbers of the blob is strict subset of those in the batch? Guessing the signedQuorumNumbers we're passing in here are those that have actually signed on the batch, whereas the quorumNumbers in the blobHeader are those that were requested?

contracts/src/libraries/EigenDACertVerificationUtils.sol
@@ -201,7 +202,7 @@ library EigenDACertVerificationUtils {

uint256 confirmedQuorumsBitmap;

for (uint i = 0; i < blobInclusionInfo.blobCertificate.blobHeader.quorumNumbers.length; i++) {
for (uint i = 0; i < signedQuorumNumbers.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

think we still only want to check the ones in the blobHeader here? Since those are the only quorums that the requester paid for and cares about

contracts/src/libraries/EigenDACertVerificationUtils.sol
confirmedQuorumsBitmap
),
"EigenDACertVerificationUtils._verifyDACertV2ForQuorums: required quorums are not a subset of the confirmed quorums"
"EigenDACertVerificationUtils._verifyDACertV2ForQuorums: blob quorums are not a subset of the confirmed quorums"
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message would be a lot more informative if it contained the actual quorum numbers that have not been signed. Is it possible to check each quorum one by one instead of checking the subset and having a generic error msg?

contracts/src/libraries/EigenDACertVerificationUtils.sol
@@ -210,16 +211,24 @@ library EigenDACertVerificationUtils {

confirmedQuorumsBitmap = BitmapUtils.setBit(
confirmedQuorumsBitmap,
uint8(blobInclusionInfo.blobCertificate.blobHeader.quorumNumbers[i])
uint8(signedQuorumNumbers[i])
);
}

require(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment above this saying something like:
"We check that the quorums that are part of the batch signatures contain both 1) the quorums requested by the particular blob, and 2) the eigenda required quorums"

contracts/src/libraries/EigenDACertVerificationUtils.sol Outdated
@@ -289,7 +301,7 @@ library EigenDACertVerificationUtils {
uint256 confirmedQuorumsBitmap;
VersionedBlobParams memory blobParams = eigenDAThresholdRegistry.getBlobParams(blobInclusionInfo.blobCertificate.blobHeader.version);

for (uint i = 0; i < blobInclusionInfo.blobCertificate.blobHeader.quorumNumbers.length; i++) {
for (uint i = 0; i < signedQuorumNumbers.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above. Why do we have this same code duplicated in both functions btw? Can we refactor into an internal function shared by both? Otherwise someone might update one and forget to update the other one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also do we still need the function _verifyDACertV2ForQuorumsForThresholds? Seems to be for allowing different thresholds for each quorum? This isn't possible right now right?
Can we document this in the natspec for this function

bxue-l2
bxue-l2 reviewed Feb 4, 2025
View reviewed changes
contracts/src/libraries/EigenDACertVerificationUtils.sol
@@ -184,7 +185,7 @@ library EigenDACertVerificationUtils {
bytes32 signatoryRecordHash
) = signatureVerifier.checkSignatures(
EigenDAHasher.hashBatchHeaderV2(batchHeader),
blobInclusionInfo.blobCertificate.blobHeader.quorumNumbers,
signedQuorumNumbers,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it right that signedQuorumNumber is based on quorum's from a batch, which contains all quorum for all blobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to put a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now yes, but doesn't necessarily have to be that way. sigma and this sigmaQuorumNumbers could be a subset of the actual batch signers, as long as it contains the quorums that the blob requested. see #1207 (comment)

contracts/src/core/EigenDACertVerifier.sol
*/
function verifyDACertV2ForZKProof(
BatchHeaderV2 calldata batchHeader,
BlobInclusionInfo calldata blobInclusionInfo,
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature,
bytes memory signedQuorumNumbers
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it is not the case, the signedQuorumNumbers is actually the all quorum for all blobs a batch, I think it makes sense.

@0x0aa0 0x0aa0 requested review from bxue-l2, samlaf and litt3 February 12, 2025 16:51
@0x0aa0 0x0aa0 changed the title CertVerifier fix CertVerifier updates Feb 12, 2025
@samlaf
Copy link
Contributor

samlaf commented Feb 12, 2025

@0x0aa0 please address the comments and make the CI pass before re-requesting for review. it's very hard to review otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mooselumph mooselumph Awaiting requested review from mooselumph

@cody-littley cody-littley Awaiting requested review from cody-littley

@ian-shim ian-shim Awaiting requested review from ian-shim

@bxue-l2 bxue-l2 Awaiting requested review from bxue-l2

@samlaf samlaf Awaiting requested review from samlaf

@litt3 litt3 Awaiting requested review from litt3

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0x0aa0 @samlaf @bxue-l2 @litt3