init: ignore repeated `-addnode` startup values #36014

w0xlt commented at 8:28 PM on August 18, 2026: contributor

Startup -addnode values are copied directly into the connection manager's added-node list. Unlike runtime addnode add calls, this path does not reject repeated values.

For example:

-addnode=example.com
-addnode=example.com

Both entries are currently stored. While the destination is disconnected, ThreadOpenAddedConnections() processes each entry during every retry cycle.

This can cause redundant DNS lookups, connection attempts, and log messages. getaddednodeinfo also reports the repeated entry.

This change is complementary to, and independent of, #35600.

#35600 prevents overlapping manual connection attempts by tracking destinations while a connection attempt is in progress. This PR instead removes repeated -addnode values before the connection threads start, preventing duplicate stored entries and sequential redundant retries.

It does not replace or broaden #35600's in-flight connection handling.

DrahtBot commented at 8:28 PM on August 18, 2026: contributor

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

External sites

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

Reviews

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

Type	Reviewers
Concept ACK	danielabrozzoni, RandyMcMillan
Stale ACK	pablomartin4btc, AndreaDiazCorreia

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35940 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35940.svg"></sub> (net: allow selecting BIP152 high-bandwidth peers with -addnode by w0xlt)
#30951 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/30951.svg"></sub> (net: option to disallow v1 connection on ipv4 and ipv6 peers by stratospher)

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.

pablomartin4btc commented at 6:30 PM on August 19, 2026: member

Concept ACK

Comparing this patch to the validation performed in CConnman::AddNode() (the RPC path), which resolves the target — full resolution is out of scope as clearly explained in the description and commit body. What about also checking the port (perhaps using SplitHostPort), so e.g. node.example and node.example:8333 (mainnet default port) would be treated as the same target, and the second one excluded as a duplicate?

danielabrozzoni commented at 2:15 PM on August 27, 2026: member

Concept ACK on ignoring repeated values.

When looking at how addnode handles repeated values, I noticed that it uses CConnMan::AddNode, which invokes LookupNumeric to parse IP addresses and remove equivalent representations (it doesn't do any DNS resolution). For consistency, it may make sense for -addnode values to use the same check.

One way would be to call AddNode inside of CConnman::Init, but I'm not sure if it's okay to call LogWarning from there:

diff --git a/src/net.h b/src/net.h
index ea0c651d11..d69b9a4df3 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1140,12 +1140,13 @@ public:
         vWhitelistedRangeIncoming = connOptions.vWhitelistedRangeIncoming;
         vWhitelistedRangeOutgoing = connOptions.vWhitelistedRangeOutgoing;
         {
-            LOCK(m_added_nodes_mutex);
             // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
             // peer doesn't support it or immediately disconnects us for another reason.
             const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
             for (const std::string& added_node : connOptions.m_added_nodes) {
-                m_added_node_params.push_back({added_node, use_v2transport});
+                if (!AddNode({added_node, use_v2transport})) {
+                    LogWarning("Ignoring duplicate -addnode value: %s", added_node);
+                }
             }
         }

Otherwise, you can duplicate the logic in init.cpp.

To be clear, I'm okay even with the code as-is, if it gets too complicated to add the LookupNumeric checks.

w0xlt force-pushed on Sep 1, 2026

w0xlt commented at 11:34 PM on September 1, 2026: contributor

@pablomartin4btc @danielabrozzoni Thanks for the suggestions. Great catches. I split the change into two commits and credited each of you as a co-author on the respective commit.

DrahtBot added the label CI failed on Sep 1, 2026

DrahtBot removed the label CI failed on Sep 2, 2026

in src/net.h:1148 in 4ebff1af2a outdated

1150 | +        // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
1151 | +        // peer doesn't support it or immediately disconnects us for another reason.
1152 | +        const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
1153 | +        for (const std::string& added_node : connOptions.m_added_nodes) {
1154 | +            if (!AddNode({added_node, use_v2transport})) {
1155 | +                LogWarning("Ignoring duplicate -addnode value: %s", added_node);

pablomartin4btc commented at 1:30 AM on September 11, 2026:

minor nit: this logs only the rejected value, not what it matched against. For a user staring at their config wondering why an entry got dropped, "Ignoring duplicate -addnode value: node.example:8333 (matches node.example)" would be more actionable — especially since the whole point of this PR is catching non-obvious duplicates (different port representations), where "this is a duplicate" alone doesn't explain why. Not blocking, just a nice-to-have.

in src/net.cpp:3849 in 4ebff1af2a outdated

3846 | +        uint16_t existing_port{GetDefaultPort(it.m_added_node)};
3847 | +        if (resolved_is_valid && resolved == LookupNumeric(it.m_added_node, existing_port)) return false;
3848 | +
3849 | +        std::string existing_host;
3850 | +        if (host_port_is_valid && SplitHostPort(it.m_added_node, existing_port, existing_host) &&
3851 | +            host == existing_host && port == existing_port) {

pablomartin4btc commented at 1:37 AM on September 11, 2026:

One more equivalence worth considering, in the same spirit as the host/port work here: hostname comparison is case-sensitive (host == existing_host), so -addnode=Example.com and -addnode=example.com wouldn't be caught as duplicates, since DNS hostnames are case-insensitive. This isn't new — the original exact-string check had the same gap — and it's arguably out of scope for this PR. But since the whole point here is catching duplicate representations of the same target rather than just literal string matches, case-folding the hostname comparison feels like a natural extension of the same fix rather than a separate concern, so wanted to flag it while this code is already being touched. No test currently covers it either way, consistent with it not being handled. Happy to leave it out if you'd rather keep this PR narrowly scoped to the port-equivalence case.

pablomartin4btc commented at 1:39 AM on September 11, 2026: member

ACK 4ebff1af2acc6e846eb7932ff8ece03ffa3a9066

Thanks for taking my suggestion!

Left a couple of inline nits I noticed on a re-read — nothing blocking.

DrahtBot requested review from danielabrozzoni on Sep 11, 2026

AndreaDiazCorreia commented at 11:43 PM on September 19, 2026: none

tested ACK 4ebff1af2acc6e846eb7932ff8ece03ffa3a9066

Built and tested both commits separately. Also checked on regtest that -addnode=first.node -addnode=first.node:18444 keeps a single entry in getaddednodeinfo.

The only thing I found is the case-sensitive hostname comparison @pablomartin4btc already flagged, which seems fine as a follow-up.

DrahtBot added the label Needs rebase on Oct 7, 2026

net: reject equivalent addnode host-port pairs

CConnman::AddNode rejects equivalent numeric addresses, but compares
unresolved hostnames as raw strings. As a result, the same hostname
with and without an explicit default port can be stored twice.

Compare successfully parsed literal hosts using their effective ports.
Do not perform DNS resolution, and keep non-default ports distinct.

Co-authored-by: Pablo Martin <pablomartin4btc@gmail.com>

91683b1202

init: add startup nodes through CConnman::AddNode

Startup -addnode values are copied directly into CConnman state,
bypassing the duplicate checks used by the addnode RPC. Equivalent
values are consequently stored and retried independently.

Insert startup values through CConnman::AddNode so exact, numeric, and
effective-port duplicates share the runtime behavior. Keep the first
entry and log each rejected duplicate.

Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com>

55003ac5fb

w0xlt force-pushed on Oct 8, 2026

RandyMcMillan commented at 12:10 AM on October 9, 2026: contributor

Concept ACK

DrahtBot removed the label Needs rebase on Oct 9, 2026