Implement EIP-2315: subroutines #754

cgewecke · 2020-05-27T05:21:25Z

Implements EIP-2315 which introduces three new opcodes and a subroutine "return stack" to help separate control flow logic from computational operations on the main VM stack. This provides some efficiency benefits and is convenient for static analysis tools that require clear control flow semantics.

A detailed analysis of the EIP can be found at ethereum-magicians here

Added tests include the EIP examples plus a couple from the besu and geth implementations.
Have made the Stack class's overflow threshold configurable because the return stack's maximum height is spec'd at 1023 (1 less than normal)

(In draft pending updates to the ethereum tests (and common Berlin hardfork opcodes?))

codecov · 2020-05-27T05:44:09Z

Codecov Report

Merging #754 into master will increase coverage by 5.65%.
The diff coverage is 100.00%.

Flag	Coverage Δ
#account	`92.85% <ø> (ø)`
#block	`79.11% <ø> (ø)`
#blockchain	`82.88% <ø> (ø)`
#common	`93.98% <ø> (-0.16%)`	⬇️
#ethash	`81.93% <ø> (+1.11%)`	⬆️
#tx	`94.02% <ø> (-0.14%)`	⬇️
#vm	`92.49% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

packages/vm/lib/evm/interpreter.ts

cgewecke · 2020-05-28T16:02:18Z

There are ongoing revisions to EIP-2315 at eips/2676

Proposed changes in gas costs to

OP	COST
BEGINSUB	2
JUMPSUB	10
RETURNSUB	5

ethereum/tests 693 updates the tests for these values

[EDIT: addressed in eb77f6a]

packages/vm/lib/evm/opFns.ts

cgewecke · 2020-05-29T17:39:01Z

There is a change to the opcodes proposed in EIP 2682

OP	CODE
BEGINSUB	0x5c
JUMPSUB	0x5e
RETURNSUB	0x5d

NB: bytecodes in the test examples will need an update.

[EDIT: addressed in eb77f6a]

holgerd77 · 2020-06-11T10:59:10Z

Let's move forward here a bit. The temporary Yolo testnet - also https://yolonet.xyz for the status - is out and people will start testing their stuff against Berlin behavior.

For past hardforks we followed the procedure that we at some point activated a new HF in the supportedHardfork list and marked as experimental in the doc comments. We than subsequently added the HF changes in following releases and at some point declared the HF stable along some minor release. This doesn't need to follow semver since people doesn't expect a future HF to be stable anyhow at the point of releases and will start production integration based on the release marked as stable regarding to Berlin.

I would suggest that we directly activate Berlin on the v5 VM release and integrate the changes from here as a first EIP. Then people get the notion that the VM "is on its way" and can adopt their setup and start experimenting. The default HF setting will stay pre-Berlin for now.

Somewhat independently from the release decision we can update here on Berlin awareness, @chris: some notes on what to do here, this Petersburg PR #433 gives also some orientation, but is on the other hand also a bit outdated:

Update supportedHardfork list with Berlin support
Eventually update the Common dependency
Run at least one API test on this (just to see the fork can be activated minimally)
Add switches like runState._common.gteHardfork('constantinople') to ensure that new logic is only activated on a Berlin-and-onwards HF setting (e.g. like this along the new opcodes)
I would assume that Berlin test execution (see Developer docs) is possible without adoption of the test runner code, but this might need a check
Add Berlin test run to CI here
Add the new ethereumjs-testing v1.3.2 dependency (or temporarily link to the branch) to run against the first Berlin consensus tests, see New release v1.3.2 ethereumjs-testing#47

Hope I didn't forget anything. 😄

cgewecke · 2020-06-16T04:13:15Z

@holgerd77

Have gone through the list as suggested but am not sure I've enabled everything correctly 😄

~~Does the test config also need to be modified or does everything work if you just update the test package version?~~ (Ok, I can see that only the Istanbul tests are running in for the test:blockchain CI job)

