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

WIP: sync main to proof-chunk branch #1750

Conversation

hero78119
Copy link
Member

No description provided.

ChihChengLiang and others added 30 commits October 4, 2023 10:27
### Description

1. Capture the error detail of the unsatisfied circuit from
CircuitTestBuilder
2. Split the body of CircuitTestBuilder::run() into 3 methods for
readability.
3. Revamp the error handling in CircuitTestBuilder

### Issue Link

privacy-scaling-explorations#1637  

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)



### Design choices

The error handling has been a pain point for us for a long time. 
The design of error handling in this PR is heavily inspired by [this
blog post](https://mmapped.blog/posts/12-rust-error-handling.html).
…ng-explorations#1640)

### Description

We upstream scroll's precompile handling in bus mapping so that the EVM
circuit warms the precompile addresses at the beginning of the
transaction.

### Issue Link

privacy-scaling-explorations#1626

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

### Content

- Warm precompile addresses
- Fix some minor issues in StepRws offset and the Rws debugging
messages.

---------

Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com>
…rt 1 - pi circuit (privacy-scaling-explorations#1634)

### Description

1st part of privacy-scaling-explorations#1369, mainly on pi circuit integration


### Issue Link

privacy-scaling-explorations#1369 

### 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

- `Withdrawal` struct definition and `WdTable` implementation
- adding `withdrawals` and `withdrawal_root` in block struct
- `wd_table` assignment in pi circuit
…tions#1610)

### Description

Witness generator fixes:

- When branch is hashed, its children are stored as hashes. This causes
problems when computing the neighbour node which is needed for the case
when a node is deleted from a branch with two nodes (the branch
dissappears, the remaining node moves one level up).
- Placeholder storage leaf RLP - when the leaf value is set to zero, the
RLP of the placeholder leaf RLP needs to be updated to reflect the
shortened value.
- `CreateObject` called when there is no storage trie for an account.
- Setting code hash fixed.

Circuit fix:
When value is set to 0 (which deletes the key) and there are two nodes
in the branch, then the node is deleted and the other node moves up one
level (replaces the branch). The returned node is then this moved node
which doesn't have the value 0 and the lookup into MPT tables fails.
Now the circuit is fixed to ensure the new value in the table is 0.

Besides that, some other problems are fixed which were found by @adria0
when integrating the light client and MPT (these were temporary hacks in
MPT for testing purposes that were forgotten to be removed):

-  `SetState` calls removed in `prepare_witness.go` calls from
-  `oracle.PrefetchStorage` call uncommented in `prepare_witness.go`
- `oracle.PrefetchStorage` call uncommented in `state/state_object.go
updateTrie`
- Account nodes were not appended to all nodes in `prepare_witness.go`.

### 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

### How Has This Been Tested?

No test until now covered this scenario, now
`TestNeighbourNodeInHashedBranch` has been added.
### Description

During the review of the codebase, I found some nitpicks in Options and
pattern matching. Here's a quick PR to improve readability.


### Type of change

Nitpick

### Rationale

- Nested conditions hurt readability, match patterns when possible
- unwrapping Options inside a branch beats the purpose of Options
### Description


### Issue Link

privacy-scaling-explorations#1658

### Type of change

Breaking change (fix or feature that would cause existing functionality
to not work as expected)

### Contents

- [x] Use proper Go libraries to dump the json files. This reduces the
number of serialization/deserialization codes and errors.
- [x] Regenerate JSON files.
- [x] Rewrite the parsing code in MPT circuit.

### How Has This Been Tested?

See if the new JSON format can be consumed by the MPT circuit and run
the test successfully.
…scaling-explorations#1662)

### Description

block_table `tag`, `index` those 2 value are not only non-constrained
but also be advice column, which means the `tag`, `index` can be witness
in any value/order and lead to a situation of lookup blocktag under
wrong value. E.g. use `timestamp` value to represent `base_fee`.

Block_table `value` column is constrained by public input so there is no
such issue.

Since `block_table` per row are pre-defined, this PR fixed soundness by
move `tag`, `index` to fixed column, so value will be encoded in verify
key and avoid malicious manipulation.

Folk Scroll previously has similar fixed
scroll-tech@d3922e1#diff-632aa6def09f1668ecdf89781dbf23c2c3cf3bc379209b19cce79abd0a1d0bfe

### Issue Link

N/A

### 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
…privacy-scaling-explorations#1664)

### Description

This works
```rust
cb.step_first(|cb| {
            cb.require_equal("xxxxx", A, B); // it works!
});
```

However it doesnt works on any lookup wrap by step, e.g.
```rust
cb.step_first(|cb| {
       cb.call_context_lookup_write(Some(call_id.expr()), field_tag, value); // lookup will happen on any step other than step 1
});
```

The reason is because lookup constraints expression not including
location selector when query lookup cell.

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs#L1489-L1496
While the location selector are created in execution.rs at later phase 


https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/09651d40afc59462fd943e9789be8f8b0ccb90d9/zkevm-circuits/src/evm_circuit/execution.rs#L731-L757

when converting constraints to custom gate. This works for normal
constraints but not works for lookup expression.

This PR add error pruning step to avoid lookup usage against
step_first/step_last

### 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
### Description

i think this pr can fix many errs of testool report

1. fix bugs of
privacy-scaling-explorations#1639
2. refactor rw reading using the rw iterator, which is far more friendly
to understand compared to the current manually maintained rw offset way.
3. another refactor here, is move the "init code copy event" from callee
stage to caller stage (before state.push_call). It is more intuitive.
Inside execution, we can think the initcode is "copied from
CopyDataType::Memory to CopyDataType::Bytecode" then is executed. So
doing the copy event inside callee stage is a bit wired.

Btw i have another double on hi-lo (which i am not very familiar with):
is this correct?
https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/09651d40afc59462fd943e9789be8f8b0ccb90d9/zkevm-circuits/src/evm_circuit/execution/create.rs#L545
is_address_collision is boolean, which is not equal to the expr used to
construct the is_zero gadget?

### Issue Link


privacy-scaling-explorations#1639

### 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
### Description

current codes try to get offet/len inside handle_restore_context for
return/revert. When a stack underflow happens, parsing trace will fail.

This pr can fix testool `returndatacopy_after_failing_callcode_d0_g0_v0`

### Issue Link

[_link issue here_]

### 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
### Description

This PR merges the current milestone's light client PoC into main. Take
into consideration:

- The circuit is almost fished, although there are some small things
that can be to be improved
- There are some changes that maybe needs to be done in the go wrapper
(like sending proofs results in the wrapper call, instead of go code to
retrieve himself) that will requiere changes in the light client. This
is the main cause to currently disable tests since it requieres to
access to a RPC node. In a future changes will be mocked.
- There's still pending some tests passing from a local geth node and
mainnet blocks, when finished, the idea is to reuse some of the code for
the MPT tests, to prove that a set of mainnet block state transitions
can be successfully verified.

### Issue Link

-
privacy-scaling-explorations#1619
-
privacy-scaling-explorations#1543

### 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

- A circuit that verifies the state updates requiered to go from one
state root to another
- The witness generator for this circuit, that also, calls internally
the `mpt-witness-generator` wrapper.
- Tests to verify mainnet blocks
- Test to verify a local geth node with `eth_accessListByNumber` RPC
enabled
- A draft of a server to create proofs and serve them. Note that this
can be finished when local geth node tests will be passing.
- Upstreamed MPT golang witness generator changes from miha branch

---------

Co-authored-by: ChihChengLiang <chihchengliang@gmail.com>
Co-authored-by: adria0 <adria0@nowhere>
Co-authored-by: Miha Stopar <stopar.miha@gmail.com>
Co-authored-by: Eduard S <eduardsanou@posteo.net>
…1671)

### Description

[_PR description_]

### Issue Link

fix
privacy-scaling-explorations#1670

### 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?

[_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
…g-explorations#1669)

### Description

solve
privacy-scaling-explorations#1592

### 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_]

<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
…scaling-explorations#1676)

### Description

geth_step.error is not enough. It only contains error inside geth trace,
only stack err and oog err. Exec_step.error contains all types of err we
recovered during parsing trace.

This pr will fix some failures of testool, like
`CreateOOGFromCallRefunds_d1(SStore_Refund_OoG)_g0_v0`

### 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
### Description

this is a minor PR to remove duplicate entries for keccak circuit.

### Issue Link

[_link issue here_]

### 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
…privacy-scaling-explorations#1621)

### Description

Solve privacy-scaling-explorations#1513 by adding a testutil.rs and test cases

### Issue Link

privacy-scaling-explorations#1513 

### 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

- Add a unit test to buffer reader gadget in memory gadget and test
util.
- The testcases cover completeness and soundness.

### Rationale

I followed test util in `math_gadget` and the design of test container.
I removed lookup table part in the `test_util.rs` of math_gadget, since
buffer reader gadget didn't use lookup table.

### How Has This Been Tested?

`cargo test --package zkevm-circuits --features test-circuits --
evm_circuit::util::memory_gadget`

---------

Co-authored-by: nooma <suncloud2203357667@gmail.com>
### Description

Move SC compilation step to a build script. Rewrite deployment function
based on SC rust bindings generated with

[ethers_contract_abigen::Abigen](https://docs.rs/ethers-contract-abigen/latest/ethers_contract_abigen/)
and submit Txs for proof generation benchmarking.

### Issue Link

privacy-scaling-explorations#1557 

### Type of change

- New feature (non-breaking change which adds functionality)

### Contents

- build.rs : Compile smart contracts and generate rust bindings
- gen_blockchain_data: 
- Remove compilation step and rewrite fn deploy. New one uses the
bindings generated at build step to deploy SCs and also instantiates SC
instances to interact with.
- Create a dump TxTrace function [needed for testing future
implementations of benchmark contracts - needs attribute
allow(dead_code)]
- Submit Txs optimized for maximum calls of MLOAD and SDIV up to 300k
gas
### How Has This Been Tested?
```
https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6264433154
```


~~https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6234076847/job/16920593180~~
~~>> Workflow build step is currently failing~~

---------

Co-authored-by: ntampakas <nick@gmx.co.uk>
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
…-4896 withdrawals==None (privacy-scaling-explorations#1688)

### Description

Integration Tests have been failing since introduction of
privacy-scaling-explorations#1369,
due to Option::Unwrap on None value, when eth_block.withdrawals==None.

### 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

- zkevm-circuits/zkevm-circuits/src/witness/block.rs

### Rationale
replace eth_block.withdrawals.clone().unwrap() with
eth_block.withdrawals.clone().unwrap_or_default()

### How Has This Been Tested?

https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6968928576/job/18963801769
<hr>

### Issue

privacy-scaling-explorations#1687
### Description

This PR aims to get the benchmark tests running.

### Issue Link

privacy-scaling-explorations#1646

### 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

- Updates default degree in the benchmark tests

### Rationale

[_design decisions and extended information_]

### How Has This Been Tested?

```
cargo test --release --features=benches bytecode
cargo test --release --features=benches copy
cargo test --release --features=benches evm
cargo test --release --features=benches exp
cargo test --release --features=benches mpt
cargo test --release --features=benches keccak
cargo test --release --features=benches pi
cargo test --release --features=benches state
cargo test --release --features=benches super
cargo test --release --features=benches tx
```
### Description

Continuation of privacy-scaling-explorations#1261

### Issue Link

No issue link

### Type of change

New feature (non-breaking change which adds functionality)

---------

Co-authored-by: Aurélien Nicolas <info@nau.re>
### Description

This commit fixes privacy-scaling-explorations#1697 - excludes solidty from doc build

### Issue Link 

privacy-scaling-explorations#1697 

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

### Contents

- Exclude solidty in doc build

### How Has This Been Tested?

Running `cargo doc --no-deps --workspace --exclude integration-tests
`

cc: @ChihChengLiang

Signed-off-by: Shikhar Vashistha <51105234+shikharvashistha@users.noreply.github.com>
### Description

Resolves
privacy-scaling-explorations#1611
Upstreamed from taikoxyz#157 with
some additional changes.

Adds an additional execution state for transactions that are invalid and
need to be skipped. This execution state needs to be enabled instead of
the normal BeginTx/EndTx for valid transactions.

Support skipping the following invalid transactions for now:
- nonce too low
- nonce too high
- intrinsic gas too low
- insufficient funds for gas * price + value

More cases will be added in future PRs.

### Issue Link


privacy-scaling-explorations#1611

### 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?

- Added required unit tests
- make test-all passes

---------

Co-authored-by: Cecilia Zhang <zhangyixin319@gmail.com>
### Description

browsing for how to submit PR, found this typo **responsability**

### Issue Link

[_link issue here_]

### 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
### Description

This is a follow-up PR for
[PR#1396](privacy-scaling-explorations#1396)
from @roynalnaruto. After merging in 1396's changes to a fresh fork, the
branch is compared with Scroll's develop head to incorporate latest
patterns on precompile-related development.

### Issue Link

[Aggregating precompile issue
924](privacy-scaling-explorations#924)

### 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

### Design choices

See
[documentation](https://www.notion.so/scrollzkp/PR-1396-Follow-up-Precompile-Identity-for-Upstream-3624af055d9b4820a0354e9af4fa7918)

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Hello I am learning web3 programming, I went through some of the code
because I was told zk is fascinating (and it is). I found some little
errors in comments, nothing important. I hope you don't mind opening a
PR for such a little thing, I read the contributing file and didn't find
it being forbidden, but even if it is little I hope i'll help you even
just a bit in your research. Thank you for making that repo public. Have
a nice day.
### Description

This PR will replace old modified extension node PRs (there were
multiple because the witness generator was in a separate repository at
that time).

Support for modified extension nodes.

This happens when an extension node is replaced by another (shorter or
longer) extension node. For example, we have an extension node at `n1 n2
n3 n4 n5 n6` with branch with two leaves at positions `n` and `m`. If we
add a leaf at `n1 n2 n3 n4 n7` where `n5 != n7`, a new extension node is
inserted at `n1 n2 n3 n4` with a new branch with an extension node at
position `n5` (with only one nibble `n6`) and a leaf at position `n7`.
In this case, the S proof contains the extension node at `n1 n2 n3 n4 n5
n6` and no underlying branch and leaf (the modification happens at `n1
n2 n3 n4 n7` and only an extension node is find there), thus we need to
add a placeholder branch and a placeholder leaf.

### Type of change

- [x] New feature (non-breaking change which adds functionality)

---------

Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
…xplorations#1719)

### Description

This PR fix soundness by sharing txid write lookup between current step
and next step

### Issue Link

privacy-scaling-explorations#1707 

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

### Design rationale
Before fix, `cur_end_tx -> callcontext(txid, WRITE)` vs `next_begin_tx
-> callcontext(txid, WRITE)` are meant to constraint `next_tx_id =
cur_tx_id + 1`. However, because 2 callcontext write are different
records in rw_table, so actually second record are under-constraint and
tx_id can be manipulated.

Because so far we do not have negative test framework. So this PR just
simply adding a simple positive test to assure there is no consecutive
tx_id write with SAME tx_id.

If this fix make sense, will update zkevm-specs accordingly
https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/src/zkevm_specs/evm_circuit/execution/end_tx.py#L71-L79
curryrasul and others added 17 commits January 10, 2024 07:59
…-scaling-explorations#1723)

### Description

Change integration-tests/build.rs to fetch OZ submodule automatically.

### Type of change

- [X] Bug fix (non-breaking change which fixes an issue)

### Rationale

I propose to change
[integration-tests/build.rs](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/integration-tests/build.rs)
to automatically fetch OZ contracts submodule, so that initial
rust-analyzer indexation works without errors. Right now it's necessary
to fetch the submodule manually or by running
[integration-tests/run.sh](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/integration-tests/run.sh).

### How Has This Been Tested?

This feature is hard to test neither with unit-tests nor with
integration-tests, so it was tested locally, on a new & clean repo and
worked without any errors.

### Open question

A completed spawned process returns its `exit-status`/code, so possibly
it's a good tone to check if it's successful (exit code = 0); though the
command is clear and straightforward - that's why I decided not to do
that.

<hr>

### PR code explanation

This PR contains:
- Adding new error kind to `BuildError` enum:
```rust
/// Failed to pull OZ submodule from github
#[error("FailedToPullSubmodule({0:})")]
FailedToPullSubmodule(String),
```
- Process spawn to fetch submodule (the same as in
[integration-tests/run.sh](https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/integration-tests/run.sh#L94):
```rust
std::process::Command::new("git")
    .args([
        "submodule",
        "update",
        "--init",
        "--recursive",
        "--checkout",
        "contracts/vendor",
    ])
    .spawn()
    .and_then(|mut child| child.wait())
    .map_err(|err| BuildError::FailedToPullSubmodule(err.to_string()))?;
```
### Description

Had following built error on my Mac (MacOS 12.6.7, M1 Pro)
```
error[E0658]: use of unstable library feature 'stdsimd'
   --> /Users/kimi/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ahash-0.8.7/src/operations.rs:124:24
    |
124 |     let res = unsafe { vaesmcq_u8(vaeseq_u8(transmute!(value), transmute!(0u128))) };
    |                        ^^^^^^^^^^
    |
    = note: see issue #48556 <rust-lang/rust#48556> for more information
    = help: add `#![feature(stdsimd)]` to the crate attributes to enable

...
```

Had following error if only to downgrade `ahash` to `0.8.6`. 

```
error[E0432]: unresolved import `revm_precompile`
 --> bus-mapping/src/precompile.rs:7:5
  |
7 | use revm_precompile::{Precompile, PrecompileError, Precompiles};
  |     ^^^^^^^^^^^^^^^ use of undeclared crate or module `revm_precompile`
  |
```

Fixed by upgrade `revm_precompile` to `2.2.0`.

After upgrade `revm_precompile` to `2.2.0`, CI complained library
`libclang.so` missing. That's bcs `revm_precompile` `2.2.0` having
`c-kzg` feature which needs `libclang`. So in this fix, we also disable
default feature of `revm-precompile` (`default-features=false`).
### Issue Link

N/A

### 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

Pinning a fixed version is a better practice so this PR is not only
update the versions mentioned above but also using `=` to pin the
version.

---------

Co-authored-by: adria0.eth <5526331+adria0@users.noreply.github.com>
### Description

fix typos in test_ctx2.rs
…plorations#1730)

Find few typos while studying the code, in comments. Hope I can be of
any help, even if tiny.
Have a nice weekend.
…ations#1731)

### Description

Cargo defaults to `resolver = "2"` in edition 2021 only for singular
crates. Workspace tho defaults to the old `resolver = "1"` and will
override this setting for all crates in the workspace. [The
guideline](https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace)
is to manually set the `resolver = "2"` on workspace level.
This was documented recently and since a few releases produces a warning
on building. You don't see it due to the `rust-toolchain` setting.

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue) (not really a
bugfix but that's the closest match)
- [ ] 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

- specifies `resolver = "2"` on workspace level
- removes `resolver = "2"` from light-client-poc/Cargo.toml because it
was ignored anyway

This also fixes this warning:

```
warning: resolver for the non root package will be ignored, specify resolver at the workspace root:
package:   /data/bishop/zkevm-circuits/light-client-poc/Cargo.toml
workspace: /data/bishop/zkevm-circuits/Cargo.toml
```
Adds a comment so that it makes it clear what is RwTableTag when going
through the code.
…privacy-scaling-explorations#1733)

### Description

This change optimizes memory allocations in `circuit-tools`. Where
possible pre-allocates needed memory and removes some unneeded clones.

### Type of change

- [x] Perf optimization, refactor (non-breaking change which optimizes
code's performance / memory footprint)
- [ ] 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

- preallocating or bulk resizing with `.reserve()`, `.with_capacity()`
and `.resize()` or collecting
- some clones removals

### Rationale

This should increase performance a bit

### How Has This Been Tested?

```
cargo test -p zkevm-circuits
```
)

### Description

zkevm-circuits can be used in WASM to verify zkEVM proofs in the
browser.

While compiling zkevm-circuits crate for WASM target, the following
crate gives issues:
- revm-precompile: used by bus-mappings while generating witnesses
- snark-verifier: used in the root_circuit

Since use-case is to just verify proofs, `revm-precompile` dependency
can be removed during WASM compilation. And for SuperCircuit,
`snark-verifier` dependency can be removed. It should be easy to add
support for root_circuit too, by adding the WASM support to
`snark-verifier` crate such that it exposes required rust objects in
WASM mode.

### 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

- I tried to have this without using rust features, i.e. using target
arch conditional compilation, but toggling optional dependencies with
target arch seems to be very tricky. Hence, two features are introduced
`wasm` and `notwasm` (default).

### How Has This Been Tested?

Manual build:

```
cd zkevm-circuits
RUSTFLAGS='-C target-feature=+atomics,+bulk-memory'  cargo build --lib --no-default-features --features wasm  --target wasm32-unknown-unknown -Z build-std=panic_abort,std
```

And running the
[verifier](https://proofofexplo.it/verify/Qmek2Mo43HgFn3B6kjMHXBLznqbxyiyxMbTV9sYbJ4oKwE)
in the browser.

<!--
<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 responsibility

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: adria0.eth <5526331+adria0@users.noreply.github.com>
…ons#1725) (privacy-scaling-explorations#1739)

### Description

This PR is aimed to do minor style refactor/fix of privacy-scaling-explorations#1719

### Issue Link

privacy-scaling-explorations#1725 

### Type of change

- [X] Bug fix (non-breaking change which fixes an issue)

### Content

This PR addresses issue privacy-scaling-explorations#1725 to do a minor style fix, that @lispc
mentioned
[here](privacy-scaling-explorations#1719 (comment))
### Description

We removed the warning message shown during the project build.

```
warning: the following packages contain code that will be rejected by a future version of Rust: traitobject v0.1.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 2`
```
The message is caused by an unused dependency introduced during privacy-scaling-explorations#1615

### Issue Link

None

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

### Contents

- Removed some unused dependencies
- Removed some unused imports


### How Has This Been Tested?

Local build on all targets
### Description

This PR resolves the test failure in
privacy-scaling-explorations#1706.

The parameter `max_nodes` has been added to the MPT to have always the
same fixed column `q_enable`.

The fixed column `q_last` has been removed because it is not necessary -
if some nodes are added to the MPT proof after the account/storage leaf
to enable some non-valid lookups, the proof would fail because of
address/key RLC constraints (the RLC is being built node by node).

### Issue Link


privacy-scaling-explorations#1700

### 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
…vacy-scaling-explorations#1746)

### Description

While reviewing privacy-scaling-explorations#1699, I noticed some room to improve the ergonomics of
the words.

### Issue Link


### Type of change

New feature (non-breaking change which adds functionality)

### Contents

- Replaced the `address_word_to_expr` function with the `compress`
method so that we can compress a word directly without finding that
function to use.
- introduced `zero_f` and `one_f` to create zero and one for `Word<F>`.

### Rationale

I noticed that it is hard to create a `one()` and `zero()` method for
both `Word<Expression<F>>` and `Word<F>`. The compiler complains about
implementing duplicated methods. Since we already use `one()` and
`zero()` in many places for `Word<Expression<F>>`, I named `zero_f` and
`one_f` for the methods for `Word<F>`.
### Description

We add an initial structure for runtime config. In this PR, we plan to
add only the invalid tx configuration for the starter.

### Issue Link

privacy-scaling-explorations#1636 

### Type of change

New feature (non-breaking change which adds functionality)


### Decision

- Allow Geth to take invalid tx. The config doesn't affect geth util


### TODO

- [x] Fix invalid tx test
- [ ] Add tx validity check. (Will continue privacy-scaling-explorations#1740)
### Description

- halo2 (v2023_04_20 -> v0.3.0)
- halo2wrong (v2023_04_20 -> v2024_01_31)
- snark-verifier (v2023_04_20 -> v2024_01_31)
- remove halo2curves patching and fetch from `crate-io`, which got
latest release `0.6.0`

### Issue Link


privacy-scaling-explorations#1691

### Type of change
- [x] New feature (non-breaking change which adds functionality)
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-external-tracer Issues related to the external-tracer workspace member CI Issues related to the Continuous Integration mechanisms of the repository. crate-mock Issues related to the mock workspace member T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-eth-types Issues related to the eth-types workspace member crate-geth-utils Issues related to the geth-utils workspace member crate-gadgets Issues related to the gadgets workspace member labels Feb 2, 2024
@hero78119
Copy link
Member Author

Test passed, will close and update feature branch accordingly

@hero78119 hero78119 closed this Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI Issues related to the Continuous Integration mechanisms of the repository. crate-bus-mapping Issues related to the bus-mapping workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-eth-types Issues related to the eth-types workspace member crate-external-tracer Issues related to the external-tracer workspace member crate-gadgets Issues related to the gadgets workspace member crate-geth-utils Issues related to the geth-utils workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-mock Issues related to the mock workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-bench Type: benchmark improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.