refactor: improve errors & fix blame panic #53
Conversation
📝 WalkthroughWalkthroughThis pull request makes widespread improvements to error handling, logging, and method naming conventions throughout the codebase. Standard library error functions and string formatting have been replaced with the structured error wrapping provided by Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TssServer
participant KeySign
participant BlameMgr
Client->>TssServer: Send Sign Request
TssServer->>KeySign: Invoke SignMessage
KeySign->>KeySign: Process request (with enhanced error wrapping)
alt Error Occurs
KeySign->>BlameMgr: Invoke TSSMissingShareBlame/TSSWrongShareBlame
BlameMgr-->>KeySign: Return blame details
end
KeySign-->>TssServer: Return Signature or Error
TssServer-->>Client: Deliver result
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (5)
cmd/tss/mock_tss_server.go (4)
22-22: Consider using errors.Wrap for richer context.While the import has been updated to
github.com/pkg/errors, the code still useserrors.New(). Consider leveraging the additional context capabilities:- return errors.New("you ask for it") + return errors.New("failed to start mock TSS server: you ask for it")
40-40: Consider using errors.Wrap for richer context.Similar to the previous instance, consider enhancing the error message with more context:
- return keygen.Response{}, errors.New("you ask for it") + return keygen.Response{}, errors.New("failed to generate key: you ask for it")
53-53: Consider using errors.Wrap for richer context.Update the error creation to provide more context:
- return []keygen.Response{{}}, errors.New("you ask for it") + return []keygen.Response{{}}, errors.New("failed to generate keys for all algorithms: you ask for it")
63-63: Consider using errors.Wrap for richer context.Update the error creation to provide more context:
- return keysign.Response{}, errors.New("you ask for it") + return keysign.Response{}, errors.New("failed to sign key: you ask for it")storage/localstate_mgr.go (1)
156-157: Corrected spelling in comment.Updated "unmarshalling" to "unmarshaling" for consistency with Go's spelling conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
blame/blame.go(1 hunks)blame/policy.go(5 hunks)blame/types.go(1 hunks)cmd/tss-benchgen/main.go(1 hunks)cmd/tss-benchsign/main.go(2 hunks)cmd/tss-recovery/tool.go(2 hunks)cmd/tss/mock_tss_server.go(1 hunks)cmd/tss/tss_http.go(2 hunks)common/tss.go(19 hunks)common/tss_helper.go(6 hunks)common/tss_test.go(2 hunks)conversion/conversion.go(5 hunks)conversion/key_provider.go(5 hunks)conversion/tss_helper.go(2 hunks)go.mod(2 hunks)keygen/ecdsa/keygen_test.go(1 hunks)keygen/ecdsa/tss_keygen.go(5 hunks)keygen/eddsa/keygen_test.go(1 hunks)keygen/eddsa/tss_keygen.go(7 hunks)keysign/ecdsa/keysign_old_test.go(3 hunks)keysign/ecdsa/keysign_test.go(1 hunks)keysign/ecdsa/tss_keysign.go(7 hunks)keysign/eddsa/keysign_test.go(1 hunks)keysign/eddsa/tss_keysign.go(4 hunks)keysign/notifier.go(3 hunks)keysign/signature_notifier.go(6 hunks)messages/p2p_message.go(1 hunks)p2p/communication.go(6 hunks)p2p/communication_test.go(7 hunks)p2p/leader_provider.go(1 hunks)p2p/leader_provider_test.go(1 hunks)p2p/party_coordinator.go(8 hunks)p2p/party_coordinator_test.go(4 hunks)p2p/peer_status.go(1 hunks)p2p/stream_helper.go(3 hunks)p2p/stream_helper_test.go(2 hunks)storage/localstate_mgr.go(4 hunks)tss/keysign.go(4 hunks)tss/tss.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (15)
cmd/tss/tss_http.go (1)
tss/tss.go (9) (9)
t(144-147)t(150-160)t(163-165)t(168-170)t(172-176)t(178-202)t(205-207)t(210-227)t(230-232)
common/tss_helper.go (1)
messages/p2p_message.go (3) (3)
TssTaskNotifier(86-88)WrappedMessage(47-51)BroadcastMsgChan(54-57)
conversion/tss_helper.go (6)
conversion/conversion_test.go (1) (1)
testPubKeys(18-23)tss/tss_4nodes_test.go (1) (1)
testPubKeys(40-45)common/tss_test.go (1) (1)
testPubKeys(35-40)keygen/eddsa/keygen_test.go (1) (1)
testPubKeys(34-39)keygen/ecdsa/keygen_test.go (1) (1)
testPubKeys(38-43)blame/policy_test.go (1) (1)
testPubKeys(20-25)
keysign/signature_notifier.go (3)
p2p/stream_helper.go (1) (1)
NewStreamMgr(40-46)p2p/stream_helper_test.go (14) (14)
s(86-88)m(36-41)m(43-45)m(47-49)m(51-53)m(55-57)m(59-61)m(63-68)m(70-75)m(77-79)m(81-84)m(90-95)m(97-99)m(101-103)keysign/notifier.go (5) (5)
n(56-64)n(68-81)n(87-130)n(135-163)n(166-168)
p2p/party_coordinator_test.go (2)
p2p/communication_test.go (2) (2)
logger(122-128)_(19-19)p2p/party_coordinator.go (16) (16)
PartyCoordinator(30-38)NewPartyCoordinator(41-62)pc(65-69)pc(71-111)pc(113-135)pc(138-175)pc(177-181)pc(183-194)pc(196-207)pc(209-238)pc(240-251)pc(253-297)pc(299-391)pc(393-443)pc(445-482)pc(484-486)
keysign/eddsa/tss_keysign.go (1)
keysign/ecdsa/tss_keysign.go (6) (6)
tKeySign(65-67)tKeySign(69-71)tKeySign(73-92)tKeySign(95-209)tKeySign(211-318)localData(135-135)
blame/policy.go (1)
blame/policy_helper.go (3) (3)
m(6-25)m(27-53)m(56-68)
storage/localstate_mgr.go (1)
cmd/tss-recovery/tool.go (1) (1)
KeygenLocalState(20-25)
tss/keysign.go (2)
keysign/response.go (1) (1)
Response(17-21)common/status.go (3) (3)
Status(3-3)Fail(8-8)Success(7-7)
keygen/ecdsa/tss_keygen.go (1)
keygen/keygen.go (2) (2)
Request(22-27)r(40-50)
common/tss.go (7)
blame/manager.go (1) (1)
localParty(102-102)common/tss_helper.go (3) (3)
GetMsgRound(164-281)t(283-307)t(309-341)conversion/conversion.go (2) (2)
AccPubKeysFromPartyIDs(61-75)BytesToHashString(195-202)blame/types.go (3) (3)
TssBrokenMsg(14-14)ErrNotEnoughPeer(20-20)ErrNotMajority(21-21)p2p/communication.go (1) (1)
Message(40-43)messages/p2p_message.go (2) (2)
WireMessage(67-72)WrappedMessage(47-51)p2p/party_coordinator.go (1) (1)
msg(149-149)
p2p/communication.go (3)
p2p/stream_helper.go (1) (1)
NewStreamMgr(40-46)p2p/metric_reporter.go (1) (1)
NewResourceMetricReporter(16-20)logs/fields.go (2) (2)
Host(10-10)Peer(9-9)
keysign/notifier.go (4)
common/tss_helper.go (2) (2)
data(117-117)verifySignature(101-106)keysign/eddsa/tss_keysign.go (1) (1)
New(39-62)keygen/eddsa/tss_keygen.go (1) (1)
New(36-60)keysign/response.go (1) (1)
Signature(9-14)
p2p/communication_test.go (1)
p2p/communication.go (17) (17)
NewCommunication(63-101)c(104-106)c(109-111)c(114-121)c(123-142)c(144-164)c(166-216)c(218-225)c(227-270)c(272-365)c(367-386)c(388-444)c(447-454)c(457-466)c(468-478)c(480-489)c(491-507)
p2p/party_coordinator.go (1)
p2p/stream_helper.go (3) (3)
NewStreamMgr(40-46)WriteStreamWithBuffer(127-162)ReadStreamWithBuffer(93-124)
🪛 GitHub Check: CodeQL
storage/localstate_mgr.go
[failure] 144-144: Uncontrolled data used in path expression
This path depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (233)
blame/types.go (2)
7-7: Enhanced error handling with pkg/errors.The replacement of the standard errors package with github.com/pkg/errors is a significant improvement. The pkg/errors package provides additional functionality like stack traces and error wrapping capabilities, making debugging easier and error messages more informative.
19-25: Error declarations now leverage pkg/errors capabilities.These error declarations now benefit from the additional context provided by github.com/pkg/errors. While the implementation hasn't changed, these errors can now be wrapped with additional context when propagated up the call stack, improving error traceability.
messages/p2p_message.go (1)
80-80: Fixed JSON tag typo: "reqest_hash" → "request_hash".This correction ensures proper JSON serialization/deserialization of the TssControl struct. Accurate JSON tags are essential for maintaining API compatibility and preventing unmarshaling errors.
p2p/peer_status.go (2)
7-7: Improved error handling with pkg/errors.The switch from the standard errors package to github.com/pkg/errors enhances error context and debugging capabilities throughout the file. This is part of a broader effort to standardize error handling across the codebase.
95-95: Error now created with enhanced context capabilities.The error created here now leverages the github.com/pkg/errors package, which allows for stack traces and wrapping. When this error is caught by higher-level functions, they can now add more context without losing the original error information.
p2p/leader_provider.go (2)
9-9: Enhanced error handling with pkg/errors.The switch to github.com/pkg/errors is consistent with the overall PR goal of improving error handling across the codebase. This package provides better debugging capabilities through stack traces and error wrapping.
15-15: Error now supports enhanced context capabilities.This error creation now benefits from the github.com/pkg/errors package's functionality. While the immediate usage is the same as before, the error can now carry stack trace information and be wrapped with additional context as it propagates through the application.
keysign/eddsa/keysign_test.go (1)
293-293: Update test expectation to match refactored error message.The error assertion has been updated to match the new error message format, which removes the "ignored" suffix. This aligns with the refactored error handling approach using
github.com/pkg/errorsthroughout the codebase.cmd/tss/mock_tss_server.go (1)
4-6: Migrate to pkg/errors for enhanced error handling.The import has been updated to use
github.com/pkg/errorsinstead of the standard library's errors package, which is consistent with the codebase-wide error handling refactoring.keygen/eddsa/keygen_test.go (1)
241-241: Update test expectation to match refactored error message.The error assertion has been updated to match the new error message format, removing the "ignored" suffix. This change aligns with similar updates in other test files and reflects the refactored error handling approach throughout the codebase.
p2p/party_coordinator_test.go (3)
103-104: Improve test logging with named loggers.The test now uses a named logger instance instead of a no-op logger, which will improve observability during test execution by clearly identifying which test is generating specific log messages.
Also applies to: 111-111
144-145: Improve test logging with named loggers.Similar to the previous test, this change enhances observability by using a named logger that will clearly identify the source of log messages during test execution.
Also applies to: 151-151
221-222: Improve test logging with named loggers.Consistent with the other test changes, this improves observability by using a named logger for test-specific log output.
Also applies to: 233-233
keygen/ecdsa/keygen_test.go (1)
357-357: Updated error assertion for duplicated notification.The error pattern in the assertion has been modified to remove "ignored" from the expected error message. This aligns with the broader effort to refactor error handling across the codebase, ensuring consistent error messages.
cmd/tss-benchgen/main.go (1)
168-168: Improved error handling using github.com/pkg/errors.The change from
fmt.Errorftoerrors.Errorfenhances error reporting by providing better context and stack traces. This is a positive improvement that aligns with modern Go error handling practices.keysign/ecdsa/keysign_test.go (1)
512-512: Updated error assertion to match refactored error message.The test now correctly verifies the updated error message format without the word "ignored". This change maintains consistency with similar error message modifications in other test files.
cmd/tss-benchsign/main.go (2)
123-123: Enhanced error reporting with github.com/pkg/errors.Replacing
fmt.Errorfwitherrors.Errorfimproves error context and debug information. This change is part of the broader effort to standardize error handling in the codebase.
167-167: Improved error handling consistency.Using
errors.Errorfinstead offmt.Errorfprovides more comprehensive error information, including stack traces, which is valuable for debugging issues in a distributed system. This change is consistent with the error handling approach in other parts of the codebase.p2p/leader_provider_test.go (1)
17-21: Good formatting improvement.The code has been reformatted to improve readability by placing each peer address on a separate line, which is a good practice for maintaining array elements. This change is purely cosmetic and doesn't affect functionality.
tss/tss.go (1)
251-251: Simplified error creation with appropriate method.The error creation has been changed from using
fmt.Errorf()to using the more appropriateerrors.New()since no formatting is required for this static error message. This aligns with the codebase's migration to thegithub.com/pkg/errorspackage for more consistent error handling.common/tss_test.go (2)
14-14: Updated import for enhanced error handling.The import has been changed from the standard library
errorspackage togithub.com/pkg/errors, which offers enhanced error handling capabilities including wrapping errors with context and preserving stack traces.
444-446: Updated error message verification.The error message pattern being checked has been updated to match the new error message format produced after the error handling refactoring. This ensures test compatibility with the modified codebase while maintaining the same verification logic.
cmd/tss/tss_http.go (3)
10-10: Updated import for enhanced error handling.The standard library
errorspackage has been replaced withgithub.com/pkg/errors, which provides better error context and stack trace preservation capabilities.
169-172: Improved error handling with context.The error handling has been enhanced by using
errors.Wrap()instead of a likely previousfmt.Errorf()implementation. This approach preserves the original error context and stack trace, making debugging easier while maintaining the same semantic message.
174-177: Enhanced error context with wrapping.Similar to the previous change, this improves error reporting by wrapping the original error with context instead of simply formatting it. This approach retains the complete error chain and stack trace information.
blame/blame.go (1)
33-40: Improved String() method implementation.The code has been refactored to use a single
fmt.Sprintfcall instead of multiple string operations, resulting in cleaner and more maintainable code.cmd/tss-recovery/tool.go (4)
16-16: Good switch to structured error handling.Replacing standard errors package with
github.com/pkg/errorsprovides better context and stack traces, which will improve debugging capabilities.
23-23: Fixed typo in comment.Comment has been corrected from "paticipant" to "participant".
34-36: Updated to use non-deprecated file reading function.Good replacement of deprecated
ioutil.ReadFilewithos.ReadFilealong with improved error wrapping.
41-41: Enhanced error context with proper wrapping.Using
errors.Wrapfprovides better context when unmarshalling fails, preserving the original error details.p2p/stream_helper_test.go (2)
10-12: Updated error handling package and added logging dependencies.Replacing standard errors with
github.com/pkg/errorsand introducingzerologfor structured logging is consistent with the project-wide improvements.
238-238: Enhanced test configuration with logging capabilities.The test now initializes the StreamMgr with a test-specific logger, allowing for better observability of test execution.
common/tss_helper.go (6)
22-22: Standardized error handling approach.Good switch to
github.com/pkg/errorsfor consistent error handling throughout the codebase.
57-61: Improved error context in MsgToHashString.Enhanced error handling with proper wrapping now preserves the original error details when SHA-256 hash calculation fails.
167-168: Better error reporting for wire message parsing failures.Error context has been improved by wrapping the original error, making it easier to diagnose parsing issues.
279-280: Simplified error handling in default case.Clean transition from formatted error to standard error, maintaining consistent style with the rest of the function.
285-289: Enhanced error context for JSON marshalling failures.Error wrapping now provides clear context when task notifications fail to marshal.
325-327: Consistent error handling for request message processing.The error wrapping pattern is consistently applied here, maintaining the standardized approach throughout the file.
keysign/ecdsa/tss_keysign.go (9)
17-17: Good improvement to error handling.Using the
github.com/pkg/errorspackage is a more robust choice than the standard library for error handling as it provides valuable stack traces and better error wrapping capabilities.
102-103: Correctly updated to wrap errors.Replacing
fmt.Errorfwitherrors.Wrappreserves the original error context while adding more specific information about the failure.
112-113: Appropriate error enhancement.The wrapped error now provides better context for debugging threshold calculation failures.
123-124: Good error context improvement.Enhanced error message for hash conversion failures.
129-130: Consistent error wrapping.The error now provides clear information about the failure point during party creation.
138-139: Better error message for unmarshal failures.The wrapped error provides specific context about which data structure failed to unmarshal.
152-154: Enhanced error visibility.Numbering the identical operation (#1) helps distinguish between multiple similar setup operations when debugging.
157-159: Good distinction between similar errors.Numbering the identical operation (#2) provides clarity when reviewing logs.
188-189: Improved error context for key sign processing.The error wrapping provides a clear hierarchy of what operation failed.
conversion/tss_helper.go (3)
13-14: Good choice of error package.Replacing the standard errors package with github.com/pkg/errors enhances error handling capabilities.
67-69: Excellent error message enhancement.The error now includes the specific pubkey that caused the issue, which significantly improves debugging capability.
72-74: Better context for peer address errors.Including the index in the error message helps identify which peer address is problematic.
keysign/signature_notifier.go (12)
14-15: Good package selection for error handling.Using github.com/pkg/errors provides better error wrapping capabilities.
53-58: Improved logger initialization.The logger is now properly structured with component and host information, enhancing log readability and context.
153-155: Enhanced error message for stream creation failures.The error now includes both the protocol and peer ID, which provides better context for debugging network issues.
159-162: Code comment needs attention.The TODO comment raises a valid question about why the stream is being added to streamMgr.
Consider reviewing the necessity of this stream management approach. Is this stream reused after the function returns? Why not close the stream instead?
174-176: Improved error message for marshalling operations.The error provides clearer context for the specific marshalling failure.
186-188: Consistent error wrapping pattern.Follows the established pattern for error wrapping, maintaining code consistency.
191-193: Better diagnostic information for stream write failures.The error now clearly indicates which operation failed.
196-198: Clear error message for deadline setting.Error messages consistently follow the pattern of operation + context.
206-208: Clean function signature.Function signature now appears more consistent with other function definitions.
210-241: Improved variable usage in broadcast method.The code now more directly uses the signatureItem struct without unnecessary intermediate variables.
306-307: Enhanced error message for notifier creation.The error message clearly indicates the operation that failed.
319-320: Better timeout error message.The error now includes the specific timeout duration, which aids in debugging timing issues.
conversion/conversion.go (4)
23-24: Good choice of error package.Replacing the standard errors package with github.com/pkg/errors enhances error handling capabilities.
32-34: Improved error message for key conversion failures.The error now provides better context about the specific operation that failed.
119-121: Excellent error message enhancement.The error now includes the specific public key string that failed to unmarshal, significantly improving debugging capability.
197-199: Simplified error handling pattern.Error handling has been consolidated to a single if statement with clear context about the hash operation.
p2p/communication_test.go (5)
10-10: Updated import to use entire zerolog package.This change improves the codebase by using the full zerolog package rather than just the log variable, enabling creation of custom loggers for test cases.
22-22: Enhanced logging with context-aware test loggers.Using the custom logger function with the test name as a parameter improves test logs by providing clear context about which test produced each log entry.
43-43: Created dedicated logger for test case.Creating a dedicated logger with the test name improves log readability and debugging capabilities throughout the test function.
66-66: Consistent logger usage throughout test case.The consistent usage of the same logger instance across all communication instances ensures that logs from the same test case can be easily identified and grouped together.
Also applies to: 72-72, 82-82, 94-94, 114-114
122-128: Added helper function to create consistent loggers for tests.The new
loggerfunction creates properly configured loggers with consistent settings (console output, timestamps, and test name context), improving test output readability and standardizing the logging approach.keysign/ecdsa/keysign_old_test.go (4)
21-21: Added pkg/errors for improved error handling.Good addition of the
github.com/pkg/errorspackage which provides better error wrapping capabilities than the standard library.
62-65: Enhanced error context with errors.Wrap.The code now properly wraps unmarshal errors with contextual information using
errors.Wrapinstead of simple string concatenation, improving error traceability and debugging.
73-76: Improved marshalling error handling with errors.Wrap.Using
errors.Wrapfor JSON marshalling errors provides better context while preserving the original error, enhancing error diagnostics.
456-456: Simplified error assertion message.The test assertion for duplicate notification has been simplified to focus on the core issue without unnecessary verbiage, making the test more maintainable.
go.mod (4)
32-32: Moved golang.org/x/sync to direct dependencies.This change correctly identifies
golang.org/x/syncas a direct dependency rather than an indirect one, improving dependency transparency.
38-38: Added github.com/pkg/errors as a direct dependency.Explicitly declaring
github.com/pkg/errorsas a direct dependency aligns with the codebase changes to use it for enhanced error handling throughout the project.
231-233: Added helpful comments about tss-lib fork source.These comments provide valuable context about the origin and purpose of the tss-lib fork, making maintenance and future updates easier to understand.
236-236: Updated tss-lib dependency to point to zeta-chain fork.The replace directive now points to a specific version of the zeta-chain fork, ensuring consistent builds and reducing ambiguity about which implementation is being used.
blame/policy.go (9)
7-7: Added pkg/errors for enhanced error handling.Importing
github.com/pkg/errorsis a good practice for providing more context to errors through wrapping capabilities.
14-22: Simplified standby set initialization.The code now initializes the standby set more directly using a loop instead of the previous slice-based approach, improving clarity and maintaining the same functionality.
39-40: Enhanced error context for blame key derivation.Using
errors.Wrapprovides better context for errors when deriving blame public keys, improving error diagnostics and traceability.
53-54: Improved peer ID conversion error handling.The error from peer ID conversion is now wrapped with additional context, making it clearer where and why the error occurred.
86-87: Enhanced timeout error with specific context.Wrapping
ErrTimeoutTSSwith specific information about failing to find peers for the given message type improves error diagnostics.
100-101: Improved error context for blame peer retrieval.The error is now wrapped with specific information about being unable to get blamed peers, enhancing error diagnostics.
115-116: Added context to tssTimeoutBlame errors.Wrapping errors from
tssTimeoutBlamewith the source function name improves error stack traces and debugging.
131-132: Standardized error message wording.The error message has been updated to use consistent "unable to" phrasing, improving consistency across the codebase.
136-137: Enhanced context for party ID conversion error.Adding specific context through
errors.Wrapabout being unable to convert party ID to public key improves error diagnostics.conversion/key_provider.go (10)
16-16: Good addition of thegithub.com/pkg/errorspackage.The switch from standard library error formatting to structured error wrapping is a solid improvement that will make debugging easier by preserving stack traces and error context.
23-24: Improved error handling witherrors.Wrapf.This change enhances error context by preserving the original error while adding more specific information about what operation failed and with which input.
28-29: Enhanced error message with proper context.The error now clearly indicates the failure is related to converting the unmarshal operation for secp256k1 keys, which improves troubleshooting.
53-54: Consistent error wrapping in GetPeerIDs.The error handling approach is consistently applied throughout the codebase, making error patterns more predictable.
68-69: Proper error context in GetPubKeysFromPeerIDs.Error now includes which specific peerID caused the failure, aiding in troubleshooting.
81-82: Comprehensive error wrapping in GetPubKeyFromPeerID.All error paths now include proper context about which operation failed and with which input, creating a consistent error handling pattern.
Also applies to: 86-87, 91-92
94-95: Improved variable declaration for pubKey.This change simplifies the code by using a more direct initialization of the coskey.PubKey structure with the raw bytes.
102-103: Enhanced error messages in GetPriKey.The error messages now clearly identify which specific decoding operation failed without losing the original error context.
Also applies to: 107-108
110-110: Simplified return statement.This change improves code readability by directly returning the private key from the raw bytes.
126-127: Consistent error handling in CheckKeyOnCurve.The error message pattern is maintained throughout the codebase, creating a more maintainable error handling system.
keygen/ecdsa/tss_keygen.go (12)
13-13: Appropriate import of structured error handling package.Using
github.com/pkg/errorsenables better error context preservation across function calls.
77-80: Improved parameter naming and added nil check.The change from
keygenReqtoreqis more concise, and the early check for nil pre-parameters prevents potential nil pointer dereferences later in the code.
84-85: Enhanced error context for keygen parties failure.The wrapped error provides more context about the specific operation that failed.
94-95: Better error context for threshold calculation failure.Error now includes information about the specific operation that failed.
97-107: Improved variable declaration organization.Grouping related variable declarations into a single var block enhances code readability and maintainability.
109-117: Streamlined error handling for ID map setup.The error handling has been simplified with clear error messages that distinguish between the two separate setup operations.
153-154: Proper error context in ProcessOutCh failure.The error now preserves the original error context while adding information about the specific operation that failed.
164-164: Consistent spacing throughout the code.The added spacing improves code readability by separating logical blocks of code.
251-252: Enhanced error context for Thorchain pubkey retrieval failure.The wrapped error provides more context about the specific operation that failed.
256-257: Improved error handling for marshaling operation.Using
errors.Wrappreserves the original error context while adding specific information about the marshaling operation.
262-263: Better error context for state saving failure.The error now includes which specific storage operation failed and what was being saved.
267-268: Consistent error logging for address book saving failure.The error logging maintains the same pattern used throughout the codebase.
keysign/notifier.go (10)
14-14: Appropriate import of structured error handling package.Using
github.com/pkg/errorsenables better error context preservation across function calls.
90-92: Simplified variable reference.The variable reference
pkis now directly used, improving code readability.
93-96: Enhanced error context for pubkey unmarshalling failure.The wrapped error provides specific information about which pubkey failed to unmarshal.
102-103: Better error message for pubkey parsing failure.Error now includes which specific pubkey caused the parsing failure.
107-108: Simplified error message for verification failure.The error message is now more direct and concise.
110-110: Improved code flow with explicit nil return.Adding an explicit return statement makes the function flow clearer.
114-115: Consistent error handling patterns throughout verifySignature.All error paths now use the same error wrapping pattern, improving code maintainability.
Also applies to: 118-119, 123-123
139-141: Early return for empty data slice.This change improves function efficiency by handling the empty data case early.
143-157: Streamlined signature verification process.The signature verification loop has been simplified with clearer error handling, improving readability and maintenance.
159-161: Simplified success path.Code flow is more logical with the success handling at the end of the function after all validations have passed.
tss/keysign.go (8)
28-28: Good introduction of a constant for repeated string literal.Using a constant for the "signature generated" message improves code maintainability and reduces the risk of typos.
148-149: Consistent use of the string constant.Using the defined constant instead of string literals ensures consistency throughout the codebase.
162-163: Standardized error handling pattern.The consistent use of the constant for signaling signature generation improves code clarity.
171-172: Consistent notification of signature generation.Using the defined constant maintains consistency in how signature generation is signaled.
175-176: Enhanced error context for signature broadcasting.The wrapped error provides more context about the specific operation that failed.
182-183: Simplified metrics update function.The method has been reduced to a single line that clearly expresses its intent, making the code more maintainable.
259-260: Better error context for local state retrieval failure.The error now includes specific information about the state manager operation that failed.
266-267: Enhanced error message for message decoding failure.The wrapped error includes which specific message failed to decode, aiding in troubleshooting.
storage/localstate_mgr.go (11)
21-21: Appropriate addition of the errors package.Adding the
github.com/pkg/errorspackage aligns with the PR objective of improving error handling throughout the codebase.
108-108: Improved error context with error wrapping.Converting from a basic formatted error to using
errors.Wrappreserves the original error context and stack trace, making debugging easier.
113-114: Better error context for file path errors.Using
errors.Wrapprovides additional context about where in the execution path the error occurred.
118-119: Enhanced encryption error clarity.The error message now provides clear context that the failure occurred during fragment encryption.
146-147: Enhanced file read error with path information.Using
errors.Wrapfwith path interpolation improves error messages by including the specific file path that failed to be read.
151-152: Improved error context for decryption failures.Wrapping the error provides better context that the failure occurred during data decryption.
159-160: Enhanced error context for unmarshal failures.Wrapping the error with specific context about the unmarshal target type improves debugging.
167-168: Improved error context for marshal operations.Using
errors.Wrapprovides better error context, specifying which data structure failed to marshal.
173-174: Added explicit mutex unlocking.The code now uses an explicit
deferstatement to ensure the mutex is unlocked, improving readability and reducing the risk of deadlocks.
232-233: Enhanced error context for address validation failures.Using
errors.Wrapfwith interpolation provides better context about the specific validation failure.
237-238: Improved code structure with consistent line breaks.Adding a consistent empty line here improves code readability.
keygen/eddsa/tss_keygen.go (14)
12-12: Added proper error handling package.Including the
github.com/pkg/errorspackage enables improved error context throughout the file.
70-70: Simplified parameter naming.Changing the parameter name from
keygenReqtoreqimproves readability while maintaining semantics.
72-75: Enhanced error context for party initialization.Using
errors.Wrapprovides better context when failures occur during party initialization.
78-79: Updated parameter reference.Changed to use
req.Keysto match the new parameter name.
83-85: Improved threshold error handling.Using
errors.Wrapadds more context to threshold calculation errors.
87-95: Improved variable declaration block.Consolidating variable declarations into a single
varblock improves readability and maintainability.
98-106: Enhanced error messages for ID map setup.The error messages now distinguish between different setup attempts with numbered identifiers, making debugging easier.
122-122: Consistent line spacing.Added line break for better code structure and readability.
125-125: Improved code formatting.Added consistent line break before the goroutine implementation.
135-135: Consistent spacing.Added line break for better visual separation of the goroutine.
140-142: Enhanced error context for key generation processing.Using
errors.Wrapprovides better context when key generation processing fails.
153-153: Consistent spacing.Added line break before the return statement for better readability.
237-238: Enhanced error context for public key retrieval.Using
errors.Wrapprovides more context when failures occur during public key conversion.
247-248: Improved error context for storage operations.Better error message specifies that the failure occurred during the save operation to storage.
p2p/stream_helper.go (19)
5-6: Added necessary bytes package import.The
bytespackage is needed for the buffer operations added later in the file.
13-14: Integrated better error handling package.Adding the
github.com/pkg/errorspackage supports the improved error reporting throughout the file.
16-16: Added import for structured logging.The
logspackage provides constants for consistent logging field names across the codebase.
21-23: Standardized timeout constants.Consistent timeout constant naming and formatting improves readability and maintainability.
36-36: Improved mutex naming convention.Renaming
streamLockertomufollows Go's standard naming conventions for mutex variables.
40-45: Enhanced stream manager constructor.Adding logger parameter enables structured logging and improves component identification in logs.
49-58: Streamlined read lock handling.The code now uses a more idiomatic pattern for read locks and implements an early return pattern for the no-op case.
60-66: Enhanced stream reset error handling.Improved error reporting with structured logging provides better diagnostics when stream reset fails.
69-72: Improved mutex handling for stream cleanup.More straightforward locking pattern when cleaning up stream references.
80-87: Simplified stream addition logic.The code now uses defer for mutex unlocking and implements a more direct approach when adding a new entry to the map.
89-89: Simplified slice append operation.Direct assignment to the map entry improves readability.
105-109: Improved header reading error handling.Using
errors.Wrapfwith byte count information provides more context when header reading fails.
111-114: Enhanced payload size validation.Better error message includes both the received size and maximum allowed size for clarity.
116-121: Improved payload reading error context.Error now includes both the number of bytes read and the expected payload size for better diagnostics.
123-123: Consistent return formatting.Added line break before the return statement for better readability.
128-131: Enhanced payload size validation.Clearer variable naming and error message that includes both the actual and maximum payload sizes.
135-142: Improved deadline handling.Better variable naming and more descriptive error messages for deadline-related failures.
145-154: Enhanced buffer construction.The code now uses a more idiomatic approach with
bytes.Bufferfor message header and payload construction.
156-159: Improved write error reporting.Error message now includes information about both the written and expected byte counts.
p2p/communication.go (15)
70-75: Improved address creation readability.Extracting the address format to a named variable
listenTomakes the code easier to read and maintain.
77-77: Explicit initialization of multiaddr variable.Explicitly setting
externalAddrto nil makes the code's intent clearer.
80-84: Enhanced external address error handling.Using a named variable
myselffor the external address format and including it in the error message improves diagnostics.
98-98: Updated stream manager initialization.Now correctly passes the logger to the stream manager for consistent logging.
151-156: Improved error handling control flow.Using a switch statement makes the error handling logic clearer and more maintainable.
275-276: Enhanced key unmarshaling error context.Better error message provides specific context about the key unmarshaling operation.
322-325: Improved resource manager error context.Enhanced error message specifies the failure occurred during resource manager creation.
329-330: Better connection manager error context.More specific error message about the connection manager creation failure.
341-342: Enhanced host creation error reporting.Error message now clearly indicates the failure occurred during p2p host creation.
345-348: Improved structured logging.The code now uses structured logging with field names from the logs package, improving log consistency and readability.
353-362: Enhanced bootstrap connection retry logic.Improved switch/case structure makes the retry logic clearer and more maintainable.
364-364: Better bootstrap connection error reporting.Enhanced error message provides context about the bootstrap peer connection failure.
367-369: Improved parameter naming and debug logging.More consistent parameter name
pidand clearer debug log message.
375-375: Enhanced peer connection logging.Using structured logging with the logs.Peer constant improves log consistency.
380-383: Improved stream creation error context.Error message now includes the protocol ID for better diagnostics.
keysign/eddsa/tss_keysign.go (7)
16-16: Good addition of the errors package.Adopting
github.com/pkg/errorsinstead of the standard library'serrorspackage provides enhanced error handling capabilities such as error wrapping and stack trace capture.
109-111: Proper error wrapping improves error context.Using
errors.Wrappreserves the original error while adding contextual information, making it easier to trace the error path.
118-120: Consistent error handling approach.This change maintains the same pattern of wrapping errors with context, ensuring consistency across the codebase.
130-132: Improved error context for hash conversion failures.The error message now provides clear context when message-to-hash conversion fails.
137-140: Clear error context for party creation failures.This provides better diagnostics when party creation fails during batch signing.
146-149: More specific error message for unmarshalling failures.Changed from generic "fail to unmarshal the local saved data" to the more specific "fail to unmarshal LocalPartySaveData", which provides clearer context about what exactly failed to unmarshal.
192-194: Consistent error handling in process key sign.Maintains the pattern of using
errors.Wrapfor consistent error handling throughout the module.common/tss.go (25)
4-5: Proper import organization.Added the
bytespackage directly with the standard library imports and includedgithub.com/pkg/errorsfor enhanced error handling.Also applies to: 15-15
127-134: Improved variable declaration structure.Using a
varblock for related variables improves readability and clearly groups related variables together. This is a common Go idiom for better code organization.
136-139: More descriptive error message.Changed from the generic "broken tss share" to the more specific "GetMsgRound failed", which provides clearer context about the nature of the failure.
141-149: Simplified control flow for error handling.The error handling flow has been streamlined, with clearer assignment of the message identifier and more consistent error handling patterns.
214-219: Variable naming consistency and improved error message.Changed
pubkeystopubKeysfor better naming convention adherence and improved the error message clarity witherrors.New.
239-240: Proper error wrapping with context.Using
errors.Wrapto provide additional context while preserving the original error details and stack trace.
361-363: Improved nil checks with cleaner syntax.The condition has been rewritten to check for nil more explicitly with
nil == wrappedMsg, followed by a more descriptive error message.
365-374: Streamlined error handling pattern in decoder.Using the consistent pattern of
if err := dec.Decode(&msg); nil != err {for error handling throughout the switch statement improves readability and maintainability.
381-385: Explicit variable assignment and improved error handling.The variable assignment for return value is now more explicit, and the error handling is more consistent.
411-418: Added explicit nil check for message.Added an explicit check for
msg.Msg == nilto prevent potential nil pointer dereferences, which enhances the robustness of the code.
426-429: Simplified return path for processing messages.Streamlined the code flow for handling received messages, making the logic easier to follow.
492-495: Better error wrapping for marshalling failures.Provides more specific context when marshalling fails, which helps with debugging and error tracing.
496-499: Consistent error wrapping pattern.Maintains the same pattern of using
errors.Wrapfor error handling, ensuring consistency throughout the codebase.
507-511: Improved error context for marshalling failures.Better error message when failing to marshal wire messages, which provides more clarity during debugging.
519-535: Enhanced peer collection logic.Restructured the peer ID collection logic to be more readable and efficient, with clear error handling and improved variable naming.
575-603: Consistent map iteration pattern.Using consistent variable naming and handling in the Range operations across cached wire message lists, which improves code readability and maintainability.
614-636: Improved control flow with switch-case for error handling.Restructured the error handling using a switch-case statement based on error types, which makes the code more readable and maintainable. The error messages are also more descriptive.
642-646: Clearer comments and simplified cleanup.Added a clearer comment about information removal, and simplified the cleanup logic, which improves code readability.
770-774: Improved error message for hash calculation failures.Better error context when failing to hash wire messages, providing more clarity during debugging.
778-780: Clearer error message for peer resolution failures.More specific error message when failing to find data owner peer ID, which provides better context for debugging.
781-785: Structured variable declarations.Using a
varblock for related variables improves readability and organization, following Go best practices.
795-800: Improved handling of empty peer lists.Added a clear comment explaining why an empty peer list might occur (during E2E tests) and handled this case gracefully, which improves robustness.
803-805: Consistent error wrapping pattern.Maintains the same pattern of using
errors.Wrapfor error handling with descriptive context.
855-857: Better error context for hash calculation failures.More specific error message when failing to hash wire messages, providing better diagnostics.
877-879: Consistent error handling pattern.Using
errors.Wrapwith descriptive context for threshold calculation failures, maintaining the consistent pattern throughout the codebase.p2p/party_coordinator.go (14)
12-15: Good package imports for error handling and concurrency.Added
github.com/pkg/errorsfor enhanced error handling andgolang.org/x/sync/errgroupfor better concurrent error management, which improves code robustness.
56-57: Proper logger initialization for stream manager.Now passing the logger to the stream manager, which allows for better tracing and debugging of stream-related operations.
201-202: Enhanced error context for peer ID decoding failures.Using
errors.Wrapfprovides better context by including the problematic peer ID in the error message.
212-214: Improved error logging for marshalling failures.Enhanced error logging by including the message in the log for better debugging.
216-238: Excellent use of errgroup for concurrent operations.Replaced
sync.WaitGroupwitherrgroup.Groupfor managing concurrent goroutines. This is a more elegant approach for concurrent error handling, though in this case errors are only logged, not propagated.
243-244: Consistent error wrapping pattern.Using
errors.Wrapfor providing context when marshalling fails, maintaining consistency across the codebase.
247-248: Better error context for sending failures.Enhanced error message when sending messages to peers fails, aiding in troubleshooting.
266-267: More specific error message for stream creation failures.The error message now includes both the protocol ID and the peer ID, providing more context for debugging connection issues.
270-271: Preserved important comment as a TODO.Maintained a comment about potentially redundant stream management as a TODO, which is good practice to highlight areas for future improvement.
281-283: Consistent error wrapping for stream write failures.Using
errors.Wrapprovides better context when writing to streams fails.
285-294: Simplified control flow for response handling.Inverted the condition check to return early when a response is not needed, which makes the code more readable by reducing nesting.
306-308: Explicit message type assignment.Added explicit assignment of
MsgTypefield in theJoinPartyLeaderCommstructure, ensuring correct message type for the request.
323-327: Enhanced error logging for request failures.Added more detailed error logging when requests to the leader fail, including the message ID and leader ID, which aids in troubleshooting.
424-428: Explicit message type assignment for response.Similar to the request case, added explicit assignment of
MsgTypefield for the response message.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
keysign/signature_notifier.go (2)
204-204: Remove unnecessary error return.Line 204 returns
errdirectly, but at this point in the function,errshould be nil since any error would have been returned earlier. This appears to be a redundant return that could be simplified.- return err + return nil
212-226: Rename local variable to avoid confusion.The local variable
sig(line 221) has the same name as the parametersig(line 212), which could lead to confusion. Consider renaming the local variable to something more descriptive likesignatureItemorsigItem.- sig := &signatureItem{ + sigItem := &signatureItem{ messageID: messageID, peerID: peerID, signatureData: sig, }And update the reference on line 231:
- if err := s.sendOneMsgToPeer(sig); err != nil { + if err := s.sendOneMsgToPeer(sigItem); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
keysign/signature_notifier.go(6 hunks)p2p/communication.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- p2p/communication.go
🧰 Additional context used
🧬 Code Definitions (1)
keysign/signature_notifier.go (2)
p2p/stream_helper.go (2) (2)
NewStreamMgr(40-46)WriteStreamWithBuffer(127-162)keysign/notifier.go (5) (5)
n(56-64)n(68-81)n(87-130)n(135-163)n(166-168)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (6)
keysign/signature_notifier.go (6)
53-57: Good enhancement of logger initialization.The initialization of
streamMgrwith the logger parameter and the enhanced logger configuration with component and host ID information improves observability. This approach provides more context in logs, making troubleshooting easier.
152-155: Appropriate use of errors.Wrapf for better error context.The replacement of direct error returns with
errors.Wrapfprovides valuable context about the failure, including protocol and peer information. This follows best practices for error handling.
173-175: Good error handling improvement.The error message now clearly indicates what operation failed, making debugging easier.
184-202: Well-structured error handling for stream operations.The consistent use of
errors.Wrapfwith specific context for each error case significantly improves the diagnostic capabilities of this code.
231-236: Improved error logging with context.The error logging now includes more context (message ID and peer ID) using structured logging, which will make troubleshooting easier.
321-321: Better timeout error message.The error message now clearly indicates the timeout duration, making it easier to understand the issue.
|
Got panic during E2E. Related: #42. UPD: fixed 2025-03-21T15:41:55Z ERR error in get unicast blame error="fail to find peers of the given msg type: error timeout TSS" component=keysign module=tss msg_id=ddd514b6135f15d4552c0267820ab4b9329196f62324cd52a5b197166bfaaf50
panic: runtime error: index out of range [8] with length 8
goroutine 3351 [running]:
github.com/zeta-chain/go-tss/blame.(*Manager).TssMissingShareBlame(0x4001b69c00, 0x8, 0x1e)
/go/pkg/mod/github.com/zeta-chain/[email protected]/blame/policy.go:159 +0x7f0
github.com/zeta-chain/go-tss/keysign/ecdsa.(*TssKeySign).processKeySign(0x4001131800, 0x3, 0x4002c95e30, 0x40006ffce0, 0x40006ffd50)
/go/pkg/mod/github.com/zeta-chain/[email protected]/keysign/ecdsa/tss_keysign.go:276 +0xabc
github.com/zeta-chain/go-tss/keysign/ecdsa.(*TssKeySign).SignMessage(0x4001131800, {0x4002e21440, 0x3, 0x4}, {{0x40003bba40, 0x4b}, {0x4002b70000, 0x2d2d, 0x2d2d}, {0x4000c99200, ...}, ...}, ...)
/go/pkg/mod/github.com/zeta-chain/[email protected]/keysign/ecdsa/tss_keysign.go:185 +0x9fc
github.com/zeta-chain/go-tss/tss.(*Server).generateSignature(0x4001294a20, {0x4002d1e4c0, 0x40}, {0x4002e21440, 0x3, 0x4}, {{0x400292b810, 0x4b}, {0x4002211e60, 0x3, ...}, ...}, ...)
/go/pkg/mod/github.com/zeta-chain/[email protected]/tss/keysign.go:156 +0x988
github.com/zeta-chain/go-tss/tss.(*Server).KeySign.func4()
/go/pkg/mod/github.com/zeta-chain/[email protected]/tss/keysign.go:318 +0xd4
created by github.com/zeta-chain/go-tss/tss.(*Server).KeySign in goroutine 3419
/go/pkg/mod/github.com/zeta-chain/[email protected]/tss/keysign.go:316 +0xec8 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tss/keysign.go (1)
36-44: Consider extracting panic recovery to a utility function.Both panic recovery blocks are identical except for the message. Consider creating a helper function to reduce code duplication:
+func (t *Server) recoverPanic(msgID, functionName string) { + if r := recover(); r != nil { + t.logger.Error(). + Str(logs.MsgID, msgID). + Any("panic", r). + Bytes("stack_trace", debug.Stack()). + Msgf("PANIC during %s", functionName) + } +} func (t *Server) waitForSignatures(...) (...) { - defer func() { - if r := recover(); r != nil { - t.logger.Error(). - Str(logs.MsgID, msgID). - Any("panic", r). - Bytes("stack_trace", debug.Stack()). - Msg("PANIC during waitForSignatures") - } - }() + defer t.recoverPanic(msgID, "waitForSignatures") // Rest of the function } func (t *Server) generateSignature(...) (...) { - defer func() { - if r := recover(); r != nil { - t.logger.Error(). - Str(logs.MsgID, msgID). - Any("panic", r). - Bytes("stack_trace", debug.Stack()). - Msg("PANIC during generateSignature") - } - }() + defer t.recoverPanic(msgID, "generateSignature") // Rest of the function }This would improve maintainability and ensure consistent panic handling.
Also applies to: 70-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blame/policy.go(7 hunks)keygen/eddsa/tss_keygen.go(7 hunks)keysign/ecdsa/tss_keysign.go(9 hunks)messages/TssPhaseMessages.go(1 hunks)tss/keysign.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- keysign/ecdsa/tss_keysign.go
- blame/policy.go
- keygen/eddsa/tss_keygen.go
🧰 Additional context used
🧬 Code Definitions (1)
tss/keysign.go (6)
common/tss.go (5) (5)
t(122-153)t(155-161)t(164-166)t(168-170)t(172-174)keysign/response.go (1) (1)
Response(17-21)logs/fields.go (1) (1)
MsgID(8-8)common/status.go (3) (3)
Status(3-3)Fail(8-8)Success(7-7)blame/types.go (1) (1)
Blame(41-47)keysign/notifier_test.go (1) (1)
_(17-17)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (9)
tss/keysign.go (9)
5-5: Good addition of debug support.Adding the debug package import is necessary for capturing stack traces in panic recovery blocks, which significantly improves diagnostic capabilities.
29-29: Excellent constant introduction.Extracting the repeated string literal "signature generated" into a constant improves maintainability and reduces the risk of typos or inconsistencies across the codebase.
36-44: Strong improvement to error handling.The addition of panic recovery with stack trace logging is an excellent defensive programming practice. This will significantly improve the ability to diagnose issues in production by providing complete context when unexpected failures occur.
70-78: Consistent panic handling approach.This panic recovery follows the same pattern as implemented in
waitForSignatures, providing consistent error handling throughout the codebase. The inclusion of stack traces will be invaluable for troubleshooting.
169-170: Good use of the new constant.Replacing all occurrences of the string literal with the constant
msgSignatureGeneratedimproves consistency and maintainability.Also applies to: 183-184, 192-193
196-197: Proper error context with errors.Wrap.Using
errors.Wrapprovides better context than the previous approach, making error diagnosis more straightforward. This follows the refactoring goal of improving error handling throughout the codebase.
203-204: Clean code simplification.Nicely simplified the method to a single line that accomplishes the same functionality in a more readable manner.
214-215: Consistent error handling improvement.The consistent use of
errors.Wrapanderrors.Wrapfthroughout theKeySignmethod enhances the error context and aligns with the PR's objective of improving error handling.Also applies to: 280-281, 287-288
275-276:❓ Verification inconclusive
Verify panic handling for resource cleanup.
The
deferblock for cleanup doesn't have panic recovery. Consider whether this section should also have panic recovery to ensure resources are properly released even in case of unexpected errors.
🏁 Script executed:
#!/bin/bash # Check if the codebase applies consistent panic recovery in resource cleanup defer blocks # Search for defer blocks that release resources without panic recovery rg -A 5 "defer func\(\)" --type go | grep -B 5 -A 5 "Release|Cancel|Remove" | grep -v "if r := recover" # Check if there are any instances where panic recovery is applied in cleanup defer blocks rg -A 10 "defer func\(\)" --type go | grep -B 5 -A 10 "Release|Cancel|Remove" | grep "if r := recover"Length of output: 210
Panic Recovery for Resource Cleanup in tss/keysign.go
It appears that the
deferblock at lines 275–276 currently invokes resource cleanup viat.partyCoordinator.RemovePeerGroup(msgID)without an accompanying panic recovery step. Our recent search did not reveal any similar instances where a recovery mechanism (e.g., checkingif r := recover()) is applied to cleanup defer blocks. To ensure that resources are reliably released even in the event of unexpected panics, please manually verify whether a panic recovery wrapper should be incorporated here (or if the current strategy is a deliberate design choice consistent with the rest of the codebase).
swift1337
changed the title
|
E2E passes in node's repo setup | ⏳ e2e tests passed, checking tx priority
setup | ✅ e2e tests completed in 5m56.781293397s
setup | ---📈 Network Report ---
setup | Block Height: 204
setup | CCTX Processed: 83
setup | Emissions Pool Balance: 19998066ZETA
e2e passed |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
common/tss.go (1)
196-214:⚠️ Potential issueInconsistent error variable usage
There's a mismatch between the error variable used in the log message (
err.Cause()) and the error variable fromconversion.AccPubKeysFromPartyIDs(errBlame).-t.logger.Error().Err(err.Cause()).Msg("error in get the blame nodes") +t.logger.Error().Err(errBlame).Msg("error in get the blame nodes")
🧹 Nitpick comments (4)
common/tss.go (4)
252-256: Unclear error message for unknown partyThe error message "get message from unknown party" followed by a party ID is logically inconsistent. If the party is unknown, we shouldn't have its ID.
-return errors.Errorf("get message from unknown party %q", partyID.Id) +return errors.Errorf("party ID %q not found in party map", wireMsg.Routing.From.Id)
394-395: Improve error message for duplicate notificationThe error message could be more descriptive about why duplicate notifications are problematic.
-return errors.Errorf("duplicated notification from peer %s", peerID) +return errors.Errorf("ignoring duplicate task completion notification from peer %s (already recorded)", peerID)
523-524: Improve error message for party lookup failureThe current error message uses
party.String()which might not provide the most useful information.-t.logger.Error().Str("party", party.String()).Msg("Unable in find the P2P ID") +t.logger.Error().Str("partyId", party.Id).Msg("Unable to find P2P ID for party")
627-630: Simplify error wrappingThe error wrapping could be simplified to avoid double-wrapping.
-return errors.Wrap(blame.ErrHashCheck, "error in getting the blame nodes") +return errors.New("error in getting the blame nodes: hash check failed")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
blame/policy.go(6 hunks)blame/policy_test.go(2 hunks)common/tss.go(19 hunks)keygen/ecdsa/keygen_test.go(8 hunks)keygen/ecdsa/tss_keygen.go(6 hunks)keygen/eddsa/keygen_test.go(2 hunks)keygen/eddsa/tss_keygen.go(6 hunks)keygen/keygen.go(1 hunks)keysign/ecdsa/tss_keysign.go(10 hunks)keysign/eddsa/tss_keysign.go(5 hunks)p2p/communication.go(7 hunks)tss/keygen.go(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- blame/policy_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- keygen/eddsa/keygen_test.go
- keygen/ecdsa/keygen_test.go
- keysign/ecdsa/tss_keysign.go
- blame/policy.go
- p2p/communication.go
- keysign/eddsa/tss_keysign.go
🧰 Additional context used
🧬 Code Definitions (3)
keygen/ecdsa/tss_keygen.go (1)
conversion/conversion.go (6)
localPartyID(113-113)GetParties(110-142)GetThreshold(204-210)SetupPartyIDMap(77-83)SetupIDMaps(99-108)GetPeersID(85-97)
keygen/eddsa/tss_keygen.go (1)
conversion/conversion.go (6)
localPartyID(113-113)GetParties(110-142)GetThreshold(204-210)SetupPartyIDMap(77-83)SetupIDMaps(99-108)GetPeersID(85-97)
common/tss.go (5)
common/tss_helper.go (3)
GetMsgRound(164-281)t(283-307)t(309-341)conversion/conversion.go (2)
AccPubKeysFromPartyIDs(61-75)BytesToHashString(195-202)blame/types.go (1)
TssBrokenMsg(14-14)tss/tss.go (1)
New(69-141)messages/p2p_message.go (2)
WireMessage(67-72)WrappedMessage(47-51)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (38)
keygen/keygen.go (1)
17-18: Method renaming enhances code clarity.The renaming of methods from
GetTssKeyGenChannels()toKeygenChannel()andGetTssCommonStruct()toCommon()improves code readability by making the interface more concise while preserving its functionality.tss/keygen.go (7)
36-48: Constructor naming updated for consistency.The change from
ecdsa.NewTssKeyGentoecdsa.Newfollows the Go convention of using shorter constructor names when the package name already provides context. This is a good improvement for code style.
66-66: Method reference updated to match interface change.This change correctly updates the method call to use the renamed interface method
KeygenChannel().
83-83: Method reference updated to match interface change.This change correctly updates the method call to use the renamed interface method
Common().
210-222: Constructor naming updated for consistency.The change from implicit
ecdsa.NewTssKeyGentoecdsa.Newmaintains consistency with the earlier similar change.
244-244: Method reference updated to match interface change.This change correctly updates the method call to use the renamed interface method
KeygenChannel().
262-262: Method reference updated to match interface change.This change correctly updates the method call to use the renamed interface method
Common().
331-331: Method reference updated to match interface change.This change correctly updates the method call to use the renamed interface method
Common().keygen/ecdsa/tss_keygen.go (11)
13-13: Improved error handling with pkg/errors.Switching to
github.com/pkg/errorsprovides better error wrapping capabilities with stack traces, which is essential for debugging distributed systems like TSS.
26-36: Type name simplified for better readability.Renaming from
TssKeyGentoKeygenfollows Go conventions better by avoiding redundancy, as the package nameecdsaalready provides context.
38-50: Constructor naming simplified.Renaming from
NewTssKeyGentoNewfollows standard Go conventions and improves code readability, as the package and type names already provide sufficient context.
51-51: Improved logging with component and message ID.The structured logging approach ensures consistent log entries with relevant contextual information, which is crucial for troubleshooting.
66-72: Method renaming for improved clarity.The methods have been renamed from traditional getter-style names to more concise, idiomatic Go names that better reflect their purpose.
74-77: Added pre-parameters validation.This new validation prevents potential nil pointer dereferences later in the code, which could cause panics. This is a good defensive programming practice.
79-92: Enhanced error messaging with contextual information.Using
errors.Wrapfinstead of basic error creation improves error context, making it easier to diagnose issues by providing more detailed error paths.
94-104: Variable declarations grouped for better readability.Grouping variable declarations in a single
varblock improves code organization and makes it clearer that these variables are conceptually related.
106-114: Improved error handling for ID map setup.The enhanced error messages now distinguish between the first and second ID map setup failures, making it easier to identify which specific operation failed.
147-151: Enhanced error handling for process key sign failure.Using
errors.Wrapfprovides better context about the specific error that occurred during key generation processing.
169-264: Log and error message improvements throughout processKeyGen.The changes consistently update logging and error handling to use the new struct name references and provide more informative error messages.
keygen/eddsa/tss_keygen.go (7)
12-12: Improved error handling with pkg/errors.Switching to
github.com/pkg/errorsprovides better error wrapping capabilities with stack traces, which is consistent with the changes in the ECDSA implementation.
25-34: Type name simplified for better readability.Renaming from
KeyGentoKeygenfollows the same pattern as in the ECDSA implementation, promoting consistency across the codebase.
62-68: Method renaming for improved clarity.These changes are consistent with the ECDSA implementation, ensuring a uniform interface across all key generation implementations.
70-85: Enhanced error messaging with contextual information.Using
errors.Wrapinstead of standard error creation improves error context, aligned with the approach in the ECDSA implementation.
87-106: Variable declarations grouped for better readability and improved error handling.The grouped variable declarations and enhanced error handling for ID map setup mirror the improvements in the ECDSA implementation, ensuring consistency across the codebase.
138-142: Enhanced error handling for process key gen failure.Using
errors.Wrapprovides better context about the specific error that occurred during key generation processing, consistent with the ECDSA implementation.
157-254: Consistent improvements in processKeyGen method.The method has been updated to use the new struct name, enhanced error handling, and improved logging, all consistent with the changes in the ECDSA implementation.
common/tss.go (12)
127-133: Good code organization with var blockThe restructuring of variable declarations into a var block improves readability and follows Go best practices.
137-138: Improved error message for better debuggingThe error message now clearly indicates what operation failed rather than using a generic "broken tss share" message.
143-148: Better error handling flowThe error handling has been improved with a clearer variable name and more descriptive error message.
214-218: Improved variable naming consistencyChanging
pubkeystopubKeysaligns with Go naming conventions and maintains consistency throughout the codebase.
239-240: Better error context with errors.WrapUsing
errors.Wrapproperly preserves the error stack trace and adds helpful context for debugging.
365-374: Consistent error handling with switch statementThe switch statement pattern for message type handling provides a cleaner and more maintainable approach compared to nested if statements.
411-429: Improved nil handlingThe code now properly checks if
msg.Msgis nil before attempting to use it, which prevents potential nil pointer dereferences.
519-528: Enhanced peer management with error checkingThe improved peer ID handling includes proper error checking and a more readable implementation.
615-637: Cleaner control flow with switch statementUsing a switch statement with case conditions based on error types makes the code more readable and maintainable than nested if statements.
775-785: Improved variable organizationUsing a var block to group related variable declarations improves code readability and organization.
796-800: Appropriate handling of empty peer listThe code now correctly handles the case when there are no peers to broadcast to, which could happen during E2E testing with only 2 validators.
770-774: Better error handling with contextThe improved error message provides more context about what operation failed, making debugging easier.
fmt.Errorf()witherrors.Wrap()Closes #49
Closes #42
Summary by CodeRabbit
Refactor
Bug Fixes
Chore