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

Fix benchmarks #1646

Closed
ed255 opened this issue Oct 5, 2023 · 3 comments · Fixed by #1686
Closed

Fix benchmarks #1646

ed255 opened this issue Oct 5, 2023 · 3 comments · Fixed by #1686
Assignees
Labels
good first issue Good for newcomers

Comments

@ed255
Copy link
Member

ed255 commented Oct 5, 2023

In https://github.com/privacy-scaling-explorations/zkevm-circuits/tree/main/circuit-benchmarks/src we have a series of tests to build a proof of each subcircuit to get benchmark results.
These benchmarks have been broken for a while and need to be fixed.

Example:

$ DEGREE=10 cargo test --features=benches state_circuit::tests::bench_state_circuit_prover
warning: `zkevm-circuits` (lib) generated 1 warning
    Finished test [unoptimized + debuginfo] target(s) in 0.23s
     Running unittests src/lib.rs (/mnt/data/dev/git/zkevm/zkevm-circuits/target/debug/deps/circuit_benchmarks-6d442a10c45f553e)

running 1 test
test state_circuit::tests::bench_state_circuit_prover ... FAILED

failures:

---- state_circuit::tests::bench_state_circuit_prover stdout ----
Start:   State Circuit [Setup generation] with degree = 10
End:     State Circuit [Setup generation] with degree = 10 .........................1.323s
thread 'state_circuit::tests::bench_state_circuit_prover' panicked at 'keygen_vk should not fail: NotEnoughRowsAvailable { current_k: 10 }', circuit-benchmarks/src/state_circuit.rs:58:61
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    state_circuit::tests::bench_state_circuit_prover

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 9 filtered out; finished in 1.33s

error: test failed, to rerun pass `--lib`
@ed255 ed255 added the good first issue Good for newcomers label Oct 5, 2023
@zemse
Copy link
Member

zemse commented Oct 16, 2023

Since StateCircuit uses the U16 table, it seems to me that the degree should have to be at least 16. Should the fix simply add a minimum default value for the DEGREE env similar to other tests?

How are the benchmark tests triggered? Is it run individually passing in the DEGREE manually or using cargo test inside circuit-benchmarks?

circuit-benchmarks $ cargo test --features=benches

@ed255
Copy link
Member Author

ed255 commented Oct 18, 2023

Since StateCircuit uses the U16 table, it seems to me that the degree should have to be at least 16. Should the fix simply add a minimum default value for the DEGREE env similar to other tests?

That makes sense. Your suggestion would be a possible solution. Another solution that comes to my mind is to just check if DEGREE is >= 16 for the StateCircuit benchmark, and if not panic early with a message. Also the same process should be done for the other benchmakrs: check if the other circuits have a minimum degree, and either panic with a useful message when degree is lower, and/or set a default degree that is the minimum.

How are the benchmark tests triggered? Is it run individually passing in the DEGREE manually or using cargo test inside circuit-benchmarks?

circuit-benchmarks $ cargo test --features=benches

You can trigger them with this command from the circuit-benchmarks directory:

DEGREE=10 cargo test --features=benches state_circuit::tests

You can replace state_circuit by any of the implemented benchmarks which are:

  • bytecode_circuit
  • copy_circuit
  • evm_circuit
  • exp_circuit
  • mpt_circuit
  • packed_multi_keccak
  • pi_circuit
  • state_circuit
  • super_circuit
  • tx_circuit

@ChihChengLiang
Copy link
Collaborator

Assign this task to @zemse

@zemse zemse mentioned this issue Nov 23, 2023
4 tasks
@ChihChengLiang ChihChengLiang linked a pull request Nov 29, 2023 that will close this issue
4 tasks
github-merge-queue bot pushed a commit that referenced this issue Nov 29, 2023
### Description

This PR aims to get the benchmark tests running.

### Issue Link

#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
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants