net: Refactor message parsing (CNetMessage), adds flexibility

jonasschnelli commented at 9:20 am on August 24, 2018: contributor

This refactors the code how incoming messages gets handled. It adds flexibility for adding new transport types (encryption handshake, new message format).

This is a subset of #14032 and a pre-requirement for BIP151.

jonasschnelli added the label P2P on Aug 24, 2018

in src/net_message.h:21 in 59a7ce0f9d outdated

16+// base class for format agnostic network messages
17+class NetMessageBase
18+{
19+public:
20+    CDataStream vRecv; // received message data
21+    int64_t nTime;     // time (in microseconds) of message receipt.

scravy commented at 10:00 am on August 24, 2018:

According to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md these should be prefixed m_.

jonasschnelli commented at 10:02 am on August 24, 2018:

I kept it because it is moved code and to simplify the review.

promag commented at 10:02 am on August 24, 2018:

This is moved code right?

promag commented at 10:01 am on August 24, 2018: member

Any reason to not use unique_ptr instead? I don’t see sharing of message ownership.

jonasschnelli commented at 11:14 am on August 24, 2018: contributor

I think shared is fine here and does allow more flexible handling in future with little cost (ref counting)

MarcoFalke renamed this:
~~Refactor message parsing (CNetMessage), adds flexibility~~
net: Refactor message parsing (CNetMessage), adds flexibility
on Aug 24, 2018

DrahtBot commented at 12:16 pm on August 24, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16562 (Refactor message transport packaging by jonasschnelli)
#16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
#16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
#16273 (refactor: Remove unused includes by practicalswift)
#16248 (Make whitebind/whitelist permissions more flexible by NicolasDorier)
#16202 (Refactor network message deserialization by jonasschnelli)
#15206 (Immediately disconnect on invalid net message checksum by jonasschnelli)
#15197 (Refactor and slightly stricter p2p message processing by jonasschnelli)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke commented at 1:27 pm on August 24, 2018: member

utACK 59a7ce0f9de950b64c5e89dd29ebcd0b607b982e

gmaxwell commented at 4:40 pm on August 24, 2018: contributor

Adding an extra memory allocation for every network message isn’t free. I’m concerned that we may be falling into overusing sharedptr “just in case”, which is bad because it leaks performance in a diffuse way that doesn’t show up well in profiles. Please don’t use sharedptr where unique will work. If something new comes in in the future that needs a sharedptr then we can switch then.

sipa commented at 5:15 pm on August 24, 2018: member

@gmaxwell unique_ptr and shared_ptr have the same number of allocations (if make_shared is used, at least), though shared_ptr uses more memory (both for the pointer itself and for the allocated memory), and has atomics to update the refcounts (which may reduce performance).

To be clear, I agree we should avoid shared_ptr unless there is a good reason.

in src/net.cpp:755 in 9f5702205d outdated

754+        if (vRecvMsg.empty() || vRecvMsg.back()->complete()) {
755+            vRecvMsg.emplace_back(std::make_shared<CNetMessage>(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));
756+        }
757 
758-        CNetMessage& msg = vRecvMsg.back();
759+        CNetMessageRef msg = vRecvMsg.back();

MarcoFalke commented at 5:28 pm on August 24, 2018:

nit: In commit " Use shared pointers for CNetMessage handling " you could keep this a plain reference to keep the diff smaller, I think.

in src/net_processing.cpp:3016 in 9f5702205d outdated

3014+        pfrom->nProcessQueueSize -= msgs.front()->vRecv.size() + CMessageHeader::HEADER_SIZE;
3015         pfrom->fPauseRecv = pfrom->nProcessQueueSize > connman->GetReceiveFloodSize();
3016         fMoreWork = !pfrom->vProcessMsg.empty();
3017     }
3018-    CNetMessage& msg(msgs.front());
3019+    CNetMessageRef msg(msgs.front());

MarcoFalke commented at 5:28 pm on August 24, 2018:

Same here

in src/net_message.h:18 in 59a7ce0f9d outdated

