Failure to decode Prague-statetests with auths

[holiman]

I reported this via Cantina already, as I thought it was a security issue.

Example statetest

{
  "00000012-mixed-0": {
    "env": {
      "currentCoinbase": "b94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "currentDifficulty": "0x200000",
      "currentRandom": "0x0000000000000000000000000000000000000000000000000000000000200000",
      "currentGasLimit": "0x26e1f476fe1e22",
      "currentNumber": "0x1",
      "currentTimestamp": "0x3e8",
      "previousHash": "0x044852b2a670ade5407e78fb2863c51de9fcb96542a07186fe3aeda6bb8a116d",
      "currentBaseFee": "0x10"
    },
    "pre": {
      "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b": {
        "code": "0x",
        "storage": {},
        "balance": "0xffffffffff",
        "nonce": "0x0"
      }
    },
    "transaction": {
      "maxFeePerGas": "0x10",
      "maxPriorityFeePerGas": "0x10",
      "nonce": "0x0",
      "to": "0xa94f5374Fce5edBC8E2a8697C15331677e6EbF0B",
      "data": [
        "0xa7b60ed2052378dae9393a455e165b151e1d32739a49d96a"
      ],
      "gasLimit": [
        "0x116d8"
      ],
      "value": [
        "0x5fc5"
      ],
      "secretKey": "0x45a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8",
      "sender": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "authorizationList": [
        {
          "chainId": "0x0",
          "address": "0x00000000000000000000000000000000000000f1",
          "nonce": "0x0",
          "v": "0x0",
          "r": "0x667b9452dc136352a55a76bd52a7e086df817d87c22c0247767cd671a798334a",
          "s": "0x358363478dd4436d20378032ad0b50212e6a8ef5038d529c969f8c650108992c",
          "signer": "0x19e7e376e7c213b7e7e7e46cc70a5dd086daff2a"
        },
        {
          "chainId": "0x0",
          "address": "0x5cbdd86a2fa8dc4bddd8a8f69dba48572eec07fb",
          "nonce": "0x1",
          "v": "0x1",
          "r": "0x15330e6c0a850bf0446bf5fc66749f73a9848cbb1389085e911f9eac69d356e7",
          "s": "0x50ccbbd41435a2e3952d3c1bc712736cf7cff2c3ee47ebf546590aaba9a47815",
          "signer": "0x19e7e376e7c213b7e7e7e46cc70a5dd086daff2a"
        }
      ]
    },
    "out": "0x",
    "post": {
      "Prague": [
        {
          "hash": "0x87f5cdf75bf22bf9c68961303bed8e64b54e9ae6502be414b5a39f94254677eb",
          "logs": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
          "indexes": {
            "data": 0,
            "gas": 0,
            "value": 0
          }
        }
      ]
    }
  }
}

This fails on erigon, because the second auth get's a yParity value of 0 -- the v is just ignored.
Erigon simply decodes auths in statetests using the same json as the auths in transactions, which is now what is used by the reference tests, nor the other clients.

[Giulio2002]

Thanks for reporting. what version of Erigon?

[holiman]

Recent, any/all versions

[somnathb1]

The use of auths is only in context of EIP-7702, the main document of which suggests the field name y_parity, whose json naming should be yParity. Please suggest a reason to differ from that.

[holiman]

The only reason I can provide is that the actual reference-tests use 'v', and all other clients (at least besu, reth, geth, nethermind and evmone) support it. Reth also actively rejects 'yParity', otherwise a workaround would have been to have both. As it is, erigon cannot pass the de-facto reference tests for Prague.

[somnathb1]

But spec is spec. When I am not paying attention, someone could just align everything to the spec. All "other" clients did kill Holesky a week or so ago, so we are walking on egg-shells now.

[somnathb1]

Just to be clear, before people start barging in about tests passing, that I am not saying Erigon shouldn't pass the tests. I am only suggesting the tests to be aligned to the specs, to avoid confusion. Even if I did change it to pass tests now, there is no correct reason for the tests to continue with the "wrong" naming.

