feat(p2p): PeerDiscovery: streamline code, improve test cases & logging #39
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant modifications across several files, primarily enhancing the configurability and error handling of the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@swift1337 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[config_reader] The configuration option 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (13)
p2p/discovery_test.go (4)
23-26: Consider reducing gossip interval and extracting magic numberWhile the setup is clean, consider these improvements:
- The 500ms interval might slow down tests unnecessarily
- The magic number should be extracted as a constant
+const testGossipInterval = 100 * time.Millisecond + opts := []CommOpt{ // fast test cases - WithGossipInterval(500 * time.Millisecond), WithLogger(logger), + WithGossipInterval(testGossipInterval), WithLogger(logger), }
36-45: Consider extracting test fixturesThe hard-coded bootstrap peer credentials should be moved to test fixtures for better maintainability.
Consider creating a test fixtures file:
// test_fixtures.go var ( TestBootstrapPrivKey = "6LABmWB4iXqkqOJ9H0YFEA2CSSx6bA7XAKGyI/TDtas=" TestBootstrapPeer = "/ip4/127.0.0.1/tcp/2220/p2p/16Uiu2HAm4TmEzUqy3q3Dv7HvdoSboHk5sFj2FH3npiN5vDbJC6gh" )
64-87: Consider reducing duplication with helper functionThe peer startup code follows a repetitive pattern that could be extracted into a helper function.
func startTestPeer(t *testing.T, bootstrapAddrs []maddr.Multiaddr, port int, peer testPeer, opts ...CommOpt) *Communication { t.Helper() comm, err := NewCommunication(bootstrapAddrs, port, externalIP, whitelistedPeers, opts...) require.NoError(t, err) require.NoError(t, comm.Start(peer.skBytes)) t.Cleanup(func() { require.NoError(t, comm.Stop()) }) return comm }
91-97: Extract test timing configurationsThe timeout and polling interval should be configurable constants.
+const ( + testTimeout = 10 * time.Second + testPollInterval = 500 * time.Millisecond +) + eventually := func(c *Communication, name string) { // ... - assert.EventuallyWithT(t, check, 10*time.Second, 500*time.Millisecond, name) + assert.EventuallyWithT(t, check, testTimeout, testPollInterval, name) }tss/tss.go (2)
50-61: LGTM: Clean options pattern implementation.The functional options pattern is well-implemented, making the API extensible while maintaining backward compatibility.
Consider adding documentation about default values for the options struct fields.
74-80: Consider grouping required parameters into a config struct.The constructor has 9 required parameters before the variadic options, which could impact maintainability.
Consider grouping the required parameters into a configuration struct:
+type TssConfig struct { + BootstrapPeers []maddr.Multiaddr + P2PPort int + PrivateKey tcrypto.PrivKey + BaseFolder string + Config common.TssConfig + PreParams *bkeygen.LocalPreParams + ExternalIP string + Password string + WhitelistedPeers []peer.ID +} -func NewTss(cmdBootstrapPeers []maddr.Multiaddr, p2pPort int, priKey tcrypto.PrivKey, - baseFolder string, conf common.TssConfig, preParams *bkeygen.LocalPreParams, - externalIP string, tssPassword string, whitelistedPeers []peer.ID, opts ...Opt, -) (*TssServer, error) { +func NewTss(config TssConfig, opts ...Opt) (*TssServer, error) {p2p/communication.go (3)
61-63: Add documentation for the gossipInterval fieldThe
gossipIntervalfield lacks documentation explaining its purpose and acceptable values. Consider adding a comment to improve code maintainability.+ // gossipInterval determines the frequency at which peer information is shared + // across the network. Must be a positive duration. gossipInterval time.Duration
Line range hint
372-384: Improve error handling for bootstrap peer conversionThe current implementation silently skips invalid bootstrap peers, which could lead to runtime issues if too many peers are invalid. Consider:
- Collecting and reporting all conversion errors
- Validating the minimum required number of valid bootstrap peers
bootstrapPeerAddrInfos := make([]peer.AddrInfo, 0, len(c.bootstrapPeers)) + var conversionErrors []error for _, addr := range c.bootstrapPeers { peerInfo, err := peer.AddrInfoFromP2pAddr(addr) if err != nil { - c.logger.Error().Err(err).Msgf("fail to convert multiaddr to peer info: %s", addr) - continue + conversionErrors = append(conversionErrors, fmt.Errorf("invalid peer %s: %w", addr, err)) + continue } bootstrapPeerAddrInfos = append(bootstrapPeerAddrInfos, *peerInfo) } + + if len(conversionErrors) > 0 { + c.logger.Error().Msgf("failed to convert %d bootstrap peers: %v", len(conversionErrors), conversionErrors) + } + + if len(bootstrapPeerAddrInfos) == 0 && len(c.bootstrapPeers) > 0 { + return fmt.Errorf("all bootstrap peers are invalid") + } c.discovery = NewPeerDiscovery(c.host, bootstrapPeerAddrInfos, c.gossipInterval, c.logger) c.discovery.Start(context.Background())
383-384: Consider using a cancellable context for PeerDiscoveryUsing
context.Background()makes it harder to properly clean up resources. Consider passing a context that can be cancelled when the Communication instance is stopped.- c.discovery = NewPeerDiscovery(c.host, bootstrapPeerAddrInfos, c.gossipInterval, c.logger) - c.discovery.Start(context.Background()) + ctx, cancel := context.WithCancel(context.Background()) + c.discoveryCancel = cancel + c.discovery = NewPeerDiscovery(c.host, bootstrapPeerAddrInfos, c.gossipInterval, c.logger) + c.discovery.Start(ctx)This would require adding a
discoveryCancelfield to the Communication struct and calling it in theStopmethod.p2p/discovery.go (4)
90-115: Optimize locking in 'ensurePeer' to reduce contentionThe
ensurePeermethod holds a write lock for its entire duration, which could become a bottleneck under high concurrency. Refactoring to minimize lock scope can improve performance.Consider the following adjustments:
- Acquire the lock only when modifying shared resources.
- Use a read lock initially to check if the peer exists.
- Upgrade to a write lock only if an update is necessary.
137-142: Include local peer ID in logging context for better traceabilityAdding the local peer ID to the logging context enhances traceability and aids in debugging distributed systems.
lf := map[string]any{ "stream.remote_peer_id": s.Conn().RemotePeer().String(), + "stream.local_peer_id": pd.host.ID().String(), // ... }
208-209: Parameterize 'maxGossipConcurrency' for flexibilityThe hardcoded concurrency limit may not suit all deployment environments. Allowing this value to be configurable enhances scalability.
Introduce a configuration option or adjust the limit dynamically based on the number of peers:
func NewPeerDiscovery( /* existing params */, maxConcurrency int) *PeerDiscovery { if maxConcurrency <= 0 { maxConcurrency = DefaultMaxGossipConcurrency } // ... }
231-235: Avoid named return variables if not necessary to prevent confusionUsing named return variables like
errcan lead to shadowing and unintended side effects, especially in longer functions.Remove the named return variable for clarity:
-func (pd *PeerDiscovery) gossipPeer(ctx context.Context, p peer.AddrInfo) (err error) { +func (pd *PeerDiscovery) gossipPeer(ctx context.Context, p peer.AddrInfo) error { if err := pd.host.Connect(ctx, p); err != nil { return errors.Wrap(err, "failed to connect to peer") } // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
p2p/communication.go(8 hunks)p2p/discovery.go(4 hunks)p2p/discovery_test.go(1 hunks)tss/tss.go(6 hunks)
🔇 Additional comments (9)
p2p/discovery_test.go (3)
12-12: LGTM: Clean logger setup
The test logger setup using zerolog.NewTestWriter is appropriate for test scenarios and follows best practices.
Also applies to: 14-14, 20-20
28-30: LGTM: Excellent cleanup pattern
The use of t.Cleanup with require.NoError ensures proper resource cleanup and error handling.
112-134: LGTM: Well-structured test helper
The testPeer implementation is clean, well-documented, and follows testing best practices with proper use of t.Helper().
tss/tss.go (3)
124-131: Verify the impact of default gossip interval.
The code uses p2p.DefaultGossipInterval, which could affect network performance and resource usage.
Let's check for other gossip interval configurations:
✅ Verification successful
Default gossip interval configuration is properly implemented and used consistently
The codebase shows a well-structured implementation of gossip interval configuration:
- Default value of 30 seconds is defined as
DefaultGossipIntervalinp2p/discovery.go - The value is properly propagated through the communication stack:
- Used as default in
communicationConfig - Passed down to
PeerDiscovery - Fallback logic exists in
PeerDiscoverywhen zero value is provided
- Used as default in
- Test code appropriately overrides the interval for faster testing (500ms)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gossip interval configurations
rg -A 2 "GossipInterval|gossipInterval" --type go
Length of output: 1922
258-268: Verify peer ID conversion implementation.
The peer ID conversion looks correct, but let's ensure the implementation is robust.
Let's check the conversion implementation:
✅ Verification successful
Peer ID conversion implementation is robust and secure
The implementation demonstrates proper handling of peer ID conversion with:
- Comprehensive error handling at each conversion step
- Secure use of standard crypto libraries (libp2p, cosmos-sdk)
- Clear validation chain from bech32 format through secp256k1 public key to peer.ID
- Proper memory management with no unsafe operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GetPeerIDsFromPubKeys implementation
ast-grep --pattern 'func GetPeerIDsFromPubKeys'
Length of output: 49
Script:
#!/bin/bash
# Let's try searching for the file containing the conversion package first
fd conversion
# Then let's search for GetPeerIDsFromPubKeys using ripgrep for a broader search
rg "GetPeerIDsFromPubKeys" -A 5
Length of output: 1186
Script:
#!/bin/bash
# Let's examine the implementation of both conversion functions
cat conversion/key_provider.go
Length of output: 4026
238-252: LGTM: Improved version handling and error cases.
The code properly handles version checking and provides clear error messages.
Let's verify the version constant usage:
✅ Verification successful
Let me verify the version constant usage further by checking for any version comparison utilities:
Let me check the version comparison implementation and its usage:
Version handling implementation is robust and correctly used
The version comparison implementation and usage is well-structured and secure:
- Uses semantic versioning through
github.com/blang/semverfor reliable version comparisons VersionLTCheckproperly validates both current and expected versions- The constant
NEWJOINPARTYVERSION = "0.14.0"is consistently used across the codebase - Version checks are performed early in the execution path with clear error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NEWJOINPARTYVERSION usage
rg -A 2 "NEWJOINPARTYVERSION" --type go
Length of output: 854
Script:
#!/bin/bash
# Search for VersionLTCheck usage and implementation
rg -A 3 "VersionLTCheck" --type go
# Search for version-related utilities in conversion package
fd "conversion" --type f --exec cat {}
Length of output: 16479
Script:
#!/bin/bash
# Search for semver imports and usage
rg -A 3 "semver" --type go
# Search for tss_helper.go to see the full version comparison implementation
fd "tss_helper.go" --type f --exec cat {}
Length of output: 10562
p2p/communication.go (1)
236-238: LGTM: Appropriate log level adjustment
The change from error to warn level is appropriate as missing bootstrap nodes might be an intentional configuration.
p2p/discovery.go (2)
34-35: 🛠️ Refactor suggestion
Encapsulate 'gossipInterval' to prevent external modification
The gossipInterval field is currently exported, allowing external packages to modify it directly. To enhance encapsulation and maintain thread-safety, consider making gossipInterval unexported.
-type PeerDiscovery struct {
+type PeerDiscovery struct {
// ...
- gossipInterval time.Duration
+ gossipInterval time.Duration // unexported to prevent external modification
// ...
}Likely invalid or redundant comment.
127-129: Verify deterministic sorting of peers by ID
The sorting function uses < on peer IDs, which are strings. Ensure that the comparison provides a consistent order since string comparisons may not suffice for complex ID types.
Run the following script to confirm that sorting peer IDs yields a deterministic order:
✅ Verification successful
The initial test shows string sorting works as expected, but we need to verify the actual type implementation in the codebase to ensure the comparison is appropriate for the ID type being used.
We need to understand how the Peer ID is defined and used specifically in the discovery context to verify the sorting implementation.
We need to understand the host implementation and its ID type to verify if string comparison is appropriate for the peer IDs.
Peer ID sorting implementation is correct and deterministic
The implementation uses peer.ID from the github.com/libp2p/go-libp2p/core/peer package, which is a well-established type in the libp2p ecosystem. The string comparison (<) operation is appropriate here as:
- The peer IDs are handled by the libp2p framework, ensuring consistent formatting
- The implementation in
PeerDiscoverystruct uses the standardpeer.IDtype - The sorting is used purely for response determinism, not for cryptographic or security purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent sorting of peer IDs.
# Simulate peer IDs for testing
peer_ids=("peer3" "peer1" "peer2")
sorted_ids=($(printf '%s\n' "${peer_ids[@]}" | sort))
echo "Sorted Peer IDs:"
printf '%s\n' "${sorted_ids[@]}"
Length of output: 230
Script:
#!/bin/bash
# Let's check the actual type of peer ID in the codebase
rg -B2 -A2 "type Peer struct"
# Also check if there are any custom ID type definitions
rg "type.*ID"
# Look for any custom string conversion or comparison methods for peer IDs
ast-grep --pattern 'func ($id ID) String() string { $$$ }'
Length of output: 433
Script:
#!/bin/bash
# Let's examine the Peer struct and ID field usage in discovery.go
rg -B5 -A5 "peers\[.*\].ID" p2p/discovery.go
# Check for any peer ID related imports
rg "^import.*peer" p2p/discovery.go
# Look for any ID type declarations or usage in discovery.go
rg "ID.*=|:=.*ID" p2p/discovery.go
Length of output: 657
Script:
#!/bin/bash
# Check the host interface/type definition to understand the ID type
rg -B2 -A2 "type.*Host.*interface"
# Look for the actual host implementation
ast-grep --pattern 'type $_ struct {
$$$
host $_
$$$
}'
# Check imports in discovery.go for any peer ID related packages
rg "^import \(" -A 10 p2p/discovery.go
Length of output: 2925
| // Start periodic gossip | ||
| go pd.startGossip(ctx) | ||
| go pd.gossipWorker(ctx) |
There was a problem hiding this comment.
Check for context cancellation in 'gossipWorker' goroutine
The gossipWorker method starts a goroutine but doesn't handle context cancellation properly, which might result in goroutines not terminating when the context is done.
Ensure that the goroutine listens for ctx.Done() to handle cancellations:
-go pd.gossipWorker(ctx)
+go func() {
+ pd.gossipWorker(ctx)
+}()And within gossipWorker, regularly check ctx.Done() in your loops to exit when canceled.
Committable suggestion skipped: line range outside the PR's diff.
|
I wouldn't spend too much time on this as we're leaning towards disabling/removing the discovery logic anyway |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
p2p/discovery.go (2)
20-28: Document constants with explanatory commentsAdd documentation comments to explain the purpose and reasoning behind the constant values, especially for
maxGossipConcurrencyandmaxGossipTimeout.const ( + // DiscoveryProtocol defines the protocol version for peer discovery DiscoveryProtocol = "/tss/discovery/1.0.0" + // DefaultGossipInterval defines how often peers exchange their known peers DefaultGossipInterval = 30 * time.Second ) const ( + // maxGossipConcurrency limits concurrent gossip operations to prevent resource exhaustion maxGossipConcurrency = 50 + // maxGossipTimeout defines the maximum time allowed for a single gossip operation maxGossipTimeout = 10 * time.Second )
118-129: Optimize slice allocation in getKnownPeersPre-allocate the slice capacity to avoid potential reallocation during append operations.
func (pd *PeerDiscovery) getKnownPeers() []peer.AddrInfo { pd.mu.RLock() defer pd.mu.RUnlock() peers := make([]peer.AddrInfo, 0, len(pd.knownPeers)) for _, p := range pd.knownPeers { peers = append(peers, p) } // sort peers by id (in place) to make response deterministic sort.Slice(peers, func(a, b int) bool { return peers[a].ID < peers[b].ID }) return peers }tss/tss.go (2)
250-256: Enhance error messages for better debuggingConsider improving error messages to include more context:
switch { case err != nil: - return nil, "", fmt.Errorf("fail to parse the version with error: %w", err) + return nil, "", fmt.Errorf("failed to parse version %q: %w", version, err) case oldJoinParty: - return nil, "", fmt.Errorf("only %q party version is supported", messages.NEWJOINPARTYVERSION) + return nil, "", fmt.Errorf("version %q is not supported, minimum required version is %q", version, messages.NEWJOINPARTYVERSION)
262-264: Improve error handling and logging contextThe error handling and logging can be enhanced:
-t.logger.Info().Msg("we apply the join party with a leader") +t.logger.Info(). + Int("participant_count", len(participants)). + Int("threshold", threshold). + Str("msg_id", msgID). + Msg("applying join party with leader") peersID, err := conversion.GetPeerIDsFromPubKeys(participants) if err != nil { - return nil, "", errors.New("fail to convert the public key to peer ID") + return nil, "", fmt.Errorf("failed to convert public keys to peer IDs: %w", err) }Also applies to: 260-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
p2p/discovery.go(4 hunks)tss/tss.go(6 hunks)
🔇 Additional comments (2)
tss/tss.go (2)
51-62: Well-structured options pattern implementation!
The implementation follows the functional options pattern effectively, providing a clean and backward-compatible way to configure the TssServer.
128-135: Verify DefaultGossipInterval constant
The code uses p2p.DefaultGossipInterval. Let's ensure this constant is properly defined.
✅ Verification successful
DefaultGossipInterval constant is properly defined
The constant is correctly defined in p2p/discovery.go with a value of 30 seconds, making it suitable for the p2p.Communication initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DefaultGossipInterval definition
rg -A 1 'DefaultGossipInterval.*='
Length of output: 114
| return errors.Wrap(err, "failed to read from stream") | ||
| } | ||
|
|
||
| pd.logger.Debug().Bytes("gossip.response", buf).Msg("Received peer data") |
There was a problem hiding this comment.
Remove sensitive data from logs
Logging raw response bytes could expose sensitive information. Consider logging only metadata.
- pd.logger.Debug().Bytes("gossip.response", buf).Msg("Received peer data")
+ pd.logger.Debug().Int("gossip.response_size", len(buf)).Msg("Received peer data")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pd.logger.Debug().Bytes("gossip.response", buf).Msg("Received peer data") | |
| pd.logger.Debug().Int("gossip.response_size", len(buf)).Msg("Received peer data") |
| pd.logger.Debug(). | ||
| Stringer("gossip.received_peer_id", p.ID). | ||
| Interface("gossip.received_peer_address", p.Addrs). | ||
| Msg("Ensuring peer") |
There was a problem hiding this comment.
Fix incorrect peer reference in logs
The logging statement uses p instead of remotePeer, which logs incorrect peer information.
pd.logger.Debug().
- Stringer("gossip.received_peer_id", p.ID).
- Interface("gossip.received_peer_address", p.Addrs).
+ Stringer("gossip.received_peer_id", remotePeer.ID).
+ Interface("gossip.received_peer_address", remotePeer.Addrs).
Msg("Ensuring peer")Committable suggestion skipped: line range outside the PR's diff.
| limitedReader := io.LimitReader(s, 1<<20) // Limit to 1MB | ||
| buf, err := io.ReadAll(limitedReader) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to read from stream") | ||
| } |
There was a problem hiding this comment.
Add size validation for received peer data
While there's a size limit on the reader, there should be a check to ensure we received some data.
limitedReader := io.LimitReader(s, 1<<20) // Limit to 1MB
buf, err := io.ReadAll(limitedReader)
if err != nil {
return errors.Wrap(err, "failed to read from stream")
}
+ if len(buf) == 0 {
+ return errors.New("received empty peer data")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| limitedReader := io.LimitReader(s, 1<<20) // Limit to 1MB | |
| buf, err := io.ReadAll(limitedReader) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to read from stream") | |
| } | |
| limitedReader := io.LimitReader(s, 1<<20) // Limit to 1MB | |
| buf, err := io.ReadAll(limitedReader) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to read from stream") | |
| } | |
| if len(buf) == 0 { | |
| return errors.New("received empty peer data") | |
| } |
p2p/discovery.go
Outdated
| errg.Go(func() error { | ||
| if err := pd.gossipPeer(ctx, remotePeer); err != nil { | ||
| pd.logger.Error().Err(err).Msg("Failed to gossip peer") | ||
| } | ||
| for _, p := range recvPeers { | ||
| pd.logger.Debug(). | ||
| Stringer("peer", p). | ||
| Msg("Adding peer") | ||
| pd.addPeer(p) | ||
| } | ||
| }(p) | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in gossip operations
The current implementation swallows errors from individual gossip operations. Consider aggregating errors for better debugging.
errg.Go(func() error {
if err := pd.gossipPeer(ctx, remotePeer); err != nil {
pd.logger.Error().Err(err).Msg("Failed to gossip peer")
+ return errors.Wrapf(err, "failed to gossip with peer %s", remotePeer.ID)
}
-
- return nil
+ return nil
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| errg.Go(func() error { | |
| if err := pd.gossipPeer(ctx, remotePeer); err != nil { | |
| pd.logger.Error().Err(err).Msg("Failed to gossip peer") | |
| } | |
| for _, p := range recvPeers { | |
| pd.logger.Debug(). | |
| Stringer("peer", p). | |
| Msg("Adding peer") | |
| pd.addPeer(p) | |
| } | |
| }(p) | |
| return nil | |
| }) | |
| errg.Go(func() error { | |
| if err := pd.gossipPeer(ctx, remotePeer); err != nil { | |
| pd.logger.Error().Err(err).Msg("Failed to gossip peer") | |
| return errors.Wrapf(err, "failed to gossip with peer %s", remotePeer.ID) | |
| } | |
| return nil | |
| }) |
| func NewPeerDiscovery( | ||
| h host.Host, | ||
| bootstrapPeers []peer.AddrInfo, | ||
| gossipInterval time.Duration, | ||
| logger zerolog.Logger, | ||
| ) *PeerDiscovery { | ||
| if gossipInterval == 0 { | ||
| gossipInterval = DefaultGossipInterval | ||
| } | ||
|
|
||
| pd := &PeerDiscovery{ | ||
| host: h, | ||
| knownPeers: make(map[peer.ID]peer.AddrInfo), | ||
| bootstrapPeers: bootstrapPeers, | ||
| logger: log.With().Str("module", "peer-discovery").Logger(), | ||
| closeChan: make(chan struct{}), | ||
| gossipInterval: gossipInterval, | ||
| logger: logger.With().Str("module", "peer_discovery").Logger(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation in constructor
The constructor should validate the host parameter and ensure logger is not nil.
func NewPeerDiscovery(
h host.Host,
bootstrapPeers []peer.AddrInfo,
gossipInterval time.Duration,
logger zerolog.Logger,
) *PeerDiscovery {
+ if h == nil {
+ panic("host cannot be nil")
+ }
+
if gossipInterval == 0 {
gossipInterval = DefaultGossipInterval
}
+
+ if logger == (zerolog.Logger{}) {
+ logger = zerolog.New(io.Discard)
+ }
pd := &PeerDiscovery{Committable suggestion skipped: line range outside the PR's diff.
| tssOptions := options{ | ||
| logger: log.Logger, | ||
| } | ||
|
|
||
| for _, fn := range opts { | ||
| fn(&tssOptions) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logger initialization robustness
While the default logger is set, consider adding validation to ensure the logger is properly configured:
tssOptions := options{
- logger: log.Logger,
+ logger: log.Logger.With().Str("component", "tss").Logger(),
}
for _, fn := range opts {
fn(&tssOptions)
}
+if !tssOptions.logger.GetLevel().Enabled(zerolog.InfoLevel) {
+ tssOptions.logger = tssOptions.logger.Level(zerolog.InfoLevel)
+}Committable suggestion skipped: line range outside the PR's diff.
In the process of exploring internal of
go-tssI found several areas of improvements regarding peer discoveryChanges
stream.Close()With*peer-discovery->peer_discoverycommunication->peer_communicationSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores