Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: fix flaw in state test runner #26299

Closed
wants to merge 1 commit into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 4, 2022

This PR fixes a flaw uncovered by @guidovranken in a bounty-report. Essentially, even on berlin rules, geth reacts to the RANDOM being set, and clears the DIFFICULTY, resulting in the DIFFICULTY op returning zero later on.

Geth trace, where geth spits out 0x0 for difficulty:

{"pc":0,"op":68,"gas":"0xb6c8f8","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"DIFFICULTY"}
{"pc":1,"op":105,"gas":"0xb6c8f6","gasCost":"0x0","memSize":0,"stack":["0x0"],"depth":1,"refund":0,"opName":"PUSH10"}

I modified the statetest from the report a bit, so that it has currentDifficulty at 0x020001 and currentRandom set to ..2002, to disambiguate the two:

{
  "TEST": {
    "env": {
      "currentBaseFee": "0x0a",
      "currentCoinbase": "0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba",
      "currentDifficulty": "0x020001",
      "currentGasLimit": "0xb71b00",
      "currentNumber": "0xc5d488",
      "currentRandom": "0x0000000000000000000000000000000000000000000000000000000000020002",
      "currentTimestamp": "0x03e8",
      "previousHash": "0x5e20a0453cecd065ea59c37ac63e079ee08998b6045136a8ce6635c7912ec0b6"
    },
    "post": {
      "Berlin": [
        {
          "hash": "0x2063f93162ceacaf61a2cb7b6875eb701e788e03070af77c25d33b4d68cf81ee",
          "indexes": {
            "data": 0,
            "gas": 0,
            "value": 0
          },
          "logs": "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
          "txbytes": "0x"
        }
      ]
    },
    "pre": {
      "0x6105f4d50D5A1B669E0C87e67Ba663A6Bf1957D7": {
        "balance": "0x00",
        "code": "0x4469614de8ee56f00cbc4139fdc02589cc81d36b",
        "nonce": "0x00",
        "storage": {}
      },
      "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b": {
        "balance": "0x0fffffffffffffffffffffffffffffffffffffffffffffffff",
        "code": "0x",
        "nonce": "0x00",
        "storage": {}
      }
    },
    "transaction": {
      "data": [
        "0x"
      ],
      "gasLimit": [
        "0xb71b00",
        "0x0ee6b280"
      ],
      "gasPrice": "0x0a",
      "nonce": "0x00",
      "secretKey": "0x45a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8",
      "sender": "0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "to": "0x6105f4d50D5A1B669E0C87e67Ba663A6Bf1957D7",
      "value": [
        "0x00"
      ]
    }
  }
}

With this PR, geth now correctly spits out 0x20001

{"pc":0,"op":68,"gas":"0xb6c8f8","gasCost":"0x2","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"DIFFICULTY"}
{"pc":1,"op":105,"gas":"0xb6c8f6","gasCost":"0x3","memSize":0,"stack":["0x20001"],"depth":1,"refund":0,"opName":"PUSH10"}

As for besu and nethermind, nethermind erroneously spits out 0x20002

{"pc":0,"op":68,"gas":"0xb6c8f8","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"DIFFICULTY"}
{"pc":1,"op":105,"gas":"0xb6c8f6","gasCost":"0x0","memSize":0,"stack":["0x20002"],"depth":1,"refund":0,"opName":"PUSH10"}

But besu correctly spits out 0x20001:

{"pc":0,"op":68,"gas":"0xb6c8f8","gasCost":"0x0","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"DIFFICULTY"}
{"pc":1,"op":105,"gas":"0xb6c8f6","gasCost":"0x0","memSize":0,"stack":["0x20001"],"depth":1,"refund":0,"opName":"PUSH10"}

cc @MarekM25 @LukaszRozmej

This test does only

DIFFICULTY
PUSH10 0x614de8ee56f00cbc4139
REVERT

It caused a differing state root because REVERT(0x614de8ee56f00cbc4139, 0) succeeds (size zero) whereas a non-zero size fails. However, the stateroot does not change if 0x20002 is used instead of 0x20001. So it's not a "great" test to include as is in the reference tests, but it should be fairly simple to tweak it and include it. cc @winsvega

@winsvega
Copy link
Contributor

winsvega commented Dec 4, 2022

The random being set in t8n env params on <=berlin? So its a t8n bug only?

In the tests if current Difficulty is set and current random is set together. Current random will be passed to t8n only on >=Merge, current Difficulty is passed for test generated <Merge. If no difficulty is set it comes as default 0x020000 on pre Merge. If no current random is set, the current Difficulty is used as Random on Merge. This when formibg evm t8n env file

@holiman
Copy link
Contributor Author

holiman commented Dec 5, 2022

This is not t8n, but the execution of state-tests. The state tests contains only one env section, and the env section contains both difficulty and random:

    "env": {
      "currentBaseFee": "0x0a",
      "currentCoinbase": "0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba",
      "currentDifficulty": "0x020001",
      "currentGasLimit": "0xb71b00",
      "currentNumber": "0xc5d488",
      "currentRandom": "0x0000000000000000000000000000000000000000000000000000000000020002",
      "currentTimestamp": "0x03e8",
      "previousHash": "0x5e20a0453cecd065ea59c37ac63e079ee08998b6045136a8ce6635c7912ec0b6"
    },

Since it's a general state tests, it may contain multiple post-sections, but in this case there was only one, for Berlin. The state test runner should then use 0x20001 as the number to return if the DIFFICULTY/RANDOM opcode is processed.

@holiman
Copy link
Contributor Author

holiman commented Dec 6, 2022

I just tried updating the tests, btw, and the bug that this PR fixes also triggers on cases like https://github.com/ethereum/tests/blob/develop/GeneralStateTests/stRandom/randomStatetest153.json#L13 .

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, I think I remember when I wrote this I thought that random should be applied whenever set, but it makes more sense to only set it post-london. It kinda means that tests without random for london are not really possible though (when random is defined)

@holiman
Copy link
Contributor Author

holiman commented Dec 6, 2022

This (alone) causes test failures, I think it's better to do in tandem with a test-update, as in #26314

@holiman holiman closed this Dec 6, 2022
holiman added a commit that referenced this pull request Dec 20, 2022
This PR builds on #26299, but also updates the tests to the most recent version, which includes tests regarding TheMerge.

This change adds checks to the beacon consensus engine, making it more strict in validating the pre- and post-headers, and not relying on the caller to have already correctly sanitized the headers/blocks.
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
This PR builds on ethereum#26299, but also updates the tests to the most recent version, which includes tests regarding TheMerge.

This change adds checks to the beacon consensus engine, making it more strict in validating the pre- and post-headers, and not relying on the caller to have already correctly sanitized the headers/blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants