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(prover): Update witness generator to zkevm_test_harness 0.150.6 #3029

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

0xVolosnikov
Copy link
Contributor

What ❔

Why ❔

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via zk_supervisor fmt and zk_supervisor lint.

Copy link
Contributor

@EmilLuta EmilLuta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very WIP state, but I love the direction. Not having to deal with partial circuits is neat. What I don't understand, how did we manage to compact it, without blowing up the memory?

P.S. We'll have to adjust circuit prover as well.

@@ -470,6 +470,7 @@ impl Keystore {
}

/// Async loads mapping of all circuits to setup key, if successful
#[cfg(feature = "gpu")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, was this a miss on my end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

Copy link
Contributor

@EmilLuta EmilLuta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a separate note, did you manage to run this?

let artifacts_receiver_handle = thread::spawn(move || {
let span = tracing::info_span!(parent: make_circuits_span_copy, "make_circuits_blocking");

while let Ok(artifact) = artifacts_receiver.recv() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, but shouldn't we do something if we can't receive? Maybe log the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we can have only RecvError, which occurs if the sender is disconnected. This is the expected state after test_harness has finished its work, I don't think we want to log this.

@0xVolosnikov
Copy link
Contributor Author

@EmilLuta

how did we manage to compact it

This process will happen in two steps: in the first step (this PR) we simply fill unnecessary fields with zeros to maintain backward compatibility. The next step is to change the input structures of the circuits, from which we will remove unnecessary fields. We will do this during the protocol update.

did you manage to run this?

Yes. But we need to update Shivini, which is part of another monorepo, so that the prover can be compiled in CI

We'll have to adjust circuit prover as well.

I'm not sure about it. I'll check, but at this stage it shouldn't affect it in any way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants