EIP 6780 Tests

[jwasinger]

I'm unsure how to fill these. I have tried:

>  tf --filler-path="fillers" --output="fixtures" --test-categories eips.eip6780  --force-refill

DEBUG:ethereum_test_filling_tool.filler:searching eips.eip6780.eip6780 for fillers
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.github.com:443
DEBUG:urllib3.connectionpool:https://api.github.com:443 "GET /repos/ethereum/EIPs/contents/EIPS/eip-6780.md HTTP/1.1" 200 3449

After a bit of digging, I figured out that the filler is not detected because it is configured for an upcoming fork (have also tried using Cancun with the same result).

[danceratopz]

I'm unsure how to fill these. I have tried:

>  tf --filler-path="fillers" --output="fixtures" --test-categories eips.eip6780  --force-refill

DEBUG:ethereum_test_filling_tool.filler:searching eips.eip6780.eip6780 for fillers
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.github.com:443
DEBUG:urllib3.connectionpool:https://api.github.com:443 "GET /repos/ethereum/EIPs/contents/EIPS/eip-6780.md HTTP/1.1" 200 3449

After a bit of digging, I figured out that the filler is not detected because it is configured for an upcoming fork (have also tried using Cancun with the same result).

Hi @jwasinger, I just struggled to fill these myself. It turns out that you need to specify --latest-fork Cancun (I found this info hidden in the v0.2.5 release notes)

tf --latest-fork Cancun --test-categories eips.eip6780

If I run this using the evm tool from jwasinger/go-ethereum@06c795d branch I get:

➜ tf --latest-fork Cancun --test-categories eips.eip6780 --traces
DEBUG:ethereum_test_filling_tool.filler:searching eips.eip6780.eip6780 for fillers
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): api.github.com:443
DEBUG:urllib3.connectionpool:https://api.github.com:443 "GET /repos/ethereum/EIPs/contents/EIPS/eip-6780.md HTTP/1.1" 200 None
INFO:ethereum_test_filling_tool.filler:collected 2 fillers
DEBUG:ethereum_test_filling_tool.filler:skipping - eips/eip6780/eip6780/eip6780_create_selfdestruct_same_tx
Traceback (most recent call last):
  File "/home/dtopz/code/github/jwasinger/execution-spec-tests/venv/bin/tf", line 8, in <module>
<- snip ->
Exception: failed to evaluate: ERROR(3): failed constructing chain configuration: unsupported fork "ShanghaiToCancunAtTime15k"

I get further, but the filler's post condition fails, if I run it against the version of go-ethereum (mdehoog/go-ethereum@ac64c44) that was used for the execution-spec-tests v0.2.5 release), but mdehoog/go-ethereum@ac64c44 is focussed on 4844 and hasn't implemented 6780.

@marioevz or @spencer-tb, perhaps you've already cleared this up? If not, could you help further?

[spencer-tb]

Had a proper look at this! Thanks for contributing - we are hoping to make it much easier to write tests very soon :D

I believe in order to fully fill a valid test fixture here we need a combination of the mdehoog/go-ethereum@ac64c44 branch and the jwasinger/go-ethereum@06c795d for now in order to use the appropriate fork.

But as far as I am aware the mdehoog/go-ethereum@ac64c44 branch is mostly a test branch for 4844.

As there were a lot of merge conflicts between the two I manually added the changes from jwasinger/go-ethereum@06c795d to mdehoog/go-ethereum@ac64c44 here -> https://github.com/spencer-tb/go-ethereum/tree/eip-6780-test. So many branches :D

This successfully created a fixture for the first test create_selfdestruct_same_tx as intended, but not for the second test selfdestruct_prev_created. This failed with the exception:

Exception: expected account not found: 0x111111...1111111

I believe the expected outcome here is for account 0x1111...1111 to be found with 0x01 in storage slot 0x00 alongside its code. My understanding was that the account was being deleted like the normal selfdestruct behavior. The balance was however being successfully transferred to the account 0x0000...0000.

After some debugging I made a small change here:
spencer-tb/go-ethereum@5a1923d

It looked like the account was being marked as deleted if it called selfdestruct, regardless of whether it was created in the same transaction. After adding the extra logic here: (obj.suicided && obj.created) || ... from obj.suicided || ..., the second test behaved as expected and successfully filled the tests.

