{"action":"created","issue":{"url":"https://api.github.com/repos/bitcoin/bitcoin/issues/8594","repository_url":"https://api.github.com/repos/bitcoin/bitcoin","labels_url":"https://api.github.com/repos/bitcoin/bitcoin/issues/8594/labels{/name}","comments_url":"https://api.github.com/repos/bitcoin/bitcoin/issues/8594/comments","events_url":"https://api.github.com/repos/bitcoin/bitcoin/issues/8594/events","html_url":"https://github.com/bitcoin/bitcoin/pull/8594","id":173338388,"number":8594,"title":"Do not add random inbound peers to addrman.","user":{"login":"gmaxwell","id":858454,"avatar_url":"https://avatars.githubusercontent.com/u/858454?v=3","gravatar_id":"","url":"https://api.github.com/users/gmaxwell","html_url":"https://github.com/gmaxwell","followers_url":"https://api.github.com/users/gmaxwell/followers","following_url":"https://api.github.com/users/gmaxwell/following{/other_user}","gists_url":"https://api.github.com/users/gmaxwell/gists{/gist_id}","starred_url":"https://api.github.com/users/gmaxwell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gmaxwell/subscriptions","organizations_url":"https://api.github.com/users/gmaxwell/orgs","repos_url":"https://api.github.com/users/gmaxwell/repos","events_url":"https://api.github.com/users/gmaxwell/events{/privacy}","received_events_url":"https://api.github.com/users/gmaxwell/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/bitcoin/bitcoin/labels/P2P","name":"P2P","color":"006b75"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":14,"created_at":"2016-08-25T23:53:02Z","updated_at":"2016-08-26T20:12:55Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/8594","html_url":"https://github.com/bitcoin/bitcoin/pull/8594","diff_url":"https://github.com/bitcoin/bitcoin/pull/8594.diff","patch_url":"https://github.com/bitcoin/bitcoin/pull/8594.patch"},"body":"We should learn about new peers via address messages.\r\n\r\nAn inbound peer connecting to us tells us nothing about\r\n its ability to accept incoming connections from us, so\r\n we shouldn't assume that we can connect to it based on\r\n this.\r\n\r\nThe vast majority of nodes on the network do not accept\r\n incoming connections, adding them will only slow down\r\n the process of making a successful connection in the\r\n future.\r\n\r\nNodes which have configured themselves to not announce would prefer we\r\n not violate their privacy by announcing them in GETADDR responses."},"comment":{"url":"https://api.github.com/repos/bitcoin/bitcoin/issues/comments/242839289","html_url":"https://github.com/bitcoin/bitcoin/pull/8594#issuecomment-242839289","issue_url":"https://api.github.com/repos/bitcoin/bitcoin/issues/8594","id":242839289,"user":{"login":"gmaxwell","id":858454,"avatar_url":"https://avatars.githubusercontent.com/u/858454?v=3","gravatar_id":"","url":"https://api.github.com/users/gmaxwell","html_url":"https://github.com/gmaxwell","followers_url":"https://api.github.com/users/gmaxwell/followers","following_url":"https://api.github.com/users/gmaxwell/following{/other_user}","gists_url":"https://api.github.com/users/gmaxwell/gists{/gist_id}","starred_url":"https://api.github.com/users/gmaxwell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gmaxwell/subscriptions","organizations_url":"https://api.github.com/users/gmaxwell/orgs","repos_url":"https://api.github.com/users/gmaxwell/repos","events_url":"https://api.github.com/users/gmaxwell/events{/privacy}","received_events_url":"https://api.github.com/users/gmaxwell/received_events","type":"User","site_admin":false},"created_at":"2016-08-26T20:12:55Z","updated_at":"2016-08-26T20:12:55Z","body":"@EthanHeilman  hosts already send an addr message on connect (see the above stanza), which will add it to new.  So I think also doing it there doesn't add anything new except making it more likely that they'll advertise themselves accidentally. \r\n\r\nThe fact that it was ever added to tried at all was a mistake that violated the design of the system, tried table was supposed to be protected against self-selection by actual trying :) --  I think there could be some legitimate concern that actual useful hosts will be promoted to tried somewhat slower, but I believe that is addressed by the feeler connections. \r\n\r\nI would not be surprised if, after this change is widespread, if tried has _more_ reachable hosts in it-- because nodes will pollute their tried table with hosts that aren't reachable less often. \r\n\r\n> I've checked a bunch of older bitcoind logs and it appears that when the host is behind a NAT it sets the IP address in the version message to either 0.0.0.0 or 127.0.0.1. Not sure if that is always true.\r\n\r\nNo, it sets it to its current discovered address.  If you are behind NAT and don't have discovery (e.g. via UPNP or configuration) then you won't have a discovered address, and so you'll get 0.0.0.0.  Because of security concerns with the upnp library we were using we've currently got it off by default, but I hope to have discovery come back in the future.\r\n\r\n> I am operating under the assumption that most nodes that do not accept incoming connections do so because they are behind NATs and therefore would not be added to the tried table\r\n\r\nEliminating inbound connections is a good way to mitigate a number of DOS vulnerabilities. A standard recommended commercial configuration is to have production nodes which you care about connect _only_ to other somewhat trusted nodes.\r\n\r\n > From a security standpoint IP addresses from address messages are more vulnerable to manipulation than IP addresses from incoming connections.\r\n\r\nIf we want to privilege address announcements that also happen to come from the host telling us about them (e.g. using a lower last seen time for them), I think that would be completely reasonable. But there is no need to infer an addr announcement from the version message.\r\n\r\nIn the future we'll likely want to carry additional metadata with announcements, which might not be possible to just infer from version handshakes in any case. (Already we can't get port numbers that way).\r\n"}}
{"actor":{"display_login":"gmaxwell"}}