feat(cancun): EIP-5656: MCOPY - Memory copying instruction #528

0xJepsen · 2023-06-20T18:39:39Z

Started working on this. Still needs work.

Will Close #523

crates/interpreter/src/instructions/memory.rs

rakita · 2023-06-21T12:48:25Z

crates/interpreter/src/instructions/memory.rs

+        InstructionResult::InvalidOperandOOG
+    );
+    let (data_end, overflow) = dest.overflowing_add(len);
+    if overflow || data_end > interpreter.return_data_buffer.len() {


wait silly question but isn't mcopy just copying data from memory to memory and not from return data buffer.

You should have memory_resize somewhere here

Yeah this is correct.

rakita · 2023-06-21T12:49:02Z

crates/interpreter/src/instructions/memory.rs

+        interpreter.instruction_result = InstructionResult::OutOfOffset;
+        return;
+    }
+    interpreter.memory.set_data(src, dest, len, &interpreter.contract.input);


And here we are coping the contract.input that is different from the thing we check above

Yep this is right, i read the memory from slice to pass the right data here

rakita

left few comments

0xJepsen · 2023-06-21T14:02:35Z

left few comments

Awesome thanks for the comments. Will work some more on this today

rakita · 2023-06-22T11:15:08Z

crates/interpreter/src/instructions/memory.rs

+        InstructionResult::InvalidOperandOOG
+    );
+    // Read data with length len from src
+    let data = interpreter.memory.clone();


Cloning memory is a no go, it is expensive.

You could probably add copy function to memory that would done copying. Just figure out how to do in in place with vec

rakita · 2023-06-22T11:16:48Z

crates/interpreter/src/instructions/memory.rs

+        dest,
+        InstructionResult::InvalidOperandOOG
+    );
+    memory_resize!(interpreter, dest, len);


I would assume that both dest and src can increment memory. So you need to find what is bigger and increment memory by a bigger value

I made a function to check and handle this memory_copy_resize that was inspired by the geth pr's implementation in the memory_table.go

tynes · 2023-06-23T17:56:56Z

crates/interpreter/src/interpreter/memory.rs

+    /// Copies memory inplace to a returns a [u8]
+    #[inline(always)]
+    pub fn copy(&self, offset: usize, size: usize) -> [u8; 32] {
+        let mut copy = [0u8; 32];


This looks like copy_word because copy is only 32, it should be size

addressed this with returning a vec of bytes and then passing as a slice to the data

0xJepsen · 2023-06-24T00:07:23Z

So the thing i want to try and fix now is that the memory_copy_resize has some nasty unwraps because the peek returns a result. I thought to use the peek because its how i was reading the Back method in the go implementation. But im not sure we have a similar implementation. I could try to write a something with similar functionality. What do you think?

0xJepsen · 2023-06-24T00:15:58Z

So the thing i want to try and fix now is that the memory_copy_resize has some nasty unwraps because the peek returns a result. I thought to use the peek because its how i was reading the Back method in the go implementation. But im not sure we have a similar implementation. I could try to write a something with similar functionality. What do you think?

In the geth implementation they validate the stack length in the interpreter here
https://github.com/ethereum/go-ethereum/blob/942ba4ddaa0e2591b77a42f40059babbaaa44154/core/vm/interpreter.go#L183C1-L187C4

Where as we are doing it in the peek() implementation which is why we get a result there

rakita · 2023-06-26T09:38:25Z

crates/interpreter/src/instructions/memory.rs

+    interpreter.memory.set_data(src, dest, len, data);
+}
+
+pub fn memory_copy_resize(interpreter: &mut Interpreter, _host: &mut dyn Host) {


This is faulty, and you already have dest,src, len in fn mcopy so this fn is not needed. What is needed is to memory_resize

rakita · 2023-06-26T09:43:39Z

crates/interpreter/src/instructions/memory.rs

+
+    let src = as_usize_or_fail!(interpreter, src, InstructionResult::InvalidOperandOOG);
+    // Read data with length len from src
+    let binding = interpreter.memory.copy_to_vec(src, len);


It is not optimal to create temporary vec. There is a memory operation from C that does copying of overlapping data so I assume there is something similar in rust. Make memory.copy that does internal copying of data.

I found and used copy_within
https://doc.rust-lang.org/std/ptr/fn.copy.html

This works perfectly

rakita · 2023-06-26T09:47:45Z

crates/interpreter/src/instructions/memory.rs

+pub fn mcopy<SPEC: Spec>(interpreter: &mut Interpreter, host: &mut dyn Host) {
+    check!(interpreter, SPEC::enabled(CANCUN));
+    pop!(interpreter, dest, src, len);
+    if len == U256::ZERO {


This is not correct, even if len is zero, some gas needs to be deduced.
maybe it is best to check if len == 0 after [gas_or_fail](gas::verylowcopy_cost) is added.

crates/interpreter/src/instructions/memory.rs

rakita · 2023-06-27T07:41:15Z

crates/interpreter/src/interpreter/memory.rs

}
-

nit: add new line here.

crates/interpreter/src/interpreter/memory.rs

crates/interpreter/src/instructions/memory.rs

rakita · 2023-06-27T08:08:34Z

I think we are close!

Co-authored-by: rakita <[email protected]>

0xJepsen · 2023-06-27T21:17:18Z

I think we are close!

Awesome, thanks for all the feedback! it's been really helpful in learning all the details here. Let me know if there are any more problems you see here.

rakita · 2023-06-28T07:40:40Z

I think we are close!

Awesome, thanks for all the feedback! it's been really helpful in learning all the details here. Let me know if there are any more problems you see here.

Can you check why eth tests are failing? And can you write some tests for this

0xJepsen · 2023-06-30T15:15:09Z

Wrote some tests and got CI passing!

crates/interpreter/src/instructions/memory.rs

rakita

gj!

0xJepsen added 2 commits June 20, 2023 12:38

first pass

6537323

added mcopy const to instruction.rs

02d1480

rakita reviewed Jun 21, 2023

View reviewed changes

crates/interpreter/src/instructions/memory.rs Show resolved Hide resolved

rakita reviewed Jun 21, 2023

View reviewed changes

rakita changed the title ~~first pass~~ feat(cancun): EIP-5656: MCOPY - Memory copying instruction Jun 21, 2023

I think this is correct but this clone() on memory is not good

a1df5a3

rakita reviewed Jun 22, 2023

View reviewed changes

fix the copy clone

3615f07

tynes reviewed Jun 23, 2023

View reviewed changes

resize + copy in place

01e2841

rakita reviewed Jun 26, 2023

View reviewed changes

0xJepsen added 3 commits June 26, 2023 18:33

Memory resize + in memory copy

3a257ca

fmt

e490c74

remove copy to vec

be43cca

rakita reviewed Jun 27, 2023

View reviewed changes

crates/interpreter/src/instructions/memory.rs Outdated Show resolved Hide resolved

rakita reviewed Jun 27, 2023

View reviewed changes

crates/interpreter/src/instructions/memory.rs Outdated Show resolved Hide resolved

rakita reviewed Jun 27, 2023

View reviewed changes

crates/interpreter/src/interpreter/memory.rs Outdated

Comment on lines 68 to 69

}

Copy link

Member

rakita Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add new line here.

rakita reviewed Jun 27, 2023

View reviewed changes

crates/interpreter/src/interpreter/memory.rs Outdated Show resolved Hide resolved

rakita reviewed Jun 27, 2023

View reviewed changes

crates/interpreter/src/instructions/memory.rs Outdated Show resolved Hide resolved

0xJepsen and others added 2 commits June 27, 2023 15:33

Update crates/interpreter/src/instructions/memory.rs

fc0cb3a

Co-authored-by: rakita <[email protected]>

Update crates/interpreter/src/instructions/memory.rs

71d9eca

Co-authored-by: rakita <[email protected]>

0xJepsen and others added 3 commits June 27, 2023 15:34

Update crates/interpreter/src/interpreter/memory.rs

9bf0669

Co-authored-by: rakita <[email protected]>

Update crates/interpreter/src/instructions/memory.rs

2fd74c6

Co-authored-by: rakita <[email protected]>

fmt + max import

97b4ed9

0xJepsen mentioned this pull request Jun 27, 2023

EIP-4844: Shard Blob Transactions #526

Closed

specID

8fd3de5

0xJepsen force-pushed the waylon/EIP5656MCOPY branch from 914a46c to 8fd3de5 Compare June 29, 2023 02:08

0xJepsen and others added 3 commits June 29, 2023 18:11

cargo update + memory test

576fa1c

Merge branch 'main' into waylon/EIP5656MCOPY

2742804

hashbrown duplicate

a54a86e

0xJepsen marked this pull request as ready for review June 30, 2023 00:25

Satisfy nit

53ca642

rakita reviewed Jun 30, 2023

View reviewed changes

crates/interpreter/src/instructions/memory.rs Outdated Show resolved Hide resolved

Update crates/interpreter/src/instructions/memory.rs

10a91d3

rakita approved these changes Jun 30, 2023

View reviewed changes

rakita merged commit c153428 into bluealloy:main Jul 1, 2023

This was referenced Jul 31, 2023

fix(interpreter): mcopy call order #570

Merged

fix(mcopy): memory expansion and add eth tests to ci #586

Merged

rakita mentioned this pull request Aug 10, 2023

Ethereum hardfork: Cancun #527

Closed

5 tasks

github-actions bot mentioned this pull request Jan 12, 2024

chore: release #973

Closed

This was referenced Feb 4, 2025

chore: release leovct/revm#1

Closed

chore: release leovct/revm#2

Open

chore: release abipalli/revm#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cancun): EIP-5656: MCOPY - Memory copying instruction #528

feat(cancun): EIP-5656: MCOPY - Memory copying instruction #528

0xJepsen commented Jun 20, 2023

rakita Jun 21, 2023

0xJepsen Jun 22, 2023

rakita Jun 21, 2023

0xJepsen Jun 22, 2023

rakita left a comment

0xJepsen commented Jun 21, 2023

rakita Jun 22, 2023

rakita Jun 22, 2023

0xJepsen Jun 23, 2023

tynes Jun 23, 2023

0xJepsen Jun 23, 2023

0xJepsen commented Jun 24, 2023

0xJepsen commented Jun 24, 2023 •

edited

Loading

rakita Jun 26, 2023 •

edited

Loading

rakita Jun 26, 2023

0xJepsen Jun 27, 2023

rakita Jun 27, 2023

rakita Jun 26, 2023

rakita Jun 27, 2023

rakita commented Jun 27, 2023

0xJepsen commented Jun 27, 2023 •

edited

Loading

rakita commented Jun 28, 2023 •

edited

Loading

0xJepsen commented Jun 30, 2023

rakita left a comment

feat(cancun): EIP-5656: MCOPY - Memory copying instruction #528

feat(cancun): EIP-5656: MCOPY - Memory copying instruction #528

Conversation

0xJepsen commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakita left a comment

Choose a reason for hiding this comment

0xJepsen commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xJepsen commented Jun 24, 2023

0xJepsen commented Jun 24, 2023 • edited Loading

rakita Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakita commented Jun 27, 2023

0xJepsen commented Jun 27, 2023 • edited Loading

rakita commented Jun 28, 2023 • edited Loading

0xJepsen commented Jun 30, 2023

rakita left a comment

Choose a reason for hiding this comment

0xJepsen commented Jun 24, 2023 •

edited

Loading

rakita Jun 26, 2023 •

edited

Loading

0xJepsen commented Jun 27, 2023 •

edited

Loading

rakita commented Jun 28, 2023 •

edited

Loading