Hopefully this helps moving forward :D

[spencer-tb]

One thing I just realized is that the new selfdestruct behaviour will change the outcome of this test: https://github.com/ethereum/execution-spec-tests/blob/main/fillers/security/selfdestruct_balance_bug.py

I am thinking we could run the security tests in a different scope, or have a method of specifying a specific evm version for these tests.

[jwasinger]

@spencer-tb @danceratopz thanks for taking a further look into this.

It looked like the account was being marked as deleted if it called selfdestruct, regardless of whether it was created in the same transaction.

This is the existing behavior for selfdestruct that we retain for pre-6780 blocks. With 6780, the suicided flag is no longer set and now sendalled is set when an account is the originator of selfdestruct. The only case where we delete a non-empty state object is when the state object created and selfdestructed in the same tx ((obj.created && obj.sendalled)).

The reason the test was failing to fill is that the Cancun instruction set wasn't being enabled. I've added that to jwasinger/go-ethereum@0aebb62 and removed spencer-tb/go-ethereum@5a1923d . The tests now fill for me.

One thing I just realized is that the new selfdestruct behaviour will change the outcome of this test: https://github.com/ethereum/execution-spec-tests/blob/main/fillers/security/selfdestruct_balance_bug.py
I am thinking we could run the security tests in a different scope, or have a method of specifying a specific evm version for these tests.

IMO it should be sufficient to just disable filling this test for post-cancun/6780 forks.

[jwasinger]

IMO, a way to make it easier to fill tests for feature branches included with upcoming hard-forks (where the fork timestamp is unknown) would be to allow tf to take a custom genesis configuration.

[marioevz]

Hey @jwasinger, we recently made a lot of changes to the repo with the inclusion of the pytest framework, if you need any help running your tests with the new changes, just let me, @danceratopz or @spencer-tb and we can help you out :)

[danceratopz]

Hi @jwasinger, I just rebased this on main and refactored the tests to run with the new pytest-based execution-spec-tests framework - we can pick up work on these tests again!

You can run the tests with:

fill tests/cancun/eip6780_selfdestruct/ --fork=Shanghai --evm-bin=/path/to/jwasinger/go-ethereum/build/bin/evm  -v --runxfail 

The test test_selfdestruct_prev_created is currently marked to xfail, --runxfail is a pytest flag which runs this test as though it's not marked to xfail.

Couple of comments:

1. I tried to refactor the bytecode to use the Opcodes class: https://github.com/ethereum/execution-spec-tests/blob/a14258b19ff35ebefb25d33628dc8b34c9f76cb4/src/ethereum_test_tools/vm/opcode.py. Perhaps it helps you, I think the bytecode for test_create_selfdestruct_same_tx is still a work in progress? Either way feel free to use your original code instead.
2. The fork (Shanghai) in the above command-line call is currently wrong, as is the validity marker in the test:
   

   execution-spec-tests/tests/cancun/eip6780_selfdestruct/test_selfdestruct.py

   Line 19 in 7dc5a63

   @pytest.mark.valid_from("Shanghai")

   
   This is a work-around to run the test with the evm t8n tool from your branch - the framework would otherwise pass "Cancun" as the fork, which is currently not implemented in your branch (rightly so 🙂). We both need to have a look how to enable 6780 by specifying the EIP to evm t8n: We need to make a minor improvement on our side to (re-)enable this functionality (I think it was working, but relatively untested before). And I think you need to add 6780 to the list of "avaliable extra eips", I refer to this list from evm t8n --help:

             Shanghai
           Available extra eips:
               1153, 1344, 1884, 2200, 2929, 3198, 3529,
         3855, 3860
           Syntax <forkname>(+ExtraEip)
   

There's no rush on 2., as it currently stands we can resume work on these tests.

[jwasinger]

Thanks for updating this @danceratopz . I have pushed a change to the 6780 geth branch to make the EIP available as an extra eip via t8n.

[danceratopz]

Thanks for updating this @danceratopz . I have pushed a change to the 6780 geth branch to make the EIP available as an extra eip via t8n.

Perfect, thanks for the fast repsonse @jwasinger, we'll get that running on our side soon!

[marioevz]

Thanks for updating this @danceratopz . I have pushed a change to the 6780 geth branch to make the EIP available as an extra eip via t8n.