[holiman]

Spec is spec sure, but we're talking about statetest spec, not tx spec. Not sure where the statetest spec lives. @danceratopz any input?

[Giulio2002]

seems only a mismatch so I am going to demote importance for the sake of our release process

[danceratopz]

Spec is spec sure, but we're talking about statetest spec, not tx spec. Not sure where the statetest spec lives. @danceratopz any input?

The statetest spec is currently not easily accessible, the fields for an AuthorizationTuple are defined in this class and its method: https://eest.ethereum.org/main/library/ethereum_test_types/?h=authorizationtuple#ethereum_test_types.AuthorizationTuple

This is not acceptable and ethereum/execution-spec-tests#1282 fixes the schema export of our fixture formats and we will make them readily available in the future.

I agree with @somnathb1 that the naming of the statetest field v is not as expected (it should be yParity, as per the EIP-7702 spec). However, I would strongly prefer to not change the naming of a fixture field at this stage in the fork dev cycle due to the high chance of introducing other errors and the incompatibility with other existing EEST pectra fixture releases, I hope Erigon understands this and can fix the discrepancy on your side.

CCing @marioevz @spencer-tb @winsvega

[somnathb1]

Spec is spec sure, but we're talking about statetest spec, not tx spec. Not sure where the statetest spec lives. @danceratopz any input?

The statetest spec is currently not easily accessible, the fields for an AuthorizationTuple are defined in this class and its method: https://eest.ethereum.org/main/library/ethereum_test_types/?h=authorizationtuple#ethereum_test_types.AuthorizationTuple

This is not acceptable and ethereum/execution-spec-tests#1282 fixes the schema export of our fixture formats and we will make them readily available in the future.

I agree with @somnathb1 that the naming of the statetest field v is not as expected (it should be yParity, as per the EIP-7702 spec). However, I would strongly prefer to not change the naming of a fixture field at this stage in the fork dev cycle due to the high chance of introducing other errors and the incompatibility with other existing EEST pectra fixture releases, I hope Erigon understands this and can fix the discrepancy on your side.

CCing @marioevz @spencer-tb @winsvega

Yeah but if I change it, something else might break. Someone may misunderstand something and re-change it or use the json incorrectly later on. Any argument like different "Txn Spec", "Statetest spec", "Other clients understanding" will not help with that. Every debug will start with reading 7702 spec, and confusions will pile on (just like it did when I first started looking into this issue report). The change is small, but the impact down the line could be big.
Tests should reflect mainnet behaviour. If that's different, I will change it. If the tests have a difference with that, I would much rather fail the test correctly. This is more important at this juncture of fork dev cycle.

[VBulikov]

somnathb1 added Imp2 as Importance

[holiman]

Note, though, we already have a different json-representation of a "statetest-transaction", from the regular "actual transaction". The reason for this is that the statetest has a sender field, which can be used instead of the r,s,v fields.

Similarly, the "statetest-auth" has a signer field. You can choose to use the signer-field instead of deriving the signer via the v/r/s-fields, if you wish.

So we'll have to maintain two somewhat differing specs regardless. But I agree the mixup is a bit unfortunate.

I don't overly care who changes what, but I operate a fuzzer which generates statetests to find differences in clients. Currently, I cannot run reth and erigon on the same tests, so in practice that means that one of them is getting less coverage.

[holiman]

Sweet, reth fixed the problem on their side, by not actively rejecting yParity: bluealloy/revm#2150

[danceratopz]

• EEST will additionally include the yParity field in statetest fixture formats, which is duplicated as v (to prevent breaking changes for clients):
  • bug(fill,types): duplicate FixtureAuthorizationTuple's v field as yParity ethereum/execution-spec-tests#1286
• Reth updated their fixture consumer to accept a fixture format which contains both v and yParity
  • test: allow duplicate v and yparity in test files bluealloy/revm#2150

We'll make a fixture release which will include the yParity change, then erigon will be able to correctly run all the state tests without any changes on your side.