block: Set hardforkbyblocknumber true on rlp block constructor #2081
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
acolytec3
changed the title
| // 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) { |
There was a problem hiding this comment.
hardforkByTTD doesn't exist, this should be hardforkByTD.
There was a problem hiding this comment.
🤦 Ack. Will add it to the fixes list.
There was a problem hiding this comment.
Wait, I have another comment... 😋
There was a problem hiding this comment.
I will soon write a more general comment on this solution itself, but if we would take the solution here, wouldn't we need to also check that Common is not provided? otherwise we'll overwrite in many cases (which is likely not intended by the user)? 🤔
|
I am generally not so sure if I like this solution so much, since this will often lead to false results in unknown chain scenarios where no common is provided (this leads on the side line also to the Would the following mechanims an alternative? On a first round I would think this has substantially less side effects. |
|
Hmm, that makes sense and I'd be open to that route. One other edge case to consider (my original issue) is that if we have an RLP serialized block in the pre-london format and someone passes in common with london or later hardfork set, should we throw or just let the user proceed? In this instance, the user is giving a completely packaged up RLP encoded block (which we assume they retrieved from some RPC endpoint or something similar rather than encoding by hand) so if there is incomplete data in the block (i.e. no basefee) and they've set the hardfork as london, should we throw or let the constructor provide the default base fee? I'm mainly thinking about the exact edge case I encountered in Ultralight where I wasn't thinking about setting the hardfork and assumed the |
|
Ok, nice! 🙂
I had a first thinking of generalize here and doing these kind of things in the main header constructor, but then draw back for various reasons (our current base fee default behavior e.g. which is also convenient I guess and also makes sense in various use case scenarios). So, yes, I guess it makes sense to directly throw in these cases in the RLP static constructor, so both for:
Does this make sense and is an accurate task description? |
|
Yep! |
We encountered a scenario where instantiating a block header using
Blockheader.fromRLPSerializedHeaderwas producing incorrect block hashes for block RLPs pulled from mainnet prior to London if theblockOpt.hardforkByBlockNumberis not set to true because the default hardfork is now London and if no baseFee is provided in the header data and london is active, the blockheader constructor adds a default basefee field which causes the block hash to be calculated with the additional field included in the hash. DefaultinghardforkByBlockNumberto true in cases where it is not provided and we are using RLP serialized header data seems like a reasonable default setting to avoid strange scenarios where a user expects the constructor to produce the correct block but it has a basefee field attached that wasn't in the RLP data.