[Net] Split resolve out of connect and add addnode parallel outbound connections limit #2587
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Added btc 9953, 10234 and 11363 as well and updated PR description. Ready now. |
furszy
requested review from
Fuzzbawls and
random-zebra
and removed request for
Fuzzbawls
furszy
force-pushed
the
2021_netbase_split_connect
branch
from
c6f95f4
to
c87b00e
Compare
|
rebased, conflicts solved. |
furszy
force-pushed
the
2021_netbase_split_connect
branch
from
c87b00e
to
99fb9bf
Compare
|
had to dismiss the approval and rebase it once more, it was having conflicts on the release-notes file. |
furszy
force-pushed
the
2021_netbase_split_connect
branch
from
99fb9bf
to
848d2bb
Compare
Adaptation of btc@50bd12ce0c49e574a5baf1a8df3a667810c6ad1e. Previously addnodes were in competition with outbound connections for access to the eight outbound slots. One result of this is that frequently a node with several addnode configured peers would end up connected to none of them, because while the addnode loop was in its two minute sleep the automatic connection logic would fill any free slots with random peers. This is particularly unwelcome to users trying to maintain links to specific nodes for fast block relay or purposes. Another result is that a group of nine or more nodes which are have addnode configured towards each other can become partitioned from the public network. This commit introduces a new limit of eight connections just for addnode peers which is not subject to any of the other connection limitations (including maxconnections). The choice of eight is sufficient so that under no condition would a user find themselves connected to fewer addnoded peers than previously. It is also low enough that users who are confused about the significance of more connections and have gotten too copy-and-paste happy will not consume more than twice the slot usage of a typical user. Any additional load on the network resulting from this will likely be offset by a reduction in users applying even more wasteful workaround for the prior behavior. The retry delays are reduced to avoid nodes sitting around without their added peers up, but are still sufficient to prevent overly aggressive repeated connections. The reduced delays also make the system much more responsive to the addnode RPC. Ban-disconnects are also exempted for peers added via addnode since the outbound addnode logic ignores bans. Previously it would ban an addnode then immediately reconnect to it. A minor change was also made to CSemaphoreGrant so that it is possible to re-acquire via an object whos grant was moved.
Also adds a comment about the netgroup exclusion behavior.
ConnectSocketByName handled resolves as necessary, obscuring the connection process. With them separated, each can be handled asynchronously. Also, since proxies must be considered now anyway, go ahead and eliminate the ConnectSocket wrapper and use ConnectSocketDirectly... directly. Adapting btc@2416dd7cc94e765efbbf9069dbf0f6fb71404ebb
This was added in order to help OpenNetworkConnection avoid creating a connection that it would end up aborting. It was necessary because resolving was done as part of the connection process. Now that resolving is separated from connecting, this case is detected before the connection is attempted.
Adaptation of btc@b887676e1b86ce03181b5876cf6203d617750d0a
Also, check for the correct error during socket creation
We use select in ConnectSocketDirectly, so this check needs to happen before that. IsSelectableSocket will not be relevant after upcoming changes to remove select.
…nections This allows const references to be passed around, making it clear where the socket may and may not be invalidated.
Plus corrected p2p_disconnect_ban.py listbanned number.
We previously would block waiting for a CSemaphoreGrant in ThreadOpenAddedConnections, when we did not need to. This would block as the posts in CConnman shutdown were both to the wrong semaphore and in the wrong location.
furszy
force-pushed
the
2021_netbase_split_connect
branch
from
848d2bb
to
d231be4
Compare
Fuzzbawls
approved these changes
Nov 22, 2021
furszy
added a commit
that referenced
this pull request
Dec 7, 2021
e1d12d3 Add Clang thread safety analysis annotations (furszy) 5716940 net: Add missing locks in net.{cpp,h} (furszy) 8c02b59 net: simplify fRelayTxes flag processing (furszy) 71667df remove unused IsArgSet check (Marko Bencun) 729c63d add m_added_nodes to connman options (Marko Bencun) 8c8ad18 [net] Fix use of uninitialized value in getnetworkinfo(const JSONRPCRequest& request) (practicalswift) a13b7c9 Add vConnect to CConnman::Options (Marko Bencun) 987342e ActiveMasternode: fix not initialized socket. (furszy) 8d788ba add SeedNodes to CConnman::Options (Marko Bencun) d9e91ff add Binds, WhiteBinds to CConnman::Options (Marko Bencun) 41c89af add WhitelistedRange to CConnman::Options (Marko Bencun) Pull request description: More groundwork for the LLMQ sessions connections work, built on top of #2586 and #2587 (starts in 10efb72). Focused on cleaning the connman init/start by decoupling the command line arguments. Backported PRs list: * bitcoin#10467. * bitcoin#10496. * bitcoin#10596. * bitcoin#10977. * bitcoin#11301. * bitcoin#11744 (partially, without the outbound members changes as we don't have them). ACKs for top commit: random-zebra: utACK e1d12d3 Fuzzbawls: ACK e1d12d3 Tree-SHA512: 81a1ab7a1e7f487330354631ee728be9ec78223fe4978c8b9c97b7fbc8d2bfe4f4ea9e88ac4a3d1f0553f7adad871c81261b1a7545bae710a4e3200b8a5031d7
random-zebra
added a commit
that referenced
this pull request
Dec 17, 2021
…+ enable p2p_leak.py test 56f07da net_processing: 'SENDADDRV2' msg can be received before a verack. (furszy) 2f5339c test: update and enable p2p_leak.py (furszy) 475b1c6 net: require a verack before responding to anything else (Cory Fields) 8a6c72b net: correctly ban before the handshake is complete (furszy) Pull request description: More updates, corrections and test coverage to the network layer for the LLMQ MNs connections work (deep rabbit hole..). Built on top of #2587. PR starts in cd00e31, focused on: 1) Correctly ban before the handshake is complete (c45b9fb). 2) Require a `verack` before responding to anything else (cbfc5a6). 3) Update and enable `p2p_leak.py` functional test (check commit, the functional test framework was too ahead of it). 4) Move `SENDADDRV2` msg processing so it can be processed before a verack. ACKs for top commit: random-zebra: ACK 56f07da Fuzzbawls: ACK 56f07da Tree-SHA512: b23a6003c6b815f797bd77a646023632d9087be530ad1651255005f9aa5764f9c91ce9de97eac9c9180384442fceeb4ff41ee9b81500246e01e324dcd20f52f6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
More net updates from upstream, this time focused on:
addnodefunctionality will have its own connections slots, there will be no interference betweenaddnodeconnections and automatic outbound connections.Adapted the following PRs from upstream: