p2p: Begin encapsulation #8085

theuni · 2016-05-22T09:04:33Z

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 Defer inserting into maprelay until just before relaying. #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

NicolasDorier · 2016-05-23T04:04:14Z

nit typo: "SocketSendData resurns written size"

sipa · 2016-05-24T14:50:32Z

src/net.h

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.

@sipa See 46cf675#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.

Nevermind, indeed.

sipa · 2016-05-24T16:37:34Z

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

theuni · 2016-05-24T16:48:43Z

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

theuni · 2016-05-26T01:30:23Z

@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 · 2016-05-27T05:34:00Z

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 · 2016-06-06T21:21:00Z

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

kazcw · 2016-06-10T19:12:28Z

src/init.cpp

 static const bool DEFAULT_DISABLE_SAFEMODE = false;
 static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false;

+std::shared_ptr<CConnman> g_connman;


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

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.

kazcw · 2016-06-10T20:42:10Z

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 · 2016-06-10T21:14:09Z

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

JeremyRubin · 2016-06-14T19:10:53Z

src/net.h

+        int nMaxConnections = 0;
+        int nMaxOutbound = 0;
+        int nBestHeight = 0;
+        CClientUIInterface* interface = nullptr;


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.

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.

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.

JeremyRubin · 2016-06-15T15:11:59Z

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

instagibbs · 2016-06-16T14:00:41Z

src/rpc/misc.cpp

    obj.push_back(Pair("timeoffset",    GetTimeOffset()));
-    obj.push_back(Pair("connections",   (int)vNodes.size()));
+    if(g_connman)
+        obj.push_back(Pair("connections",   (int)g_connman->GetNodeCount(CConnman::CONNECTIONS_ALL)));


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

…f documenting

…rather than a std::function to eliminate std::function overhead

theuni · 2016-09-08T17:18:09Z

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

laanwj · 2016-09-09T08:47:23Z

Thanks for the review @sipa.

laanwj · 2016-09-09T09:44:59Z

ACK 0103c5b

0103c5b net: move MAX_FEELER_CONNECTIONS into connman (Cory Fields) e700cd0 Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead (Jeremy Rubin) d1a2295 Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting (Jeremy Rubin) 98591c5 net: move vNodesDisconnected into CConnman (Cory Fields) fa2f8bc net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options (Cory Fields) a19553b net: Introduce CConnection::Options to avoid passing so many params (Cory Fields) bafa5fc net: Drop StartNode/StopNode and use CConnman directly (Cory Fields) e81a602 net: pass CClientUIInterface into CConnman (Cory Fields) f60b905 net: Pass best block known height into CConnman (Cory Fields) fdf69ff net: move max/max-outbound to CConnman (Cory Fields) 8a59369 net: move semOutbound to CConnman (Cory Fields) bd72937 net: move nLocalServices/nRelevantServices to CConnman (Cory Fields) be9c796 net: move SendBufferSize/ReceiveFloodSize to CConnman (Cory Fields) 63cafa6 net: move send/recv statistics to CConnman (Cory Fields) adf5d4c net: SocketSendData returns written size (Cory Fields) ee44fa9 net: move messageHandlerCondition to CConnman (Cory Fields) 960cf2e net: move nLocalHostNonce to CConnman (Cory Fields) 551e088 net: move nLastNodeId to CConnman (Cory Fields) 6c19d92 net: move whitelist functions into CConnman (Cory Fields) 53347f0 net: create generic functor accessors and move vNodes to CConnman (Cory Fields) c0569c7 net: Add most functions needed for vNodes to CConnman (Cory Fields) 8ae2dac net: move added node functions to CConnman (Cory Fields) 502dd3a net: Add oneshot functions to CConnman (Cory Fields) a0f3d3c net: move ban and addrman functions into CConnman (Cory Fields) aaf018e net: handle nodesignals in CConnman (Cory Fields) b1a5f43 net: move OpenNetworkConnection into CConnman (Cory Fields) 02137f1 net: Move socket binding into CConnman (Cory Fields) 5b446dd net: Pass CConnection to wallet rather than using the global (Cory Fields) 8d58c4d net: Pass CConnman around as needed (Cory Fields) d7349ca net: Add rpc error for missing/disabled p2p functionality (Cory Fields) cd16f48 net: Create CConnman to encapsulate p2p connections (Cory Fields) d93b14d net: move CBanDB and CAddrDB out of net.h/cpp (Cory Fields) 531214f gui: add NodeID to the peer table (Cory Fields)

rebroad · 2016-09-11T10:02:30Z

