fix: avoid panic in inscription parsing #3155
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a series of updates to the ZetaChain node, including new features such as the ability to whitelist SPL tokens on Solana and enhancements to build reproducibility. It also integrates SPL deposits and withdrawals and improves testing with new scenarios for concurrent transactions and Bitcoin end-to-end tests. Significant refactoring is present, including the removal of the HSM signer and simplification of configurations. Additionally, various fixes address issues related to emissions, peer discovery, and error handling in Bitcoin inscription parsing. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3155 +/- ##
========================================
Coverage 62.64% 62.64%
========================================
Files 424 424
Lines 30115 30115
========================================
Hits 18866 18866
Misses 10408 10408
Partials 841 841
|
ws4charlie
added
chain:bitcoin
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: 2
🧹 Outside diff range and nitpick comments (8)
changelog.md (1)
28-28: Enhance the fix description for better clarity.The changelog entry correctly documents the fix for PR #3155, but could be more descriptive about the specific panic scenario being addressed.
Consider expanding the description to:
-* [3155](https://github.com/zeta-chain/node/pull/3155) - fix potential panic in the Bitcoin inscription parsing +* [3155](https://github.com/zeta-chain/node/pull/3155) - fix potential panic in Bitcoin inscription parsing by adding null pointer checks in checkInscriptionEnvelopezetaclient/chains/bitcoin/tx_script_test.go (7)
Line range hint
625-631: Userequire.NoErrorinstead ofrequire.Nilfor error assertionsIn Go tests, it's idiomatic to use
require.NoError(t, err)when checking for the absence of an error. This provides clearer output if the test fails.Apply this diff to improve the error assertion:
- require.Nil(t, err) + require.NoError(t, err)
Line range hint
625-638: Externalize large hexadecimal strings for better readabilityEmbedding large hexadecimal strings directly in the test code reduces readability and can make maintenance more challenging. Consider moving these strings to external test data files or constants.
Line range hint
639-646: Userequire.NoErrorinstead ofrequire.Nilfor error assertionsConsistent with Go testing best practices, replace
require.Nil(t, err)withrequire.NoError(t, err)for clearer test output.Apply this diff:
- require.Nil(t, err) + require.NoError(t, err)
Line range hint
639-650: Externalize large hexadecimal strings for better readabilitySimilar to the previous suggestion, consider moving the hexadecimal strings to external files or constants to enhance readability and maintainability.
672-675: Correct the opcode value in the comment for accuracyThe comment incorrectly states that
OP_CODESEPARATORhas the opcode0xac, which is actually the opcode forOP_CHECKSIG. The correct opcode forOP_CODESEPARATORis0xab. Updating this will improve clarity.Apply this diff to correct the comment:
- // require OP_CHECKSIG (0xac) but OP_CODESEPARATOR (0xac) is found + // require OP_CHECKSIG (0xac) but OP_CODESEPARATOR (0xab) is found
683-691: Userequire.NoErrorinstead ofrequire.Nilfor error assertionsReplace
require.Nil(t, err)withrequire.NoError(t, err)for consistency and clearer test failure messages.Apply this diff:
- require.Nil(t, err) + require.NoError(t, err)
693-701: Userequire.NoErrorinstead ofrequire.Nilfor error assertionsMaintain consistency in error checking by using
require.NoError(t, err).Apply this diff:
- require.Nil(t, err) + require.NoError(t, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
changelog.md(1 hunks)zetaclient/chains/bitcoin/tx_script.go(1 hunks)zetaclient/chains/bitcoin/tx_script_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
zetaclient/chains/bitcoin/tx_script.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/tx_script_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (1)
zetaclient/chains/bitcoin/tx_script.go (1)
Line range hint 339-348: Consider adding validation for public key data
The function checks for the correct opcodes but doesn't validate the public key data itself.
Consider adding validation for the public key data:
func checkInscriptionEnvelope(t *txscript.ScriptTokenizer) error {
if !t.Next() || t.Opcode() != txscript.OP_DATA_32 {
if err := t.Err(); err != nil {
return fmt.Errorf("failed to parse public key: %w", err)
}
return fmt.Errorf("expected OP_DATA_32 for public key, got %v", t.Opcode())
}
+
+ // Validate public key data
+ pubKey := t.Data()
+ if len(pubKey) != 32 {
+ return fmt.Errorf("invalid public key length: got %d, want 32", len(pubKey))
+ }
if !t.Next() || t.Opcode() != txscript.OP_CHECKSIG {
if err := t.Err(); err != nil {
return fmt.Errorf("failed to parse OP_CHECKSIG: %w", err)
}
return fmt.Errorf("expected OP_CHECKSIG, got %v", t.Opcode())
}
return nil
}| return fmt.Errorf("public key not found") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error context and check for tokenizer errors
While the error message has been simplified, we should still check for tokenizer errors and provide more context for debugging purposes.
- return fmt.Errorf("public key not found")
+ if err := t.Err(); err != nil {
+ return fmt.Errorf("failed to parse public key: %w", err)
+ }
+ return fmt.Errorf("expected OP_DATA_32 for public key, got %v", t.Opcode())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return fmt.Errorf("public key not found") | |
| } | |
| if err := t.Err(); err != nil { | |
| return fmt.Errorf("failed to parse public key: %w", err) | |
| } | |
| return fmt.Errorf("expected OP_DATA_32 for public key, got %v", t.Opcode()) | |
| } |
| return fmt.Errorf("OP_CHECKSIG not found") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error context and check for tokenizer errors
Similar to the previous comment, enhance error handling for the OP_CHECKSIG case.
- return fmt.Errorf("OP_CHECKSIG not found")
+ if err := t.Err(); err != nil {
+ return fmt.Errorf("failed to parse OP_CHECKSIG: %w", err)
+ }
+ return fmt.Errorf("expected OP_CHECKSIG, got %v", t.Opcode())Committable suggestion skipped: line range outside the PR's diff.
Description
This PR is to avoid potential panic in method
checkInscriptionEnvelope.How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests