Backport Bitcoin PR#8085: p2p: Begin encapsulation#1537
Merged
UdjinM6 merged 33 commits intodashpay:v0.12.2.xfrom Jul 21, 2017
Merged
Backport Bitcoin PR#8085: p2p: Begin encapsulation#1537UdjinM6 merged 33 commits intodashpay:v0.12.2.xfrom
UdjinM6 merged 33 commits intodashpay:v0.12.2.xfrom
Conversation
This will eventually solve a circular dependency
This behavior seems to have been quite racy and broken. Move nLocalHostNonce into CNode, and check received nonces against all non-fully-connected nodes. If there's a match, assume we've connected to ourself.
These are in-turn passed to CNode at connection time. This allows us to offer different services to different peers (or test the effects of doing so).
CConnman then passes the current best height into CNode at creation time. This way CConnman/CNode have no dependency on main for height, and the signals only move in one direction. This also helps to prevent identity leakage a tiny bit. Before this change, an attacker could theoretically make 2 connections on different interfaces. They would connect fully on one, and only establish the initial connection on the other. Once they receive a new block, they would relay it to your first connection, and immediately commence the version handshake on the second. Since the new block height is reflected immediately, they could attempt to learn whether the two connections were correlated. This is, of course, incredibly unlikely to work due to the small timings involved and receipt from other senders. But it doesn't hurt to lock-in nBestHeight at the time of connection, rather than letting the remote choose the time.
…rather than a std::function to eliminate std::function overhead
tgflynn
approved these changes
Jul 18, 2017
|
Note above approval was only intended for 1 commit, not whole PR. |
tgflynn
reviewed
Jul 18, 2017
tgflynn
reviewed
Jul 18, 2017
tgflynn
reviewed
Jul 18, 2017
| stats.nodeStateStats.nCommonHeight = -1; | ||
| stats.fNodeStateStatsAvailable = false; | ||
| pnode->copyStats(stats.nodeStats); | ||
| stats.nodeStats = nodestats; |
There was a problem hiding this comment.
This looks strange. Why recopy the same data multiple times ?
Author
There was a problem hiding this comment.
For better modularity and readability, I presume. Instead of iterating through all vNodes holding a lock, a method has been introduced to get copy of stats. This can potentially allow to completely encapsulate how stats are stored and access them only through method.
Although this code is less efficient,
- amount of data copied is small,
- code is not on critical path.
UdjinM6
approved these changes
Jul 19, 2017
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
looks good 👍
tested (local wallet, not mn), ACK
tgflynn
reviewed
Jul 21, 2017
| void CConnman::ForEachNode(std::function<void(CNode* pnode)> func) | ||
| { | ||
| LOCK(cs_vNodes); | ||
| for (auto&& node : vNodes) |
|
utACK (all commits) |
UdjinM6
added a commit
to UdjinM6/dash
that referenced
this pull request
Aug 21, 2017
UdjinM6
added a commit
that referenced
this pull request
Aug 23, 2017
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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 backport of Bitcoin PR bitcoin#8085.
Brace yourself, this is the biggest PR (33 commits) I'm sending in in networking refactoring series and most important one.
All 33 commits are relatively small, so I'd advise to review them one by one, rather than just overall file changes.
When merging this PR, please don't squash commits, just use normal merge. Otherwise it will be difficult to understand these changes when searching in history later.
The original PR description follows.
This work creates CConnman. The idea is to begin moving data structures and functionality out of globals in net.h and into an instanced class, in order to avoid side-effects in networking code. Eventually, an (internal) api begins to emerge, and as long as the conditions of that api are met, the inner-workings may be a black box.
For now (for ease), a single global CConnman is created. Down the road, the instance could be passed around instead. Also, CConnman should be moved out of net.h/net.cpp, but that can be done in a separate move-only-ish step.
A few todos remain here:
there areif(g_connman)'s everywhere. A few places should assume that a connman is running (or it should be passed around). For example, ProcessMessages.GetRelayTx/RelayTransaction are awkward. Where should mapRelay live? Should it be dealt with directly rather than hidden inside of relay functions? (rebased on top of Defer inserting into maprelay until just before relaying. bitcoin/bitcoin#8082)How to deal with CConnman parameters? Continue adding them to Start(), or have it parse the program options itself? (Created CConnman::Options)RPC needs a new error for dealing with the "p2p not enabled" case.Todos for a follow-up PR:
Drop StartNode/StopNode and just use CConnman::Stop/Start directly (Need to make sure that MapPort can be moved directly into Init without side-effects)Create CConnmanOptions to reduce the verbosity of CConnman::Start()