Implement EIP-663 variant C #98
Conversation
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
=========================================
- Coverage 93.46% 90.4% -3.06%
=========================================
Files 18 18
Lines 1866 1938 +72
Branches 197 202 +5
=========================================
+ Hits 1744 1752 +8
- Misses 97 161 +64
Partials 25 25 |
chfast
force-pushed
the
eip-663-c
branch
2 times, most recently
from
babd644
to
689f713
Compare
lib/evmone/analysis.cpp
Outdated
|
|
||
| int load_1byte_immediate_value(const uint8_t* code, size_t code_size, size_t& index) noexcept | ||
| { | ||
| return (index + 1 < code_size) ? code[++index] : 0; |
There was a problem hiding this comment.
Does this mean that code not having an argument behaves the same as argument 0? And costs the same gas? Is this correct? (how is this with the code not having enough bytes after PUSH?)
There was a problem hiding this comment.
It's the same case as implicit zero bytes after any PUSH instructions.
| @@ -124,7 +135,41 @@ code_analysis analyze( | |||
| instr.arg.p.number = static_cast<int>(i); | |||
| else if (c >= OP_LOG0 && c <= OP_LOG4) | |||
| instr.arg.p.number = c - OP_LOG0; | |||
| else if (is_terminator(c)) | |||
|
|
|||
| if (rev >= EVMC_ISTANBUL) | |||
There was a problem hiding this comment.
I think this should be better else if, so that it's not even considered if it's some other opcode.
lib/evmone/analysis.cpp
Outdated
| @@ -15,6 +15,18 @@ bool is_terminator(uint8_t c) noexcept | |||
| return c == OP_JUMP || c == OP_JUMPI || c == OP_STOP || c == OP_RETURN || c == OP_REVERT || | |||
| c == OP_SELFDESTRUCT; | |||
| } | |||
|
|
|||
| int load_1byte_immediate_value(const uint8_t* code, size_t code_size, size_t& index) noexcept | |||
There was a problem hiding this comment.
Minor stylistic suggestion: I would leave updating of index on the calling side, i.e. pass it by value here. Then it would be easier to understand what's happending with it, when you look the calling side
There was a problem hiding this comment.
Yeah, but we update the index conditionally. We can rename load_ into consume_ though.
| EXPECT_STATUS(EVMC_SUCCESS); | ||
| EXPECT_OUTPUT_INT(1023); | ||
|
|
||
| execute(full_stack_code + OP_DUPN + "03fe" + OP_SWAPSN + "00" + OP_RETURN); |
There was a problem hiding this comment.
why does it not use ret_top() and how does it return anything without storing in memory?
There was a problem hiding this comment.
The ret_top() requires additional push therefore more stack space. We only check the output size which matches the last duped item.
|
Unit test suggestions:
|
| std::pair<int, size_t> load_1byte_immediate_value( | ||
| const uint8_t* code, size_t code_size, size_t index) noexcept | ||
| { | ||
| return {(index + 1 < code_size) ? code[++index] : 0, index}; |
There was a problem hiding this comment.
This doesn't look right, because the order of evaluation of pair constructor arguments is not defined, is it?
There was a problem hiding this comment.
I thought this is now specified in C++17, but it is not. To be fixed.
There was a problem hiding this comment.
(re the need to update index conditionally: even if would update it unconditionally on the calling side and go beyond the code size, it will correctly break out of the loop, so updating here doesn't seem necessary to me)
There was a problem hiding this comment.
I though about this, but sounds risky. It would not work if we used != in the loop condition, or used an iterator instead of an index. Quite fragile to have the correctness of the implementation of this helper depend on some other loop implementation outside.
|
Can I close this? |
This implement the variant C of the EIP-663 by adding 4 new instructions for EVM stack manipulation:
DUPN,SWAPN,DUPSN,SWAPSN.The function change is only done in the analysis phase when the instruction argument (stack item index) is decode. During execution the same procedure is used as for legacy
DUPandSWAPinstructions.Closes #84.