net: Decouple CConnman and message serialization

theuni commented at 6:28 am on November 11, 2016: member

This contains a more complete version of #9117, as well as the changes promised to @TheBlueMatt in #8708. They’re combined here because the PushMessage changes require that the SendVersion is sane. This will be my last time messing with PushMessage, and it’s nearing the end of the net changes needed outside of actual net code…

The changes here:

Don’t send messages after fDisconnect is set. (feeler connections are a TODO exception).
Don’t use CSerializeData for outgoing data, which incurred a memory_cleanse for each message. Messages aren’t secret, and going forward, if they are they’ll be encrypted.
Add a thin, featureless serializer to turn any set of args into a byte vector.
Add an go-between message maker so that CConnman sees only serialized messages for pushing.
That means that the caller is now responsible for determining the node’s send version. That’s a good thing, it can now move to CNodeState as later step.
CConnman attaches the header and hashes the byte vector. @jonasschnelli: This should be very easy to adapt to bip151. A new flag SERIALIZE_BIP151_ENCRYPTED or so can be added and used similar to SERIALIZE_TRANSACTION_NO_WITNESS.

fanquake added the label P2P on Nov 11, 2016

in src/main.cpp: in 2392f30d5f outdated

6508@@ -6502,9 +6509,13 @@ bool SendMessages(CNode* pto, CConnman& connman)
6509     const Consensus::Params& consensusParams = Params().GetConsensus();
6510     {
6511         // Don't send anything until we get its version message
6512-        if (pto->nVersion == 0)
6513+        if (!pto->fSuccessfullyConnected || pto->fDisconnect)

rebroad commented at 6:50 am on November 11, 2016:

thank you!!

dcousens commented at 6:52 am on November 11, 2016:

// Don’t send anything until we get its version message

Comment is out-of-date?

theuni commented at 7:03 am on November 11, 2016:

@dcousens the version message sets fSuccessfullyConnected, but I guess that’s not as clear after this change.

How about “don’t send anything until we’re successfully connected and ok with the peer’s version” ?

dcousens commented at 7:27 am on November 11, 2016:

I think if we are still referencing version here to explain why, then fSuccessfullyConnected isn’t appropriate. Otherwise, just say “don’t send anything until we’re successfully connected”.

theuni commented at 7:04 am on November 11, 2016: member

Grr, will fix up the Windows build problem in the morning.

venzen commented at 7:04 am on November 11, 2016: none

netmessagemaker and handling of initial Version message in main.cpp are good improvements.

TheBlueMatt commented at 2:46 am on November 12, 2016: member

Concept ACK, though I’m not convinced there arent too many copies when making the message and passing it into CConnman, but will audit when you fix up build, etc.

theuni commented at 4:11 am on November 12, 2016: member

@TheBlueMatt there’s only 1 copy (other than the serialization itself), and it comes from combining the header and payload. See the note here: https://github.com/bitcoin/bitcoin/pull/9128/files#diff-9a82240fe7dfe86564178691cc57f2f1R2590

Either way, it for sure cuts down on copies compared to before.

in src/main.cpp: in 2392f30d5f outdated

5729@@ -5723,8 +5730,8 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
5730                 pfrom->id,
5731                 FormatStateMessage(state));
5732             if (state.GetRejectCode() < REJECT_INTERNAL) // Never send AcceptToMemoryPool's internal codes over P2P
5733-                connman.PushMessage(pfrom, NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),
5734-                                   state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash);
5735+                 connman.PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),

jonasschnelli commented at 7:52 am on November 15, 2016:

nit: indent

jonasschnelli commented at 7:54 am on November 15, 2016: contributor

Concept ACK.

laanwj commented at 9:49 am on November 15, 2016: member

winsok needs an extra cast:

0../../src/net.cpp: In function ‘size_t SocketSendData(CNode*)’:
1../../src/net.cpp:777:131: error: invalid conversion from ‘const value_type* {aka const unsigned char*}’ to ‘const char*’ [-fpermissive]
2         int nBytes = send(pnode->hSocket, &data[pnode->nSendOffset], data.size() - pnode->nSendOffset, MSG_NOSIGNAL | MSG_DONTWAIT);

