Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
Jul 1, 2023
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6537323
first pass
0xJepsen Jun 20, 2023
02d1480
added mcopy const to instruction.rs
0xJepsen Jun 20, 2023
a1df5a3
I think this is correct but this clone() on memory is not good
0xJepsen Jun 22, 2023
3615f07
fix the copy clone
0xJepsen Jun 23, 2023
01e2841
resize + copy in place
0xJepsen Jun 23, 2023
3a257ca
Memory resize + in memory copy
0xJepsen Jun 27, 2023
e490c74
fmt
0xJepsen Jun 27, 2023
be43cca
remove copy to vec
0xJepsen Jun 27, 2023
fc0cb3a
Update crates/interpreter/src/instructions/memory.rs
0xJepsen Jun 27, 2023
71d9eca
Update crates/interpreter/src/instructions/memory.rs
0xJepsen Jun 27, 2023
9bf0669
Update crates/interpreter/src/interpreter/memory.rs
0xJepsen Jun 27, 2023
2fd74c6
Update crates/interpreter/src/instructions/memory.rs
0xJepsen Jun 27, 2023
97b4ed9
fmt + max import
0xJepsen Jun 27, 2023
8fd3de5
specID
0xJepsen Jun 28, 2023
576fa1c
cargo update + memory test
0xJepsen Jun 30, 2023
2742804
Merge branch 'main' into waylon/EIP5656MCOPY
0xJepsen Jun 30, 2023
a54a86e
hashbrown duplicate
0xJepsen Jun 30, 2023
53ca642
Satisfy nit
0xJepsen Jun 30, 2023
10a91d3
Update crates/interpreter/src/instructions/memory.rs
rakita Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/interpreter/src/instructions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ pub fn eval<H: Host, S: Spec>(opcode: u8, interp: &mut Interpreter, host: &mut H
opcode::DELEGATECALL => host::delegate_call::<S>(interp, host), //check
opcode::STATICCALL => host::static_call::<S>(interp, host), //check
opcode::CHAINID => host_env::chainid::<S>(interp, host),
opcode::MCOPY => memory::mcopy::<S>(interp, host),
_ => return_not_found(interp, host),
}
}
41 changes: 40 additions & 1 deletion crates/interpreter/src/instructions/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
use crate::{gas, interpreter::Interpreter, primitives::U256, Host, InstructionResult};
use revm_primitives::SpecId::CANCUN;

use crate::{
gas,
interpreter::Interpreter,
primitives::{Spec, U256},
Host, InstructionResult,
};

pub fn mload(interpreter: &mut Interpreter, _host: &mut dyn Host) {
gas!(interpreter, gas::VERYLOW);
Expand Down Expand Up @@ -35,3 +42,35 @@ pub fn msize(interpreter: &mut Interpreter, _host: &mut dyn Host) {
gas!(interpreter, gas::BASE);
push!(interpreter, U256::from(interpreter.memory.effective_len()));
}
// From EIP-5656 MCOPY
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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

return;
}

let len = as_usize_or_fail!(interpreter, len, InstructionResult::InvalidOperandOOG);
gas_or_fail!(interpreter, gas::verylowcopy_cost(len as u64));

// memory_resize!(interpreter, dest, len);
memory_copy_resize(interpreter, host);
let dest = as_usize_or_fail!(interpreter, dest, InstructionResult::InvalidOperandOOG);


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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

This works perfectly

let data = binding.as_slice() ;
// Write data to dest
interpreter.memory.set_data(src, dest, len, data);
}

pub fn memory_copy_resize(interpreter: &mut Interpreter, _host: &mut dyn Host) {
Copy link
Member

@rakita rakita Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

let mut mstart = as_usize_or_fail!(interpreter, interpreter.stack.peek(0).unwrap()); // stack[0]: dst

if as_usize_or_fail!(interpreter, interpreter.stack.peek(1).unwrap()) > mstart {
mstart = as_usize_or_fail!(interpreter, interpreter.stack.peek(1).unwrap()); // stack[1]: source
}
memory_resize!(interpreter, mstart, as_usize_or_fail!(interpreter, interpreter.stack.peek(2).unwrap()));
}
5 changes: 3 additions & 2 deletions crates/interpreter/src/instructions/opcode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ pub const JUMPI: u8 = 0x57;
pub const PC: u8 = 0x58;
pub const MSIZE: u8 = 0x59;
pub const JUMPDEST: u8 = 0x5b;
pub const MCOPY: u8 = 0x5e;
pub const PUSH0: u8 = 0x5f;
pub const PUSH1: u8 = 0x60;
pub const PUSH2: u8 = 0x61;
Expand Down Expand Up @@ -276,7 +277,7 @@ pub const OPCODE_JUMPMAP: [Option<&'static str>; 256] = [
/* 0x5b */ Some("JUMPDEST"),
/* 0x5c */ None,
/* 0x5d */ None,
/* 0x5e */ None,
/* 0x5e */ Some("MCOPY"),
/* 0x5f */ Some("PUSH0"),
/* 0x60 */ Some("PUSH1"),
/* 0x61 */ Some("PUSH2"),
Expand Down Expand Up @@ -658,7 +659,7 @@ macro_rules! gas_opcodee {
OpInfo::jumpdest(),
/* 0x5c */ OpInfo::none(),
/* 0x5d */ OpInfo::none(),
/* 0x5e */ OpInfo::none(),
/* 0x5e MCOPY */ OpInfo::dynamic_gas(),
/* 0x5f PUSH0 */
OpInfo::gas(if SpecId::enabled($spec_id, SpecId::SHANGHAI) {
gas::BASE
Expand Down
8 changes: 8 additions & 0 deletions crates/interpreter/src/interpreter/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ impl Memory {
&self.data[offset..offset + size]
}

/// Memory copy
/// Copies memory inplace to a returns a [u8]
#[inline(always)]
pub fn copy_to_vec(&self, offset: usize, size: usize) -> Vec<u8> {
let mut vec = vec![0u8; size];
vec.copy_from_slice(&self.data[offset..size]);
vec
}
/// Set memory region at given offset
///
/// # Safety
Expand Down