[WIP]Implement simple subroutine opcodes #1937

Peppece · 2020-05-28T20:06:54Z

What was wrong?

EIP 2315 introduces three new opcodes (BEGINSUB, JUMPSUB, ENDSUB) which deal with subroutines and needed to be implemented.

How was it fixed?

The opcodes were implemented.
Also, while installing py-evm on a Mac during the project, I noticed that the doc page contributing.rst didn't include a step without which py-evm couldn't be installed on the system. I have fixed the doc accordingly.
Finally, this also implements the code at #1933 as a basis for the Berlin VM.

Fixes #1926

To-Do

Implement logic in eth/vm/logic
Write tests

Cute Animal Picture

(https://images.pexels.com/photos/146083/pexels-photo-146083.jpeg)

carver · 2020-05-28T23:56:57Z

@cburgdorf since you started taking a look at Berlin already, do you mind coordinating this review?

cburgdorf · 2020-05-29T07:29:34Z

since you started taking a look at Berlin already, do you mind coordinating this review?

@carver yep, will do.

cburgdorf · 2020-05-29T07:34:34Z

@Peppece thanks for kicking this off!

I think you could rebase this on top of #1933. This will already give you the basic scaffolding for the Berlin fork including the BerlinVM which you'll need in test_opcodes.py

I just wanted to know if the mantainers think the changes I made up to this point conform to the style of the project

Yep, so far so good 👍

I also wanted to ask if it was a good idea to create another file inside of the logic directory to implement the logic for the opcodes.

I think the implementation of the new opcodes should go into /eth/vm/logic/flow.py

Peppece · 2020-05-29T10:09:33Z

@cburgdorf Awesome! So how do I rebase things on #1933? Do I just copy + paste the code in my branch or is there some git fu I'm missing?

cburgdorf · 2020-05-29T10:15:35Z

You can either add my branch as a remote and pull it down or you edit your .git/config to always pull down PRs (makes your life much easier).

Then you can simple do:

git fetch origin
git rebase origin/pr/1933

carver · 2020-05-29T16:00:11Z

Do I just copy + paste the code in my branch or is there some git fu I'm missing?

Just in case: after you pull down the other PR, you would literally git rebase on top of it. (then git push -f) -- notice that these are destructive, and can cause you to lose code, especially if you're still getting comfortable with this tools.

If you're worried about that, you might want to save a reference to your starting point. I still occasionally find myself doing a:

git branch copy-of-branch
git rebase some-branch-i-want-to-add-my-changes-on-top-of
# tool around, write some more code, test it
# take a look around, realize I hate it, decide to blow all changes away since the git branch above
git reset --hard copy-of-branch

cburgdorf · 2020-06-17T12:23:28Z

Hey @Peppece just wanted to hear how things are going? Are you still working on this?

Peppece · 2020-06-17T12:26:05Z

Hey @Peppece just wanted to hear how things are going? Are you still working on this?

Hey @cburgdorf I was working on this while a school deadline blocked me and I had to focus on that. However, in max 3 days I’ll be back on this.

cburgdorf · 2020-06-18T08:12:59Z

@Peppece Sounds good!

pipermerriam

Thanks for working on this.

pipermerriam · 2020-07-14T13:32:42Z

eth/vm/logic/flow.py

+
+def returnsub(computation: BaseComputation) -> None:
+
+    if computation.rstack.length == 0:


rather than doing this check explicitely I think it would more closely follow our existing conventions to have the call to computation.rstack_pop1_int() throw an exception and to do this via a try/except block.

pipermerriam · 2020-07-14T13:35:26Z

eth/vm/rstack.py

+        self.values = values
+        self._append = values.append
+        self._pop_typed = values.pop
+        self.__len__ = values.__len__


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.

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.

pipermerriam · 2020-07-14T13:36:45Z

eth/vm/rstack.py

+            elif item_type is bytes:
+                return big_endian_to_int(popped)  # type: ignore
+            else:
+                raise _busted_type(item_type, popped)


I don't think this helper function is justified. I think it would better fit our conventions to inline the exception message and raising here.

pipermerriam · 2020-07-14T13:40:33Z

eth/vm/logic/flow.py

+
+    next_opcode = computation.code.peek()
+
+    if next_opcode != BEGINSUB:


I think you need to first check computation.code.is_valid_opcode(sub_loc) to ensure that the position is not part of PUSHXX data, then check if computation.code[sub_loc] == BEGINSUB, then you can set the program counter to the new position safely.

This prevents leaving the code stream in an awkward invalid position in the event of an invalid JUMPSUB.

eth/vm/computation.py

Peppece · 2020-09-16T14:24:30Z

I only noticed your review now. Will get back to work on this ASAP.

carver · 2020-09-16T23:17:59Z

It's probably a good idea to rebase against the latest master, to minimize conflicts for yourself.

pipermerriam

You should focus on getting the tests passing.

docs/contributing.rst

Peppece · 2020-11-17T09:01:51Z

Hello! Is there a particular reason this isn't getting merged? Feel like it has been stuck for a while now.

carver

Some items remaining:

Rebase on master
Drop all the ralexstokes commits

I haven't finished the review, but here are a few thoughts along the way.

eth/vm/logic/flow.py

carver · 2021-02-23T00:17:18Z

eth/vm/logic/flow.py

+                computation.code.program_counter)
+        )
+
+    if computation.code.is_valid_opcode(sub_loc):


