torcontrol: only output disconnect if -debug=tor by dcousens · Pull Request #7035 · bitcoin/bitcoin

dcousens · 2015-11-17T01:13:06Z

Resolves [the noise aspect of] #7032

dcousens · 2015-11-17T01:13:55Z

src/torcontrol.cpp

Not sure if this is the right way to do this, only other example I saw was in https://github.com/bitcoin/bitcoin/blob/master/src/httpserver.cpp#L398

If you just change to LogPrint? ( https://github.com/laanwj/bitcoin/blob/2015_11_development_guidelines/doc/developer-notes.md#strings-and-formatting )

yes this would be equivalent:

LogPrint("tor", "tor: Disconnected from Tor control port %s, trying to reconnect\n", target);

Thanks @MarcoFalke, Ill change it

dcousens · 2015-11-17T01:15:17Z

I'm also not sure if you'd always want to silence all these messages, especially if a tor connection did exist at one point.

gmaxwell · 2015-11-17T01:18:46Z

utACK, we should at least be silencing these if it was never up. Otherwise it's noise for people who don't have it configured. This was why I was asking the other day if the authstring is ever empty: perhaps we could not bother trying if its empty?

maflcko · 2015-11-17T08:16:23Z

The problem is, that this can be caused by various things:

Tor not running
Wrong tor password provided
...?

Event though, you can use an empty string as password, bitcoin will consider it as no password ( https://github.com/bitcoin/bitcoin/pull/6639/files#diff-eceb168df4d9787d4474a99c72db339aR585 )

So maybe a safe solution is to not print further tor debug stuff if the password is empty? (Same holds for non existent auth cookie?)

laanwj · 2015-11-17T09:03:33Z

So maybe a safe solution is to not print further tor debug stuff if the password is empty? (Same holds for non existent auth cookie?)

That doesn't work. It only knows where the auth cookie is after connecting to Tor. Also the user may start Tor later, resulting in a different auth cookie (or even auth cookie location). It simply cannot know without trying.

I think this solution is fine.

Would be somewhat better to detect if there was a connection in the first place, and to always log a message if there was, and log a different message only w/ debug=tor if there wasn't (as this is only useful for troubleshooting).

dcousens · 2015-11-18T09:01:54Z

Rebased and now uses LogPrint (instead of LogPrintf and LogAccept)

maflcko · 2015-11-18T10:44:21Z

I think this solution is fine.

utACK 3d8792f

Would be somewhat better to detect if there was a connection in the first place, and to always log a message if there was, and log a different message only w/ debug=tor if there wasn't (as this is only useful for troubleshooting).

Can be another PR.

laanwj · 2015-11-19T13:06:34Z

src/torcontrol.cpp

If we intend to keep this message unconditional, we should make it "Not connected to Tor control port" as @petertodd suggests in #7032. This doesn't imply being connected before.

@laanwj agreed

@laanwj shall it be done in this PR?

@dcousens Yes, because the "if there was a connection in the first place" logic is not here yet.

Yes, it's one or the other:

Make the logic determine whether there was a connection, report if there was one and it was lost, report a different message if we're re-trying to connect because connection failed

Change to a blanket message that covers both cases

If not, people that have -debug=tor enabled can still complain that the message is not correct.
The second is just a matter of changing a message so should be done in this PR, IMO.

@dcousens ping

sipa · 2015-11-28T13:07:02Z

Needs rebase.

laanwj · 2015-11-30T10:03:10Z

Needs rebase and nit fix.

dcousens · 2015-11-30T11:18:56Z

@laanwj on it, just haven't had time yet. Tomorrow 👍
(It was waiting on #7058, but now that's merged, its just blocked by me)

4531fc4 torcontrol: only output disconnect if -debug=tor (Daniel Cousens)

dcousens · 2015-12-01T00:31:08Z

@laanwj your comment is still relevant, but maybe another PR.

dcousens reviewed Nov 17, 2015
View reviewed changes

laanwj added the P2P label Nov 17, 2015

dcousens mentioned this pull request Nov 19, 2015

Fix ZMQ docs - Improve logging #7058

Merged

laanwj reviewed Nov 19, 2015
View reviewed changes

torcontrol: only output disconnect if -debug=tor

4531fc4

laanwj merged commit 4531fc4 into bitcoin:master Nov 30, 2015

laanwj added a commit that referenced this pull request Nov 30, 2015

Merge pull request #7035

9b8fc6c

4531fc4 torcontrol: only output disconnect if -debug=tor (Daniel Cousens)

dcousens deleted the torlog branch December 1, 2015 00:30

maflcko mentioned this pull request Dec 16, 2015

[qt] Fix misleading translation #7218

Merged

str4d mentioned this pull request Mar 24, 2017

Tor ephemeral hidden services zcash/zcash#2177

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

dcousens commented Nov 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Nov 17, 2015

Uh oh!

gmaxwell commented Nov 17, 2015

Uh oh!

maflcko commented Nov 17, 2015

Uh oh!

laanwj commented Nov 17, 2015

Uh oh!

dcousens commented Nov 18, 2015

Uh oh!

maflcko commented Nov 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 28, 2015

Uh oh!

laanwj commented Nov 30, 2015

Uh oh!

dcousens commented Nov 30, 2015

Uh oh!

dcousens commented Dec 1, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants