This repository was archived by the owner on Jul 5, 2024. It is now read-only.
Integration test of EVM circuit should cache the proving/verifying keys#1148
Merged
Conversation
github-actions
Bot
added
crate-bus-mapping
leolara
changed the title
CPerezz
suggested changes
Feb 8, 2023
| let pk = if $real_prover { Some((*EVM_CIRCUIT_KEY).clone()) } else { None }; | ||
| let (builder, _) = gen_inputs(*block_num).await; | ||
| let mut block = block_convert::<Fr>(&builder.block, &builder.code_db).unwrap(); | ||
| block.circuits_params.max_evm_rows = 0; |
Contributor
There was a problem hiding this comment.
It's 0 by default. And here we have a default circuits_params right? So this statement is unneded.
Contributor
There was a problem hiding this comment.
Also ,if we want to reuse the PK/VK, we should set a fixed value instead of compute at runtime.
Contributor
Author
There was a problem hiding this comment.
Yes, this statement is a left over, and perhaps it is causing the errorsm thanks
Contributor
Author
There was a problem hiding this comment.
Except it is not 0 by default in the integration tests. I set it to zero to trigger the calculation of the minimum rows, but then I created the method that does that without setting any parameters.
Anyway, it is an error, and might cause an error
Contributor
There was a problem hiding this comment.
Ohh by default i meant the Default trait impl for CircuitsParams.
Contributor
There was a problem hiding this comment.
I think in integration tests is where we need to set MAX_EVM_ROWS instead of 0.
Contributor
Author
There was a problem hiding this comment.
Yes, that is done already in https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1148/files#diff-a5f41858194d9e4113ec4124717d29358cca4df88def6d33815a7ab20205e3d1R57
Before I set it to zero there to obtain the calcualted number of rows, but with the new method https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/1148/files#diff-b4e5f73a1df679165a0103a62054eeff42c1bcbba32eff0ba8287c52740f2738R214-R223 that was not necessary and could actually caused problems
| } | ||
| } | ||
|
|
||
| num_rows + 2 // EndBlock and at least one unused |
Contributor
There was a problem hiding this comment.
Not sure this comment is enough to understand why we add 2 precisely. and why EndBlock only requires 1.
Contributor
Author
There was a problem hiding this comment.
I really don't actually know the answer, I just refactored the value that had before. An idea of a better explanation for this?
Contributor
There was a problem hiding this comment.
Nope.. But we can file an issue for that! :)
Contributor
Author
There was a problem hiding this comment.
I don't know the details of the EVM circuit, I guess I should put some time into that. It seems one of the most advanced ones.
leolara
force-pushed
the
leo/integration-tests-cache-keys-evm
branch
from
f55ab94 to
47d5b00
Compare
leolara
force-pushed
the
leo/integration-tests-cache-keys-evm
branch
from
47d5b00 to
f1e8986
Compare
Collaborator
|
Note: should not be an issue on the circuit tests. But rather it could be something on the flow For example type of errors like: "before using 0 generating parameters for the circuit, if we have more than 1 instance of a circuit, need proving keys for the bigger one and trigger for the smallers." Prove for circuits annotation: #1108 |
Contributor
Author
There is no code in that issue so I cannot run the test on this circuit. |
Contributor
|
I just realized that now the ExpTable contains a fixed column, and when the table is used without its circuit, we're missing the padding to a fixed size Although this may be unrelated to this issue, I'm not sure if any of the integration tests we have use the EXP opcode. |
Contributor
Author
|
Currently we have these results: DetailsEdited by @CPerezz so that the msg does not take the entire thread. |
Contributor
|
I have analyzed the non-constant fixed columns error with the copy_circuit and found the issue: The TxTable contains a fixed column, and when used without the TxCircuit it varies depending on the input. At least in the scenario of the Integration Tests. I'm working on a fix. |
Contributor
Author
1 task
Contributor
I think merging this PR is a viable option: we will see the failures on main, but this will allow others to work on fixes via parallel PRs. BTW, before reading your message I submitted this PR #1169 with the intention to fix the copy circuit error you shared, but I think it also fixes the evm circuit error. I'm not sure about the super circuit, as I haven't tested it. |
Contributor
Author
|
@ed255 I think merging it is the best as I would need to be fixing conflics for a long time otherwise. |
Contributor
Author
|
Also, Super uses EVM and EVM uses copy so it is likely that it will solve it. |
Contributor
Author
ed255
approved these changes
Feb 14, 2023
ed255
left a comment
ed255
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Nice refactor as well! :D
This was referenced Feb 14, 2023
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
This was referenced Feb 16, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #967
Additionally moves max_evm_rows.
Closes #1122
3 out of 5 real prover integration tests still are failing, perhaps there is another variation between circuits instances other than row number.