13+/** Maximum length of incoming protocol messages (no message over 4 MB is currently acceptable). */
14+static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;
15+
16+// base class for format agnostic network messages
17+class NetMessageBase
18+{

l2a5b1 commented at 8:31 pm on August 26, 2018:

Implement a virtual destructor to protect against undefined behavior in the event that an object of a derived NetMessageBase class with a destructor is destroyed through a pointer to this base class.

0virtual ~NetMessageBase() {}

jonasschnelli commented at 11:34 am on August 27, 2018:

Done

in src/net_message.h:35 in 59a7ce0f9d outdated

29+    virtual uint32_t getMessageSize() const = 0;           //returns 0 when message has not yet been parsed
30+    virtual uint32_t getMessageSizeWithHeader() const = 0; //return complete message size including header
31+
32+    virtual std::string getCommandName() const = 0; //returns the command name. Returns an empty string when no command name is supported
33+
34+    void SetVersion(int nVersionIn)

l2a5b1 commented at 8:31 pm on August 26, 2018:

Make SetVersion a virtual member function to be sure that derived implementations are called when called via a pointer to this base class.

0virtual void SetVersion(int nVersionIn)

in src/net_message.h:44 in 59a7ce0f9d outdated

38+
39+    virtual int read(const char* pch, unsigned int nBytes) = 0; //parse bytes
40+
41+    virtual bool verifyMessageStart() const = 0;
42+    virtual bool verifyHeader() const = 0;
43+    virtual bool verifyChecksum(std::string& error) const = 0;

l2a5b1 commented at 8:33 pm on August 26, 2018:

The developer notes state that for new symbols it is preferred that: “Class names, function names and method names are UpperCamelCase (PascalCase)”

0     virtual bool VerifyMessageStart() const = 0;
1     virtual bool VerifyHeader() const = 0;
2     virtual bool VerifyChecksum(std::string& error) const = 0;

Can you check if all the new method names are UpperCamelCase?

in src/net_message.h:23 in 59a7ce0f9d outdated

18+{
19+public:
20+    CDataStream vRecv; // received message data
21+    int64_t nTime;     // time (in microseconds) of message receipt.
22+
23+    NetMessageBase(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : vRecv(nTypeIn, nVersionIn)

l2a5b1 commented at 8:50 pm on August 26, 2018:

Remove the const CMessageHeader::MessageStartChars& pchMessageStartIn argument from the constructor’s argument list?

0NetMessageBase(int nTypeIn, int nVersionIn) : vRecv(nTypeIn, nVersionIn)

in src/net_message.h:110 in 59a7ce0f9d outdated

104+    int readHeader(const char* pch, unsigned int nBytes);
105+    int readData(const char* pch, unsigned int nBytes);
106+
107+    bool verifyMessageStart() const;
108+    bool verifyHeader() const;
109+    bool verifyChecksum(std::string& error) const;

l2a5b1 commented at 8:55 pm on August 26, 2018:

Use override to ensure that these methods are overriding virtual methods from the base class.

0    bool VerifyMessageStart() const override;
1    bool VerifyHeader() const override;
2    bool VerifyChecksum(std::string& error) const override;

in src/net_message.h:71 in 59a7ce0f9d outdated

65+        in_data = false;
66+        nHdrPos = 0;
67+        nDataPos = 0;
68+    }
69+
70+    bool complete() const

l2a5b1 commented at 8:56 pm on August 26, 2018:

Specify this method as override

0bool complete() const override

in src/net_message.h:78 in 59a7ce0f9d outdated

72+        if (!in_data)
73+            return false;
74+        return (hdr.nMessageSize == nDataPos);
75+    }
76+
77+    uint32_t getMessageSize() const

l2a5b1 commented at 8:56 pm on August 26, 2018:

Specify this method as override

0uint32_t getMessageSize() const override

in src/net_message.h:86 in 59a7ce0f9d outdated

80+            return 0;
81+        }
82+        return hdr.nMessageSize;
83+    }
84+
85+    uint32_t getMessageSizeWithHeader() const

l2a5b1 commented at 8:57 pm on August 26, 2018:

Specify this method as override

0uint32_t getMessageSizeWithHeader() const override

in src/net_message.h:91 in 59a7ce0f9d outdated

