fix: wrong block hash in subscribe new heads #3047
Conversation
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant updates to the ZetaChain node, encompassing new features, refactoring, tests, fixes, and CI improvements. Key additions include stateful precompiled contracts, a common RPC package, and enhanced Bitcoin chain support. Modifications to the block retrieval methods enhance data completeness by integrating validator account information. The test suite has been expanded to cover new scenarios, particularly around error handling. Overall, the changes aim to improve functionality, error handling, and testing coverage across the system. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (8)
rpc/types/block.go (2)
235-236: Clarify the purpose of Hash field and its relationship with RLP encoding.The comment "overwrite rlpHash" is unclear. Please document:
- Why this field needs to override the RLP hash
- How this field relates to the block's hash calculation
215-237: Enhance struct documentation for better maintainability.The struct lacks comprehensive documentation about:
- Its relationship with Ethereum block headers
- The purpose of optional fields (BaseFee, WithdrawalsHash)
- Field validation requirements
Add a doc comment like:
// Header represents an Ethereum block header compatible with the Ethereum JSON-RPC API. // It includes optional fields for post-London and post-Shanghai upgrades (BaseFee and WithdrawalsHash). // The Hash field represents the computed block hash based on all header fields.rpc/backend/utils.go (1)
323-334: Enhance error handling and input validation.The implementation is solid but could benefit from additional safeguards:
- Input validation for nil parameters
- More specific error messages
Consider this enhancement:
func GetValidatorAccount(header *tmtypes.Header, qc *types.QueryClient) (sdk.AccAddress, error) { + if header == nil { + return nil, fmt.Errorf("header cannot be nil") + } + if qc == nil { + return nil, fmt.Errorf("query client cannot be nil") + } + if len(header.ProposerAddress) == 0 { + return nil, fmt.Errorf("proposer address cannot be empty") + } + res, err := qc.ValidatorAccount( types.ContextWithHeight(header.Height), &evmtypes.QueryValidatorAccountRequest{ ConsAddress: sdk.ConsAddress(header.ProposerAddress).String(), }, ) if err != nil { - return nil, fmt.Errorf("failed to get validator account %w", err) + return nil, fmt.Errorf("failed to get validator account for proposer %s: %w", + sdk.ConsAddress(header.ProposerAddress).String(), err) } return sdk.AccAddressFromBech32(res.AccountAddress) }rpc/namespaces/ethereum/eth/filters/api.go (1)
381-382: Address the TODO comment regarding bloom retrievalThe comment indicates a pending implementation for fetching bloom from events.
Would you like me to help implement the bloom retrieval from events or create a GitHub issue to track this task?
rpc/backend/blocks_test.go (2)
1194-1194: Consider using a descriptive constant for test validator address.The validator address is used across multiple test cases. Consider extracting it to a package-level constant or test suite field for better maintainability.
-validator := sdk.AccAddress(tests.GenerateAddress().Bytes()) +const testValidatorAddress = "0x..." // Add actual address +validator := sdk.AccAddress(common.HexToAddress(testValidatorAddress).Bytes())
1689-1690: Consider grouping related test setup operations.The validator setup and registration are separated by other operations. Consider grouping related setup operations together for better readability.
validator := sdk.AccAddress(tests.GenerateAddress().Bytes()) - suite.backend.indexer = nil client := suite.backend.clientCtx.Client.(*mocks.Client) queryClient := suite.backend.queryClient.QueryClient.(*mocks.EVMQueryClient) + // block contains block real and synthetic tx RegisterBlock(client, 1, []tmtypes.Tx{realTx, tx}) RegisterBlockResultsWithTxResults(client, 1, []*types.ResponseDeliverTx{{}, &txRes}) RegisterBaseFee(queryClient, sdk.NewInt(1)) RegisterValidatorAccount(queryClient, validator)Also applies to: 1698-1699
changelog.md (1)
62-62: Consider expanding the changelog entry with more details.The current entry "wrong block hash in subscribe new heads" could be more descriptive. Based on the PR objectives, this fix addresses multiple aspects including block hash, miner field, and base fee retrieval from events in the WebSocket subscription for new heads.
Consider updating the entry to:
-* [3047](https://github.com/zeta-chain/node/pull/3047) - wrong block hash in subscribe new heads +* [3047](https://github.com/zeta-chain/node/pull/3047) - fix block hash, miner field, and base fee in WebSocket "subscribe new heads" responserpc/backend/blocks.go (1)
Line range hint
525-556: Refactor to useGetValidatorAccountfor consistencyIn the
RPCBlockFromTendermintBlockfunction, the logic to retrieve the validator account is manually implemented. This duplicates code and can lead to inconsistencies. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, refactor this function to utilize theGetValidatorAccounthelper function.Apply this diff to refactor the code:
req := &evmtypes.QueryValidatorAccountRequest{ ConsAddress: sdk.ConsAddress(block.Header.ProposerAddress).String(), } -var validatorAccAddr sdk.AccAddress - -ctx := rpctypes.ContextWithHeight(block.Height) -res, err := b.queryClient.ValidatorAccount(ctx, req) +validatorAccount, err := GetValidatorAccount(&block.Header, b.queryClient) if err != nil { - b.logger.Debug( - "failed to query validator operator address", - "height", block.Height, - "cons-address", req.ConsAddress, - "error", err.Error(), - ) - // use zero address as the validator operator address - validatorAccAddr = sdk.AccAddress(common.Address{}.Bytes()) + b.logger.Error( + "failed to fetch validator account from pruned block. Check node pruning configuration", + "height", block.Height, + "error", err, + ) + validatorAccount = sdk.AccAddress(common.Address{}.Bytes()) -} else { - validatorAccAddr, err = sdk.AccAddressFromBech32(res.AccountAddress) - if err != nil { - return nil, err - } } -validatorAddr := common.BytesToAddress(validatorAccAddr) +validatorAddr := common.BytesToAddress(validatorAccount)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- changelog.md (1 hunks)
- rpc/backend/blocks.go (3 hunks)
- rpc/backend/blocks_test.go (18 hunks)
- rpc/backend/utils.go (2 hunks)
- rpc/namespaces/ethereum/eth/filters/api.go (4 hunks)
- rpc/types/block.go (2 hunks)
- rpc/types/utils.go (3 hunks)
- rpc/websockets.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
rpc/backend/blocks.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/backend/blocks_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/backend/utils.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/namespaces/ethereum/eth/filters/api.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/types/block.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/types/utils.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.rpc/websockets.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (10)
rpc/types/block.go (2)
30-30: LGTM: Import statement is correctly placed and necessary.The
ethtypesimport is required for the newHeadertype and follows Go import conventions.
215-237: Verify header field usage in WebSocket subscriptions.The Header structure aligns with the PR's objective to fix block hash issues in WebSocket subscriptions. Let's verify the implementation:
✅ Verification successful
Let me gather more specific information about the WebSocket subscription implementation.
This will help us verify how the Header type is used in WebSocket subscriptions for new heads, which is crucial for the PR's objective of fixing block hash issues.
Header structure implementation aligns with WebSocket subscription requirements
The verification confirms that the Header structure in
rpc/types/block.gois correctly implemented and properly utilized in the WebSocket subscription system:
- The Header type is used in the subscription system (
rpc/namespaces/ethereum/eth/filters/filter_system.go) with a dedicated channel for header events- The implementation in
rpc/namespaces/ethereum/eth/filters/api.goproperly handles new head subscriptions and notifications- The explicit Hash field in the Header structure ensures block hash information is preserved during WebSocket notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Header type usage in WebSocket subscription handlers # Search for WebSocket subscription handlers using this Header type rg -l "subscribe.*new.*heads" --type go # Search for Header type usage in those files rg "types\.Header" --type goLength of output: 9426
Script:
#!/bin/bash # Check the WebSocket subscription implementation for new heads ast-grep --pattern 'func $_($_, $_, $_) { $$$ subscribe.*new.*heads $$$ }' # Check specific implementation in filters rg -A 10 "NewHeads" rpc/namespaces/ethereum/eth/filters/Length of output: 3579
rpc/backend/utils.go (2)
29-29: LGTM: Import statement is properly placed and utilized.The addition of the
tmtypesimport is correctly placed and necessary for the new functionality.
323-334: Verify WebSocket subscription integration.The function appears to be part of the fix for the "subscribe new heads" WebSocket functionality. Let's verify its integration:
✅ Verification successful
Integration verified for NewHeads subscription and validator account handling
The code analysis reveals proper integration of
GetValidatorAccountwithin the WebSocket subscription system:
- The function is correctly used in the NewHeads subscription handler (rpc/namespaces/ethereum/eth/filters/api.go)
- It's properly integrated into the event processing pipeline for new block headers
- The validator account information is appropriately incorporated into the Ethereum header structure
- Error handling is implemented correctly with appropriate logging
The implementation follows the expected pattern for WebSocket subscriptions and properly handles the block hash fix requirement in the "subscribe new heads" functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of GetValidatorAccount in WebSocket subscription handlers # Check for WebSocket subscription handlers using this function echo "Checking WebSocket subscription handlers..." rg -l "GetValidatorAccount.*subscribe.*heads" --type go # Check for any potential concurrent access patterns echo "Checking for concurrent access patterns..." rg "go\s+.*GetValidatorAccount" --type goLength of output: 293
Script:
#!/bin/bash # Let's broaden our search to understand the usage context of GetValidatorAccount # Check direct usage of GetValidatorAccount echo "Direct usage of GetValidatorAccount:" rg "GetValidatorAccount" -B 2 -A 2 --type go # Check files containing both websocket/subscription and validator references echo -e "\nFiles with websocket/subscription and validator patterns:" rg -l "(?i)(websocket|subscribe|subscription).*validator|validator.*(websocket|subscribe|subscription)" --type go # Check for NewHeads subscription handling echo -e "\nNewHeads subscription handling:" rg "subscribe.*heads|newheads|new.*heads" -B 2 -A 2 --type goLength of output: 2329
Script:
#!/bin/bash # Let's examine the newheads subscription context more specifically # Check the websockets.go file for newheads subscription echo "WebSocket newheads subscription implementation:" rg -B 10 -A 10 "newheads" rpc/websockets.go # Check the filters implementation echo -e "\nFilters implementation for newheads:" rg -B 10 -A 10 "newheads" rpc/namespaces/ethereum/eth/filters/api.go # Look for block header processing echo -e "\nBlock header processing in filters:" rg "Header.*subscribe" rpc/namespaces/ethereum/eth/filters/api.goLength of output: 461
Script:
#!/bin/bash # Let's try a different approach to find the subscription handling # Look for eth_subscribe method implementation echo "eth_subscribe implementation:" rg -B 5 -A 10 "eth_subscribe" --type go # Check NewHeads (case insensitive) references echo -e "\nNewHeads references:" rg -i "newheads" --type go # Look for block header event handling in filters echo -e "\nBlock header event handling in filters:" cat rpc/namespaces/ethereum/eth/filters/api.goLength of output: 23551
rpc/types/utils.go (2)
65-70: LGTM: Function signature change aligns with requirementsThe addition of the
miner sdk.AccAddressparameter is appropriate for providing accurate validator information in the Ethereum header.
79-79: Consider adding miner address validationWhile the implementation correctly uses the provided miner address, consider adding validation to ensure the address is not empty before conversion to prevent potential issues with zero addresses.
Consider adding validation:
func EthHeaderFromTendermint(header tmtypes.Header, bloom ethtypes.Bloom, baseFee *big.Int, miner sdk.AccAddress) *ethtypes.Header { + if len(miner) == 0 { + panic("miner address cannot be empty") + } txHash := ethtypes.EmptyRootHashrpc/namespaces/ethereum/eth/filters/api.go (2)
86-92: LGTM: Clean struct field additionThe
queryClientfield is well-positioned and properly typed for validator account queries.
104-109: 🛠️ Refactor suggestionConsider adding error handling for QueryClient initialization
While the initialization is correct, consider handling potential errors from
types.NewQueryClient()to ensure robust error propagation.- queryClient: types.NewQueryClient(clientCtx), + queryClient: types.NewQueryClient(clientCtx), // Add error handling if NewQueryClient can return an errorLikely invalid or redundant comment.
rpc/backend/blocks_test.go (1)
1295-1300: Verify validator parameter usage in EthHeaderFromTendermint.The validator parameter is now being used in header construction. Ensure that this matches the behavior in the production code.
✅ Verification successful
Let me check the implementation of
EthHeaderFromTendermintto verify how the validator parameter is being used.
Validator parameter usage in EthHeaderFromTendermint is consistent
The implementation shows that the validator parameter (named
minerin the function signature) is correctly used as theCoinbasefield in the Ethereum header construction. This matches the behavior in both test and production code, as evidenced by:
- Implementation in
rpc/types/utils.gosetsCoinbase: common.BytesToAddress(miner)- Consistent usage across multiple call sites:
rpc/websockets.gorpc/namespaces/ethereum/eth/filters/api.gorpc/backend/blocks.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the validator parameter usage in EthHeaderFromTendermint # Test: Search for EthHeaderFromTendermint usage to ensure consistent validator parameter handling rg -A 5 'EthHeaderFromTendermint.*validator'Length of output: 2128
Script:
#!/bin/bash # Search for the implementation of EthHeaderFromTendermint ast-grep --pattern 'func EthHeaderFromTendermint($_, $_, $_, $_) $_ { $$$ }'Length of output: 1635
changelog.md (1)
62-62: LGTM: Correct section placement.The entry is appropriately placed in the "Fixes" section under "Unreleased" as it addresses a bug fix that hasn't been released yet.
Description
NOTE: applied same fix to ethereum filters, need to test
How Has This Been Tested?
locally, checking block hash, miner and base fee fields in subscribe new heads, and getBlockByNumber, this is result after these fixes, these 3 fields are the same in both responses:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
These updates significantly enhance functionality, improve error handling, and ensure a more robust experience for users of the ZetaChain node.