feat: sgx verifier #58

zimpha · 2024-10-29T13:52:54Z

No description provided.

src/L1/rollup/IScrollChain.sol

src/L1/rollup/ScrollChain.sol

src/sgx-verifier/AttestationVerifier.sol

src/sgx-verifier/SGXVerifier.sol

Thegaram · 2024-11-04T05:36:17Z

src/sgx-verifier/SGXVerifier.sol

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?

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.

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.)

(In practice it's batchVersion ==> sgxVerifier ==> attestationVerifier.)

true, we can decide it later. It is only related to deployment.

src/L1/rollup/IScrollChain.sol

Thegaram · 2024-11-05T13:55:41Z

src/L1/rollup/ScrollChain.sol

        // Pop finalized and non-skipped message from L1MessageQueue.
        _finalizePoppedL1Messages(_totalL1MessagesPoppedOverall);

+        emit VerifyBatchWithTee(_batchIndex);
        emit FinalizeBatch(_batchIndex, _batchHash, _postStateRoot, _withdrawRoot);


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.)

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.

Thegaram · 2024-11-05T13:57:56Z

src/L1/rollup/ScrollChain.sol

        finalizedStateRoots[_batchIndex] = _postStateRoot;
        withdrawRoots[_batchIndex] = _withdrawRoot;


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.

We can override the function of finalizedStateRoots and withdrawRoots, to make it return bytes(0) for all indices after lastFinalizedBatchIndex.

Thegaram · 2024-11-05T14:06:40Z

src/L1/rollup/ScrollChain.sol

+            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;


@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.)

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.
Before ResolveState

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 pls.

Thegaram · 2024-11-05T14:08:58Z

src/mocks/ScrollChainMockFinalize.sol

So with MockFinalize mode we'd still need to run an SGX prover?

Yeah, running a SGX prover is cheap. I think it is ok to run it in sepolia.

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?

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.

I see two options two make ScrollChainMockFinalize useful:

Use address(0) as verifier so that we bypass the whole verification step. Then, rollup-relayer can call finalizeBundleWithTeeProof when 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.)

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.

src/L1/rollup/ScrollChain.sol

colinlyguo · 2024-11-06T04:34:58Z

src/sgx-verifier/AttestationVerifier.sol