~~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.

carver · 2021-02-23T00:20:31Z

eth/vm/logic/flow.py

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


__len__() is a "magic" python function that makes len() work. So this is preferred:

Suggested change

code_range_length = computation.code.__len__()

code_range_length = len(computation.code)

carver · 2021-02-23T00:21:39Z

eth/vm/logic/flow.py

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


Maybe include code_range_length too?

carver · 2021-02-23T00:22:42Z

eth/vm/logic/flow.py

+        if sub_op == BEGINSUB:
+            temp = computation.code.program_counter
+            computation.code.program_counter = sub_loc + 1
+            computation.rstack_push_int(temp)


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

carver · 2021-02-23T00:24:21Z

tests/core/opcodes/test_opcodes.py

+    assert comp.get_gas_used() == expect_gas_used
+
+
+@pytest.mark.xfail(reason="invalid subroutines")


xfail is better used for things like known bugs. It can hide problems like an incorrect exception instead of a computation that's not successful.

carver · 2021-02-23T00:25:28Z

tests/core/opcodes/test_opcodes.py

+        computation.msg,
+        computation.transaction_context,
+    )
+    assert comp.is_success


Even better here would be to assert that it's a failure, and validate the type of exception that is raised. Typically we would do that with a

Suggested change

assert comp.is_success

with pytest.raises(expected_exception):

comp.raise_if_error()

(expected_exception can be passed in with the test parameters)

carver

Okay, I finished the full pass on this. There's enough going on here that I'll do another pass on it after all these changes are in & tests are green. Feel free to @ me then.

Don't worry about trying to get the ethereum/tests Berlin tests going, it's going to be too tough to run those tests until all of the Berlin EIPs are implemented. But there are a couple more manual tests to add that I mentioned.

carver · 2021-02-24T00:05:42Z

eth/vm/logic/flow.py

