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.

Sign up for GitHub

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

Jump to bottom

Transaction tests for EIP-3860: Limit and meter initcode #1012

Merged
merged 3 commits into from
Dec 1, 2022
Merged

Transaction tests for EIP-3860: Limit and meter initcode #1012

merged 3 commits into from
Dec 1, 2022

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Jan 27, 2022 •
edited
Loading

These tests check transaction validity rules change of https://eips.ethereum.org/EIPS/eip-3860
Geth implementation in ethereum/go-ethereum#23847

@gumb0 gumb0 mentioned this pull request Jan 27, 2022
Merged
@gumb0 gumb0 mentioned this pull request Jan 27, 2022
Closed
@holgerd77 holgerd77 mentioned this pull request Jan 28, 2022
Merged
@jochem-brouwer
Copy link
Member

Is this one an updated version of #990?

@gumb0
Copy link
Member Author

gumb0 commented Feb 2, 2022 •
edited
Loading

Is this one an updated version of #990?

No, this is a follow-up to #990 (independent of it), here it's only transaction tests, testing specifically new rules for when transactions are invalid.

(Not that we have not updated EIP text yet to make transactions with too big initcode invalid, sorry.
But the tests here consider them invalid)

@gumb0
Copy link
Member Author

gumb0 commented Feb 4, 2022

(Not that we have not updated EIP text yet to make transactions with too big initcode invalid, sorry. But the tests here consider them invalid)

EIP is updated now. ethereum/EIPs#4757

@jochem-brouwer
Copy link
Member

Ah yes this makes sense, we are not (yet) using the TransactionTests directory in JS, we should do that rather soon especially with these transactions. Thanks for the updates! 😄

@jochem-brouwer
Copy link
Member

Note: TransactionTests only supports LegacyTransactions ATM, so #990 should also include state tests or blockchain tests for situations where (1) initcode of Tx (legacy/2929/1559) is longer than max initcode and (2) sender does not pay the upfront cost of initcode analysis.

@gumb0
Copy link
Member Author

gumb0 commented Mar 3, 2022

Note: TransactionTests only supports LegacyTransactions ATM, so #990 should also include state tests or blockchain tests for situations where (1) initcode of Tx (legacy/2929/1559) is longer than max initcode and (2) sender does not pay the upfront cost of initcode analysis.

It might be possible to add TransactionTests with different transaction types, I'll check.

But adding them as state tests is also a good idea.

@jochem-brouwer
Copy link
Member

jochem-brouwer commented May 30, 2022 •
edited
Loading

@gumb0 do these tests include tests regarding CREATE/CREATE2 with sizes maxInitCodeSize and maxInitCodeSize+1?

EDIT: nvm, these are TxTests

winsvega
winsvega reviewed Nov 21, 2022
View reviewed changes
src/TransactionTestsFiller/ttData/DataTestNotEnoughGasInitCodeFiller.yml Outdated
# Creation transaction with gas limit not enough to cover initcode charge of EIP-3860
DataTestNotEnoughGasInitCode:
expectException:
London: ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an exception on London?

winsvega
winsvega reviewed Nov 21, 2022
View reviewed changes
src/TransactionTestsFiller/ttData/DataTestInitCodeLimitFiller.yml Outdated
@@ -0,0 +1,15 @@
# Creation transaction with initcode with max size allowed by EIP-3860
DataTestInitCodeLimit:
expectException: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this field (expecException) required if its empty?

winsvega
winsvega requested changes Nov 22, 2022
View reviewed changes
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

Update fork Merge+3860
Put EIP3860 in the test name or a new folder ttEIP3860
London: "" in expectException can be deleted
expectException: {} can be deleted

gumb0
gumb0 commented Nov 29, 2022
View reviewed changes
src/TransactionTestsFiller/ttData/DataTestInitCodeTooBigFiller.yml Outdated
- Merge+3860
expectException:
Merge: ""
Merge+3860: "TR_InitCodeLimitExceeded"
Copy link
Member Author

Choose a reason for hiding this comment

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

This requires a new mapping in retesteth config:

      "TR_InitCodeLimitExceeded" : "init code size limit exceeded",

@gumb0
Copy link
Member Author

gumb0 commented Nov 29, 2022

I would leave Merge: "" and expectedException: {} for explicitness, to show that we're testing that no exception is expected.

@gumb0 gumb0 marked this pull request as ready for review November 29, 2022 16:14
@gumb0 gumb0 requested a review from winsvega November 29, 2022 16:14
winsvega
winsvega reviewed Nov 30, 2022
View reviewed changes
src/TransactionTestsFiller/ttEIP3860/DataTestNotEnoughGasInitCodeFiller.yml
DataTestNotEnoughGasInitCode:
expectException:
Merge: ""
Merge+3860: "TR_IntrinsicGas"
Copy link
Collaborator

Choose a reason for hiding this comment

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

add Merge+3860 in additionalForks section that is missing here. the test is not generated for this fork.

winsvega
winsvega reviewed Nov 30, 2022
View reviewed changes
src/TransactionTestsFiller/ttEIP3860/DataTestInitCodeLimitFiller.yml
@@ -0,0 +1,15 @@
# Creation transaction with initcode with max size allowed by EIP-3860
DataTestInitCodeLimit:
expectException: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

additionalforks Merge+3860

winsvega
winsvega reviewed Nov 30, 2022
View reviewed changes
src/TransactionTestsFiller/ttEIP3860/DataTestEnoughGasInitCodeFiller.yml
@@ -0,0 +1,18 @@
# Creation transaction with gas limit just enough to cover initcode charge of EIP-3860
DataTestEnoughGasInitCode:
expectException: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

additional forks Merge+3860

winsvega
winsvega requested changes Nov 30, 2022
View reviewed changes
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

by default tests are generated for each fork in progression of Forks.
but since you use fork+eip here, need to specify in the test that it should also be generated using fork+eip by defining it in additionalForks section in the test.

retesteth config change no needed.

good to have small trasaction unit tests here. very clear.

@gumb0 gumb0 requested a review from winsvega November 30, 2022 18:26
winsvega
winsvega reviewed Dec 1, 2022
View reviewed changes
src/TransactionTestsFiller/ttEIP3860/DataTestNotEnoughGasInitCodeFiller.yml
additionalForks:
- Merge+3860
expectException:
Merge: ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

if it is empty, then record not needed

@winsvega winsvega merged commit 24fa31a into ethereum:develop Dec 1, 2022
@axic axic deleted the eip-3860-transactions branch December 2, 2022 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winsvega winsvega winsvega requested changes

Assignees
No one assigned
Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @jochem-brouwer @winsvega