Consensys / corset

14 stars 13 forks source link

Option to display module specific instruction names #252

Open OlivierBBB opened 3 months ago

OlivierBBB commented 3 months ago

The following modules MMU / MMIO / OOB / EXP / ... have their own "instruction sets" independent of the EVM's instruction set, see the respective specifications. These are typically 2 byte integers (to differentiate them from the 1 byte opcodes of the EVM.) It would be great help if we could annotate these columns in corset to display the associated instruction names in corset inspect. The current state of things makes it hard to deciper what is going on.

Here is an example from the MMU module. The MMU module has 3 "instruction" columns

The stuff below is supposed to represent the sequence of MMU instructions the HUB creates in response to dealing with KECCAK, MSTORE, KECCAK, MSTORE, LOG2, REVERT (with LOG2 deliberately not drawn due to reverting and REVERT missing ~for reasons I'm still investigating~ due to this being the root context of the transaction.)

image
letypequividelespoubelles commented 3 months ago

we introduced LEQ and GEQ for WCP too. He's not dealing with only EVM's instructions.

OlivierBBB commented 3 months ago

@letypequividelespoubelles yes

DavePearce commented 2 months ago

So, currently there is a display option :opcode. You're thinking of something like that?

OlivierBBB commented 2 days ago

Yes. For instance we have special "opcodes" for OOB and MMU. E.g.

OOB_INST_CALL            <-->  0x ca
OOB_INST_SHA2            <-->  0x ff 02
MMU_INST_BLAKE2F_PARAMS  <-->  <some value I don't recall>
MMIO_INST_mload          <-->  <some value I don't recall>

E.g. when we do a STATICCALL to the SHA2-256 precompile we see both 0x ca and 0x ff 02 rather than the expected OOB_INST_CALL and OOB_INST_SHA2.

image

It would be nicer if we could display e.g. OOB_INST_CALL etc ... rather than these cryptic pseudo opcodes. Also since the LHS of the screen displays the column name (e.g. misc/OOB_INST, misc/MMU_INST, misc/EXP_INST, misc/MXP_INST) we don't need to include the XXX_INST_ prefix in the display name.