I did get the ethereum/tests subroutine tests to pass locally, after setting the default fork to Berlin. It reduces the test set to just these though.

$ ts-node ./tests/tester --blockchain
TAP version 13
# BlockchainTests
# file: beginSubAtEndOfCode test: beginSubAtEndOfCode_d0g0v0_Berlin
ok 1 correct pre stateRoot
ok 2 correct genesis RLP
# file: shouldErrorWhenExecuteBeginSub test: shouldErrorWhenExecuteBeginSub_d0g0v0_Berlin
ok 3 correct pre stateRoot
ok 4 correct genesis RLP
# file: shouldErrorWhenJumpToJumpDest test: shouldErrorWhenJumpToJumpDest_d0g0v0_Berlin
ok 5 correct pre stateRoot
ok 6 correct genesis RLP
# file: shouldErrorWhenReturnStackGrowsAbove1023 test: shouldErrorWhenReturnStackGrowsAbove1023_d0g0v0_Berlin
ok 7 correct pre stateRoot
ok 8 correct genesis RLP
# file: shouldErrorWhenSubroutineEnteredViaBeginSub test: shouldErrorWhenSubroutineEnteredViaBeginSub_d0g0v0_Berlin
ok 9 correct pre stateRoot
ok 10 correct genesis RLP
# file: shouldSucceedWhenReturnStackGrowsUntil1023 test: shouldSucceedWhenReturnStackGrowsUntil1023_d0g0v0_Berlin
ok 11 correct pre stateRoot
ok 12 correct genesis RLP
# file: simpleSubroutine test: simpleSubroutine_d0g0v0_Berlin
ok 13 correct pre stateRoot
ok 14 correct genesis RLP
# file: subroutineAtEndOfCode test: subroutineAtEndOfCode_d0g0v0_Berlin
ok 15 correct pre stateRoot
ok 16 correct genesis RLP
# file: subroutineInvalidJump test: subroutineInvalidJump_d0g0v0_Berlin
ok 17 correct pre stateRoot
ok 18 correct genesis RLP
# file: subroutineShallowReturnStack test: subroutineShallowReturnStack_d0g0v0_Berlin
ok 19 correct pre stateRoot
ok 20 correct genesis RLP
# file: twoLevelsSubroutines test: twoLevelsSubroutines_d0g0v0_Berlin
ok 21 correct pre stateRoot
ok 22 correct genesis RLP

1..22
# tests 22
# pass  22

# ok

cgewecke · 2020-06-16T14:05:54Z

Looking at this again, it seems test-vm-state includes EIP 2315 tests and they're passing.

Raw logs from most recent CI

holgerd77 · 2020-06-18T10:33:46Z

@cgewecke Thanks, nice! 😄 The tests haven't been regenerated for Berlin yet, so this is why only the small subset of subroutine tests are running.

cgewecke · 2020-06-18T15:15:58Z

The tests haven't been regenerated for Berlin yet, so this is why only the small subset of subroutine tests are running.

Ah! I guess will wait here for ethereum/test 692 and update when the name is finalized / the tests are regenerated. [Edit - actually I'm not sure what's right].

Think the only logic thing left is make the tester's default fork Berlin and map its alias correctly.

holgerd77 · 2020-08-12T08:10:34Z

Rebased this and did the following changes along:

