net: have CConnman handle message sending #8708

theuni · 2016-09-13T02:00:44Z

See individual commits for more description. In particular, 75ead75890b9b5ca5565a31d5fe9ad31dc9a96fa.

This (along with #8707) fixes the current maxupload test failure.

laanwj · 2016-09-20T13:40:15Z

Concept ACK

laanwj · 2016-09-25T11:40:00Z

Needs rebase

jtimon · 2016-09-30T16:58:25Z

Concept ACK

TheBlueMatt

Got halfway through a review and realized I just dont know how I feel about this...CNode moving towards a "I speak to the CConnman and serialize messages for the wire" seems like a sane design (and seems to be what you're going for here), but it means there is just no good place to put serialization-version logic that isnt a complete layer violation.

TheBlueMatt · 2016-10-06T16:54:55Z

src/main.cpp

@@ -5038,7 +5038,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,

        // Be shy and don't send version until we hear
        if (pfrom->fInbound)
-            pfrom->PushVersion();
+            connman.PushVersion(pfrom, GetAdjustedTime());


Any idea why we use a different time for inbound vs outbound peers? If anything I'd think we'd announce our real time to inbound peers and our adjusted time to outbound ones, but we do the opposite.

@TheBlueMatt I'm not sure I see the reasoning for either. I'd expect us to use our best guess of time everywhere.

TheBlueMatt · 2016-10-06T17:00:39Z

src/net.cpp

@@ -389,6 +389,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo

        // Add node
        CNode* pnode = new CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), pszDest ? pszDest : "", false);
+
+        PushVersion(pnode, GetTime());


Can we go ahead and move the PushVersion logic into InitializeNode? I think thats the right place since it will move into net_processing.cpp with the rest of the "node-message-processing" stuff.

theuni · 2016-10-06T19:25:47Z

On Oct 6, 2016 1:06 PM, "Matt Corallo" notifications@github.com wrote:

@TheBlueMatt commented on this pull request.

Got halfway through a review and realized I just dont know how I feel
about this...CNode moving towards a "I speak to the CConnman and serialize
messages for the wire" seems like a sane design (and seems to be what
you're going for here), but it means there is just no good place to put
serialization-version logic that isnt a complete layer violation.

Yea, I agree. I started creating a CSendMessage that would hold the
serialized result, which could go directly to CConnman, but decided for the
more straightforward change here. I'm happy to make that change, either as
a part of this pr or after.

That has the benefit of testing the serialized result for p2p without
actually requiring the send.

In src/main.cpp:

@@ -5038,7 +5038,7 @@ bool static ProcessMessage(CNode* pfrom, string
strCommand, CDataStream& vRecv, // Be shy and don't send version until we
hear if (pfrom->fInbound) - pfrom->PushVersion(); +
connman.PushVersion(pfrom, GetAdjustedTime());

Any idea why we use a different time for inbound vs outbound peers? If
anything I'd think we'd announce our real time to inbound peers and our
adjusted time to outbound ones, but we do the opposite.

Unsure of the reason there.

In src/net.cpp:

@@ -389,6 +389,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
const char pszDest, bool fCo // Add node CNode pnode = new
CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket,
addrConnect, CalculateKeyedNetGroup(addrConnect), pszDest ? pszDest : "",
false); + + PushVersion(pnode, GetTime());

Can we go ahead and move the PushVersion logic into InitializeNode? I
think thats the right place since it will move into net_processing.cpp with
the rest of the "node-message-processing" stuff.

I had planned on doing exactly that as a next step, but it looked kinda out
of place by itself. With your changes, it makes perfect sense. Happy to
change it to whatever makes it easier for you to move.

Glad to see we're in sync :)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

TheBlueMatt · 2016-10-06T19:47:35Z

Yea, seems like we're mostly on the same page here... Probably reasonable to give the serialization to net_processing, though I am concerned with how to merge type with P2P encryption, given that it also changes the message hashing, but only after messages are exchanged (I believe? Maybe @jonasschnelli wants to comment). Even more unsure of how to handle that.

