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

Fix circuit benchmarks #1686

Merged

Conversation

zemse
Copy link
Member

@zemse zemse commented Nov 23, 2023

Description

This PR aims to get the benchmark tests running.

Issue Link

#1646

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

  • 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

@github-actions github-actions bot added T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member labels Nov 23, 2023
@zemse
Copy link
Member Author

zemse commented Nov 23, 2023

Two benchmark tests are failing: bytecode & copy from this PR. The following are errors that seem to be coming from dependencies:

---- bytecode_circuit::tests::bench_bytecode_circuit_prover stdout ----
Start:   Bytecode Circuit [Setup generation] with degree = 26
End:     Bytecode Circuit [Setup generation] with degree = 26 ......................1128.019s
thread 'bytecode_circuit::tests::bench_bytecode_circuit_prover' panicked at 'assertion failed: `(left == right)`
  left: `4`,
 right: `8`', /home/ubuntu/.cargo/git/checkouts/halo2-afe2d0b0be6b3c3a/be95568/halo2_proofs/src/poly/domain.rs:101:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- copy_circuit::tests::bench_copy_circuit_prover stdout ----
Start:   Copy Circuit [Setup generation] with degree = 14
End:     Copy Circuit [Setup generation] with degree = 14 ..........................589.546ms
Start:   Copy Circuit [Proof generation] with degree = 14
End:     Copy Circuit [Proof generation] with degree = 14 ..........................160.797ms
Start:   Copy Circuit [Proof verification]
thread 'copy_circuit::tests::bench_copy_circuit_prover' panicked at 'assertion failed: (input == 0u8) | (input == 1u8)', /Users/sohamzemse/.cargo/registry/src/index.crates.io-6f17d22bba15001f/subtle-2.5.0/src/lib.rs:230:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@ChihChengLiang ChihChengLiang self-requested a review November 27, 2023 09:49
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I tried to look into the two failing benchmarks and didn't find quick solutions.
Share you some of my initial thoughts.

@@ -31,7 +31,7 @@ mod tests {
let proof_gen_prfx = crate::constants::PROOFGEN_PREFIX;
let proof_ver_prfx = crate::constants::PROOFVER_PREFIX;
let degree: u32 = var("DEGREE")
.unwrap_or_else(|_| "15".to_string())
.unwrap_or_else(|_| "26".to_string())
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be that 26 is too big so that it triggers the Halo2 error. We can try tweaking somewhere else.

I changed line 52 from 2usize.pow(degree), to 2usize.pow(degree - 1), and got a ConstraintSystemFailure instead.

We can also create a new issue to specifically fix bytecode circuit benchmark.

Comment on lines +55 to +56
block.circuits_params.max_rws = 10_000;
block.circuits_params.max_copy_rows = 10_000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we have to calculate this to get a number that works. We can fix in the next PR.

@zemse zemse marked this pull request as ready for review November 28, 2023 15:50
@zemse
Copy link
Member Author

zemse commented Nov 28, 2023

@ChihChengLiang thanks for taking a look!

@zemse zemse force-pushed the fix-circuit-benchmarks branch from fac9b8d to af17779 Compare November 28, 2023 15:56
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM.
IIUC, after this PR, we should have the rest of the benchmarks working.
Bytecode and copy circuit remain failing and will be fixed in future PRs.

Copy link
Contributor

@rrtoledo rrtoledo left a comment

Choose a reason for hiding this comment

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

LGTM with the changes CC mentioned! :)

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Nov 29, 2023
@ChihChengLiang ChihChengLiang linked an issue Nov 29, 2023 that may be closed by this pull request
@ChihChengLiang
Copy link
Collaborator

I created #1694 and #1695 for the benchmarks to fix.

Merged via the queue into privacy-scaling-explorations:main with commit 5867b5f Nov 29, 2023
13 checks passed
@zemse zemse deleted the fix-circuit-benchmarks branch January 31, 2024 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member T-bench Type: benchmark improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix benchmarks
3 participants