-
Notifications
You must be signed in to change notification settings - Fork 808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Two initial checks added #1
Two initial checks added #1
Conversation
src/state_circuit/memory.rs
Outdated
let bus_mapping = | ||
self.step(&mut region, offset + step_offset, address, step)?; | ||
for step in op.steps.iter() { | ||
let bus_mapping = self.step(&mut region, offset, address, step)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't need the extra + step_offset
. Thanks for the catch!
src/state_circuit/memory.rs
Outdated
let q_read = { | ||
let one = Expression::Constant(F::one()); | ||
one - flag.clone() | ||
}; | ||
|
||
// step_prev < step_cur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we would use the is_zero
gadget + the monotonicity gadget.
// step_prev < step_cur | |
// If address_prev == address_cur, step_prev < step_cur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks, I will change it. BTW, shall I rename step into global_counter everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i think renaming step
-> global_counter
would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, just renamed it. We don't need NUM_STEPS, right?
src/state_circuit/memory.rs
Outdated
let q_read = { | ||
let one = Expression::Constant(F::one()); | ||
one - flag.clone() | ||
}; | ||
|
||
// step_prev < step_cur | ||
// TODO: Figure out how to enforce this. Suggestion: lookup step_cur, step_prev, | ||
// and their difference `step_cur - step_prev` in a table. | ||
|
||
// TODO: address[i] == address[i + 1] == ... == address[i + NUM_STEPS - 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Monotonicity check.
// TODO: address[i] == address[i + 1] == ... == address[i + NUM_STEPS - 1] | |
// TODO: if address_prev != address_cur, address_prev < address_cur |
Agreed with @therealyingtong to merge this PR. |
### Description Keccak circuit logs messages that make CI logs hard to reason. We can set it from "INFO" level to "DEBUG" level to suppress the messages in the CI output but still keep it for develoepers. https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/4955284960/jobs/8864549606#step:13:50 <details><summary>Details</summary> <p> ``` test mock_prover::serial_test_exp_circuit_multiple_transfers_0 ... ok [2023-05-12T05:00:22Z INFO integration_tests::integration_test_circuits] test Keccak circuit, block: #1 - Transfer 0 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] - Post absorb: [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Lookups: 2 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Columns: 7 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] - Post padding: [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Lookups: 1 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Columns: 10 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] - Post theta: [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Lookups: 14 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Columns: 38 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] - Post rho/pi: [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Lookups: 53 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Columns: 191 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] - Post chi: [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Lookups: [52](https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/4955284960/jobs/8864549606#step:13:53) [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Columns: 195 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] - Post squeeze: [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Lookups: 1 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Columns: 198 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Degree: 4 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Minimum rows: [61](https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/4955284960/jobs/8864549606#step:13:62) [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Total Lookups: 123 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] Total Columns: 198 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] num unused cells: 229 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] part_size absorb: 4 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] part_size theta: 2 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] part_size theta c: 2 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] part_size theta t: 3 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] part_size rho/pi: 3 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] part_size chi base: 3 [2023-05-12T05:00:22Z INFO zkevm_circuits::keccak_circuit] uniform part sizes: [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2] ``` </p> </details> ### Issue Link ### 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
Two memory checks added - whether the initial state is ok and whether the value that is read is the same as the value that was written at the same address.