This repository has been archived by the owner on May 3, 2024. It is now read-only.
Circuit tools with refactored CellManager #96
Conversation
This file contains 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
### Description Fix a typo found by another community developer ### Issue Link N/A ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents N/A ### How Has This Been Tested? N/A
…1300) ### Issue Link * First mentioned in privacy-scaling-explorations#953 * Pr link This is the second pr for this. The older one privacy-scaling-explorations#1003 is abandoned as there are too much conflicts. ### Contents * Gather test related into test mod. * Gather most of the `impl Circuit` into `dev.mod` This is for distinct the `test-circuits` and the test features. privacy-scaling-explorations#1144
…#1311) ### Description When building AccessSet, we read stack input of opcode(eg: read last element of stack when processing BALANCE opcode). But this method will encounter error when dealing with some ill-formed bytecode(BALANCE when empty stack). So in this PR, when access parsing for a single step encounters errors, the error is printed, the access parsing procedure will continue instead of being interrupted and return. ### Issue Link [_link issue here_] ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - [_item_] ### Rationale [_design decisions and extended information_] ### How Has This Been Tested? "StackUnderFlowContractCreation_d0_g0_v0" in testool can pass with this PR <hr> ## How to fill a PR description Please give a concise description of your PR. The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request. MUST: Reference the issue to resolve ### Single responsability Is RECOMMENDED to create single responsibility commits, but not mandatory. Anyway, you MUST enumerate the changes in a unitary way, e.g. ``` This PR contains: - Cleanup of xxxx, yyyy - Changed xxxx to yyyy in order to bla bla - Added xxxx function to ... - Refactored .... ``` ### Design choices RECOMMENDED to: - What types of design choices did you face? - What decisions you have made? - Any valuable information that could help reviewers to think critically Co-authored-by: Ming <hero78119@gmail.com>
…aling-explorations#1340) ### Description Remove code duplication between returndatacopy and others as they use similar test codes to construct bytecode for testing. ### Issue Link privacy-scaling-explorations#1324 ### Type of change - [x] Refactor code ### Contents Remove code duplication in unit tests between returndatacopy, balance, calldataload and others. The unit tests should behave the same. ### Rationale It's easier to manage the mock bytecode when we move similar code into one place. ### How Has This Been Tested? Use the command `make test` to run unit tests <hr> --------- Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
…ons#1339) ### Description Refer to comment in [privacy-scaling-explorations#1161](privacy-scaling-explorations#1161 (comment)) ### Issue Link [privacy-scaling-explorations#1308](privacy-scaling-explorations#1308) ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### This PR contains: - Deleted [L401-402](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/7e9603a28a818819c071c81fd2f4f6b58737dea6/zkevm-circuits/src/state_circuit/constraint_builder.rs#L402) in StateCircuit
…eturn in bus_mapping (privacy-scaling-explorations#1342) ### Description - unify handle_restore_context and gen_restore_context_ops ### Issue Link fix privacy-scaling-explorations#1186 ### Type of change - [x] refactor (non-breaking change which adds functionality) ### Contents - merge `gen_restore_context_ops` into `handle_restore_context`
…#1341) fix privacy-scaling-explorations#1302 --------- Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
) ### Description Fixes bug soundness in bytecode circuit, where an attacker can insert as many bytes after the last byte (row where `q_last`=1). ### Issue Link privacy-scaling-explorations#1332 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Bug fix 1: Add a constraint that forces the next state to be "Header" when the current state is "Byte" and `index == length - 1` - Bug fix 2: set `q_enable=1` for the row where `q_last=1` ### Rationale This would patch the possibility of an attacker inserting false bytes that extend beyond the correct bytecode. ### How Has This Been Tested? Created a test called `bytecode_soundness_bug_1` that overwrites the last rows with`tag=Byte`. `cargo test --features test --package zkevm-circuits --lib -- bytecode_circuit::circuit::tests::bytecode_soundness_bug_1 --exact --nocapture` Also ran other unit tests to check correctness was maintained. --------- Co-authored-by: Eduard S <eduardsanou@posteo.net>
…s#1299) ### Description This PR aims to remove `ExecutionState::{ErrorDepth,InsufficientBalance ,ErrorContractAddressCollision}` from `ExecutionState::halts_in_exception` and handle `InsufficientBalance` properly by `CallOpGadget` ### Issue Link privacy-scaling-explorations#1298 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents Some exception states happen right in the moment when we are doing `*CALL*` or `CREATE*`, including: - `ErrorDepth` - `ErrorInsufficientBalance` - `ErrorContractAddressCollision` Which will only be treated as failure of sub-call, and won't halt current call. ```mermaid stateDiagram-v2 state fork <<fork>> state join <<join>> state NextCall { ...* --> join } state CurrentCall { ... --> fork fork --> CALL|CREATE fork --> ErrorDepth fork --> ErrorInsufficientBalance fork --> ErrorContractAddressCollision CALL|CREATE --> NextCall ErrorDepth --> join ErrorInsufficientBalance --> join ErrorContractAddressCollision --> join join --> NextStep } ``` However, if we want to handle these as a separate state we'd have many constraint duplicated (e.g. gas cost calculation), so `ErrorInsufficientBalance` is merged into `CALL` in privacy-scaling-explorations#998 without too much overhead. For the other 2 states we could apply the same strategy to avoid confusion. ### Rationale ### How Has This Been Tested? ### Design choices
### Description Fix issue privacy-scaling-explorations#1192 ### Issue Link Issue privacy-scaling-explorations#1192 ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - [_item_] ### Rationale [_design decisions and extended information_] ### How Has This Been Tested? [_explanation_] <hr> ## How to fill a PR description Please give a concise description of your PR. The target readers could be future developers, reviewers, and auditors. By reading your description, they should easily understand the changes proposed in this pull request. MUST: Reference the issue to resolve ### Single responsability Is RECOMMENDED to create single responsibility commits, but not mandatory. Anyway, you MUST enumerate the changes in a unitary way, e.g. ``` This PR contains: - Cleanup of xxxx, yyyy - Changed xxxx to yyyy in order to bla bla - Added xxxx function to ... - Refactored .... ``` ### Design choices RECOMMENDED to: - What types of design choices did you face? - What decisions you have made? - Any valuable information that could help reviewers to think critically
…orations#1363) ### Description Updates the geth dependency in geth-utils to latest release. Pulled this out from privacy-scaling-explorations#1361. ### Issue Link Related to privacy-scaling-explorations#1362. ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Update geth dependency (and ran `go mod tidy` to update sub-dependencies) - Update code bindings to struct changes ### Rationale N/A ### How Has This Been Tested? Using the testing suite in the repo.
…g-explorations#1288) ### Description Spec Markdown PR privacy-scaling-explorations/zkevm-specs#397 #### Summary 1. Merge OOG error `ExtCodeCopy` into `MemoryCopy`. 2. Implement bus-mapping and circuit for this OOG error of `CALLDATACOPY`, `CODECOPY`, `EXTCODECOPY` and `RETURNDATACOPY` opcodes. 3. OOG error executions of `CALLDATACOPY`, `CODECOPY` and `RETURNDATACOPY` are same. 4. `EXTCODECOPY` has extra `1` stack pop, and constant gas costs are different for cold (2600) and warm (100) accounts according to EIP-2929 (could reference [go-etherum gasExtCodeCopyEIP2929 function](https://github.com/ethereum/go-ethereum/blob/master/core/vm/operations_acl.go#L116)). 5. Fix `memory_copier_gas_cost` function to not calculate memory expansion for zero copy size in gas utils of `eth-types`. ### Issue Link Close privacy-scaling-explorations#1266 ### Type of change - [X] New feature (non-breaking change which adds functionality) ### How Has This Been Tested? Add new unit-test cases for this error state.
…explorations#1365) ### Description Refactor and add more helper functions to Bytecode: - Removed several functions such as `call`, `balance`, `mstore`, `calldatacopy`, and `return_bytecode` - Added `op_jumpdest` function - Implemented `impl_push_n` macro to generate functions for `op_push1` to `op_push32` - Implemented `impl_other_opcodes` macro to generate functions for various opcodes like `op_stop`, `op_add`, `op_mul`, `op_sub`, and so on - Renamed opcode helper functions to `op_{opcode}` for consistency and clarity ### Issue Link N/A ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Removal of several functions - Addition of `op_jumpdest` function - Implementation of `impl_push_n` macro to generate functions for `op_push1` to `op_push32` - Implementation of `impl_other_opcodes` macro to generate functions for various opcodes - Renaming of opcode helper functions ### Rationale This refactor improves the code by removing unused functions, adding a new function, and implementing macros to generate functions for opcodes. The renaming of the opcode helper functions to `op_{opcode}` provides consistency and clarity to the codebase. ### How Has This Been Tested? https://github.com/scroll-tech/zkevm-circuits/actions/runs/4751119765
…orations#1373) ### Description Upgrade toolchain to nightly-2023-04-24 for future compatibilities. ### Issue Link N/A ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Toolchain upgrade to nightly-2023-04-24 - Fix lint errors ### Rationale The toolchain currently in use is outdated and lacks stable features. Issue privacy-scaling-explorations#1372 requires GAT, which has been stabilized since version `1.65.0`. Meanwhile, the current stable Rust version is `1.69.0`. ### How Has This Been Tested? N/A
…der (privacy-scaling-explorations#1318) ### Description evm ConstraintBuilder and BaseConstraintBuilder have: require_zero require_equal require_boolean require_in_set condition add_constraints add_constraint validate_degree all with very similar implementation. Deduplicate them ### Issue Link privacy-scaling-explorations#1202 ### Type of change - Refactor ### Contents - Created a trait for common code. ### Rationale DRY ### How Has This Been Tested? Run tests. ### Notes (TODO?) - condition and validate_degree use the state, I don't know what could be the rusty way of making this code common. - Perhaps we should rethought the names of the trait and structs, BaseConstraintBuilder seems to be used mostly outside EVM circuit, while ConstraintBuilder is used in the EVM circuit exclusively. --------- Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com>
…orations#1372) ### Description Added memory reconstruction for call to precompile contracts. ### Issue Link No issue is associated with this pull request. ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Added a new function `is_precompiled` to check if the address of the contract is a precompiled contract. - Added a new function `execute_precompiled` to execute a precompiled contract. - Added a new helper function `code_address` associated with `Call` struct to get the actual address of code execution. - Reconstruct memory for call to precompile address. ### Rationale The addition of support for call to precompiled contracts in bus-mapping allows no-memory exec trace. ### How Has This Been Tested? This PR only includes the memory reconstruction part. It has been test locally via `bus_mapping::evm::opcodes::callop::tests::test_precompiled_call`. --------- Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
…acy-scaling-explorations#1282) ### Description To remove magic numbers for blinding factors, this PR adds an associated function `unusable_rows` for `SubCircuit` to returns their `unusable_rows`, which should be `meta.blinding_factors() + 1`. Tests are also added to make sure the returned values are correct. Note that currently `KeccakCircuit` could be configured by environment variable `KECCAK_ROWS` to decide how many rows a round would take, which affect the different rotation queried, and it's unfortunately not a easy way to compute how many different rotations are used, so this PR hardcodes the unusable rows from `KECCAK_ROWS = 1` to `20` for easy lookup. ### Issue Link privacy-scaling-explorations#949 ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - [_item_] ### Rationale [_design decisions and extended information_] ### How Has This Been Tested? [_explanation_] --------- Co-authored-by: David Nevado <davidnevadoc@users.noreply.github.com> Co-authored-by: "adria0.eth" <5526331+adria0@users.noreply.github.com> Co-authored-by: Eduard S <eduardsanou@posteo.net>
privacy-scaling-explorations#1317) ### Description Fix successful run cases with Uint64 overflow for multiple opcodes. 1. Add `WordByteRangeGadget` to constrain if Word is within the specified byte range. 2. Add `WordByteCapGadget` to constrain if Word is within the specified byte range (implemented by WordByteRangeGadget) and less than a maximum cap (used to replace a WordByteRangeGadget and LtGadget). 3. Fix bus-mapping and zkevm-circuits to handle overflow cases. And add unit-tests for these cases. TODO: will try to handle memory offset overflow with zero length in another PR (try to rebase for this local PR scroll-tech#393) and related issue privacy-scaling-explorations#1301. ### Rationale Reference detailed code in `go-etherum` as: . [BLOCKHASH](https://github.com/ethereum/go-ethereum/blob/master/core/vm/instructions.go#L438) . [CALLDATALOAD](https://github.com/ethereum/go-ethereum/blob/master/core/vm/instructions.go#L285) . [CALLDATACOPY](https://github.com/ethereum/go-ethereum/blob/master/core/vm/instructions.go#L306) . [CODECOPY](https://github.com/ethereum/go-ethereum/blob/master/core/vm/instructions.go#L364) . [EXTCODECOPY](https://github.com/ethereum/go-ethereum/blob/master/core/vm/instructions.go#L382) . [JUMPI](https://github.com/ethereum/go-ethereum/blob/master/core/vm/instructions.go#L550) ### Issue Link Close privacy-scaling-explorations#1276 ### Type of change - [X] Bug fix (non-breaking change which fixes an issue) ### How Has This Been Tested? Add unit-test cases for Uint64 overflow values. --------- Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
### Description Add Byte Range Checks in `LtChip`. This PR modifies the `LtChip`: - Added Fixed Column `u8` to the configuration of the chip - Added a lookup constraint between each `diff` advice column and the `u8` column - Added `load` function that assigns each integer from 0 to 255 (8bits) to the `u8` column Furthermore, the following have been applied to the tests: - In `less_than` testing, the circuit now takes `k = 9` to support a higher number of rows - In `copy_circuit` testing, the `assign_copy_events` function now calls `load` on the `lt_chip` to load the `u8` column ### Issue Link Related to privacy-scaling-explorations#916 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) ### How Has This Been Tested? `make test-all` --------- Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com> Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
… gen_end_tx_ops (privacy-scaling-explorations#1336) ### Description fix privacy-scaling-explorations#1180 ### Issue Link privacy-scaling-explorations#1180 ### Type of change - [x] New feature (non-breaking change which adds functionality) ### Contents see privacy-scaling-explorations#1180 ### Rationale [_design decisions and extended information_] ### How Has This Been Tested? Pass all the existing test cases --------- Co-authored-by: Han <tinghan0110@gmail.com>
…ons#1378) ### Description We tried to remove some unused tx structs. ### Issue Link privacy-scaling-explorations#528 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Remove zkevm-circuits/src/witness/call.rs and the `Call` struct - Remove fields in circuit_input_builder Transaction struct. Reuse Geth type Transaction struct whenever possible - Remove duplicated call data gas cost computation - Add APIs to get "To" address so that we know when we want contract address or zero address. ### Rationale We try to start the de-duplication from the low-hanging fruits. So I didn't remove the witness.rs Transaction struct in this PR. ### How Has This Been Tested? Local build tests and quick tests
…ons#1389) ### Description An important typo. ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) ### Contents - Add term `a[3] * b[1]` (was written as `b[2]`). Co-authored-by: Aurélien Nicolas <info@nau.re>
…1374) ### Description Update halo2 to the latest dependency, which contains some changes affecting the `ff` trait. ### Issue Link Resolve privacy-scaling-explorations#1366 ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Rationale I've extended the `Field` trait we define in `eth_types` to contain the common traits used in different places, to avoid having a custom list of traits in each function.
condition excluded, would be better if optimize condition as well
CeciliaZ030
force-pushed
the
circuit-tools
branch
from
009365d to
5d29b5b
Compare
CeciliaZ030
force-pushed
the
circuit-tools
branch
from
5d29b5b to
0632daf
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
CellManager_
Check out
CellManager_with the underscore, the one without is old:Column<Advice>CellTypeare made generic so that user can specified different types, and it should be initialized withCellConfig<C: CellTypeTrait> { cell_type: C, num_columns: usize, phase: u8, is_permute: bool, }Vec<&dyn LookupTable>to look at how many columns any table needs then automatically generate them.Still very messy because I tried different things to make circuit-tools completely independent from the zkevm constructs. The latest attempt I believe should be backward compatible, except that I can't avoid adding a few generics like
<C: CellTypeTrait>ahead of some functions.Ignore
<T: TableType>for now, it will be cleaned up.