chore: upgrade btcd (and tss deps) #2750
Conversation
gartnera
added
no-changelog
|
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 WalkthroughWalkthroughThe recent code changes primarily focus on upgrading the Changes
Assessment against linked issues
This assessment confirms that the primary objective of upgrading the 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
|
gartnera
force-pushed
the
upgrade-btc-deps
branch
from
4556920 to
a5324c5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2750 +/- ##
==========================================
Coverage ? 67.12%
==========================================
Files ? 378
Lines ? 21121
Branches ? 0
==========================================
Hits ? 14178
Misses ? 6276
Partials ? 667
|
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
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: 1
Outside diff range, codebase verification and nitpick comments (2)
zetaclient/chains/bitcoin/signer/signer_test.go (1)
122-122: UpdateNewEngineUsages in TestsThe
NewEnginefunction usages insigner_test.godo not include the new parametertxscript.NewMultiPrevOutFetcher(nil). Please update these instances to ensure the changes are correctly integrated and tested.
zetaclient/chains/bitcoin/signer/signer_test.go: Update all usages ofNewEngineto include the new parameter.Analysis chain
Verify
NewEngineparameter updates.The
NewEnginefunction now includestxscript.NewMultiPrevOutFetcher(nil). Ensure this change is correctly integrated and tested.Run the following script to verify the usage of
NewEngineacross the codebase:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `NewEngine` to ensure correct parameter integration. # Test: Search for the function usage. Expect: Only occurrences with the new parameter. rg --type go $'NewEngine'Length of output: 386
zetaclient/tss/tss_signer.go (1)
17-18: Re-evaluatebtcutilRemoval and Address Integration ConcernsThe
btcutilpackage is still widely used across the codebase, indicating that its removal fromzetaclient/tss/tss_signer.gois likely incorrect and could lead to functionality issues. Additionally, while thekeygenpackage integration appears extensive, ensure that all relevant TODOs and FIXMEs are addressed to solidify its implementation.
btcutilUsage: Review the necessity ofbtcutilinzetaclient/tss/tss_signer.goand other files to prevent potential issues.keygenIntegration: Address any pending TODOs or FIXMEs related tokeygento ensure complete and correct integration.Please verify these aspects to maintain code integrity and functionality.
Analysis chain
Import changes approved, verify new usage.
The addition of the
keygenpackage and removal ofbtcutilare appropriate. Ensure that the newkeygenusage aligns with the intended functionality and that the removal ofbtcutildoes not affect existing functionality.Verify the usage of
keygento ensure it is correctly integrated.Run the following script to verify the usage of
keygen:
Verify the impact of
btcutilremoval and ensurekeygenintegration is complete.The
keygenpackage has been integrated across multiple files for TSS key generation. Ensure that its usage aligns with the intended functionality. Additionally, verify that the removal ofbtcutildoes not affect other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `keygen` in the codebase. # Test: Search for the usage of `keygen`. Expect: Correct integration and usage. rg --type go -A 5 $'keygen'Length of output: 86398
Script:
#!/bin/bash # Check for any remaining usage of `btcutil` in the codebase to ensure its removal does not affect functionality. rg --type go 'btcutil'Length of output: 17314
Script:
#!/bin/bash # Verify the integration of `keygen` by checking for any related TODOs or FIXMEs that might indicate incomplete implementation. rg --type go 'TODO|FIXME' -A 3Length of output: 58531
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (43)
- cmd/zetaclientd/gen_pre_params.go (1 hunks)
- cmd/zetaclientd/keygen_tss.go (1 hunks)
- cmd/zetaclientd/main.go (1 hunks)
- e2e/e2etests/helpers.go (1 hunks)
- e2e/e2etests/test_bitcoin_withdraw_legacy.go (1 hunks)
- e2e/e2etests/test_bitcoin_withdraw_p2sh.go (1 hunks)
- e2e/e2etests/test_bitcoin_withdraw_p2wsh.go (1 hunks)
- e2e/e2etests/test_bitcoin_withdraw_segwit.go (1 hunks)
- e2e/e2etests/test_migrate_tss.go (1 hunks)
- e2e/e2etests/test_stress_btc_withdraw.go (1 hunks)
- e2e/runner/balances.go (1 hunks)
- e2e/runner/bitcoin.go (1 hunks)
- e2e/runner/runner.go (1 hunks)
- e2e/runner/setup_bitcoin.go (3 hunks)
- e2e/runner/setup_zeta.go (1 hunks)
- e2e/utils/utils.go (1 hunks)
- go.mod (7 hunks)
- pkg/chains/address.go (1 hunks)
- pkg/chains/address_taproot.go (1 hunks)
- pkg/chains/address_test.go (2 hunks)
- pkg/crypto/tss.go (1 hunks)
- pkg/crypto/tss_test.go (1 hunks)
- pkg/proofs/bitcoin/proof.go (1 hunks)
- pkg/proofs/bitcoin/proof_test.go (1 hunks)
- pkg/proofs/headers.go (1 hunks)
- pkg/proofs/proof.go (1 hunks)
- pkg/proofs/proof_test.go (1 hunks)
- x/crosschain/types/tx_body_verification.go (2 hunks)
- x/lightclient/keeper/grpc_query_prove.go (1 hunks)
- zetaclient/chains/bitcoin/fee.go (1 hunks)
- zetaclient/chains/bitcoin/fee_test.go (3 hunks)
- zetaclient/chains/bitcoin/observer/inbound_test.go (1 hunks)
- zetaclient/chains/bitcoin/observer/observer.go (1 hunks)
- zetaclient/chains/bitcoin/signer/signer.go (3 hunks)
- zetaclient/chains/bitcoin/signer/signer_keysign_test.go (3 hunks)
- zetaclient/chains/bitcoin/signer/signer_test.go (7 hunks)
- zetaclient/chains/bitcoin/tx_script.go (1 hunks)
- zetaclient/chains/bitcoin/utils.go (1 hunks)
- zetaclient/chains/interfaces/interfaces.go (1 hunks)
- zetaclient/testutils/mocks/btc_rpc.go (1 hunks)
- zetaclient/testutils/mocks/tss_signer.go (1 hunks)
- zetaclient/tss/tss_signer.go (2 hunks)
- zetaclient/zetacore/broadcast.go (2 hunks)
Files skipped from review due to trivial changes (26)
- e2e/e2etests/helpers.go
- e2e/e2etests/test_bitcoin_withdraw_p2wsh.go
- e2e/e2etests/test_bitcoin_withdraw_segwit.go
- e2e/e2etests/test_migrate_tss.go
- e2e/e2etests/test_stress_btc_withdraw.go
- e2e/runner/bitcoin.go
- e2e/runner/runner.go
- e2e/utils/utils.go
- pkg/chains/address.go
- pkg/chains/address_taproot.go
- pkg/crypto/tss.go
- pkg/crypto/tss_test.go
- pkg/proofs/bitcoin/proof.go
- pkg/proofs/bitcoin/proof_test.go
- pkg/proofs/headers.go
- pkg/proofs/proof.go
- pkg/proofs/proof_test.go
- x/lightclient/keeper/grpc_query_prove.go
- zetaclient/chains/bitcoin/fee.go
- zetaclient/chains/bitcoin/observer/inbound_test.go
- zetaclient/chains/bitcoin/observer/observer.go
- zetaclient/chains/bitcoin/tx_script.go
- zetaclient/chains/bitcoin/utils.go
- zetaclient/chains/interfaces/interfaces.go
- zetaclient/testutils/mocks/btc_rpc.go
- zetaclient/testutils/mocks/tss_signer.go
Additional context used
Path-based instructions (16)
e2e/e2etests/test_bitcoin_withdraw_legacy.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_bitcoin_withdraw_p2sh.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/gen_pre_params.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/main.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/setup_bitcoin.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/zetacore/broadcast.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/tx_body_verification.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/balances.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/setup_zeta.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetaclientd/keygen_tss.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.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/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/fee_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/tss/tss_signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (1)
zetaclient/chains/bitcoin/signer/signer_keysign_test.go (2)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:184-247 Timestamp: 2024-07-05T00:02:31.446Z Learning: The `CreateSignerObserverBTC` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-07-04T23:46:38.428Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
GitHub Check: codecov/patch
x/crosschain/types/tx_body_verification.go
[warning] 139-139: x/crosschain/types/tx_body_verification.go#L139
Added line #L139 was not covered by testszetaclient/chains/bitcoin/signer/signer.go
[warning] 273-273: zetaclient/chains/bitcoin/signer/signer.go#L273
Added line #L273 was not covered by tests
[warning] 297-301: zetaclient/chains/bitcoin/signer/signer.go#L297-L301
Added lines #L297 - L301 were not covered by tests
Additional comments not posted (28)
e2e/e2etests/test_bitcoin_withdraw_legacy.go (1)
4-4: Import Path Update: Verify Compatibility with New Package StructureThe import path for
btcutilhas been updated to"github.com/btcsuite/btcd/btcutil". Ensure that the new package structure is compatible with the existing code and that any changes in functionality are accounted for in the tests.e2e/e2etests/test_bitcoin_withdraw_p2sh.go (1)
4-4: Import Path Update: Verify Compatibility with New Package StructureThe import path for
btcutilhas been updated to"github.com/btcsuite/btcd/btcutil". Ensure that the new package structure is compatible with the existing code and that any changes in functionality are accounted for in the tests.cmd/zetaclientd/gen_pre_params.go (1)
9-9: Import Path Update: Verify Key Generation FunctionalityThe import path for
keygenhas been updated to"github.com/bnb-chain/tss-lib/ecdsa/keygen". Ensure that the new repository provides the same or improved functionality for key generation and that any changes in the API are reflected in the code.cmd/zetaclientd/main.go (1)
8-8: Import Path Update: Verify Key Generation FunctionalityThe import path for
ecdsakeygenhas been updated to"github.com/bnb-chain/tss-lib/ecdsa/keygen". Ensure that the new repository provides the same or improved functionality for key generation and that any changes in the API are reflected in the code.e2e/runner/setup_bitcoin.go (2)
7-7: Import Path Update: Verify Compatibility with NewbtcecVersionThe import path for
btcechas been updated to"github.com/btcsuite/btcd/btcec/v2". Ensure that the new version is compatible with the existing code and that any changes in functionality are accounted for.
64-64: API Usage Update: VerifyPrivKeyFromBytesChangesThe
PrivKeyFromBytesfunction calls have been modified to remove the explicitbtcec.S256()parameter. Verify that this change aligns with the updated API and does not affect the expected behavior of the code.Also applies to: 87-87
zetaclient/chains/bitcoin/signer/signer_keysign_test.go (3)
10-11: Update import paths forbtceclibrary.The import paths have been updated to use version 2 of the
btceclibrary. Ensure that all dependencies in the project are compatible with this version.
104-104: Ensure correct usage ofNewCannedPrevOutputFetcher.The
NewCannedPrevOutputFetcheris used here with empty data and zero value. Verify that this is intentional and that it aligns with the expected transaction context.
151-155: Improve signature handling withModNScalar.The use of
ModNScalarfor R and S values in the signature construction is a robust approach. Ensure that this change is tested thoroughly to confirm it enhances security and correctness.x/crosschain/types/tx_body_verification.go (2)
7-7: Update import path forbtceclibrary.The import path has been updated to use version 2 of the
btceclibrary. Ensure that this change is compatible with other parts of the codebase.
139-139: SimplifyParsePubKeyusage.The
ParsePubKeyfunction now omits the second argument,btcec.S256(). Verify that this change leverages the default behavior correctly and does not introduce any issues.Tools
GitHub Check: codecov/patch
[warning] 139-139: x/crosschain/types/tx_body_verification.go#L139
Added line #L139 was not covered by testse2e/runner/balances.go (1)
8-8: Update import path forbtcutil.The import path has been changed to
"github.com/btcsuite/btcd/btcutil". Verify that this change does not affect any existing functionalities and aligns with the project's dependency management strategy.e2e/runner/setup_zeta.go (1)
7-7: Update import path forbtcutil.The import path has been updated to
"github.com/btcsuite/btcd/btcutil". Confirm that this update is consistent with the project's overall upgrade strategy and does not disrupt any existing code.cmd/zetaclientd/keygen_tss.go (1)
136-136: Ensure compatibility with ECDSA changes.The addition of
tsscommon.ECDSAin thekeygen.NewRequestcall enhances the key generation process. Verify that all components interacting with this function are compatible with ECDSA.Run the following script to verify the usage of
keygen.NewRequestacross the codebase:zetaclient/chains/bitcoin/signer/signer_test.go (4)
67-67: Update test forPrivKeyFromBytesusage.The
PrivKeyFromBytesfunction now requires a single argument. Ensure that this change is reflected in all relevant tests.
171-171: Ensure robust handling ofNewTxSigHashes.The
NewTxSigHashesfunction now usestxscript.NewCannedPrevOutputFetcher. Verify that the test logic correctly handles this change.
211-211: Confirm signature creation logic.The signature creation now uses
btcecdsa.Sign. Ensure that this logic is correctly tested and aligns with the updated API.
219-219: VerifyNewEngineparameter updates in P2WPH test.Ensure the
NewEnginefunction's new parameter is correctly integrated in the P2WPH test.pkg/chains/address_test.go (2)
7-7: Update import path forbtcutil.The import path has been updated to
github.com/btcsuite/btcd/btcutil. Ensure all usages ofbtcutilare consistent with this change.
48-48: Refine error message for invalid address.The error message for decoding an invalid Bitcoin address has been improved for clarity. Ensure this change is reflected in all relevant test cases.
zetaclient/chains/bitcoin/signer/signer.go (2)
12-13: Update imports forbtcecandbtcecdsa.The import paths have been updated to reflect the new version of the
btcecpackage. Ensure all related code is compatible with these changes.
297-301: Ensure correctness of signature creation logic.The signature creation now uses
btcecdsa.NewSignature. Verify that this logic is correctly implemented and aligns with the updated API.Tools
GitHub Check: codecov/patch
[warning] 297-301: zetaclient/chains/bitcoin/signer/signer.go#L297-L301
Added lines #L297 - L301 were not covered by testsgo.mod (3)
35-35: Upgradebtcsuite/btcdto v0.24.2.The
btcsuite/btcdpackage has been upgraded. Ensure all code interacting with this package is compatible with the new version.
365-367: Replacethorchain/tsswithzeta-chain/go-tss.The
thorchain/tssdependency has been replaced. Ensure that all references to this package have been updated.
59-59: Addbnb-chain/tss-libdependency.The new dependency
bnb-chain/tss-libhas been added. Verify its integration and usage within the codebase.Run the following script to verify the usage of
bnb-chain/tss-libacross the codebase:Verification successful
Integration of
bnb-chain/tss-libVerifiedThe
bnb-chain/tss-liblibrary is properly integrated and actively used in the codebase for key generation and pre-parameter handling. The usage is confirmed in the following files:
zetaclient/tss/tss_signer.gocmd/zetaclientd/gen_pre_params.gocmd/zetaclientd/main.goThe integration appears correct and functional.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `bnb-chain/tss-lib` to ensure correct integration. # Test: Search for the package usage. Expect: Proper integration and usage. rg --type go $'bnb-chain/tss-lib'Length of output: 266
Script:
#!/bin/bash # Check the usage of `bnb-chain/tss-lib/ecdsa/keygen` in the identified files. # Extract lines around the import in zetaclient/tss/tss_signer.go rg --type go --context 5 'github.com/bnb-chain/tss-lib/ecdsa/keygen' zetaclient/tss/tss_signer.go # Extract lines around the import in cmd/zetaclientd/gen_pre_params.go rg --type go --context 5 'github.com/bnb-chain/tss-lib/ecdsa/keygen' cmd/zetaclientd/gen_pre_params.go # Extract lines around the import in cmd/zetaclientd/main.go rg --type go --context 5 'github.com/bnb-chain/tss-lib/ecdsa/keygen' cmd/zetaclientd/main.goLength of output: 1031
Script:
#!/bin/bash # Search for usages of the `keygen` package in the identified files to verify integration. # Look for function calls or operations involving `keygen` in zetaclient/tss/tss_signer.go rg --type go 'keygen' zetaclient/tss/tss_signer.go # Look for function calls or operations involving `keygen` in cmd/zetaclientd/gen_pre_params.go rg --type go 'keygen' cmd/zetaclientd/gen_pre_params.go # Look for function calls or operations involving `keygen` in cmd/zetaclientd/main.go rg --type go 'keygen' cmd/zetaclientd/main.goLength of output: 641
zetaclient/chains/bitcoin/fee_test.go (3)
8-9: Import changes approved.The update to use version 2 of the
btcecpackage and the inclusion of theecdsasubpackage are appropriate and align with the library's new version.
63-63: Key generation simplification approved.The removal of the explicit curve parameter in the
NewPrivateKeycall simplifies the process and is appropriate if the default curve is intended.
158-163: Signature process enhancements approved.The use of
txscript.NewTxSigHasheswithNewCannedPrevOutputFetcherandbtcecdsa.Signenhances the signature hashing and signing processes, likely improving security and accuracy.Ensure that the new signature process is thoroughly tested to confirm its correctness and compatibility with existing systems.
Run the following script to verify the usage of
signTxand ensure all tests pass:
gartnera
force-pushed
the
upgrade-btc-deps
branch
from
a5324c5 to
024e94a
Compare
gartnera
force-pushed
the
upgrade-btc-deps
branch
from
85b4411 to
a36fc6d
Compare
gartnera
force-pushed
the
upgrade-btc-deps
branch
from
a36fc6d to
c498c9b
Compare
gartnera
changed the title
gartnera
force-pushed
the
upgrade-btc-deps
branch
from
784cb5c to
6c70f22
Compare
Upgrade github.com/btcsuite/btcd. Use tss-lib fork which upgrades btcd.
TODO:
Deferred items:
txscript.NewCannedPrevOutputFetcher([]byte{}, 0)Should probably merge #2767 rapidly after this to get on the latest tss-lib. Update: this work was merged into this change.
Closes #2728
Summary by CodeRabbit
New Features
keygenpackage, enhancing functionality.SignTxmethod.Bug Fixes
Chores