85+    uint32_t getMessageSizeWithHeader() const
86+    {
87+        return hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
88+    }
89+
90+    std::string getCommandName() const

l2a5b1 commented at 8:57 pm on August 26, 2018:

Specify this method as override

0std::string getCommandName() const override

in src/net_message.h:98 in 59a7ce0f9d outdated

92+        return hdr.pchCommand;
93+    }
94+
95+    const uint256& GetMessageHash() const;
96+
97+    void SetVersion(int nVersionIn)

l2a5b1 commented at 8:58 pm on August 26, 2018:

Specify as override

0void SetVersion(int nVersionIn) override

in src/net_message.h:104 in 59a7ce0f9d outdated

 98+    {
 99+        hdrbuf.SetVersion(nVersionIn);
100+        NetMessageBase::SetVersion(nVersionIn);
101+    }
102+
103+    int read(const char* pch, unsigned int nBytes);

l2a5b1 commented at 8:59 pm on August 26, 2018:

Specify as override

0int read(const char* pch, unsigned int nBytes) override;

in src/net_message.h:112 in 59a7ce0f9d outdated

107+    bool verifyMessageStart() const;
108+    bool verifyHeader() const;
109+    bool verifyChecksum(std::string& error) const;
110+};
111+
112+typedef std::shared_ptr<NetMessageBase> NetMessageBaseRef;

l2a5b1 commented at 8:59 pm on August 26, 2018:

Prefer using over typedef?

0using NetMessageBaseRef = std::shared_ptr<NetMessageBase>;

in src/net_message.h:113 in 59a7ce0f9d outdated

108+    bool verifyHeader() const;
109+    bool verifyChecksum(std::string& error) const;
110+};
111+
112+typedef std::shared_ptr<NetMessageBase> NetMessageBaseRef;
113+typedef std::shared_ptr<NetMessage> NetMessageRef;

l2a5b1 commented at 9:00 pm on August 26, 2018:

NetMessageRef doesn’t seem to be used. Can NetMessageRef be deleted?

jonasschnelli force-pushed on Aug 27, 2018

jonasschnelli commented at 11:19 am on August 27, 2018: contributor

@sipa, @gmaxwell: The reason why I used shared_ptr over unique_ptr is the polymorphism I’d like to use for the net messages and I once read that shared_ptr are recommended for polymorphic inheritance.

However, I changed this PR now to use unique_ptr.

Thanks @251Labs for the review, implemented all your suggestions.

jonasschnelli force-pushed on Aug 27, 2018

in src/net.h:579 in df04817181 outdated

