feat(protocol): Enable protocol to handle multiple proof types #13745

adaki2004 · 2023-05-12T09:48:08Z

Related to: #13731

Code changes required to verify multiple proofs
Code changes required to overwrite unverified fork choice
Test changes of multiple proofs (ZKP & SGX) + overwrite unverified FK
Adjust other tests to changes

Brechtpd

Looking good!

Brechtpd · 2023-05-12T10:26:17Z

packages/protocol/contracts/L1/TaikoData.sol

+        // RESERVED_Y_ONLY,     // 0000 1000
+        // ZKP_AND_SGX,         // 0000 0011
+        // X_ZKP_SGX,           // 0000 0111
+        uint8 proofTypeEnabled;


Nice! Maybe a more clear name would be proofToggleMask or something.

Brechtpd · 2023-05-12T10:27:59Z

packages/protocol/contracts/L1/libs/LibProving.sol

-                        )
+        // config.proofTypeEnabled can support 8 different proofs on 8 bits
+        for (uint8 index; index < 8; ) {
+            if (index == 0 && ((config.proofTypeEnabled >> index) & 1) == 1) {


Suggested change

if (index == 0 && ((config.proofTypeEnabled >> index) & 1) == 1) {

if (((config.proofTypeEnabled >> index) & 1) == 1) {

if (index == 0) {

// ...

} else if (index == 1) {

// ...

}

Brechtpd · 2023-05-12T10:33:06Z

packages/protocol/contracts/L1/libs/LibProving.sol

-                            "signal_service",
-                            false
-                        )
+        // config.proofTypeEnabled can support 8 different proofs on 8 bits


The plan is still to abstract the actual implementations away in the verifier, or do you think a simple for loop with some ifs would also work? Seems okay to me like this, but I'm mostly concerned about the additional data needed for the different proof types, having a separate contract for each allows things to be very flexible with what a new proof type needs

Yep, agree, thanks for the remark! Outsourced to new files now.

- delete systemProofCoolDown config param - rename oracle test and add new test case checking the signature check logic

dantaik · 2023-05-15T05:23:55Z

packages/protocol/contracts/L1/libs/proofTypes/LibVerifySGX.sol

+library LibVerifySGX {
+    error L1_INVALID_SGX_SIGNATURE();
+
+    function verifySgxProof(


I think we can also use this library for non-sgx signers, so I suggested to change the name to LibVerifyTrusted

dantaik · 2023-05-15T05:24:54Z

packages/protocol/contracts/L1/libs/proofTypes/LibVerifySGX.sol

+library LibVerifySGX {
+    error L1_INVALID_SGX_SIGNATURE();
+
+    function verifySgxProof(


rename to verifyProof?

dantaik · 2023-05-15T05:24:58Z

packages/protocol/contracts/L1/libs/proofTypes/LibVerifyZKP.sol

+library LibVerifyZKP {
+    error L1_INVALID_PROOF();
+
+    function verifyZkProof(


rename to verifyProof?

dantaik · 2023-05-15T05:29:05Z

packages/protocol/contracts/L1/TaikoData.sol

@@ -74,6 +81,11 @@ library TaikoData {
        TaikoData.EthDeposit[] depositsProcessed;
    }

+    struct BlockProof {


How about we added a verifierType here?
uint16 proofType,

dantaik · 2023-05-15T05:42:47Z

packages/protocol/contracts/L1/libs/LibProving.sol

+        }
+
+        // config.proofToggleMask can support 8 different proof types on 8 bits
+        for (uint8 index; index < 8; ) {


Should we do something like this?

uint8 mask = config.proofToggleMask; for (uint i = 0; i < blockProofs.length; i++) { TypedProof memory proof = blockProofs[i]; uint8 bitMask = 1 << (proof.proofType - 1); require(mask & bitMask) != 0, "type_not_enabled_or_duplicated"); _verifyTypedProof(proof); mask &= ~bitMask; } require(mask == 0, "not_all_requierd_proof_verified");

I also suggest that the cooldown period is a value returned from _verifyTypedProof:

uint8 mask = config.proofToggleMask; uint64 maxCoolDownPeriod; for (uint i = 0; i < blockProofs.length; i++) { TypedProof memory proof = blockProofs[i]; uint8 bitMask = 1 << (proof.proofType - 1); require(mask & bitMask) != 0, "type_not_enabled_or_duplicated"); uint64 coolDownPeriod = _verifyTypedProof(proof); maxCoolDownPeriod = maxCoolDownPeriod.max(coolDownPeriod); mask &= ~bitMask; } require(mask == 0, "not_all_requierd_proof_verified"); fc.provenAt = uint64(block.timestamp +maxCoolDownPeriod );

Inside, LibVerifyerSGX and LibVerifyingZKP, in the verify function, we also return a cooldown period. This allows for different cooldown period for different verifiers, then we just take the max.

Question to this cooldown period: Since this affects proofTime = provenAt - proposedAt and cooldown only delays verification, why should not we let the the LibVerifying.sol do this job and revert ?

Inside, LibVerifyerSGX and LibVerifyingZKP, in the verify function, we also return a cooldown period. This allows for different cooldown period for different verifiers, then we just take the max.

Maybe min makes more sense otherwise adding a proof type can increase the cooldown, although cooldown period length will always a bit tricky and handwavy and not sure if something that can be decided per proof type.

Question to this cooldown period: Since this affects proofTime = provenAt - proposedAt and cooldown only delays verification, why should not we let the the LibVerifying.sol do this job and revert ?

What do you mean with revert?

What do you mean with revert?

Simply break/revert, not allow to be verified.

dantaik · 2023-05-15T05:43:37Z

packages/protocol/contracts/L1/TaikoData.sol

@@ -74,6 +81,11 @@ library TaikoData {
        TaikoData.EthDeposit[] depositsProcessed;
    }

+    struct BlockProof {


BlockProof => TypedProof

dantaik · 2023-05-15T05:45:17Z

packages/protocol/contracts/L1/TaikoData.sol

+        // RESERVED_Y_ONLY,     // 0000 1000
+        // ZKP_AND_SGX,         // 0000 0011
+        // X_ZKP_SGX,           // 0000 0111
+        uint8 proofToggleMask;


dantaik · 2023-05-15T05:55:34Z

I think this PR still has a high level design issue: the proofToggleMask can only specify an AND logics, but not something like: *if a valid SGX proof is given, then ZKP is not necessary; *.

dantaik · 2023-05-15T05:56:50Z

Need to remvoe ""oracle_prover" and "system_prover" everywhere.

adaki2004 · 2023-05-15T08:17:51Z

I think this PR still has a high level design issue: the proofToggleMask can only specify an AND logics, but not something like: *if a valid SGX proof is given, then ZKP is not necessary; *.

I guess this is a 'special proving scenario' only available if we explicitly set so (in the config). Then
if (proofToggleMask == type(uint16).max) could be one.
Let me know if it works with you. I guess if this is the only special scenario we want to do support (like the system_proof previously) this could work well.

Brechtpd · 2023-05-15T10:15:50Z

I think this PR still has a high level design issue: the proofToggleMask can only specify an AND logics, but not something like: *if a valid SGX proof is given, then ZKP is not necessary; *.

Could you give a scenario where we would use conditional logic like that? I saw this only really as an all proofs that are enabled are required thing.

I could see some benefit of the different proofs coming in at different times (like SGX proof first, which would set the cooldown period still pretty high, then a zkp proof that lowers it) but in practice I think probably doesn't really make a difference (we'll always want the zkp anyway, which is the slowest, and all of them being required all the time is best for security), and complicates things quite a bit like the fee payments.

We could still implement something like this in a verifier contract, the main protocol would would just require a single proof related to that verifier contract, but internally the verifier contract can do whatever it wants so could contain multiple sub-proofs with some special logic. This would actually be a pretty nice way to implement the testnet scenario where we want to skip some ZKP proofs, then there's no need to build this into the core protocol.

So maybe let's simplify the core protocol as much as possible and only let it support a single "proof" (as in some data and a verifier contract to call), then let the verifier contract handle the rest.

adaki2004 · 2023-05-18T11:45:29Z

@Brechtpd @dantaik
Thinking out loud, shouldn't we branch off from current main - call it alpha4 - and then every protocol change could go there from now on ?

Brechtpd · 2023-05-18T16:57:20Z

Probably makes more sense to do the opposite which I think is what we already do, make a release for alpha-n, and keep working on main.

adaki2004 · 2023-05-22T09:30:08Z

Will incorporate #13777 into this PR as well!

adaki2004 · 2023-05-23T07:17:14Z

packages/protocol/contracts/L1/libs/LibProving.sol

+    function setForkChoice(
+        TaikoData.State storage state,
+        TaikoData.Config memory config,
+        AddressResolver resolver,
+        uint256 blockId,
+        TaikoData.BlockEvidence memory evidence


Put back this functionality into proveBlock()

Hmmm why? The functionality of this is significantly different from proveBlock, which made proveBlock really complicated before.

adaki2004 · 2023-05-23T07:36:58Z

packages/protocol/contracts/L1/TaikoData.sol

+        // RESERVED_Y_ONLY,     // 0000 1000
+        // ZKP_AND_SGX,         // 0000 0011
+        // X_ZKP_SGX,           // 0000 0111
+        uint16 proofToggleMask;


Maybe have it static 2 proofs now - instead of a bitmap. Future updates will handle rest.

davidtaikocha · 2023-05-23T09:20:40Z

packages/protocol/contracts/L1/libs/proofTypes/LibVerifyZKP.sol

+            .resolve(LibUtils.getVerifierName(verifierId), false)
+            .staticcall(bytes.concat(inputHash, proof));
+
+        if (!verified || ret.length != 32 || bytes32(ret) != keccak256("taiko"))


Shall we define keccak256("taiko") as a library constant to save some gas?

Yep, good one ! Thanks David.

- Get rid of toggle mask and restrict to ZKP + SGX only (in case latter is enabled) - Removed setForkChoice() function and its logis is incorporated to proveBlock() hance function signature/interface is the same - Added SGX enable flag - Removed unnecessary vars (like proofType) because the static position within the array will determine if ZKP or SGX - Removed unused errors - Removed irrelevant tests

adaki2004 · 2023-05-23T13:02:57Z

@dantaik
Incorporated all the changes we discussed this morning.

(Latest commit changes):

Get rid of toggle mask and restrict to ZKP + SGX (only in case it is enabled)
Removed setForkChoice() function and incorporated the (Super Prover) into proveBlock() since sharing the same function signature/interface
Added SGX enable flag
Removed unnecessary vars (like proofType) because the static position in the array will determine if ZKP or SGX (we can save some gas here)
Removed unused errors
Removed irrelevant tests

Now we have 2 options to prove blocks.
A: ZKP + SGX (if enabled, or ZKP only in case we dont enable SGX) and those have to come together
B: There is a SuperProver which can write the proof

Gas comparison:
Old (but still with SGX): 36.805 gwei on average

New: 34.143 gwei

If I modify the code in a way that inline both the code of the LibVerifyZKP.verifyProof and LibVerifyTrusted.verifyProof we could save approx. 94gwei, but i thought does not worth the effort vs. maintenance (readability) later on.

dantaik · 2023-07-01T02:18:20Z

@adaki2004 given that our core protocol and tokenomics have changed a lot, shall we close this PR then re-implement the idea later?

adaki2004 · 2023-07-01T06:20:50Z

Yes,im closing it (for now) without deletion.

Enable protocol to handle multiple proof types

60ef24b

adaki2004 changed the title ~~Enable protocol to handle multiple proof types~~ feat(protocol): Enable protocol to handle multiple proof types May 12, 2023

Brechtpd reviewed May 12, 2023

View reviewed changes

adaki2004 added 2 commits May 12, 2023 14:16

Outsouce proof verification logic

9fa1641

Remove unnecessary imports

920677b

vercel bot deployed to Preview – bridge-ui May 12, 2023 12:20 View deployment

vercel bot deployed to Preview – geth-diff May 12, 2023 12:21 View deployment

Rename custom error

feeca2b

vercel bot deployed to Preview – bridge-ui May 12, 2023 12:44 View deployment

vercel bot deployed to Preview – geth-diff May 12, 2023 12:44 View deployment

Changes:

3ac6a30

- delete systemProofCoolDown config param - rename oracle test and add new test case checking the signature check logic

vercel bot deployed to Preview – bridge-ui May 12, 2023 17:01 View deployment

vercel bot deployed to Preview – geth-diff May 12, 2023 17:01 View deployment

Brechtpd mentioned this pull request May 13, 2023

feat(prover): only submit an oracle proof when there is a new invalid normal proof taikoxyz/taiko-client#222

Closed

dantaik reviewed May 15, 2023

View reviewed changes

PR changes

3a1ad2c

dantaik linked an issue May 16, 2023 that may be closed by this pull request

feat(protocol): Even more basic multi-prover with PoA #13731

Closed

dantaik added area.protocol meta.alpha-4 labels May 16, 2023

dantaik assigned adaki2004 May 16, 2023

dantaik added category.feature status.in-progress labels May 16, 2023

Merge main into feature

31d5539

vercel bot deployed to Preview – bridge-ui May 17, 2023 08:17 View deployment

vercel bot deployed to Preview – status-page May 17, 2023 08:17 View deployment

vercel bot deployed to Preview – website May 17, 2023 08:18 View deployment

vercel bot deployed to Preview – website May 18, 2023 11:35 View deployment

adaki2004 marked this pull request as ready for review May 18, 2023 11:37

adaki2004 requested review from Brechtpd, dantaik and davidtaikocha May 18, 2023 11:37

Remove line break

f3f5398

adaki2004 mentioned this pull request May 22, 2023

feat(protocol): Separate proof verification to a standalone, deployed contract #13794

Closed

adaki2004 commented May 23, 2023

View reviewed changes

Merge main into multi_prover_possibility

083f995

vercel bot deployed to Preview – website May 23, 2023 08:35 View deployment

adaki2004 and others added 2 commits May 23, 2023 10:45

Adjust test and indentation

a807dee

Add auto-generated contract documentation

6f6c848

vercel bot deployed to Preview – website May 23, 2023 08:52 View deployment

davidtaikocha reviewed May 23, 2023

View reviewed changes

Merge branch 'main' into multi_prover_possibility

72e5901

adaki2004 mentioned this pull request May 23, 2023

refactor(protocol): add typed proof #13804

Closed

adaki2004 and others added 2 commits May 23, 2023 14:56

Add auto-generated contract documentation

9fc7f27

vercel bot deployed to Preview – website May 23, 2023 13:04 View deployment

adaki2004 mentioned this pull request May 24, 2023

feat: Separate Tokenomics from Core Protocol to a Controller Contract #13791

Closed

dantaik marked this pull request as draft May 28, 2023 05:22

adaki2004 closed this Jul 1, 2023

dantaik deleted the multi_prover_possibility branch September 25, 2023 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol): Enable protocol to handle multiple proof types #13745

feat(protocol): Enable protocol to handle multiple proof types #13745

adaki2004 commented May 12, 2023 •

edited

Loading

Brechtpd left a comment

Brechtpd May 12, 2023

Brechtpd May 12, 2023

Brechtpd May 12, 2023

adaki2004 May 12, 2023

dantaik May 15, 2023

dantaik May 15, 2023

dantaik May 15, 2023

dantaik May 15, 2023

dantaik May 15, 2023

dantaik May 15, 2023

dantaik May 15, 2023

adaki2004 May 15, 2023 •

edited

Loading

Brechtpd May 15, 2023

adaki2004 May 15, 2023

dantaik May 15, 2023

dantaik May 15, 2023

dantaik commented May 15, 2023

dantaik commented May 15, 2023

adaki2004 commented May 15, 2023

Brechtpd commented May 15, 2023

adaki2004 commented May 18, 2023

Brechtpd commented May 18, 2023

adaki2004 commented May 22, 2023

adaki2004 May 23, 2023

Brechtpd May 23, 2023

adaki2004 May 23, 2023 •

edited

Loading

davidtaikocha May 23, 2023

adaki2004 May 23, 2023

adaki2004 commented May 23, 2023

dantaik commented Jul 1, 2023

adaki2004 commented Jul 1, 2023

-            if (index == 0 && ((config.proofTypeEnabled >> index) & 1) == 1) {
+            if (((config.proofTypeEnabled >> index) & 1) == 1) {
+               if (index == 0) {
+                 // ...
+               } else if (index == 1) {
+                 // ...
+               }

feat(protocol): Enable protocol to handle multiple proof types #13745

feat(protocol): Enable protocol to handle multiple proof types #13745

Conversation

adaki2004 commented May 12, 2023 • edited Loading

Brechtpd left a 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

adaki2004 May 15, 2023 • 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

dantaik commented May 15, 2023

dantaik commented May 15, 2023

adaki2004 commented May 15, 2023

Brechtpd commented May 15, 2023

adaki2004 commented May 18, 2023

Brechtpd commented May 18, 2023

adaki2004 commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adaki2004 May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adaki2004 commented May 23, 2023

dantaik commented Jul 1, 2023

adaki2004 commented Jul 1, 2023

adaki2004 commented May 12, 2023 •

edited

Loading

adaki2004 May 15, 2023 •

edited

Loading

adaki2004 May 23, 2023 •

edited

Loading