As for this PR, I'm fine with it as it is now, noting that we want to tweak things further as we split code, though I'd prefer we go ahead and move the version message handling to Initialize so we don't end up moving the same block three times.

On October 6, 2016 3:25:54 PM EDT, Cory Fields notifications@github.com wrote:

On Oct 6, 2016 1:06 PM, "Matt Corallo" notifications@github.com
wrote:

@TheBlueMatt commented on this pull request.

Got halfway through a review and realized I just dont know how I feel
about this...CNode moving towards a "I speak to the CConnman and
serialize
messages for the wire" seems like a sane design (and seems to be what
you're going for here), but it means there is just no good place to put
serialization-version logic that isnt a complete layer violation.

Yea, I agree. I started creating a CSendMessage that would hold the
serialized result, which could go directly to CConnman, but decided for
the
more straightforward change here. I'm happy to make that change, either
as
a part of this pr or after.

That has the benefit of testing the serialized result for p2p without
actually requiring the send.

In src/main.cpp:

@@ -5038,7 +5038,7 @@ bool static ProcessMessage(CNode* pfrom,
string
strCommand, CDataStream& vRecv, // Be shy and don't send version until
we
hear if (pfrom->fInbound) - pfrom->PushVersion(); +
connman.PushVersion(pfrom, GetAdjustedTime());

Any idea why we use a different time for inbound vs outbound peers?
If
anything I'd think we'd announce our real time to inbound peers and our
adjusted time to outbound ones, but we do the opposite.

Unsure of the reason there.

In src/net.cpp:

@@ -389,6 +389,9 @@ CNode* CConnman::ConnectNode(CAddress
addrConnect,
const char pszDest, bool fCo // Add node CNode pnode = new
CNode(GetNewNodeId(), nLocalServices, GetBestHeight(), hSocket,
addrConnect, CalculateKeyedNetGroup(addrConnect), pszDest ? pszDest :
"",
false); + + PushVersion(pnode, GetTime());

Can we go ahead and move the PushVersion logic into InitializeNode? I
think thats the right place since it will move into net_processing.cpp
with
the rest of the "node-message-processing" stuff.

I had planned on doing exactly that as a next step, but it looked kinda
out
of place by itself. With your changes, it makes perfect sense. Happy to
change it to whatever makes it easier for you to move.

Glad to see we're in sync :)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#8708 (comment)

laanwj · 2016-10-25T10:44:13Z

Needs rebase

theuni · 2016-10-27T18:47:28Z

rebased.

@TheBlueMatt I added some commits to handle the version push in InitializeNode. I left the serialization happening in CConnman for now. Agreed that it's a layer violation and have plans for separating it, but I'd prefer not to change the scope here too much unless you hugely object.

TheBlueMatt

A few nits, and some things that look like missing locks to me - I think we should do a pass over CNode and just convert a bunch of the various ints to std::atomic to fix most of the issues (ie they aren't issues on at least x86 right now).

TheBlueMatt · 2016-10-28T15:14:38Z

src/net.cpp

-    assert(ssSend.size() == 0);
-    ssSend << CMessageHeader(Params().MessageStart(), pszCommand, 0);
-    LogPrint("net", "sending: %s ", SanitizeString(pszCommand));
+    return {SER_NETWORK, nVersion ? nVersion | flags : pnode->GetSendVersion() | flags, CMessageHeader(Params().MessageStart(), sCommand.c_str(), 0) };


GetSendVersion needs a lock (or make it atomic with release/aquire).

See https://github.com/bitcoin/bitcoin/pull/8708/files#diff-1a8b9d1ad0a6fda5e751286c73102fc2R755

TheBlueMatt · 2016-10-28T15:16:05Z

src/net.cpp

+    WriteLE32((uint8_t*)&strm[CMessageHeader::MESSAGE_SIZE_OFFSET], nSize);
+    // Set the checksum
+    uint256 hash = Hash(strm.begin() + CMessageHeader::HEADER_SIZE, strm.end());
+    assert(strm.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);


Can you move this up to cover the MESSAGE_SIZE_OFFSET write two lines up?

TheBlueMatt · 2016-10-28T15:20:52Z

src/net.cpp

 }

-void CNode::EndMessage(const char* pszCommand) UNLOCK_FUNCTION(cs_vSend)
+void CConnman::PushMessage(CNode* pnode, CDataStream& strm, const std::string& sCommand)


strm can be const, I believe? Would be nice to go ahead and do that since I'd like to use this to create a cmpctblock fully-encoded message once and then just call this on each node that wants it.

Er, this isn't intended to be used that way. This is internal and should be private.

The next set of changes will introduce a CSendMessage which is fully serialized. PushMessage will take that instead. That fixes the current layer violation here, and your issue at the same time I think?

OK, sounds good.

TheBlueMatt · 2016-10-28T15:46:11Z

src/net.cpp

@@ -411,6 +414,24 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
    return NULL;
 }

+void CConnman::PushVersion(CNode* pnode, int64_t nTime)


nit: I'd kinda prefer we not move PushVersion twice.

TheBlueMatt · 2016-10-28T16:10:11Z

src/main.cpp

+    CAddress addrYou = (addr.IsRoutable() && !IsProxy(addr) ? addr : CAddress(CService(), addr.nServices));
+    CAddress addrMe = CAddress(CService(), nLocalNodeServices);
+
+    connman.PushMessageWithVersion(pnode, INIT_PROTO_VERSION, NetMsgType::VERSION, PROTOCOL_VERSION, (uint64_t)nLocalNodeServices, GetTime(), addrYou, addrMe,


This is a slight change in behavior - we are now pushing our non-adjusted time to all peers, instead of just outbound peers. Not actually sure if it matters, but taking note here.

Whoops, good catch. That was not intentional. Will fix.

TheBlueMatt · 2016-10-28T16:15:12Z

src/main.cpp

+void PushNodeVersion(CNode *pnode, CConnman& connman)
+{
+    ServiceFlags nLocalNodeServices = pnode->GetLocalServices();
+    uint64_t nonce = pnode->GetLocalNonce();


I think half the functions here need locks or to become atomics to be correct. (unless I'm missing a required lock to call this function, in which case please AssertLockHeld to make it obvious).

May be easiest to just make half the ints in CNode atomic.

This value doesn't change. I have a branch with additional commits that turns a bunch of stuff into const to make it clear that it's threadsafe. I could add those here if you'd like?

I'm ok with this happening in a separate PR, as AFAICT there are already dragons here, but should definitely happen before 0.14.

TheBlueMatt · 2016-10-28T16:33:40Z

src/net.h

+        // once the handshake is complete. Any attempt to set this twice is an
+        // error.
+        assert(nSendVersion == 0);
+        *const_cast<int*>(&nSendVersion) = nVersionIn;


This violates C++ spec - nSendVersion must exist non-const somewhere for const_cast to be permitted (I believe).

Ok, I'll just change this back to non-const. The const was only to prove that it's not racy, since it can only be set once.

theuni · 2016-10-29T20:38:47Z

Updated for @TheBlueMatt's nits. Since the changes were very small, I went ahead and squashed.

PRs are coming up to address the const issues, as well as fixing the serialization layer violation.

TheBlueMatt

Aside from the few comments, utACK 2df814b2ecfcfe67f5a7419b662df1ec1efd9d2d.

TheBlueMatt · 2016-10-31T23:22:22Z

src/net.cpp

+    // Set the size
+    unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
+    WriteLE32((uint8_t*)&strm[CMessageHeader::MESSAGE_SIZE_OFFSET], nSize);
+    assert(strm.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);


This assert needs to move up a line.

TheBlueMatt · 2016-10-31T23:23:37Z

src/net.cpp

-    assert(ssSend.size () >= CMessageHeader::CHECKSUM_OFFSET + CMessageHeader::CHECKSUM_SIZE);
-    memcpy((char*)&ssSend[CMessageHeader::CHECKSUM_OFFSET], hash.begin(), CMessageHeader::CHECKSUM_SIZE);
+    unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
+    LogPrint("net", "%s (%d bytes) peer=%d\n", sCommand.c_str(), nSize, pnode->id);


nit: any reason you changed the text here? It used to read "sending: %s (%d bytes) peer=%d\n", SanitizeString(pszCommand, nSize, id) (split across two lines).

No, they just ended up combined since the actual data push is all done in one place now.

Is there a reason to prefer having them split? How about adding back the "sending" for easy grepping, and the sanitizestring for extra paranoia, but leaving it as a combined message?

TheBlueMatt · 2016-11-01T00:07:26Z

src/net.cpp

@@ -1156,10 +1137,6 @@ void CConnman::ThreadSocketHandler()
                {
                    TRY_LOCK(pnode->cs_vSend, lockSend);
                    if (lockSend) {
-                        if (pnode->nOptimisticBytesWritten) {


I cant see anything wrong with this, but it seems obviously equivalent...what was the reason for nOptimisticBytesSent previously?

It was a hack. In fact, this PR was originally created to fix this.

If the node is in charge of pushing data, Connman has no way of knowing if optimistic writes have succeeded. The hackish solution was to cache the periodically check for optimistic written bytes (stored by CNode) and add them to the node's total, but that fails miserably if the optimistic writes succeed most of the time.

Now that CConnman is in charge of the writes, it knows immediately how much was written, and doesn't have to try to catch up with it later.

TheBlueMatt · 2016-11-01T21:44:03Z

Oh totally, didn't mean to imply the sending message should be two prints, just that you changed the text spuriously.

On November 1, 2016 5:08:13 PM EDT, Cory Fields notifications@github.com wrote:

theuni commented on this pull request.

// Set the checksum

uint256 hash = Hash(ssSend.begin() + CMessageHeader::HEADER_SIZE,
ssSend.end());

assert(ssSend.size () >= CMessageHeader::CHECKSUM_OFFSET +
CMessageHeader::CHECKSUM_SIZE);

memcpy((char*)&ssSend[CMessageHeader::CHECKSUM_OFFSET],
hash.begin(), CMessageHeader::CHECKSUM_SIZE);

unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;

LogPrint("net", "%s (%d bytes) peer=%d\n", sCommand.c_str(),
nSize, pnode->id);

No, they just ended up combined since the actual data push is all done
in one place now.

Is there a reason to prefer having them split? How about adding back
the "sending" for easy grepping, and the sanitizestring for extra
paranoia, but leaving it as a combined message?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#8708

laanwj · 2016-11-02T20:42:43Z

src/net.h

-    void PushVersion();
-
-
-    void PushMessage(const char* pszCommand)


Good to get rid of this repetition.

laanwj · 2016-11-02T20:45:30Z

src/net.cpp

-    // The -*messagestest options are intentionally not documented in the help message,
-    // since they are only used during development to debug the networking code and are
-    // not intended for end-users.
-    if (mapArgs.count("-dropmessagestest") && GetRand(GetArg("-dropmessagestest", 2)) == 0)


Yeah, I don't think anyone is using these, or has been using these for years.
Would be nice to get #7940 in at some point, it provides a better fuzzing framework which is not dependent on the network code.

@laanwj looks like I missed #7940. Agree with doing it at a lower level.

laanwj · 2016-11-02T20:51:19Z

src/net.h

@@ -136,6 +136,33 @@ class CConnman

    bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);

+    template <typename... Args>


These are not used?

PushMessageWithVersionAndFlag could be private I suppose. It's only used by the others.

PushMessageWithFlag is the same as before.

PushMessageWithVersion is used explicitly before the handshake is complete, before we know what upgraded version to use.

Note that as next steps, an intermediary and fully serialized "CSendMessage" (or so) class will be added, which CConnman accepts rather than bare arguments, and forwards to the network. This makes CConnman mostly oblivious as to what it's pushing (other than the type of message), which further improves encapsulation.

Also, The sendversion logic will move to CNodeState once the locking situation there is improved.

laanwj · 2016-11-02T20:51:47Z

How does this overlap with #9039? That pull changes the serialization framework to get rid of nVersion and nType in many places, does that mean some things can be simpllified here?

sipa · 2016-11-02T21:56:49Z

I believe the removal of CNode::Fuzz maybe remove the need for CDataStream::insert or CDataStream::erase.

sipa · 2016-11-02T21:57:47Z

As for overlap with #9039, I expect it to be trivial: just dropping the nVersion and nType everywhere. I'm happy to rebase #9039 on top of this if it gets merged first.

theuni · 2016-11-02T23:07:24Z

Agree with @sipa. Either order is fine by me.

sipa

utACK

sipa · 2016-11-03T06:41:20Z

src/net.cpp

@@ -2686,6 +2687,52 @@ void CNode::EndMessage(const char* pszCommand) UNLOCK_FUNCTION(cs_vSend)
    LEAVE_CRITICAL_SECTION(cs_vSend);
 }

+CDataStream CConnman::BeginMessage(CNode* pnode, int nVersion, int flags, const std::string& sCommand)
+{
+    return {SER_NETWORK, nVersion ? nVersion | flags : pnode->GetSendVersion() | flags, CMessageHeader(Params().MessageStart(), sCommand.c_str(), 0) };


That's pretty hard to read. Can you factor the version logic out, or at least combine as (nVersion ? nVersion : pnode->GetSendVersion()) | flags?

sipa · 2016-11-03T06:47:55Z

src/net.cpp

+            return;
+        }
+        bool optimisticSend(pnode->vSendMsg.empty());
+        pnode->vSendMsg.emplace_back(strm.begin(), strm.end());


Not for this PR, but we should try to make a CSerializeData move-constructable from a CDataStream, and make PushMessage take a CDataStream&& perhaps. Or is this going away anyway?

Yea, going away. Instead we'll have a new (very simple) stream type that serializes args for the network, which CConnman will use directly. As it'll construct in-place and be movable, it should eliminate at least 2 copies, I think.

sipa · 2016-11-03T06:54:27Z

src/main.cpp

@@ -355,10 +355,10 @@ void UpdatePreferredDownload(CNode* node, CNodeState* state)
 }

 void InitializeNode(NodeId nodeid, const CNode *pnode) {
+    CAddress addr = pnode->addr;


Why copy pnode->addr twice?

Also add a variadic CDataStream ctor for ease-of-use.

The changes here are dense and subtle, but hopefully all is more explicit than before. - CConnman is now in charge of sending data rather than the nodes themselves. This is necessary because many decisions need to be made with all nodes in mind, and a model that requires the nodes calling up to their manager quickly turns to spaghetti. - The per-node-serializer (ssSend) has been replaced with a (quasi-)const send-version. Since the send version for serialization can only change once per connection, we now explicitly tag messages with INIT_PROTO_VERSION if they are sent before the handshake. With this done, there's no need to lock for access to nSendVersion. Also, a new stream is used for each message, so there's no need to lock during the serialization process. - This takes care of accounting for optimistic sends, so the nOptimisticBytesWritten hack can be removed. - -dropmessagestest and -fuzzmessagestest have not been preserved, as I suspect they haven't been used in years.

Drop all of the old stuff.

This is now handled properly in realtime.

theuni · 2016-11-03T14:15:40Z

Rebased after #9050. The net.cpp diff before/after the rebase:

git diff 19d3daef936905743ef108aee1ccfe46fd77b283..c7b8effb96726ba5367a7b1c9c7374c4d2cc10e2 -- net.cpp

can be seen here: https://gist.github.com/theuni/cc4eb87f0196ddadb637d1a41c5c5b2e.

It's just #9050 plus @sipa's simplification suggestion.

TheBlueMatt · 2016-11-03T20:17:55Z

Looks like it needs rebased again, sorry :/.

sipa · 2016-11-03T21:05:38Z

The rebase is trivial: https://github.com/sipa/bitcoin/commits/pr_8708

9027680 net: handle version push in InitializeNode (Cory Fields) 7588b85 net: construct CNodeStates in place (Cory Fields) 440f1d3 net: remove now-unused ssSend and Fuzz (Cory Fields) 5c2169c drop the optimistic write counter hack (Cory Fields) ea33268 net: switch all callers to connman for pushing messages (Cory Fields) 3e32cd0 connman is in charge of pushing messages (Cory Fields) b98c14c serialization: teach serializers variadics (Cory Fields)

laanwj · 2016-11-07T09:37:00Z

Was merged via c8c572f (from sipa's rebased branch https://github.com/sipa/bitcoin/commits/pr_8708)

theuni · 2016-11-07T18:58:17Z

Thanks!

rebroad · 2016-11-11T12:52:39Z

src/main.cpp

@@ -18,6 +18,7 @@
 #include "init.h"
 #include "merkleblock.h"
 #include "net.h"
+#include "netbase.h"


what does it use in netbase.h?

@rebroad An easy way to find out is to remove the line and compile. The compiler will tell you what is used but missing.

Upstream serialization improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5264 - bitcoin/bitcoin#6914 - bitcoin/bitcoin#6215 - bitcoin/bitcoin#8068 - Only the `COMPACTSIZE` wrapper commit - bitcoin/bitcoin#8658 - bitcoin/bitcoin#8708 - Only the serializer variadics commit - bitcoin/bitcoin#9039 - bitcoin/bitcoin#9125 - Only the first two commits (the last two block on other upstream PRs) Part of #2074.

30d5c66 net: correct addrman logging (Fuzzbawls) 8a2b7fe Don't send layer2 messages to peers that haven't completed the handshake (Fuzzbawls) dc10100 [bugfix] Making tier two thread interruptable. (furszy) 2ae76aa Move CNode::addrLocal access behind locked accessors (Fuzzbawls) 470482f Move CNode::addrName accesses behind locked accessors (Fuzzbawls) 35365e1 Move [clean|str]SubVer writes/copyStats into a lock (Fuzzbawls) d816a86 Make nServices atomic (Matt Corallo) 8a66add Make nStartingHeight atomic (Matt Corallo) 567c9b5 Avoid copying CNodeStats to make helgrind OK with buggy std::string (Matt Corallo) aea5211 Make nTimeConnected const in CNode (Matt Corallo) cf46680 net: fix a few races. (Fuzzbawls) c916fcf net: add a lock around hSocket (Cory Fields) cc8a93c net: rearrange so that socket accesses can be grouped together (Cory Fields) 6f731dc Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set (Matt Corallo) 07c8d33 Ensure cs_vNodes is held when using the return value from FindNode (Matt Corallo) 110a44b Delete some unused (and broken) functions in CConnman (Matt Corallo) 08a12e0 net: log an error rather than asserting if send version is misused (Cory Fields) cd8b82c net: Disallow sending messages until the version handshake is complete (Fuzzbawls) 54b454b net: don't run callbacks on nodes that haven't completed the version handshake (Cory Fields) 2be6877 net: deserialize the entire version message locally (Fuzzbawls) 444f599 Dont deserialize nVersion into CNode (Fuzzbawls) f30f10e net: remove cs_vRecvMsg (Fuzzbawls) 5812f9e net: add a flag to indicate when a node's send buffer is full (Fuzzbawls) 5ec4db2 net: Hardcode protocol sizes and use fixed-size types (Wladimir J. van der Laan) de87ea6 net: Consistent checksum handling (Wladimir J. van der Laan) d4bcd25 net: push only raw data into CConnman (Cory Fields) b79e416 net: add CVectorWriter and CNetMsgMaker (Cory Fields) 63c51d3 net: No need to check individually for disconnection anymore (Cory Fields) 07d8c7b net: don't send any messages before handshake or after fdisconnect (Cory Fields) 9adfc7f net: Set feelers to disconnect at the end of the version message (Cory Fields) f88c06c net: handle version push in InitializeNode (Cory Fields) 04d39c8 net: construct CNodeStates in place (Cory Fields) 40a6c5d net: remove now-unused ssSend and Fuzz (Cory Fields) 681c62d drop the optimistic write counter hack (Cory Fields) 9f939f3 net: switch all callers to connman for pushing messages (Cory Fields) 8f9011d connman is in charge of pushing messages (Cory Fields) f558bb7 serialization: teach serializers variadics (Cory Fields) 01ea667 net: Use deterministic randomness for CNode's nonce, and make it const (Cory Fields) de1ad13 net: constify a few CNode vars to indicate that they're threadsafe (Cory Fields) 34050a3 Move static global randomizer seeds into CConnman (Pieter Wuille) 1ce349f net: add a flag to indicate when a node's process queue is full (Fuzzbawls) 5581b47 net: add a new message queue for the message processor (Fuzzbawls) 701b578 net: rework the way that the messagehandler sleeps (Fuzzbawls) 7e55dbf net: Add a simple function for waking the message handler (Cory Fields) 47ea844 net: record bytes written before notifying the message processor (Cory Fields) ffd4859 net: handle message accounting in ReceiveMsgBytes (Cory Fields) 8cee696 net: log bytes recv/sent per command (Fuzzbawls) 754400e net: set message deserialization version when it's time to deserialize (Fuzzbawls) d2b8e0a net: make CMessageHeader a dumb storage class (Fuzzbawls) cc24eff net: remove redundant max sendbuffer size check (Fuzzbawls) 32ab0c0 net: wait until the node is destroyed to delete its recv buffer (Cory Fields) 6e3f71b net: only disconnect if fDisconnect has been set (Cory Fields) 1b0beb6 net: make GetReceiveFloodSize public (Cory Fields) 229697a net: make vRecvMsg a list so that we can use splice() (Fuzzbawls) d2d71ba net: fix typo causing the wrong receive buffer size (Cory Fields) 50bb09d Add test-before-evict discipline to addrman (Ethan Heilman) Pull request description: This is a combination of multiple upstream PRs focused on optimizing the P2P networking flow after the introduction of CConnman encapsulation, and a few older PRs that were previously missed to support the later optimizations. The PRs are as follows: - bitcoin#9037 - net: Add test-before-evict discipline to addrman - bitcoin#5151 - make CMessageHeader a dumb storage class - bitcoin#6589 - log bytes recv/sent per command - bitcoin#8688 - Move static global randomizer seeds into CConnman - bitcoin#9050 - net: make a few values immutable, and use deterministic randomness for the localnonce - bitcoin#8708 - net: have CConnman handle message sending - bitcoin#9128 - net: Decouple CConnman and message serialization - bitcoin#8822 - net: Consistent checksum handling - bitcoin#9441 - Net: Massive speedup. Net locks overhaul - bitcoin#9609 - net: fix remaining net assertions - bitcoin#9626 - Clean up a few CConnman cs_vNodes/CNode things - bitcoin#9698 - net: fix socket close race - bitcoin#9708 - Clean up all known races/platform-specific UB at the time PR was opened - Excluded bitcoin/bitcoin@512731b and bitcoin/bitcoin@d8f2b8a, to be done in a separate PR ACKs for top commit: furszy: code ACK 30d5c66 , testnet sync from scratch went well and tested with #1829 on top as well and all good. furszy: mainnet sync went fine, ACK 30d5c66 . random-zebra: ACK 30d5c66 and merging... Tree-SHA512: 09689554f53115a45f810b47ff75d887fa9097ea05992a638dbb6055262aeecd82d6ce5aaa2284003399d839b6f2c36f897413da96cfa2cd3b858387c3f752c1

jonasschnelli added the P2P label Sep 13, 2016

maflcko mentioned this pull request Sep 16, 2016

net: fix maxuploadtarget setting #8707

Merged

theuni force-pushed the connman-send branch from 89c5742 to 2fe517f Compare September 30, 2016 19:36

theuni mentioned this pull request Oct 1, 2016

Fix a few cases where messages were sent after requested disconnect #8862

Merged

theuni force-pushed the connman-send branch from 2fe517f to c295afa Compare October 4, 2016 01:55

TheBlueMatt reviewed Oct 6, 2016

View reviewed changes

theuni force-pushed the connman-send branch from c295afa to f14fc2e Compare October 27, 2016 18:43

TheBlueMatt reviewed Oct 28, 2016

View reviewed changes

theuni force-pushed the connman-send branch from f14fc2e to 2df814b Compare October 29, 2016 20:37

theuni mentioned this pull request Oct 31, 2016

net: make a few values immutable, and use deterministic randomness for the localnonce #9050

Merged

TheBlueMatt reviewed Nov 1, 2016

View reviewed changes

laanwj reviewed Nov 2, 2016

View reviewed changes

src/net.h

void PushVersion();

void PushMessage(const char* pszCommand)

Copy link

Member

laanwj Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to get rid of this repetition.

laanwj reviewed Nov 2, 2016

View reviewed changes

sipa reviewed Nov 3, 2016

View reviewed changes

serialization: teach serializers variadics

db78f31

Also add a variadic CDataStream ctor for ease-of-use.

theuni added 6 commits November 3, 2016 09:56

net: switch all callers to connman for pushing messages

a6eff7c

Drop all of the old stuff.

drop the optimistic write counter hack

7f10bc8

This is now handled properly in realtime.

net: remove now-unused ssSend and Fuzz

22f2af6

net: construct CNodeStates in place

6ba267c

net: handle version push in InitializeNode

c7b8eff

theuni force-pushed the connman-send branch from 19d3dae to c7b8eff Compare November 3, 2016 14:06

laanwj closed this Nov 7, 2016

laanwj mentioned this pull request Nov 7, 2016

Various serialization simplifcations and optimizations #9039

Merged

This was referenced Nov 9, 2016

net: don't send feefilter messages before the version handshake is complete #9117

Merged

net: Decouple CConnman and message serialization #9128

Merged

rebroad reviewed Nov 11, 2016

View reviewed changes

OlegGirko mentioned this pull request Jul 26, 2017

Backport Bitcoin PR#8708: net: have CConnman handle message sending dashpay/dash#1553

Merged

str4d mentioned this pull request Apr 17, 2018

Upstream serialization improvements zcash/zcash#3180

Merged

dagurval mentioned this pull request Jun 8, 2018

More net cherries bitcoinxt/bitcoinxt#444

Merged

meshcollider moved this from In progress to Done in P2P refactor Dec 2, 2018

Fuzzbawls mentioned this pull request Sep 3, 2020

[Net] Massive network refactoring and speedup PIVX-Project/PIVX#1835

Merged

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

		@@ -136,6 +136,33 @@ class CConnman

		bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);

		template <typename... Args>

net: have CConnman handle message sending #8708

net: have CConnman handle message sending #8708

Conversation

theuni commented Sep 13, 2016

laanwj commented Sep 20, 2016

laanwj commented Sep 25, 2016

jtimon commented Sep 30, 2016

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Oct 6, 2016

TheBlueMatt commented Oct 6, 2016

laanwj commented Oct 25, 2016

theuni commented Oct 27, 2016

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Oct 29, 2016

TheBlueMatt left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Nov 2, 2016

sipa commented Nov 2, 2016

sipa commented Nov 2, 2016

theuni commented Nov 2, 2016

sipa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented Nov 3, 2016

TheBlueMatt commented Nov 3, 2016 • edited

sipa commented Nov 3, 2016

laanwj commented Nov 7, 2016

theuni commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Oct 6, 2016 •

edited

TheBlueMatt left a comment •

edited

TheBlueMatt commented Nov 3, 2016 •

edited