Bytecode unroller #170
Bytecode unroller #170
Conversation
github-actions
bot
added
T-opcode
|
I would suggest renaming the folder |
Brechtpd
force-pushed
the
bytecode-unroller
branch
from
be4f1c0
to
525fda4
Compare
Brechtpd
force-pushed
the
bytecode-unroller
branch
from
525fda4
to
7e5bdb1
Compare
There was a problem hiding this comment.
Nice!
Sorry for not reviewing this PR earlier! It looks really good to me and can be merged after rebase from my point of view. At first I thought padding could be set to 0 in is_final or before, but then I saw you have a constraint for padding to be 0 when is_final, so all good!
| meta.query_selector(q_enable) | ||
| * not::expr(meta.query_fixed(q_first, Rotation::cur())) | ||
| }, | ||
| |meta| meta.query_advice(push_rindex, Rotation::prev()), |
There was a problem hiding this comment.
one question: why not current rotation meta.query_advice(push_rindex, Rotation::cur()) when checking push_rindex?
There was a problem hiding this comment.
Certainly possible to do it in different ways, but I think always necessary to check 2 rows for some column because inherently we do not have enough data on a single row (need some history to handle the PUSH data). With how it currently works, the push_rindex == 0 check is done against the previous row so that push_rindex of the current row can store the rindex for the byte in the current row (push_rindex := is_code ? byte_push_size : push_rindex_prev - 1). It's also possible I guess to check the value in the current row, and write the value in the next row, which is kind of the same thing but I think complicates things somewhat (because we then need special handling of the last byte which cannot use a value for the next row).
There was a problem hiding this comment.
sound good to me, thanks !
| meta.query_advice(index, Rotation::cur()), | ||
| meta.query_advice(index, Rotation::prev()) + 1u64.expr(), | ||
| ); | ||
| // `is_code := push_rindex_prev == 0` |
There was a problem hiding this comment.
how about is_code := push_rindex == 0 ?
There was a problem hiding this comment.
Same explanation as above I think.
|
@Brechtpd Can we do a rebase for this? |
| bytecodes.iter().map(|v| v.bytes.clone()).enumerate() | ||
| { | ||
| let hash: F = keccak(&bytecode[..], self.r); | ||
| let rlc: F = linear_combine(bytecode.clone(), self.r); |
There was a problem hiding this comment.
one more: keccak table validity(hash correctness) will be checked elsewhere like RW_table, Tx_table, right?
There was a problem hiding this comment.
That's right, will be checked in the keccak circuit.
Brechtpd
force-pushed
the
bytecode-unroller
branch
from
7e5bdb1
to
506fadc
Compare
Done. The latest |
|
@Brechtpd Merged, thank you for the epic work! |
Spec: privacy-scaling-explorations/zkevm-specs#74
Disabled the
bytecode_invalid_indexfor now because with the halo update it now gives an internal error on a circuit that should fail:panicked at 'internal error: entered unreachable code', /home/brecht/.cargo/git/checkouts/halo2-b8251ca10f182e2b/b78c39c/src/dev/util.rs:48:42. With the previous halo this test worked fine so not sure what's going on yet.The constraint builder could be modified a bit more to make the circuit writing for these standalone circuits even a bit nicer, but Han's PR already does a couple of good changes so will await that one instead before doing so.