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

Block: do not auto-activate hardforkByBlockNumber in static BlockHeader constructor #2205

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

holgerd77
Copy link
Member

This PR reverts on #2081 after a discussion with @acolytec3 on Discord, some extracts:

Example posted by @acolytec3, which was not working any more after the changes:

grafik

Reasoning to revert by @holgerd77:


I am coming more and more to the conclusion that this in BlockHeader.fromRLPSerializedHeader() was the wrong choice:

// If an RLP serialized block header is provided and no `hardforkByBlockNumber` opt is provided, default true to
    // avoid scenarios where no `opts.common` or `opts.hardforkByBlockNumber` is provided and a serialized blockheader
    // is provided that predates London result in a default base fee being added to the block
    // (resulting in an erroneous block hash since the default `common` hardfork is London and the blockheader constructor
    // adds a default basefee if EIP-1559 is active and no basefee is provided in the `headerData`)
    if (opts.hardforkByTTD === undefined) {
      opts.hardforkByBlockNumber = opts.hardforkByBlockNumber ?? true
    }

This is again one of these implicit-options behaviors which is not clear from the outside and this also breaks the above example. If someone wants a hardforkByBlockNumber behavior, it is very much possible to set the respective option (that's where the option is for). If someone wants a "HF has precedense" behavior, he/she sets the explicit HF (here: London). There one would except that everything stays in the London realm (this expectation is broken here).

On top I am already pretty sure that there are these use cases out there where people (mainly these dev tool guys like HH/Ganache) have some mainnet RLP serialized block with some artificial block number which should behave in a London (e.g.) context. We had such cases various times. This forcing into the block number logic will likely lead to problems for them.

I would somewhat be inclined to revert (unless I might have forgotten the strong reasoning why we introduced).


@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #2205 (4181609) into master (c6254ce) will decrease coverage by 1.98%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 93.03% <ø> (-0.05%) ⬇️
blockchain 88.82% <ø> (ø)
client 87.00% <ø> (ø)
common 98.09% <ø> (ø)
devp2p 92.25% <ø> (ø)
ethash ∅ <ø> (∅)
evm 79.07% <ø> (?)
rlp ∅ <ø> (∅)
statemanager 88.09% <ø> (ø)
trie 90.15% <ø> (ø)
tx 97.98% <ø> (ø)
util 92.33% <ø> (ø)
vm 85.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 17e3dc0 into master Aug 22, 2022
@holgerd77 holgerd77 deleted the block-remove-implicit-rlp-constructor-hf-setting branch August 22, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants