torcontrol: only output disconnect if -debug=tor #7035
Conversation
src/torcontrol.cpp
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
There was a problem hiding this comment.
yes this would be equivalent:
LogPrint("tor", "tor: Disconnected from Tor control port %s, trying to reconnect\n", target);
|
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. |
|
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? |
|
The problem is, that this can be caused by various things:
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?) |
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/ |
|
Rebased and now uses |
utACK 3d8792f
Can be another PR. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@dcousens Yes, because the "if there was a connection in the first place" logic is not here yet.
There was a problem hiding this comment.
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.
|
Needs rebase. |
|
Needs rebase and nit fix. |
4531fc4 torcontrol: only output disconnect if -debug=tor (Daniel Cousens)
|
@laanwj your comment is still relevant, but maybe another PR. |
Tor ephemeral hidden services Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6503 (included to reduce merge conflicts) - bitcoin/bitcoin#6639 - bitcoin/bitcoin#6643 - bitcoin/bitcoin#7090 - bitcoin/bitcoin#7035 - bitcoin/bitcoin#7170 - bitcoin/bitcoin#7218 (non-QT part) - bitcoin/bitcoin#7313 - bitcoin/bitcoin#7438 - bitcoin/bitcoin#7553 - bitcoin/bitcoin#7637 - bitcoin/bitcoin#7683 - bitcoin/bitcoin#7813 - bitcoin/bitcoin#7703 - bitcoin/bitcoin#8203 - bitcoin/bitcoin#9004 - bitcoin/bitcoin#9234 - bitcoin/bitcoin#9911 (partial) Closes #2061.
6 participants
Resolves [the noise aspect of] #7032