Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Improve supercircuit usage #994

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Improve supercircuit usage #994

merged 1 commit into from
Jan 5, 2023

Conversation

ed255
Copy link
Member

@ed255 ed255 commented Dec 16, 2022

Add two new unit tests for the super circuit:

  • block with 1 tx with max_txs=2
  • block with 2 txs with max_txs=2 The super circuit unit tests are still marked as ignored so that they are skipped in the github PR checks because they take a long time to run

Improve ergonomics of rows estimation per circuit. Add a function min_num_rows_block to each circuit to get the minimum number of required rows to prove a given block. Related to #982

Resolve #883

@github-actions github-actions bot added crate-integration-tests Issues related to the integration-tests workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Dec 16, 2022
Copy link
Contributor

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

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

Really clear and easy to follow changes, good job! :)
Just left a couple of comments, otherwise LGTM

// --all-features -- --nocapture`
// The value rows_range_chip_table has been optained by patching the halo2
// library to report the number of rows used in the range chip table
// region. TODO: Figure out a way to get these numbers automatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good workaround! We should open an issue to address this once we merge the PR

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Could min_num_rows_block end up as a trait-fn associated to the SubCircuit trait or something similar?? I think it would make much more sense than having it as we have it here. Specially now that #937 is merged and integrated cross all the circuits.

zkevm-circuits/src/evm_circuit.rs Show resolved Hide resolved
// --all-features -- --nocapture`
// The value rows_range_chip_table has been optained by patching the halo2
// library to report the number of rows used in the range chip table
// region. TODO: Figure out a way to get these numbers automatically.
Copy link
Member

Choose a reason for hiding this comment

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

zkevm-circuits/src/tx_circuit/sign_verify.rs Show resolved Hide resolved
zkevm-circuits/src/witness/block.rs Outdated Show resolved Hide resolved
@aguzmant103
Copy link
Member

Hey @ed255 just pinging you to give visibility on CPerezz comment's before holiday :)

@ed255 ed255 force-pushed the feature/super-circuit-tests branch from 36f6040 to 6562e83 Compare January 4, 2023 16:11
Add two new unit tests for the super circuit:
- block with 1 tx with max_txs=2
- block with 2 txs with max_txs=2
The super circuit unit tests are still marked as ignored so that they are
skipped in the github PR checks because they take a long time to run

Improve ergonomics of rows estimation per circuit.  Add a function
`min_num_rows_block` to each circuit (via the SubCircuit trait) to get the
minimum number of required rows to prove a given block. Related to
#982

Resolve #883
@ed255 ed255 force-pushed the feature/super-circuit-tests branch from 6562e83 to 8f934de Compare January 4, 2023 16:12
@ed255
Copy link
Member Author

ed255 commented Jan 4, 2023

Could min_num_rows_block end up as a trait-fn associated to the SubCircuit trait or something similar?? I think it would make much more sense than having it as we have it here. Specially now that #937 is merged and integrated cross all the circuits.

This is a great idea! It looks better organized this way :) And I actually found that I forgot to add this function to the PiCircuit! I've applied this in the last PR update.

@ed255
Copy link
Member Author

ed255 commented Jan 4, 2023

I think I've addressed all comments from @davidnevadoc and @CPerezz
@CPerezz could you check this PR again? :)

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM. Just left a comment. Address it if you consider.

zkevm-circuits/src/witness/block.rs Show resolved Hide resolved
@ed255 ed255 merged commit 878e5e6 into main Jan 5, 2023
@CPerezz CPerezz deleted the feature/super-circuit-tests branch January 5, 2023 11:13
noel2004 pushed a commit to noel2004/zkevm-circuits that referenced this pull request Nov 29, 2023
* temporarily comment out rlc

* correct modulo
and ec mul

* correct import

* rlc from word limbs:

* fix ec mul

* ec mul adapt rlc pattern

* change ecrecover structs

* correct parts of ecrecover gadget

* fix modulo conflict

* fix precompile failed error state

* remove import

* fix precompile gadget

* fit lt_word

* remove import

* remove import

* remove import

* fix callop

* remove import

* fix modulo

* restore rlc

* minor math fix

* fix lt generic

* remove lt word cell type

* fix lt word gadget related code

* fix modulo

* fix ecrecover constants

* remove import

---------

Co-authored-by: DreamWuGit <wwuwwei@126.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-integration-tests Issues related to the integration-tests workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more super circuit unit tests
4 participants