torcontrol: only output disconnect if -debug=tor #7035

pull dcousens wants to merge 1 commits into bitcoin:master from dcousens:torlog changing 2 files +4 −2

dcousens commented at 1:13 AM on November 17, 2015: contributor

Resolves [the noise aspect of] #7032
in src/torcontrol.cpp:None in d6be882148 outdated
```
 607 | @@ -608,7 +608,11 @@ void TorController::disconnected_cb(TorControlConnection& conn)
 608 |      service = CService();
 609 |      if (!reconnect)
 610 |          return;
 611 | -    LogPrintf("tor: Disconnected from Tor control port %s, trying to reconnect\n", target);
 612 | +
 613 | +    if (LogAcceptCategory("tor")) {
 614 | +        LogPrintf("tor: Disconnected from Tor control port %s, trying to reconnect\n", target);
 615 | +    }
```
dcousens commented at 1:13 AM on November 17, 2015:

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

MarcoFalke commented at 7:56 AM on November 17, 2015:

If you just change to LogPrint? ( https://github.com/laanwj/bitcoin/blob/2015_11_development_guidelines/doc/developer-notes.md#strings-and-formatting )
laanwj commented at 8:48 AM on November 18, 2015:

yes this would be equivalent:

LogPrint("tor", "tor: Disconnected from Tor control port %s, trying to reconnect\n", target);
dcousens commented at 8:59 AM on November 18, 2015:

Thanks @MarcoFalke, Ill change it
dcousens commented at 1:15 AM on November 17, 2015: contributor

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 commented at 1:18 AM on November 17, 2015: contributor

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?
MarcoFalke commented at 8:16 AM on November 17, 2015: member
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 commented at 9:03 AM on November 17, 2015: member

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).
laanwj added the label P2P on Nov 17, 2015
dcousens commented at 9:01 AM on November 18, 2015: contributor

Rebased and now uses LogPrint (instead of LogPrintf and LogAccept)
MarcoFalke commented at 10:44 AM on November 18, 2015: member

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.
in src/torcontrol.cpp:None in 3d8792f261 outdated
```
 607 | @@ -608,7 +608,9 @@ void TorController::disconnected_cb(TorControlConnection& conn)
 608 |      service = CService();
 609 |      if (!reconnect)
 610 |          return;
 611 | -    LogPrintf("tor: Disconnected from Tor control port %s, trying to reconnect\n", target);
 612 | +
 613 | +    LogPrint("tor", "tor: Disconnected from Tor control port %s, trying to reconnect\n", target);
```
laanwj commented at 1:06 PM on November 19, 2015:

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.

instagibbs commented at 1:54 PM on November 19, 2015:

@laanwj agreed

dcousens commented at 8:49 PM on November 19, 2015:

@laanwj shall it be done in this PR?

MarcoFalke commented at 10:04 PM on November 19, 2015:

@dcousens Yes, because the "if there was a connection in the first place" logic is not here yet.
laanwj commented at 8:07 AM on November 20, 2015:

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.
MarcoFalke commented at 4:10 PM on November 28, 2015:

@dcousens ping
sipa commented at 1:07 PM on November 28, 2015: member

Needs rebase.
laanwj commented at 10:03 AM on November 30, 2015: member

Needs rebase and nit fix.
dcousens commented at 11:18 AM on November 30, 2015: contributor

@laanwj on it, just haven't had time yet. Tomorrow :+1: (It was waiting on #7058, but now that's merged, its just blocked by me)
torcontrol: only output disconnect if -debug=tor 4531fc4272
laanwj merged this on Nov 30, 2015
laanwj closed this on Nov 30, 2015
laanwj referenced this in commit 9b8fc6c89a on Nov 30, 2015
dcousens deleted the branch on Dec 1, 2015
dcousens commented at 12:31 AM on December 1, 2015: contributor

@laanwj your comment is still relevant, but maybe another PR.
zkbot referenced this in commit 45faa928ec on Mar 26, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7032 torcontrol is noisy in debug.log if no Tor running #7218 [qt] Fix misleading translation