net: prevent duplication manual connections (take 2) #35600

1. 
   willcl-ark commented at 11:37 AM on June 25, 2026: member

   Fixes #5299.

   -connect and -addnode can attempt the same manual connection concurrently during startup. The existing duplicate checks cannot see a peer until it has been added to m_nodes, so both attempts can pass and open connections.

   Reserve the destination in OpenNetworkConnection() before checking connected peers or starting the connection. The reservation stays in place until the attempt fails or the new peer has been added to m_nodes.

   This applies to ConnectionType::MANUAL, including -connect, -addnode, manual reconnections, and addnode onetry. Automatic outbound connections are outside this change.

   The functional test checks the original two-node startup case. It also holds a manual connection in a local proxy handshake and verifies that an overlapping attempt is rejected, that a failed attempt releases its reservation, and that equivalent numeric addresses share a reservation.

   Unlike #27804, this handles attempts in the connection manager rather than deduplicating configuration arguments. Hostnames use their original string as the reservation key, so different names that resolve to the same endpoint may still overlap. This avoids an extra DNS lookup.

2. DrahtBot added the label P2P on Jun 25, 2026
3. 
   DrahtBot commented at 11:37 AM on June 25, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35600
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35600.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35600#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   Concept ACK	sedited, w0xlt
   Stale ACK	8144225309

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32278 _{<img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/32278.svg">} (doc: better document NetEventsInterface and the deletion of "CNode"s by vasild)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. 
   sedited commented at 10:55 AM on July 24, 2026: contributor

   Concept ACK

5. DrahtBot added the label Needs rebase on Jul 24, 2026
6. willcl-ark force-pushed on Jul 25, 2026
7. DrahtBot removed the label Needs rebase on Jul 25, 2026
8. DrahtBot added the label CI failed on Jul 25, 2026
9. maflcko removed the label CI failed on Jul 30, 2026
10. 
    w0xlt commented at 8:24 PM on August 18, 2026: contributor

    Concept ACK

11. willcl-ark force-pushed on Sep 24, 2026
12. 
    8144225309 commented at 12:24 AM on October 4, 2026: contributor

    ACK 081ba2b98c

    Built and ran the new test, the functional suite and the net unit tests. The new test fails with the duplicate check disabled, and also when the key normalization or the release after a failed attempt is broken.

    The test still passes if the reservation covers every connection type (net.cpp:3139). An addconnection to the same destination while a manual attempt is in progress would catch that. Non-blocking.

13. DrahtBot requested review from sedited on Oct 4, 2026
14. net: reserve manual destinations before checking peers

    Concurrent -connect and -addnode attempts could pass connected-peer checks
    before either published a node. Reserve a normalized destination until the
    dial fails or its node enters m_nodes. Check connected peers while holding
    the reservation, so proxied hostnames cannot use a stale precheck.
    
    Keep the reservation scoped to this call and leave the addnode scheduler
    unchanged. Numeric destinations include their port; unresolved hostnames
    use the original string without an extra DNS lookup.

    3bab40f22f
15. test: verify overlapping manual connections are rejected

    The startup topology alone can pass without exercising pending-attempt
    exclusion. Hold one manual connection in a local proxy handshake and require a
    second attempt to be rejected while the first is pending.

    590947d783
16. in src/net.cpp:365 in 081ba2b98c

     360 | +{
     361 | +    const CService resolved{MaybeFlipIPv6toCJDNS(LookupNumeric(dest, GetDefaultPort(dest)))};
     362 | +    return resolved.IsValid() ? resolved.ToStringAddrPort() : dest;
     363 | +}
     364 | +
     365 | +bool CConnman::MarkManualConnectionInProgress(const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_mutex)
    

    ---

    ---

    

    maflcko commented at 8:56 AM on October 4, 2026:

    ralph says to drop them, which seems right? https://git.fish.foo/bitcoin/bitcoin/pulls/35600#author-ralph

    ---

    

    willcl-ark commented at 8:12 AM on October 5, 2026:

    Good bot. Taken.

17. willcl-ark force-pushed on Oct 5, 2026

Contributors

willcl-arkDrahtBotseditedw0xlt8144225309maflcko

Review Requested
sedited