Clarify hard fork names in transaction tests

[norswap]

Currently, in the TransactionTests, multiple names are confusing:

See for instance this logic I implemented in my toy client:

    /** Returns the starting block height for the given version. */
    private static int blockHeight (String version) {
        return switch (version) {
            case "Byzantium"            -> BYZANTIUM.startBlock;
            case "Constantinople"       -> CONSTANTINOPLE.startBlock;
            case "ConstantinopleFix"    -> CONSTANTINOPLE.startBlock;
            case "EIP150"               -> TANGERINE_WHISTLE.startBlock;
            case "EIP158"               -> SPURIOUS_DRAGON.startBlock; // (*)
            case "Frontier"             -> FRONTIER.startBlock;
            case "Homestead"            -> HOMESTEAD.startBlock;
            case "Istanbul"             -> ISTANBUL.startBlock;
            default -> throw new AssertionError("unreachable");

            // (*) if interpreted as EIP-161 that supersedes EIP-158
        };
    }

The left column are the names used in the transaction test JSON files.

1. The "EIP" names should be the proper forks name, and "ConstantinopleFix" should probably be called "SaintPetersburg".
2. I think EIP158 was never actually implemented on mainnet, as it was superceded by EIP-161. I'm not sure how a client is supposed to test the "EIP158" results. Was EIP158 deployed earlier on test nets perhaps?
3. Was the "Berlin" fork skipped on purpose because all results are expected to be the same? If not, is there some automated way to run the tests in a client at a Berlin block height and update the test files?

[axic]

This was previously discussed in #488, and probably the people sharing your view are still on that opinion. Here was my suggestion #488 (comment).

[norswap]

I see, I guess it's going to stay the same! I can live with it.

The argument that it's closed doesn't really work for new client implementations, as you need to implement everything including networking, JSON-RPC, etc to be able to run retesteth.

Feel free to close then, although I'd still like to know the answer of question (3) about the Berlin fork.

[winsvega]

About the #488
fork names in the tests are internal for the tests. the names can be different in clients. there is a config with replacement map for each client. in retesteth.

implementing retesteth support does not mean implementing the whole client. retesteth just feed the test data via rpc / t8ntool protocol. so devs don't have to implement test parser.

about the transaction tests.
there is currently no way to generate transaction tests. so they stay unupdated.
we are introducing transaction exceptions in state tests for clients to reject transactions on the given state.

"I'm not sure how a client is supposed to test the "EIP158" regardless of its name in tests the fork represent the actual fork happend on mainnet.

[norswap]

Understood.

implementing retesteth support does not mean implementing the whole client. retesteth just feed the test data via rpc / t8ntool protocol. so devs don't have to implement test parser.

To clarify, if one wants to write a new Ethereum client, this means you have to implement the JSON-RPC endpoint before you can run any tests on your WIP implementation. My implementation is very early stage, so I can only create transactions and validate their format & signature. So I'm only interested in pure transaction tests. It sounds like I could implement a JSON-RPC end points that just gets these tests - I'll look into it.

we are introducing transaction exceptions in state tests for clients to reject transactions on the given state.

That wouldn't be useful for my (specific) use case, since I can't run transactions yet. I just want to see a lot of different transaction formats so that I can test if I can validate them correctly. Anything stateful would require a ton more implementation (almost a full client, excepted the networking layer).

So what I am saying is that stateless transaction tests are valuable for new ethereum client implementations.

[qbzzt]

By "stateless transaction test" you mean that you just want the transaction bytes in RLP format and what they mean?

You can get this from our state tests. For example, take https://github.com/ethereum/tests/blob/develop/GeneralStateTests/VMTests/vmLogTest/log1.json. You can see the transaction's fields in log1.transaction. Some fields are lists because they can have multiple values.

{
         "data":[
            "0x693c61390000000000000000000000000000000000000000000000000000000000000000",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000001",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000002",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000003",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000004",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000005",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000006",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000007",
            "0x693c61390000000000000000000000000000000000000000000000000000000000000008"
         ],
         "gasPrice":"0x0a",
         "gasLimit":[
            "0x04c4b400"
         ],
         "nonce":"0x00",
         "secretKey":"0x45a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8",
         "to":"0xcccccccccccccccccccccccccccccccccccccccc",
         "value":[
            "0x01"
         ]
}

