-
-
Notifications
You must be signed in to change notification settings - Fork 789
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.
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
Implement OVM backend #2416
Implement OVM backend #2416
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2416 +/- ##
==========================================
- Coverage 85.49% 84.62% -0.88%
==========================================
Files 91 94 +3
Lines 9059 9423 +364
Branches 2158 2205 +47
==========================================
+ Hits 7745 7974 +229
- Misses 809 939 +130
- Partials 505 510 +5
Continue to review full report at Codecov.
|
This pull request introduces 5 alerts when merging e0fb487 into 31dd776 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 82bda5c into 31dd776 - view on LGTM.com new alerts:
|
This produces bytecode now. I'm going to clean up the commit history and then it should be more or less ready for review |
27b8ae9
to
c836253
Compare
This pull request introduces 2 alerts when merging a92bc51 into 31dd776 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging 7b32577 into 31dd776 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging e296ae6 into 31dd776 - view on LGTM.com new alerts:
|
I manually went through an example "subroutine" to make sure the stack behaves as expected (seq
(label _ovm_sload)
(seq_unchecked
(with
key
pass
(with
buf
msize
(seq_unchecked
(mstore buf 2180116536)
(seq (mstore (add buf 32) key))
pass
; omitted for brevity: (ovm_exec (add 28 buf) 36 buf 32)
pass
(seq_unchecked (mload (add buf 0))) ; load output to stack
(calldatacopy buf calldatasize (sub msize buf)))))
(swap1 pass pass) ; swap output with return_pc
(jump pass)) ; consume return_pc, jump, stack now just has output
) emits the following (generated by vyper-lll and hand annotated by me) _sym__ovm_sload
JUMPDEST
# subroutine entry point
# STACK: return_pc, key (input)
MSIZE # return_pc, key, buf
PUSH4 129 241 240 56 # return_pc, key, buf, method_id
DUP2 # return_pc, key, buf, method_id, buf
MSTORE # return_pc, key, buf
DUP2 # return_pc, key, buf, key
PUSH1 32 # return_pc, key, buf, key, 32
DUP3 # return_pc, key, buf, key, 32, buf
ADD # return_pc, key, buf, key, buf+32
MSTORE # return_pc, key, buf
# OVM_EXEC string
PUSH1 0 # return_pc, key, buf, 0
DUP2 # return_pc, key, buf, 0, buf
ADD # return_pc, key, buf, buf
MLOAD # return_pc, key, buf, value
DUP2 # return_pc, key, buf, value, buf
MSIZE # return_pc, key, buf, value, buf, new_mem
SUB # return_pc, key, buf, value, new_mem - buf
CALLDATASIZE # return_pc, key, buf, value, new_mem - buf, calldatasize
DUP4 # return_pc, key, buf, value, new_mem - buf, calldatasize, buf
CALLDATACOPY # return_pc, key, buf, value
SWAP1 # return_pc, key, value, buf
POP # return_pc, key, value
SWAP1 # return_pc, value, key
POP # return_pc, value
SWAP1 # value, return_pc
JUMP # value
|
OVM statements like mstore 0 (seq_unchecked... goto ovm_sstore) not compiling
And add a sanity check so it doesn't happen again!
|
||
if use_ovm: | ||
for opcodes_for_evm_version in evm._evm_opcodes.values(): | ||
ovm.monkey_patch_evm_opcodes(opcodes_for_evm_version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ovm.monkey_patch_evm_opcodes(opcodes_for_evm_version) | |
ovm.monkeypatch_evm_opcodes(opcodes_for_evm_version) |
_revert0_string = ["_sym_revert0", "JUMPDEST", "PUSH1", 0, "DUP1", "REVERT"] | ||
if use_ovm: | ||
_revert0_string = ["_sym_revert0", "JUMPDEST", "PUSH1", 0, "DUP1"] + [ | ||
"_sym_ovm_revert", | ||
"JUMP", | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can _revert0_string
be named differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions? It was already in there ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so esoteric, I'm not sure what to suggest
@@ -0,0 +1,2 @@ | |||
from .asm import rewrite_asm_for_ovm | |||
from .spec import monkey_patch_evm_opcodes, rewrite_lll_for_ovm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can spec
module be renamed to something clearer? Maybe transpile
?
# https://community.optimism.io/docs/protocol/protocol.html#execution-contracts | ||
# * The CALLER, CALL, and REVERT opcodes are also disallowed, except in the special case that they appear as part of one of the following strings of bytecode: # noqa: 501 | ||
# CALLER PUSH1 0x00 SWAP1 GAS CALL PC PUSH1 0x0E ADD JUMPI RETURNDATASIZE PUSH1 0x00 DUP1 RETURNDATACOPY RETURNDATASIZE PUSH1 0x00 REVERT JUMPDEST RETURNDATASIZE PUSH1 0x01 EQ ISZERO PC PUSH1 0x0a ADD JUMPI PUSH1 0x01 PUSH1 0x00 RETURN JUMPDEST # noqa: 501 | ||
# CALLER POP PUSH1 0x00 PUSH1 0x04 GAS CALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# https://community.optimism.io/docs/protocol/protocol.html#execution-contracts | |
# * The CALLER, CALL, and REVERT opcodes are also disallowed, except in the special case that they appear as part of one of the following strings of bytecode: # noqa: 501 | |
# CALLER PUSH1 0x00 SWAP1 GAS CALL PC PUSH1 0x0E ADD JUMPI RETURNDATASIZE PUSH1 0x00 DUP1 RETURNDATACOPY RETURNDATASIZE PUSH1 0x00 REVERT JUMPDEST RETURNDATASIZE PUSH1 0x01 EQ ISZERO PC PUSH1 0x0a ADD JUMPI PUSH1 0x01 PUSH1 0x00 RETURN JUMPDEST # noqa: 501 | |
# CALLER POP PUSH1 0x00 PUSH1 0x04 GAS CALL | |
# https://community.optimism.io/docs/protocol/protocol.html#execution-contracts | |
# NOTE: The CALLER, CALL, and REVERT opcodes are also disallowed, except in the special | |
# case that they appear as part of one of the following sequences of bytecode: | |
# * `ALLOWED_EXEC_SEQ` (see below) | |
# * `ALLOWED_COPY_SEQ` (see below) |
ALLOWED_EXEC_STRING = "CALLER PUSH1 0x00 SWAP1 GAS CALL PC PUSH1 0x0E ADD JUMPI RETURNDATASIZE PUSH1 0x00 DUP1 RETURNDATACOPY RETURNDATASIZE PUSH1 0x00 REVERT JUMPDEST RETURNDATASIZE PUSH1 0x01 EQ ISZERO PC PUSH1 0x0a ADD JUMPI PUSH1 0x01 PUSH1 0x00 RETURN JUMPDEST" # noqa: 501 | ||
OVM_EXEC_ASM = [parse_asm_item(x) for x in ALLOWED_EXEC_STRING.split(" ")] | ||
|
||
# Call the identity precompile. | ||
# this expects the last 4 args of identity precompile already on the stack: | ||
# args_loc, args_len, return_loc, return_len | ||
ALLOWED_COPY_STRING = "CALLER POP PUSH1 0x00 PUSH1 0x04 GAS CALL" | ||
OVM_COPY_ASM = ALLOWED_COPY_STRING.split(" ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALLOWED_EXEC_STRING = "CALLER PUSH1 0x00 SWAP1 GAS CALL PC PUSH1 0x0E ADD JUMPI RETURNDATASIZE PUSH1 0x00 DUP1 RETURNDATACOPY RETURNDATASIZE PUSH1 0x00 REVERT JUMPDEST RETURNDATASIZE PUSH1 0x01 EQ ISZERO PC PUSH1 0x0a ADD JUMPI PUSH1 0x01 PUSH1 0x00 RETURN JUMPDEST" # noqa: 501 | |
OVM_EXEC_ASM = [parse_asm_item(x) for x in ALLOWED_EXEC_STRING.split(" ")] | |
# Call the identity precompile. | |
# this expects the last 4 args of identity precompile already on the stack: | |
# args_loc, args_len, return_loc, return_len | |
ALLOWED_COPY_STRING = "CALLER POP PUSH1 0x00 PUSH1 0x04 GAS CALL" | |
OVM_COPY_ASM = ALLOWED_COPY_STRING.split(" ") | |
ALLOWED_EXEC_SEQ = ( | |
"CALLER PUSH1 0x00 SWAP1 GAS CALL PC PUSH1 0x0E ADD JUMPI RETURNDATASIZE PUSH1 0x00 DUP1 " | |
"RETURNDATACOPY RETURNDATASIZE PUSH1 0x00 REVERT JUMPDEST RETURNDATASIZE PUSH1 0x01 " | |
"EQ ISZERO PC PUSH1 0x0a ADD JUMPI PUSH1 0x01 PUSH1 0x00 RETURN JUMPDEST" | |
) | |
OVM_EXEC_ASM = [parse_asm_item(x) for x in ALLOWED_EXEC_SEQ.split(" ")] | |
# Call the identity precompile. | |
# this expects the last 4 args of identity precompile already on the stack: | |
# args_loc, args_len, return_loc, return_len | |
ALLOWED_COPY_SEQ = "CALLER POP PUSH1 0x00 PUSH1 0x04 GAS CALL" | |
OVM_COPY_ASM = ALLOWED_COPY_SEQ.split(" ") |
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
bool_ = "bool" | ||
uint256 = "uint256" | ||
bytes_ = "bytes" | ||
address = "address" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird
return lll_nodes, lll_runtime | ||
|
||
|
||
def generate_assembly(lll_nodes: parser.LLLnode) -> list: | ||
def generate_assembly(lll_nodes: parser.LLLnode, use_ovm: bool = False) -> list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment, no change needed. Could be a bit nitpicky or YAGNI syndrome, the approach of passing a bool flag across the call stack works here for what we want.
Wondering if other EVM implementations like zkEVM, etc, may also need to rewrite of opcodes, then this solution will become adding more bool flags? which at that point it may be better to go with a Config struct arg or dictionary or change to approach to functional composition generate_assembly_ovm
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way you're thinking. Just to give some context -- the structure of this module is already a little weird. The entry point goes thru CompilerData which is a struct with all the config data. And then it calls these functions like generate_assembly
which are in fact private to the module, passing around these arguments.
I actually did consider refactoring how the config is threaded through the compiler. The reason I chose to do it this way instead is because OVM support is experimental, I wanted to minimize the diff to the existing compiler code. If we choose to remove support, say, in the next release, it won't be so hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on minimizing the diff to get this out. Sounds good to refactor later if needed.
Lol I got a couple style things to fix -- rename monkey_patch to monkeypatch, rename the ovm/spec.py module, cut up long lines |
What I did
Implement #2171. It can be used with any evm version by adding the
--ovm
flag on the command line. I only added the option to the CLI for now.How I did it
Replace opcodes which need to call into the OVM execution manager with a jump to a newly created ovm subroutine section. For instance,
sstore(x, y)
turns into_sstore_exit_1 x y jump(ovm_sstore) (label _sstore_exit_1)
. ovm_sstore calls the execution manager ovmSSTORE(x, y) and then jumps back to_sstore_exit_1
. I did this to minimize the bloat to the generated code - each OVM instruction gets about 7 bytes of overhead in the bytecode instead of inlining the whole procedure to call ovmSSTORE (which takes about a gajillion instructions), which is appended at the end as a code block shared between every invocation of sstore. For more details see the explanation in vyper/ovm/spec.pyHow to verify it
Compile a couple simple contracts.
Description for the changelog
Add experimental OVM backend
Cute Animal Picture