fix: allow parallel connection attempts (max 3 concurrent)

[sanity]

Problem

In small network tests, peers form a star topology (all connected only to gateway) instead of a mesh topology. The root cause is serialized connection acquisition in the maintenance loop.

Location: crates/core/src/ring/mod.rs

The code used a single live_tx: Option<Transaction> to track pending connections. This means only ONE connection attempt could proceed at a time. If a connection takes 60 seconds (OPERATION_TTL), ALL other attempts are blocked for 60 seconds.

With 25 min_connections target and serial acquisition, reaching minimum connections takes far longer than test windows allow.

Solution

Replace the single live_tx tracking with a concurrent limit using live_tx_tracker.active_transaction_count():

• Add MAX_CONCURRENT_CONNECTIONS = 3 constant
• Allow new connection attempts while active_count < MAX_CONCURRENT_CONNECTIONS
• Remove the now-unused still_alive method from LiveTransactionTracker
• Add active_transaction_count() method to count all active transactions

Testing

• All 9 connect unit tests pass
• Compile check passes with no errors

Stack

This PR stacks on #2172 (seeding/subscriber NAT routing).

Part of #2173 (Priority 1).

[AI-assisted - Claude]

[Copilot]

Pull request overview

This PR aims to enable parallel connection attempts (up to 3 concurrent) to improve network formation speed by replacing a single-transaction tracking mechanism with a concurrent limit approach. However, the implementation does not achieve the intended parallelism.

Key Changes:

• Added MAX_CONCURRENT_CONNECTIONS constant (set to 3) to limit concurrent connection attempts
• Replaced single live_tx: Option<Transaction> with active_transaction_count() method for tracking
• Removed unused still_alive() method from LiveTransactionTracker

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
crates/core/src/ring/mod.rs	Added concurrent connection limit constant and modified maintenance loop to check active transaction count instead of single transaction tracking
crates/core/src/ring/live_tx.rs	Removed still_alive() method and added active_transaction_count() to sum all active transactions

Comments suppressed due to low confidence (1)

crates/core/src/ring/mod.rs:486

• The loop only processes one location per iteration, which prevents achieving the desired parallelism. To actually run up to MAX_CONCURRENT_CONNECTIONS concurrent connections, the code should loop while active_count < MAX_CONCURRENT_CONNECTIONS and attempt to start multiple connections until the limit is reached or pending_conn_adds is empty.

Suggested fix:

// Acquire new connections up to MAX_CONCURRENT_CONNECTIONS limit
let mut active_count = live_tx_tracker.active_transaction_count();
while active_count < MAX_CONCURRENT_CONNECTIONS {
    if let Some(ideal_location) = pending_conn_adds.pop_first() {
        // ... existing connection attempt code ...
        if tx.is_some() {
            active_count += 1; // Update count for next iteration
        }
    } else {
        break; // No more pending connections
    }
}

            if let Some(ideal_location) = pending_conn_adds.pop_first() {
                if active_count < MAX_CONCURRENT_CONNECTIONS {
                    tracing::info!(
                        active_connections = active_count,
                        max_concurrent = MAX_CONCURRENT_CONNECTIONS,
                        "Attempting to acquire new connection for location: {:?}",
                        ideal_location
                    );
                    let tx = self
                        .acquire_new(
                            ideal_location,
                            &skip_list,
                            &notifier,
                            &live_tx_tracker,
                            &op_manager,
                        )
                        .await
                        .map_err(|error| {
                            tracing::error!(
                                ?error,
                                "FATAL: Connection maintenance task failed - shutting down"
                            );
                            error
                        })?;
                    if tx.is_none() {
                        let conns = self.connection_manager.connection_count();
                        tracing::warn!(
                            "acquire_new returned None - likely no peers to query through (connections: {})",
                            conns
                        );
                    } else {
                        tracing::info!(
                            active_connections = active_count + 1,
                            "Successfully initiated connection acquisition"
                        );
                    }
                } else {
                    tracing::debug!(
                        active_connections = active_count,
                        max_concurrent = MAX_CONCURRENT_CONNECTIONS,
                        "At max concurrent connections, re-queuing location {}",
                        ideal_location
                    );
                    pending_conn_adds.insert(ideal_location);
                }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The new active_transaction_count() method lacks test coverage. Consider adding unit tests to verify:

• Returns 0 when no transactions are tracked
• Correctly counts a single transaction for one peer
• Correctly sums transactions across multiple peers
• Updates correctly after transactions are removed via remove_finished_transaction()

Example test:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn active_transaction_count_empty() {
        let tracker = LiveTransactionTracker::new();
        assert_eq!(tracker.active_transaction_count(), 0);
    }

    #[test]
    fn active_transaction_count_multiple_peers() {
        let tracker = LiveTransactionTracker::new();
        let addr1 = "127.0.0.1:8080".parse().unwrap();
        let addr2 = "127.0.0.1:8081".parse().unwrap();
        
        tracker.add_transaction(addr1, Transaction::new::<()>());
        tracker.add_transaction(addr1, Transaction::new::<()>());
        tracker.add_transaction(addr2, Transaction::new::<()>());
        
        assert_eq!(tracker.active_transaction_count(), 3);
    }
}

