refactor: use upgraded btcd library to handle Taproot address #3039
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📝 WalkthroughWalkthroughThe pull request introduces significant updates to the ZetaChain node, including new features, refactoring, and extensive testing improvements. Key enhancements involve support for stateful precompiled contracts, a common RPC package, and improved Bitcoin chain handling, particularly with Taproot addresses. The changes also include the introduction of a staking precompiled contract, better error handling, and the removal of unused code. Testing has been expanded to cover new functionalities and ensure robustness. Overall, the updates enhance the protocol's capabilities and maintainability. 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
|
ws4charlie
changed the title
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito,
swift1337 and
fbac
as code owners
ws4charlie
added
zetaclient
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
pkg/chains/address.go (2)
Line range hint
57-64: LGTM! Consider using errors.Wrapf for consistent error handling.The panic recovery implementation is robust and well-placed. However, for consistency with other error handling in the codebase, consider using
errors.Wrapf.- err = fmt.Errorf("input address:%s, chainId:%d, err:%s", inputAddress, chainID, err.Error()) + err = errors.Wrapf(err, "panic during address decode for input address:%s, chainId:%d", inputAddress, chainID)
Line range hint
108-116: LGTM! Consider adding version/format documentation.The address type support is well-organized and correctly uses the btcd library's types. Consider adding brief documentation about the supported address formats (e.g., "P2TR: Pay to Taproot, BIP-341").
func IsBtcAddressSupported(addr btcutil.Address) bool { + // Supported Bitcoin address types: + // - P2TR (Pay to Taproot, BIP-341) + // - P2WSH (Pay to Witness Script Hash, BIP-141) + // - P2WPKH (Pay to Witness Public Key Hash, BIP-141) + // - P2SH (Pay to Script Hash, BIP-16) + // - P2PKH (Pay to Public Key Hash, BIP-13) switch addr.(type) {zetaclient/chains/bitcoin/signer/signer_keysign_test.go (2)
Line range hint
57-59: Address empty test methodTestSubmittedTx.The test method appears to be empty. Consider either:
- Implementing the test to cover transaction submission scenarios
- Removing the empty test if it's no longer needed
- Adding a TODO comment if implementation is planned for future
Would you like assistance in implementing test cases for transaction submission scenarios?
Line range hint
42-56: Enhance test coverage with table-driven tests.Consider improving the test suite by:
- Converting to table-driven tests to cover various scenarios
- Adding test cases for error conditions
- Documenting test scenarios and expected outcomes
Here's a suggested refactor:
func (suite *BTCSignTestSuite) TestSign() { tests := []struct { name string prevOut string amount int64 wantErr bool errContains string }{ { name: "successful signing", prevOut: prevOut, amount: 47000, wantErr: false, }, { name: "invalid previous output", prevOut: "invalid", amount: 47000, wantErr: true, errContains: "invalid hash", }, // Add more test cases } for _, tt := range tests { suite.Run(tt.name, func() { tx, txSigHashes, idx, amt, subscript, privKey, compress, err := buildTX() if tt.wantErr { suite.Require().Error(err) suite.Contains(err.Error(), tt.errContains) return } suite.Require().NoError(err) // ... rest of the test }) } }zetaclient/chains/bitcoin/signer/signer_test.go (1)
250-251: LGTM: Comprehensive test coverage for withdrawal scenariosThe script generation for receiver addresses is correctly implemented. The test suite provides good coverage with various scenarios including edge cases.
Consider adding test cases for:
- Taproot addresses (P2TR) to align with the PR's objective
- Multi-signature (P2SH) addresses for completeness
pkg/chains/address_test.go (2)
Line range hint
171-411: Consider extracting test data into a shared test fixture.While the test coverage is comprehensive, the test data for different address types could be organized more efficiently. Consider creating a shared test fixture to reduce duplication and improve maintainability.
Example implementation:
type btcAddressTestCase struct { name string addr string chainId int64 supported bool addrType string // e.g., "P2TR", "P2WSH", etc. txURL string // Optional: for reference } var sharedTestCases = []btcAddressTestCase{ { name: "mainnet taproot address", addr: "bc1p4scddlkkuw9486579autxumxmkvuphm5pz4jvf7f6pdh50p2uzqstawjt9", chainId: BitcoinMainnet.ChainId, supported: true, addrType: "P2TR", txURL: "https://mempool.space/tx/259fc21e63e138136c8f19270a0f7ca10039a66a474f91d23a17896f46e677a7", }, // ... more test cases }This approach would:
- Reduce code duplication
- Make it easier to add new test cases
- Allow for shared test logic across different address types
Line range hint
171-411: Consider adding test data documentation.While the inclusion of mempool.space transaction references is excellent, consider adding:
- A comment block explaining the test data selection criteria
- Documentation about why specific transactions were chosen as test cases
- Any constraints or requirements for adding new test cases
This would help maintainers understand the test data's significance and how to extend it properly.
Example documentation:
// TestAddressData contains real-world Bitcoin transactions that demonstrate // various address types across different networks. Each transaction was selected // to represent typical usage patterns and edge cases. // // Selection criteria: // 1. Transaction must be confirmed and easily verifiable // 2. Address should represent typical usage of the address type // 3. Include examples from different time periods to ensure compatibilityzetaclient/chains/bitcoin/signer/signer.go (2)
Line range hint
282-282: Consider breaking down complex functionsBoth
SignWithdrawTxandTryProcessOutboundare marked with TODOs for simplification. These functions have high cyclomatic complexity and handle multiple responsibilities.Consider breaking these functions into smaller, more focused functions:
- For
SignWithdrawTx:func (s *Signer) SignWithdrawTx(...) (*wire.MsgTx, error) { tx, err := s.prepareWithdrawTx(...) if err != nil { return nil, err } return s.signPreparedTx(tx, ...) }
- For
TryProcessOutbound:func (s *Signer) TryProcessOutbound(...) { if err := s.validateOutbound(...); err != nil { return } tx, err := s.prepareOutboundTx(...) if err != nil { return } s.broadcastAndTrack(tx, ...) }Also applies to: 467-467
Line range hint
513-532: Consider using a dedicated retry packageWhile the current exponential backoff implementation is functional, consider using a dedicated retry package for more robust error handling and configuration.
Consider using a package like
github.com/cenkalti/backoff/v4:operation := func() error { return signer.Broadcast(tx) } backOff := backoff.NewExponentialBackOff() backOff.InitialInterval = broadcastBackoff backOff.MaxElapsedTime = broadcastBackoff * (1 << broadcastRetries) err := backoff.Retry(operation, backOff) if err != nil { logger.Error().Err(err).Msg("failed to broadcast transaction after all retries") return }This would provide:
- More configurable retry behavior
- Better handling of different error types
- Built-in jitter for distributed systems
zetaclient/chains/bitcoin/fee_test.go (2)
264-264: LGTM: Correct type updates for nil address declarationsThe update to use
btcutiltypes for nil address declarations is correct and consistent with the PR's objective to use the upgraded btcd library.Consider adding test cases for edge cases such as:
- Invalid address formats
- Maximum and minimum size addresses for each type
- Addresses with special characters (if applicable)
Line range hint
1-438: Consider improving test organization with test tablesThe test file has good coverage but could benefit from using table-driven tests for better maintainability and readability. This would:
- Reduce code duplication across similar test cases
- Make it easier to add new test cases
- Improve test documentation
Consider refactoring the address type tests into a table-driven format. For example:
func TestGetOutputSizeByAddress(t *testing.T) { tests := []struct { name string addr btcutil.Address wantSize uint64 wantErr bool errContains string }{ { name: "nil P2TR address", addr: (*btcutil.AddressTaproot)(nil), wantSize: 0, }, // Add more test cases here } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { size, err := GetOutputSizeByAddress(tt.addr) if tt.wantErr { require.Error(t, err) require.Contains(t, err.Error(), tt.errContains) } else { require.NoError(t, err) } require.Equal(t, tt.wantSize, size) }) } }changelog.md (1)
34-34: Enhance the changelog entry to better communicate the value proposition.Consider expanding the changelog entry to highlight the benefits of using native APIs:
-* [3039](https://github.com/zeta-chain/node/pull/3039) - use `btcd` native APIs to handle Bitcoin Taproot address +* [3039](https://github.com/zeta-chain/node/pull/3039) - use `btcd` v1.16 native APIs to handle Bitcoin Taproot addresses, removing workaround code and improving maintainability
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- changelog.md (1 hunks)
- e2e/e2etests/test_bitcoin_withdraw_taproot.go (2 hunks)
- pkg/chains/address.go (3 hunks)
- pkg/chains/address_taproot.go (0 hunks)
- pkg/chains/address_taproot_test.go (0 hunks)
- pkg/chains/address_test.go (1 hunks)
- zetaclient/chains/bitcoin/fee.go (1 hunks)
- zetaclient/chains/bitcoin/fee_test.go (3 hunks)
- zetaclient/chains/bitcoin/signer/signer.go (2 hunks)
- zetaclient/chains/bitcoin/signer/signer_keysign_test.go (1 hunks)
- zetaclient/chains/bitcoin/signer/signer_test.go (4 hunks)
- zetaclient/chains/bitcoin/tx_script.go (2 hunks)
💤 Files with no reviewable changes (2)
- pkg/chains/address_taproot.go
- pkg/chains/address_taproot_test.go
🧰 Additional context used
📓 Path-based instructions (9)
e2e/e2etests/test_bitcoin_withdraw_taproot.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/chains/address.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/chains/address_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/fee.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/fee_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/signer/signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/signer/signer_keysign_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/signer/signer_test.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.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (17)
e2e/e2etests/test_bitcoin_withdraw_taproot.go (2)
4-4: LGTM: Import change aligns with library upgrade.The addition of
btcutilimport is consistent with the PR's objective to utilize the upgraded btcd library's native Taproot support.
18-18: LGTM: Type assertion updated for native Taproot support.The change from
chains.AddressTaproottobtcutil.AddressTaprootcorrectly implements the transition to using native btcd library support for Taproot addresses.Let's verify that all Taproot address handling has been consistently updated across the codebase:
✅ Verification successful
Taproot address type assertions have been consistently updated across the codebase
The verification confirms that:
- No instances of the legacy
chains.AddressTaprootremain in the codebase- All Taproot address type assertions consistently use
btcutil.AddressTaproot- The type assertion is used appropriately in key Bitcoin-related components:
- Address validation and conversion (pkg/chains/address.go)
- Transaction script handling (zetaclient/chains/bitcoin/tx_script.go)
- Fee calculation logic (zetaclient/chains/bitcoin/fee.go)
- Test suites (fee_test.go, address_test.go, test_bitcoin_withdraw_taproot.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent usage of btcutil.AddressTaproot across the codebase # and ensure no legacy chains.AddressTaproot remains # Check for any remaining usage of chains.AddressTaproot rg "chains\.AddressTaproot" # Check for consistent usage of btcutil.AddressTaproot rg "btcutil\.AddressTaproot"Length of output: 489
pkg/chains/address.go (2)
73-78: LGTM! Clean and efficient implementation.The address decoding logic has been simplified while maintaining robust error handling. The removal of custom Taproot handling in favor of the btcd library's native support is a good improvement.
Line range hint
1-116: Verify btcd library upgrade impact.The upgrade to btcd v1.16 and the removal of custom Taproot handling code requires verification of consistent usage across the codebase.
✅ Verification successful
Taproot implementation and btcd usage are consistent across the codebase
The verification shows:
- All Taproot references are using the standard
btcutil.AddressTaprootimplementation- The btcd library is properly upgraded to v0.24.2 with consistent dependency versions
- Taproot handling is properly integrated in address validation, tests, and e2e scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining custom Taproot implementations or old btcd usage patterns echo "Checking for potential missed Taproot implementations..." rg -g '*.go' -i 'taproot|taprootaddress' --type go echo "Verifying btcd version in go.mod..." rg -g 'go.mod' 'github.com/btcsuite/btcd'Length of output: 2172
zetaclient/chains/bitcoin/signer/signer_keysign_test.go (1)
98-98: LGTM: Direct usage of txscript package is appropriate.The change to use
txscript.PayToAddrScriptdirectly instead of the wrapper is aligned with the PR's objective to leverage the upgraded btcd library and removes an unnecessary abstraction layer.zetaclient/chains/bitcoin/fee.go (1)
Line range hint
106-124: Consider optimizing nil checks and enhancing error messages.The switch statement is well-structured and the migration to
btcutiltypes is a good improvement. However, there are two potential optimizations:
- The nil checks might be redundant as type assertions would fail for nil values.
- The error message could be more informative about supported address types.
Consider this improved implementation:
func GetOutputSizeByAddress(to btcutil.Address) (uint64, error) { switch addr := to.(type) { case *btcutil.AddressTaproot: - if addr == nil { - return 0, nil - } return bytesPerOutputP2TR, nil case *btcutil.AddressWitnessScriptHash: - if addr == nil { - return 0, nil - } return bytesPerOutputP2WSH, nil case *btcutil.AddressWitnessPubKeyHash: - if addr == nil { - return 0, nil - } return bytesPerOutputP2WPKH, nil case *btcutil.AddressScriptHash: - if addr == nil { - return 0, nil - } return bytesPerOutputP2SH, nil case *btcutil.AddressPubKeyHash: - if addr == nil { - return 0, nil - } return bytesPerOutputP2PKH, nil default: - return 0, fmt.Errorf("cannot get output size for address type %T", to) + return 0, fmt.Errorf("unsupported Bitcoin address type %T. Supported types: P2TR, P2WSH, P2WPKH, P2SH, P2PKH", to) } }Let's verify if the nil checks are truly redundant:
zetaclient/chains/bitcoin/tx_script.go (2)
270-270: LGTM! Type assertion updated for btcutil compatibility.The change to use
*btcutil.AddressTaprootin the type assertion is consistent with the library upgrade and maintains compatibility with other address types in the switch statement.
83-83: LGTM! Verify removal of chains.NewAddressTaproot usage.The change to use
btcutil.NewAddressTaprootaligns with the PR objective of utilizing native Taproot support from the upgraded btcd library.Let's verify the complete removal of the custom implementation:
✅ Verification successful
This will help us confirm that the custom implementation has been completely removed from the chains package.
Custom Taproot implementation successfully removed
The verification confirms that:
- No custom
AddressTaproottype definitions exist in the chains package- All references to
AddressTaprootare now using thebtcutilpackage- No legacy Taproot implementation files remain in the chains directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that chains.NewAddressTaproot is no longer used rg "chains\.NewAddressTaproot" # Verify that btcutil.NewAddressTaproot is used consistently rg "btcutil\.NewAddressTaproot"Length of output: 174
Script:
#!/bin/bash # Let's verify if there are any remaining references to AddressTaproot in the chains package rg "type.*AddressTaproot" ./zetaclient/chains/ rg "AddressTaproot" ./zetaclient/chains/ # Also check if the old implementation files are completely removed fd "taproot" ./zetaclient/chains/Length of output: 454
zetaclient/chains/bitcoin/signer/signer_test.go (3)
78-78: LGTM: Appropriate use of txscript package for script generationThe change to use
txscript.PayToAddrScriptaligns with the upgrade to the latest btcd library and maintains the same functionality for P2PKH address script generation.
242-243: LGTM: Consistent script generation for TSS addressThe change maintains consistency in using
txscript.PayToAddrScriptfor TSS address script generation in withdrawal tests.
Line range hint
150-171: LGTM: Comprehensive test coverage for witness transactionsThe changes correctly utilize
txscript.PayToAddrScriptfor P2WPKH transactions while maintaining thorough test coverage for both automated witness signature generation and manual witness construction.✅ Verification successful
The previous attempt with
go testfailed as expected since we're in a sandbox environment without build tools. Let's analyze the codebase statically to verify the witness transaction test coverage and implementation details.
PayToAddrScript usage is consistent and well-tested across witness and non-witness transactions
The codebase shows comprehensive test coverage for witness transactions with:
- Proper PayToAddrScript usage in both test and implementation files
- Dedicated witness parsing tests in
observer/witness_test.go- Integration with TSS (Threshold Signature Scheme) address generation
- Test coverage for both witness and non-witness transaction scenarios
The changes in the review segment maintain consistency with the established patterns across the codebase and correctly handle P2WPKH transaction construction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify test coverage for witness-related functions go test -v -run "TestP2WPH" ./... -coverprofile=coverage.outLength of output: 130
Script:
#!/bin/bash # Search for witness-related test functions and their implementations ast-grep --pattern 'func TestP2WPH($$$) { $$$ }' # Find all references to PayToAddrScript in test files rg "PayToAddrScript" --type go -A 2 -B 2 # Look for other witness-related test functions rg "Test.*Witness" --type goLength of output: 4555
pkg/chains/address_test.go (2)
182-182: LGTM: Type assertion aligns with btcd library upgrade.The change from custom types to
btcutiltypes is consistent with the PR's objective of utilizing the upgraded btcd library for Bitcoin address handling.
Line range hint
39-108: LGTM: Comprehensive error handling test coverage.The error handling tests are well-structured and cover important scenarios:
- Invalid input validation
- Network mismatches
- Type conversions
- Edge cases (nil, non-string, non-error types)
Also applies to: 412-441
zetaclient/chains/bitcoin/signer/signer.go (1)
161-164: LGTM: Appropriate use of native btcd library APIsThe change to use
txscript.PayToAddrScriptdirectly from the btcd library is a good improvement, aligning with the PR's objective to leverage native APIs for Bitcoin address handling.Also applies to: 170-174
zetaclient/chains/bitcoin/fee_test.go (2)
69-69: LGTM: Correct usage of txscript packageThe update to use
txscript.PayToAddrScriptdirectly is aligned with the PR's objective to utilize the upgraded btcd library.
86-86: LGTM: Consistent package usageThe update maintains consistency with other changes in using the
txscriptpackage directly.changelog.md (1)
Line range hint
1-1000: LGTM! Well-structured changelog.The changelog follows best practices with:
- Clear version headers and semantic versioning
- Consistent categorization of changes
- Proper PR linking
- Chronological ordering of entries
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3039 +/- ##
===========================================
+ Coverage 64.12% 64.13% +0.01%
===========================================
Files 412 411 -1
Lines 28961 28837 -124
===========================================
- Hits 18570 18496 -74
+ Misses 9596 9559 -37
+ Partials 795 782 -13
|
Description
btcdversionv1.16native APIs to handle Bitcoin Taproot address.How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests