fix(cancun): revamp MCOPY with overlapping
#254
Conversation
github-actions
bot
added
the
crate: evm_arithmetization
| // stack: count, DST, SRC, count, retdest | ||
| %lt_const(0x21) | ||
| // stack: count <= 32, DST, SRC, count, retdest | ||
| %jumpi(memcpy_bytes_finish) |
There was a problem hiding this comment.
Won't memcpy_bytes_finish copy the first count bytes instead of the last count bytes here?
There was a problem hiding this comment.
From my understanding it shouldn't. Small sequences are not using this %memcpy_bytes_backwards macro (see last comment in sys_mcopy code). For larger sequences (once the below DST / SRC update is fixed as per your comment), then we should enter this last iteration with the correct final offsets and the remaining count being size % 32.
| %sub_const(0x40) | ||
| // Decrement SRC by 32. | ||
| SWAP1 | ||
| %sub_const(0x20) |
There was a problem hiding this comment.
These subtraction can "underflow" when the offset of SRC or the original DST are <32.
There was a problem hiding this comment.
Ah yeah good catch
| let post_memory = hex!("0000010203040506070000000000000000000000000000000000000000000000"); | ||
|
|
||
| assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok()) | ||
| } |
There was a problem hiding this comment.
The test
#[test]
fn test() {
let dest_offset = 1;
let offset = 0;
let size = 33;
let pre_memory = hex!("000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f2021222324252627");
let post_memory = hex!("00000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20222324252627");
assert!(test_mcopy(dest_offset, offset, size, &pre_memory, &post_memory).is_ok())
}currently fails due to the issues mentioned above. Maybe we can add it here.
There was a problem hiding this comment.
Yeah good point. The EIP specifies a wider set of tests for all edge cases, could be worth adding a few others as well.
|
@wborgeaud let me know if this looks good to you now! |
* fix: check invalid offsets * Update per Will's comment * Minor * fix(cancun): revamp `MCOPY` with overlapping (#254)
cf comment in #252 (comment): this handles properly cases of overlapping with risk of overwrite.
Also adds some tests from the EIP