feat: collect addresses from connected peers

[xdustinface]

We currently don't request any addresses from connected peers so we ever only end up with the 3 addresses we initially try to connect to from the DNS seed lookup. This PR fixes it by making sure we send a GetAddr after handshake and process the responses properly.

See individual commits and their messages.

Based on:

• fix: store all peers in storage #420

Working towards closing:

• fix: peer discovery and reputation usage #416

Summary by CodeRabbit

• Bug Fixes
  • Strengthened peer address discovery by implementing active address requests after connection
  • Enhanced address validation with timestamp window checks to prevent invalid entries
  • Optimized address storage with automatic eviction of outdated peer information
  • Added transparent conversion of legacy address formats

[coderabbitai]

📝 Walkthrough

Walkthrough

The address storage mechanism in the network module is refactored from a Vec to a HashMap, introducing timestamp validation and an eviction policy. Address discovery is enhanced through GetAddr requests, with legacy address formats converted and unified into the new storage system.

Changes

Cohort / File(s)	Summary
Address Storage Refactoring dash-spv/src/network/addrv2.rs	Storage changed from Vec<AddrV2Message> to HashMap<SocketAddr, AddrV2Message>. New evict_if_needed function maintains capacity with oldest-first eviction. Constants ONE_WEEK and TEN_MINUTES define timestamp validity windows. Updated initialization, timestamp validation in handle_addrv2, and entry deduplication logic. Methods get_addresses_for_peer and get_known_addresses now read from HashMap values. Enhanced logging tracks received, added, updated, and total peer counts.
Network Integration & Discovery dash-spv/src/network/manager.rs	Imports AddrV2 for address handling. Issues GetAddr request following successful peer connection for address discovery. Extends NetworkMessage::Addr handling to convert legacy address entries to AddrV2Message and route through addrv2_handler. Legacy messages received during peer startup are normalized to AddrV2 format before storage.

Sequence Diagram

sequenceDiagram
    participant Self as Local Node
    participant Peer as Peer Node
    participant Handler as AddrV2 Handler
    
    Self->>Peer: TCP Connect
    Peer-->>Self: Connection Established
    
    Self->>Peer: GetAddr Request
    Peer-->>Self: Addr Response (Legacy or V2)
    
    alt Legacy Format
        Self->>Self: Convert Addr to AddrV2
    end
    
    Self->>Handler: Validate Timestamps
    Handler->>Handler: Filter by ONE_WEEK & TEN_MINUTES
    
    Handler->>Handler: Insert/Update HashMap<SocketAddr, AddrV2>
    Handler->>Handler: Evict if exceeds MAX_ADDR_TO_STORE
    
    Handler-->>Self: Updated Peer List

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Addresses now rest in hashmaps so keen,
With timestamps checked—fresh within our scene,
Legacy formats are swiftly transformed,
Peer discovery flows, beautifully formed,
Eviction ensures our storage stays trim! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately captures the main objective of the changeset: collecting peer addresses from connected peers through GetAddr requests and proper handling of Addr/AddrV2 responses.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/peer-discovery

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

dash-spv/src/network/addrv2.rs (1)

206-234: ⚠️ Potential issue | 🟡 Minor

Test comments are misleading — the "too old" message is actually valid by timestamp rules.

A 4-hour-old timestamp is well within the 1-week window, so it passes timestamp validation. It only gets skipped because the same SocketAddr (127.0.0.1:9999) was already inserted with a fresher timestamp (now), so the dedup logic at Line 81 (existing.time >= msg.time) rejects it.

The test passes for the right reason (HashMap dedup), but the comments claim the timestamp itself is invalid. This will confuse future readers. Consider either:

1. Fixing the comments to explain the actual reason each message is accepted/rejected.
2. Using distinct addresses per message so the test truly exercises timestamp filtering in isolation.

Option 2: Use distinct addresses to properly test timestamp filtering

         let messages = vec![
             // Valid: current time
             AddrV2Message {
                 time: now,
                 services: ServiceFlags::from(1),
                 addr: AddrV2::Ipv4(ipv4_addr),
-                port: addr.port(),
+                port: 9999,
             },
-            // Invalid: too old (4 hours ago)
+            // Valid timestamp (4h ago, within 1 week) but uses distinct port
             AddrV2Message {
                 time: now.saturating_sub(14400),
                 services: ServiceFlags::from(1),
                 addr: AddrV2::Ipv4(ipv4_addr),
-                port: addr.port(),
+                port: 10000,
             },
-            // Invalid: too far in future (20 minutes)
+            // Invalid: too far in future (20 minutes), distinct port
             AddrV2Message {
                 time: now + 1200,
                 services: ServiceFlags::from(1),
                 addr: AddrV2::Ipv4(ipv4_addr),
-                port: addr.port(),
+                port: 10001,
             },
         ];
 
         handler.handle_addrv2(messages).await;
 
-        // Only the valid message should be stored
+        // Two valid messages stored (current + 4h old); the future one is rejected
         let known = handler.get_known_addresses().await;
-        assert_eq!(known.len(), 1);
+        assert_eq!(known.len(), 2);

🧹 Nitpick comments (1)

dash-spv/src/network/addrv2.rs (1)

70-70: now + TEN_MINUTES could theoretically overflow u32.

While not a practical concern until ~2106 (u32 epoch overflow), the code uses saturating_sub defensively for the lower bound but plain addition for the upper bound. For symmetry and defensive coding:

Proposed fix

-            if msg.time < now.saturating_sub(ONE_WEEK) || msg.time > now + TEN_MINUTES {
+            if msg.time < now.saturating_sub(ONE_WEEK) || msg.time > now.saturating_add(TEN_MINUTES) {