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

Flag guard the dual quorum in disperser #389

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

jianoaix
Copy link
Contributor

@jianoaix jianoaix commented Mar 25, 2024

Why are these changes needed?

Use flag guard the dual quorum feature in offchain system, so it can be rolled out in a controlled way.

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

@jianoaix jianoaix requested review from mooselumph and 0x0aa0 March 25, 2024 02:44
@jianoaix
Copy link
Contributor Author

@0x0aa0 we may need a separate blob verification onchain for this.

<*rpc.jsonError | 0xc0006d6ba0>: 
      execution reverted: EigenDARollupUtils.verifyBlob: required quorums are not a subset of the confirmed quorums
      {
          Code: 3,
          Message: "execution reverted: EigenDARollupUtils.verifyBlob: required quorums are not a subset of the confirmed quorums",
          Data: <string>"0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000005[94](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?pr=389#step:16:95)56[96](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?
```pr=389#step:16:97)7656e4441526f6c6c75705574696c732e766572696679426c6f623a20726571756[97](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?pr=389#step:16:98)265642071756f72756d7320617265206e6f74206120737562736574206f662074686520636f6e6669726d65642071756f72756d7300000000000000",
      }
  to be nil

@ian-shim
Copy link
Contributor

@0x0aa0 we may need a separate blob verification onchain for this.

<*rpc.jsonError | 0xc0006d6ba0>: 
      execution reverted: EigenDARollupUtils.verifyBlob: required quorums are not a subset of the confirmed quorums
      {
          Code: 3,
          Message: "execution reverted: EigenDARollupUtils.verifyBlob: required quorums are not a subset of the confirmed quorums",
          Data: <string>"0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000005[94](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?pr=389#step:16:95)56[96](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?
```pr=389#step:16:97)7656e4441526f6c6c75705574696c732e766572696679426c6f623a20726571756[97](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?pr=389#step:16:98)265642071756f72756d7320617265206e6f74206120737562736574206f662074686520636f6e6669726d65642071756f72756d7300000000000000",
      }
  to be nil

The test should pass if you enable dual quorum for the integration test

@mooselumph
Copy link
Collaborator

mooselumph commented Mar 25, 2024

I don't really understand why we need this. I thought the point of putting the required quorums onto the chain was to make this automatic. When we're ready for the dual quorum, wouldn't we just make the contract upgrade then?

@jianoaix
Copy link
Contributor Author

@0x0aa0 we may need a separate blob verification onchain for this.

<*rpc.jsonError | 0xc0006d6ba0>: 
      execution reverted: EigenDARollupUtils.verifyBlob: required quorums are not a subset of the confirmed quorums
      {
          Code: 3,
          Message: "execution reverted: EigenDARollupUtils.verifyBlob: required quorums are not a subset of the confirmed quorums",
          Data: <string>"0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000005[94](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?pr=389#step:16:95)56[96](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?
```pr=389#step:16:97)7656e4441526f6c6c75705574696c732e766572696679426c6f623a20726571756[97](https://github.com/Layr-Labs/eigenda/actions/runs/8414425031/job/23038077048?pr=389#step:16:98)265642071756f72756d7320617265206e6f74206120737562736574206f662074686520636f6e6669726d65642071756f72756d7300000000000000",
      }
  to be nil

The test should pass if you enable dual quorum for the integration test

The point is not to trick the test. The different path should also be tested.

@jianoaix
Copy link
Contributor Author

I don't really understand why we need this. I thought the point of putting the required quorums onto the chain was to make this automatic. When we're ready for the dual quorum, wouldn't we just make the contract upgrade then?

It's about which way is easier, and it look flipping flags is easier than contract

@mooselumph
Copy link
Collaborator

I don't really understand why we need this. I thought the point of putting the required quorums onto the chain was to make this automatic. When we're ready for the dual quorum, wouldn't we just make the contract upgrade then?

It's about which way is easier, and it look flipping flags is easier than contract

I see.

One thing to note is that any rollup integrations will likely not function with required_quorums set on the contracts but not respected by the disperser since the rollup utils library will require these quorums to be present. So if we plan to be testing with rollup integrations we'll need to be using the contract upgrade path.

@jianoaix
Copy link
Contributor Author

I don't really understand why we need this. I thought the point of putting the required quorums onto the chain was to make this automatic. When we're ready for the dual quorum, wouldn't we just make the contract upgrade then?

It's about which way is easier, and it look flipping flags is easier than contract

I see.

One thing to note is that any rollup integrations will likely not function with required_quorums set on the contracts but not respected by the disperser since the rollup utils library will require these quorums to be present. So if we plan to be testing with rollup integrations we'll need to be using the contract upgrade path.

Yep, when this flag is off (set to false), the rollup is only partially function (i.e. can confirm batch, but cannot verify blobs), until this flag is enabled. cc @0x0aa0

@jianoaix
Copy link
Contributor Author

I think @0x0aa0 is making an alternative mock rollup and that will be ready a bit later. So for this PR, I'll just enable it in inabox to unbreak the test and move forward.

@jianoaix jianoaix merged commit 292e203 into Layr-Labs:master Mar 25, 2024
5 checks passed
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.

3 participants