574+    CDataStream vRecv; // received message data
575+    int64_t nTime;     // time (in microseconds) of message receipt.
576+
577+    NetMessageBase(int nTypeIn, int nVersionIn) : vRecv(nTypeIn, nVersionIn)
578+    {
579+        nTime = 0;

promag commented at 0:29 am on August 29, 2018:

could move to init list , nTime(0).

promag commented at 0:33 am on August 29, 2018: member

Fix commit message “Use shared pointers for CNetMessage handling”.

jonasschnelli referenced this in commit bd9e18e797 on Aug 29, 2018

laanwj added this to the "Blockers" column in a project

jonasschnelli force-pushed on Aug 31, 2018

jonasschnelli commented at 7:51 pm on August 31, 2018: contributor

Fixed commit message issue reported by @promag

in src/net.h:636 in 689e20ddc5 outdated

632@@ -681,6 +633,7 @@ class CNode
633 
634     mapMsgCmdSize mapSendBytesPerMsgCmd;
635     mapMsgCmdSize mapRecvBytesPerMsgCmd;
636+    void RecordRecvBytesPerMsgCmd(const std::string& cmd, uint32_t bytes);

Empact commented at 8:28 pm on September 1, 2018:

nit: could be private

jonasschnelli commented at 7:52 am on September 18, 2018:

Fixed.

in src/net_message.h:87 in 689e20ddc5 outdated

82+        return hdr.nMessageSize;
83+    }
84+
85+    uint32_t GetMessageSizeWithHeader() const override
86+    {
87+        return hdr.nMessageSize + CMessageHeader::HEADER_SIZE;

Empact commented at 9:06 pm on September 1, 2018:

Seems these other hdr accesses be guarded against !in_data, as they’re relatively uninitialized data in that case, e.g.nMessageSize could be -1.

promag commented at 0:18 am on September 4, 2018:

@Empact looks like that would be a bad call, maybe assert(in_data) or hdr.nMessageSize >= 0?

jonasschnelli commented at 7:43 am on September 18, 2018:

I basically moved this code part… I think its fine.

in src/net_message.cpp:78 in 689e20ddc5 outdated

73+    return data_hash;
74+}
75+
76+bool NetMessage::VerifyMessageStart() const
77+{
78+    return memcmp(hdr.pchMessageStart, Params().MessageStart(), CMessageHeader::MESSAGE_START_SIZE) == 0 ? true : false;

practicalswift commented at 5:20 pm on September 10, 2018:

0src/net_message.cpp:78:108: warning: redundant boolean literal in ternary expression result [readability-simplify-boolean-expr]

jonasschnelli commented at 7:48 am on September 18, 2018:

Fixed.

in src/net_message.cpp:61 in 689e20ddc5 outdated

56+    if (vRecv.size() < nDataPos + nCopy) {
57+        // Allocate up to 256 KiB ahead, but never more than the total message size.
58+        vRecv.resize(std::min(hdr.nMessageSize, nDataPos + nCopy + 256 * 1024));
59+    }
60+
61+    hasher.Write((const unsigned char*)pch, nCopy);

practicalswift commented at 5:21 pm on September 10, 2018:

0src/net_message.cpp:61:18: warning: C-style casts are discouraged; use reinterpret_cast [google-readability-casting]
1src/net_message.cpp:61:18: warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

jonasschnelli commented at 7:47 am on September 18, 2018:

It’s moved code and the scope of this PR is not refactor.

jonasschnelli force-pushed on Sep 18, 2018

in src/net.h:584 in 1d294afb9f outdated

579+        nTime = 0;
580+    }
581+    virtual ~NetMessageBase() {}
582 
583+    virtual bool Complete() const = 0;
584+    virtual uint32_t GetMessageSize() const = 0;           //returns 0 when message has not yet been parsed

ajtowns commented at 2:47 pm on September 18, 2018:

nit: missing space after // ?

in src/net.cpp:1309 in c2f9c04583 outdated

1305@@ -1372,9 +1306,9 @@ void CConnman::ThreadSocketHandler()
1306                         size_t nSizeAdded = 0;
1307                         auto it(pnode->vRecvMsg.begin());
1308                         for (; it != pnode->vRecvMsg.end(); ++it) {
1309-                            if (!it->complete())
1310+                            if (!it->get()->Complete())

ajtowns commented at 3:38 pm on September 18, 2018:

Could be (*it)->Complete() instead, but maybe what you’ve got is already clearer?

ajtowns commented at 4:05 pm on September 18, 2018: member

c2f9c04583a3ffcae4a54d85bb49def58e79cef2 looks reasonable to me

in src/net_message.h:21 in c2f9c04583 outdated

16+// base class for format agnostic network messages
17+class NetMessageBase
18+{
19+public:
20+    CDataStream vRecv; // received message data
21+    int64_t nTime;     // time (in microseconds) of message receipt.

promag commented at 2:33 pm on September 26, 2018:

nit, int64_t nTime{0}; and remove from the constructor.

l2a5b1 commented at 8:07 pm on September 26, 2018:

That’s a nice nit @promag. Alternatively, nTime can be initialized with an empty initializer list int64_t nTime {};

jonasschnelli commented at 9:02 am on September 27, 2018:

Agree. Though I’d say scope of the PR is not to refactor code parts that where moved 1:1. Lets do further refactors in another PR.

promag commented at 9:06 am on September 27, 2018:

SGTM.

promag commented at 2:35 pm on September 26, 2018: member

utACK c2f9c04.

in src/net_message.cpp:23 in f412d34503 outdated

18+}
19+
20+int NetMessage::ReadHeader(const char* pch, unsigned int nBytes)
21+{
22+    // copy data to temporary parsing buffer
23+    unsigned int nRemaining = 24 - nHdrPos;

etscrivner commented at 0:23 am on September 27, 2018:

I see the magic constant 24 here quite a bit. Would it be useful to extract this into an appropriately named constant? Perhaps HEADER_NUM_BYTES or equivalent?

jonasschnelli commented at 9:02 am on September 27, 2018:

Agree. Though I’d say scope of the PR is not to refactor code parts that where moved 1:1. Lets do further refactors in another PR.

etscrivner commented at 1:13 am on September 27, 2018: contributor

utACK c2f9c04583a3ffcae4a54d85bb49def58e79cef2

etscrivner commented at 3:34 pm on September 27, 2018: contributor

ACK c2f9c04583a3ffcae4a54d85bb49def58e79cef2

in src/net_message.h:47 in c2f9c04583 outdated

42+    virtual bool VerifyMessageStart() const = 0;
43+    virtual bool VerifyHeader() const = 0;
44+    virtual bool VerifyChecksum(std::string& error) const = 0;
45+};
46+
47+//basic network message for the currently used unencryted p2p communication

practicalswift commented at 8:22 pm on October 16, 2018:

Should be “unencrypted” :-)

