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.

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

[WIP]Implement simple subroutine opcodes #1937

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion docs/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ Thank you for your interest in contributing! We welcome all contributions no mat
Setting the stage
~~~~~~~~~~~~~~~~~

First we need to clone the Py-EVM repository. Py-EVM depends on a submodule of the common tests across all clients, so we need to clone the repo with the ``--recursive`` flag. Example:
.. note::
If it is the first time you install py-evm on macOS, check https://py-evm.readthedocs.io/en/latest/guides/quickstart.html#installing-on-macos

We need to clone the Py-EVM repository. Py-EVM depends on a submodule of the common tests across all clients, so we need to clone the repo with the ``--recursive`` flag. Example:

.. code:: sh

Expand Down
4 changes: 2 additions & 2 deletions docs/guides/quickstart.rst
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ Finally, we can install the ``py-evm`` package via pip.
Installing on macOS
-------------------

First, install Python 3 with brew:
First, install Python 3 and LevelDB with brew:

.. code:: sh

brew install python3
brew install python3 leveldb

.. note::
.. include:: /fragments/virtualenv_explainer.rst
Expand Down
15 changes: 15 additions & 0 deletions eth/abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1655,6 +1655,21 @@ def stack_dup(self, position: int) -> None:
"""
...

#
# Return Stack Managemement
#
@abstractmethod
def rstack_push_int(self) -> Callable[[int], None]:
"""
Push integer onto the return stack.
"""

@abstractmethod
def rstack_pop1_int(self) -> Callable[[int], None]:
"""
Pop integer off the return stack and return it.
"""

#
# Computation result
#
Expand Down
4 changes: 2 additions & 2 deletions eth/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class OutOfGas(VMError):

class InsufficientStack(VMError):
"""
Raised when the stack is empty.
Raised when the stack or the return stack is empty.
"""
pass

Expand All @@ -131,7 +131,7 @@ class FullStack(VMError):

class InvalidJumpDestination(VMError):
"""
Raised when the jump destination for a JUMPDEST operation is invalid.
Raised when the jump destination for a JUMPDEST or JUMPSUB operation is invalid.
"""
pass

Expand Down
15 changes: 15 additions & 0 deletions eth/vm/computation.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@
from eth.vm.stack import (
Stack,
)
from eth.vm.rstack import (
RStack,
)


def NO_RESULT(computation: ComputationAPI) -> None:
Expand Down Expand Up @@ -140,6 +143,7 @@ def __init__(self,

self._memory = Memory()
self._stack = Stack()
self._rstack = RStack()
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, also think that spelling out the full attribute name is friendlier, like:

Suggested change
self._rstack = RStack()
self._return_stack = ReturnStack()

self._gas_meter = self.get_gas_meter()

self.children = []
Expand Down Expand Up @@ -321,6 +325,17 @@ def stack_push_int(self) -> Callable[[int], None]:
def stack_push_bytes(self) -> Callable[[bytes], None]:
return self._stack.push_bytes

#
# Return Stack Management
#
@cached_property
def rstack_push_int(self) -> Callable[[int], None]:
carver marked this conversation as resolved.
Show resolved Hide resolved
return self._rstack.push_int

@cached_property
def rstack_pop1_int(self) -> Callable[[], int]:
return self._rstack.pop1_int

#
# Computation result
#
Expand Down
32 changes: 32 additions & 0 deletions eth/vm/forks/berlin/opcodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@
)
from eth.vm.opcode import Opcode

from eth import constants

from eth.vm import (
mnemonics,
opcode_values,
)
from eth.vm.opcode import as_opcode
from eth.vm.logic import flow

UPDATED_OPCODES: Dict[int, Opcode] = {
# New opcodes
Expand All @@ -17,3 +25,27 @@
copy.deepcopy(MUIR_GLACIER_OPCODES),
UPDATED_OPCODES,
)


UPDATED_OPCODES = {
opcode_values.BEGINSUB: as_opcode(
logic_fn=flow.beginsub,
mnemonic=mnemonics.BEGINSUB,
gas_cost=constants.GAS_BASE,
),
opcode_values.JUMPSUB: as_opcode(
logic_fn=flow.jumpsub,
mnemonic=mnemonics.JUMPSUB,
gas_cost=constants.GAS_HIGH,
),
opcode_values.RETURNSUB: as_opcode(
logic_fn=flow.returnsub,
mnemonic=mnemonics.RETURNSUB,
gas_cost=constants.GAS_LOW,
),
}

BERLIN_OPCODES = merge(
copy.deepcopy(MUIR_GLACIER_OPCODES),
UPDATED_OPCODES,
)
49 changes: 49 additions & 0 deletions eth/vm/logic/flow.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
from eth.exceptions import (
InvalidJumpDestination,
InvalidInstruction,
OutOfGas,
Halt,
InsufficientStack,
)

from eth.vm.computation import BaseComputation
from eth.vm.opcode_values import (
JUMPDEST,
BEGINSUB,
)


Expand Down Expand Up @@ -57,3 +60,49 @@ def gas(computation: BaseComputation) -> None:
gas_remaining = computation.get_gas_remaining()

computation.stack_push_int(gas_remaining)


def beginsub(computation: BaseComputation) -> None:
raise OutOfGas("Error: at pc={}, op=BEGINSUB: invalid subroutine entry".format(
computation.code.program_counter)
)


def jumpsub(computation: BaseComputation) -> None:
sub_loc = computation.stack_pop1_int()
code_range_length = len(computation.code)

if sub_loc >= code_range_length:
raise InvalidJumpDestination(
"Error: at pc={}, code_length={}, op=JUMPSUB: invalid jump destination".format(
computation.code.program_counter,
code_range_length)
)

if computation.code.is_valid_opcode(sub_loc):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this valid opcode check looks wrong to me. Ah, interesting, this was not made clear by the spec, IMO. But discussion seems to support the idea that the validity check is required: https://ethereum-magicians.org/t/eip-2315-simple-subroutines-for-the-evm/3941/70?u=carver

So... maybe the only thing to do here is to add an explicit test for JUMPSUBing into tho data section of a PUSH.


sub_op = computation.code[sub_loc]

if sub_op == BEGINSUB:
temp = computation.code.program_counter
computation.code.program_counter = sub_loc + 1
computation.rstack_push_int(temp)
Comment on lines +86 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the else block that aborts if the sub_op is not BEGINSUB? (and the test that verifies that behavior)


else:
raise InvalidJumpDestination(
"Error: at pc={}, code_length={}, op=JUMPSUB: invalid jump destination".format(
computation.code.program_counter,
code_range_length)
)


def returnsub(computation: BaseComputation) -> None:
try:
ret_loc = computation.rstack_pop1_int()
except InsufficientStack:
raise InsufficientStack(
"Error: at pc={}, op=RETURNSUB: invalid retsub".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this error message can be more friendly. Something like:

Suggested change
"Error: at pc={}, op=RETURNSUB: invalid retsub".format(
"Error: at pc={}, op=RETURNSUB: empty return stack".format(

computation.code.program_counter)
)

computation.code.program_counter = ret_loc
6 changes: 6 additions & 0 deletions eth/vm/mnemonics.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@
LOG3 = 'LOG3'
LOG4 = 'LOG4'
#
# Subroutines
#
BEGINSUB = 'BEGINSUB'
JUMPSUB = 'JUMPSUB'
RETURNSUB = 'RETURNSUB'
#
# System
#
CREATE = 'CREATE'
Expand Down
8 changes: 8 additions & 0 deletions eth/vm/opcode_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@
JUMPDEST = 0x5b


#
# Subroutines
#
Comment on lines +96 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these opcodes fit in the 0x5_ block nicely, maybe just:

Suggested change
#
# Subroutines
#
# Subroutines

BEGINSUB = 0x5c
RETURNSUB = 0x5d
JUMPSUB = 0x5e


#
# Push Operations
#
Expand Down
68 changes: 68 additions & 0 deletions eth/vm/rstack.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
from typing import (
List,
Tuple,
Union,
)

from eth.exceptions import (
InsufficientStack,
FullStack,
)

from eth.validation import (
validate_stack_int,
)

from eth_utils import (
big_endian_to_int,
ValidationError,
)

"""
This module simply implements for the return stack the exact same design used for the data stack.
As this stack must simply push_int or pop1_int any time a subroutine is accessed or left, only
those two functions are provided.
For the same reason, the class RStack doesn't inherit from the abc StackAPI, as it would require
to implement all the abstract methods defined.
"""


class RStack:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's very rare that we use initials in method and class names. A friendlier name here IMO:

Suggested change
class RStack:
class ReturnStack:

"""
VM Return Stack
"""

__slots__ = ['values', '_append', '_pop_typed', '__len__']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__slots__ = ['values', '_append', '_pop_typed', '__len__']
__slots__ = ['values', '_append', '_pop', '__len__']


def __init__(self) -> None:
values: List[Tuple[type, Union[int, bytes]]] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Values can't be bytes anymore.

Suggested change
values: List[Tuple[type, Union[int, bytes]]] = []
values: List[int] = []

self.values = values
self._append = values.append
self._pop_typed = values.pop
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._pop_typed = values.pop
self._pop = values.pop

self.__len__ = values.__len__
Copy link
Member

Choose a reason for hiding this comment

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

You might consider using collections.UserList for this class as I believe it comes built-in with some of the functionality you're doing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though perhaps it's maybe just as effective to subclass list, as the docs seem to recommend:

The need for this class has been partially supplanted by the ability to subclass directly from list; however, this class can be easier to work with because the underlying list is accessible as an attribute.

Not sure there's much benefit to being able to access the underlying list via .data here, though.


Actually, on second thought, we don't really want/need to expose all the list methods anyway, so maybe the current approach is better.


def push_int(self, value: int) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there aren't two type options anymore, this method can just be a push.

Suggested change
def push_int(self, value: int) -> None:
def push(self, value: int) -> None:

if len(self.values) > 1023:
raise FullStack('Stack limit reached')

validate_stack_int(value)

self._append((int, value))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._append((int, value))
self._append(value)


def pop1_int(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def pop1_int(self) -> int:
def pop1(self) -> int:

#
# Note: This function is optimized for speed over readability.
#
if not self.values:
raise InsufficientStack("Wanted 1 stack item as int, had none")
else:
item_type, popped = self._pop_typed()
if item_type is int:
return popped # type: ignore
elif item_type is bytes:
return big_endian_to_int(popped) # type: ignore
else:
raise ValidationError(
"Stack must always be bytes or int, "
f"got {item_type!r} type"
)
Comment on lines +56 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a single type lets us simplify a lot, and even do try-first instead of check-first:

Suggested change
if not self.values:
raise InsufficientStack("Wanted 1 stack item as int, had none")
else:
item_type, popped = self._pop_typed()
if item_type is int:
return popped # type: ignore
elif item_type is bytes:
return big_endian_to_int(popped) # type: ignore
else:
raise ValidationError(
"Stack must always be bytes or int, "
f"got {item_type!r} type"
)
try:
return self._pop()
except IndexError:
raise InsufficientStack("Wanted 1 return stack item, had none")

75 changes: 75 additions & 0 deletions tests/core/opcodes/test_opcodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
from eth.exceptions import (
InvalidInstruction,
VMError,
InvalidJumpDestination,
InsufficientStack,
OutOfGas,
)
from eth.rlp.headers import (
BlockHeader,
Expand Down Expand Up @@ -1439,3 +1442,75 @@ def test_blake2b_f_compression(vm_class, input_hex, output_hex, expect_exception
comp.raise_if_error()
result = comp.output
assert result.hex() == output_hex


@pytest.mark.parametrize(
'vm_class, code, expect_gas_used',
(
(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to add a comment here linking to the EIP's test cases:
https://eips.ethereum.org/EIPS/eip-2315#test-cases

BerlinVM,
'0x60045e005c5d',
18,
),
(
BerlinVM,
'0x6800000000000000000c5e005c60115e5d5c5d',
36,
),
(
BerlinVM,
'0x6005565c5d5b60035e',
30,
),
)
)
def test_jumpsub(vm_class, code, expect_gas_used):
computation = setup_computation(vm_class, CANONICAL_ADDRESS_B, decode_hex(code))
comp = computation.apply_message(
computation.state,
computation.msg,
computation.transaction_context,
)
assert comp.is_success
assert comp.get_gas_used() == expect_gas_used


@pytest.mark.parametrize(
'vm_class, code, expected_exception',
(
(
BerlinVM,
'0x5d5858',
InsufficientStack,
),
(
BerlinVM,
'0x6801000000000000000c5e005c60115e5d5c5d',
InvalidJumpDestination,
),
(
BerlinVM,
'0x5c5d00',
OutOfGas,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Another failing test case, that should fail with an InvalidJumpDestination:

        (
            BerlinVM,
            '0x60035e00',
            InvalidJumpDestination,
        ),

( # tests if the opcode raises error when trying to jump to BEGINSUB into pushdata
BerlinVM,
'0x60055e61005c58',
InvalidJumpDestination,
),
( # tests if the opcode raises error when trying to jump to an opcode other than BEGINGSUB
BerlinVM,
'0x6100055e0058',
InvalidJumpDestination,
)
)
)
def test_failing_jumpsub(vm_class, code, expected_exception):
computation = setup_computation(vm_class, CANONICAL_ADDRESS_B, decode_hex(code))
comp = computation.apply_message(
computation.state,
computation.msg,
computation.transaction_context,
)
with pytest.raises(expected_exception):
comp.raise_if_error()