Skip to content

netbase: clean up Proxy logging #29882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 24, 2024
Merged

netbase: clean up Proxy logging #29882

merged 1 commit into from
Apr 24, 2024

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Apr 15, 2024 •
edited
Loading

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

2024-04-15T17:54:51Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:52Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:52Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:53Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:54:53Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)

After #27375 if unix proxy is unreachable:

2024-04-15T17:54:03Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:03Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
2024-04-15T17:54:04Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:04Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
2024-04-15T17:54:04Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:04Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor
2024-04-15T17:54:05Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T17:54:05Z Cannot connect to socket for /Users/matthewzipkin/Desktop/tor

After this PR:

2024-04-15T18:18:51Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T18:18:51Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T18:18:52Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)
2024-04-15T18:18:52Z connect() to /Users/matthewzipkin/Desktop/tor failed: No such file or directory (2)

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 15, 2024 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK tdb3, laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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.

laanwj
laanwj reviewed Apr 15, 2024
View reviewed changes
src/netbase.cpp
@@ -632,11 +632,6 @@ std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connecti

std::unique_ptr<Sock> Proxy::Connect() const
{
if (!IsValid()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably a good idea to keep the !IsValid check here, just to be sure. Fine with removing the log.

Copy link
Contributor

@tdb3 tdb3 Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

bitcoin/src/netbase.cpp

Lines 603 to 604 in d1af442

LogConnectFailure(manual_connection, "connect() to %s failed: %s", dest_str, NetworkErrorString(WSAGetLastError()));
return false;

and reached in Proxy::Connect() through either the call to ConnectToSocket():

bitcoin/src/netbase.cpp

Lines 653 to 655 in d1af442

if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) {
return {};
}

or by way of ConnectDirectly():

bitcoin/src/netbase.cpp

Lines 626 to 628 in d1af442

if (!ConnectToSocket(*sock, (struct sockaddr*)&sockaddr, len, dest.ToStringAddrPort(), manual_connection)) {
return {};
}

Copy link
Member

@laanwj laanwj Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoring the check, without the log and revising the PR description.

tdb3
tdb3 reviewed Apr 16, 2024
View reviewed changes
Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK for d1af442
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
@@ -632,11 +632,6 @@ std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connecti

std::unique_ptr<Sock> Proxy::Connect() const
{
if (!IsValid()) {
Copy link
Contributor

@tdb3 tdb3 Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

bitcoin/src/netbase.cpp

Lines 603 to 604 in d1af442

LogConnectFailure(manual_connection, "connect() to %s failed: %s", dest_str, NetworkErrorString(WSAGetLastError()));
return false;

and reached in Proxy::Connect() through either the call to ConnectToSocket():

bitcoin/src/netbase.cpp

Lines 653 to 655 in d1af442

if(!ConnectToSocket(*sock, (struct sockaddr*)&addrun, len, path, /*manual_connection=*/true)) {
return {};
}

or by way of ConnectDirectly():

bitcoin/src/netbase.cpp

Lines 626 to 628 in d1af442

if (!ConnectToSocket(*sock, (struct sockaddr*)&sockaddr, len, dest.ToStringAddrPort(), manual_connection)) {
return {};
}

@DrahtBot DrahtBot mentioned this pull request Apr 16, 2024
Draft
@pinheadmz pinheadmz force-pushed the cleaner-proxy-logs branch from d1af442 to fb4cc5f Compare April 16, 2024 14:51
tdb3
tdb3 reviewed Apr 16, 2024
View reviewed changes
Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR ACK for fb4cc5f

laanwj
laanwj approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@laanwj laanwj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fb4cc5f

@fanquake fanquake merged commit 19d59c9 into bitcoin:master Apr 24, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Apr 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj approved these changes

+2 more reviewers

@tdb3 tdb3 tdb3 left review comments

@dasibcryptoidology dasibcryptoidology dasibcryptoidology approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pinheadmz @DrahtBot @laanwj @tdb3 @dasibcryptoidology @fanquake