feat: sgx verifier #58
Conversation
There was a problem hiding this comment.
How will we deal with protocol upgrades (multiple batch versions)?
Will we deploy multiple instances of SGXVerifier with different attestationVerifier contracts? Or will they reuse the same attestationVerifier?
There was a problem hiding this comment.
We will have a MultipleVersionRollupVerifier contract deployed only for SGXVerifier and the version managed in this contract. So each time we upgrade batch version, both MultipleVersionRollupVerifier for zkp and tee should be updated.
For different SGXVerifier, I believe we can share the same attestationVerifier.
There was a problem hiding this comment.
For different SGXVerifier, I believe we can share the same attestationVerifier.
I'm not sure. Basically we'd need batchVersion ==> mrEnclave mapping implemented by MultipleVersionRollupVerifier. (In practice it's batchVersion ==> sgxVerifier ==> attestationVerifier.)
There was a problem hiding this comment.
(In practice it's
batchVersion==>sgxVerifier==>attestationVerifier.)
true, we can decide it later. It is only related to deployment.
src/L1/rollup/ScrollChain.sol
Outdated
| // Pop finalized and non-skipped message from L1MessageQueue. | ||
| _finalizePoppedL1Messages(_totalL1MessagesPoppedOverall); | ||
|
|
||
| emit VerifyBatchWithTee(_batchIndex); | ||
| emit FinalizeBatch(_batchIndex, _batchHash, _postStateRoot, _withdrawRoot); |
There was a problem hiding this comment.
Just a note to self (and to @colinlyguo): This will keep the semantics of FinalizeBatch as before, but indexers might still need to be updated if they want to decode the transaction calldata. (Maybe only relevant for commit, not finalize.)
There was a problem hiding this comment.
chain-monitor (does not monitor batch finalize events, only monitors finalize deposit/withdraw events), bridge-history (withdraw proof), and l2geth-verifier (finalize batch verification) are compatible. I believe sync-from-DA feature is also compatible.
We can keep a close eye on monitoring these components after the upgrade as well.
src/L1/rollup/ScrollChain.sol
Outdated
| finalizedStateRoots[_batchIndex] = _postStateRoot; | ||
| withdrawRoots[_batchIndex] = _withdrawRoot; |
There was a problem hiding this comment.
These are not finalized at this point. Is there a risk that clients will incorrectly rely on unfinalized roots?
The question is: Should we prevent this programmatically / through renaming? Or should we just use documentation to communicate that clients must use lastFinalizedBatchIndex or isBatchFinalized before reading from finalizedStateRoots.
There was a problem hiding this comment.
We can override the function of finalizedStateRoots and withdrawRoots, to make it return bytes(0) for all indices after lastFinalizedBatchIndex.
There was a problem hiding this comment.
I choose to add comments about this. Overriding those two functions make the code more complex.
src/L1/rollup/ScrollChain.sol
Outdated
| emit ResolveState(state.batchIndex, state.stateRoot, state.withdrawRoot); | ||
|
|
||
| // reset zkp verified batch index, zk prover need to reprove everything after this batch | ||
| lastZkpVerifiedBatchIndex = state.batchIndex; |
There was a problem hiding this comment.
@colinlyguo any thoughts what we'd need to add to be able to recover from this relatively painlessly? Maybe just listen to ResolveState events in rollup-relayer and update db accordingly? (Though in this case most likely we'll need to fix either zk or sgx prover.)
There was a problem hiding this comment.
Maybe just listen to ResolveState events in rollup-relayer and update db accordingly
agree.
Though in this case most likely we'll need to fix either zk or sgx prover.
BeforeResolveState
This solution makes sense, and there are two cases that I can think of:
- if no need to fix zk prover or it has been fixed, then can rely on auto-recover of rollup-relayer.
- if we just ResolveState first then fix zk prover, then add a flag to halt finalization.
point me out if missing any corner cases.
There was a problem hiding this comment.
So with MockFinalize mode we'd still need to run an SGX prover?
There was a problem hiding this comment.
Yeah, running a SGX prover is cheap. I think it is ok to run it in sepolia.
There was a problem hiding this comment.
Since the verifier is set as address(0), does that mean that we just need someone to call finalizeBundleWithTeeProof (e.g. in rollup-relayer, when fake finalize mode is enabled), and the verifyBundleProof call will be a no-op?
There was a problem hiding this comment.
seems that I forget to change the constructor. The verifier should not be set as address(0). I did change ScrollChainMockBlob since it is used in unit tests. I'm thinking we should also add unit tests for ScrollChainMockFinalize.
There was a problem hiding this comment.
I see two options two make ScrollChainMockFinalize useful:
- Use
address(0)as verifier so that we bypass the whole verification step. Then,rollup-relayercan callfinalizeBundleWithTeeProofwhen fake finalization is enabled. - Keep the verifier, but somehow bypass
verifyAttestation, so that we can run the "SGX" prover on normal hardware. (Setting up SGX hardware is too complex for a devnet.)
There was a problem hiding this comment.
For option 2, we need test the feature in sepolia first before goes to mainnet. After test we can do anything we like, like bypass verifyAttestation or fake finalization on tee proof.
| /// @notice Update the bundle size | ||
| /// @param size The new bundle size. | ||
| /// @param batchIndex The start batch index for new bundle size. | ||
| function updateBundleSize(uint128 size, uint128 batchIndex) external onlyOwner { |
There was a problem hiding this comment.
Is there a risk that we (accidentally) set a very large index, and then we have no way to change this?
There was a problem hiding this comment.
ok, make the last item updatable.
| ); | ||
|
|
||
| // initialize the first element in the array | ||
| bundleSize.push(BundleSizeStruct(_bundleSize, uint128(cachedIndex))); |
There was a problem hiding this comment.
How will the upgrade happen? Basically right after executing the upgrade we'll need to reset all pending bundles in the rollup-relayer, right?
There was a problem hiding this comment.
add a reminder here that rollup-relayer should configure _bundleSize to be set and remove the timeout force building bundle feature.
or we can monitor:
InitializeBundleSize event -> (i) clear unfinalized bundles; (ii) update _bundleSize.
ChangeBundleSize event -> update _bundleSize. -> or use getBundleSizeGivenEndBatchIndex, it depends.
There was a problem hiding this comment.
Currently, the bundle size is about 100, right? We can use this for the rollup-relayer, and no need to do anything switch for rollup-relayer during upgrade.
There was a problem hiding this comment.
makes sense. it's 30, can change it to 100 in advance.
There was a problem hiding this comment.
added relevant tweaks of bundle-proposer in this PR: scroll-tech/scroll#1566
No description provided.