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

CertVerifier updates #1207

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
6 changes: 5 additions & 1 deletion api/clients/v2/payload_disperser.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,11 @@ func (pd *PayloadDisperser) buildEigenDACert(

timeoutCtx, cancel := context.WithTimeout(ctx, pd.config.ContractCallTimeout)
defer cancel()
nonSignerStakesAndSignature, err := pd.certVerifier.GetNonSignerStakesAndSignature(

// TODO (litt3): @reviewers I think this method (buildEigenDACert) is the main end user of GetNonSignerStakesAndSignature,
// and we are just discarding the signedQuorumNumbers returned from GetNonSignerStakesAndSignature. IMO this points
// to returning signedQuorumNumbers being the wrong API choice
nonSignerStakesAndSignature, _, err := pd.certVerifier.GetNonSignerStakesAndSignature(
timeoutCtx, blobStatusReply.GetSignedBatch())
if err != nil {
return nil, fmt.Errorf("get non signer stake and signature: %w", err)
Expand Down
23 changes: 14 additions & 9 deletions api/clients/v2/verification/cert_verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type ICertVerifier interface {
GetNonSignerStakesAndSignature(
ctx context.Context,
signedBatch *disperser.SignedBatch,
) (*verifierBindings.NonSignerStakesAndSignature, error)
) (*verifierBindings.NonSignerStakesAndSignature, []uint8, error)
}

// CertVerifier is responsible for making eth calls against the CertVerifier contract to ensure cryptographic and
Expand Down Expand Up @@ -151,7 +151,8 @@ func (cv *CertVerifier) VerifyCertV2(
&bind.CallOpts{Context: ctx},
eigenDACert.BatchHeader,
eigenDACert.BlobInclusionInfo,
eigenDACert.NonSignerStakesAndSignature)
eigenDACert.NonSignerStakesAndSignature,
eigenDACert.SignedQuorumNumbers)

if err != nil {
return fmt.Errorf("verify cert v2: %w", err)
Expand All @@ -161,7 +162,11 @@ func (cv *CertVerifier) VerifyCertV2(
}

// GetNonSignerStakesAndSignature calls the getNonSignerStakesAndSignature view function on the EigenDACertVerifier
// contract, and returns the resulting NonSignerStakesAndSignature object.
// contract, and returns the resulting NonSignerStakesAndSignature object and signed quorum numbers. The signed quorum
// numbers are the quorum numbers that directly match the quorum numbers in the SignedBatch.Attestation.
//
// TODO (litt3): I don't like that we are simply returning a portion of the input parameter here. I think it's a
// potential source of confusion. @reviewers, please consider this structure and opine
//
// Before getting the NonSignerStakesAndSignature, this method will wait for the internal client to advance to a
// sufficient block height. This wait will time out if the duration exceeds the timeout configured for the input ctx
Expand All @@ -171,27 +176,27 @@ func (cv *CertVerifier) VerifyCertV2(
func (cv *CertVerifier) GetNonSignerStakesAndSignature(
ctx context.Context,
signedBatch *disperser.SignedBatch,
) (*verifierBindings.NonSignerStakesAndSignature, error) {
) (*verifierBindings.NonSignerStakesAndSignature, []uint8, error) {

signedBatchBinding, err := SignedBatchProtoToBinding(signedBatch)
if err != nil {
return nil, fmt.Errorf("convert signed batch: %w", err)
return nil, nil, fmt.Errorf("convert signed batch: %w", err)
}

err = cv.MaybeWaitForBlockNumber(ctx, signedBatch.GetHeader().GetReferenceBlockNumber())
if err != nil {
return nil, fmt.Errorf("wait for block number: %w", err)
return nil, nil, fmt.Errorf("wait for block number: %w", err)
}

nonSignerStakesAndSignature, err := cv.certVerifierCaller.GetNonSignerStakesAndSignature(
nonSignerStakesAndSignature, signedQuorumNumbers, err := cv.certVerifierCaller.GetNonSignerStakesAndSignature(
&bind.CallOpts{Context: ctx},
*signedBatchBinding)

if err != nil {
return nil, fmt.Errorf("get non signer stakes and signature: %w", err)
return nil, nil, fmt.Errorf("get non signer stakes and signature: %w", err)
}

return &nonSignerStakesAndSignature, nil
return &nonSignerStakesAndSignature, signedQuorumNumbers, nil
}

// MaybeWaitForBlockNumber waits until the internal eth client has advanced to a certain targetBlockNumber, unless
Expand Down
33 changes: 23 additions & 10 deletions api/clients/v2/verification/conversion_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,9 @@ func blobHeaderProtoToBinding(inputHeader *commonv2.BlobHeader) (*contractEigenD
math.MaxUint16)
}

var quorumNumbers []byte
for _, quorumNumber := range inputHeader.GetQuorumNumbers() {
if quorumNumber > math.MaxUint8 {
return nil, fmt.Errorf(
"quorum number overflow: value was %d, but max allowable value is %d",
quorumNumber,
uint8(math.MaxUint8))
}

quorumNumbers = append(quorumNumbers, byte(quorumNumber))
quorumNumbers, err := QuorumNumbersUint32ToUint8(inputHeader.GetQuorumNumbers())
if err != nil {
return nil, fmt.Errorf("convert quorum numbers to uint8: %s", err)
}

convertedBlobCommitment, err := blobCommitmentProtoToBinding(inputHeader.GetCommitment())
Expand Down Expand Up @@ -308,3 +301,23 @@ func BlobCommitmentsBindingToInternal(

return blobCommitment, nil
}

// QuorumNumbersUint32ToUint8 accepts an array of uint32 quorum numbers, and converts it into an array of uint8 quorum
// numbers.
//
// Returns an error if any quorum number is too large to fit into a uint8
func QuorumNumbersUint32ToUint8(inputQuorums []uint32) ([]uint8, error) {
var outputQuorums []byte
for _, quorumNumber := range inputQuorums {
if quorumNumber > math.MaxUint8 {
return nil, fmt.Errorf(
"quorum number overflow: value was %d, but max allowable value is %d",
quorumNumber,
uint8(math.MaxUint8))
}

outputQuorums = append(outputQuorums, byte(quorumNumber))
}

return outputQuorums, nil
}
11 changes: 10 additions & 1 deletion api/clients/v2/verification/eigenda_cert.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type EigenDACert struct {
BlobInclusionInfo contractEigenDACertVerifier.BlobInclusionInfo
BatchHeader contractEigenDACertVerifier.BatchHeaderV2
NonSignerStakesAndSignature contractEigenDACertVerifier.NonSignerStakesAndSignature
SignedQuorumNumbers []byte
}

// BuildEigenDACert creates a new EigenDACert from a BlobStatusReply, and NonSignerStakesAndSignature
Expand All @@ -28,15 +29,23 @@ func BuildEigenDACert(
return nil, fmt.Errorf("convert inclusion info to binding: %w", err)
}

bindingBatchHeader, err := BatchHeaderProtoToBinding(blobStatusReply.GetSignedBatch().GetHeader())
signedBatch := blobStatusReply.GetSignedBatch()

bindingBatchHeader, err := BatchHeaderProtoToBinding(signedBatch.GetHeader())
if err != nil {
return nil, fmt.Errorf("convert batch header to binding: %w", err)
}

quorumNumbers, err := QuorumNumbersUint32ToUint8(signedBatch.GetAttestation().GetQuorumNumbers())
if err != nil {
return nil, fmt.Errorf("convert quorum numbers to uint8: %w", err)
}

return &EigenDACert{
BlobInclusionInfo: *bindingInclusionInfo,
BatchHeader: *bindingBatchHeader,
NonSignerStakesAndSignature: *nonSignerStakesAndSignature,
SignedQuorumNumbers: quorumNumbers,
}, nil
}

Expand Down
69 changes: 35 additions & 34 deletions contracts/bindings/EigenDACertVerifier/binding.go

Large diffs are not rendered by default.

18 changes: 13 additions & 5 deletions contracts/src/core/EigenDACertVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,13 @@ contract EigenDACertVerifier is IEigenDACertVerifier {
* @param batchHeader The batch header of the blob
* @param blobInclusionInfo The inclusion proof for the blob cert
* @param nonSignerStakesAndSignature The nonSignerStakesAndSignature to verify the blob cert against
* @param signedQuorumNumbers The signed quorum numbers corresponding to the nonSignerStakesAndSignature
*/
function verifyDACertV2(
BatchHeaderV2 calldata batchHeader,
BlobInclusionInfo calldata blobInclusionInfo,
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature,
bytes memory signedQuorumNumbers
) external view {
EigenDACertVerificationUtils._verifyDACertV2ForQuorums(
eigenDAThresholdRegistry,
Expand All @@ -112,7 +114,8 @@ contract EigenDACertVerifier is IEigenDACertVerifier {
blobInclusionInfo,
nonSignerStakesAndSignature,
securityThresholdsV2,
quorumNumbersRequiredV2
quorumNumbersRequiredV2,
signedQuorumNumbers
);
}

Expand Down Expand Up @@ -145,11 +148,13 @@ contract EigenDACertVerifier is IEigenDACertVerifier {
* @param batchHeader The batch header of the blob
* @param blobInclusionInfo The inclusion proof for the blob cert
* @param nonSignerStakesAndSignature The nonSignerStakesAndSignature to verify the blob cert against
* @param signedQuorumNumbers The signed quorum numbers corresponding to the nonSignerStakesAndSignature
*/
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)

) external view returns (bool) {
try EigenDACertVerificationUtils.verifyDACertV2ForQuorumsExternal(
eigenDAThresholdRegistry,
Expand All @@ -159,7 +164,8 @@ contract EigenDACertVerifier is IEigenDACertVerifier {
blobInclusionInfo,
nonSignerStakesAndSignature,
securityThresholdsV2,
quorumNumbersRequiredV2
quorumNumbersRequiredV2,
signedQuorumNumbers
) {
return true;
} catch {
Expand All @@ -172,10 +178,12 @@ contract EigenDACertVerifier is IEigenDACertVerifier {
/**
* @notice Returns the nonSignerStakesAndSignature for a given blob cert and signed batch
* @param signedBatch The signed batch to get the nonSignerStakesAndSignature for
* @return nonSignerStakesAndSignature The nonSignerStakesAndSignature for the given signed batch attestation
* @return signedQuorumNumbers The signed quorum numbers for the given signed batch attestation
*/
function getNonSignerStakesAndSignature(
SignedBatch calldata signedBatch
) external view returns (NonSignerStakesAndSignature memory) {
) external view returns (NonSignerStakesAndSignature memory, bytes memory) {
return EigenDACertVerificationUtils._getNonSignerStakesAndSignature(
operatorStateRetriever,
registryCoordinator,
Expand Down
22 changes: 20 additions & 2 deletions contracts/src/interfaces/IEigenDACertVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,13 @@ interface IEigenDACertVerifier is IEigenDAThresholdRegistry {
* @param batchHeader The batch header of the blob
* @param blobInclusionInfo The inclusion proof for the blob cert
* @param nonSignerStakesAndSignature The nonSignerStakesAndSignature to verify the blob cert against
* @param signedQuorumNumbers The signed quorum numbers corresponding to the nonSignerStakesAndSignature
*/
function verifyDACertV2(
BatchHeaderV2 calldata batchHeader,
BlobInclusionInfo calldata blobInclusionInfo,
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature,
bytes memory signedQuorumNumbers
) external view;

/**
Expand All @@ -49,13 +51,29 @@ interface IEigenDACertVerifier is IEigenDAThresholdRegistry {
BlobInclusionInfo calldata blobInclusionInfo
) external view;

/**
* @notice Thin try/catch wrapper around verifyDACertV2 that returns false instead of panicing
* @dev The Steel library (https://github.com/risc0/risc0-ethereum/tree/main/crates/steel)
* currently has a limitation that it can only create zk proofs for functions that return a value
* @param batchHeader The batch header of the blob
* @param blobInclusionInfo The inclusion proof for the blob cert
* @param nonSignerStakesAndSignature The nonSignerStakesAndSignature to verify the blob cert against
* @param signedQuorumNumbers The signed quorum numbers corresponding to the nonSignerStakesAndSignature
*/
function verifyDACertV2ForZKProof(
BatchHeaderV2 calldata batchHeader,
BlobInclusionInfo calldata blobInclusionInfo,
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature,
bytes memory signedQuorumNumbers
) external view returns (bool);

/**
* @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?)


/**
* @notice Verifies the security parameters for a blob cert
Expand Down
Loading
Loading