deduplication for COPY operations#1655
Conversation
|
This is fine but doesnt fix the issue, there is also the opcode implementation to dedup |
|
@DaniPopes what's up with this clippy thing, I didn't even touch pairings or utils! when I run |
harsh-ps-2003
changed the title
harsh-ps-2003
changed the title
|
It's due to Rust 1.80 just being released, it should be fixed on main with #1661 |
| len, | ||
| interpreter.contract.bytecode.original_byte_slice(), | ||
| ); | ||
| let source = interpreter.contract.bytecode.original_byte_slice().to_vec(); |
There was a problem hiding this comment.
These to_vec clones can be removed by using raw pointers. Unfortunately for performance we have to use them to get around the aliasing issue with &mut Interpreter
You can change source: &[u8] to source: *const [u8]
| memory_offset: U256, | ||
| data_offset: usize, | ||
| len: usize, | ||
| source: *const u8, |
There was a problem hiding this comment.
source's length is not len, this should be *const [u8]. This is why tests are failing
| data_offset: usize, | ||
| len: usize, | ||
| source: *const [u8], | ||
| ) { |
There was a problem hiding this comment.
I would return a Option<usize> here that represents memory_offset.
https://github.com/bluealloy/revm/pull/1655/files#r1694998333
Without return function silently fails, and can work only if it is in the last line of instruction.
| unsafe { | ||
| interpreter | ||
| .shared_memory | ||
| .set_data(memory_offset, data_offset, len, &*source); | ||
| } | ||
| } |
There was a problem hiding this comment.
I would remove this, and change behaviour of function to do gas, len check, memory resize.
As memory_offset is reused, we can return that from this function as Option<usize>
There was a problem hiding this comment.
The problem with changing the functions behaviour as suggested by you is that we encountering is a classic case of Rust's borrow checker preventing simultaneous mutable and immutable borrows of the same data.
When we do :
let source = interpreter.contract.input.as_ref();
let memory_offset = copy_to_memory(interpreter, memory_offset, data_offset, len);
if let Some(memory_offset) = memory_offset {
// Note: this can't panic because we resized memory to fit.
interpreter.shared_memory.set_data(memory_offset, data_offset, len, source);
};We create an immutable borrow of interpreter.contract.input and then we try to mutably borrow interpreter in the copy_to_memory function call. Finally, we try to use the immutable borrow source again in the set_data call.
The Rust borrow checker doesn't allow this because it can't guarantee that the mutable borrow in copy_to_memory won't invalidate the source reference.
How do you suggest I tackle this! @rakita
There was a problem hiding this comment.
can this line: let source = interpreter.contract.input.as_ref(); be moved inside if let Some(memory_.. so you don't have a borrow?
There was a problem hiding this comment.
Now when I return Option<usize> I get expected enum std::option::Option<usize>
found unit type () from the gas! macro. Thats why the tests are failing. How do you suggest I tackle this! @rakita
There was a problem hiding this comment.
- There is a gas! version that takes Return.
gas!(10,None)something similar should be added forgas_or_fail! - Change
as_usize_or_failtoas_usize_or_fail_ret!(interpreter, memory_offset,None); - Change
resize_memory!toresize_memory!(interpreter, memory_offset, len, None);
|
Superseded by #1799 |
3 participants
Ref #1637
It reduces code duplication by creating a generic
copy_costfunction. It maintains the special logic forextcodecopy_costwhile still using the generic function.It will make adding new COPY operations easier in the future, as they can all use the same copy_cost function.
Also adds
copy_to_memorymethod for the COPY opcodes to remove code duplication.