[Net] Pre-requirements for network encapsulation #1704
Merged
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
Locking for each operation here is unnecessary, and solves the wrong problem. Additionally, it introduces a problem when cs_vNodes is held in an owning class, to which invididual CNodeRefs won't have access. These should be weak pointers anyway, once vNodes contain shared pointers. Rather than using a refcounting class, use a 3-step process instead. 1. Lock vNodes long enough to snapshot the fields necessary for comparing 2. Unlock and do the comparison 3. Re-lock and mark the resulting node for disconnection if it still exists
- Ban/Unban/ClearBan call uiInterface.BannedListChanged() as necessary - Ban/Unban/ClearBan sync to disk if the operation is user-invoked - Mark node for disconnection automatically when banning - Lock cs_vNodes while setting disconnected - Don't spin in a tight loop while setting disconnected
ConnectNode is intended to be an internal function, use OpenNetworkConnection instead.
For when you need to keep track of the last N items you've seen, and can tolerate some false-positives. Rebased-by: Pieter Wuille <[email protected]>
Use a probabilistic bloom filter to keep track of which addresses we think we have given our peers, instead of a list. This uses much less memory, at the cost of sometimes failing to relay an address to a peer-- worst case if the bloom filter happens to be as full as it gets, 1-in-1,000. Measured memory usage of a full mruset setAddrKnown: 650Kbytes Constant memory usage of CRollingBloomFilter addrKnown: 37Kbytes. This will also help heap fragmentation, because the 37K of storage is allocated when a CNode is created (when a connection to a peer is established) and then there is no per-item-remembered memory allocation. I plan on testing by restarting a full node with an empty peers.dat, running a while with -debug=addrman and -debug=net, and making sure that the 'addr' message traffic out is reasonable. (suggestions for better tests welcome)
While CBloomFilter is usually used with an explicitly set nTweak, CRollingBloomFilter is only used internally. Requiring every caller to set nTweak is error-prone and redundant; better to have the class handle that for you with a high-quality randomness source. Additionally when clearing the filter it makes sense to change nTweak as well to recover from a bad setting, e.g. due to insufficient randomness at initialization, so the clear() method is replaced by a reset() method that sets a new, random, nTweak value.
Nodes can have divergent policies on which transactions they will accept and relay. This can cause you to repeatedly request and reject the same tx after its inved to you from various peers which have accepted it. Here we add rolling bloom filter to keep track of such rejections, clearing the filter every time the chain tip changes. Credit goes to Alex Morcos, who created the patch that this code is based on. Original code by Peter Todd. Refactored to not construct the filter at startup time by Pieter Wuille.
This avoids that premature return in the condition that a new chain is initialized results in NULL pointer errors due to recentReject not being constructed. Also add assertions where it is used.
We used to have a trickle node, a node which was chosen in each iteration of the send loop that was privileged and allowed to send out queued up non-time critical messages. Since the removal of the fixed sleeps in the network code, this resulted in fast and attackable treatment of such broadcasts. This pull request changes the 3 remaining trickle use cases by random delays: * Local address broadcast (while also removing the the wiping of the seen filter) * Address relay * Inv relay (for transactions; blocks are always relayed immediately) The code is based on older commits by Patrick Strateman.
Mruset setInventoryKnown was reduced to a remarkably small 1000 entries as a side effect of sendbuffer size reductions in 2012. This removes setInventoryKnown filtering from merkleBlock responses because false positives there are especially unattractive and also because I'm not sure if there aren't race conditions around the relay pool that would cause some transactions there to be suppressed. (Also, ProcessGetData was accessing setInventoryKnown without taking the required lock.)
Previously this logic could erroneously filter a MSG_BLOCK inventory message.
Previously this logic could erroneously filter a MSG_BLOCK inventory message.
Tests if addresses are online or offline by briefly connecting to them. These short lived connections are referred to as feeler connections. Feeler connections are designed to increase the number of fresh online addresses in tried by selecting and connecting to addresses in new. One feeler connection is attempted on average once every two minutes. This change was suggested as Countermeasure 4 in Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman, Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report 2015/263. March 2015.
We should learn about new peers via address messages. An inbound peer connecting to us tells us nothing about its ability to accept incoming connections from us, so we shouldn't assume that we can connect to it based on this. The vast majority of nodes on the network do not accept incoming connections, adding them will only slow down the process of making a successful connection in the future. Nodes which have configured themselves to not announce would prefer we not violate their privacy by announcing them in GETADDR responses.
`nMinPingUsecTime` was left uninitialized in CNode. The correct initialization for a minimum-until-now is int64_t's max value, so initialize it to that. Thanks @MarcoFalke for noticing.
random-zebra
force-pushed
the
2020_net-refactor-prereqs
branch
from
2ed5a59 to
6f41b3e
Compare
furszy
reviewed
Jun 27, 2020
There was a problem hiding this comment.
Overall looking pretty good, nice work ☕ .
Not really needed here but would be nice to add bitcoin#8944 and bitcoin/bitcoin@d74e352 simple back ports, those are directly following bitcoin#8282 feeler connections.
|
Great job. ACK 6f41b3e and merging... |
21 tasks
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.
This is a culmination of several upstream PRs touching the P2P/Networking code, and resulting in a state just prior to the P2P/Network encapsulation, which will be it's own PR.
Backported upstream PRs included here: