fix infinite discovery address leak #37
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant PeerDiscovery
participant Multiaddr
participant KnownPeers
PeerDiscovery->>Multiaddr: Check if address exists
Multiaddr-->>PeerDiscovery: Return result
alt Address does not exist
PeerDiscovery->>KnownPeers: Add address to known peers
else Address exists
PeerDiscovery->>KnownPeers: Skip adding address
end
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-lintlevel=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: 2
🧹 Outside diff range and nitpick comments (2)
p2p/discovery_test.go (1)
86-90: Consider strengthening the test assertionsWhile the new assertions help catch the infinite address leak, there are opportunities to make the test more robust:
- The test relies on a 5-second sleep which could be flaky. Consider implementing a retry mechanism with timeout.
- The assertion only verifies the count of addresses but not their uniqueness or validity.
Here's a suggested improvement:
- time.Sleep(5 * time.Second) + // Helper function to wait for discovery with timeout + func waitForDiscovery(t *testing.T, comm *Communication) { + start := time.Now() + timeout := 10 * time.Second + for time.Since(start) < timeout { + if len(comm.discovery.knownPeers) == 3 { + return + } + time.Sleep(100 * time.Millisecond) + } + t.Fatal("Discovery timeout") + } + + waitForDiscovery(t, comm) assert.Equal(t, 4, len(comm.host.Peerstore().Peers())) assert.Equal(t, 4, len(comm2.host.Peerstore().Peers())) assert.Equal(t, 4, len(comm3.host.Peerstore().Peers())) assert.Equal(t, 4, len(comm4.host.Peerstore().Peers())) assert.Equal(t, 3, len(comm.discovery.knownPeers)) for peer, knownPeers := range comm.discovery.knownPeers { - assert.LessOrEqual(t, len(knownPeers.Addrs), 4, "%s has more than 4 addresses (%d)?", peer.String(), len(knownPeers.Addrs)) + addrs := knownPeers.Addrs + assert.LessOrEqual(t, len(addrs), 4, "%s has more than 4 addresses (%d)?", peer.String(), len(addrs)) + // Verify address uniqueness + seen := make(map[string]bool) + for _, addr := range addrs { + addrStr := addr.String() + assert.False(t, seen[addrStr], "Duplicate address found: %s", addrStr) + seen[addrStr] = true + } }p2p/discovery.go (1)
Line range hint
1-215: Consider implementing address pruning mechanismTo further prevent address accumulation over time, consider implementing a mechanism to periodically prune old or unreachable addresses. This would add an extra layer of protection against potential memory leaks.
Key considerations for implementation:
- Track address last-seen timestamps
- Periodically remove addresses that haven't been seen/used
- Remove addresses that consistently fail connection attempts
In the zetaclient logs, you'll see spam of thousands of addresses and OOM crashes
Add unit test to detect this scenario.
masterfails like this:slices.Compareapparently doesn't actually work onmultiaddr.Multiaddrapparently. So the addresses are appended until the process runs out of memory.Summary by CodeRabbit
Bug Fixes
Tests
TestDiscoveryfunction with additional assertions to verify the state of known peers and enforce limits on the number of addresses per peer.