VM: Contribute missing EIP-1153 Transient Storage Test to execution-spec-tests

[holgerd77]

We soon have our new t8ntool for the execution-spec-tests repo finalized (see #3603) thanks to the great work from @jochem-brouwer! 🤩

I would like to give this a first test by trying to come up with a test for the EIP-1153 bug we had (see #3625, so: transient storage not cleared). I just tested this by filling the 1153 tests with Jochem's instructions with:

fill -vv -x --evm-bin=../ethereumjs-monorepo/packages/vm/test/t8n/ethereumjs-t8ntool.sh --fork=Cancun tests/cancun/eip1153_tstore

This passes with on our current master but it passes also if one is doing the respective code change (or actually: reverse) in the EVM from the bugfix PR (so: delete the this.postMessageCleanup() call and (!) rebuild the EVM) and then run the test filler again.

So this scenario is apparently not tested yet in the collected tests.

If I get things together correctly I would assume this can be tested by running two txs in a block, the first doing some TSTORE and writing something to transient storage and the second tx reading from the same position. So we should come up with a scenario here which does fail when re-adding the bug to the code base and passes if the fix code is in.

Also, if I get correctly, I assume that this type of test (with 2 txs) needs a BlockchainTestFiller, I looked a bit around and this EIP-7685 test seems to be a somewhat simple blueprint on how to setup such a test, maybe used as an inspiration together with a simple state test like the access list test file as well as having some look at how other 1153 tests are done.

I would like to have the above confirmed by @jochem-brouwer (Jochem can you do that?), so that it's clear that all thest assumptions make sense!

Eventually I try to do this myself, or we make this a joint effort, or someone else takes the lead. The main thing should be that this is not done too quickly 😋 and rather in a step-by-step way over the next week or so so that this can serve as some educational project for the team on how to write these kind of tests! 🙂

[jochem-brouwer]

Hi Holger, you are correct. If this test case was actually in the tests, our CI would have reported the bug, and the bug would never be there :)

To setup the test which should trigger this test, you need two transactions:

The first is a CREATE transaction (so with a to: null field). This should TSTORE something. (Let's TSTORE 1 in slot 1). After this transaction, the new transaction should call into the deployed contract (which the previous transaction deployed). The code itself should now TLOAD slot 1, and then SSTORE this value into some slot (lets take slot 2). The expectation is that this stores 0 into the slot 2, so it does nothing for the state output. However, if we revert the change on master (so remove the postMessageCleanup) it will actually store 1 in slot 2.

So this test might look somewhat easy to make, it is a bit tricky, since tx 1 does two things, it should (1) TSTORE and (2) deploy the code for tx 2.

[jochem-brouwer]

As a side note, I actually wanted to add this test to the execution-spec-tests, it's super great that this issue is opened, thanks a lot 👍 We should indeed document the steps so we can provide more tests later on!

[jochem-brouwer]

Have assigned this to myself, will write down the process here.

[jochem-brouwer]

Process to add test: (will edit this). PR is here ethereum/execution-spec-tests#798

1. Setup execution-spec-tests as usual
2. Follow this: https://ethereum.github.io/execution-spec-tests/v2.1.1/getting_started/setup_vs_code/ (might have to reload VSCode)
3. Each time a py file is saved, some checks are ran and auto-lint should happen
4. tox -e tests will verify the format

I started by copying one of the available tests to edit. Some gotchas:

• Tests which require more than one tx need a blockchain need a blockchain_test instead of a state_test
• There are helpful methods, such as Initcode where one can add arguments like deploy_code (contract deployed code) and initcode_prefix (whatever code is executed before dumping the contract)
• To append opcodes, use +, like Op.SSTORE(1, Op.TLOAD(1)) + Op.TSTORE(1, 1)

To get the contract deployment address, use:

    deployment_tx = Transaction(
        gas_limit=100000,
        data=code,
        to=None,
        sender=sender,
    )

    address = deployment_tx.created_contract

To build the test in question, we need to figure out how to write this test. Our error case happened (only) in the following situation: (1) a contract is created (at tx-level, so with to field set to null). When this contract is created, the transient storage is filled (it is non-empty). (2) after creating the contract, with a "normal" tx we call the created contract, and then saw that the transient storage is not empty (where it should be).

So, a simple test format would be: (1) create a contract which sets (for instance) transient storage slot 1 to value 1, and then deploy a contract which simply stores into the contract storage the value loaded from the transient storage at slot (1).

We can use the Initcode execution-spec-tests helper for this. It allows to set deploy_code (the code which is ran when being called by a tx) and the initcode (code which is being ran when deploying the contract).

Here is the relevant code:

@pytest.mark.valid_from("Cancun")
def test_tstore_clear_after_deployment_tx(
    blockchain_test: BlockchainTestFiller,
    pre: Alloc,
):
    env = Environment()

    init_code = Op.TSTORE(1, 1)
    deploy_code = Op.SSTORE(1, Op.TLOAD(1))

    code = Initcode(deploy_code=deploy_code, initcode_prefix=init_code)

This thus matches the example text we just wrote to write this test. To deploy the contract:

    sender = pre.fund_eoa()

    deployment_tx = Transaction(
        gas_limit=100000,
        data=code,
        to=None,
        sender=sender,
    )

`pre.fund_eoa() will ensure that the sender has enough balance to cover the transaction costs (defaults to funding account with 1000 eth)

The deployment_tx has data=code (this is the initcode we just created), set to explicitly to None to ensure we are creating a contract, set a high-enough gas limit (not doing this will default the tx gas limit to 21k, which thus cannot cover the costs at all), and obviously the sender to whatever account we just funded.

We now have deployed the contract and should call it, for this we need to know the address the contract we just deployed at. Luckily, execution-spec-tests has this provided, and we can directly invoke:

    address = deployment_tx.created_contract

    invoke_contract_tx = Transaction(gas_limit=100000, to=address, sender=sender)

The gas_limit is again set to something higher than 21k, since we are calling a contract and thus need some execution gas.

The final thing to do is to run the test and to validate our expected output (the storage slot 1 of the address should stay empty (0)):

    txs = [deployment_tx, invoke_contract_tx]

    post = {
        address: Account(storage={0x01: 0x00}),
    }

    blockchain_test(genesis_environment=env, pre=pre, post=post, blocks=[Block(txs=txs)])

Running this on EthereumJS with the bug fixes (passes) and the bug un-applied (comment out the postMessageCleanup) will fail the test, so just verified that the test does what we want.

Full pytest code:

@pytest.mark.valid_from("Cancun")
def test_tstore_clear_after_deployment_tx(
    blockchain_test: BlockchainTestFiller,
    pre: Alloc,
):
    env = Environment()

    init_code = Op.TSTORE(1, 1)
    deploy_code = Op.SSTORE(1, Op.TLOAD(1))

    code = Initcode(deploy_code=deploy_code, initcode_prefix=init_code)

    sender = pre.fund_eoa()

    deployment_tx = Transaction(
        gas_limit=100000,
        data=code,
        to=None,
        sender=sender,
    )

    address = deployment_tx.created_contract

    invoke_contract_tx = Transaction(gas_limit=100000, to=address, sender=sender)

    txs = [deployment_tx, invoke_contract_tx]

    post = {
        address: Account(storage={0x01: 0x00}),
    }

    blockchain_test(genesis_environment=env, pre=pre, post=post, blocks=[Block(txs=txs)])

This is placed inside ./tests/cancun/eip1153_tstore/test_tstore_clear_after_tx.py. To run/test it:

fill -k test_tstore_clear_after_deployment_tx --fork Cancun tests/cancun/eip1153_tstore --evm-bin=<PATH_TO_ETHEREUMJS_T8N.sh>

I have submitted this code as PR, the only thing I had to do to make the CI pass was to run black tests locally to reformat/lint the tests.

[jochem-brouwer]

Ok, I'm sorry, I am doing this wayyy too quickly. It might be worth to setup maybe a meeting next week to do some pair programming and step through these steps for those who are interested?

[holgerd77]

Ok, I'm sorry, I am doing this wayyy too quickly. It might be worth to setup maybe a meeting next week to do some pair programming and step through these steps for those who are interested?

Ah, so for met that's ok! 😄 I think it's generally a good idea that you actually do this test, guess that is in fact quickest/easiest (also these somewhat complex CREATE constallations you mentioned), but generally just also put a somewhat stronger emphasis on the docs part and generally taking us with you! 🙂 The above was already a really good start! 👍

[jochem-brouwer]

I will update the comment with further additions! 😄

[jochem-brouwer]

I have updated the comment with a full breakdown on the test and how to write it, hope this helps 😄 👍

[jochem-brouwer]

The TSTORE tests have been merged into execution-spec-tests, so I will close this one!

However is there somewhere a place I can copy / pin this comment: #3666 (comment) for future references to write tests?

ethereum/execution-spec-tests#798
ethereum/execution-spec-tests#799

[holgerd77]

🤩 👍

I would create a new section in https://github.com/ethereumjs/ethereumjs-monorepo/tree/master/packages/vm/test/t8n and it would also be good if we link to respectively mention the whole document in https://github.com/ethereumjs/ethereumjs-monorepo/tree/master/packages/vm#development .