EIP-3541 ethereum/tests Run by holgerd77 · Pull Request #1247 · ethereumjs/ethereumjs-monorepo

holgerd77 · 2021-05-09T15:37:03Z

This is a test run of a our EIP-3541 implementation from #1240 towards the tests implemented at ethereum/tests#835.

DO NOT MERGE!!!

@axic I noted that our test setup is very much not prepared to adhoc integrate these invented fork names, I couldn't imminently get this work to run on CI.

I will instead take this PR as a base and do manual tests runs of the different files.

tx: create access list utility

tx: export EIP1559

common: add docs getEIPActivationBlockNumber common: add extra constant

block: add more EIP1559 tests

block: enable access list transactions as TxData and EIP1559 transactions block: add base fee test check

tx: add EIP1559-related gas logic block: use EIP1559 api from tx

tx: fix gas limit not present in signed EIP1559 txns tx/vm: lint

…rice (with extended 0x-instantiation guard) to legacy and EIP-2930 txs

…igned default value instantiation for fromValuesArray() calls

…uctor, introduced TRANSACTION_TYPE and TRANSACTION_TYPE_BUFFER constants to EIP-2930 tx, added v, r, s aliases for consistency

…two fixes on TRANSACTION_TYPE_BUFFER Buffer initialization, fixed transaction type Buffer comparison in fromSerializedTx()

Co-authored-by: Ryan Ghods <ryan@ryanio.com>

holgerd77 · 2021-05-09T15:46:52Z

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stBadOpcode/undefinedOpcodeFirstByte.json --fork=Berlin+3541

1..114
# tests 114
# pass  114

Counter test:

1..114
# tests 114
# pass  114

Actually also pass, this doesn't seem to be right I guess? 🤔

holgerd77 · 2021-05-09T15:48:54Z

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreate2/CREATE2_EOF1_loop.json --fork=Berlin+3541

1..1
# tests 1
# pass  1

Counter test:

# tests 1
# pass  0
# fail  1

Everything ok here.

holgerd77 · 2021-05-09T15:51:27Z

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreateTest/CREATE_EOF1.json --fork=Berlin+3541

1..4
# tests 4
# pass  4

Counter test:

# tests 4
# pass  0
# fail  4

Everything ok here.

holgerd77 · 2021-05-09T15:53:28Z

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreateTest/CREATE_EOF1_loop.json --fork=Berlin+3541

1..1
# tests 1
# pass  1

Counter test:

# tests 1
# pass  0
# fail  1

Everything ok here.

holgerd77 · 2021-05-09T15:55:05Z

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stTransactionTest/CreateTransactionEOF1.json --fork=Berlin+3541

# tests 4
# pass  4

Counter test:

# tests 4
# pass  0
# fail  4

Everything ok here.

holgerd77 · 2021-05-09T15:58:22Z

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stTransactionTest/Opcodes_TransactionInit.json --fork=Berlin+3541

1..130
# tests 130
# pass  130

ok 128 the state roots should match
ok 129 the state roots should match
not ok 130 the state roots should match

1..130
# tests 130
# pass  129
# fail  1

On counter test run only the last failing? 🤔 Is this correct?

axic · 2021-05-09T16:01:31Z

@gumb0 can you comment when you get a chance please?

holgerd77 · 2021-05-09T16:24:03Z

For undefinedOpcodeFirstByte actually the post sections for Berlin and Berlin+3541 seem to be indentical.

holgerd77 · 2021-05-09T16:26:18Z

Ah, ok, have found the following comment from @gumb0 on the original PR:

"This could be merged separately already, because EIP-3541 doesn't change this behavior."

😄

holgerd77 · 2021-05-09T16:30:15Z

Have found this comment for Opcodes_TransactionInit on the tests PR, also from @gumb0:

"I didn't care to make all possible invalid opcodes here.

And this also can be merged independently, because this behavior is not changing yet."

This actually doesn't explain yet why 1 test is failing for us on berlin. If the test case is new for berlin then this might be a bug in our implementation. If the test case is old (we passed before) this might be a newly introduced bug in the tests.

(there is also always the third possibility that there is something wrong in our test setup 😄 )

holgerd77 · 2021-05-09T16:43:38Z

It's the data=129 test failing on berlin (so with data set to 0x6000600060006000600073b94f5374fce5edbc8e2a8697c15331677e6ebf0b61c350f15060ef60005360016000f3) which can be run in isolation with ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stTransactionTest/Opcodes_TransactionInit.json --fork=Berlin+3541 --data=129.

jochem-brouwer · 2021-05-09T17:11:04Z

Found the problem, we are deploying a contract with 0xEF as code.

jochem-brouwer · 2021-05-09T17:13:54Z


  const state = new Trie()
-  const hardfork = options.forkConfigVM
+  const hardfork = 'berlin'


?? Now we run all in Berlin and thus the --fork param is ignored?

Ah ok this PR is supposed to be hacky :)

codecov · 2021-05-09T17:22:45Z

Codecov Report

❗ No coverage uploaded for pull request base (eip1559@3efb919). Click here to learn what that means.
The diff coverage is n/a.

Flag	Coverage Δ
block	`81.92% <0.00%> (?)`
blockchain	`84.16% <0.00%> (?)`
client	`84.93% <0.00%> (?)`
devp2p	`83.97% <0.00%> (?)`
ethash	`82.08% <0.00%> (?)`
tx	`68.25% <0.00%> (?)`
vm	`80.80% <0.00%> (?)`

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

jochem-brouwer · 2021-05-09T17:27:21Z

Ran this;

ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreate2/CREATE2_EOF1.json --fork=Berlin+3541
ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stBadOpcode/undefinedOpcodeFirstByte.json --fork=Berlin+3541
ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreate2/CREATE2_EOF1_loop.json --fork=Berlin+3541
ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreateTest/CREATE_EOF1.json --fork=Berlin+3541
ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stCreateTest/CREATE_EOF1_loop.json --fork=Berlin+3541
ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stTransactionTest/CreateTransactionEOF1.json --fork=Berlin+3541
ts-node ./tests/tester --state --customStateTest=../ethereum-tests/GeneralStateTests/stTransactionTest/Opcodes_TransactionInit.json --fork=Berlin+3541

Result: all pass.

Counter test, all fail except

ethereum-tests/GeneralStateTests/stBadOpcode/undefinedOpcodeFirstByte.json: all pass
ethereum-tests/GeneralStateTests/stTransactionTest/Opcodes_TransactionInit.json: all pass except the final test (deploy 0xEF contract and store value in some other contract)

CC @axic @gumb0

jochem-brouwer · 2021-05-09T17:29:06Z

Ah ok I think I got a bit confused by the process here.

@holgerd77 Yes that one test fails is correct, I reproduced this as well; in your final test a 0xEF contract is deployed. For Berlin this is fine (it is allowed) but for Berlin+3541 this is not OK as it is an invalid contract. So it is correct that this is the only test which fails on Berlin+3541.

holgerd77 · 2021-05-09T18:49:08Z

@jochem-brouwer I have to say I can't completely "read" this explanation. First - these are not "my tests", these are the tests from @gumb0 submitted to the ethereum/tests repo.

Also: if there is a test case failing, there definitely something behaves wrong. Either in:

a) Our code
b) our test setup
c) the test itself

What are you suggesting and with what argumentation? 😄

(again: I'm just not able to interpret your current answer).

jochem-brouwer · 2021-05-09T19:02:40Z

@holgerd77 I just passed all tests on Berlin+EIP3541. This runs on a Common with Berlin as hardfork and 3541 activated as EIP.

So this is the test in question right?

ts-node ./tests/tester --state --customStateTest=../ethereumtests/GeneralStateTests/stTransactionTest/Opcodes_TransactionInit.json --fork=Berlin+3541 --data=129

Except now in the counter test we run this with no EIPs (so EIP 3541 is not activated). We still run it in the context of Berlin+3541. Then this test indeed fails. This is because:

This test creates a contract
In the deploy code of creating this contract, it calls to another contract.
This contract stores 1 in storage slot 0.
Now we return to the deploy code
This MSTORE8s 0xEF at memory slot 0
We return memory [0:1] (so we return 0xEF)
This thus deploys a contract with code 0xEF

If we activate 3541, this creation fails, and thus the storage slot write gets reverted.
If we don't activate it, then the creation does not fail, and thus the storage slot gets written. Therefore, there is a difference between the test where we activated the EIP and where we did not; since we are running the test with the same expected results, at least one has thus fail (and in this case the one where we don't activate the EIP thus fails, which is correct per the spec).

holgerd77 · 2021-05-09T19:14:46Z

@jochem-brouwer ah, yes, you're right, sorry, I was confused as well. So I guess the tests created are all ok. 😄

gumb0 · 2021-05-10T10:56:26Z

Confirming that tests/GeneralStateTests/stBadOpcode/undefinedOpcodeFirstByte.json should pass for both EIP3541 activated and not. (I'll remove it from ethereum/tests#835, the improved version is in ethereum/tests#836, you could check that, too)

In Opcodes_TransactionInit.json one new test is also supposed to behave without change after fork:
https://github.com/ethereum/tests/pull/835/files#diff-767d260c1cb56c8ea5548501176577874ca06d50339aa65fc236dfc37e3cb20eR373
and another one changes behavior with EIP3541:
https://github.com/ethereum/tests/pull/835/files#diff-767d260c1cb56c8ea5548501176577874ca06d50339aa65fc236dfc37e3cb20eR374
I think you've already figured this out correctly.

holgerd77 · 2021-05-10T12:20:39Z

@gumb0 cool, thanks for the feedback here! 👍

holgerd77 · 2021-05-17T07:43:56Z

Results have been picked up on the ethereum/tests side, will close.

jochem-brouwer and others added 30 commits May 9, 2021 16:33

tx: implement EIP1559 transaction base body

97b1c06

tx: create access list utility

tx: remove gasPrice from baseTransaction

4e2c814

tx: export EIP1559

tx: fix build problems

d4938d4

common: add eip1559

e9c0e35

block: integrate base fee EIP1559 param in header

65baa0a

common: add getEIPActivationBlockNumber

95213f3

common: add EIP1559 constants

49b6e97

common: add docs getEIPActivationBlockNumber common: add extra constant

block header: implement EIP1559 validation checks

90d0590

block: add baseFee tests

eddb7e9

block: fix bug EIP1559

ef87af0

block: add more EIP1559 tests

tx: add factory support for EIP1559

22c542e

block: enable access list transactions as TxData and EIP1559 transactions block: add base fee test check

vm: implement EIP1559

8fad4d7

tx: add EIP1559-related gas logic block: use EIP1559 api from tx

vm: implement EIP1559 test

7b39475

tx: fix gas limit not present in signed EIP1559 txns tx/vm: lint

tx/block: fix tests

8bb5e10

tx/vm: fix EIP2930 toJSON, expand EIP1559 tests

0787970

vm: add test to check if GASPRICE reports effective price

1f44898

tx: fixed additional BaseTransaction constructor conflict, added gasP…

73d32ab

…rice (with extended 0x-instantiation guard) to legacy and EIP-2930 txs

tx: added hashMessage parameter to EIP1559 tx getMessageToSign() method

79560dc

tx: fixed emptyBuffer accessList default value bug in EIP-1559 tx, al…

e90e128

…igned default value instantiation for fromValuesArray() calls

tx: added tx type injection in the super() call in EIP-1559 tx constr…

9ca19aa

…uctor, introduced TRANSACTION_TYPE and TRANSACTION_TYPE_BUFFER constants to EIP-2930 tx, added v, r, s aliases for consistency

tx: fixed BasedTransaction default value initializatioon rebase bug, …

f09d63c

…two fixes on TRANSACTION_TYPE_BUFFER Buffer initialization, fixed transaction type Buffer comparison in fromSerializedTx()

tx: fix EIP-2930 signed tx toJSON() output comparison test

b1cc595

block/tx: fix merge conflict + lint

ddea807

block/vm: fix tests

4b96b88

common: add london

7d02c4f

block/tx: EIP1559 support fixes

d825e10

client: temporary fixes to sync with aleut

67823a3

vm: add cliqueBeneficiary to runTx

6ca39d7

client: validateConsensus set to true

d65e6a2

block: apply suggestions from code review

97edff1

Co-authored-by: Ryan Ghods <ryan@ryanio.com>

holgerd77 mentioned this pull request May 9, 2021

Implement EIP3541 #1240

Merged

jochem-brouwer reviewed May 9, 2021

View reviewed changes

vm/common: fix tests

1a91590

holgerd77 force-pushed the eip1559 branch from 3efb919 to cf8f43c Compare May 10, 2021 10:06

holgerd77 force-pushed the eip1559 branch 2 times, most recently from 4a92829 to c8aeabf Compare May 11, 2021 13:00

ryanio force-pushed the eip1559 branch from a139366 to 850f9af Compare May 14, 2021 08:55

holgerd77 closed this May 17, 2021

holgerd77 deleted the eip-3541-test branch May 17, 2021 07:44

Uh oh!

Conversation

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

axic commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holgerd77 commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jochem-brouwer commented May 9, 2021

Uh oh!

jochem-brouwer May 9, 2021

Choose a reason for hiding this comment

Uh oh!

jochem-brouwer May 9, 2021

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jochem-brouwer commented May 9, 2021

Uh oh!

jochem-brouwer commented May 9, 2021

Uh oh!

holgerd77 commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jochem-brouwer commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

holgerd77 commented May 9, 2021

Uh oh!

gumb0 commented May 10, 2021

Uh oh!

holgerd77 commented May 10, 2021

Uh oh!

holgerd77 commented May 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holgerd77 commented May 9, 2021 •

edited

Loading

holgerd77 commented May 9, 2021 •

edited

Loading

codecov Bot commented May 9, 2021 •

edited

Loading

holgerd77 commented May 9, 2021 •

edited

Loading

jochem-brouwer commented May 9, 2021 •

edited

Loading