optimize memory recording

[h3lio5]

Record memory only when necessary, i.e., the opcode modifies the memory.

Ported the modifies_memory!() macro from foundry

Fixes #64

[mattsse]

okay final request, is the type changes to Bytes because Bytes is cheap to clone, unlike Vec

and we should add a one line comment (see foundry code) that explains why we're using the previous step's memory

[h3lio5]

if we change the type to Bytes, we may need to change some methods of RecordedMemory also no? Like we wouldn't need the resize(), len(), memory_chunks() methods.

[mattsse]

hmm, good point, I need to do some digging first and find out why we're resizing or whether this is even correct...

[mattsse]

lgtm, tysm!

I need to check the expected behaviour of vmtrace before merging

[h3lio5]

not relevant to this issue but can you please suggest me where I can learn more in-depth about call traces, revm-inspectors design and how / where is it used (applications)?

[mattsse]

you can checkout the trace_ debug_ endpoints in reth:

https://github.com/paradigmxyz/reth/blob/71c404d9cb7a678d4c0f6442a9c2b8449529fe2a/crates/rpc/rpc/src/trace.rs#L41-L41

https://github.com/paradigmxyz/reth/blob/71c404d9cb7a678d4c0f6442a9c2b8449529fe2a/crates/rpc/rpc/src/debug.rs#L44-L44

[mattsse]

okay @h3lio5 I did some digging and it looks like, for geth memory is recorded before the expansion which we also do

https://github.com/ethereum/go-ethereum/blob/767b00b0b514771a663f3362dd0310fc28d40c25/core/vm/interpreter.go#L262-L274

however, this means, that this is now a bit more difficult to track, because we can only reuse the previous step's memory if the previous step does not modify memory.

for the parity tracer I think we can optimize the memory delta computation separately, I believe this is currently implemented wrong:
https://github.com/paradigmxyz/evm-inspectors/blob/main/src/tracing/builder/parity.rs#L388-L388
I assume this should only track expanded memory? which we could track separately in RecordedMemory on step_end

[h3lio5]

@mattsse Maybe I'm missing the point here, could you please clarify why we cannot reuse the previous step's memory if it modifies memory?

iiuc, the memory field in CallTraceSteptracks the state of memory at that step, if the previous step modified memory and the current step doesn't modify it then the state of the memory should remain unchanged no, i.e., copy prev step memory?

edit: ah, I get it now. The previous step memory snapshot doesn't capture the expanded memory (does tracing before expansion), so we cannot reuse the prev memory?

[h3lio5]

I think we can fetch the prev step's opcode (if it isn't the first step in the trace) using trace.trace.steps.get(trace.trace.steps.len() - 1), if it does not modify memory then reuse it's memory, otherwise get the shared_memory.context_memory(). Do you think this approach might work? @mattsse

[mattsse]

Do you think this approach might work?

that works yes

why we cannot reuse the previous step's memory if it modifies memory?

we need the memory when the opcode is executed, before memory expansion, when the step_start is invoked, the opcode hasn't been executed yet, so same memory as the after the previous opcode.

edit: ah, I get it now. The previous step memory snapshot doesn't capture the expanded memory (does tracing before expansion), so we cannot reuse the prev memory?

exactly

[mattsse]

haven't forgotten about this, just need to run a few more cross client tests...

[DaniPopes]

Verified manually with the forge debugger, i removed the resizing because i don't think it's worth having, this will slightly modify its behavior because loads/stores won't be highlighted if they also expand memory

I also fixed the logic because the step memory is from the previous opcode, so we have to check modifies_memory only on the previous step.