feat: cert verifier v3 + router #1518

pakim249CAL · 2025-04-30T22:21:17Z

Why are these changes needed?

Closes DAINT-383.

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 :(

fix: remove operator state retriever change fix: readd v1 test

contracts/src/libraries/V2/EigenDACertVerificationV2Lib.sol

contracts/src/interfaces/IEigenDACertVerifier.sol

contracts/src/libraries/V3/EigenDACertVerificationV3Lib.sol

contracts/src/libraries/V3/EigenDATypesV3.sol

contracts/src/periphery/EigenDACertVerifierRouter.sol

contracts/src/periphery/EigenDACertVerifierV3.sol

feat: remove old files refactor: file layout

contracts/src/interfaces/IEigenDACertVerifier.sol

contracts/src/periphery/EigenDACertVerifierRouter.sol

…r-v3

…ntract-types

…r-v3

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

contracts/src/periphery/cert/v3/EigenDACertVerifierV3.sol

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

contracts/src/periphery/cert/EigenDACertTypes.sol

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

…r-v3

Co-authored-by: ethenotethan <[email protected]>

contracts/src/periphery/cert/interfaces/IEigenDACertTypeBindings.sol

contracts/src/periphery/cert/interfaces/IEigenDACertVerifierBase.sol

contracts/src/periphery/cert/interfaces/IEigenDACertVerifierRouter.sol

contracts/test/unit/EigenDACertVerifierV3Unit.t.sol

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

contracts/src/periphery/cert/v3/EigenDACertVerifierV3.sol

pakim249CAL · 2025-05-15T13:33:33Z

contracts/test/unit/EigenDACertVerifierV2Unit.t.sol

-        EigenDATypesV1.NonSignerStakesAndSignature memory nonSignerStakesAndSignature;
-        nonSignerStakesAndSignature.nonSignerQuorumBitmapIndices = nssas.nonSignerQuorumBitmapIndices;
-        nonSignerStakesAndSignature.nonSignerPubkeys = nssas.nonSignerPubkeys;
-        nonSignerStakesAndSignature.quorumApks = nssas.quorumApks;
-        nonSignerStakesAndSignature.apkG2 = nssas.apkG2;
-        nonSignerStakesAndSignature.sigma = nssas.sigma;
-        nonSignerStakesAndSignature.quorumApkIndices = nssas.quorumApkIndices;
-        nonSignerStakesAndSignature.totalStakeIndices = nssas.totalStakeIndices;
-        nonSignerStakesAndSignature.nonSignerStakeIndices = nssas.nonSignerStakeIndices;


If you look carefully, you will see that none of this is used at all. Only the _nonSignerStakesAndSignature below is. Hence the deletions.

ethenotethan

LGTM

litt3 · 2025-05-16T14:12:27Z

contracts/src/periphery/cert/interfaces/IEigenDACertVerifier.sol

-    /// @notice Check a DA cert's validity
-    /// @return status An enum value. Success is always mapped to 1, and other values are errors specific to each CertVerifier.
-    function checkDACert(bytes calldata certBytes) external view returns (uint8 status);
+interface IEigenDACertVerifier is IEigenDACertVerifierBase {


I chatted with @samlaf about this, and thought deeper about the interface structure here. This is how I think it ought to be:

When you call IEigenDACertVerifierRouter.getCertVerifierAt, you get back an address

The address can be cast to an IVersionedCertVerifier

You call IVersionedCertVerifier.getVersion

Based on the version, you know what specific type of verifier you have (IEigenDACertVerifierV3 in this case), so you know what to cast the address to, specifically

And this is the current implementation, as I understand it:

I'm not sure what the data flow would look like here. After getting back the address from the router, you'd have to directly cast to the IEigenDACertVerifier type. But that means we're locked into that specific interface: what if we want to create an implementation of the CertVerifier which has different types of blob parameters, for example?

https://app.excalidraw.com/s/1XPZRMVbRNH/5Dnis41FFwD

samlaf · 2025-05-16T13:43:42Z

contracts/src/periphery/cert/interfaces/IEigenDACertVerifier.sol

+    /// @return The EigenDA signature verifier contract.
+    function eigenDASignatureVerifier() external view returns (IEigenDASignatureVerifier);


Why does this need to be exposed in the interface? Its just called under the hood when verifying the cert right?

I agree actually. This shouldn't be here. I'll remove it.

Can you please add commit links? Makes the reviewer's job much much easier.

contracts/src/periphery/cert/interfaces/IEigenDACertVerifier.sol

samlaf · 2025-05-16T13:56:35Z

contracts/src/periphery/cert/interfaces/IEigenDACertVerifierRouter.sol

+import {IEigenDACertVerifierBase} from "src/periphery/cert/interfaces/IEigenDACertVerifierBase.sol";
+
+interface IEigenDACertVerifierRouter is IEigenDACertVerifierBase {
+    /// @notice Returns the address for a cert verifier at a given reference block number.


Suggested change

/// @notice Returns the address for a cert verifier at a given reference block number.

/// @notice Returns the address for the active cert verifier at a given reference block number.

Prob need to mention the conditions around this. Does every RBN always return an address? Even RBN=0? Why? What about RBN=super-far-block-in-the-future? Or should this only be called with an RBN that is <= current block?

I personally think that's a bit TMI. If a reader is curious about the edge cases, they can simply read the very short router code and the comments within. But generally, I don't believe general integrators would care about these details since the router handles edge cases already by being initialized with a cert verifier.

I think that's completely wrong. This is just basic information that any person calling this function should know. I was wondering myself while reading this. And in fact because of this querying I realized that there was a bug where we were allowing people to call this function with an RBN in the future.

(bug being #1518 (comment))

samlaf · 2025-05-16T13:58:04Z

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

+    function getCertVerifierAt(uint32 referenceBlockNumber) public view returns (address) {
+        return certVerifiers[_findPrecedingRegisteredABN(referenceBlockNumber)];


should this not revert if RBN > block.number?

I think we should not introduce this in this PR because the existing cert verifier does not do this check either. And the block number check seems like something that should be happening in the verifier itself, not in the router. Hence, this suggestion IMO is a suggestion of a change in verification logic

Not sure I'm following. Where/how does the block number check belong to verifier logic? Its purely for determining which UNIQUE certVerifier NEEDS to attest to the validity of a given cert. Without this check then the validity of a cert can change over time right? First call router with a block BN in the future - cert is valid - then a new cert verifier is activated at the block BN-1. Then check again at block BN - cert is invalid.

samlaf · 2025-05-16T14:00:47Z

contracts/src/periphery/cert/interfaces/IEigenDACertVerifierRouter.sol

+
+import {IEigenDACertVerifierBase} from "src/periphery/cert/interfaces/IEigenDACertVerifierBase.sol";
+
+interface IEigenDACertVerifierRouter is IEigenDACertVerifierBase {


Add comment describing what the purpose of this router is: proxy that holds access to past implementations, and where new implementations can only be added in the future, etc.

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

samlaf · 2025-05-16T14:08:56Z

contracts/src/periphery/cert/libraries/EigenDACertVerificationLib.sol

+     * @return errParams Additional error parameters
+     * @return confirmedQuorumsBitmap The bitmap of confirmed quorums
+     */
+    function checkSignaturesAndBuildConfirmedQuorums(


Why does this say signatureS (with an s)? We're only checking a single (aggregate) signature right?

It is the same nomenclature as the underlying signature verifier though.

samlaf · 2025-05-16T14:17:20Z

contracts/src/periphery/cert/libraries/EigenDACertVerificationLib.sol

+    function checkBlobQuorumsSubset(bytes memory blobQuorumNumbers, uint256 confirmedQuorumsBitmap)
+        internal
+        pure
+        returns (StatusCode err, bytes memory errParams, uint256 blobQuorumsBitmap)
+    {
+        blobQuorumsBitmap = BitmapUtils.orderedBytesArrayToBitmap(blobQuorumNumbers);
+
+        if (BitmapUtils.isSubsetOf(blobQuorumsBitmap, confirmedQuorumsBitmap)) {
+            return (StatusCode.SUCCESS, "", blobQuorumsBitmap);
+        } else {
+            return (StatusCode.BLOB_QUORUMS_NOT_SUBSET, abi.encode(blobQuorumsBitmap, confirmedQuorumsBitmap), 0);
+        }


why don't you compute the blobQuorumsBitmap outside and then just pass both bitmaps to this function? Feels a lot more natural than having this function return a blobQuorumsBitmap, which shouldnt have anything to do with checking blobQuorumsSubset.

I think this is out of scope of this PR since this is the same logic that was in the cert verifier before.

You've literally complained about every single thing in our current contracts' implementations though. So why does this one fall outside the purview of the refactor to make this contract more readable/understandable?

My POV is that these PRs are the only chance for reviewers to actually read the code and catch things like this. If we don't do them now these minor improvements never happen and the complexity overflows to our customers who have to read this difficult to understand code. And minor improvements compound (positively!) so I do think we should include small changes that require few lines of code to change (of course I agree things that require 50+ lines of code change should be done in separate PRs).

samlaf · 2025-05-16T14:26:27Z

contracts/src/periphery/cert/libraries/EigenDACertVerificationLib.sol

+        SUCCESS, // Verification succeeded
+        INVALID_INCLUSION_PROOF, // Merkle inclusion proof is invalid
+        SECURITY_ASSUMPTIONS_NOT_MET, // Security assumptions not met
+        BLOB_QUORUMS_NOT_SUBSET, // Blob quorums not a subset of confirmed quorums


is "confirmedQuorum" something used outside of this contract? Or is it a term you invented? I think "batchSignedQuorums" might make more sense? Otherwise its unclear what/how they got confirmed, and what confirmed means

It was the nomenclature of the verification logic previously. I did not invent it.

contracts/src/periphery/cert/legacy/IEigenDACertVerifierLegacy.sol

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

contracts/src/periphery/cert/EigenDACertVerifier.sol

samlaf

Blocking this PR because I'm pretty unhappy with your comments addressal.

Main issue is that I think there's still a bug in the implementation: #1518 (comment)
Bug which you've completely dismissed #1518 (comment)
I might be completely wrong here, but it seems like you just dismissed this without giving it a second's thought.

In order to get this merge and keep the ball moving, I'm happy to merge after the above is fixed (unless I'm completely wrong and the bug isnt actually a bug). But I will add that I expect more of you. You haven't given commit links to addressals, so its very hard to re-review. Plus you've thumbs upped some of my requests without actually addressing them, so I have to go check one by one what is done and what is not... and I generally don't feel confident with this PR because of the bug found above. You also very easily dismiss some of my comments for better code quality like here, despite constantly arguing that our contract code is subpar. The CertVerifierV3 is supposed to be a clean slate, so I don't see why keeping around old shitty code is a valuable strategy when the refactor being asked is very simple and makes for much more readable code.

Dismissing my previous "requested changes" review because I'm going to bed but would like this to get merged today if possible.

We synced offline, and requires changes before merging are:

adding check in router that RBN cannot be in future
adding IEigenDACertVerifier interface

pakim249CAL · 2025-05-19T21:09:03Z

Adding cert verifier in future 41a28c4

ICertVerifier interface e3aa022

@samlaf @litt3 @ethenotethan

samlaf

LGTM

samlaf · 2025-05-20T12:33:33Z

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

+        // We disallow adding cert verifiers at the current block number to avoid a race condition of
+        // adding a cert verifier at the current block and verifying in the same block
+        if (activationBlockNumber <= block.number) {


this is actually a great catch! We want cert verification at a block number to be deterministic. That can only be the case if block numbers are activated as a precursor to a given block's execution, which is exactly what our current design does now.

Otherwise you can have in a single block:

tx1 verifies against certVerifier N

certVerifier N+1 gets activated

tx2 verifies same cert as tx1 but against certVerifier N+1

Right! I think these checks really need to be specified up front in a spec because it is hard to know whether this is even an invariant I want to protect without that. We should not just add require statements ad hoc because we can miss interactions across different functions.

litt3 · 2025-05-20T14:11:46Z

contracts/src/periphery/cert/interfaces/IEigenDACertVerifier.sol

+
+import {EigenDATypesV1 as DATypesV1} from "src/core/libraries/v1/EigenDATypesV1.sol";
+
+interface IEigenDACertVerifier is IEigenDACertVerifierBase, IVersionedEigenDACertVerifier {


I don't think we should delay any further, but this doesn't match what we had discussed with the interface structure

I think we should do a fast follow to make it match

cc @pakim249CAL @samlaf

Sorry I didn't loop you into this convo but I brought up a concern with sam that there didn't seem to be a good place to put an IEigenDACertVerifierV3 interface. It seemed better to name it IEigenDACertVerifier instead, and as they become deprecated we can add the version number and dump it into legacy. This is because we don't want to go back to the version numbers in the main folder for periphery, and there should only be one canonical interface that outside integrators should need to refer to. Version numbers would be confusing and would bring questions about which versions they should use.

My only issue with "legacy" is it sounds like we no longer support it, whereas our clients and proxy need to be backward compatible with ALL versions >= 3 going forward, because we HAVE to assume some customer is running these versions in prod somewhere, and make sure to not break any of them. Something like "LTS" name makes more sense than "legacy" to me. But we can figure out the proper naming and place to put them later.

Refactor: Consolidate struct types into versioned libraries

d93e21e

fix: remove operator state retriever change fix: readd v1 test

pakim249CAL force-pushed the refactor/version-contract-types branch from 9b31539 to d93e21e Compare April 30, 2025 22:56

pakim249CAL added 2 commits April 30, 2025 23:07

fix: fmt

7ce68c3

feat: cert verifier v3

4d17389

pakim249CAL force-pushed the feat/cert-verifier-v3 branch from 3ec0ab9 to 4d17389 Compare April 30, 2025 23:22

pakim249CAL added 5 commits May 1, 2025 00:57

fix: ci

7757b99

fix: ci

b3fe8c1

fix: remove certverifier binding

52d12dc

fix: ci

0985b47

fix: ci

5be5313

samlaf reviewed May 1, 2025

View reviewed changes

pakim249CAL added 3 commits May 1, 2025 20:50

feat: new file structure

2957935

feat: remove old files refactor: file layout

fix: ci

f451cb7

fix: ci

0dd14a2

ethenotethan reviewed May 2, 2025

View reviewed changes

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

contracts/src/periphery/EigenDACertVerifierRouter.sol Outdated Show resolved Hide resolved

pakim249CAL added 4 commits May 2, 2025 14:52

Merge branch 'refactor/version-contract-types' into feat/cert-verifie…

5ef491b

…r-v3

Merge remote-tracking branch 'origin/master' into refactor/version-co…

b964301

…ntract-types

fix: bindings

f597d07

Merge branch 'refactor/version-contract-types' into feat/cert-verifie…

8f59b00

…r-v3

pakim249CAL marked this pull request as ready for review May 2, 2025 15:57

ethenotethan reviewed May 2, 2025

View reviewed changes

pakim249CAL and others added 8 commits May 2, 2025 17:33

feat: add dummy interface

7e26828

chore: address comments

c1b9fdb

Merge branch 'refactor/version-contract-types' into feat/cert-verifie…

c2db1b4

…r-v3

chore: address comments

d4d304f

chore: add comment explaining ordering of fields, fix rbn position

c9a5699

fix: field position

35cd63f

fix: encoding

887ffae

Update contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol

6cbf4be

Co-authored-by: ethenotethan <[email protected]>

ethenotethan reviewed May 5, 2025

View reviewed changes

pakim249CAL commented May 15, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into feat/cert-verifier-v3

59db366

pakim249CAL requested a review from ethenotethan May 15, 2025 19:16

pakim249CAL marked this pull request as ready for review May 15, 2025 19:16

fix: ci

8bf0a01

ethenotethan previously approved these changes May 15, 2025

View reviewed changes

pakim249CAL dismissed litt3’s stale review via 8bf0a01 May 15, 2025 22:07

litt3 reviewed May 16, 2025

View reviewed changes

samlaf reviewed May 16, 2025

View reviewed changes

litt3 reviewed May 16, 2025

View reviewed changes

contracts/src/periphery/cert/legacy/IEigenDACertVerifierLegacy.sol Show resolved Hide resolved

litt3 reviewed May 16, 2025

View reviewed changes

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol Show resolved Hide resolved

contracts/src/periphery/cert/router/EigenDACertVerifierRouter.sol Show resolved Hide resolved

chore: address comments, add test

61ce636

pakim249CAL dismissed ethenotethan’s stale review via 61ce636 May 16, 2025 19:37

fix: remove mistakenly added file

296f23c

litt3 reviewed May 16, 2025

View reviewed changes

contracts/src/periphery/cert/EigenDACertVerifier.sol Outdated Show resolved Hide resolved

litt3 previously approved these changes May 19, 2025

View reviewed changes

samlaf previously requested changes May 19, 2025

View reviewed changes

fix: enforce abn in future when adding cert verifier

41a28c4

pakim249CAL dismissed litt3’s stale review via 41a28c4 May 19, 2025 20:06

feat: add eigenda cert verifier interface

e3aa022

pakim249CAL requested review from samlaf, litt3 and ethenotethan May 19, 2025 21:06

samlaf approved these changes May 20, 2025

View reviewed changes

litt3 approved these changes May 20, 2025

View reviewed changes

litt3 reviewed May 20, 2025

View reviewed changes

pakim249CAL merged commit dbc01a2 into master May 20, 2025
13 checks passed

pakim249CAL deleted the feat/cert-verifier-v3 branch May 20, 2025 18:42

		/// @return The EigenDA signature verifier contract.
		function eigenDASignatureVerifier() external view returns (IEigenDASignatureVerifier);

	/// @notice Returns the address for a cert verifier at a given reference block number.
	/// @notice Returns the address for the active cert verifier at a given reference block number.

		function getCertVerifierAt(uint32 referenceBlockNumber) public view returns (address) {
		return certVerifiers[_findPrecedingRegisteredABN(referenceBlockNumber)];


		import {IEigenDACertVerifierBase} from "src/periphery/cert/interfaces/IEigenDACertVerifierBase.sol";

		interface IEigenDACertVerifierRouter is IEigenDACertVerifierBase {


		import {EigenDATypesV1 as DATypesV1} from "src/core/libraries/v1/EigenDATypesV1.sol";

		interface IEigenDACertVerifier is IEigenDACertVerifierBase, IVersionedEigenDACertVerifier {

feat: cert verifier v3 + router #1518

feat: cert verifier v3 + router #1518

Uh oh!

Conversation

pakim249CAL commented Apr 30, 2025 • edited by samlaf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Checks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethenotethan left a comment

Choose a reason for hiding this comment

Uh oh!

litt3 May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakim249CAL May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pakim249CAL commented Apr 30, 2025 •

edited by samlaf

Loading

litt3 May 16, 2025 •

edited

Loading

pakim249CAL May 16, 2025 •

edited

Loading

pakim249CAL May 16, 2025 •

edited

Loading