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

V2 Blob Verification #781

Merged
merged 26 commits into from
Dec 11, 2024
Merged

V2 Blob Verification #781

merged 26 commits into from
Dec 11, 2024

Conversation

0x0aa0
Copy link
Contributor

@0x0aa0 0x0aa0 commented Oct 2, 2024

  • V2
  • consolidates structs into common interface
  • moves V1 thresholds to ThresholdRegistry while maintaining DASM interface
  • renames RollupUtils to BlobVerificationUtils which implements V2 and batched util for V1
  • introduces chain abstract immutable BlobVerifier contract to interact with BlobVerificationUtils
  • adds relay registry

@0x0aa0 0x0aa0 changed the title blob verification refactor V2 Blob Verification Nov 19, 2024
@0x0aa0 0x0aa0 marked this pull request as ready for review November 19, 2024 15:34
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

few questions


bytes public quorumConfirmationThresholdPercentages;

bytes public quorumNumbersRequired;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still necessary? I thought we were going to give flexibility to disperse to any quorums except charging the same amount for dispersing to 1 quorum vs. all required quorums

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya this is still needed for V1 verification

contracts/src/interfaces/IEigenDABlobVerifier.sol Outdated Show resolved Hide resolved

mapping(uint16 => VersionedBlobParams) public versionedBlobParams;

SecurityThresholds public defaultSecurityThresholdsV2;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we allowing the ability to provide custom security thresholds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you can supply custom security thresholds in V2 verification

* @param blobHeader The blob header to verify
* @param blobVerificationProof The blob verification proof to verify the blob against
*/
function verifyBlobV1(
Copy link
Contributor

Choose a reason for hiding this comment

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

do existing v1 integrations need to update and call this new method or can they continue using the old verify method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

old library will continue to work

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 we need V1 verifications - anyone calling these functions will do so directly through the Service Manager wrapper (i.e, Verifier contracts in Nitro). Don't see how this enables or helps cross compatibility between V1 <--> V2.

contract EigenDARelayRegistry is IEigenDARelayRegistry, OwnableUpgradeable {

mapping(uint32 => string) public relayIdToURL;
mapping(address => uint32) public relayAddressToId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious what's a relay address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onchain address of a relay, the thinking here is that in the future we may want this mapping to permission updates from relays

contracts/src/interfaces/IEigenDAStructs.sol Outdated Show resolved Hide resolved
@0x0aa0 0x0aa0 mentioned this pull request Nov 20, 2024
@0x0aa0 0x0aa0 requested a review from EthenNotEthan December 2, 2024 20:32
contracts/compile.sh Outdated Show resolved Hide resolved
return nextRelayKey++;
}

function getRelayURL(uint32 key) external view returns (string memory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should it check if key < nextRelayKey?


/// @notice Returns the blob params for a given blob version
function getBlobParams(uint16 version) external view returns (VersionedBlobParams memory) {
return versionedBlobParams[version];
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, should we check version < nextBlobVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dont think it is totally necessary to require on the getter

@ian-shim
Copy link
Contributor

ian-shim commented Dec 3, 2024

One thing to keep consistent is which uint types represent blob versions/relay keys. Offchain, we use uint8 for blob versions and uint16 for relay keys, but here we're using uint16 and uint32 respectively. Which types do we want? cc @mooselumph @cody-littley

@@ -226,6 +242,42 @@ contract EigenDADeployer is DeployOpenEigenLayer {
)
);

eigenDAThresholdRegistryImplementation = new EigenDAThresholdRegistry();

VersionedBlobParams[] memory versionedBlobParams = new VersionedBlobParams[](0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to hardcode the params for version 0 here if we're going to use the same values across all environments?

_disableInitializers();
}

function initialize(
Copy link

Choose a reason for hiding this comment

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

addd natspec to interfaces and use @inheritdoc

}

/// @notice Returns the bytes array of quorumAdversaryThresholdPercentages
function quorumConfirmationThresholdPercentages() external view returns (bytes memory) {
return hex"373737";
return eigenDAThresholdRegistry.quorumConfirmationThresholdPercentages();
}

/// @notice Returns the bytes array of quorumsNumbersRequired
function quorumNumbersRequired() external view returns (bytes memory) {
Copy link

Choose a reason for hiding this comment

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

is this for backwards compatability or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

function getQuorumAdversaryThresholdPercentage(
uint8 quorumNumber
) public view virtual returns (uint8 adversaryThresholdPercentage) {
if(quorumAdversaryThresholdPercentages.length > quorumNumber){
Copy link

Choose a reason for hiding this comment

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

we should revert if no?

Comment on lines +124 to +126
BatchHeaderV2 calldata batchHeader,
BlobVerificationProofV2 calldata blobVerificationProof,
NonSignerStakesAndSignature calldata nonSignerStakesAndSignature
Copy link
Contributor

Choose a reason for hiding this comment

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

on the rollup side we interpret this three element tuple as a certificate type. Is there any benefit to representing the EigenDA certificate type here?

Copy link
Contributor

Choose a reason for hiding this comment

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

we implicitly use this term across our documentation and integrations - would be nice to have an explicit definition

import {IEigenDARelayRegistry} from "../interfaces/IEigenDARelayRegistry.sol";
import "../interfaces/IEigenDAStructs.sol";

contract EigenDABlobVerifier is IEigenDABlobVerifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

In lieu of all the renamings going on - does it make sense to call this contract a BlobVerifier? It's not actually verifying the blob but instead a tuple of different commitments which attest that the blob has been successfully aggregated and dispersed. Verifying the blob makes me think of providing a kzg witness proof generated from the blob and verifying against the data commitment which this contract doesn't do.

CC @samlaf @bxue-l2

* @param blobHeader The blob header to verify
* @param blobVerificationProof The blob verification proof to verify the blob against
*/
function verifyBlobV1(
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 we need V1 verifications - anyone calling these functions will do so directly through the Service Manager wrapper (i.e, Verifier contracts in Nitro). Don't see how this enables or helps cross compatibility between V1 <--> V2.

Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

One comment outside this PR but relevant, shouldn't the reference block number be removed from blob certificate?

@0x0aa0 0x0aa0 merged commit 3ed9ef6 into master Dec 11, 2024
9 checks passed
@@ -65,7 +70,6 @@ struct BlobVerificationProofV2 {

struct BlobCertificate {
BlobHeaderV2 blobHeader;
uint32 referenceBlockNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be removed from Attestation as well

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.

5 participants