Isthmus: withdrawals root in block header

[protolambda]

Description

L2 withdrawals storage root is retrieve from the state upon block-sealing, and then inserted into the header as withdrawals-root.

The L1 withdrawals-root is computed as an MPT hash of withdrawals operations. The OP-Stack uses contract-storage instead. In some places this means we have to verify the block-body withdrawals-list is empty, while the header is set to the storage root.

Tests

Work in progress.

Additional context

This PR is used as feature-branch. Changes will not be merged into optimism until the withdrawals changes (specs, op-geth, op-node, and ideally other implementations) are complete for Holocene upgrade.

Metadata

Part of ethereum-optimism/optimism#12044

[vdamle]

Rebased changes in this branch with latest changes in optimism branch

[protolambda]

Functionality is almost good to go.
I did a full review of all withdrawal mentions in op-geth, and found some minor issues, and I pushed a fix commit for the tedious bits.

• There is a quirk where newFetchResult sees a non-nil withdrawals-root,
  and does not match the hash of empty withdrawals, and it will try fetch withdrawals that aren't there.
  This is fine, we can ignore it for a simple diff, but it is a slight optimization loss.

• In a few places it was logging a common.Hash with %x, which I believe first calls .String(), and then formats the string as hex.
  This is double hex encoding, not desired. Changed it to %s. Would may be a neat little PR to upstream geth, albeit only better error messaging.

• debug_executionWitness I think will work without changes,
  since it happens to run ValidateState with a side-effect on the pre-fetcher because of the storage-root retrieval during validation.
  I added a warning to the debug method implementation, so it's not such an opaque side-effect.

• beacon/exec_data_test.go fuzz test is kind of broken, and should not be merged in its current state:

  • Chained decode code-paths in single fuzzer -> e.g. we don't reach exec data if receipt fails.
  • If we do want this fuzz test, then there should be be multiple fuzz definitions, one per time. Perhaps using a helper func with generics,
    to not repeat the var value V, decodeEncodeJSON(input, &value) ... each time.
  • Raw byte array as json input -> fuzzer has to guess at key words, go fuzzer is not that good.
  • It is only testing types not even part of the beacon package (light client beacon-chain support),
    we shouldn't have to touch this package much at all, except ExecutableData because of the engine-API.
  • Try to put a simple panic in ExecutableData -> no panic actually hit. Always many iterations, then some nonsense other JSON related error (see below).

    // example of something you could add to `func (e ExecutableData) MarshalJSON()` in `gen_ed.go` without effect:
    if enc.BlobGasUsed != nil {
        panic("the fuzzer found a JSON field")
    }
    

  • It does hit other nonsense things like: (bytes 7b7d = utf8 {}, just short enough to generate after 145272 attempts on my machine):

    --- FAIL: FuzzJSON (13.00s)
    --- FAIL: FuzzJSON (0.00s)
        exec_data_test.go:43: encode-decode is not equal, 
            input : 7b7d
            output: 7b2252656365697665644174223a22303030312d30312d30315430303a30303a30305a222c22526563656976656446726f6d223a6e756c6c7d
    

    And the output here is {"ReceivedAt":"0001-01-01T00:00:00Z","ReceivedFrom":null}, i.e. internal fields of the block-type,
    because types.Block isn't actually meant to be json-encoded/decoded (there are API different handlers for it).
  • Or another fun one: hex: 5b205d, utf8: [ ]. Not equal to 5b5d, utf8 [], i.e. whitespace is lost in JSON.

• The fork.yaml EVM t8ntool entry doesn't mention any file path.

• Please change each of the commits to the <domain>: <change description> format that geth uses.

• The commits author info (email or name) isn't set to your github account data exactly, you may want to amend those commits.

[vdamle]

@protolambda Thank you for taking a look, for the changes you pushed and the suggestions. I have done the following:

• Cleaned all commit messages to adhere to the format. Couple of commits have additional descriptions, from what I can tell, that should be fine.
• Updated the author email to the public email listed in my github profile.
• Split the fuzz tests into two files:
  • exec_data_test.go now tests only ExecutableData
  • core types are now fuzz tested in core/types/json_fuzzer_test.go -- with a generic fuzzJSONForType()
  • split the fuzz tests into individual once to address the problem you highlighted. Also sanitized the input/output by stripping out spaces/CR/LF so that the checks can complete.
  • I have added a seed corpus struct for all fuzz tests and a f.Add(object). It obviously increases the diff size, but seems to be a best practice noted in go fuzz testing doc.

Please take a look whenever you get a chance. FYI @tynes

[protolambda]

LGTM, but after discussing with @tynes I think we are over-thinking the json fuzz-tests:

• Even if they work, upstream geth seems to do fine without them
• The json functionality is auto-generated with a lib that is widely used in geth already.
• The fuzzing doesn't seem to run in CI or get enough fuzzing time to be meaningful, especially since it's fuzzing the raw JSON, and even with a fuzz-corpus hint it might not get far.

Let's drop the fuzz tests and get this merged.

[vdamle]

LGTM, but after discussing with @tynes I think we are over-thinking the json fuzz-tests:

• Even if they work, upstream geth seems to do fine without them
• The json functionality is auto-generated with a lib that is widely used in geth already.
• The fuzzing doesn't seem to run in CI or get enough fuzzing time to be meaningful, especially since it's fuzzing the raw JSON, and even with a fuzz-corpus hint it might not get far.

Let's drop the fuzz tests and get this merged.

Thanks for the feedback @protolambda @tynes

I've dropped the fuzz tests from the earlier commit and rebased against the updated base branch. Should be ready for a final pass and hopefully it's good to go!

[protolambda]

LGTM! (cannot approve my own PR, but at this point it's really @vdamle authoring the commits here)