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

adaki2004 · 2023-05-22T11:44:58Z

Base branch of this introduced multi-prover possibilities.
This one outsourced proof verification into a separate, deployed contract.

Gas comparison:

Base: (42.074 gwei)

This one (47.418 gwei)

IMPORTANT NOTE

Code to calculate the public instance below stayed in LibProving.

// Put together the input for proof and signature verification
        uint256[9] memory inputs;
...

        bytes32 instance;
        assembly {
            instance := keccak256(inputs, mul(32, 9))
        }

I decided to leave it there, because:

tested a version where i pass the Evidence towards the verifier contract and calculate it there, but it adds additional data (overhead) which costs roughly +1K gas.
In case there is an additional protocol upgrade which changes the Evidence we need to upgrade this new Verifier anyways as well..

Only issue I see with this is: there will be a completely new proof method which will require only some portion of the Evidence so would be useful having that extra data at the verifier contract.

With current plans (ZK, SGX and Failsafe signature overwrite) i see it a no issue, but who knows, maybe worth to pass the whole Evidence data ?
Please let me know what you think @dantaik @Brechtpd

dantaik · 2023-05-22T13:37:57Z

packages/protocol/contracts/L1/TaikoProofVerifier.sol

+    function verifyProof(
+        bytes32 instance, 
+        TaikoData.TypedProof[] calldata blockProofs,
+        AddressResolver resolver


We should not pass in the resolver as this contract shall be a resolver itself.

Well, yes, so I did it on purpose (until there is no setter for AddressManager and wanted to use the 'common' one). Will change tomorrow when addressing the PR findings.

dantaik · 2023-05-22T13:39:33Z

packages/protocol/contracts/L1/ITaikoProofVerifier.sol

+     */
+    function verifyProof(
+        bytes32 instance, 
+        TaikoData.TypedProof[] calldata blockProofs,


What if you change calldata to memory? Will the cost be lower?

No, calldata is much cheaper.

dantaik · 2023-05-22T13:40:20Z

packages/protocol/contracts/L1/ITaikoProofVerifier.sol

+    function verifyProof(
+        bytes32 instance, 
+        TaikoData.TypedProof[] calldata blockProofs,
+        AddressResolver resolver


Should remove resolver as it is implementation specific.

Referred to this in the prev. point.

dantaik · 2023-05-22T13:40:50Z

packages/protocol/contracts/L1/ITaikoProofVerifier.sol

+import {TaikoData} from "./TaikoData.sol";
+import {AddressResolver} from "../common/AddressResolver.sol";
+
+interface IProofVerifier {


rename the file to IProofVerifier.sol

dantaik · 2023-05-22T13:41:32Z

packages/protocol/contracts/L1/TaikoProofVerifier.sol

+}
+
+/// @custom:security-contact [email protected]
+contract TaikoProofVerifier is TaikoErrors {


Rename to ProofVerifier? (and the filename)

dantaik · 2023-05-22T13:42:08Z

packages/protocol/test/TaikoL1MultiProving.t.sol

@@ -13,6 +13,7 @@ import {TaikoToken} from "../contracts/L1/TaikoToken.sol";
 import {SignalService} from "../contracts/signal/SignalService.sol";
 import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 import {TaikoL1TestBase} from "./TaikoL1TestBase.t.sol";
+import {TaikoProofToggleMask, TaikoProofVerifier} from "../contracts/L1/TaikoProofVerifier.sol";


TaikoProofToggleMask not used at all.

Indeed, here not, but we need to have it for toggle mask setter during upgrades. If you referred to this line exaclty then sure, i'll delete it, thanks!

dantaik · 2023-05-22T13:42:42Z

packages/protocol/contracts/L1/TaikoProofVerifier.sol

+import {LibVerifyTrusted} from "./libs/proofTypes/LibVerifyTrusted.sol";
+import {LibVerifyZKP} from "./libs/proofTypes/LibVerifyZKP.sol";
+
+library TaikoProofToggleMask {


Why do we need the code in a library?

Not necessary, just given the fact it's 3 lines and highly related to the ProofVerifier code (so during uprages, this is how we can manage setting the mask) i thought fine setting here. Let me know and i can put it to a different file.

dantaik · 2023-05-22T13:43:53Z

packages/protocol/contracts/L1/ITaikoProofVerifier.sol

+     * @param blockProofs Proof array
+     * @param resolver Current (up-to-date) address resolver
+     */
+    function verifyProof(


rename to verifyProofs?

dantaik · 2023-05-22T13:47:54Z

packages/protocol/contracts/L1/TaikoProofVerifier.sol

+        }
+
+        if(mask != 0) {
+            revert L1_NOT_ALL_REQ_PROOF_VERIFIED();


@adaki2004 Lets talk about it tomorrow. But i feel like this current code can only express AND logic, not the OR logic.

This is true however:

This scenario is covered by an option to (over)write the correct fork choice so a 'SuperProver' is still available.

Is somewhat Brecht's answer here answering your question ?

If you mean 'in general' I'd say - the current implementation of this is indeed does not implement an OR relationship, but this is the beauty of this separation. We do not need to handle things just now, it lives as a standalone deployment and in case we need to OR / AND, we can very easily adjust it.

Brechtpd · 2023-05-22T15:29:35Z

packages/protocol/contracts/L1/IProofVerifier.sol

+    function verifyProofs(
+        bytes32 instance, 
+        TaikoData.TypedProof[] calldata blockProofs,
+        AddressResolver resolver
+    ) external;


Suggested change

function verifyProofs(

bytes32 instance,

TaikoData.TypedProof[] calldata blockProofs,

AddressResolver resolver

) external;

function verifyProof(bytes32 instance, bytes calldata proof) external;

What do you think of this API? This way we don't have any dependencies and the proof can really be whatever, and proofs don't have to confirm to TypedProof which may not always fit.

EDIT: Updated to still have the instance separate. I would say not to pass in the AddressResolver, I think addresses accessed by the verifier should not be set on the protocol AddressResolver, they should store their own data.

Brechtpd · 2023-05-22T15:34:17Z

packages/protocol/contracts/L1/ProofVerifier.sol

+    external
+    view
+    {
+        uint16 mask = getToggleMask();


Maybe we should simply remove the mask? A verifier just always hard-codes the logic that needs to be followed. If this logic changes, a different verifier contract should be deployed.

Well, it is maybe useful for testing still, so this could be the testnet verifier contract or something like that. But on mainnet, probably just want to keep it as simple as possible.

Well, makes things more difficult with testing (multi prover vs. exisitng ZK only proofs).
So, i'd just leave it for now - we shall keep it in mind - but since harmless even on mainnet i'd just handle it with low prio - and change it close to mainnet. (?)

dantaik · 2023-05-23T06:12:56Z

@adaki2004 , as mentioned #13791 (comment), I now perfer not splitting verification logics into their own contracts.

adaki2004 · 2023-05-23T06:14:44Z

@adaki2004 , as mentioned #13791 (comment), I now perfer not splitting verification logics into their own contracts.

Yes, i got it.

adaki2004 · 2023-05-23T07:01:26Z

Closing for now!

adaki2004 added 2 commits May 22, 2023 13:29

Separate proof verification to a standalon deployed contract

b7cdbb8

Fix function name

8b096ed

adaki2004 requested review from Brechtpd and dantaik May 22, 2023 11:44

adaki2004 changed the title ~~feat(protocol): Separate proof verification to a standalon deployed contract~~ feat(protocol): Separate proof verification to a standalone, deployed contract May 22, 2023

adaki2004 self-assigned this May 22, 2023

adaki2004 added area.protocol meta.alpha-4 category.feature status.in-progress labels May 22, 2023

Change to calldata

73cb2e7

dantaik reviewed May 22, 2023

View reviewed changes

dantaik requested changes May 22, 2023

View reviewed changes

dantaik reviewed May 22, 2023

View reviewed changes

PR comments

c62d790

Brechtpd reviewed May 22, 2023

View reviewed changes

dantaik removed the meta.alpha-4 label May 23, 2023

adaki2004 closed this May 23, 2023

dantaik deleted the standalone_proof_verifier branch September 25, 2023 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adaki2004 commented May 22, 2023

dantaik May 22, 2023

adaki2004 May 22, 2023 •

edited

Loading

dantaik May 22, 2023

adaki2004 May 22, 2023

dantaik May 22, 2023

adaki2004 May 22, 2023 •

edited

Loading

dantaik May 22, 2023

dantaik May 22, 2023

dantaik May 22, 2023

adaki2004 May 22, 2023 •

edited

Loading

dantaik May 22, 2023

adaki2004 May 22, 2023 •

edited

Loading

dantaik May 22, 2023

dantaik May 22, 2023

adaki2004 May 22, 2023

Brechtpd May 22, 2023 •

edited

Loading

Brechtpd May 22, 2023

adaki2004 May 22, 2023

dantaik commented May 23, 2023

adaki2004 commented May 23, 2023

adaki2004 commented May 23, 2023

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

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

Conversation

adaki2004 commented May 22, 2023

Choose a reason for hiding this comment

adaki2004 May 22, 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

adaki2004 May 22, 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

adaki2004 May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adaki2004 May 22, 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

Brechtpd May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantaik commented May 23, 2023

adaki2004 commented May 23, 2023

adaki2004 commented May 23, 2023

adaki2004 May 22, 2023 •

edited

Loading

adaki2004 May 22, 2023 •

edited

Loading

adaki2004 May 22, 2023 •

edited

Loading

adaki2004 May 22, 2023 •

edited

Loading

Brechtpd May 22, 2023 •

edited

Loading