Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(protocol): deploy the generated Yul plonk verifier #13016

Merged
merged 15 commits into from
Jan 26, 2023

Conversation

davidtaikocha
Copy link
Member

@davidtaikocha davidtaikocha commented Jan 23, 2023

ref: #12126

Not included in this PR:

  • verificationKey is hard-coded in the generated PlonkVerifier, need to make it as a configurable parameter.
  • public input is assembled in client software for testing purposes right now, need to move this part of logic into protocol.

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #13016 (23acef5) into main (088933e) will decrease coverage by 0.38%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main   #13016      +/-   ##
==========================================
- Coverage   65.34%   64.97%   -0.38%     
==========================================
  Files         113      112       -1     
  Lines        3105     3095      -10     
  Branches      386      383       -3     
==========================================
- Hits         2029     2011      -18     
- Misses        999     1008       +9     
+ Partials       77       76       -1     
Flag Coverage Δ *Carryforward flag
bridge-ui 92.61% <ø> (ø) Carriedforward from 088933e
protocol 56.75% <50.00%> (-0.75%) ⬇️
relayer 69.10% <ø> (ø) Carriedforward from 088933e
ui 100.00% <ø> (ø) Carriedforward from 088933e

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
packages/protocol/contracts/L1/ProofVerifier.sol 0.00% <0.00%> (ø)
...ckages/protocol/contracts/L1/libs/LibProposing.sol 9.23% <ø> (ø)
packages/protocol/contracts/L1/libs/LibProving.sol 0.00% <ø> (ø)
...ckages/protocol/contracts/L1/libs/LibVerifying.sol 13.33% <0.00%> (ø)
packages/protocol/contracts/libs/LibZKP.sol 0.00% <0.00%> (ø)
...ackages/protocol/contracts/test/L1/TestTaikoL1.sol 87.50% <ø> (ø)
.../contracts/test/L1/TestTaikoL1EnableTokenomics.sol 0.00% <ø> (ø)
packages/protocol/contracts/L1/TaikoL1.sol 38.00% <100.00%> (-1.22%) ⬇️
packages/protocol/contracts/L1/libs/LibUtils.sol 27.27% <100.00%> (ø)
...es/protocol/contracts/thirdparty/LibMerkleTrie.sol 83.13% <0.00%> (-7.23%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@davidtaikocha davidtaikocha marked this pull request as ready for review January 23, 2023 03:46
@dantaik
Copy link
Contributor

dantaik commented Jan 23, 2023

  • verificationKey

@davidtaikocha is there only one verification key or multiple ones?

@davidtaikocha
Copy link
Member Author

  • verificationKey

@davidtaikocha is there only one verification key or multiple ones?

I think right now this generated PlonkVerifier contract file is for only one verification key, but need @smtmfft to confirm.

@davidtaikocha davidtaikocha requested a review from dantaik January 23, 2023 14:43
dantaik
dantaik previously approved these changes Jan 23, 2023
@dantaik
Copy link
Contributor

dantaik commented Jan 23, 2023

Right now, we also look up from AddressManager the verification key, if there is only one of them built into Plonk Verifier, not sure if we need this look up.

@davidtaikocha
Copy link
Member Author

davidtaikocha commented Jan 23, 2023

Right now, we also look up from AddressManager the verification key, if there is only one of them built into Plonk Verifier, not sure if we need this look up.

will also update this part of logic if needed (might in another PR if this one got merged) after @smtmfft 's confirmation.

@smtmfft
Copy link
Contributor

smtmfft commented Jan 24, 2023

The circuit just has some max limits (tx number, calldata size, etc), so far I think we could have only one for test purpose.
Will see if we need more after we get more statistics.
I think the only difference is the proof generation performance of the k-degree circuit, for example, if a k=18 (2**18 rows) circuit could pack 10 txs (not even possible so far) and ideally it is roughtly 2x faster then k=19 (2**19 rows) circuit which could pack 20 txs, we can use different circuits setup, i.e., k=18 for txs <= 10 & k=19 for txs in (10..20].

cyberhorsey
cyberhorsey previously approved these changes Jan 24, 2023
@Brechtpd
Copy link
Contributor

Seems like some manual work to get yul support in. Is it not possible to use existing plugins like https://github.com/ControlCplusControlV/hardhat-Yul? I never used hardhat so no idea if this would be sufficient or not.

@davidtaikocha
Copy link
Member Author

Seems like some manual work to get yul support in. Is it not possible to use existing plugins like https://github.com/ControlCplusControlV/hardhat-Yul? I never used hardhat so no idea if this would be sufficient or not.

image
yeah I tried this hardhat plugin at first, but it failed to compile our generated yulp contract and returned the error in the screenshot above, looks like it was because this plugin is using a javescript yulp compiler that hasn't been updated in two years. And since I couldn't find some other good hardhat plugin options, so I decided to write the compiling job that using the official c++ sloc compiler manually. If the script works good in our usage scenario, then maybe we also can publish it as a hardhat plugin?

@dantaik dantaik dismissed stale reviews from cyberhorsey and themself via bf9807e January 25, 2023 13:25
dantaik
dantaik previously approved these changes Jan 25, 2023
@dantaik dantaik enabled auto-merge (squash) January 26, 2023 01:46
@dantaik dantaik merged commit eb5d564 into main Jan 26, 2023
@dantaik dantaik deleted the integrate-public-input-verifier branch January 26, 2023 01:52
@github-actions github-actions bot mentioned this pull request Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants