Add EIP-3607 tests for Sender not EOA #963

marioevz · 2021-10-13T00:26:58Z

This PR closes #952 .
The following test cases are included:

An account with non-empty code sends the following transactions: send value, calls a contract, calls itself, deploys contract, init code executes: call, delegatecall
An account with empty code attempts to init into a non-empty code address: contract deployment to non-empty address, init code executes: call, delegatecall, create, create2

All tests are negative test cases and correctly fail.

This PR excludes tests where create and create2 deploy to an existing address since this is already tested by: GeneralStateTests/stCreate2, GeneralStateTests/stCreateTest

holiman · 2021-10-13T07:09:16Z

Just reading the description, I think this PR might be a bit over-ambitious?
The thing is, any transaction made from a non-EOA account should be rejected out of hand. So I think it would be sufficient to test whether a basic 'send ether to a recipient', 'make a create-tx' are rejected or not. That said, I'm fine with it being as it is, just noting my thoughts.

Also, to clarify; this change should be retroactive, so could be extended for pre-berlin aswell, but that's not important either.

marioevz · 2021-10-13T15:52:09Z

Thanks for the feedback, what I had in mind was that perhaps other clients implemented this differently and adding some extra checks just to be safe would be ok, but if everyone is inclined to reduce the number of test cases, that is also ok by me.
Also, I will change the fork to be retroactive.
Thanks again.

qbzzt · 2021-10-13T17:42:52Z

Thanks for the feedback, what I had in mind was that perhaps other clients implemented this differently and adding some extra checks just to be safe would be ok, but if everyone is inclined to reduce the number of test cases, that is also ok by me.

My opinion is that having a few extra tests won't hurt anything, and might save us from an unspecified error in the future.

MariusVanDerWijden · 2021-10-13T18:55:39Z

Reminder to self: selfdestruct a contract and send a tx from it after

marioevz · 2021-10-13T23:06:32Z

Reminder to self: selfdestruct a contract and send a tx from it after

Added this test case in the latest commit.
I first tried the contract to send the self-destruct funds to itself, but since the account is deleted, the funds also are destroyed.

The test case I ended up writing:

Self-destruct sends the funds to another account
This other account returns the funds to the address that just self-destructed
The funds are then finally retrieved with the secretKey

Let me know if this is ok.
Thanks.

holiman · 2021-10-14T06:49:23Z

Let me know if this is ok.

👍 Looks good to me!

winsvega

I think can merge many tests into one and use data as array.

winsvega · 2021-10-15T16:59:01Z

src/GeneralStateTestsFiller/stEIP3607/nonEmptyAccount_initsFiller.yml

+  # Transaction originating from account with code
+  transaction:
+    data:
+    - ':raw 0x00'


you can define multiple data raws here.
all the transactions would get rejected. I don't think there is a much need to cover every possible init data code.
but because each state test is translated into bc test for each fork here. please limit the amount of tests.
if the pre state is the same you can define it in one file with different data raws.

winsvega · 2021-10-15T17:00:08Z

src/GeneralStateTestsFiller/stEIP3607/nonEmptyAccount_contractDeployFiller.yml

+    - '400000'
+    gasPrice: '10'
+    nonce: '0'
+    to: ''


the 2 cases here is really when transaction has destination.
and the second when it runs creation. (empty to) this are 2 different files

winsvega · 2021-10-15T17:02:19Z

src/GeneralStateTestsFiller/stEIP3607/initNonEmptyAccount_callFiller.yml

@@ -0,0 +1,80 @@
+# Init into a non-empty account, call from init code
+
+initNonEmptyAccount_call:


this is an interesting case. collision and wrong sender at the same time.

winsvega · 2021-10-15T17:05:32Z

yes a good test case but in blockchain test within one block
first transaction destroys the account (selfdestruct)
second one is coming from the destroyed account. (should be valid now, but fail due to the account empty balance)

marioevz · 2021-10-15T19:42:36Z

yes a good test case but in blockchain test within one block

Yes this was created as a blockchain test in src/BlockchainTestsFiller/ValidBlocks/bcStateTests/contractSuicideThenSendFiller.yml

Also reduced the number of files as suggested in latest commit, and changed the fork to ">=London" to generate less tests.

winsvega · 2021-10-15T21:36:44Z

src/GeneralStateTestsFiller/stEIP3607/nonEmptyAccount_initFiller.yml

+        value: !!int -1
+
+      network:
+        - '>=London'


ok here could be >=Frontier

winsvega · 2021-10-15T21:38:00Z

src/GeneralStateTestsFiller/stEIP3607/nonEmptyAccount_callsFiller.yml

+    095e7baea6a6c7c4c2dfeb977efac326af552d87:
+      balance: '0'
+      nonce: '0'
+      code: ':raw 0xdeadbeef'


put something meaningfull like 0x6000600155

winsvega · 2021-10-15T21:39:38Z

src/GeneralStateTestsFiller/stEIP3607/nonEmptyAccount_callsFiller.yml

+        value: !!int -1
+
+      network:
+        - '>=London'


here >=Frontiner

winsvega · 2021-10-15T21:41:24Z

src/GeneralStateTestsFiller/stEIP3607/initNonEmptyAccountFiller.yml

@@ -0,0 +1,102 @@
+# Account attempts to send tx to create a contract on a non-empty address
+
+initNonEmptyAccount:


put here that collision involved

winsvega · 2021-10-15T21:42:47Z

src/GeneralStateTestsFiller/stEIP3607/initNonEmptyAccountFiller.yml

+        - '>=London'
+
+      result:
+        6295ee1b4f6dd65047762f924ecd367c17eabf8f:


check a94f5374fce5edbc8e2a8697c15331677e6ebf0b nonce

winsvega · 2021-10-15T21:43:49Z

src/BlockchainTestsFiller/ValidBlocks/bcStateTests/contractSuicideThenSendFiller.yml

+    0xfebc70C40FadB14d796F1d499b3f247e12FBbd94:
+      balance: '10000000000'
+      nonce: '0'
+      code: :raw 0x737ae27a42774a1f034ac7edb93d23cb5a662017d5FF


source code???

winsvega · 2021-10-15T21:46:04Z

src/BlockchainTestsFiller/ValidBlocks/bcStateTests/contractSuicideThenSendFiller.yml

+
+    # We send back the assets received from the self-destruct opcode
+    - data: ''
+      gasLimit: 21000


put more gaslimit as it tend to change to higher number in the future forks

winsvega · 2021-10-15T21:47:23Z

src/BlockchainTestsFiller/ValidBlocks/bcStateTests/contractSuicideThenSendFiller.yml

@@ -0,0 +1,92 @@
+# Contract self-destructs and then a private key with colliding address attempts to retrieve value from the account.
+
+contractSuicideThenSend:


transaction from selfdestruct

winsvega · 2021-10-15T21:48:17Z

src/BlockchainTestsFiller/ValidBlocks/bcStateTests/contractSuicideThenSendFiller.yml

+      0xd0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0:
+        balance: '9995800000'
+
+      0xfebc70C40FadB14d796F1d499b3f247e12FBbd94:


check nonce

marioevz added 3 commits October 11, 2021 21:33

Add first EIP3607 tests

d0beeb1

Add first init test for non-empty account

d53c949

Added test cases that deploy on non-empty account

915c720

marioevz requested review from winsvega, qbzzt and MariusVanDerWijden October 13, 2021 00:26

Add test for contract suicide then sends a tx

c2c9121

winsvega requested changes Oct 15, 2021

View reviewed changes

Reduced files, and forks to test, ran fillchain

1fcb9b3

winsvega requested changes Oct 15, 2021

View reviewed changes

marioevz added 2 commits October 15, 2021 22:36

Addressed comments and renamed tests

6fed934

Filled BCGeneralStateTests/stEIP3607

052db29

winsvega merged commit 779a31c into develop Oct 17, 2021

winsvega deleted the eip3607 branch October 17, 2021 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EIP-3607 tests for Sender not EOA #963

Add EIP-3607 tests for Sender not EOA #963

marioevz commented Oct 13, 2021

holiman commented Oct 13, 2021

marioevz commented Oct 13, 2021

qbzzt commented Oct 13, 2021

MariusVanDerWijden commented Oct 13, 2021

marioevz commented Oct 13, 2021

holiman commented Oct 14, 2021 •

edited

Loading

winsvega left a comment

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega commented Oct 15, 2021

marioevz commented Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

winsvega Oct 15, 2021

		@@ -0,0 +1,80 @@
		# Init into a non-empty account, call from init code

		initNonEmptyAccount_call:

		@@ -0,0 +1,102 @@
		# Account attempts to send tx to create a contract on a non-empty address

		initNonEmptyAccount:

		@@ -0,0 +1,92 @@
		# Contract self-destructs and then a private key with colliding address attempts to retrieve value from the account.

		contractSuicideThenSend:

Add EIP-3607 tests for Sender not EOA #963

Add EIP-3607 tests for Sender not EOA #963

Conversation

marioevz commented Oct 13, 2021

holiman commented Oct 13, 2021

marioevz commented Oct 13, 2021

qbzzt commented Oct 13, 2021

MariusVanDerWijden commented Oct 13, 2021

marioevz commented Oct 13, 2021

holiman commented Oct 14, 2021 • edited Loading

winsvega 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

winsvega commented Oct 15, 2021

marioevz commented Oct 15, 2021

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

holiman commented Oct 14, 2021 •

edited

Loading