When SetSockAddr() fails in AcceptConnection(), the code logs a warning but continues processing the connection with a default empty CService address. This means ban checks, whitelist matching, and all downstream logic operate on an invalid address.
Return early on failure instead, consistent with other error paths in the same function.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | Bortlesboat |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK on the AcceptConnection early return — continuing with a default empty CService after SetSockAddr failure means ban checks and whitelist matching run against an invalid address, which is clearly wrong.
One question: the diff also includes a change to the ConnectNode log line (removing ConnectionTypeAsString(conn_type) from the debug log). The PR description only mentions the AcceptConnection fix. Is the logging change intentional? If so, it might be cleaner as a separate commit since the two changes are unrelated.
Concept ACK on the
AcceptConnectionearly return — continuing with a default emptyCServiceafterSetSockAddrfailure means ban checks and whitelist matching run against an invalid address, which is clearly wrong.One question: the diff also includes a change to the
ConnectNodelog line (removingConnectionTypeAsString(conn_type)from the debug log). The PR description only mentions theAcceptConnectionfix. Is the logging change intentional? If so, it might be cleaner as a separate commit since the two changes are unrelated.
the only change is in AcceptConnection()
Was this LLM generated? What are the steps to test this? What is the output before and after the changes here?
The fix is a straightforward control flow change I made to make this consistent with other error paths in the same function (e.g. the accept() failure check just above it uses the same pattern).
if (!sock) {
const int nErr = WSAGetLastError();
if (nErr != WSAEWOULDBLOCK) {
LogInfo("socket error accept failed: %s\n", NetworkErrorString(nErr));
}
return;
}
So, when SetSockAddr() fails, addr stays as a default-constructed CService - which is a zeroed-out address (0.0.0.0:0). Then CreateNodeFromAcceptedSocket() is called with that zeroed addr.
Testing this code path requires triggering an incoming connection, which isn't easily reproducible in the test framework. It can't be explicitly tested - there's no test for AcceptConnection() in the test suite.
This is a low risk fix, the worst case outcome is a legitimate connection gets dropped and immediately reconnects.
1744 | @@ -1745,9 +1745,9 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1745 | CService addr;
1746 | if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr, len)) {
1747 | LogWarning("Unknown socket family\n");
The LogWarning here predates this PR, but since you are already touching this block, would it be worth including sockaddr.ss_family in the message to make it more actionable for anyone debugging an unexpected socket family in the wild?
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.