p2p: Begin encapsulation

theuni commented at 9:04 am on May 22, 2016: member

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 are if(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 #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:

Actually create connman.h/cpp and move the class there.
Pass CConnman into QT similar to the wallet
~~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)~~
Pass CChainParams into CConnman and pass it around
~~Create CConnmanOptions to reduce the verbosity of CConnman::Start()~~
Deal with fDiscover/fListen/etc in CConnman

laanwj added the label P2P on May 22, 2016

NicolasDorier commented at 4:04 am on May 23, 2016: contributor

nit typo: “SocketSendData resurns written size”

in src/net.h: in 46cf6754cf outdated

489@@ -490,12 +490,17 @@ class CNode
490     CNode(const CNode&);
491     void operator=(const CNode&);
492 
493+    uint64_t nLocalHostNonce;

sipa commented at 2:50 pm on May 24, 2016:

This won’t work. It needs to be a ConnMan-scoped variable, not a CNode-scoped one, as the nonce will come back through another link than the one we sent it through.

theuni commented at 4:13 pm on May 24, 2016:

@sipa See https://github.com/bitcoin/bitcoin/pull/8085/commits/46cf6754cf6ac7bab76fb61ad9494741de235f6c#diff-9a82240fe7dfe86564178691cc57f2f1R351.

I started by simply moving this to CConnman as you suggest, but it’s racy and dependent on the optimistic send succeeding (nonce is set in one thread, tested in another). With this change, I believe we now guard against rapid-fire connections that may connect to self, which would have been missed before.

sipa commented at 4:17 pm on May 24, 2016:

Nevermind, indeed.

sipa commented at 4:37 pm on May 24, 2016: member

#8082 moves mapRelay from net to main. I think it indeed belongs with node processing code and not in base networking code.

theuni commented at 4:48 pm on May 24, 2016: member

@sipa Indeed, that’s definitely the correct approach and the change here can be dropped.

theuni commented at 1:30 am on May 26, 2016: member

@laanwj I pushed a bunch of new commits to address the review you gave me. The commit history doesn’t really make sense now, though, as there’s a good bit of reverting. Please let me know if you’d like me to squash down to a more logical history for easier review, or continue to pile up changes and squash at the end.

theuni force-pushed on May 27, 2016

theuni commented at 5:34 am on May 27, 2016: member

Rebased to master and squashed down changes so that the history makes sense again.

Though there’s still a good bit left to do, I think this is a good stopping point. I’ll avoid making further changes here unless requested.

theuni force-pushed on Jun 6, 2016

theuni commented at 9:21 pm on June 6, 2016: member

Rebased to master after #8082, and dropped that commit here. Should be ready for review.

in src/init.cpp: in f86d0bebcb outdated

69@@ -69,6 +70,7 @@ static const bool DEFAULT_REST_ENABLE = false;
70 static const bool DEFAULT_DISABLE_SAFEMODE = false;
71 static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false;
72 
73+std::shared_ptr<CConnman> g_connman;

kazcw commented at 7:12 pm on June 10, 2016:

I don’t see g_connman requiring any refcounting, could this be a plain unique_ptr?

theuni commented at 7:22 pm on June 10, 2016:

This was originally passed around differently, but refactored to essentially be a unique_ptr. So yes :)

Though it’s worth mentioning that the global is only temporary until the rpc/qt interfaces learn how to accept/maintain an instance. So it’s probably even more important to not encourage any sharing semantics.

Will change.

in src/net.cpp: in f86d0bebcb outdated

1937+NodeId CConnman::GetNewNodeId()
1938+{
1939+    return nLastNodeId.fetch_add(1, std::memory_order_relaxed);
1940+}
1941+
1942+bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, std::string& strNodeError, Options connOptions)

kazcw commented at 7:23 pm on June 10, 2016:

Lifetime safety is more robust without the existence of constructed-not-started and stopped-not-destructed twilight states. What do you think about doing the Start/Stop stuff in the ctor/dtor – would the stuff that AppInit2 currently does between construction and Start make sense to put into the CConnman::Options?

theuni commented at 7:55 pm on June 10, 2016:

The reason for start/stop doesn’t really become obvious until the async behavior comes in. As a next step, all of the connecting/resolving behavior will be replaced with async events handled via callbacks. It’s necessary to be able to have some basic connman functionality before the event thread spins up (or in case it’s never going to). The most straightforward example is that we’ll want to stop app startup if all bind()s fail, before spawning any connection threads.

As a better example, you can see that none of the tests require that Start() has been called.

kazcw commented at 8:28 pm on June 10, 2016:

I’d be more comfortable with a builder doing those binds or something – it just seems like it would be easy for a future change to introduce an unchecked dependency on the CConnman state, that could make for subtle and unpredictable bugs. But this is a complex change you’re certainly more familiar with than I, so ACK however you decide to ensure safety against that kind of case.

theuni commented at 8:39 pm on June 10, 2016:

Roger. I see your point and agree, I’m just trying to avoid overthinking this until the async stuff is added. Once it’s all hooked up, maybe we can switch to a model where Start() returns some kind of object that guarantees a certain state that CConnman does not.

The alternative would be to move out things that don’t require a running state (addrman/bandb as examples) and let them have separate life-cycles, but I’d like to do that as a next step once all callers understand that those things aren’t global.

in src/net.h: in f86d0bebcb outdated

115+    void Stop();
116+    bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false);
117+    bool OpenNetworkConnection(const CAddress& addrConnect, CSemaphoreGrant *grantOutbound = NULL, const char *strDest = NULL, bool fOneShot = false);
118+    bool CheckIncomingNonce(uint64_t nonce);
119+
120+    bool ForEachNode(std::function<bool(CNode* pnode)> func);

kazcw commented at 7:34 pm on June 10, 2016:

Nit: std::function indirection is fairly heavyweight, templating these would make the functors inlineable.

theuni commented at 7:57 pm on June 10, 2016:

Good point.

in src/main.cpp: in f86d0bebcb outdated

2919@@ -2927,7 +2920,7 @@ static void NotifyHeaderTip() {
2920  * or an activated best chain. pblock is either NULL or a pointer to a block
2921  * that is already loaded (to avoid loading it again from disk).
2922  */

kazcw commented at 7:43 pm on June 10, 2016:

Nit: could use a comment documenting connman nullability.

theuni commented at 8:01 pm on June 10, 2016:

Agreed, will do. In case it’s not clear, the intention is to allow us to avoid skipping the network altogether for utility functions. Beyond the startup stuff where it’s currently skipped, in the future it may be interesting to do things like on-the-fly reindexing without having to shut down.

in src/main.cpp: in f86d0bebcb outdated

4429+
4430+    // Relay to a limited number of other nodes
4431+    // Use deterministic randomness to send to the same nodes for 24 hours
4432+    // at a time so the addrKnowns of the chosen nodes prevent repeats
4433+    static uint64_t salt0 = 0, salt1 = 0;
4434+    while (salt0 == 0 && salt1 == 0) {

kazcw commented at 7:48 pm on June 10, 2016:

There’s a newer version of this initialization in master from #8173, the commit is 8884830.

theuni commented at 8:02 pm on June 10, 2016:

Thanks, I’ll note this for the next rebase round.

kazcw commented at 8:42 pm on June 10, 2016: contributor

utACK the src/. changes at f86d0be (with minor notes in comments) [didn’t review the subdirectory stuff thoroughly; I’m less familiar with that code]

theuni commented at 9:14 pm on June 10, 2016: member

@kazcw Thanks for the excellent review! I’ll pick this back up in ~2 weeks. Obviously if that’s going to stall progress on lock refactors too much, go ahead with them and I’ll cope with it here.

in src/net.h: in 5b81b0aef5 outdated

100+    {
101+        uint64_t nLocalServices = 0;
102+        int nMaxConnections = 0;
103+        int nMaxOutbound = 0;
104+        int nBestHeight = 0;
105+        CClientUIInterface* interface = nullptr;

JeremyRubin commented at 7:10 pm on June 14, 2016:

nit: unclear why interface is in options when it doesn’t really seem to be an option. Perhaps having a struct Context to hold state such as interface, scheduler, and threadgroup would be a better encapsulation.

theuni commented at 1:25 am on June 15, 2016:

This will eventually hold pointers to several structures, chainparams for example.

It may be reasonable to split it into a set of truly immutable options, and stateful app pointers.

kazcw commented at 7:51 pm on June 15, 2016:

ChainParams seems like it could qualify as an option. In the long term I think it would make more sense for the UI to connect to the various backend signals rather than the net layer having any knowledge of the UI, but that change seems outside the scope of this PR.

in src/rpc/net.cpp: in 9def7c0bb0 outdated

318+        BOOST_FOREACH(const std::string& strAddNode, laddedNodes) {
319             if (strAddNode == strNode)
320             {
321-                laddedNodes.push_back(strAddNode);
322+                laddedNodes.assign(1, strAddNode);
323                 break;

JeremyRubin commented at 8:10 pm on June 14, 2016:

note: This section is a bit more than just a code move, but it seems to have no functional change. (the couple lines around this section)

in src/net.cpp: in a01fa6c5a2 outdated

2520@@ -2531,6 +2521,50 @@ void CNode::EndMessage(const char* pszCommand) UNLOCK_FUNCTION(cs_vSend)
2521     LEAVE_CRITICAL_SECTION(cs_vSend);
2522 }
2523 
2524+bool CConnman::ForEachNode(std::function<bool(CNode* pnode)> func)

JeremyRubin commented at 11:21 pm on June 14, 2016:

Nit: I would rename ForEachNode as ForEachNodeContinueIf or something to signify that the return value being true is critical. I would then make a version of ForEachNode that does not check for the return value.

This is more consistent* with other “foreach” API’s generally, eg, boost foreach or javascript foreach iterate over all elements
- although boost foreach does allow break
It is unclear if ForEachNodeContinueIf is/will be used anywhere in the codebase, but it could introduce bugs (say, if false is somehow returned by buggy code elsewhere or later), so it’s probably better to not have a check that is never used.

theuni commented at 1:17 am on June 15, 2016:

Ack. I meant to document these, but self-doc would be better.

The intention is to use these internally later, for example to replace the FindNode’s which iterate and early-return when possible, to avoid holding the lock longer than necessary.

But since those changes aren’t part of this pull, I think that simply dropping the bool return and reintroducing it as ForEachNodeContinueIf later is the way to go.

JeremyRubin commented at 10:37 pm on June 15, 2016:

I’ll take care of it…

in src/wallet/wallet.cpp: in a01fa6c5a2 outdated

1341@@ -1342,8 +1342,15 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)
1342     {
1343         if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) {
1344             LogPrintf("Relaying wtx %s\n", GetHash().ToString());
1345-            RelayTransaction((CTransaction)*this);
1346-            return true;
1347+            if (connman) {
1348+                CInv inv(MSG_TX, GetHash());
1349+                connman->ForEachNode([&inv](CNode* pnode)

JeremyRubin commented at 11:26 pm on June 14, 2016:

Nit: appears correct, but isn’t this the same code as in RelayTransaction?

theuni commented at 1:20 am on June 15, 2016:

Yes, but it’s simple enough that I didn’t bother factoring it out. No reason not to, except that there’s not an obvious place to stick it. On Jun 14, 2016 19:26, “Jeremy Rubin” notifications@github.com wrote:

In src/wallet/wallet.cpp #8085 (review):
@@ -1342,8 +1342,15 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman) { if (GetDepthInMainChain() == 0 && !isAbandoned() && InMempool()) { LogPrintf(“Relaying wtx %s\n”, GetHash().ToString());
       RelayTransaction((CTransaction)*this);
       return true;
       if (connman) {
           CInv inv(MSG_TX, GetHash());
           connman->ForEachNode([&inv](CNode* pnode)
Nit: appears correct, but isn’t this the same code as in RelayTransaction?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/8085/files/a57b3f21e3228f838382e7510dc4e8f2bd023237..a01fa6c5a2e5884af64c0153dbfbd6a848128129#r67076355, or mute the thread https://github.com/notifications/unsubscribe/AAZdE-sEqwxHrvI3wdUKtoHlUCgD1RM2ks5qLzi2gaJpZM4Ij8mn .

in src/net.cpp: in 148163c9b1 outdated

2337@@ -2337,6 +2338,16 @@ uint64_t CConnman::GetLocalServices() const
2338     return nLocalServices;
2339 }
2340 
2341+void CConnman::SetBestHeight(int height)
2342+{
2343+    nBestHeight.store(height, std::memory_order_release);

JeremyRubin commented at 2:54 pm on June 15, 2016:

I’m not sure this actually presents a bug, but it might at least exhibit some undesired behavior.

This should replace the value only if it is greater than the previous value, unless it is desirable to roll back (but this seems like a different case?).

~~This may also not play well with out of order headers.~~ (ignore, headers aren’t processed here/should be in order)

In any case, I’m not sure this is a consensus critical thing, but it could be the source of future bugs or further identity leaks.

This (might) also be fixed by moving the call to SetBestHeight inside of Lock(cs_main) in ActivateBestChain. Would also be reasonable to just document (or self-doc) it a bit better about what you are expecting this to do.

sipa commented at 1:11 pm on September 8, 2016:

I don’t think this matters much.

in src/net.cpp: in cb9c81ee78 outdated

1981@@ -1976,7 +1982,7 @@ bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, ui
1982         }
1983     }
1984 
1985-    uiInterface.InitMessage(_("Loading banlist..."));
1986+    clientInterface->InitMessage(_("Loading banlist..."));

JeremyRubin commented at 3:02 pm on June 15, 2016:

may want to null check…

in src/net.cpp: in cb9c81ee78 outdated

1966@@ -1962,7 +1967,8 @@ bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, ui
1967 
1968     SetBestHeight(nBestHeightIn);
1969 
1970-    uiInterface.InitMessage(_("Loading addresses..."));
1971+    clientInterface = interfaceIn;
1972+    clientInterface->InitMessage(_("Loading addresses..."));

JeremyRubin commented at 3:02 pm on June 15, 2016:

may want to null check…

theuni commented at 7:58 am on June 28, 2016:

Yep, thanks.

in src/init.cpp: in 2258f0730e outdated

1423@@ -1423,9 +1424,14 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
1424     if (GetBoolArg("-listenonion", DEFAULT_LISTEN_ONION))
1425         StartTorControl(threadGroup, scheduler);
1426 
1427+    Discover(threadGroup);
1428+
1429+    // Map ports with UPnP
1430+    MapPort(GetBoolArg("-upnp", DEFAULT_UPNP));

JeremyRubin commented at 3:10 pm on June 15, 2016:

nit: I think I would separate out the GetBoolArg put it into Options.

Then keep the calls to MapPort into Start/Stop.

seems a little more pragmatic & good for debugging (e.g., can dump a nodes options and know how the node was configured)

theuni commented at 7:58 am on June 28, 2016:

Yes, I plan to move the remaining args into Options as a next step. There are a handful left to go still.

JeremyRubin commented at 3:11 pm on June 15, 2016: contributor

utack, other than the atomic::store problem I marked this refactor seems safe.

in src/rpc/misc.cpp: in f86d0bebcb outdated

88@@ -89,7 +89,8 @@ UniValue getinfo(const UniValue& params, bool fHelp)
89 #endif
90     obj.push_back(Pair("blocks",        (int)chainActive.Height()));
91     obj.push_back(Pair("timeoffset",    GetTimeOffset()));
92-    obj.push_back(Pair("connections",   (int)vNodes.size()));
93+    if(g_connman)
94+        obj.push_back(Pair("connections",   (int)g_connman->GetNodeCount(CConnman::CONNECTIONS_ALL)));

instagibbs commented at 2:00 pm on June 16, 2016:

nit: perhaps just add 0 if !g_connman or some relevant message

in src/net.cpp: in f86d0bebcb outdated

2322+uint64_t CConnman::GetLocalServices() const
2323+{
2324+    return nLocalServices;
2325+}
2326+
2327+void CConnman::SetBestHeight(int height)

kazcw commented at 2:21 pm on June 16, 2016:

nit: I think this could use documentation that the setter requires cs_main to be held to prevent older values from overwriting newer values. Also, I don’t see any other data that would need to be synchronized with the height via acquire/release semantics; if there are no writes before the release that need to be visible side-effects after the acquire, relaxed load/store would be clearer.

JeremyRubin commented at 2:45 pm on June 16, 2016:

@kazcw see #8085 (review).

I don’t even think holding cs_main would help unless the new height is also checked to be > than the old one (suppose an orphaned block gets relayed, this would decrease the height).

kazcw commented at 4:09 pm on June 16, 2016:

Oh, I see… that issue is not introduced by this PR; what’s called GetHeight in main and is not treated as non-decreasing is used in net as a “best height”. Ensuring modification order for nBestHeight matches the order established by cs_main would be sufficient to maintain the current behaviour.

It does seem like it would be better to have a high-water-mark variable protected by cs_main that main code could use to signal when a new best height was reached. I don’t see the need for that change necessarily to be included in this PR though.

JeremyRubin commented at 8:07 pm on June 16, 2016:

I would agree that this PR doesn’t need any behavior change. Perhaps just renaming BestHeight -> LastSeenHeight or something. Also a proper best height is semantically unclear – is it MaxHeight or MaxWorkHeight

theuni commented at 8:37 am on June 28, 2016:

Yea, I don’t believe this deviates from the current behavior much, other than slightly obfuscating the exact current known height, since the value is grabbed at connection time now rather than during the handshake.

Though there’s the advantage of using the accurate cached value based on a push event, rather than requiring a lock-and-pull as before. I don’t think there’s any need to try to replicate that old behavior here.

laanwj added this to the milestone 0.14 on Jun 17, 2016

theuni commented at 8:48 am on June 28, 2016: member

Addressed most (all?) of the comments here, and added some commits.

For future comparison, the current tip is https://github.com/theuni/bitcoin/commit/f96d164f83239e74288ea6a707228be9a4f3ae27. I’ll now work on rebasing to master.

btcdrak commented at 8:23 pm on August 24, 2016: contributor

needs rebase.

laanwj commented at 6:50 am on August 25, 2016: member

Yes, needs rebase, should be merged not very long after that IMO

theuni commented at 5:44 pm on August 30, 2016: member

I have this rebased locally, I’ve been pulling out more controversial chunks for individual PR. Would you prefer to just do it all in one go here?

btcdrak commented at 5:44 pm on August 30, 2016: contributor

@theuni all in one go please.

theuni commented at 3:26 am on August 31, 2016: member

ok, will push up a rebased version tomorrow.

theuni force-pushed on Aug 31, 2016

theuni commented at 6:24 pm on August 31, 2016: member

Rebased. This has been rebased through lots of changes, though I believe the newer stuff is accounted for.

I’d recommend taking a look at the final CConnman first to get an idea of how it looks, then each individual commit should make more sense. @jonasschnelli 1cf69d418576a0096f8f3e83494a107774dde2ed is included as we discussed, so that kicking can be done in a more straightforward way, but it’s also just nice to have in the gui.

Because this is such a monster, at this point I’d prefer to add fixes on top rather than rebasing to include them.

jonasschnelli commented at 1:23 pm on September 1, 2016: contributor

~~Started IBDing a node with this PR (-listen=0, connected to a in-sync mainnet node on the same system). Its built with --enable-debug and IBDs with -debug=net, -prune=550 and -dbcache=4000.~~

~~Reached progress progress=0.097296 after 4h where I normally do a full IBD (with --enable-debug) under 4h on the same machine.~~

~~Either the -debug=net is causing the slow down or something regarding this PR. I can try to run more authentic benchmarks though (direct compare master against this PR on a “quite” machine).~~

theuni commented at 2:16 am on September 2, 2016: member

@jonasschnelli Thanks for having a look. I’ll try to reproduce.

laanwj commented at 7:19 am on September 8, 2016: member

Either the -debug=net is causing the slow down

Hah, I wouldn’t be surprised. Thanks for testing.

Is this performance regression serious enough to block this, or shouldit be solved later? This is a large change, which has had a lot of review by now, and it again needs rebase…

jonasschnelli commented at 7:21 am on September 8, 2016: contributor

Sorry this was my fault in doing benchmark in conjunctions with --enable-debug

laanwj commented at 7:34 am on September 8, 2016: member

Sorry this was my fault in doing benchmark in conjunctions with –enable-debug

Ok, good to know

in src/net.h: in d4fce84b9a outdated

110     bool Start(boost::thread_group& threadGroup, std::string& strNodeError);
111     void Stop();
112+    bool BindListenPort(const CService &bindAddr, std::string& strError, bool fWhitelisted = false);
113+
114 private:
115+    struct ListenSocket {

sipa commented at 10:56 am on September 8, 2016:

Just a tip, and not a suggestion to change in this PR right now, you can just say ‘struct ListenSocket;’ here, and fully define it in the cpp file as ‘struct CConnman::ListenSocket { … };’, since I don’t think any of the users of this class need the fully materialized definition.

theuni commented at 1:25 pm on September 8, 2016:

Hmm, I’ve never had luck with forward-declaring nested classes. I’ll give it a shot!

in src/net.h: in ac198b7dc4 outdated

392@@ -347,12 +393,6 @@ class CNode
393     const uint64_t nKeyedNetGroup;
394 protected:
395 
396-    // Denial-of-service detection/prevention
397-    // Key is IP address, value is banned-until-time
398-    static banmap_t setBanned;

sipa commented at 11:01 am on September 8, 2016:

I am so happy to see these globals-disguised-as-static-fields disappear!

in src/rpc/net.cpp: in ac198b7dc4 outdated

488@@ -489,6 +489,8 @@ UniValue setban(const UniValue& params, bool fHelp)
489                             + HelpExampleCli("setban", "\"192.168.0.0/24\" \"add\"")
490                             + HelpExampleRpc("setban", "\"192.168.0.6\", \"add\", 86400")
491                             );
492+    if(!g_connman)

sipa commented at 11:11 am on September 8, 2016:

Style nit: space after if.

in src/main.cpp: in 55af13c9fa outdated

483@@ -484,11 +484,12 @@ void MaybeSetPeerAsAnnouncingHeaderAndIDs(const CNodeState* nodestate, CNode* pf
484         if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
485             // As per BIP152, we only get 3 of our peers to announce
486             // blocks using compact encodings.
487-            CNode* pnodeStop = FindNode(lNodesAnnouncingHeaderAndIDs.front());
488-            if (pnodeStop) {
489+            bool found = connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion](CNode* pnodeStop){

sipa commented at 12:01 pm on September 8, 2016:

Not being very familiar with c++11 lambdas yet, what is the reason for explicitly listing the captures, rather than using [=] ?

theuni commented at 1:20 pm on September 8, 2016:

Just personal preference, I like being explicit about what the lambda is changing. In this case, = is probably desirable, it’s [&] that I try to avoid.

sipa commented at 1:25 pm on September 8, 2016:

Seems reasonable.

sipa commented at 1:29 pm on September 8, 2016:

It seems you can use [=,x&,y&,…] as well to mean “capture everything by value, but x and y by reference”, neat.

theuni commented at 1:44 pm on September 8, 2016:

Ah, perfect. That’s great as a general rule.

in src/net.cpp: in 55af13c9fa outdated

2692+
2693+bool CConnman::ForEachNodeThen(std::function<bool(CNode* pnode)> pre, std::function<void()> post)
2694+{
2695+    bool ret = true;
2696+    LOCK(cs_vNodes);
2697+    for (auto&& node : vNodes)

sipa commented at 12:20 pm on September 8, 2016:

A universal reference rvalue seems unnecessary here, since they’re just pointers you can copy.

theuni commented at 1:22 pm on September 8, 2016:

Yea, this is just a habit I’ve picked up. See the “notes” here: http://en.cppreference.com/w/cpp/language/range-for

in src/net.cpp: in f1bd45d11e outdated

2038@@ -2041,9 +2039,13 @@ bool StartNode(CConnman& connman, boost::thread_group& threadGroup, CScheduler&
2039     return ret;
2040 }
2041 
2042-bool CConnman::Start(boost::thread_group& threadGroup, CScheduler& scheduler, std::string& strNodeError)
2043+NodeId CConnman::GetNewNodeId()
2044 {
2045+    return nLastNodeId.fetch_add(1, std::memory_order_relaxed);

sipa commented at 12:29 pm on September 8, 2016:

Neat.

in src/net.h: in 9f6a3ea5e2 outdated

119+        CClientUIInterface* interface = nullptr;
120+    };
121     CConnman();
122     ~CConnman();
123-    bool Start(boost::thread_group& threadGroup, CScheduler& scheduler, ServiceFlags nLocalServicesIn, ServiceFlags nRelevantServicesIn, int nMaxConnectionsIn, int nMaxOutboundIn, int nBestHeightIn, CClientUIInterface* interfaceIn, std::string& strNodeError);
124+    bool Start(boost::thread_group& threadGroup, CScheduler& scheduler, std::string& strNodeError, Options options);

sipa commented at 1:17 pm on September 8, 2016:

rvalue reference for options?

EDIT: nevermind, all of the fields are primitives.

theuni commented at 1:29 pm on September 8, 2016:

They’re passed by value so that they can be std::move’d in if desired, but as-is, all members are fundamental, so they’ll all be copied anyway

sipa commented at 1:24 pm on September 8, 2016: member

Concept and overall code review ACK. I think you still need to address https://github.com/bitcoin/bitcoin/pull/8085/files#r66671033. I also left a number of nits in various commits, but none of them are blockers.

Needs rebase, though.

theuni commented at 1:33 pm on September 8, 2016: member

@sipa Yikes, thanks for noticing the missed changes. I’ll rebase now and make sure that matches. Thanks for the review!

gui: add NodeID to the peer table 531214fb10

net: move CBanDB and CAddrDB out of net.h/cpp

This will eventually solve a circular dependency

d93b14dc5d

net: Create CConnman to encapsulate p2p connections cd16f48028

net: Add rpc error for missing/disabled p2p functionality d7349ca50d

net: Pass CConnman around as needed 8d58c4d81f

net: Pass CConnection to wallet rather than using the global 5b446dd5b1

net: Move socket binding into CConnman 02137f11e2

net: move OpenNetworkConnection into CConnman b1a5f43208

net: handle nodesignals in CConnman aaf018e3b7

net: move ban and addrman functions into CConnman a0f3d3cdad

net: Add oneshot functions to CConnman 502dd3a8a0

net: move added node functions to CConnman 8ae2dac1c6

net: Add most functions needed for vNodes to CConnman c0569c7fa1

net: create generic functor accessors and move vNodes to CConnman 53347f0cb9

net: move whitelist functions into CConnman 6c19d92361

net: move nLastNodeId to CConnman 551e0887db

net: move nLocalHostNonce to CConnman

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.

960cf2e405

net: move messageHandlerCondition to CConnman ee44fa9576

net: SocketSendData returns written size adf5d4c2e4

net: move send/recv statistics to CConnman 63cafa6329

net: move SendBufferSize/ReceiveFloodSize to CConnman be9c796dc5

net: move nLocalServices/nRelevantServices to CConnman

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).

bd72937dc4

net: move semOutbound to CConnman 8a593694b1

net: move max/max-outbound to CConnman fdf69ff21a

net: Pass best block known height into CConnman

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.

f60b9059e4

net: pass CClientUIInterface into CConnman e81a602cf0

net: Drop StartNode/StopNode and use CConnman directly bafa5fc5a1

net: Introduce CConnection::Options to avoid passing so many params a19553b992

net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options fa2f8bc47f

net: move vNodesDisconnected into CConnman 98591c5027

Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting d1a2295f0d

Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead e700cd0bc8

net: move MAX_FEELER_CONNECTIONS into connman 0103c5b90f

theuni force-pushed on Sep 8, 2016

theuni commented at 5:18 pm on September 8, 2016: member

Rebased, fixed up the salt changes, and squashed the win32 build fix.

laanwj commented at 8:47 am on September 9, 2016: member

Thanks for the review @sipa.

laanwj commented at 9:44 am on September 9, 2016: member

ACK 0103c5b

laanwj merged this on Sep 9, 2016

laanwj closed this on Sep 9, 2016

laanwj referenced this in commit 6423116741 on Sep 9, 2016

in src/main.cpp: in 0103c5b90f

5253@@ -5238,7 +5254,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5254                         nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER &&
5255                         (!IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) {
5256                         inv.type |= nFetchFlags;
5257-                        if (nodestate->fProvidesHeaderAndIDs && !(nLocalServices & NODE_WITNESS))
5258+                        if (nodestate->fProvidesHeaderAndIDs && !(pfrom->GetLocalServices() & NODE_WITNESS))

rebroad commented at 10:02 am on September 11, 2016:

I think the current naming is a little confusing as to be a function of pfrom implies that the “Local” Services are local to pfrom. “nLocalServices” seemed more intuitively compatible with talking about the services local to our peer.

in src/main.cpp: in 0103c5b90f

5018@@ -4984,7 +5019,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5019         }
5020 
5021         // Disconnect if we connected to ourself
5022-        if (nNonce == nLocalHostNonce && nNonce > 1)
5023+        if (pfrom->fInbound && !connman.CheckIncomingNonce(nNonce))

rebroad commented at 10:22 am on September 11, 2016:

The code isn’t as simple as it used to be - what was wrong with “nNonce == nLocalHostNonce”? it was so much clearer what the code was doing before…

sipa commented at 2:05 pm on September 11, 2016: member

@rebroad It was wrong, as is explained in the commit. If two connections arrive before the version message cycles back, nLocalHostNonce would be overwritten by the second connection before the comparison occurs.

luke-jr referenced this in commit 5ee139d6cf on Oct 20, 2016

luke-jr referenced this in commit 84e423ee49 on Dec 21, 2016

random-zebra referenced this in commit 777638e7bc on Aug 27, 2020

zkbot referenced this in commit f40121446d on Nov 12, 2020

zkbot referenced this in commit 049951dc45 on Feb 11, 2021

zkbot referenced this in commit b3a6729944 on Feb 16, 2021

zkbot referenced this in commit e85265fbd5 on Feb 17, 2021

zkbot referenced this in commit b4b07a1bbd on Feb 17, 2021

DrahtBot locked this on Sep 8, 2021

p2p: Begin encapsulation #8085