+import {BytesUtils} from "./utils/BytesUtils.sol";
+
+/// @dev This contract is modified from https://github.com/automata-network/scroll-prover/blob/demo/contracts/src/core/AttestationVerifier.sol
+contract AttestationVerifier is Ownable, IAttestationVerifier {


any doc about this verifier?

src/sgx-verifier/AttestationVerifier.sol

colinlyguo · 2024-11-06T05:10:39Z

src/L1/rollup/ScrollChain.sol

+    function initializeV2() external reinitializer(2) {
+        uint256 cachedIndex = lastZkpVerifiedBatchIndex;
+        lastTeeVerifiedBatchIndex = cachedIndex;
+        emit VerifyBatchWithTee(cachedIndex);


Would another event name to differentiate from real tee verify a better choice?

Thegaram · 2024-11-15T02:41:23Z

src/L1/rollup/IScrollChain.sol

+    );
+
+    /// @notice Emitted when a batch is verified by tee proof
+    /// @dev Tee proof always comes after zk proof. If they match, the state root and withdraw root are the same.


Suggested change

/// @dev Tee proof always comes after zk proof. If they match, the state root and withdraw root are the same.

Thegaram · 2024-11-15T02:42:57Z

src/L1/rollup/IScrollChain.sol

    /*************************
     * Public View Functions *
     *************************/

-    /// @return The latest finalized batch index.
+    /// @return The latest finalized batch index (both zkp and tee verified).


(both zkp and tee verified)

This is not always true, right?

Thegaram · 2024-11-15T04:58:49Z

src/L1/rollup/ScrollChain.sol

+
+    /// @notice The timestamp to delay proof when we only has one proof type.
+    /// @dev This is enabled after Euclid upgrade.
+    uint256 public proofDelayTimestamp;


Suggested change

uint256 public proofDelayTimestamp;

uint256 public emergencyFinalizationDelay;

The word "timestamp" is usually used for instants, not durations

Thegaram · 2024-11-15T07:11:34Z

src/L1/rollup/ScrollChain.sol

    /**********************
     * Internal Functions *
     **********************/

+    /// @dev Internal function to enable proof types.
+    /// @param mask The mask for new enabled proof types.
+    function _enableProofTypes(uint256 mask) internal {


Should require that mask <= 3

Thegaram · 2024-11-15T07:15:56Z

src/L1/rollup/ScrollChain.sol

@@ -172,6 +260,14 @@ contract ScrollChain is OwnableUpgradeable, PausableUpgradeable, IScrollChain {
        _;
    }

+    modifier whenFinalizeNotPaused(ProofType proofType) {


We should also consider paused here (operator pause) as a 3rd case

Thegaram · 2024-11-15T07:35:24Z

src/L1/rollup/ScrollChain.sol

+    /// @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 {


Is there a risk that we (accidentally) set a very large index, and then we have no way to change this?

Thegaram · 2024-11-15T16:09:59Z

src/L1/rollup/ScrollChain.sol

+        );
+
+        // initialize the first element in the array
+        bundleSize.push(BundleSizeStruct(_bundleSize, uint128(cachedIndex)));


How will the upgrade happen? Basically right after executing the upgrade we'll need to reset all pending bundles in the rollup-relayer, right?

feat: add draft codes for sgx verifier

3e19b5e

zimpha requested review from Thegaram, amoylan2 and spilehchiha October 29, 2024 13:52

zimpha self-assigned this Oct 29, 2024

feat: add comments and tweak implementation

42a514a

zimpha requested a review from icemelon November 1, 2024 06:22

zimpha added 2 commits November 1, 2024 15:05

feat: update unit tests for ScrollChain

e86a8bc

feat: update unit tests for SGXVerifier

82e17c6

Thegaram reviewed Nov 4, 2024

View reviewed changes

fix comments

65fbfca

zimpha requested a review from colinlyguo November 5, 2024 06:49

zimpha added 2 commits November 5, 2024 15:01

reset lastZkpVerifiedBatchIndex only useSGXState = true

dc6b99f

fix unit tests

acd550c

Thegaram previously approved these changes Nov 5, 2024

View reviewed changes

colinlyguo reviewed Nov 6, 2024

View reviewed changes

make tee proof and zk proof independent

566b9f0

zimpha dismissed Thegaram’s stale review via 566b9f0 November 14, 2024 08:46

Thegaram reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sgx verifier #58

feat: sgx verifier #58

zimpha commented Oct 29, 2024

Thegaram Nov 4, 2024

zimpha Nov 5, 2024

Thegaram Nov 5, 2024

zimpha Nov 6, 2024

Thegaram Nov 5, 2024

colinlyguo Nov 6, 2024

Thegaram Nov 5, 2024

zimpha Nov 6, 2024

Thegaram Nov 5, 2024

colinlyguo Nov 6, 2024 •

edited

Loading

Thegaram Nov 5, 2024

zimpha Nov 6, 2024

Thegaram Nov 6, 2024

zimpha Nov 6, 2024

Thegaram Nov 6, 2024

zimpha Nov 6, 2024

colinlyguo Nov 6, 2024

colinlyguo Nov 6, 2024

Thegaram Nov 15, 2024

Thegaram Nov 15, 2024

Thegaram Nov 15, 2024

Thegaram Nov 15, 2024

Thegaram Nov 15, 2024

Thegaram Nov 15, 2024

Thegaram Nov 15, 2024

		finalizedStateRoots[_batchIndex] = _postStateRoot;
		withdrawRoots[_batchIndex] = _withdrawRoot;

	uint256 public proofDelayTimestamp;
	uint256 public emergencyFinalizationDelay;

feat: sgx verifier #58

Are you sure you want to change the base?

feat: sgx verifier #58

Conversation

zimpha commented Oct 29, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinlyguo Nov 6, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinlyguo Nov 6, 2024 •

edited

Loading