fix(cancun): mcopy check offsets
#252
Conversation
Nashtare
changed the title
mcopy check offsets
github-actions
bot
added
the
crate: evm_arithmetization
Hm unless I'm missing something, for the wcopy syscalls a large |
| DUP2 | ||
| %ensure_offset_in_range | ||
| DUP3 | ||
| %ensure_offset_in_range |
There was a problem hiding this comment.
Not directly related to this PR, but I noticed we call %update_mem_bytes only on dest_offset+size below.
My understanding of the EIP, especially the line
If length > 0 and (src + length or dst + length) is beyond the current memory length, the memory is extended with respective gas cost applied.
would imply we also need to call it with offset+size.
There was a problem hiding this comment.
Hmm actually I think this would also be handling the error in that particular txn case. Will update the PR to have it.
There was a problem hiding this comment.
I also think there may be issues with the current mcopy_with_overlap. I don't think the current impl is valid if SRC < DST and DST - SRC < min(32, size). To alleviate it without too much overhead, I guess we could simply have a %memcpy_bytes_reversed macro that processes the copy backwards, starting from the last offset to read / write from and decrementing both DST, SRC and size.
There was a problem hiding this comment.
Would also like confirmation on this one! (Other MCOPY PR got merged into this branch)
Noticed while working with John's edge case txns.
We aren't checking for valid memory offsets, which would then cause the prover to panic and abort when performing the mem_cpy.
@wborgeaud I noticed we aren't performing this check for the other syscalls of the
wcopyfamily. Don't you think we should?Makes 4th txn of witness-0233.json pass.