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

Implement EIP-2315: subroutines #754

Merged
merged 10 commits into from
Aug 14, 2020
Merged

Implement EIP-2315: subroutines #754

merged 10 commits into from
Aug 14, 2020

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented May 27, 2020

#737

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
Copy link

codecov bot commented May 27, 2020

Codecov Report

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

Impacted file tree graph

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.

@cgewecke
Copy link
Contributor Author

cgewecke commented May 28, 2020

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]

@cgewecke
Copy link
Contributor Author

cgewecke commented May 29, 2020

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
Copy link
Member

holgerd77 commented Jun 11, 2020

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
Copy link
Contributor Author

cgewecke commented Jun 16, 2020

@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
Copy link
Contributor Author

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

Raw logs from most recent CI

@holgerd77
Copy link
Member

@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
Copy link
Contributor Author

cgewecke commented Jun 18, 2020

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
Copy link
Member

Rebased this and did the following changes along:

@holgerd77
Copy link
Member

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
Copy link
Member

Rebased this on top of #815

@jochem-brouwer
Copy link
Member

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

@holgerd77
Copy link
Member

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

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

This looks good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants