Show nodeid instead of addresses (for anonymity) unless otherwise requested #3764

This attempts to fix the problem with revealing too much information about IP addresses, but still provides a way to track the progress of different nodes (via the NodeId). Enabling IP addresses is now optional via the -logips command line option.

I like the idea of logging node IDs instead of logging IP addresses.

In principle, all -logips then has to do is log a node ID to IP:port mapping once on connection and it's possible to figure out the IPs if necessary (Though it may be more useful in practice to actually log IPs in every message anyway).

Concept ACK; I haven't reviewed the code

@laanwj yes, could make -logips display IPs instead of NodeIDs, if this is what most people want. I went with just correlating them once (upon first connection). so that it keeps the debug.log more readable and smaller in size, but the info is still there if it needs to be referenced.

node=(IP-or-id) seems ideal to me

@luke-jr = instead of : ?

@rebroad Whatever matches the existing style. The points I was making were 1) use the same key whether showing ID or IP; 2) show IP in every place ID would normally be

@luke-jr Now using node=

@luke-jr has suggested that the IP address could be added to each and every line of debug.log instead of the node number, whereas I prefer that debug.log correlates the node number with the IP address only once, so reduce debug.log output and to make it more readable.

Can I seek consensus on this please? I'm happy to adapt this pull to the majority preference.

(Luke's suggestion might possibly involve creating a new variable within the CNode struct to contain the string to display so that there didn't need to be two LogPrintfs for every time one or the other is displayed).

I like the approach of logging the IP:port info for a node only once, on connection. After that, log only the Node ID for messages concerning that node.

This minimizes the code difference between the -logips and -nologips case (ie: one message) and avoids ugly if() statements duplicating the debug message apart from the IP information...

I like the general ID of always logging a node ID with every message we emit which can be resolved against getpeerinfo— enabling after the fact diagnosis of a source while the client is still up. It needs to be a nonce though, not a sequential ID or we can correlate them based on connect time. The the logips logging can just make the connect message emit the nodeids.

There is some privacy leak, that you can see that the same node triggered two messages, but I don't think that one is concerning enough to overcome the loss of useful debugging messages.

@gmaxwell I'm not sure I understand you. How would a nonce be more useful than the node id? How/when/where would you want getpeerinfo to be used?

I'm not sure what you don't understand. Right now the connect messages show IPs and so you get some good idea of what order nodes are, so the IDs would be linking. Thats bad. Additionally, esp if we later add crypto, I worry about metadata only attackers who can observe what order people connected in. The linkage between IDs and IPs should not be externally observable.

@gmaxwell the IPs as they are currently shown will no longer be shown by default once this patch is applied, only when the -logips option is given for the very reasons you mention.

Can you please reread my answer, including the second half of it?

Wouldn't these changes like this make the current ongoing mtgox investigations harder why is this even being prioritized. On Mar 2, 2014 1:44 PM, "R E Broadley" notifications@github.com wrote:

@gmaxwell https://github.com/gmaxwell I already read your answer 3 times, and on the 4th reading my interpretation of it is no different.

Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/3764#issuecomment-36462239 .

No priority IMO, this is just @rebroad's pet peeve.

@rebroad "Additionally, esp if we later add crypto, I worry about metadata only attackers who can observe what order people connected in. The linkage between IDs and IPs should not be externally observable." someone watching who connects in what order would know the IDs even if they didn't know the content of the messages. getting the logs would tell them the content.

Logging IPs is a security regression IMO.

Thinking about it, we already concluded in #3741:

• The point was to not log IPs (esp. those associated with transactions) by default
• We are logging almost no IPs by default already: only on connect and some kinds of misbehavior:
  • Nearly all of the messages affected in this pull are only logged when specific debug classes (mempool, net) are active

So I'm not entirely sure what is the point. As for the code this introduces a lot of clutter (duplicated debug messages) making it harder to maintain. It also adds yet another option to the long list.

@gmaxwell I'm not understanding what you're not understanding. There would be no linkage as you mention unless this optional command line argument is given. Logging which node is doing what is a useful debugging too, especially when working on code such as parallel block download or anything which needs to correlate what was sent to nodes and what was received from nodes. It needs to be a debug option therefore. The alternative I can suggest is that instead of the "logips" option as I've done here, I change it to "lognodeids" option, and we leave the display of IP addresses in as it currently is, but make logging node ids dependant on this command line option given. Either way, the "problem" as you put it would still exist, which is the correlation between node ids and IP addresses, but since these options would only be enabled by developers, and are necessary for certain development work, I don't really see there being an issue. @laanwj Hopefully my explanation above also answers your query.

@rebroad: @gmaxwell is saying that if someone is able to monitor your network activity (just metadata, nor the actual traffic) + has access to these logs (even without -logips) this would allow reconstructing which nodes were talking to eachother.

I personally think this is a very limited problem, and the solution suggested (using random instead of sequentially assigned identifiers) doesn't help much against it (-logtimestamps is default now, so you would still be able to correlate with the log).

I think this can even improve usefulness of logs (both with and without -logips, though I'd prefer to only log IPs even with -logips at initial connection), as sequentially assigned ids are actually more readable than IPs when looking through logs to diagnose sync issues.

@sipa thanks. So, from the sounds of what you're saying, it doesn't sound like I can make any further improvements to this pull request?

@laanjw - this is primarily motivated by helping to debug block sync. Would it be better if it wasn't displayed in the commandline help, and perhaps only displayed node id for blocks and not for txs?

ACK, apart from minor nit above.

@sipa Hopefully I've addressed your minor nit.

What is holding this up please?

@sipa @laanwj Ok, I've just rebased this again. Is there anything still holding this up?

@luke-jr This pull request makes the logging of IPs optional instead of done by default as it currently is - so hopefully addresses your security concerns.

I know.

I don't really care about adding an optional and by-default-off method for listing IPs. For debugging it is useful, and nobody will turn it on for any reason but that.

The arguments against even providing the ability to link log messages from the same node are interesting. I'm not sure it's such a big deal. If it is, maybe we can list the peer=id message piece only when -logips is provided as well (and thus even stricten the default behaviour, which currently does list some IPs already)?

@sipa point addressed all except the making the peer logged only if a command line option is specified. I can do this if it's preferred. What is the consensus on this? @gmaxwell @laanwj @luke-jr @gavinandresen @jgarzik ?

rebaed, and nits addressed.

@rebroad you still need to define the IdentityNode function as suggested by me/@sipa to avoid if (fLogIPs) all over the place.

@laanwj There is now only one place where there is an if (fLogIPs), so the function is not needed since this would add more code than it removes.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3764_2e36866fecb7420cd73047a7aa762a6e5e225695/ 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.

Was was this merged without adding the peer index to getpeerinfo? The peer numbers given in the logs are not useful.