-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(prover): Use SNARK wrapper VK for picking prover jobs #142
fix(prover): Use SNARK wrapper VK for picking prover jobs #142
Conversation
@@ -24,6 +25,8 @@ pub struct VkCommitments { | |||
|
|||
fn circuit_commitments() -> anyhow::Result<L1VerifierConfig> { | |||
let commitments = generate_commitments().context("generate_commitments()")?; | |||
let snark_wrapper_vk = std::env::var("CONTRACTS_RECURSION_SCHEDULER_LEVEL_VK_HASH") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a JSON with snark wrapper vk (vk_setup_data_generator_server_fri/data/snark_verification_scheduler_key.json), I think you should calculate hash based on this key like it's done for other hashes and not just take it from env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the VK encodings that we use before hashing in Verifier.sol and in zkevm_test_harness are different (not only the order of fields is different, but also Verifier adds an extra field).
Unless we want to do a large copy-paste the hashes would be different. I think we should leave this for now and add actually computing the vk commitment later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really unfortunate, I'm ok if we merge it to boojum-integration
as is to unblock testing, but we should fix it before merging to main.
Co-authored-by: Stanislav Bezkorovainyi <[email protected]>
prover/vk_setup_data_generator_server_fri/src/commitment_utils.rs
Outdated
Show resolved
Hide resolved
prover/vk_setup_data_generator_server_fri/data/snark_verification_scheduler_key.json
Outdated
Show resolved
Hide resolved
…la-581-use-snark-vk-hash-while-picking-prover-jobs
d3c3f9d
to
08ba08e
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## boojum-integration #142 +/- ##
===================================================
Coverage 32.14% 32.14%
===================================================
Files 482 482
Lines 25930 25930
===================================================
Hits 8336 8336
Misses 17594 17594
☔ View full report in Codecov by Sentry. |
… Validium mode and Rollup mode (#142) * add validium env test * abstract from_env method
What ❔
Prover and Witgen now use snark wrapper VKs to pick their jobs.
Why ❔
prover_protocol_versions
table had SNARK wrapper VKs, but prover and witgens picked jobs based on FRI scheduler VKs.Since if one changes the other also does, we can use the SNARK one instead, since it's also used in
eth_sender
.