net: don't count disconnecting peers against the inbound ceiling #35239

Summary

Fixes #27843 — inbound I2P connections can push the active peer count past m_max_inbound (e.g. 190 vs the default ceiling of 125) because CreateNodeFromAcceptedSocket counts every inbound entry in m_nodes when checking the ceiling, including peers already marked fDisconnect = true by a prior eviction but not yet drained.

Skip those peers in the count. One-line behavioural change, ten-line explanatory comment for future readers.

Why this is benign for TCP and not for I2P

AcceptConnection (TCP) runs in ThreadSocketHandler, which also calls DisconnectNodes() in the same loop iteration — so a peer marked fDisconnect is removed from m_nodes before the next AcceptConnection/CreateNodeFromAcceptedSocket is reached. The stale counter is invisible.

ThreadI2PAcceptIncoming is a separate thread. Inbound I2P bursts each observe nInbound >= m_max_inbound, each call AttemptToEvictConnection (which only marks one peer fDisconnect), and each accept the new connection. The counter stays pinned at the ceiling because marked peers remain in m_nodes until ThreadSocketHandler next runs DisconnectNodes(). The active inbound count keeps growing.

Why this fix and not #27912

#27912 (closed unmerged in 2024-05 by @willcl-ark for lack of time) tackled the same issue by making eviction itself synchronous inside AttemptToEvictConnection. That approach got tangled in m_nodes_disconnected thread-safety versus cs_main ordering, and would have required the broader shared_ptr<CNode> refactor that @vasild and @willcl-ark were drafting at the time.

The present PR sidesteps the architectural change. The async-eviction contract stays intact — we only correct what the counter measures. The only observable side-effect is that m_nodes.size() may briefly hold a small number of extra entries (those marked-but-not-yet-drained) above m_max_inbound; they are already drained on the next ThreadSocketHandler iteration, which happens promptly.

This fix matches @vasild's 2023 design intent in #27912:

"I intended ThreadI2PAcceptIncoming() to be just low level socket-accept function […] And then all the higher level logic (e.g. capping the number of connections) to happen in CreateNodeFromAcceptedSocket(). It indeed checks nMaxInbound but does the disconnect asynchronously."

The check is now also semantically correct for the async-disconnect case.

Test plan

• CI green on Linux/macOS/Windows
• Repro from #27843 (test patch by @Crypt-iQ in the issue body, plus the bash spam script) — expect the active inbound count via getconnectioncount to stay ≤ -maxconnections even under sustained parallel I2P inbound bursts
• No regression on the TCP AcceptConnection path (same counter, same logic, just no longer counts marked peers; that path was always reaching it with zero marked peers in practice)

I have not added a dedicated unit test in this PR. The behaviour is testable via the CreateNodeFromAcceptedSocketPublic helper already exposed in src/test/util/net.h and used by src/test/fuzz/connman.cpp; happy to follow up with a unit test that seeds m_nodes with one fDisconnect=true inbound peer and asserts the new acceptance behaviour, on request.

cc @vasild @jonatack @Crypt-iQ @willcl-ark @mzumsande

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35239.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Thx, but LLM output is not accepted if the author can not properly explain, test or could have written the change themselves.

The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on.

Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR.

I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output.

I'm not asking you to close this PR. I am asking you to reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested.

Closing for now.