Fix Genesis block hash calculation for Testnet network
#566
Conversation
WalkthroughThe changes implement a mechanism for handling the Genesis block specifically for the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
models/block_test.go (1)
Line range hint
1-27: Ensure the fix is specific to Testnet Genesis blocksWhile the new test covers the Testnet Genesis block hash calculation, it's important to verify that this fix doesn't unintentionally affect other network types or general block hashing. Consider adding the following:
- A test for Genesis blocks of other network types (e.g., Mainnet, Emulator) to ensure their hash calculations remain unchanged.
- A test to verify that non-Genesis blocks still include the
PrevRandaofield in their hash calculation.Here are suggested additional tests:
func Test_GenesisBlockHashForOtherNetworks(t *testing.T) { networks := []flowGo.Chain{flowGo.Mainnet, flowGo.Emulator} for _, network := range networks { t.Run(string(network), func(t *testing.T) { block := GenesisBlock(network) blockHash, err := block.Hash() require.NoError(t, err) // Add assertions for expected hash values for each network // assert.Equal(t, expectedHashForNetwork, blockHash.String()) }) } } func Test_NonGenesisBlockIncludesPrevRandao(t *testing.T) { block := &Block{ Block: &types.Block{ Height: 1, PrevRandao: gethCommon.HexToHash("0x1234"), }, } hash1, err := block.Hash() require.NoError(t, err) block.PrevRandao = gethCommon.HexToHash("0x5678") hash2, err := block.Hash() require.NoError(t, err) assert.NotEqual(t, hash1, hash2, "Changing PrevRandao should result in a different hash for non-Genesis blocks") }These additional tests will help ensure that the fix is specific to Testnet Genesis blocks and doesn't affect other scenarios.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- models/block.go (1 hunks)
- models/block_test.go (1 hunks)
Additional comments not posted (2)
models/block_test.go (1)
Line range hint
1-27: Overall assessment of the changesThe new test function
Test_GenesisBlockHashForTestnetaddresses the PR objective of fixing the Genesis block hash calculation for the Testnet network. However, to ensure a comprehensive and robust solution, consider implementing the following improvements:
- Enhance the existing test with better documentation and explicit verification of the
PrevRandaofield exclusion.- Add tests for Genesis blocks of other network types to ensure the fix is specific to Testnet.
- Include a test for non-Genesis blocks to verify that
PrevRandaois still included in their hash calculation.These additions will provide better test coverage, clearer documentation, and ensure that the fix doesn't have unintended consequences on other parts of the system.
The current changes are a step in the right direction, but implementing the suggested improvements will greatly enhance the quality and reliability of this fix.
models/block.go (1)
22-47: Testnet Genesis block hash calculation logic is correctly implementedThe conditional logic introduced for the Testnet chain correctly recalculates the Genesis block hash without the
PrevRandaofield. This aligns with the PR objectives and ensures that the hash matches the expected value on Testnet.
m-Peter
force-pushed
the
fix-genesis-block-hash-for-testnet
branch
from
2a4ca9c to
ce27a36
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
models/block.go (2)
23-34: LGTM: New variables for Testnet-specific handling.The introduction of
testnetEVMBlockExecutedEventIDandtestnetParentHashMappingis a good approach to handle Testnet-specific scenarios.Consider adding a comment explaining why the specific block height (1385491) requires a manual mapping. This would improve code maintainability and help future developers understand the context.
Line range hint
1-238: Overall LGTM: Comprehensive changes for Testnet block hash calculation.The modifications effectively address the PR objectives by implementing Testnet-specific handling for the Genesis block and subsequent block hash calculations. The changes are consistent and well-integrated into the existing codebase.
Consider adding a brief comment at the beginning of the file explaining the purpose of the Testnet-specific changes. This would provide helpful context for anyone reviewing or maintaining this file in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- models/block.go (3 hunks)
Additional comments not posted (3)
models/block.go (3)
8-11: LGTM: New imports added.The new import statements are appropriate for the changes made in this file.
37-62: LGTM: Genesis block handling for Testnet.The changes to the
GenesisBlockfunction correctly implement the exclusion of thePrevRandaofield for the Testnet Genesis block, aligning with the PR objectives.As mentioned in a previous review, please handle the error returned by
blockV0.Hash(). This is important to ensure that any issues during hash calculation are caught and managed properly.
124-131: LGTM: Testnet-specific parent block hash handling.The added conditional logic correctly updates the
ParentBlockHashfor specific Testnet block heights using thetestnetParentHashMapping. This implementation aligns well with the PR objectives.
Nope, this is just a problem on |
models/block.go
Outdated
| @@ -80,6 +121,14 @@ func decodeBlockEvent(event cadence.Event) (*Block, error) { | |||
| return nil, fmt.Errorf("failed to cadence decode block [%s]: %w", event.String(), err) | |||
| } | |||
|
|
|||
| if event.EventType.ID() == testnetEVMBlockExecutedEventID { | |||
There was a problem hiding this comment.
I would move this to block decoding from database on the model, not on the decoding of events
m-Peter
force-pushed
the
fix-genesis-block-hash-for-testnet
branch
from
ce27a36 to
ec635fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
storage/pebble/blocks.go (1)
284-298: Consider logging when adjusting theParentBlockHashAdding a log statement inside the conditional block can aid in debugging and monitoring. This will provide visibility when the
ParentBlockHashis adjusted, which could be valuable for tracking the fix's application over time.Apply this diff to add a debug log:
if b.chainID == flowGo.Testnet && slices.Contains(testnetBrokenParentHashHeights, block.Height) { + log.Printf("Adjusting ParentBlockHash for block height: %d", block.Height) data, err := b.store.get(blockHeightKey, uint64Bytes(block.Height-1)) if err != nil { return nil, fmt.Errorf("failed to get block: %w", err) } parentBlock, err := models.NewBlockFromBytes(data) if err != nil { return nil, err } parentBlockHash, err := parentBlock.Hash() if err != nil { return nil, err } block.ParentBlockHash = parentBlockHash }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- storage/pebble/blocks.go (3 hunks)
🔇 Additional comments (1)
storage/pebble/blocks.go (1)
23-26: Definition oftestnetBrokenParentHashHeightsis appropriateThe addition of
testnetBrokenParentHashHeightscorrectly specifies the block heights with known parent hash issues onTestnet. This will help in addressing the inconsistencies observed in issue #534.
m-Peter
force-pushed
the
fix-genesis-block-hash-for-testnet
branch
from
97bf80c to
7ba0544
Compare
m-Peter
force-pushed
the
fix-genesis-block-hash-for-testnet
branch
from
7ba0544 to
f14ad0c
Compare
Closes: #534
Description
The hash calculation for the
Genesisblock onTestnetshould be done without thePrevRandaofield, as it was introduced later, and it produces a different hash.The
ParentHashfor the first EVM block that contains thePrevRandaofield, is pointing to a non-existent block. Relevant events:A.8c5303eaa26202d6.EVM.BlockExecuted( height: 1385490, hash: "0xfc9a0204b12bc4875ed880300d07f27de4f9857be67d13cf72f53b09e8942ced", timestamp: 1724362036, totalSupply: 37735203666010000000000, totalGasUsed: 0, parentHash: "0x95b9d3a30ce797062ab5162d6dc4c60d887ff66eeec49640931c2d7ac014974d", receiptRoot: [86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33], transactionHashRoot: [86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33] ) A.8c5303eaa26202d6.EVM.BlockExecuted( height: 1385491, hash: "0x829f0aeea56301ca085a9688f0ab238f2738e553a09e00963170d92c08957daa", timestamp: 1724362037, totalSupply: 37735203666010000000000, totalGasUsed: 0, parentHash: "0x0e742970c53cc2924d55830a588609ca1db460cce41850df7af119639fe42d91", receiptRoot: [86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33], transactionHashRoot: [86, 232, 31, 23, 27, 204, 85, 166, 255, 131, 69, 230, 146, 192, 248, 110, 91, 72, 224, 27, 153, 108, 173, 192, 1, 98, 47, 181, 227, 99, 180, 33], prevrandao: [84, 243, 47, 99, 190, 212, 43, 169, 197, 65, 166, 231, 112, 226, 218, 121, 153, 240, 149, 165, 22, 98, 37, 215, 150, 128, 215, 244, 192, 84, 175, 161] )Note: After deployment, a re-index is needed for this code to take effect.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Tests