Uncle Validation Test

[acolytec3]

In #1959 when migrating the blockchain dependent validation methods and tests from block to blockchain, I came across this test. It clearly depends on having a blockchain available to check validity of uncles and parent headers around fork blocks but some of the changes we made in terms of block validation (where a block validation method will throw in the constructor if a block has invalid parameters of whatever kind) required a fairly significant rework of the original test and I'm not 100% sure I got everything right (or if we even need to keep this test). The piece that most confused me was the last part of the test:

London block has legacy uncles, where hardforkByBlockNumber set to false (this should not throw)
In this situation, the london block creates a london uncle, but this london uncle should be
a berlin block, and therefore has no base fee. Since common will report london as active hardfork,
evaluation of uncle header will initialize base fee to 7 per default header constructor rules for
london blocks

It wasn't clear to me why we needed to generate an initial london block with a an uncle that was berlin based but where we ignore the actual hardfork the child block is on and then validate that common doesn't change.

[jochem-brouwer]

Do you remember where you took this test from? I slightly remember adding this test but you are right, it does not make much sense to test it. However I do think there was some situation where we needed this weird logic, and I think it was one of the ethereum/tests tests.

[acolytec3]

It was originally in block.spec.ts, here - https://github.com/ethereumjs/ethereumjs-monorepo/blob/v5-maintenance/packages/block/test/block.spec.ts#L500

[ScottyPoi]

This seems directly related to something I encountered in the miner test during: #1995.

There is a test in miner.spec.ts where we build a FakeChain, and assign hardforks to blockNumbers like this:

    const customChainParams = {
      hardforks: [
        { name: 'chainstart', block: 0 },
        { name: 'berlin', block: 2 },
        { name: 'london', block: 3 },
      ],
    }

Then it starts assembling blocks, using this to change the hardfork:
config.execCommon.setHardforkByBlockNumber(3)

However, in miner.ts we have VM.buildBlock() running with hardforkByBlockNumber set to true, and it always reverts back to chainstart.

I can fix this test by turning hardforkByBlockNumber off and setting the common option to this.config.common. But I think that may be ignoring the underlying issue...

[g11tech]

This seems directly related to something I encountered in the miner test during: #1995.

There is a test in miner.spec.ts where we build a FakeChain, and assign hardforks to blockNumbers like this:

    const customChainParams = {
      hardforks: [
        { name: 'chainstart', block: 0 },
        { name: 'berlin', block: 2 },
        { name: 'london', block: 3 },
      ],
    }

Then it starts assembling blocks, using this to change the hardfork: config.execCommon.setHardforkByBlockNumber(3)

However, in miner.ts we have VM.buildBlock() running with hardforkByBlockNumber set to true, and it always reverts back to chainstart.

I can fix this test by turning hardforkByBlockNumber off and setting the common option to this.config.common. But I think that may be ignoring the underlying issue...

i will try out the #1995 PR locally tomorrow to see whats going on. most likely the mockchain might not be getting build because of some validation failing (is my guess) so the miner is trying to build block from 0

[holgerd77]

@acolytec3 can you have another look and assessment on this issue?

[holgerd77]

No follow-up and likely reduced relevance, will close.