Remove all dynamic peer discovery #40
Conversation
|
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 📝 WalkthroughWalkthroughThe pull request introduces significant changes across several files, primarily focusing on the implementation of a new function, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gartnera
force-pushed
the
remove-dynamic-peer-discovery
branch
2 times, most recently
from
01805c5 to
8e95a62
Compare
gartnera
force-pushed
the
remove-dynamic-peer-discovery
branch
from
8e95a62 to
f486d1f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
conversion/tss_helper.go (1)
61-76: Add input validation for empty slices and length mismatch.The function should validate that both input slices are non-empty and have matching lengths before processing.
func TestBootstrapAddrs(ports []int, testPubKeys []string) ([]maddr.Multiaddr, error) { + if len(ports) == 0 || len(testPubKeys) == 0 { + return nil, fmt.Errorf("empty input: ports=%d, pubkeys=%d", len(ports), len(testPubKeys)) + } + if len(ports) != len(testPubKeys) { + return nil, fmt.Errorf("length mismatch: ports=%d, pubkeys=%d", len(ports), len(testPubKeys)) + } res := make([]maddr.Multiaddr, len(ports)) for i := 0; i < len(ports); i++ {tss/tss_4nodes_zeta_test.go (1)
224-240: Consider adding context to error returns in getTssServer.The error handling could be more informative by wrapping errors with context.
func (s *FourNodeScaleZetaSuite) getTssServer(c *C, index int, conf common.TssConfig) *TssServer { priKey, err := conversion.GetPriKey(testPriKeyArr[index]) - c.Assert(err, IsNil) + c.Assert(err, IsNil, Commentf("failed to get private key for index %d", index)) baseHome := path.Join(s.tmpDir, strconv.Itoa(index)) if _, err := os.Stat(baseHome); os.IsNotExist(err) { err := os.MkdirAll(baseHome, os.ModePerm) - c.Assert(err, IsNil) + c.Assert(err, IsNil, Commentf("failed to create directory %s", baseHome)) }keygen/eddsa/keygen_test.go (1)
117-120: Consider extracting common communication setup logic.The communication setup logic is repeated for each node. Consider extracting it into a helper method to improve maintainability.
+func (s *EddsaKeygenTestSuite) setupCommunication(i int, ports []int, bootstrapPeers []maddr.Multiaddr, whitelistedPeers []peer.ID, buf []byte) (*p2p.Communication, error) { + comm, err := p2p.NewCommunication(bootstrapPeers, ports[i], "", whitelistedPeers) + if err != nil { + return nil, fmt.Errorf("failed to create communication for node %d: %w", i, err) + } + if err := comm.Start(buf); err != nil { + return nil, fmt.Errorf("failed to start communication for node %d: %w", i, err) + } + return comm, nil +}Then use it in the setup:
- comm, err := p2p.NewCommunication(bootstrapPeers, ports[i], "", whitelistedPeers) - c.Assert(err, IsNil) - c.Assert(comm.Start(buf), IsNil) - s.comms[i] = comm + comm, err := s.setupCommunication(i, ports, bootstrapPeers, whitelistedPeers, buf) + c.Assert(err, IsNil) + s.comms[i] = commtss/tss_4nodes_test.go (1)
98-100: Simplify server initialization logicThe server initialization and restart logic could be further simplified by removing the redundant conditional blocks since they perform identical operations.
Consider applying this refactor:
-if idx == 0 { - s.servers[idx] = s.getTssServer(c, idx, s.tssConfig) -} else { - s.servers[idx] = s.getTssServer(c, idx, s.tssConfig) -} +s.servers[idx] = s.getTssServer(c, idx, s.tssConfig)And similarly for the restart logic:
-if shutdownIdx == 0 { - s.servers[shutdownIdx] = s.getTssServer(c, shutdownIdx, s.tssConfig) -} else { - s.servers[shutdownIdx] = s.getTssServer(c, shutdownIdx, s.tssConfig) -} +s.servers[shutdownIdx] = s.getTssServer(c, shutdownIdx, s.tssConfig)Also applies to: 320-322
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
conversion/tss_helper.go(2 hunks)keygen/ecdsa/keygen_test.go(2 hunks)keygen/eddsa/keygen_test.go(2 hunks)keysign/ecdsa/keysign_old_test.go(2 hunks)keysign/ecdsa/keysign_test.go(2 hunks)keysign/eddsa/keysign_test.go(2 hunks)p2p/communication.go(0 hunks)p2p/discovery.go(0 hunks)p2p/discovery_test.go(0 hunks)tss/tss_4nodes_test.go(4 hunks)tss/tss_4nodes_zeta_test.go(3 hunks)
💤 Files with no reviewable changes (3)
- p2p/discovery_test.go
- p2p/communication.go
- p2p/discovery.go
🧰 Additional context used
📓 Learnings (1)
tss/tss_4nodes_zeta_test.go (1)
Learnt from: gartnera
PR: zeta-chain/go-tss#22
File: tss/tss_4nodes_zeta_test.go:40-78
Timestamp: 2024-11-12T13:20:12.657Z
Learning: In the `SetUpSuite` function of `FourNodeScaleZetaSuite`, directories are cleaned up before the setup starts.
🔇 Additional comments (11)
tss/tss_4nodes_zeta_test.go (1)
28-34: LGTM! Clean struct definition with clear field purposes.
The updated struct fields properly reflect the removal of dynamic peer discovery.
keygen/eddsa/keygen_test.go (1)
105-106: LGTM! Proper error handling for bootstrap address generation.
The error check is appropriate and maintains test reliability.
keysign/eddsa/keysign_test.go (2)
142-143: LGTM: Proper initialization of bootstrap addresses
The implementation correctly generates and validates bootstrap addresses for the test setup.
154-154: LGTM: Appropriate communication setup
The communication initialization properly utilizes the bootstrap peers for node connectivity.
keygen/ecdsa/keygen_test.go (2)
119-120: LGTM: Consistent bootstrap address initialization
The implementation maintains consistency with other test files in generating bootstrap addresses.
131-131: LGTM: Proper communication setup
The communication initialization correctly uses the bootstrap peers.
tss/tss_4nodes_test.go (3)
64-68: LGTM: Improved bootstrap peer management
The change from a single bootstrap peer to a list of peers enhances the test suite's flexibility and reliability.
75-82: LGTM: Proper test setup initialization
The implementation correctly initializes the test environment with appropriate error handling.
373-373: LGTM: Proper TSS instance initialization
The NewTss call correctly utilizes the bootstrap peers list.
keysign/ecdsa/keysign_old_test.go (1)
Line range hint 126-144: LGTM: Bootstrap peer configuration properly implemented
The changes correctly implement the transition from dynamic peer discovery to bootstrap-based peer configuration, aligning with the PR objectives.
keysign/ecdsa/keysign_test.go (1)
Line range hint 145-157: LGTM: Consistent bootstrap peer implementation
The changes maintain consistency with other test files and properly implement the bootstrap-based peer configuration. The error handling is robust with appropriate assertions.
Remove all dynamic peer discovery logic and rely on precomputed bootstrap addresses.
See zeta-chain/node#3289
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores