DualVerifier BootStrapping #944
Conversation
Support old L2->L1 log proving format
Set of fixes
Port kl/sync layer reorg
…-contracts into ra/l2-native
Merge reorg stable
fix: Ra/l2 native
fix: l2-native-token, bridge fixes
feat: periodic merge with multiple fixes
Merge main into sync-layer-stable
razzorsec
requested review from
Deniallugo,
vladbochok and
StanislavBreadless
as code owners
| // slither-disable-next-line uninitialized-state | ||
| ZKChainStorage internal s; |
There was a problem hiding this comment.
Verifier is a stand alone application, should have its own storage. But actually it can have no storage and works fine
| address plonkVerifier = s.plonkVerifier; | ||
| address fflonkVerifier = s.fflonkVerifier; | ||
| uint256 fflonkProofLength = s.fflonkProofLength; | ||
| uint256 proofLength = _proof.length; |
There was a problem hiding this comment.
Too expensive to read everything from storage, we optimised verifier not to waste gas on the storage read here
|
|
||
|
|
||
| /// @inheritdoc IVerifier | ||
| function verificationKeyHash() external pure returns (bytes32 vkHash) { |
There was a problem hiding this comment.
Why did you change this function, it was different on the audit.
The idea for this function to return commitment of verification keys. Here it just calculate hash of constants, please restore the code and don't change audited code.
There was a problem hiding this comment.
I changed the function based on the suggestions during the audit (so the new change was also audited). Earlier the function used to store the values which were also constants, in memory and then produce the hash of it, here we have made those values as constants and take the hash out of it.
| /// @dev The address of the PLONK Verifier contract | ||
| address plonkVerifier; | ||
| /// @dev The address of the FFLONK Verifier contract | ||
| address fflonkVerifier; | ||
| /// @dev The length of the FFLONK proof type | ||
| uint256 fflonkProofLength; | ||
| } |
There was a problem hiding this comment.
We don't need it here. This is a storage for ZKChain contract, not for verifier. Verifier can contain it itself
There was a problem hiding this comment.
Was planning to keep the DualVerifier modular with ZKChainStorage, where if we want to simply upgrade a PLONK/FFLONK verifier, it could be easily possible. I might be wrong here though.
| // We can only process 1 batch proof at a time. | ||
| if (proofPublicInput.length != 1) { | ||
| revert CanOnlyProcessOneBatch(); | ||
| } | ||
|
|
||
| bool successVerifyProof = s.verifier.verify(proofPublicInput, _proof); | ||
| if (!successVerifyProof) { | ||
| (bool callSuccess, bytes memory successVerifyProof) = address(s.dualVerifier).delegatecall(abi.encodeWithSelector(s.dualVerifier.verify.selector, proofPublicInput, _proof)); |
There was a problem hiding this comment.
No delegate calls please! Why it can't be simple staticcall that provides some clear guaruntee like not changing the state?
There was a problem hiding this comment.
I had the idea of accessing ZKChainStorage state from the DualVerifier as a delegatecall.
| /// @inheritdoc IGetters | ||
| function getPlonkVerifier() external view returns (address) { | ||
| return s.plonkVerifier; | ||
| } | ||
| /// @inheritdoc IGetters | ||
| function getFflonkVerifier() external view returns (address) { | ||
| return s.fflonkVerifier; | ||
| } | ||
| /// @inheritdoc IGetters | ||
| function getFflonkProofLength() external view returns (uint256) { | ||
| return s.fflonkProofLength; | ||
| } |
There was a problem hiding this comment.
ZK chain should know nothing about fflonk/plonk or dual verifier. It has only one contract - verifier and it is an abstraction for the contract what proof system is inside
|
|
||
| /// @notice Change the address of the FFLONK verifier smart contract | ||
| /// @param _newFflonkVerifier FflonkVerifier smart contract address | ||
| function _setFflonkVerifier(address _newFflonkVerifier) private { |
There was a problem hiding this comment.
system should work only with one verifier, no need to set fflonk and plonk separately. The only thing zk chain should care is the address of the verifier, dual verifier should route the verification to one of them
What ❔
Why ❔
DualVerifier will act as a wrapper contract to route proof verification based on the proof length, i.e., either PLONKish(current) Verifier or FFLONK(new and upcoming) verifier.
Checklist