Moved the base opcode costs for BEGINSUB, RETURNSUB and JUMPSUB to Common (analogue to the changes from @jochem-brouwer in [VM/Common] move gas base fees to Common #806 )
Added a dedicated Berlin HF section in opcodes.ts
Updated ethereumjs-testing to v1.3.3

holgerd77 · 2020-08-12T14:47:36Z

VM / test-vm-state (Berlin) is not taking in the Berlin tests - so at the end 0 tests are run - not sure why, might have something to do with the ethereumjs-testing v1.3.3 dependency (wasn't there some similar discussion somewhere?).
//cc @evertonfraga @jochem-brouwer

Blockchain tests will need the update from #815 to work properly.

holgerd77 · 2020-08-12T15:35:29Z

Rebased this on top of #815

jochem-brouwer · 2020-08-12T16:42:12Z

@holgerd77 for the Berlin fork only use the EIP 2315 tests, i.e. add `dir=stSubroutine' (all BLS tests will fail)

… HF opcode section in opcodes.ts

…pcodes are now preselected on a HF basis in opcodes.ts)

holgerd77 · 2020-08-14T08:35:25Z

Ok, my local Berlin state tests pass here, did a last rebase and removed the HF checks in the opcodes (now obsolete due to the pre-selection of HFs in opcodes.ts) along the way:

> ts-node ./tests/tester --state "--fork=Berlin" "--dir=stSubroutine"

+--------------------------------------------------+
| VM -> GeneralStateTests                          |
|                                                  |
| TestGetterArgs                                   |
| skipTests           : 54                         |
| forkConfig          : Berlin                     |
| dir                 : stSubroutine               |
|                                                  |
| RunnerArgs                                       |
| forkConfigVM        : berlin                     |
| forkConfigTestSuite : Berlin                     |
+--------------------------------------------------+

TAP version 13
# GeneralStateTests
# file: beginSubAtEndOfCode test: beginSubAtEndOfCode
ok 1 the state roots should match
# file: shouldErrorWhenExecuteBeginSub test: shouldErrorWhenExecuteBeginSub
ok 2 the state roots should match
# file: shouldErrorWhenJumpToJumpDest test: shouldErrorWhenJumpToJumpDest
ok 3 the state roots should match
# file: shouldErrorWhenReturnStackGrowsAbove1023 test: shouldErrorWhenReturnStackGrowsAbove1023
ok 4 the state roots should match
# file: shouldErrorWhenSubroutineEnteredViaBeginSub test: shouldErrorWhenSubroutineEnteredViaBeginSub
ok 5 the state roots should match
# file: shouldSucceedWhenReturnStackGrowsUntil1023 test: shouldSucceedWhenReturnStackGrowsUntil1023
ok 6 the state roots should match
# file: simpleSubroutine test: simpleSubroutine
ok 7 the state roots should match
# file: subroutineAtEndOfCode test: subroutineAtEndOfCode
ok 8 the state roots should match
# file: subroutineInvalidJump test: subroutineInvalidJump
ok 9 the state roots should match
# file: subroutineShallowReturnStack test: subroutineShallowReturnStack
ok 10 the state roots should match
# file: twoLevelsSubroutines test: twoLevelsSubroutines
ok 11 the state roots should match

1..11
# tests 11
# pass  11

# ok

Have also given the last code missing in opFns.ts (so the core logic itself) a final review, this looks good, one comment.

Will give this a merge now, maybe @jochem-brouwer can do the final test activation along the finalization of #785

holgerd77

This looks good now.

cgewecke commented May 27, 2020

View reviewed changes

packages/vm/lib/evm/interpreter.ts Outdated Show resolved Hide resolved

evertonfraga assigned cgewecke May 27, 2020

matkt mentioned this pull request May 28, 2020

EIP-2315 changes in gascosts and update opcode ethereum/tests#693

Merged

cgewecke commented May 28, 2020

View reviewed changes

packages/vm/lib/evm/opFns.ts Show resolved Hide resolved

evertonfraga added the package: vm label Jun 2, 2020

holgerd77 mentioned this pull request Jun 11, 2020

New release v1.3.2 ethereumjs/ethereumjs-testing#47

Merged

cgewecke force-pushed the eip-2315 branch from 3b1a750 to 39e1d45 Compare June 16, 2020 02:59

cgewecke marked this pull request as ready for review June 18, 2020 23:50

cgewecke force-pushed the eip-2315 branch from 00f34e6 to 67de163 Compare June 18, 2020 23:54

jochem-brouwer mentioned this pull request Jun 25, 2020

Ethereum Core Devs Meeting 90 Agenda ethereum/pm#189

Closed

holgerd77 mentioned this pull request Aug 9, 2020

VM v5 Release Planning #681

Closed

27 tasks

holgerd77 force-pushed the eip-2315 branch from 67de163 to a1e55cb Compare August 12, 2020 08:07

holgerd77 force-pushed the eip-2315 branch from a1e55cb to 3479ce5 Compare August 12, 2020 14:30

holgerd77 force-pushed the eip-2315 branch from 3479ce5 to c7fc3f9 Compare August 12, 2020 15:34

cgewecke added 4 commits August 14, 2020 09:40

Implement EIP-2315: subroutines

34f5fd0

Update gas costs to latest EIP state

a70c80b

Use valid JUMPSUB table and normalize error messages

6c28a86

Add berlin to list of supported hardforks

6a94625

cgewecke added 2 commits August 14, 2020 09:40

Update opcodes and gas costs

553ddef

Restrict opcodes to berlin hardfork

170e5e8

holgerd77 force-pushed the eip-2315 branch from c7fc3f9 to ba7d36b Compare August 14, 2020 08:01

cgewecke and others added 4 commits August 14, 2020 10:14

Update ethereumjs-testing to 1.3.3

a8fb708

Remove rebase/merge regressions

8efca8f

vm: moved BEGINSUB, RETURNSUB and JUMPSUB opcodes to dedicated Berlin…

59d338c

… HF opcode section in opcodes.ts

vm: removed invalid opcode checks for BEGINSUB, JUMPSUB, RETURNSUB (o…

10f4186

…pcodes are now preselected on a HF basis in opcodes.ts)

holgerd77 force-pushed the eip-2315 branch from ba7d36b to 10f4186 Compare August 14, 2020 08:15

holgerd77 approved these changes Aug 14, 2020

View reviewed changes

holgerd77 merged commit 546736d into master Aug 14, 2020

holgerd77 deleted the eip-2315 branch August 14, 2020 08:36

holgerd77 mentioned this pull request Aug 21, 2020

[Berlin] EIP-2315: Simple Subroutines for the EVM #737

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EIP-2315: subroutines #754

Implement EIP-2315: subroutines #754

cgewecke commented May 27, 2020 •

edited

codecov bot commented May 27, 2020 •

edited

cgewecke commented May 28, 2020 •

edited

cgewecke commented May 29, 2020 •

edited

holgerd77 commented Jun 11, 2020 •

edited

cgewecke commented Jun 16, 2020 •

edited

cgewecke commented Jun 16, 2020

holgerd77 commented Jun 18, 2020

cgewecke commented Jun 18, 2020 •

edited

holgerd77 commented Aug 12, 2020

holgerd77 commented Aug 12, 2020

holgerd77 commented Aug 12, 2020

jochem-brouwer commented Aug 12, 2020

holgerd77 commented Aug 14, 2020

holgerd77 left a comment

Implement EIP-2315: subroutines #754

Implement EIP-2315: subroutines #754

Conversation

cgewecke commented May 27, 2020 • edited

codecov bot commented May 27, 2020 • edited

Codecov Report

cgewecke commented May 28, 2020 • edited

cgewecke commented May 29, 2020 • edited

holgerd77 commented Jun 11, 2020 • edited

cgewecke commented Jun 16, 2020 • edited

cgewecke commented Jun 16, 2020

holgerd77 commented Jun 18, 2020

cgewecke commented Jun 18, 2020 • edited

holgerd77 commented Aug 12, 2020

holgerd77 commented Aug 12, 2020

holgerd77 commented Aug 12, 2020

jochem-brouwer commented Aug 12, 2020

holgerd77 commented Aug 14, 2020

holgerd77 left a comment

Choose a reason for hiding this comment

cgewecke commented May 27, 2020 •

edited

codecov bot commented May 27, 2020 •

edited

cgewecke commented May 28, 2020 •

edited

cgewecke commented May 29, 2020 •

edited

holgerd77 commented Jun 11, 2020 •

edited

cgewecke commented Jun 16, 2020 •

edited

cgewecke commented Jun 18, 2020 •

edited