+        ret_loc = computation.rstack_pop1_int()
+    except InsufficientStack:
+        raise InsufficientStack(
+            "Error: at pc={}, op=RETURNSUB: invalid retsub".format(


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(

carver · 2021-02-24T00:07:44Z

eth/vm/opcode_values.py

+#
+# Subroutines
+#


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

Suggested change

#

# Subroutines

#

# Subroutines

carver · 2021-02-24T00:12:41Z

eth/vm/rstack.py

+        self.values = values
+        self._append = values.append
+        self._pop_typed = values.pop
+        self.__len__ = values.__len__


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.

carver · 2021-02-24T00:13:59Z

eth/vm/rstack.py

+    __slots__ = ['values', '_append', '_pop_typed', '__len__']
+
+    def __init__(self) -> None:
+        values: List[Tuple[type, Union[int, bytes]]] = []


Values can't be bytes anymore.

Suggested change

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

values: List[int] = []

carver · 2021-02-24T00:14:57Z

eth/vm/rstack.py

+        self._pop_typed = values.pop
+        self.__len__ = values.__len__
+
+    def push_int(self, value: int) -> None:


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:

carver · 2021-02-24T00:18:59Z

eth/vm/rstack.py

+    VM Return Stack
+    """
+
+    __slots__ = ['values', '_append', '_pop_typed', '__len__']


Suggested change

__slots__ = ['values', '_append', '_pop_typed', '__len__']

__slots__ = ['values', '_append', '_pop', '__len__']

carver · 2021-02-24T00:37:53Z

eth/vm/rstack.py

+        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"
+                )


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")

carver · 2021-02-24T00:40:11Z

tests/core/opcodes/test_opcodes.py

+@pytest.mark.parametrize(
+    'vm_class, code, expect_gas_used',
+    (
+        (


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

carver · 2021-02-24T00:46:18Z

tests/core/opcodes/test_opcodes.py

+        (
+            BerlinVM,
+            '0x5c5d00',
+        ),


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

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

carver · 2021-02-24T00:46:47Z

tests/core/opcodes/test_opcodes.py

+
+@pytest.mark.xfail(reason="invalid subroutines")
+@pytest.mark.parametrize(
+    'vm_class, code',


Suggested change

'vm_class, code',

'vm_class, code, expected_exception',

carver · 2021-02-24T00:56:15Z

eth/vm/computation.py

@@ -140,6 +143,7 @@ def __init__(self,

        self._memory = Memory()
        self._stack = Stack()
+        self._rstack = RStack()


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

Suggested change

self._rstack = RStack()

self._return_stack = ReturnStack()

carver · 2021-03-11T00:44:41Z

Just a heads up that EIP-2315 was pulled from Berlin, so... I guess it makes sense to pause work on this. Sorry friend :/

Peppece · 2021-03-11T00:48:37Z

Just a heads up that EIP-2315 was pulled from Berlin, so... I guess it makes sense to pause work on this. Sorry friend :/

Wow, didn't see this coming. How is this possible? I mean, hasn't this already been implemented for geth and besu?

cburgdorf · 2021-03-11T11:20:44Z

@Peppece Yes, it was pulled last minute which is something you wouldn't normaly expect to happen. You may want to read up on the history here: ethereum/pm#263

This doesn't mean your work was for nothing though. There's a chance that the concerns may be addressed and the EIP gets updated / replaced and ships in some other form in an upcoming fork. Sorry, for the disappointing news :/

Peppece force-pushed the peppe/implement-subroutine-opcodes branch from 8e8703a to 92e94c0 Compare June 28, 2020 20:43

Peppece force-pushed the peppe/implement-subroutine-opcodes branch from 92e94c0 to e166ab0 Compare July 11, 2020 15:32

pipermerriam reviewed Jul 14, 2020

View reviewed changes

Peppece force-pushed the peppe/implement-subroutine-opcodes branch from 91d4b74 to 3960dce Compare September 19, 2020 16:41

Peppece requested a review from pipermerriam October 1, 2020 11:36

pipermerriam reviewed Oct 1, 2020

View reviewed changes

Peppece force-pushed the peppe/implement-subroutine-opcodes branch from 391b4ec to e1e988d Compare October 12, 2020 21:15

Peppece force-pushed the peppe/implement-subroutine-opcodes branch from 31249be to 88aad42 Compare October 25, 2020 06:58

Peppece requested a review from pipermerriam October 25, 2020 08:01

carver reviewed Oct 26, 2020

View reviewed changes

docs/contributing.rst Outdated Show resolved Hide resolved

Peppece requested a review from carver November 3, 2020 17:41

carver reviewed Feb 23, 2021

View reviewed changes

carver reviewed Feb 24, 2021

View reviewed changes

Peppece added 2 commits March 9, 2021 23:39

[WIP] Implement subroutine opcodes

a16d9a0

[WIP] Implement subroutine opcodes

de0efa5

Peppece force-pushed the peppe/implement-subroutine-opcodes branch from 7fd5d85 to de0efa5 Compare March 9, 2021 22:43

[WIP] Implement subroutine opcodes

b5d98ab

Peppece added 6 commits March 10, 2021 20:01

[WIP] Implement subroutine opcodes

ad402ca

[WIP] Implement subroutine opcodes

d2e0e4b

[WIP] Implement subroutine opcodes

279aa26

[WIP] Implement subroutine opcodes

30ed261

[WIP] Implement subroutine opcodes

730d4bf

[WIP] Implement subroutine opcodes

c164746


		def returnsub(computation: BaseComputation) -> None:

		if computation.rstack.length == 0:


		next_opcode = computation.code.peek()

		if next_opcode != BEGINSUB:

	code_range_length = computation.code.__len__()
	code_range_length = len(computation.code)

		assert comp.get_gas_used() == expect_gas_used


		@pytest.mark.xfail(reason="invalid subroutines")

	assert comp.is_success
	with pytest.raises(expected_exception):
	comp.raise_if_error()

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

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

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

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

[WIP]Implement simple subroutine opcodes #1937

Are you sure you want to change the base?

[WIP]Implement simple subroutine opcodes #1937

Conversation

Peppece commented May 28, 2020 • edited by carver

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

carver commented May 28, 2020

cburgdorf commented May 29, 2020

cburgdorf commented May 29, 2020

Peppece commented May 29, 2020 • edited

cburgdorf commented May 29, 2020

carver commented May 29, 2020

cburgdorf commented Jun 17, 2020

Peppece commented Jun 17, 2020

cburgdorf commented Jun 18, 2020

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peppece commented Sep 16, 2020

carver commented Sep 16, 2020

pipermerriam left a comment

Choose a reason for hiding this comment

Peppece commented Nov 17, 2020 • edited

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver Feb 23, 2021 • edited

Choose a reason for hiding this comment

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Mar 11, 2021

Peppece commented Mar 11, 2021

cburgdorf commented Mar 11, 2021

Peppece commented May 28, 2020 •

edited by carver

Peppece commented May 29, 2020 •

edited

Peppece commented Nov 17, 2020 •

edited

carver Feb 23, 2021 •

edited