feat: Add INCR privileged instructions
#734
Conversation
Nashtare
requested review from
wborgeaud,
muursh and
LindaGuiga
as code owners
github-actions
bot
added
crate: evm_arithmetization
| # Circuit sizes only matter in non test_only mode. | ||
| if ! [[ $8 == "test_only" ]]; then | ||
| export ARITHMETIC_CIRCUIT_SIZE="16..21" | ||
| export BYTE_PACKING_CIRCUIT_SIZE="8..21" | ||
| export CPU_CIRCUIT_SIZE="8..21" | ||
| export KECCAK_CIRCUIT_SIZE="4..20" | ||
| export KECCAK_SPONGE_CIRCUIT_SIZE="8..17" | ||
| export LOGIC_CIRCUIT_SIZE="4..21" | ||
| export MEMORY_CIRCUIT_SIZE="17..24" | ||
| export MEMORY_BEFORE_CIRCUIT_SIZE="16..23" | ||
| export MEMORY_AFTER_CIRCUIT_SIZE="7..23" | ||
| fi |
There was a problem hiding this comment.
these match the default ones in .env
| @@ -597,6 +599,39 @@ pub(crate) fn generate_swap<F: RichField, T: Transition<F>>( | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub(crate) fn generate_incr<F: RichField, T: Transition<F>>( | |||
There was a problem hiding this comment.
I'm actually not sure why it's working. For INCR2-4, there's no problem, but for INCR1 we are reading the stack (and writing) at address stack_len - 1. There is no guarantee that the current top of the stack has been written in memory, so I'm surprised the reads don't return a wrong value sometimes.
There was a problem hiding this comment.
Some constraints seem to be missing, and some seem to be unneeded.
The current set of constraints work as intended for INCR2-4, but for INCR1 we are not checking that the output channel is equal to the next top of the stack.
Moreover, the value read in the input channel is not constrained to match the current top of the stack (tests pass so it seems to be the case, but it sounds like coincidence to me).
I think the clean way to do it is to filter all of the current constraints with lv.opcode_bits[0] (with a new one making sure that the top of the stack doesn't change), and handle INCR1 separately with filter 1 - lv.opcode_bits[0] (you can even disable the memory channels to save some memory rows).
There was a problem hiding this comment.
handle INCR1 separately with filter 1 - lv.opcode_bits[0]
This would be catching INCR3 too, which wouldn't work. If I use both bits I'll get a degree 4, but I can introduce helper columns. I'll look at it.
|
@hratoanina @LindaGuiga I've removed the memory ops for INCR1, moving the extra checks on prev/new stack tops with an extra CTL against arithmetic. Let me know if I forgot anything. |
Nashtare
requested review from
hratoanina and
LindaGuiga
and removed request for
hratoanina
|
|
||
| // Constrain the helper column | ||
| yield_constr.constraint( | ||
| base_filter |
There was a problem hiding this comment.
I think it's rather base_filter * (lv.general.incr().is_not_incr1 - lv.opcode_bits[0] * lv.opcode_bits[1]).
There was a problem hiding this comment.
A possible optimization to avoid introducing an extra CTL just for INCR1 is to keep the same arithmetic CTL for all cases, with memory channels 1 and 2. Then you add INCR1-specific constraints to check that lv.stack_top == mem_channel[1] and nv.stack_top == mem_channel[2].
It requires using the value columns of some disable channels, but I couldn't find anywhere in the code that it wasn't possible.
Add a series of 4
INCRprivileged instructions (INCR1,INCR2,INCR3andINCR4) to increment by 1 the Nth element of the stack in place (i.e. noPUSH/POP).Particularly helpful for accumulators increment previously requiring
SWAPN PUSH 1 ADD SWAPNnow only requiringINCRN.Though having an overall lesser impact, we could see how interesting a DECR variant would be (we could add it at no cost by combining it with the INCR CPU column).
Removes 4% to 5% of CPU cycles on mainnet blocks.
Total CPU columns for vanilla type1: 86
MemBeforenew initial size:63199type2:62691