Combine mload_32bytes and mstore_32bytes flags #1252
Conversation
evm/src/cpu/stack.rs
Outdated
| pushes: true, | ||
| disable_other_channels: false, | ||
| }), | ||
| memop_32bytes: None, |
There was a problem hiding this comment.
I'd add a comment to say where these constraints do live (if only to make clear that they haven't been forgotten).
| global mstore_unpacking: | ||
| // stack: context, segment, offset, value, len, retdest | ||
| %stack(context, segment, offset, value, len, retdest) -> (context, segment, offset, value, len, offset, len, retdest) | ||
| // stack: context, segment, offset, value, len, offset, len, retdest | ||
| %stack(context, segment, offset, value, len, retdest) -> (context, segment, offset, len, value, offset, len, retdest) | ||
| // stack: context, segment, offset, len, value, offset, len, retdest |
There was a problem hiding this comment.
This makes the argument order of mstore_unpacking inconsistent with MSTORE_32BYTES. I worry that it could become confusing.
In how many places is mstore_unpacking used? Would it be a lot of work to fix the argument order in those places as well?
| pub fn ctl_data_byte32<F: Field>() -> Vec<Column<F>> { | ||
| ctl_data_keccak_sponge() | ||
| } |
There was a problem hiding this comment.
nit: as we're re-using ctl_data_keccak_sponge both for MLOAD_32BYTES and MSTORE_32BYTES, perhaps a tiny comment explaining the distinction may be useful? (as the doc there is valid for MLOAD but the last memory channel is being read in the case of MSTORE, not pushed to).
|
Kudos, SonarCloud Quality Gate passed!
|
|
Given all the changes introduced recently, it seems impossible (for now at least) to merge the two flags given the current state of things. Indeed, removing one memory channel implied changing the CTL for |
pgebheim
modified the milestones:
v0.1.0-rc2 - Optimizations / Documentation,
v0.1.0-rc1 - VM Audit Readiness / Bugfixes
This PR combines the two flags:
mload_32bytesandmstore_32bytes, as well as the two associated CTLs.This required changing the order of the stack for
MSTORE_32BYTES.