Don't log IP addresses for transactions. #3741

Don't log IP addresses by default. Argument originally given in pull request #2126 by darkhosis and luke-jr.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1df708cec2da7f2a41dd3d846bead82b594f9a15 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Personally, I don't think this change is a good idea. There are reasons why one might want to have this information. If you're worried about it, perhaps add an option to control it. It could even be off by default, but I don't want to make it impossible.

You removed not only the IP adress but also the subversion. The version is very useful to know when a strange transaction comes in.

All in all, I think this makes little sense. Are you afraid of people logging IPs? Well, the people that want to log IPs can simply patch the source code to do so. Or use a sniffing tool. Some sites such as blockchain.info will already log your IP (see the sad story in #2653) and even make it queryable for all.

At this point if you don't want your IP all over the place you should use bitcoin behind Tor.

@laanwj Historically, we've had a policy (there are PRs prior to #2126 also) to not log IP addresses. The reason being, that it makes bitcoin node logs a potentially valuable resource to hack/subpoena, making a hassle for users. If an individual wants to patch it in (or even enable an option), that's fine, but having it enabled by default is IMO a real risk.

I agree with @luke-jr here, but we should perhaps allow enabling IP address logging somehow for debugging.

The complaint against making logs valuable is really only about defaults, options are fine.

I'm fine with making logging IPs an option that is off by default.

BTW the respective commit ba6a4ea added IPs in various places (and there were others as well already) so it probably makes sense to generalize this and not single out this single occurence.

Well, right now this isn't implemented as an option as far as I can tell -- it just strips it out completely. I wouldn't be very opposed to having it off by default, but it shouldn't be completely removed.

Ninja edit: Yeah, as @laanwj says, it should definitely be changed across the board, if any change is made at all. (also, TIL that Github PR pages automatically update with new posts even while you're typing.)

So, shall I change this pull request, and extend it to other areas so that IPs are logged, but only when a command line option is given, which is off by default? This is kinda what I did about 2 years ago (#1311), but perhaps policy has changed since then, although my code was pretty messy back then, so I'm not surprised much of it was never pulled.

Right now I think the only IPs we log by default are connect messages— and I actually don't see much harm in those. I'd like to eventually move in the direction of keeping a ring buffer in memory and dumping it on crash (or on command), and otherwise logging very little to disk by default.

@gmaxwell sounds like a good idea, well, more so on Windows, whereas on unix there are more system tools available for doing this externally to bitcoind.

what purpose would logging IPs serve ?

it's useful for debugging. If some node is sending you garbage it's useful to be able to reconnect to it intentionally to see what its doing and reproduce the behavior.

@gmaxwell I think you're right. For example the mempool messages affected by this pull are not logged by default, only if -debug=mempool is provided. Apart from that we log IPs for incoming/outgoing version messages and misbehaving peers.

I've just raised a pull request #3764 that hopefully addresses the points made. Thanks for any feedback on this.

@rebroad For future reference, there's no need to create a new PR. If you just push to the same branch (DontLogTxIPaddresses) the PR gets updated.

I'm confused. I thought pull #3764 provided a solution to what people were requesting in this pull request, but the arguments I'm hearing on there is that it adds an extra command line argument to what is already a list of too many command line options, yet, people on here are asking for it to be optional. Can someone please clarify what is wanted?

How many times do we need to repeat this: there is no issue!

In the default configuration (so if you don't enable -debug=net), which this 'threat scenario' is all about, those extra IP-logging messages are not logged. If you want extra network debugging you get IPs. That's exactly how it should be, IMO.