EIP-211 RETURNDATACOPY and RETURNDATASIZE #5678
Conversation
arkpar
added
A3-inprogress ⏳
arkpar
force-pushed
the
eip211
branch
2 times, most recently
from
ad57684
to
a327c9d
Compare
arkpar
added
A0-pleasereview 🤓
| } | ||
| /// Create `ReturnData` from give buffer and slice. | ||
| pub fn new(mem: Vec<u8>, offset: usize, size: usize) -> Self { | ||
| ReturnData { |
There was a problem hiding this comment.
should probably truncate the allocation here to just size to alleviate peak memory usage concerns. At least, how I understand the code is that currently all calls returning data will have their entire memory kept around as opposed to just the requested amount. I'm not aware of a fast way to do it safely, though.
let _ = mem.drain(..offset); mem.truncate(size); mem.shrink_to_fit() might be a lot of work to do on every return.
There was a problem hiding this comment.
Updated. Now it reallocates but only if too much memory is wasted.
rphmeier
added
A5-grumble 🔥
|
where is the implementation of |
arkpar
added
A3-inprogress ⏳
|
@rphmeier It was missing :) Added now |
arkpar
added
A0-pleasereview 🤓
| return ReturnData::empty() | ||
| } | ||
| if self.len() - size > MAX_RETURN_WASTE_BYTES { | ||
| let mem = self.drain(offset..).take(size).collect(); |
There was a problem hiding this comment.
i really doubt the optimizer is smart enough to turn this into a single malloc + memmove, so it ends up paying both the O(n) iteration and having to allocate self.len() + size bytes in total.
i would suggest either using a drain, truncate, shrink_to_fit combo or allocating a vec![0; size] and using copy_from_slice to pay only one of those costs.
There was a problem hiding this comment.
(unless the optimized IR/ASM shows that this is actually a non-issue)
| fn into_return_data(mut self, offset: U256, size: U256) -> ReturnData { | ||
| let offset = offset.low_u64() as usize; | ||
| let size = size.low_u64() as usize; | ||
| if !is_valid_range(offset, size) { |
There was a problem hiding this comment.
this just checks whether offset + size would overflow, but they should also be bounds-checked against self.len() at this point.
There was a problem hiding this comment.
Memory is guaranteed to be allocated at this point. This is assured by mem/gas requirements checking before each instruction. That's why none of the methods in this struct check for bounds.
rphmeier
added
A8-looksgood 🦄
|
lgtm as well |
ethereum/EIPs#211