jonasschnelli commented at 2:34 pm on October 17, 2018:

Fixed.

jonasschnelli force-pushed on Oct 17, 2018

jonasschnelli commented at 2:34 pm on October 17, 2018: contributor

Rebased.

jonasschnelli force-pushed on Oct 17, 2018

promag commented at 7:14 am on October 19, 2018: member

utACK c2f9c04.

sipa commented at 11:43 pm on October 19, 2018: member

Is it really necessary to make CNetMessage a virtual class? This seems very much overkill. Only the network deserializers should change based on what protocol is used; we don’t need to have that variability put a burden on every separate message.

Seems like you could just create a CNetMessageSerializer class (with different instances), to which some of the current CNetMessage logic is moved. This class is then responsible for turning the network byte stream into packets, and the other way around, but the CNetMessage objects remain independent of the protocol used.

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke commented at 8:07 pm on November 7, 2018: member

@jonasschnelli Mind to follow up on @sipa’s question? I have removed this from high priority for now, but happy to add back when you addressed the question.

DrahtBot added the label Needs rebase on Nov 28, 2018

jonasschnelli force-pushed on Mar 11, 2019

DrahtBot removed the label Needs rebase on Mar 11, 2019

jonasschnelli force-pushed on Mar 11, 2019

DrahtBot added the label Needs rebase on Apr 4, 2019

Use unique pointers for CNetMessage handling d67c0765d6

Split off format specific details from CNetMessage 6efdd02123

Move NetMessage (NetMessageBase) to net_message.h/cpp 76dacb63b0

Factor out RecordRecvBytesPerMsgCmd() c5816dcee3

jonasschnelli force-pushed on May 20, 2019

jonasschnelli commented at 9:53 am on May 20, 2019: contributor

Rebased

DrahtBot removed the label Needs rebase on May 20, 2019

sipa commented at 6:56 pm on May 20, 2019: member

Approach NACK. This is overkill; the message parsing/serializing logic can just move out of CNetMessage to avoid turning it into a virtual class.

jonasschnelli commented at 7:04 pm on May 20, 2019: contributor

@sipa: Thanks for the review. Currently turning this into the approach you mentioned. I think it’s doable though not super convinced right now due to the state necessary in parsing/serialization (hash, MAC). Until I’m finished I keep this rebased to not lose testability of other elements in the pipeline.

jonasschnelli commented at 10:01 am on June 13, 2019: contributor

#16202 seems to a superior solution. Will close this PR as soon as #14032 is rebased on top of #16202.

fanquake added this to the "In progress" column in a project

DrahtBot added the label Needs rebase on Aug 14, 2019

DrahtBot commented at 4:21 pm on August 14, 2019: member

fanquake referenced this in commit badca85e2c on Oct 28, 2019

jonasschnelli commented at 7:38 am on May 7, 2020: contributor

Superseded. Closing.

jonasschnelli closed this on May 7, 2020

DrahtBot locked this on Feb 15, 2022

dhruv added this to the "Closed unmerged" column in a project

net: Refactor message parsing (CNetMessage), adds flexibility #14046

Conflicts