netbase: clean up Proxy logging #29882
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
src/netbase.cpp
Outdated
There was a problem hiding this comment.
It's probably a good idea to keep the !IsValid check here, just to be sure. Fine with removing the log.
There was a problem hiding this comment.
I could be mistaken, but it looks at first glance that the same result return {} would be produced in ConnectToSocket(). I don't think it hurts anything to keep the check though (does seem safer, or at least more readable).
and reached in Proxy::Connect() through either the call to ConnectToSocket():
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L653-L655
or by way of ConnectDirectly():
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L626-L628
There was a problem hiding this comment.
Yes, it's not strictly currently necessary, but code changes sometimes. It's good to have error and unexpected situation checking in place, instead of crashing later on.
Edit: also removing a check is not strictly part of "cleaning up logging".
There was a problem hiding this comment.
Restoring the check, without the log and revising the PR description.
tdb3
left a comment
tdb3
left a comment
There was a problem hiding this comment.
ACK for d1af4422d13ba84253a1555c2fe5a42170fa8b35
Good cleanup, making logs less cluttered while keeping relevant connection state logging.
In addition to observing the lack of Cannot connect to socket for message, also ran a quick test where Tor was stopped then started again. bitcoind was still able to detect and inform the user of the loss of the proxy and subsequent peer connected message provided indication that connection through proxy was reestablished.
13308 tx=3456746 date='2024-04-16T03:14:24Z' progress=0.999999 cache=0.3MiB(285txo)
2024-04-16T03:23:15Z UpdateTip: new best=0000009be064b2b97227938e8e391444d13f6a3495ae62eb3233aa93d52abee5 height=191494 version=0x20000000 log2_work=41.113316 tx=3456773 date='2024-04-16T03:17:56Z' progress=0.999999 cache=0.3MiB(358txo)
2024-04-16T03:23:15Z New outbound-full-relay v1 peer connected: version: 70016, blocks=191494, peer=2
2024-04-16T03:23:16Z New outbound-full-relay v1 peer connected: version: 70016, blocks=191494, peer=3
2024-04-16T03:23:18Z P2P peers available. Skipped DNS seeding.
2024-04-16T03:23:18Z dnsseed thread exit
[killed Tor]
2024-04-16T03:23:21Z [error] Error while reading proxy response
2024-04-16T03:23:22Z connect() to /tmp/torsock failed: No such file or directory (2)
2024-04-16T03:23:22Z connect() to /tmp/torsock failed: No such file or directory (2)
2024-04-16T03:23:23Z connect() to /tmp/torsock failed: No such file or directory (2)
2024-04-16T03:23:23Z connect() to /tmp/torsock failed: No such file or directory (2)
2024-04-16T03:23:24Z connect() to /tmp/torsock failed: No such file or directory (2)
2024-04-16T03:23:24Z connect() to /tmp/torsock failed: No such file or directory (2)
[Started Tor again]
2024-04-16T03:23:29Z New outbound-full-relay v1 peer connected: version: 70016, blocks=191494, peer=4
src/netbase.cpp
Outdated
There was a problem hiding this comment.
I could be mistaken, but it looks at first glance that the same result return {} would be produced in ConnectToSocket(). I don't think it hurts anything to keep the check though (does seem safer, or at least more readable).
and reached in Proxy::Connect() through either the call to ConnectToSocket():
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L653-L655
or by way of ConnectDirectly():
https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L626-L628
pinheadmz
force-pushed
the
cleaner-proxy-logs
branch
from
d1af442 to
fb4cc5f
Compare
6 participants
Follow up to #27375 and see #29649 (comment)
This removes an extra log message when we can't connect to our own proxy, and another when the proxy is invalid.
Before #27375 if proxy is unreachable
After #27375 if unix proxy is unreachable:
After this PR: