This makes non-trivial unrelated changes, for example it adds DoS() to node connecting with the old protocol version.

Also, NAK on the general introduction of peer address to many messages. I'd be okay with logging the peer address of attack behavior or truly unusual exceptions, but logging by default the address of every peer we pull transactions from is not good and would create considerable incentives to compromise the security of nodes to use their logs for tracing transactions.

logtimestamps was disabled by default for similar reasons that we don't always log peer address... major privacy implications.

@gmaxwell Doesn't it make more sense to use the misbehaving function on a peer that you will always disconnect whenever it connects? Why not use it?

Re peer addresses: these are changes anyone could do anyway, it is useful debug info, especially when trying to work out why blocks aren't downloading as fast as they could be, or for troubleshooting code to determine network lag in general or per peer. The debug.log is chopped regularly, so there is usually no more than a day or two of logs. Would it help perhaps, if instead of showing IP addresses, it just used a unique number instead? The IP address itself isn't important, but correlating which of several peers is.

Re timestamps, @jgarzik - how is this a privacy implication? With timestamps off, the flushing of the wallet.dat is always timestamped, so a timestamp per line surely makes little difference, does it?

The problem isn't that people can do it— it's the expectation that having this creates that going and collecting drives from bitcoin nodes will net you valuable information. Excessive logging endangers our users in several ways— beyond the lost privacy there is the risk of node operators being subject to frivolous fishing expedition investigations just to gather their logs, being hacked to use nodes to trace back nodes, etc.

I don't believe I've ever wanted addresses in the logs of someone elses node I was troubleshooting, except on rpc authentication failures. (I have wanted addresses on my own nodes too, but I can— as you point out— enable that locally).

The default logging behavior shouldn't reflect the logging we as developers user to build the software— it should reflect the logging we need to help users with mysterious behavior out in the field... and thats all. If not for that need we wouldn't have logging at all.
Regarding timestamps— infrequent markers are not the same thing as timestamps on every message.

On the misbehaving change— I wasn't complaining that it was a bad change I was complaining that you're mixing changes that require different kinds of review in your pull.

The debug log entries are mostly cosmetic, assuming they aren't outright buggy and ignoring privacy issues the amount of harm changing them is at worst fairly minimal. Whereas a DoS change even if it works perfectly, could knock the whole network out— it requires a different kind of consideration.

As far as it goes, I think it actually is a bad change: It means that when the old node gets upgraded he may be blacklisted by the whole network and not be able to get on for another 24 hours. Because it costs basically no more to hang up due to the blacklist vs hang up at the addr message I don't see that it saves us anything.

Understood. Ok, so DoS to be removed. Timestamps off by default. How about changing it to an option to enable peer logging, that's off by default? Would that be ok then?

Looks like this pull contains code from other pull requests you've made. Shouldn't each pull request only have the code related to the specific change?

I'm fairly sure this pull also includes the code from here -> #1306
and here -> #1305

@fanquake I just did a git log -p to check, but there's no mention of proxytoo in the entire log history, so I'm not quite sure what you're looking at. oh.. but on github it does show... I'll try pushing again. Ok, I've removed the ProxyToo mention now, thanks for spotting that. An inadvertent merge, not that this'll be enough to get it ACKed. I'll add a flag for the peer logging, as I agree with @gmaxwell that it reduces anonymousness unnecessarily in most situations.

Ok, this now displays peers only if -logpeers is true, and that option isn't advertised unless the code is examined. Peers are shown for blocks, and for Dos(100) transactions only unless the option is selected. Timestamps are disabled by default. DoS for obsolete version also removed. Also, HandleSIGTERM now shows the signal number received.

Thanks for the feedback @gmaxwell, hopefully this is a more agreeable edit now.

This pull now contains no functional changes, only display changes.

meh, this thing is still freakin' huge, touching all sorts of code.

will post a few comments inline...