Fix wrong block hash value for the 1st EVM block with PrevRandao
#666
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| if err != nil { | ||
| return nil, err | ||
| } | ||
| block.FixedHash = blockHash |
There was a problem hiding this comment.
Can we move all the special cases to a separated files? Otherwise, we would have to carry this field for future blocks who doesn't need it.
There was a problem hiding this comment.
It may be somewhat misleading, but FixedHash here does not refer to some pre-calculated hash value, that is hard-coded somewhere. It refers to the fact that the Block type, does not have a Hash field, it only has a Hash method, and it is dependent on the fields of the Block type, which can change (added/removed).
There was a problem hiding this comment.
Maybe I missed something, but why a block can be changed? We could use a different type if changes are needed , and when changes are done, create this immutable block, where Hash is correct.
There was a problem hiding this comment.
We already do that: https://github.com/onflow/flow-evm-gateway/blob/main/models/block.go#L108-L131.
One example of changing the fields of the Block type, is when the PrevRandao field was introduced on testnet. We use this type throughout the code-base, that's why we need this patching here.
There was a problem hiding this comment.
Yeah, I think this logic along with the if b.chainID == flowGo.Testnet && slices.Contains(testnetBrokenParentHashBlockHeights, block.Height) {
could all go to the NewBlockFromBytes function.
There was a problem hiding this comment.
Moving the logic to NewBlockFromBytes is rather inconvenient, because that function doesn't have access to the Blocks DB. And we necessarily need access to the Blocks DB, for the following line:
parentBlock, err := b.getBlock(blockHeightKey, uint64Bytes(block.Height-1))
if err != nil {
return nil, err
}We need to recalculate the ParentBlockHash, as it was emitted with a wrong value, for the first block after the HCU. That's why it was added in this specific method, in the first place.
| @@ -253,6 +253,15 @@ func (b *Blocks) getBlock(keyCode byte, key []byte) (*models.Block, error) { | |||
| } | |||
|
|
|||
| if b.chainID == flowGo.Testnet && slices.Contains(testnetBrokenParentHashBlockHeights, block.Height) { | |||
There was a problem hiding this comment.
Do we have the same issue for mainnet?
Do we want to define brokenParentHashBlockHeights, and for testnet and mainnet, we use different heights, so that the logic to handle the heights can be reused.
There was a problem hiding this comment.
This issue is specific to testnet, because of the introduction of the PrevRandao field on EVM.BlockExecuted event, and the Block type. On mainnet, the PrevRandao field was present since launch, so this issue does not apply there.
Description
The EVM block at which
PrevRandaowas introduced, needs some special handling for setting theHash&ParentBlockHashfields, as the addition of thePrevRandaofield caused the hash calculation to change.For contributor use:
masterbranchFiles changedin the Github PR explorer