Blob verification changes #321

0x0aa0 · 2024-03-06T22:15:06Z

Why are these changes needed?

We need to include canonical values for quorums and verify those values in our blob verification

Checks

I've made sure the lint is passing in this PR.
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.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

mooselumph · 2024-03-07T00:16:04Z

contracts/src/libraries/EigenDARollupUtils.sol

+            }
+
+            // make sure that the stake signed for is greater than the given confirmationThresholdPercentage
+            require(uint8(blobVerificationProof.batchMetadata.batchHeader.signedStakeForQuorums[uint8(blobVerificationProof.quorumThresholdIndexes[i])]) 


Should we rename quorumThresholdIndexes to just quorumIndices?

ian-shim · 2024-03-07T05:31:59Z

contracts/src/core/EigenDAServiceManagerStorage.sol

@@ -29,6 +25,12 @@ abstract contract EigenDAServiceManagerStorage is IEigenDAServiceManager {
     * have to serve after they've deregistered.
     */
    uint32 public constant BLOCK_STALE_MEASURE = 150;
+
+    /// @notice The quorum adversary threshold percentages
+    bytes public constant quorumAdversaryThresholdPercentages = hex"2121";


This looks very cryptic. Can we comment on what this means?

ian-shim · 2024-03-07T05:37:02Z

contracts/src/libraries/EigenDARollupUtils.sol

+        IEigenDAServiceManager eigenDAServiceManager,
+        uint256 quorumNumber
+    ) public view returns(uint8 adversaryThresholdPercentage) {
+        if(eigenDAServiceManager.quorumAdversaryThresholdPercentages().length > quorumNumber){


should it be >=?

should be > I think. If only quorum 0 and 1 are set we want to make sure those numbers are less than the length of the byte array (2)

- rename quorumIndices - more descriptive NATSPEC

siddimore · 2024-03-13T15:31:29Z

contracts/src/rollup/MockRollup.sol

-        require(msg.value == stakeRequired, "MockRollup.registerValidator: Must send stake required to register");
-        require(!validators[msg.sender], "MockRollup.registerValidator: Validator already registered");
-        require(!blacklist[msg.sender], "MockRollup.registerValidator: Validator blacklisted");
-        validators[msg.sender] = true;
    }



can anyone call mockrollup postcommitment?

siddimore · 2024-03-13T16:05:01Z

contracts/src/rollup/MockRollup.sol

-        require(validators[msg.sender], "MockRollup.postCommitment: Validator not registered");
-        require(commitments[block.timestamp].validator == address(0), "MockRollup.postCommitment: Commitment already posted");
+        // require commitment has not already been posted
+        require(commitments[block.timestamp].confirmer == address(0), "MockRollup.postCommitment: Commitment already posted");


should there be a check on who can call confirmer? or should the comment be not a confirmer?

I removed the check to simplify so that we can just use any address to verify the blobs

contracts/src/core/EigenDAServiceManagerStorage.sol

jianoaix · 2024-03-15T23:35:36Z

contracts/src/interfaces/IEigenDAServiceManager.sol

@@ -45,7 +45,7 @@ interface IEigenDAServiceManager is IServiceManager {
    struct BatchHeader {
        bytes32 blobHeadersRoot;
        bytes quorumNumbers; // each byte is a different quorum number
-        bytes quorumThresholdPercentages; // every bytes is an amount less than 100 specifying the percentage of stake 
+        bytes signedStakeForQuorums; // every bytes is an amount less than 100 specifying the percentage of stake 
                                          // the must have signed in the corresponding quorum in `quorumNumbers` 


"must have signed" -> "have actually signed"? The former is the threshold, whereas this looks the actual signed stake

jianoaix · 2024-03-15T23:39:31Z

contracts/src/libraries/EigenDARollupUtils.sol

+                ),
+                confirmedQuorumsBitmap
+            ),
+            "EigenDARollupUtils.verifyBlob: confirmed quorums are not a subset of the required quorums"


nit: error message is reversed, should be "required quorums are not a subset of the confirmed quorums"

jianoaix

Just some nits, lgtm otherwise

contracts/src/interfaces/IEigenDAServiceManager.sol

jianoaix · 2024-03-16T00:01:14Z

contracts/src/libraries/EigenDARollupUtils.sol

+     * @notice gets the adversary threshold percentage for a given quorum
+     * @param eigenDAServiceManager the contract in which the batch was confirmed 
+     * @param quorumNumber the quorum number to get the adversary threshold percentage for
+     * @dev returns 0 if the quorumNumber is not found


There is no clause setting to 0 below for invalid quorumNumber. Does it implicit do so in Sol? Is it a good practice to set it explicitly?

Yes because adversaryThresholdPercentage is defined as 0 by default with return it will return zero if a quorum is not found

0x0aa0 added 5 commits February 28, 2024 17:20

simplify blob verificaiton interface

9b62333

add quorum confirmation threshold

5846612

threshold values

e23a416

rename to confirmationThreshold

de26613

signedStakeForQuorums name change

a45d573

0x0aa0 requested review from mooselumph and ian-shim March 6, 2024 22:15

ian-shim requested a review from gpsanant March 6, 2024 23:50

mooselumph reviewed Mar 7, 2024

View reviewed changes

mooselumph approved these changes Mar 7, 2024

View reviewed changes

mooselumph self-requested a review March 7, 2024 00:19

ian-shim reviewed Mar 7, 2024

View reviewed changes

nits

425ecee

- rename quorumIndices - more descriptive NATSPEC

0x0aa0 requested a review from ian-shim March 7, 2024 15:17

simplify mock rollup (#338)

01e0078

siddimore reviewed Mar 13, 2024

View reviewed changes

feat: required quorums check (#337)

4fd381b

mooselumph approved these changes Mar 13, 2024

View reviewed changes

mooselumph and others added 11 commits March 13, 2024 16:12

Remove security parameters from API (#298)

e77e762

update latestServeUntilBlock (#344)

c5f5ea7

Add required quorums (#347)

e10147c

Consolidate submodules (#286)

a4e17cb

fix: DeployOpenEigenLayer

167a096

Fix tests

00f9428

Fix tests

a30f1ef

Fix quorum validation bug

ab1e30c

Merge branch 'master' into blob-verificaiton

557a79c

merge master

022719f

Merge fixes

39aefa3

jianoaix reviewed Mar 15, 2024

View reviewed changes

jianoaix reviewed Mar 16, 2024

View reviewed changes

0x0aa0 and others added 3 commits March 17, 2024 09:59

nits

a749f99

fix test

a33b415

Holesky preprod deployment (#354)

7878dbb

0x0aa0 requested a review from jianoaix March 17, 2024 23:38

Fix synthetic test client

4cb62d1

jianoaix approved these changes Mar 18, 2024

View reviewed changes

mooselumph merged commit 7bd41f9 into master Mar 18, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blob verification changes #321

Blob verification changes #321

0x0aa0 commented Mar 6, 2024 •

edited

Loading

mooselumph Mar 7, 2024

ian-shim Mar 7, 2024

ian-shim Mar 7, 2024

0x0aa0 Mar 7, 2024

siddimore Mar 13, 2024 •

edited

Loading

0x0aa0 Mar 13, 2024

siddimore Mar 13, 2024

0x0aa0 Mar 13, 2024

jianoaix Mar 15, 2024

jianoaix Mar 15, 2024

0x0aa0 Mar 17, 2024

jianoaix left a comment

jianoaix Mar 16, 2024

0x0aa0 Mar 17, 2024

Blob verification changes #321

Blob verification changes #321

Conversation

0x0aa0 commented Mar 6, 2024 • edited Loading

Why are these changes needed?

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddimore Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianoaix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x0aa0 commented Mar 6, 2024 •

edited

Loading

siddimore Mar 13, 2024 •

edited

Loading