test: fix evm signer instability #3133
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 modifies several test files within the Changes
Assessment against linked issues
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
|
gartnera
force-pushed
the
test-fix-evm-signer-instability
branch
from
05790d5 to
ab02935
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3133 +/- ##
========================================
Coverage 62.64% 62.64%
========================================
Files 424 424
Lines 30115 30115
========================================
Hits 18866 18866
Misses 10408 10408
Partials 841 841 |
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 and nitpick comments (10)
zetaclient/testutils/mocks/tss_signer.go (4)
66-74: Enhance documentation and add chain validationWhile the implementation is correct, consider the following improvements:
- Expand the documentation to explicitly state how addresses are derived and the implications of using
TestPrivateKey.- Add validation for the
chainparameter to prevent initialization with invalid chains.Consider applying this enhancement:
// NewDerivedTSS creates a TSS where evmAddress and btcAdresses are always derived from the test -// private key +// private key. The addresses are deterministically generated from TestPrivateKey, ensuring +// consistent behavior across test runs. This is particularly useful for testing signature +// verification and address recovery. func NewDerivedTSS(chain chains.Chain) *TSS { + if chain.ChainId == 0 { + panic("invalid chain: chain ID cannot be 0") + } return &TSS{ paused: false, chain: chain, PrivKey: TestPrivateKey, } }
Line range hint
19-31: Consider replacing global TestPrivateKey with per-instance generationThe global
TestPrivateKeycould lead to test interference if multiple tests modify it concurrently. Consider making it instance-specific or using a key generator function.Consider refactoring to:
-// TestPrivateKey is a random private key for testing -var TestPrivateKey *ecdsa.PrivateKey - -// init generates a random private key for testing -func init() { - var err error - TestPrivateKey, err = crypto.GenerateKey() - if err != nil { - fmt.Println(err.Error()) - } -} +// generateTestPrivateKey creates a new private key for testing +func generateTestPrivateKey() (*ecdsa.PrivateKey, error) { + return crypto.GenerateKey() +}
Line range hint
124-134: Implement SignBatch for completenessThe
SignBatchmethod returns a "not implemented" error, which could be misleading in tests. Consider implementing it to match the single-signature behavior or explicitly documenting why it's not implemented.Consider implementing the batch signing:
func (s *TSS) SignBatch(_ context.Context, data [][]byte, nonce uint64, priority uint64, blockNumber int64) ([][65]byte, error) { if s.paused { return nil, fmt.Errorf("tss is paused") } - // mock not implemented yet - return nil, fmt.Errorf("not implemented") + signatures := make([][65]byte, len(data)) + for i, msg := range data { + sig, err := s.Sign(context.Background(), msg, nonce, priority, blockNumber, "") + if err != nil { + return nil, fmt.Errorf("failed to sign message %d: %w", i, err) + } + signatures[i] = sig + } + return signatures, nil }
Line range hint
201-208: Add mutex protection for paused stateThe
PauseandUnpausemethods modify shared state without synchronization, which could lead to race conditions in concurrent tests.Add mutex protection:
+import "sync" type TSS struct { + mu sync.RWMutex paused bool // ... other fields } func (s *TSS) Pause() { + s.mu.Lock() + defer s.mu.Unlock() s.paused = true } func (s *TSS) Unpause() { + s.mu.Lock() + defer s.mu.Unlock() s.paused = false } func (s *TSS) Sign(_ context.Context, data []byte, ...) ([65]byte, error) { + s.mu.RLock() + isPaused := s.paused + s.mu.RUnlock() - if s.paused { + if isPaused { return [65]byte{}, fmt.Errorf("tss is paused") } // ... rest of the implementation }zetaclient/chains/evm/signer/sign_test.go (3)
19-19: Consider extracting common test setup into a helper functionThe TSS and signer setup is duplicated across all test functions. Consider extracting this into a helper function to improve maintainability and reduce duplication.
+func setupTestSigner(t *testing.T) (*mocks.TSS, *Signer) { + tss := mocks.NewDerivedTSS(chains.BitcoinMainnet) + evmSigner, err := getNewEvmSigner(tss) + require.NoError(t, err) + return tss, evmSigner +}Usage in tests:
-tss := mocks.NewDerivedTSS(chains.BitcoinMainnet) -evmSigner, err := getNewEvmSigner(tss) -require.NoError(t, err) +tss, evmSigner := setupTestSigner(t)Also applies to: 103-103, 141-141, 179-179, 216-216
Line range hint
63-67: Consider moving gas price constants to package levelThe gas price constants could be reused across tests and their meaning would be clearer at package level.
+const ( + // Gas price constants for testing + GWei = 1_000_000_000 + PriorityFee = 1 * GWei + GasPrice = 3 * GWei +) -const ( - gwei = 1_000_000_000 - priorityFee = 1 * gwei - gasPrice = 3 * gwei -)
Line range hint
61-96: Consider adding edge cases for gas price testingWhile the dynamic fee transaction test is good, consider adding edge cases to ensure robust handling of various gas price scenarios:
- Zero gas price
- Maximum allowed gas price
- Priority fee greater than gas price
Example additional test cases:
t.Run("SignOutbound - should handle zero gas price", func(t *testing.T) { cctx := getCCTX(t) cctx.OutboundParams[0].GasPrice = "0" cctx.OutboundParams[0].GasPriorityFee = "0" // ... implement test }) t.Run("SignOutbound - should handle max gas price", func(t *testing.T) { cctx := getCCTX(t) cctx.OutboundParams[0].GasPrice = MaxGasPrice // ... implement test })zetaclient/chains/evm/signer/signer_test.go (3)
116-123: LGTM! Consider enhancing documentationThe implementation correctly uses
signer.Sender()for transaction verification, which resolves the signature style instability issues. The error handling and address comparison are robust.Consider enhancing the documentation to explain:
- Why
signer.Sender()is preferred over direct signature verification- How it handles different transaction types
- The significance of address comparison versus public key verification
Example:
// verifyTxSender is a helper function to verify the signature of a transaction -// -// signer.Sender() will ecrecover the public key of the transaction internally -// and will fail if the transaction is not valid or has been tampered with +// +// Instead of direct signature verification, this function uses signer.Sender() which: +// 1. Handles different transaction types (legacy, EIP-155, EIP-2930, EIP-1559) +// 2. Recovers the public key using the appropriate signature scheme +// 3. Derives and returns the sender's address +// +// The function will fail if the transaction signature is invalid or has been tampered with.
Line range hint
126-134: Consider enhancing transaction verificationWhile the basic verification is good, consider adding checks for other critical transaction fields that could affect signature verification.
Consider extending the verification:
func verifyTxBodyBasics( t *testing.T, tx *ethtypes.Transaction, to ethcommon.Address, nonce uint64, amount *big.Int, ) { require.Equal(t, to, *tx.To()) require.Equal(t, nonce, tx.Nonce()) require.True(t, amount.Cmp(tx.Value()) == 0) + // Verify transaction type + require.NotNil(t, tx.Type()) + // Verify chain ID if available + if tx.ChainId() != nil { + require.True(t, tx.ChainId().Sign() > 0) + } }
Line range hint
169-190: Enhance test coverage for error scenariosThe happy path is well tested, but consider adding test cases for error scenarios to ensure robust error handling.
Consider adding test cases for:
- Invalid transaction signature
- Network failures during broadcast
- Invalid chain ID
- Gas estimation failures
Example addition:
t.Run("BroadcastOutbound - should handle network failure", func(t *testing.T) { // Setup mock client to simulate network failure evmClient := mocks.NewEVMRPCClient(t) evmClient.On("SendTransaction", mock.Anything, mock.Anything). Return(errors.New("network error")) evmSigner.WithEvmClient(evmClient) // Test broadcast with network failure tx, err := evmSigner.SignERC20Withdraw(ctx, txData) require.NoError(t, err) evmSigner.BroadcastOutbound( ctx, tx, cctx, zerolog.Logger{}, sdktypes.AccAddress{}, mocks.NewZetacoreClient(t), txData, ) // Verify error handling list := evmSigner.GetReportedTxList() require.Empty(t, *list) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
zetaclient/chains/evm/signer/sign_test.go(12 hunks)zetaclient/chains/evm/signer/signer_admin_test.go(15 hunks)zetaclient/chains/evm/signer/signer_test.go(2 hunks)zetaclient/testutils/mocks/tss_signer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
zetaclient/chains/evm/signer/sign_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer_admin_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/tss_signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
zetaclient/chains/evm/signer/sign_test.go (1)
Line range hint 1-246: Implementation successfully addresses EVM signer instability
The changes effectively resolve the signer instability issues by:
- Using
NewDerivedTSSconsistently across all tests - Implementing proper transaction signature verification
- Maintaining comprehensive test coverage for different transaction types
The test suite now properly validates both legacy and dynamic fee transactions, ensuring stable signature verification.
zetaclient/chains/evm/signer/signer_admin_test.go (3)
Line range hint 1-326: Test coverage is comprehensive and well-structured
The test suite demonstrates:
- Consistent error handling across all commands
- Coverage for both successful and failure scenarios
- Clear and specific error messages
- Proper validation of transaction parameters
20-20: Verify network parameter usage across different chains
While using NewDerivedTSS(chains.BitcoinMainnet) fixes the address derivation issue, we should ensure this works correctly for other networks as well.
#!/bin/bash
# Search for other network parameters used with NewDerivedTSS
rg "NewDerivedTSS\(" --type go
# Search for BitcoinMainnet usage in tests
rg "BitcoinMainnet" --type go -C 2Also applies to: 101-101, 149-149, 214-214, 290-290
41-41: Verify implementation of verifyTxSender method
The change to use verifyTxSender aligns with using go-ethereum's more robust signature verification. Let's verify its implementation.
Also applies to: 61-61, 76-76, 90-90, 122-122, 178-178, 237-237, 253-253, 310-310
✅ Verification successful
Verified: verifyTxSender is correctly implemented within the test suite.
All instances of verifyTxSignature have been appropriately replaced with verifyTxSender in the test files, ensuring enhanced signature verification without impacting the production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for verifyTxSender implementation
ast-grep --pattern 'func verifyTxSender($params) {
$$$
}'
# Search for any direct usage of signer.Sender
rg "signer\.Sender\(" --type go
Length of output: 384
Script:
#!/bin/bash
# Search for verifyTxSender implementation across all Go files
rg "func verifyTxSender" --type go
Length of output: 197
gartnera
force-pushed
the
test-fix-evm-signer-instability
branch
from
1e81722 to
1d5fa39
Compare
The current usage of
verifyTxSignatureis not correct as it assumes a very specific signature style. It seems we could actually get transactions with a few different signature styles.There is not actually a stable way to verify the raw signature/ecrecover exported in go-ethereum.
signer.Sender(tx)seems to be the only stable way exported so let's use that instead. It will internally determine which signer was used, properly ecrecover the public key, and return the address.Snippets from go-ethereum showing how different transactions need to be verified differently
There's also some confusion with current MockTSS signers where the addresses are not derived from the private key of the signer. At least in the signer tests, we should always use a TSS implementation where the keys match. Add
NewDerivedTSSwhich ensure this happens.Closes #3081
Summary by CodeRabbit
New Features
Bug Fixes
Documentation