[sanity]

Re: Copilot's suggestion for active_transaction_count() tests

Added comprehensive unit tests for active_transaction_count() in commit 864f72a:

• active_transaction_count_empty - verifies returns 0 when no transactions
• active_transaction_count_single_peer - verifies correct count for single peer with multiple transactions
• active_transaction_count_multiple_peers - verifies correct sum across multiple peers
• active_transaction_count_after_removal - verifies count updates correctly after remove_finished_transaction()

All 4 tests pass.

[AI-assisted - Claude]

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This behavioral change from the previous implementation removes protection against known-but-incorrect NAT addresses. The old code (visible in removed lines elsewhere in this diff) would always use the transport source address when available, specifically to handle cases where NAT peers embed known-but-wrong addresses (e.g., 127.0.0.1:31337 or internal network addresses like 192.168.x.x).

The new conditional logic only fills in addresses marked as PeerAddr::Unknown, meaning if a NAT peer mistakenly sends PeerAddr::Known(127.0.0.1:31337) instead of PeerAddr::Unknown, the loopback address will not be corrected.

While this change aligns subscribe.rs with the pattern used in connect.rs and put.rs, it removes a safety mechanism that was explicitly documented as "critical for NAT peers" in the old code comments. This could break existing clients behind NAT that haven't been updated to send PeerAddr::Unknown when they don't know their external address.

Consider either:

1. Reverting to unconditional address correction for backward compatibility, or
2. Adding validation to reject obviously incorrect addresses (loopback, private ranges) when received from remote peers, or
3. Documenting this as a breaking change requiring client updates

[sanity]

Re: Copilot's suggestion for active_transaction_count() tests

Already addressed - comprehensive unit tests for active_transaction_count() were added in commit 864f72a:

• active_transaction_count_empty - verifies returns 0 when no transactions
• active_transaction_count_single_peer - verifies correct count for single peer with multiple transactions
• active_transaction_count_multiple_peers - verifies correct sum across multiple peers

The test coverage Copilot suggested is already in place.

Re: NAT address handling concern

Copilot raises a valid concern about backward compatibility with clients that might send PeerAddr::Known(127.0.0.1:31337) instead of PeerAddr::Unknown.

However, this PR is part of a larger refactor (#2164) that introduces the explicit PeerAddr::Unknown type specifically to handle this cleanly. The design decision is:

1. Correct clients send PeerAddr::Unknown when behind NAT (they don't know their external address)
2. The first recipient fills in the address from the transport source
3. This is consistent across connect.rs, put.rs, and subscribe.rs

The "old code" that always overwrote addresses was a workaround for not having an explicit "unknown" state. With PeerAddr::Unknown, we no longer need that workaround.

If backward compatibility with very old clients is a concern, that would be a separate discussion - but for the core network protocol being refactored, using explicit state is the cleaner approach.

[AI-assisted - Claude]