theuni force-pushed on Nov 16, 2016

theuni commented at 5:39 am on November 16, 2016: member

Fixed several things:

Fixed @jonasschnelli’s whitespace nit
Fixed win32 build, broken due to missing cast
Addressed @TheBlueMatt’s complaints:
- Back to using pnode->nVersion == 0 to detect lack of version message
- Added a change to hold off on setting fDisconnect for feelers until later, to avoid attempting to send messages to a disconnected node. The behavior is kept the same here, but imo it should be changed as a follow-up.
- zero copies when sending to the net. CSerializedNetMsg now advises how much padding it may possibly need. Its copy ctor and copy assignment operator are deleted to ensure no copies.
- CVectorInserter::pad and CVectorOverWriter::skip. I’m not sure if these are exactly what you needed, but I took a guess :)

laanwj commented at 3:09 pm on November 17, 2016: member

I’m testing this + #9125

sipa commented at 9:25 pm on November 17, 2016: member

Needs rebase after #9075.

in src/streams.h: in 28b1cca20d outdated

147+    }
148+    int GetType() const
149+    {
150+        return nType;
151+    }
152+    void skip(size_t nSize)

sipa commented at 10:01 pm on November 17, 2016:

CSizeComputer uses ‘seek’ for this function (inspired by the dd command, where skip is something you do on input, and seek is something you do on output).

theuni commented at 10:06 pm on November 17, 2016:

Makes sense. Will change.

in src/streams.h: in 28b1cca20d outdated

115+
116+/* Minimal stream for overwriting the contents of an existing byte vector without causing reallocations
117+ *
118+ * Note that the referenced vector must already be large enough to hold all data to be serialized.
119+ */
120+class CVectorOverWriter

sipa commented at 10:11 pm on November 17, 2016:

I’m not convinced there is a need to separate CVectorOverWriter and and CVectorInserter. Couldn’t you have a single one, which a constructor that initialized the write pointer at the beginning, and one that initializes it at the end?

Writes would involve first checking whether we’d not go out of bounds, and resizing if necessary.

theuni commented at 10:23 pm on November 17, 2016:

Yes, I think that should work fine. Will change.

in src/net.cpp: in 6d331d6736 outdated

2607-    if(strm.empty())
2608-        return;
2609-
2610-    unsigned int nSize = strm.size() - CMessageHeader::HEADER_SIZE;
2611-    LogPrint("net", "sending %s (%d bytes) peer=%d\n",  SanitizeString(sCommand.c_str()), nSize, pnode->id);
2612+    uint256 hash = Hash(msg.data.data() + msg.header_pad_size, msg.data.data() + msg.data.size());

TheBlueMatt commented at 6:27 am on November 18, 2016:

Shouldnt this happen inside CNetMsgMaker::Make? That way we can just have a BIP151MsgMaker?

theuni commented at 7:50 am on November 18, 2016:

It was left out of CNetMsgMaker because I assumed that we’d want CNode making the decision about how to wrap the data for sending. If it’s to be encrypted, we’ll need some per-node state vars.

So I guess it depends whether encryption status and state are to end up somewhere in main like CNodeState, or in CNode. Since (for once) this seems like something CConnman should know about but nobody else should have to care, I figured CNode was the likely place.

theuni commented at 7:58 am on November 18, 2016:

I should mention: I’ve gone back and forth a few times on this (probably in this PR even). If anyone has a strong preference for a different approach that makes sense, I’m fine with changing it.

TheBlueMatt commented at 8:09 am on November 18, 2016:

This isnt in CNode, though? If it were in CNode I’d be OK with that, but putting it in CConnman seems like /the/ layer violation this PR intends to fix?

theuni commented at 8:18 am on November 18, 2016:

The result of this PR is that CConnman now receives only dumb bytes with no knowledge of what they are. All internal structures could change without affecting the net side.

Dealing with wrapping the raw bytes up for transmission is something different, and arguably not something that the bitcoin side should have to know/care about. If you have a different model in your mind, please suggest it.

CNode is slowly moving towards an internal dumb storage class. So in this case, I don’t make much distinction.

in src/net.cpp: in 6d331d6736 outdated

2580@@ -2581,30 +2581,17 @@ void CNode::AskFor(const CInv& inv)
2581     mapAskFor.insert(std::make_pair(nRequestTime, inv));
2582 }
2583 
2584-CDataStream CConnman::BeginMessage(CNode* pnode, int nVersion, int flags, const std::string& sCommand)
2585+void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg msg)

TheBlueMatt commented at 6:30 am on November 18, 2016:

Shouldn’t this be CSerializedNetMsg&&? It seems super strange that we’re secretly destroying the object passed in, but only because the CSerializedNetMsg class doesnt have a copy-constructor.

theuni commented at 7:25 am on November 18, 2016:

Sure.

sipa commented at 7:30 am on November 18, 2016:

Nothing secret is being done. This would fail, for example:

0CSerializedNetMsg x = ...;
1pconman->PushMessage(pnode, x);

as the binding of the actual parameter x would need a copy constructor to assign to the formal parameter msg.

Passing a && would be preferable, though. The move construction here is slower, and only adds the possibility of passing in something that is implicitly convertible to a CSerializedNetMsg (which afaik doesn’t exist).

theuni commented at 8:03 am on November 18, 2016:

It was done this way originally so that a message could be serialized once and sent to multiple peers. If an rvalue is required, we can’t do that.

I don’t think there’s a use-case for that at the moment, though, so changing to && is fine by me.

in src/netmessagemaker.h: in 6d331d6736 outdated

