{"action":"created","issue":{"url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/issues/74","repository_url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited","labels_url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/issues/74/labels{/name}","comments_url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/issues/74/comments","events_url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/issues/74/events","html_url":"https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/74","id":171331025,"number":74,"title":"[WIP] Net:Fix for addnode bug ref #65","user":{"login":"Justaphf","id":6402604,"avatar_url":"https://avatars.githubusercontent.com/u/6402604?v=3","gravatar_id":"","url":"https://api.github.com/users/Justaphf","html_url":"https://github.com/Justaphf","followers_url":"https://api.github.com/users/Justaphf/followers","following_url":"https://api.github.com/users/Justaphf/following{/other_user}","gists_url":"https://api.github.com/users/Justaphf/gists{/gist_id}","starred_url":"https://api.github.com/users/Justaphf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Justaphf/subscriptions","organizations_url":"https://api.github.com/users/Justaphf/orgs","repos_url":"https://api.github.com/users/Justaphf/repos","events_url":"https://api.github.com/users/Justaphf/events{/privacy}","received_events_url":"https://api.github.com/users/Justaphf/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-08-16T05:57:33Z","updated_at":"2016-08-16T13:16:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/pulls/74","html_url":"https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/74","diff_url":"https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/74.diff","patch_url":"https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/74.patch"},"body":"Marking as WIP pending comments and answers to questions below.\r\n\r\nFixes the following (Issues #65):\r\n\r\n1. Specifying less than `maxoutconnections` of `addnode` options in bitcoin.conf or on the command line prevents RPC calls to `addnode <node> add` from being able to connect to more than the total number of originally configured `addnode` connections simultaneously.  In the extreme case, if no `addnode` connections were configured at startup, the user would never be able to successfully connect to any outbound `addnode` nodes as the tracking semaphore was initialized to a count of 0, which would put all grant requests into an eternal wait state.\r\nTEST SCENARIO: Do not configure any `addnode`s in bitcoin.conf or on the command line, then attempt to add a new outbound connection via `addnode <node> add`.\r\nREPRODUCTION RESULT: The node will be added to the `vAddedNodes` list and `getpeerinfo` will return that node as being in the list, however it will never attempt to connect to the node as the semaphore grant will never succeed.\r\nFIX RESULT: The node will be added to the `vAddedNodes` list and `getpeerinfo` will return that node as being in the list.  Additionally, provided `maxoutconnections` has not already been exceeded and the test node is up and accepting connections, the RPC call should succeed at adding an outbound connection to the specified node.\r\n\r\n2. Specifying too many `addnode` entries in bitcon.conf or on the command line will cause incoming connections to be limited further than intended.  Only up to `maxoutconnections` concurrent outbound connections are possible, however specifying more than `maxoutconnections` limits the total number of inbound connections based on the total `addnode` options configured in bitcon.conf or on the command line.  In the extreme case, if more `addnode` options are configured than `maxconnections - maxoutconnections`, then all inbound connections will be blocked.\r\nTEST SCENARIO: Specify maxconnections=12, maxoutconnections=4, and 8 `addnode` connections in bitcoin.conf.\r\nREPRODUCTION RESULT: You should end up with between 4 and 12 outbound connections, depending on if the configured `addnode` options are valid and can connect.  You also should never get an inbound connection.\r\nFIX RESULT: You should end up with between 4 and 8 outbound connections, depending on if the configured `addnode` options are valid and can connect.  You should also end up with up to 4 inbound connections.\r\n\r\nRemaining Issues/Questions:\r\n\r\n1. ISSUE: Both `maxconnections` and `maxoutconnections` can still be violated by use of `connect`, `connect-thinblock`, and RPC calls to `addnode <node> onetry` as none of these outbound connections are tracked.  These outbound connection methods do not check against the maximum connection constraints (no use of semaphores), and the inbound connections are unaware of these connections so also don't take them into account when determining if there is room to accept a new inbound connections.\r\nNot sure the best fix for this, short of adding tracking for these different types similar to how addnodes works.  Also not sure what the priority on this fix would be.\r\n\r\n2. QUESTION: What is the policy on excessive comments? I tend to comment heavily as an aide to my own understanding of the code and general clarification.  Provided the comments are accurate I don't see an issue with keeping them, and I had one commit specifically for clarification comments.\r\n\r\n3. QUESTION: What's the policy for determining if the automated tests need to be updated for a given fix?  If updates are needed, what's the procedure there?\r\n\r\n4. QUESTION: What is the intended behavior for the number of outbound connections?  Currently `maxoutconnections` is doubled since we are tracking seeded outbound nodes separately from `addnode` configured outbound nodes.  Seems like there should either be separate configuration options for each, or `addnode`s should be given priority over seeded nodes and we only attempt to seed outbound connections if less than `maxoutconnections` are configured as `addnode`s or we have already iterated through the `vAddedNodes` list and less than `maxoutconnections` would connect."},"comment":{"url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/issues/comments/240097980","html_url":"https://github.com/BitcoinUnlimited/BitcoinUnlimited/pull/74#issuecomment-240097980","issue_url":"https://api.github.com/repos/BitcoinUnlimited/BitcoinUnlimited/issues/74","id":240097980,"user":{"login":"ptschip","id":12862928,"avatar_url":"https://avatars.githubusercontent.com/u/12862928?v=3","gravatar_id":"","url":"https://api.github.com/users/ptschip","html_url":"https://github.com/ptschip","followers_url":"https://api.github.com/users/ptschip/followers","following_url":"https://api.github.com/users/ptschip/following{/other_user}","gists_url":"https://api.github.com/users/ptschip/gists{/gist_id}","starred_url":"https://api.github.com/users/ptschip/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ptschip/subscriptions","organizations_url":"https://api.github.com/users/ptschip/orgs","repos_url":"https://api.github.com/users/ptschip/repos","events_url":"https://api.github.com/users/ptschip/events{/privacy}","received_events_url":"https://api.github.com/users/ptschip/received_events","type":"User","site_admin":false},"created_at":"2016-08-16T13:16:43Z","updated_at":"2016-08-16T13:16:43Z","body":"QUESTION: What is the policy on excessive comments? I tend to comment heavily as an aide to my own understanding of the code and general clarification. Provided the comments are accurate I don't see an issue with keeping them, and I had one commit specifically for clarification comments.\r\n\r\nI've always found the code to be poorly commented.  As long as comments are not huge paragraphs and get in the way of reading the code, I think more comments are better.  I find myself sometimes not commenting enough.\r\n\r\nQUESTION: What's the policy for determining if the automated tests need to be updated for a given fix? If updates are needed, what's the procedure there?  The tests and automated regression tests should all work after every PR is submitted, however they generally don't work on Native Windows very well or at all.  Over at Bitcoin Core they have the automated Travis system which compiles and executes all the tests on (windows and linux) and python regression tests, on Linux.  We still don't have Travis running but will hopefully have it up in the coming weeks.  That said, yes you have to update the python tests if they need updating.  Sometimes, working on native windows, you might have to code a python fix and push your PR before seeing the result.  I've only run into that problem once but it can be a pain.  There are two problems running python regression on windows..one is that some of the scripts just hang and the other is that windows does not have \"dbm\" which is used somewhat frequently in the scripts.\r\n\r\nQUESTION: What is the intended behavior for the number of outbound connections? Currently maxoutconnections is doubled since we are tracking seeded outbound nodes separately from addnode configured outbound nodes. Seems like there should either be separate configuration options for each, or addnodes should be given priority over seeded nodes and we only attempt to seed outbound connections if less than maxoutconnections are configured as addnodes or we have already iterated through the vAddedNodes list and less than maxoutconnections would connect.\r\n\r\nYes, I think there should be a maxaddnodeconnections as well. @sickpig said he might be working on that, you can ping him if you want.  I would say that addnode should be given priority over outbound connections if there are not enough slots, which would be an unusual edge case, so not a big deal in my book, but yes I would say give priority to addnodes because that's what the user really wants.  Anybody think otherwise?"}}
{"actor":{"display_login":"ptschip"}}