#36014 init: ignore repeated `-addnode` startup values
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36014 init: ignore repeated `-addnode` startup values | Waiting on author | 0 (+2) | Strong | 21 + 38 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Startup `-addnode` arguments are routed through `CConnman::AddNode` rather than being appended directly to the added node list without deduplication. In addition, `CConnman::AddNode` is updated to compare literal hostnames alongside their effective ports so that hostnames with and without an explicit default port are recognized as duplicates.
Problem: Specifying duplicate `-addnode` entries via command line or config file bypasses the duplicate checks applied during runtime `addnode add` RPC calls. When nodes configured this way are disconnected, background connection threads perform redundant DNS lookups and connection attempts on each retry cycle for each duplicate entry, cluttering logs and `getaddednodeinfo` output.
Evidence of importance:
- PR description: ' 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.'
- PR description: 'This can cause redundant DNS lookups, connection attempts, and log messages. `getaddednodeinfo` also reports the repeated entry.'
Risk of a mistake: low (p2p). A parsing or comparison bug in node deduplication could cause a valid user-specified manual connection to be skipped.
Changed since the previous assessment: No structural changes to the approach or scope; the patch remains functionally identical to the previous version with minor updates to commit metadata.
Reviewability
Paused · Waiting on author
Open objection from pablomartin4btc with no author reply for 24 days; review would repeat what has been asked.
Waiting on (from the thread read):
- nothing
Agreement
Strong
substantive support, no open objection (danielabrozzoni, pablomartin4btc, AndreaDiazCorreia)
Thread notes: Converted r:4975506478 and r:5041831301 from objections to suggestions since neither named a cost of merging as-is. Added inline suggestions rc:3985139451 and rc:3985171084 from pablomartin4btc, and RandyMcMillan's Concept ACK. / Reclassified schema 1 objections from pablomartin4btc and danielabrozzoni as suggestions, and added pablomartin4btc's inline suggestions from rc:3985139451 and rc:3985171084 along with RandyMcMillan's Concept ACK.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| pablomartin4btc (member) | suggestion, approach | treat targets specified with and without the default port as duplicates | accepted · fix pushed · author implemented port checking; reviewer gave ACK in r:5174095555 | answered | yes | 2026-08-19: '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?' Status from: 2026-09-11: pablomartin4btc: 'Thanks for taking my suggestion!' |
| danielabrozzoni (member) | suggestion, approach | use LookupNumeric or CConnman::AddNode to deduplicate equivalent numeric IP representations | fixed · fix pushed · author implemented numeric deduplication in separate commit credited to reviewer | answered | yes | 2026-08-27: 'For consistency, it may make sense for `-addnode` values to use the same check.' Status from: 2026-09-01: w0xlt: 'I split the change into two commits and credited each of you as a co-author on the respective commit.' |
| pablomartin4btc (member) | suggestion, interface | include the matched target in the warning log when rejecting a duplicate -addnode entry | open | answered | no | 2026-09-11: 'minor nit: this logs only the rejected value, not what it matched against.' |
| pablomartin4btc (member) | suggestion, approach | case-fold hostname comparison so entries differing only in case are deduplicated | answered · AndreaDiazCorreia agreed it can be addressed in a follow-up | answered | by others | 2026-09-11: 'hostname comparison is case-sensitive (`host == existing_host`), so `-addnode=Example.com` and `-addnode=example.com` wouldn't be caught as duplicates' Status from: 2026-09-19: AndreaDiazCorreia: 'seems fine as a follow-up' |
Support:
- pablomartin4btc (member) (Concept ACK, 2026-08-19) [read: CConnman::AddNode()]: deduplicating -addnode startup values [not substantive]
- danielabrozzoni (member) (Concept ACK, 2026-08-27) [read: src/net.h]: ignoring repeated values
- pablomartin4btc (member) (ACK, 2026-09-11) [read: src/net.cpp, src/net.h]: reviewed updated commits incorporating suggested port-equivalence changes
- AndreaDiazCorreia (none) (Tested ACK, 2026-09-19) [both: getaddednodeinfo]: built both commits separately and verified regtest deduplication with default port
- RandyMcMillan (contributor) (Concept ACK, 2026-10-09): (no reason given) [not substantive]
Participants:
- pablomartin4btc (member, support, 4 statements 2026-08-19 to 2026-09-11): Concept ACK and ACK; suggested port equivalence which author implemented
- danielabrozzoni (member, support, 1 statements 2026-08-27 to 2026-08-27): Concept ACK; suggested numeric IP deduplication which author implemented
- AndreaDiazCorreia (none, support, 1 statements 2026-09-19 to 2026-09-19): Tested ACK; tested regtest behavior and agreed case-sensitive comparison can be handled in follow-up
- RandyMcMillan (contributor, support, 1 statements 2026-10-09 to 2026-10-09): Concept ACK
Files
38 lines under test/bench/ci.
- test/functional/feature_config_args.py +23/-0
- src/net.cpp +14/-2
- src/test/net_peer_connection_tests.cpp +15/-0
- src/net.h +7/-7
Card
The short text the ranking pass compares across PRs.
This pull request deduplicates startup -addnode parameters before connection threads launch and teaches CConnman::AddNode to treat hostnames using explicit default ports as identical to those omitting them. Currently, repeated startup -addnode arguments bypass runtime deduplication checks and are stored multiple times. When disconnected, connection threads run redundant DNS lookups and sequential connection retries for each duplicate entry while spamming logs and getaddednodeinfo. Startup duplicates are now rejected with a warning log, aligning startup behavior with the addnode RPC.
Data
thread read through 2026-10-09 (8 statements), head 55003ac5fb
labels: none
Categories
P2P
P3 · bug fix (ranking pass: Kept adjacent to #35600, which covers the general form of the same duplicate-connection problem.)
Prevents redundant connection attempts and DNS queries caused by duplicate `-addnode` startup entries. As noted in the description, startup `-addnode` inputs bypassed the duplicate checks enforced by runtime RPCs, causing redundant lookups and cluttered `getaddednodeinfo` output.
Membership: Touches `src/net.cpp` and `src/net.h`, modifying connection management and deduplication logic for added nodes.
Factors: security/stability 0, bug 1, performance 1, user value 1, leverage 0