@jwasinger I added the fixture so the tests run both with --state.fork=Shanghai and --state.fork=Shanghai+6780, and did the appropriate expected storage changes each time.

I'll be adding some more tests to this branch in the next couple of hours if you don't mind :)

[marioevz]

Some test ideas:

• Destroy and re-create multiple times in the same tx
• Create and destroy multiple contracts in the same tx
• Create multiple contracts in a tx and destroy only one of them, but attempt to destroy the other one in a subsequent tx
• Create contract, attempt to destroy it in a subsequent tx in the same block
• Create a contract and try to destroy using another calltype (e.g. Delegatecall then destroy)

cc @winsvega

[shemnon]

New test idea, since sending ether to a non-existant account creates it:
single tx -

• call into non-existent account, sending value
• create that account with code that will self destruct
• call the code, self destruct.
• assert the account doesn't persist.

Variation of the above, but self destruct is in the initcode, skipping the call test but keeping the assert.

[marioevz]

Added more tests and packaged them all in here:

eip-6780-blockchain-tests.tar.gz

More tests are still missing but these should provide a nice start point for implementations.

[holiman]

Great that this is coming along. I'll try to check it out in more detail, but some of the things I'd like to ensure have coverage are:

• pre: Account X has 1 wei balance. tx: sender calls Y, which creates X as a contract with code, and then calls a method on it which calls selfdestruct(y). The selfdestruct should work, and Y should be gone.
• Tx: Contract X is created with code+balance 1. Does self-destruct to self. X should be gone and 1 wei ceased to exist.
• Pre: Contract x exists with code + balance 1. Tx: does self-destruct to self, which becomes a send-all. X should still exist, with 1 wei balance.

[marioevz]

Great that this is coming along. I'll try to check it out in more detail, but some of the things I'd like to ensure have coverage are:

* pre: Account `X` has `1` wei balance. tx: sender calls `Y`, which creates `X` as a contract with code, and then calls a method on it which calls `selfdestruct(y)`. The selfdestruct should work, and `Y` should be gone.

* Tx: Contract `X` is created with code+balance `1`. Does self-destruct to self. `X` should be gone and `1` wei ceased to exist.

* Pre: Contract `x` exists with code + balance `1`. Tx: does self-destruct to self, which becomes a send-all. `X` should still exist, with `1` wei balance.

I believe all points should be covered now:

1. covered by test_create_selfdestruct_same_tx, when parametrized with selfdestruct_contract_initial_balance > 0, assuming "X should be gone"
2. also covered by test_create_selfdestruct_same_tx when parametrized with selfdestruct_contract_initial_balance > 0 and also sendall_recipient_addresses contains self.
3. covered by test_selfdestruct_pre_existing when parametrized a selfdestruct_contract_initial_balance > 0, and also sendall_recipient_addresses contains self.

[marioevz]

Current implementation assumes all clarifications indicated by ethereum/EIPs#7308 are accepted.

[shemnon]

Besu passes all the tests for self destruct.

One question, is there a way to flag this warning as "expected" and not output it?

=============================== warnings summary ===============================
tests/conftest.py:22
  /Users/dannoferrin/git/github.com/ethereum/execution-spec-tests/tests/conftest.py:22: UserWarning: Compiling Yul source with Shanghai, not Cancun.
    warnings.warn("Compiling Yul source with Shanghai, not Cancun.")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================ 148 passed, 1 deselected, 1 warning in 11.43s =================

[danceratopz]

👍

[danceratopz]

Besu passes all the tests for self destruct.

One question, is there a way to flag this warning as "expected" and not output it?

=============================== warnings summary ===============================
tests/conftest.py:22
  /Users/dannoferrin/git/github.com/ethereum/execution-spec-tests/tests/conftest.py:22: UserWarning: Compiling Yul source with Shanghai, not Cancun.
    warnings.warn("Compiling Yul source with Shanghai, not Cancun.")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================ 148 passed, 1 deselected, 1 warning in 11.43s =================

@shemnon, you can suppress this warning on the command line using the following flag:

fill  -W "ignore::UserWarning:tests.conftest"

We could certainly consider removing this warning, because, as you mention, it's expected (if any tests use Yul) for forks under development (as solc typically won't add support for the fork until after it's deployed).