17+    template <typename... Args>
18+    CSerializedNetMsg Make(int nFlags, std::string sCommand, Args&&... args)
19+    {
20+        CSerializedNetMsg msg;
21+        msg.data.assign(CSerializedNetMsg::header_pad_size, 0);
22+        msg.command = std::move(sCommand);

TheBlueMatt commented at 6:34 am on November 18, 2016:

Over-use of std::move, much? Can we stick with const std::string& in the function declaration and then use operator=() to make the copy instead of creating a new object just to std::move the data over? (Also, could definitely be missing some part of rvalue stuff, here)

theuni commented at 7:42 am on November 18, 2016:

This is standard c++11 pass-by-value-by-default. If you pass in a const-reference, a copy is guaranteed since we’re storing the result. If you pass by value, it can be moved and the copy can be skipped.

Not sure why you’d prefer a copy over 2 cheap moves.

Arguably NetMsgType should become a std::array<char, CMessageHeader::COMMAND_SIZE> so we can do away with the char arrays/strings, but that’s a change for another day.

TheBlueMatt commented at 7:51 am on November 18, 2016:

Yup, didnt realize it would avoid a copy there (though I’d be OK copying the sCommand field…), anyway, comment withdrawn.

TheBlueMatt commented at 6:41 am on November 18, 2016: member

Again didnt do a full review, just looked over the actual code changes and came up with a few comments…will do a full review when they’re addressed.

theuni force-pushed on Nov 18, 2016

theuni commented at 8:09 am on November 18, 2016: member

Rebased and updated for comments from @sipa and @TheBlueMatt.

Combined the vector serializers into CVectorWriter as requested.
skip() -> seek()
Added tests for CVectorWriter.
PushMessage requires an rvalue

sipa commented at 8:13 am on November 18, 2016: member

Hmm, I would consider CNode and CConnman to be the same layer (after all its processing related stated is moved out, and the rest is made private to the net module).

TheBlueMatt commented at 8:16 am on November 18, 2016: member

Heh, well if you consider CNode to be a part of CConnman, then the type-of-message-to-send logic should be in net_processing…either way CConnman shouldnt know anything about the serialization of messages on the wire.

MarcoFalke commented at 11:08 pm on November 21, 2016: member

Needs rebase

theuni force-pushed on Nov 22, 2016

laanwj commented at 7:02 am on November 23, 2016: member

Code review ACK 02d463d Have been testing an earlier revision on a node for 6 days without issues.

theuni force-pushed on Nov 23, 2016

theuni commented at 10:08 pm on November 23, 2016: member

updated after a discussion with @TheBlueMatt and @sipa today, I think we’re all finally aligned. @laanwj Sadly removing the “skb” was part of the compromise as it was a slight layer violation. We instead use 2 send()s (later scatter/gather) to avoid the copy.

sipa commented at 0:18 am on November 24, 2016: member

Needs rebase, sorry!

net: Set feelers to disconnect at the end of the version message

This way we're not relying on messages going out after fDisconnect has been
set.

This should not cause any real behavioral changes, though feelers should
arguably disconnect earlier in the process. That can be addressed in a later
functional change.

d74e352e01

net: don't send any messages before handshake or after requested disconnect

Also, send reject messages earlier in SendMessages(), so that disconnections are
processed earlier.

These changes combined should ensure that no message is ever sent after
fDisconnect is set.

fedea8a14d

theuni force-pushed on Nov 24, 2016

theuni commented at 1:09 am on November 24, 2016: member

Rebased after ca8549d2bd32f17f8b69d1edbe3f2976fba504b4.

sipa commented at 3:30 am on November 24, 2016: member

utACK 09a92c27d46a62d10aed95b742e891e204dcee4e

paveljanik commented at 11:33 am on November 24, 2016: contributor

ACK 09a92c2

in src/streams.h: in 09a92c27d4 outdated

80+/*
81+ * @param[in]  nTypeIn Serialization Type
82+ * @param[in]  nVersionIn Serialization Version (including any flags)
83+ * @param[in]  vchDataIn  Referenced byte vector to overwrite/append
84+ * @param[in]  nPosIn Starting position. Vector index where writes should start. If the index is past the end, the
85+ *                    vector will first be resized to fit. So to append, use vec.size().

TheBlueMatt commented at 9:03 pm on November 24, 2016:

This comment is very confusing…if nPos is 1 and vchData.size() is 1, the documentation implies that vchData will be resized so that nPos isnt past-the-end, but it will not be. Instead, vchData is resized so that, at maximum, nPos is 1-past-the-end.

theuni commented at 4:57 pm on November 25, 2016:

Hmm, yes. Will clarify.

in src/net.cpp: in 09a92c27d4 outdated

2629-    memcpy((char*)&strm[CMessageHeader::CHECKSUM_OFFSET], hash.begin(), CMessageHeader::CHECKSUM_SIZE);
2630-
2631-}
2632+    size_t nMessageSize = msg.data.size();
2633+    size_t nTotalSize = nMessageSize + CMessageHeader::HEADER_SIZE;
2634+    LogPrint("net", "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.command.c_str()), nTotalSize, pnode->id);

TheBlueMatt commented at 9:29 pm on November 24, 2016:

You changed the print - should print nMessageSize, not nTotalSize.

theuni commented at 4:13 pm on November 25, 2016:

Good call. I assumed it’d never say “sending 0 bytes”, but you’re right, I’ll avoid a behavioral change here.

TheBlueMatt commented at 9:29 pm on November 24, 2016: member

You missed one fDisconnect check in SendMessages (the last one - feefilter, maybe a rebase issue?).

net: No need to check individually for disconnection anymore b7695c2275

net: add CVectorWriter and CNetMsgMaker

CVectorWriter is useful for overwriting or appending an existing byte vector.

CNetMsgMaker is a shortcut for creating messages on-the-fly which are suitable
for pushing to CConnman.

2ec935dcaa

net: push only raw data into CConnman

This fixes one of the last major layer violations in the networking stack.

The network side is no longer in charge of message serialization, so it is now
decoupled from Bitcoin structures. Only the header is serialized and attached
to the payload.

c7be56dcef

theuni force-pushed on Nov 25, 2016

theuni commented at 5:40 pm on November 25, 2016: member

Updated and squashed for @TheBlueMatt’s comments. The diff from before is very straightforward:

 0cory@cory-i7:~/dev/bitcoin/src(connman-send)$ git diff theuni/connman-send
 1diff --git a/src/main.cpp b/src/main.cpp
 2index 4520afb..5595381 100644
 3--- a/src/main.cpp
 4+++ b/src/main.cpp
 5@@ -6983,7 +6983,7 @@ bool SendMessages(CNode* pto, CConnman& connman)
 6         // Message: feefilter
 7         //
 8         // We don't want white listed peers to filter txs to us if we have -whitelistforcerelay
 9-        if (!pto->fDisconnect && pto->nVersion >= FEEFILTER_VERSION && GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
10+        if (pto->nVersion >= FEEFILTER_VERSION && GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
11             !(pto->fWhitelisted && GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY))) {
12             CAmount currentFilter = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
13             int64_t timeNow = GetTimeMicros();
14diff --git a/src/net.cpp b/src/net.cpp
15index e05c174..ce87150 100644
16--- a/src/net.cpp
17+++ b/src/net.cpp
18@@ -2616,7 +2616,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
19 {
20     size_t nMessageSize = msg.data.size();
21     size_t nTotalSize = nMessageSize + CMessageHeader::HEADER_SIZE;
22-    LogPrint("net", "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.command.c_str()), nTotalSize, pnode->id);
23+    LogPrint("net", "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.command.c_str()), nMessageSize, pnode->id);
24 
25     std::vector<unsigned char> serializedHeader;
26     serializedHeader.reserve(CMessageHeader::HEADER_SIZE);
27diff --git a/src/streams.h b/src/streams.h
28index ef22620..b508784 100644
29--- a/src/streams.h
30+++ b/src/streams.h
31@@ -81,8 +81,8 @@ class CVectorWriter
32  * [@param](/bitcoin-bitcoin/contributor/param/)[in]  nTypeIn Serialization Type
33  * [@param](/bitcoin-bitcoin/contributor/param/)[in]  nVersionIn Serialization Version (including any flags)
34  * [@param](/bitcoin-bitcoin/contributor/param/)[in]  vchDataIn  Referenced byte vector to overwrite/append
35- * [@param](/bitcoin-bitcoin/contributor/param/)[in]  nPosIn Starting position. Vector index where writes should start. If the index is past the end, the
36- *                    vector will first be resized to fit. So to append, use vec.size().
37+ * [@param](/bitcoin-bitcoin/contributor/param/)[in]  nPosIn Starting position. Vector index where writes should start. The vector will initially
38+ *                    grow as necessary to  max(index, vec.size()). So to append, use vec.size().
39 */
40     CVectorWriter(int nTypeIn, int nVersionIn, std::vector<unsigned char>& vchDataIn, size_t nPosIn) : nType(nTypeIn), nVersion(nVersionIn), vchData(vchDataIn), nPos(nPosIn)
41     {

TheBlueMatt commented at 6:14 pm on November 25, 2016: member

utACK c7be56dceff625991cb2884f2ce053996ac613cd

sipa merged this on Nov 25, 2016

sipa closed this on Nov 25, 2016

sipa referenced this in commit 76fec09d87 on Nov 25, 2016

codablock referenced this in commit c2e469097a on Jan 16, 2018

codablock referenced this in commit ed266c0a4e on Jan 16, 2018

codablock referenced this in commit 5c0b55a7c0 on Jan 17, 2018

andvgal referenced this in commit 8ccc1e98a6 on Jan 6, 2019

CryptoCentric referenced this in commit 609e23370c on Feb 25, 2019

random-zebra referenced this in commit cbd9271afb on Sep 7, 2020

DrahtBot locked this on Sep 8, 2021

net: Decouple CConnman and message serialization #9128