Then, if you look in log1.post.Berlin[0], you'll see

            {
               "indexes":{
                  "data":0,
                  "gas":0,
                  "value":0
               },
               "hash":"0x67aa350ca5bc00d8be8b39d4199c4e02a1b00997d1a773e0e09d461f9a547436",
               "txbytes":"0xf885800a8404c4b40094cccccccccccccccccccccccccccccccccccccccc01a4693c613900000000000000000000000000000000000000000000000000000000000000001ba0e8ff56322287185f6afd3422a825b47bf5c1a4ccf0dc0389cdc03f7c1c32b7eaa0776b02f9f5773238d3ff36b74a123f409cd6420908d7855bbe4c8ff63e00d698",
               "logs":"0xae5bdbf9098f61cb46facba16127337099fc06e9bb56c37394d42995e9c53488"
            }

So the transaction bytes are log1.post.Berlin[0].txbytes and the values for data, gasLimit, and value are at location zero in their respective lists (log1.transaction.data, log1.transaction.gasLimit, and log1.transaction.value). You can interpret the other entries in log1.post.Berlin the same way, using the indexes field to know where to look in the lists in the transaction.

For your use case the difference between Berlin and London is irrelevant, since they encode transactions the same way in most cases.

[winsvega]

to make transaction structure tests that you saying we need to come up with a protocol that I can feed to t8ntool or by rpc
with the meyoaning of
"is this transaction structure could possibly be legit: 0xfc24586....235adda990"

by possibly legit I mean according to the YP. (without dependence on state)
so nonce is not validated against sender's account nonce. balance is not validated either.
what is left to check is if transaction fields are in valid range of data 0...2**256 or whatever the limits are.
and as I remember in transaction tests intrinsic gas price is taken in account.

the problem is that devs do not export internal logic for such thing. nor by rpc nor by other things. there were 2 years discussion before I could get t8ntool from the geth team. to at least get state transitions.

the paradigm that I want is that I get confiramtion from clients on wether the transaction structure is legit or not. if I would be the one verifing the structure myself it will just introduce more error points.

if you wish we can come up with transaction structure tests using your prototype and t8ntool command like tool that you could export.

[norswap]

@qbzzt That's nice! Questions:

1. Does it include negative tests also? That's one of the most important things to test, that you don't let through invalid things, especially on tricky boundary cases.
2. I am guessing however that these tests are not geared towards testing the format of transctions right? (Which the TransactionTests tests are, e.g. AddresMoreThan20.json)

@winsvega
I think I get what you're saying, but I'm not sure. You're saying existing clients have no entry point to run these tests? Or they have no entry points to validate transactions, such that you'd be confident to use them as test cases? (in that case, I guess you could just try them on a local test net however).

For me personally, just having the JSON files is useful already. Parsing them is not difficult. Maybe it doesn't fit with how this repo is used with retesteth, but that's just my perspective :)

[qbzzt]

• Does it include negative tests also? That's one of the most important things to test, that you don't let through invalid things, especially on tricky boundary cases.

No negative tests that fail on the data structure. You can find those in the TransactionTests, the ones without a hash for any fork are invalid data.

• I am guessing however that these tests are not geared towards testing the format of transctions right? (Which the TransactionTests tests are, e.g. AddresMoreThan20.json)

Yes. The state tests are geared towards testing complete clients, not just data parsing, so they don't include invalid examples.

The TransactionTests haven't been touched in over two years, probably because there hasn't been new client development and the existing clients didn't modify that part of the code so they didn't need to test it either. Note that that's just my theory, I wasn't involved in testing until last year.

[winsvega]

Now we have some invalid transactions in state tests.

For me personally, just having the JSON files is useful already. Parsing them is not difficult. Maybe it doesn't fit with how this repo is used with retesteth, but that's just my perspective :)

So to the other devs. But have you ever thought how those JSONs appeared initially?
Its not handwritten. We used aleth implementation. Now to generate the tests I can write my own transaction implementation but that would be bad as I might make a mistake. Like ori just recently found what might be a bug in my difficulty formula on edge case.

What we need is a way to ask client if it considers certain transaction valid struct or not. But devs prefere their own tests or just ready made jsons

[winsvega]

the transaction tests are being reworked. fork names identical to state tests.