src/main.cpp

                        (!IsWitnessEnabled(chainActive.Tip(), chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) {
                        inv.type |= nFetchFlags;
-                        if (nodestate->fProvidesHeaderAndIDs && !(nLocalServices & NODE_WITNESS))
+                        if (nodestate->fProvidesHeaderAndIDs && !(pfrom->GetLocalServices() & NODE_WITNESS))


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.

sipa · 2016-09-11T14:05:32Z

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

Github-Pull: bitcoin#8085 Rebased-From: 531214f

e733939 [Cleanup] Remove CConnman::Copy(Release)NodeVector, now unused (random-zebra) b09da57 [Refactor] Proper CConnman encapsulation of mnsync (random-zebra) 219061e [Refactor] Decouple SyncWithNode from CMasternodeSync::Process() (random-zebra) e1f3620 [Refactor] Proper CConnman encapsulation of proposals / votes sync (random-zebra) f38c9f9 miner: don't try to access connman if it doesn't exist (Fuzzbawls) 92ab619 Address feedback (Fuzzbawls) 9d5346a Do not set an addr time penalty when a peer advertises itself. (Gregory Maxwell) fe12ff9 net: No longer send local address in addrMe (Wladimir J. van der Laan) 14d6917 net: misc header cleanups (Cory Fields) 34ba0de net: make proxy receives interruptible (Cory Fields) 1bd97ef net: make net processing interruptable (Fuzzbawls) e24b4cc net: make net interruptible (Cory Fields) 814d0de net: add CThreadInterrupt and InterruptibleSleep (Cory Fields) 1443f2a net: a few small cleanups before replacing boost threads (Cory Fields) 58eabe4 net: move MAX_FEELER_CONNECTIONS into connman (Cory Fields) 874baba Convert ForEachNode* functions to take a templated function argument rather than a std::function to eliminate std::function overhead (Jeremy Rubin) 9485ab0 Made the ForEachNode* functions in src/net.cpp more pragmatic and self documenting (Jeremy Rubin) c1e59ad minor net cleanups (Fuzzbawls) 07ae004 net: move vNodesDisconnected into CConnman (Cory Fields) 276c946 net: add nSendBufferMaxSize/nReceiveFloodSize to CConnection::Options (Cory Fields) 22a9aff net: Introduce CConnection::Options to avoid passing so many params (Cory Fields) e4891bf net: Drop StartNode/StopNode and use CConnman directly (Cory Fields) 431575c net: pass CClientUIInterface into CConnman (Cory Fields) 48de47e net: Pass best block known height into CConnman (Cory Fields) 15eed91 net: move max/max-outbound to CConnman (Cory Fields) 2bf0921 net: move semOutbound to CConnman (Cory Fields) 481929f net: move nLocalServices/nRelevantServices to CConnman (Cory Fields) bcee6ae net: move SendBufferSize/ReceiveFloodSize to CConnman (Cory Fields) 6865469 net: move send/recv statistics to CConnman (Cory Fields) 1cec418 net: SocketSendData returns written size (Cory Fields) 2bb9dfa net: move messageHandlerCondition to CConnman (Cory Fields) 9c5a0df net: move nLocalHostNonce to CConnman (Cory Fields) a1394ef net: move nLastNodeId to CConnman (Cory Fields) 3804c29 net: move whitelist functions into CConnman (Cory Fields) dbde9be net: create generic functor accessors and move vNodes to CConnman (Cory Fields) 2e02467 net: Add most functions needed for vNodes to CConnman (Cory Fields) 5667e61 net: move added node functions to CConnman (Cory Fields) 37487ed net: Add oneshot functions to CConnman (Cory Fields) facf878 net: move ban and addrman functions into CConnman (Cory Fields) 091aaf2 net: handle nodesignals in CConnman (Cory Fields) 1e9fa0f net: move OpenNetworkConnection into CConnman (Cory Fields) 573200f net: Move socket binding into CConnman (Cory Fields) 7762b97 net: Pass CConnection to wallet rather than using the global (Fuzzbawls) 00591b8 net: Pass CConnman around as needed (Cory Fields) 2cd3d39 net: Add rpc error for missing/disabled p2p functionality (Cory Fields) f08e316 net: Create CConnman to encapsulate p2p connections (Cory Fields) 66337dc net: move CBanDB and CAddrDB out of net.h/cpp (Fuzzbawls) 10969f6 gui: add NodeID to the peer table (Cory Fields) 58044ac Fix some locks (Pieter Wuille) f9f8926 Do not shadow variables in networking code (Pavel Janík) Pull request description: This is the finalization of the upstream PRs being tracked in #1374 to update much of our networking code to more closely follow upstream improvements. The following PRs are included here: - bitcoin#8466 Do not shadow variables in networking code - bitcoin#8606 Fix some locks - bitcoin#8085 Begin encapsulation - bitcoin#9289 drop boost::thread_group - bitcoin#8740 No longer send local address in addrMe - bitcoin#8661 Do not set an addr time penalty when a peer advertises itself Additionally, during conflict resolution and backporting of 8085, the following additional upstream PR was included: - bitcoin#8715 only delete CConnman if it's been created Still TODO in future PRs: - bitcoin#9037 - bitcoin#9441 - bitcoin#9609 - bitcoin#9626 - bitcoin#9708 - bitcoin#12381 - bitcoin#18584 ACKs for top commit: furszy: same here, code ACK e733939. Nice work ☕ . furszy: ACK e733939 . random-zebra: ACK e733939 and merging... Tree-SHA512: 0fc3cca76d9ddc13f75fc9d48c48d215e6c0e0381377c0318436176a0e0ead73b511e0061a4b63f7052874730b6da8b0ffc2c94cba034bcc39aad4212b69ee22

banlist.dat: store banlist on disk Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6310 - bitcoin/bitcoin#6315 - Only the new signal and net changes, no QT. - bitcoin/bitcoin#7350 - bitcoin/bitcoin#7458 - bitcoin/bitcoin#7696 - bitcoin/bitcoin#7959 - bitcoin/bitcoin#7906 - Only the ban-related commits. - bitcoin/bitcoin#8392 - Only the second commit. - bitcoin/bitcoin#8085 - Only the second commit. - bitcoin/bitcoin#10564 Part of #2074.

banlist.dat: store banlist on disk Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6310 - bitcoin/bitcoin#6315 - Only the new signal and net changes, no QT. - bitcoin/bitcoin#7350 - bitcoin/bitcoin#7458 - bitcoin/bitcoin#7696 - bitcoin/bitcoin#7959 - bitcoin/bitcoin#7906 - Only the ban-related commits. - bitcoin/bitcoin#8392 - Only the second commit. - bitcoin/bitcoin#8085 - Only the second commit. - bitcoin/bitcoin#10564 - bitcoin/bitcoin#13061 - Wasn't needed when I first made this backport in 2017, but it is now! Part of #2074.

laanwj added the P2P label May 22, 2016

sipa reviewed May 24, 2016
View reviewed changes

sipa mentioned this pull request May 25, 2016

Use SipHash for node eviction #8086

Closed

theuni force-pushed the net-refactor13 branch 2 times, most recently from de204a2 to 2ac75d4 Compare May 27, 2016 05:05

theuni force-pushed the net-refactor13 branch from 2ac75d4 to f86d0be Compare June 6, 2016 20:50

theuni mentioned this pull request Jun 10, 2016

CNodeState encapsulation, lock reduction #8147

Closed

kazcw reviewed Jun 10, 2016
View reviewed changes

JeremyRubin reviewed Jun 14, 2016
View reviewed changes

instagibbs reviewed Jun 16, 2016
View reviewed changes

JeremyRubin and others added 3 commits September 8, 2016 13:06

Made the ForEachNode* functions in src/net.cpp more pragmatic and sel…

d1a2295

…f documenting

Convert ForEachNode* functions to take a templated function argument …

e700cd0

…rather than a std::function to eliminate std::function overhead

net: move MAX_FEELER_CONNECTIONS into connman

0103c5b

theuni force-pushed the net-refactor13 branch from 363e28d to 0103c5b Compare September 8, 2016 17:09

laanwj merged commit 0103c5b into bitcoin:master Sep 9, 2016

rebroad reviewed Sep 11, 2016
View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

gui: add NodeID to the peer table

5ee139d

Github-Pull: bitcoin#8085 Rebased-From: 531214f

laanwj mentioned this pull request Dec 2, 2016

0.13.2 backports #9264

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016

gui: add NodeID to the peer table

84e423e

Github-Pull: bitcoin#8085 Rebased-From: 531214f

sickpig mentioned this pull request Mar 13, 2017

Please steal the Network Activity Toggle from Core BitcoinUnlimited/BitcoinUnlimited#311

Open

OlegGirko mentioned this pull request Jul 18, 2017

Backport Bitcoin PR#8085: p2p: Begin encapsulation dashpay/dash#1537

Merged

dagurval mentioned this pull request Feb 7, 2018

p2p: Begin encapsulation bitcoinxt/bitcoinxt#324

Merged

Fuzzbawls mentioned this pull request Mar 4, 2020

Tracking: Backport upstream network encapsulation PIVX-Project/PIVX#1374

Closed

21 tasks

hebasto mentioned this pull request Jun 6, 2020

qt: Deduplicate NumConnections enum #19193

Closed

Fuzzbawls mentioned this pull request Aug 19, 2020

[P2P] Begin Network Encapsulation PIVX-Project/PIVX#1812

Merged

str4d mentioned this pull request Nov 12, 2020

banlist.dat: store banlist on disk zcash/zcash#2812

Merged

PRCYDev mentioned this pull request Jan 25, 2021

Tracking: Backport upstream network encapsulation PRCYCoin/PRCYCoin#4

Open

21 tasks

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

p2p: Begin encapsulation #8085

p2p: Begin encapsulation #8085

Uh oh!

Conversation

theuni commented May 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented May 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 24, 2016

Uh oh!

theuni commented May 24, 2016

Uh oh!

theuni commented May 26, 2016

Uh oh!

theuni commented May 27, 2016

Uh oh!

theuni commented Jun 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazcw commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni Jun 15, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Jun 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Sep 8, 2016

Uh oh!

laanwj commented Sep 9, 2016

Uh oh!

laanwj commented Sep 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Sep 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

theuni commented May 22, 2016 •

edited

Loading

NicolasDorier commented May 23, 2016 •

edited

Loading

kazcw commented Jun 10, 2016 •

edited

Loading