fix: avoid endless tx rescan when the inbound vote message is invalid #3184
Conversation
|
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📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a comprehensive update to the ZetaChain project, featuring new functionalities such as whitelisting SPL tokens on Solana, improved build reproducibility, and enhanced testing frameworks for concurrent operations. Key refactoring efforts include the removal of the HSM signer from the zetaclient and adjustments to the zetaclientd CLI. Several fixes address issues related to message registration, peer discovery, and error handling during omnichain calls. The overall changes focus on enhancing functionality, improving code quality, and addressing various bugs across different components. 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3184 +/- ##
===========================================
- Coverage 62.38% 62.06% -0.33%
===========================================
Files 427 428 +1
Lines 30416 30438 +22
===========================================
- Hits 18974 18890 -84
- Misses 10596 10708 +112
+ Partials 846 840 -6
|
ws4charlie
added
zetaclient
…n-invalid-vote-msg
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
zetaclient/types/event_test.go (3)
14-59: Consider adding more edge cases to strengthen test coverage.While the current test cases cover the basic scenarios, consider adding these additional cases to improve robustness:
- Invalid hex format in memo
- Memo with incorrect length
- Non-hex characters in memo
Example addition:
tests := []struct { // ... existing fields }{ // ... existing test cases + { + name: "should handle invalid hex format", + event: &types.InboundEvent{ + Memo: []byte("invalid_hex"), + }, + errMsg: "invalid hex string", + }, + { + name: "should handle incorrect address length", + event: &types.InboundEvent{ + Memo: []byte("0x1234"), // too short for an address + }, + errMsg: "invalid address length", + }, }
61-67: Consider extracting compliance config setup to a helper function.The compliance config setup could be moved to a helper function to improve test maintainability and reusability.
Example:
+func setupTestComplianceConfig(t *testing.T) { + cfg := config.Config{ + ComplianceConfig: sample.ComplianceConfig(), + } + config.LoadComplianceConfig(cfg) +} + func Test_Processability(t *testing.T) { - cfg := config.Config{ - ComplianceConfig: sample.ComplianceConfig(), - } - config.LoadComplianceConfig(cfg) + setupTestComplianceConfig(t)
69-116: Consider adding edge cases for address validation.The test cases cover the main scenarios but could be enhanced with edge cases:
Example additions:
tests := []struct { // ... existing fields }{ // ... existing test cases + { + name: "should handle empty sender address", + event: &types.InboundEvent{ + Sender: "", + Receiver: sample.EthAddress().Hex(), + }, + expected: types.InboundProcessabilityBadFormat, + }, + { + name: "should handle malformed receiver address", + event: &types.InboundEvent{ + Sender: sample.SolanaAddress(t), + Receiver: "invalid_address", + }, + expected: types.InboundProcessabilityBadFormat, + }, }x/crosschain/types/message_vote_inbound.go (1)
Line range hint
8-13: Consider parameterizing MaxMessageLengthThe hardcoded message length limit is critical for preventing endless tx rescans. Consider:
- Moving this to chain parameters for runtime configurability
- Adding metrics/logging when messages exceed this limit
- Implementing graceful error handling in the state machine as noted in issue Create a param for message max length of
MsgVoteOnObservedInboundTx#862Would you like me to help create a proposal for parameterizing this constant and implementing the suggested improvements?
zetaclient/chains/solana/observer/inbound_test.go (2)
137-168: Test cases appropriately cover error scenarios.The test cases effectively cover memo decoding failures, non-processable events, and message size validation. However, consider enhancing the message validation test case.
Consider adding these test cases for more comprehensive validation:
t.Run("should return nil if message basic validation fails", func(t *testing.T) { - // create event with donation memo - sender := sample.SolanaAddress(t) - maxMsgBytes := crosschaintypes.MaxMessageLength / 2 - event := sample.InboundEvent(chain.ChainId, sender, sender, 1280, []byte(strings.Repeat("a", maxMsgBytes+1))) + testCases := []struct { + name string + memo []byte + }{ + { + name: "message too long", + memo: []byte(strings.Repeat("a", crosschaintypes.MaxMessageLength+1)), + }, + { + name: "empty message", + memo: []byte{}, + }, + { + name: "invalid UTF-8", + memo: []byte{0xFF, 0xFE, 0xFD}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + sender := sample.SolanaAddress(t) + event := sample.InboundEvent(chain.ChainId, sender, sender, 1280, tc.memo) + msg := ob.BuildInboundVoteMsgFromEvent(event) + require.Nil(t, msg) + }) + } })
170-217: Well-structured table-driven tests with room for enhancement.The test implementation is clean and covers the main scenarios. However, consider adding more edge cases for robustness.
Consider adding these additional test cases:
tests := []struct { name string event clienttypes.InboundEvent result bool }{ // ... existing test cases ... + { + name: "should return false on empty sender", + event: clienttypes.InboundEvent{ + Sender: "", + Receiver: sample.EthAddress().Hex(), + }, + result: false, + }, + { + name: "should return false on empty receiver", + event: clienttypes.InboundEvent{ + Sender: sample.SolanaAddress(t), + Receiver: "", + }, + result: false, + }, + { + name: "should return false on zero amount", + event: clienttypes.InboundEvent{ + Sender: sample.SolanaAddress(t), + Receiver: sample.EthAddress().Hex(), + Amount: 0, + }, + result: false, + }, }zetaclient/chains/bitcoin/observer/event_test.go (1)
Line range hint
45-109: Add test case for invalid message lengthBased on the PR objectives to fix endless tx rescan for invalid inbound vote messages, consider adding a test case that verifies the behavior when message length exceeds 10,240 bytes.
tests := []struct { name string event *observer.BTCInboundEvent expected clienttypes.InboundProcessability }{ + { + name: "should return InboundProcessabilityBad for message exceeding max length", + event: &observer.BTCInboundEvent{ + FromAddress: "tb1quhassyrlj43qar0mn0k5sufyp6mazmh2q85lr6ex8ehqfhxpzsksllwrsu", + ToAddress: testutils.TSSAddressBTCAthens3, + MemoBytes: make([]byte, 10241), // Exceeds 10,240 bytes + }, + expected: clienttypes.InboundProcessabilityBad, + }, // ... existing test cases ... }zetaclient/chains/bitcoin/observer/inbound_test.go (3)
Line range hint
466-587: Consider using table-driven tests for error scenariosThe error test cases in
TestGetBtcEventErrorscould be refactored into a table-driven test format to improve maintainability and reduce code duplication.Here's a suggested refactor:
func TestGetBtcEventErrors(t *testing.T) { - // ... setup code ... + tests := []struct { + name string + modifyTx func(*btcjson.TxRawResult) + expectedError string + }{ + { + name: "invalid Vout[0] script", + modifyTx: func(tx *btcjson.TxRawResult) { + tx.Vout[0].ScriptPubKey.Hex = "0014invalid000000000000000000000000000000000" + }, + expectedError: "invalid script", + }, + { + name: "no input found", + modifyTx: func(tx *btcjson.TxRawResult) { + tx.Vin = nil + }, + expectedError: "no input found", + }, + // ... add more test cases ... + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tx := testutils.LoadBTCInboundRawResult(t, TestDataDir, chain.ChainId, txHash, false) + tt.modifyTx(tx) + + rpcClient := mocks.NewBTCRPCClient(t) + event, err := observer.GetBtcEventWithoutWitness( + rpcClient, + *tx, + tssAddress, + blockNumber, + log.Logger, + net, + feeCalculator, + ) + require.ErrorContains(t, err, tt.expectedError) + require.Nil(t, event) + }) + } }
Line range hint
32-40: Consider documenting test data versioningThe test relies on archived Bitcoin transaction data. Consider adding version information and data source documentation to prevent test brittleness.
Add a comment block explaining the test data:
+// TestDataDir contains archived Bitcoin transaction data from mainnet +// Data source: mempool.space API +// Last updated: <date> +// Version: <version> +// Note: Update this comment when adding or modifying test data var TestDataDir string
Line range hint
588-626: Consider adding network failure test casesThe test suite would benefit from additional test cases covering network-related failures such as timeouts and connection issues.
Add test cases for:
- RPC timeout scenarios
- Network disconnection during transaction fetch
- Partial response handling
Example:
t.Run("should handle RPC timeout", func(t *testing.T) { rpcClient := mocks.NewBTCRPCClient(t) rpcClient.On("GetRawTransaction", mock.Anything). After(2*time.Second). // Simulate timeout Return(nil, errors.New("context deadline exceeded")) // ... test implementation })changelog.md (1)
32-32: Verify changelog entry matches PR objectivesThe changelog entry correctly:
- Is placed in the "Fixes" section
- Links to PR fix: avoid endless tx rescan when the inbound vote message is invalid #3184
- Describes the fix for endless tx rescan
- Uses consistent formatting with other entries
However, consider adding more context about the max message length validation that triggers this issue, as mentioned in the PR objectives.
Consider expanding the entry to:
-* [3184](https://github.com/zeta-chain/node/pull/3184) - zetaclient should not retry if inbound vote message validation fails +* [3184](https://github.com/zeta-chain/node/pull/3184) - avoid endless tx rescan by skipping inbound vote messages that exceed max length (10,240 bytes)zetaclient/types/event.go (2)
95-98: Avoid code duplication by reusingDecodeMemomethodIn the
Processabilitymethod, you're parsing the memo to extract the receiver address, similar to the logic inDecodeMemo(). Consider invokingDecodeMemo()withinProcessability()to prevent code duplication and ensure consistency in memo decoding.
93-111: Reorder checks for efficiency and correctnessIn the
Processabilitymethod, the donation check is performed after the restricted address check. Since donation transactions are handled differently and may not require further processing, consider checking for donations first. This can improve efficiency by avoiding unnecessary computations for donation transactions.zetaclient/chains/solana/observer/inbound.go (1)
274-276: Optimize parameter passing by using a pointer receiverThe
CheckEventProcessabilitymethod accepts theeventparameter by value, which may lead to unnecessary copying of theInboundEventstruct. Passing by pointer is more efficient and aligns with Go best practices.Apply this diff to update the function signature and its invocation:
- func (ob *Observer) CheckEventProcessability(event clienttypes.InboundEvent) bool { + func (ob *Observer) CheckEventProcessability(event *clienttypes.InboundEvent) bool { // In BuildInboundVoteMsgFromEvent - if !ob.CheckEventProcessability(*event) { + if !ob.CheckEventProcessability(event) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
changelog.md(1 hunks)x/crosschain/types/message_vote_inbound.go(1 hunks)zetaclient/chains/bitcoin/observer/event.go(4 hunks)zetaclient/chains/bitcoin/observer/event_test.go(6 hunks)zetaclient/chains/bitcoin/observer/inbound.go(1 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(1 hunks)zetaclient/chains/solana/observer/inbound.go(3 hunks)zetaclient/chains/solana/observer/inbound_test.go(3 hunks)zetaclient/compliance/compliance.go(0 hunks)zetaclient/types/event.go(2 hunks)zetaclient/types/event_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- zetaclient/compliance/compliance.go
🧰 Additional context used
📓 Path-based instructions (9)
x/crosschain/types/message_vote_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/types/event.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/types/event_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (2)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:37-38
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:366-371
Timestamp: 2024-11-12T13:20:12.658Z
Learning: The `bitcoin.CalcDepositorFee` function is covered by live tests.
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 369-369: zetaclient/chains/bitcoin/observer/inbound.go#L369
Added line #L369 was not covered by tests
[warning] 375-377: zetaclient/chains/bitcoin/observer/inbound.go#L375-L377
Added lines #L375 - L377 were not covered by tests
zetaclient/chains/solana/observer/inbound.go
[warning] 324-326: zetaclient/chains/solana/observer/inbound.go#L324-L326
Added lines #L324 - L326 were not covered by tests
zetaclient/types/event.go
[warning] 79-80: zetaclient/types/event.go#L79-L80
Added lines #L79 - L80 were not covered by tests
🔇 Additional comments (11)
zetaclient/types/event_test.go (1)
1-12: LGTM! Well-structured test package setup.
The package follows Go testing best practices by using a separate _test package for black-box testing, and imports are well-organized.
x/crosschain/types/message_vote_inbound.go (2)
25-25: LGTM: Comment update improves clarity
The comment update better reflects the function's purpose by removing the redundant "Memo" prefix.
Line range hint 143-159: Verify validation error handling in upstream components
The ValidateBasic() implementation correctly validates message length, but let's ensure the error is properly handled upstream to prevent endless rescans.
Let's verify the error handling implementation:
✅ Verification successful
Based on the search results, I can see that ValidateBasic() is properly handled in various components:
- In CLI commands and RPC endpoints, validation errors are caught and returned early
- In the observer components (Bitcoin, Solana), validation errors are logged and handled gracefully
- No endless retry loops were found in the codebase that could cause rescans
- The validation errors are properly propagated up the call stack
Validation is properly handled upstream, preventing endless rescans
The implementation shows proper error handling patterns across different components:
- Early validation in RPC/CLI layers
- Proper error logging in observer components
- No retry loops that could cause endless rescans
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for handlers of ValidateBasic errors
rg -A 5 "msg\.ValidateBasic\(\)"
# Search for potential retry loops that might cause endless rescans
ast-grep --pattern 'for {
$$$
ValidateBasic()
$$$
}'
Length of output: 34597
zetaclient/chains/solana/observer/inbound_test.go (1)
5-5: LGTM: Import additions and client setup changes are appropriate.
The new imports and modified client setup align well with the test requirements.
Also applies to: 13-13, 113-115
zetaclient/chains/bitcoin/observer/event.go (2)
22-22: LGTM: Clean import addition
The addition of the clienttypes import aligns with the refactoring to use centralized enum definitions.
Line range hint 53-75: LGTM: Clean enum migration
The Processability method has been successfully migrated to use the centralized enum type while maintaining the same logical flow and comprehensive compliance checks.
zetaclient/chains/bitcoin/observer/event_test.go (2)
25-25: LGTM: Clean import addition
The addition of clienttypes import aligns with the type migration and follows Go conventions.
45-45: LGTM: Clean function rename and type migration
The function rename is more concise and the type changes are consistent with the migration to clienttypes package.
Also applies to: 56-56
zetaclient/chains/bitcoin/observer/inbound.go (1)
364-379: LGTM! The validation effectively prevents endless rescan.
The addition of message validation before returning successfully addresses the PR objective. This ensures that invalid messages are caught early, preventing endless rescan attempts.
Enhance error logging for better debugging.
Consider including more context in the error log by adding the validation error details:
- ob.Logger().Inbound.Error().Err(err).Fields(lf).Msg("invalid inbound vote message")
+ ob.Logger().Inbound.Error().Err(err).Fields(lf).
+ Msgf("invalid inbound vote message: %v", err)Add test coverage for validation logic.
The static analysis indicates that the validation error path (lines 375-377) lacks test coverage. Consider adding test cases for:
- Invalid message scenarios that should trigger validation errors
- Edge cases around memo processing
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 369-369: zetaclient/chains/bitcoin/observer/inbound.go#L369
Added line #L369 was not covered by tests
[warning] 375-377: zetaclient/chains/bitcoin/observer/inbound.go#L375-L377
Added lines #L375 - L377 were not covered by tests
zetaclient/chains/bitcoin/observer/inbound_test.go (1)
160-162: LGTM: Mock client setup properly initialized
The addition of OperatorAddress with a sample Bech32 address ensures proper initialization of the mock client.
zetaclient/types/event.go (1)
78-80:
Clarify the 'unreachable code' comment and ensure proper test coverage
The comment // unreachable code suggests that the error handling block may not be executed. However, if memo.DecodeLegacyMemoHex can return an error, this code is reachable and should be handled appropriately. Please verify whether this error case is possible.
- If the error is possible: Remove the 'unreachable code' comment and add unit tests to cover this error handling path.
- If the error is impossible: Remove the error handling code to simplify the function.
To confirm whether memo.DecodeLegacyMemoHex can return an error, you can review its implementation with the following script:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-80: zetaclient/types/event.go#L79-L80
Added lines #L79 - L80 were not covered by tests
…id-btc-solana-inbound-endless-rescan-on-invalid-vote-msg
…n-invalid-vote-msg
…id-btc-solana-inbound-endless-rescan-on-invalid-vote-msg
…n-invalid-vote-msg
…n-invalid-vote-msg
…id-btc-solana-inbound-endless-rescan-on-invalid-vote-msg
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14567965 | Triggered | Generic Password | d723e09 | cmd/zetaclientd/start.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
…id-btc-solana-inbound-endless-rescan-on-invalid-vote-msg
ws4charlie
deleted the
avoid-btc-solana-inbound-endless-rescan-on-invalid-vote-msg
branch
Description
The maximum length of inbound message is defined as
const MaxMessageLength = 10240in thezetacore. If for some reason (it is impossible at the moment), an inbound transaction carries a message longer message, thePostVoteInboundRPC call in observer code below will return an error (due toValidateBasicfailure), which result in endless rescan of the tx.To avoid potential endless tx rescan, zetaclient should
SKIPinvalid inbound vote message.How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes