p2p: Refactor network message deserialization

jonasschnelli commented at 9:59 am on June 13, 2019: contributor

This refactors the network message deserialization.

It transforms the CNetMessage into a transport protocol agnostic message container.
A new class TransportDeserializer (unique pointer of CNode) is introduced, handling the network buffer reading and the decomposing to a CNetMessage
No behavioral changes (in terms of disconnecting, punishing)
Moves the checksum finalizing into the SocketHandler thread (finalizing was in ProcessMessages before)

The optional last commit makes the TransportDeserializer following an adapter pattern (polymorphic interface) to make it easier to later add a V2 transport protocol deserializer.

Intentionally not touching the sending part.

Pre-Requirement for BIP324 (v2 message transport protocol). Replacement for #14046 and inspired by a comment from sipa

jonasschnelli added the label P2P on Jun 13, 2019

fanquake added the label Refactoring on Jun 13, 2019

DrahtBot commented at 11:13 am on June 13, 2019: 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:

#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16562 (Refactor message transport packaging 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.

jonasschnelli force-pushed on Jun 13, 2019

jonasschnelli force-pushed on Jun 14, 2019

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

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

in src/net.h:668 in 7bd4dd2160 outdated

664+        return (in_data && hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH);
665+    }
666+    int Read(const char *pch, unsigned int nBytes) {
667+        return in_data ? readData(pch, nBytes) : readHeader(pch, nBytes);
668+    }
669     int readHeader(const char *pch, unsigned int nBytes);

ariard commented at 2:15 pm on July 23, 2019:

Shouldn’t these both methods be part of the private interface given they are version-message specific ?

ariard commented at 3:50 pm on July 23, 2019:

And if yes, I think checks in ProcessMessages won’t have anymore a reason to exist?

jonasschnelli commented at 6:40 am on July 24, 2019:

I agree with that they should be private methods (will fix). But I don’t think we should make behavioral changes in this PR that’s why I tried to not move the checks. PRs that “optimize” those checks are #15206 and #15197.

in src/net.h:642 in 7bd4dd2160 outdated

604@@ -605,6 +605,18 @@ class CNetMessage {
605  * transport protocol agnostic CNetMessage (command & payload)
606  */
607 class TransportDeserializer {
608+public:
609+    virtual void Reset() = 0;

ariard commented at 2:16 pm on July 23, 2019:

nit: if it’s a public interface maybe add a short line of comment on what is expected for every method

in src/net.h:657 in 7bd4dd2160 outdated

614+    virtual CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) = 0;
615+    virtual ~TransportDeserializer() {}
616+};
617+
618+class V1TransportDeserializer : public TransportDeserializer
619+{

ariard commented at 2:31 pm on July 23, 2019:

IMO, you may also move more members in the private interface, what’s the reason to have in_data as public ?

in src/net.cpp:595 in 7bd4dd2160 outdated

582             CNetMessage msg = m_deserializer->GetMessage(Params().MessageStart(), nTimeMicros);
583 
584             //store received bytes per message command
585             //to prevent a memory DOS, only allow valid commands
586-            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(m_deserializer->hdr.pchCommand);
587+            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command);

ariard commented at 2:52 pm on July 23, 2019:

nit : wonder if this change shouldn’t be part of first commit

in src/net.cpp:699 in 2cd45b2f4e outdated

692+    // decompose a single CNetMessage from the TransportDeserializer
693+    CNetMessage msg(vRecv);
694+
695+    // store state about valid header, netmagic and checksum
696+    msg.m_valid_header = hdr.IsValid(message_start);
697+    msg.m_valid_netmagic = (memcmp(hdr.pchMessageStart, message_start, CMessageHeader::MESSAGE_START_SIZE) == 0);

ariard commented at 3:23 pm on July 23, 2019:

Isn’t this check redundant with first check in IsValid ? Furthermore isn’t netmagic validity a subset of header one, and so you may remove it completely and only rely on m_valid_header ?

jonasschnelli commented at 6:48 am on July 24, 2019:

Yes. It’s redundant for the same reason as said above. Further optimizations would change the behavior of v1 decomposing.

ProcessMessages() happens after de-queuing deserialized messages. If we would check during deserialization (and disconnect), we would change the behavior.

Also, ProcessMessages() differentiates between invalid netmagic and invalid header. It tolerates invalid headers (while not invalid netmagics), thus we need a way to transport those check-states without breaking the current behavior.

Since this PR aims to do a refactoring, I don’t think behavioral changes are ideal here.

ariard commented at 3:37 pm on July 24, 2019:

Ah yes exact, you may have a valid netmagic and an invalid header and get accepted.

Okay, I agree if these points are addressed later in behavioral changes specific PRs that’s better than everything at once.

in src/net.h:620 in 2cd45b2f4e outdated

577+ */
578 class CNetMessage {
579+public:
580+    CDataStream m_recv;              // received message data
581+    int64_t m_time;                  // time (in microseconds) of message receipt.
582+    bool m_valid_netmagic;

ariard commented at 3:33 pm on July 23, 2019:

So next step would be to move these transport-protocol-specific fields in the Transport Deserializer ? If a CNetMessage doesn’t have a valid_header it shouldn’t succeed deserialization right ?

jonasschnelli commented at 6:49 am on July 24, 2019:

Next step would be to further avoid transport-specific stuff in ProcessMessages(). I think #15206 and #15197 are a good next step (would need rebase after this).

ariard commented at 3:32 pm on July 24, 2019:

Ok thanks, going to check them once they are rebased

in src/net_processing.cpp:3297 in 2cd45b2f4e outdated

3289+        LogPrint(BCLog::NET, "PROCESSMESSAGE: INVALID MESSAGESTART %s peer=%d\n", SanitizeString(msg.m_command), pfrom->GetId());
3290         pfrom->fDisconnect = true;
3291         return false;
3292     }
3293 
3294     // Read header

ariard commented at 3:49 pm on July 23, 2019:

nit: “check header”

ariard commented at 3:55 pm on July 23, 2019: member

+1 for the adapter pattern, there may still room for few interfaces improvements.

Current draft of BIP 324 : https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52 (a help for review and not yet in bips repo I think)

jonasschnelli force-pushed on Jul 24, 2019

jonasschnelli force-pushed on Jul 25, 2019

jonasschnelli commented at 11:09 am on July 25, 2019: contributor

Thanks @ariard for the review. Fixed those points/nits. Mainly the private declaration of members and methods of the V1TransportDeserializer class.

in src/net.h:611 in e85d88de78 outdated

607+ */
608+class TransportDeserializer {
609+public:
610+    // prepare for next message
611+    virtual void Reset() = 0;
612+    // retruns true if the current deserialization is complete

ariard commented at 2:28 pm on July 25, 2019:

nit: s/retruns/returns/

jonasschnelli commented at 11:54 am on August 5, 2019:

fixed.

ariard approved

ariard commented at 2:29 pm on July 25, 2019: member

utACK e85d88de7

sipa commented at 10:56 pm on July 29, 2019: member

Concept and approach ACK.

I’ll go through the code in more detail soon, but one thing I noticed is that this introduces an unnecessary copy of the message payload in the GetMessage() function. It can be avoided by moving the CDataStream: https://github.com/sipa/bitcoin/tree/pr16202

jonasschnelli commented at 2:22 pm on July 30, 2019: contributor

Thanks @sipa. Added sipa’s 6e4d18346195310df96f42dcce4e29eec2fcc2eb to this PR

jonasschnelli force-pushed on Aug 5, 2019

promag commented at 1:24 am on August 6, 2019: member

Concept ACK, following #14046 (comment) does makes sense vs #14046.

jonasschnelli force-pushed on Aug 7, 2019

dongcarl assigned dongcarl on Aug 22, 2019

in src/net_processing.cpp:3263 in b830a602b8 outdated

3279@@ -3280,7 +3280,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
3280             return false;
3281         // Just take one message
3282         msgs.splice(msgs.begin(), pfrom->vProcessMsg, pfrom->vProcessMsg.begin());
3283-        pfrom->nProcessQueueSize -= msgs.front().m_recv.size() + CMessageHeader::HEADER_SIZE;
3284+        pfrom->nProcessQueueSize -= msgs.front().m_raw_message_size;

dongcarl commented at 5:24 pm on September 5, 2019:

Had to do a double-take here… But this shouldn’t change behaviour as msgs.front().m_recv.size() is always equal to msgs.front().m_message_size because the msgs.front().m_recv hasn’t advanced its nReadPos yet. Verified by placing an assert and running the unit and functional tests.

dongcarl approved

dongcarl commented at 5:25 pm on September 5, 2019: member

ACK https://github.com/bitcoin/bitcoin/commit/7370bed970e9455dc686c8088dd3e3afb219bbee Reviewed code, ran unit tests and functional tests successfully.

etscrivner commented at 3:17 pm on September 24, 2019: contributor

ACK 7370bed970e9455dc686c8088dd3e3afb219bbee

Approach is very clean. All unit and functional tests pass. Manually tested some P2P functionality to provide additional confidence.

in src/net.h:628 in 6b18269ca7 outdated

584+    bool m_valid_checksum;
585+    uint32_t m_message_size;         // size of the payload
586+    std::string m_command;
587+
588+    CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {
589+        m_time = 0;

jkczyz commented at 3:26 pm on September 25, 2019:

Nit: Use default member initializers for these rather than repeating the members in the constructor.

promag commented at 11:14 pm on October 15, 2019:

d8c3e95a443cb581825e0d4456f0b7886960e489

Agree with @jkczyz.

in src/net.h:677 in 6b18269ca7 outdated

620+    TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
621+        Reset();
622+    }
623 
624-    CNetMessage(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
625+    void Reset() {

jkczyz commented at 3:42 pm on September 25, 2019:

Rather than implementing a custom Reset function, would it be simpler to use unique_ptr::reset with a new deserializer in CNode whenever a new message is parsed? That is, an instance of TransportDeserializer would be responsible for deserializing a single message. Then there is no risk of forgetting to update the Reset method as the code changes, which could leave the deserializer in a partially reset state.

jonasschnelli commented at 6:51 am on October 18, 2019:

For the current use case, I find a explicit manual reset better understand- and readable.

jkczyz commented at 6:30 pm on September 25, 2019: contributor

Not sure if more refactoring would be first required, but it would be nice if TransportDeserializer’s interface could be simplified to a single method. Roughly speaking:

0StatusOr<CNetMessage> Deserialize(CDataStream& stream);

Where StatusOr is essentially a variant type of error or result, though I suppose exceptions could be used instead. An output parameter for the result is another possibility. I’m not sure what the project conventions are for the error or result pattern.

Then TransportDeserializer could be stateless. Instead all state would be local to the Deserialize method. Additionally, the return value (or exception) could be examined for error status. This could reduce the complexity of the code quite a bit.

Otherwise, the client is responsible for knowing in which order the methods of TransportDeserializer should be called in, whether calling a method is valid at a given point, and in general the inner-workings of the TransportDeserializer implementation.

jonatack commented at 10:03 am on September 27, 2019: member

ACK 7370bed970e9455dc686c8088dd3e3afb219bbee, reviewed code, ran unit and functional tests both at the mentioned commit and also rebased on current master fdfaeb67de3694b436656f57ff65f1dc94b893f0, with additional asserts and custom logging as a sanity check. I am currently observing a running node with these changes rebased on master with custom logging and asserts.

I do agree that continued refactoring to simplify handling of state might potentially be worthwhile. Given this PR is the result of at least one previous refactoring attempt and has been in review since June (and depending on the author’s motivation to keep reworking it), it might be more flexible to try it in a follow-up PR if merging this one as-is can further v2 p2p progress.

fjahr commented at 5:16 pm on September 30, 2019: member

ACK 7370bed

Successfully ran unit and functional tests, reviewed code. I agree with @jkczyz as in I would prefer a stateless deserializer as well but also with @jonatack that this would be better as a follow-up PR.

in src/net_processing.cpp:3292 in 7370bed970 outdated

3324     {
3325-        LogPrint(BCLog::NET, "%s(%s, %u bytes): CHECKSUM ERROR expected %s was %s\n", __func__,
3326-           SanitizeString(strCommand), nMessageSize,
3327-           HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE),
3328-           HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE));
3329+        LogPrint(BCLog::NET, "%s(%s, %u bytes): CHECKSUM ERROR\n", __func__,

marcinja commented at 0:56 am on October 4, 2019:

It might be useful to print pfrom->GetId() in here also.

marcinja commented at 1:00 am on October 4, 2019: contributor

ACK 7370bed970e9455dc686c8088dd3e3afb219bbee

Reviewed code and ran tests at each commit. I also agree with other reviewers that a stateless deserializer would be nice for a potential follow-up.

practicalswift commented at 4:51 am on October 4, 2019: contributor

@jonasschnelli Have you considered adding a fuzz harness to test the robustness? What would be a good harness entry point?

DrahtBot added the label Needs rebase on Oct 9, 2019

jonasschnelli force-pushed on Oct 10, 2019

jonasschnelli commented at 7:35 pm on October 10, 2019: contributor

Rebased

DrahtBot removed the label Needs rebase on Oct 10, 2019

practicalswift commented at 10:08 pm on October 13, 2019: contributor

@jonasschnelli Friendly ping regarding my fuzzing question :)

jonasschnelli commented at 12:33 pm on October 15, 2019: contributor

@practicalswift: this PR doesn’t change the behaviour and the way it reads into the buffers is identical. More fuzz tests for the network layer are upcoming (already in my p2p-v2 branch).

in src/net.cpp:600 in d8c3e95a44 outdated

610+            CNetMessage msg = m_deserializer->GetMessage(Params().MessageStart(), nTimeMicros);
611+
612             //store received bytes per message command
613             //to prevent a memory DOS, only allow valid commands
614-            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.hdr.pchCommand);
615+            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(m_deserializer->hdr.pchCommand);

promag commented at 11:08 pm on October 15, 2019:

d8c3e95a443cb581825e0d4456f0b7886960e489

Just use msg:

0mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command);

in src/net.cpp:604 in d8c3e95a44 outdated

615+            mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(m_deserializer->hdr.pchCommand);
616             if (i == mapRecvBytesPerMsgCmd.end())
617                 i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
618             assert(i != mapRecvBytesPerMsgCmd.end());
619-            i->second += msg.hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
620+            i->second += m_deserializer->hdr.nMessageSize + CMessageHeader::HEADER_SIZE;

promag commented at 11:09 pm on October 15, 2019:

d8c3e95a443cb581825e0d4456f0b7886960e489

0i->second += msg.m_message_size + CMessageHeader::HEADER_SIZE;

in src/net.cpp:715 in d8c3e95a44 outdated

711+                 HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE),
712+                 HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE));
713+    }
714+
715+    // store command string, payload size
716+    msg.m_command = hdr.GetCommand();

promag commented at 11:11 pm on October 15, 2019:

d8c3e95a443cb581825e0d4456f0b7886960e489

nit, could move these up before L707 and avoid the duplicate GetCommand() in L709.

in src/net_processing.cpp:3283 in d8c3e95a44 outdated

3287-        LogPrint(BCLog::NET, "PROCESSMESSAGE: ERRORS IN HEADER %s peer=%d\n", SanitizeString(hdr.GetCommand()), pfrom->GetId());
3288+        LogPrint(BCLog::NET, "PROCESSMESSAGE: ERRORS IN HEADER %s peer=%d\n", SanitizeString(msg.m_command), pfrom->GetId());
3289         return fMoreWork;
3290     }
3291-    std::string strCommand = hdr.GetCommand();
3292+    std::string strCommand = msg.m_command;

promag commented at 11:20 pm on October 15, 2019:

d8c3e95a443cb581825e0d4456f0b7886960e489

Make this const std::string&? Changed locally and looks good.

in src/net.h:655 in b8e9633d12 outdated

650+    // returns true if the current deserialization is complete
651+    virtual bool Complete() const = 0;
652+    // checks if the potential message in deserialization is oversized
653+    virtual bool OversizedMessageDetected() const = 0;
654+    // set the serialization context version
655+    virtual void SetVersion(int nVersionIn) = 0;

promag commented at 11:23 pm on October 15, 2019:

b8e9633d127fd1e740b3c9e25a6ed5d480a77f25

nit, use correct style in new code? int version.

in src/net.h:657 in b8e9633d12 outdated

652+    // checks if the potential message in deserialization is oversized
653+    virtual bool OversizedMessageDetected() const = 0;
654+    // set the serialization context version
655+    virtual void SetVersion(int nVersionIn) = 0;
656+    // read and deserialize data
657+    virtual int Read(const char *pch, unsigned int nBytes) = 0;

promag commented at 11:24 pm on October 15, 2019:

b8e9633d127fd1e740b3c9e25a6ed5d480a77f25

nit, virtual int Read(const char* data, unsigned int bytes) = 0

promag commented at 11:27 pm on October 15, 2019: member

Refactor LGTM, checked out each commit + tests, played around with further changes and I’m leaving some comments below for you consideration.

Tested ACK b8e9633d127fd1e740b3c9e25a6ed5d480a77f25.

Refactor: split network transport deserializing from message container 6294ecdb8b

Remove transport protocol knowhow from CNetMessage / net processing 1a5c656c31

Use adapter pattern for the network deserializer efecb74677

jonasschnelli commented at 6:59 am on October 18, 2019: contributor

Fixed all remaining nits and issues.

jonasschnelli force-pushed on Oct 18, 2019

practicalswift commented at 7:25 am on October 18, 2019: contributor

@jonasschnelli Sounds very good! Are the fuzzers available to look at? I couldn’t find the p2p-v2 branch :)

jonasschnelli commented at 7:42 am on October 18, 2019: contributor

@practicalswift The branch is not yet pushed. Sorry.

However. Fuzzing is not a planed part of this PR.

in src/net.h:626 in 6294ecdb8b outdated

622+    bool m_valid_header = false;
623+    bool m_valid_checksum = false;
624+    uint32_t m_message_size = 0;         // size of the payload
625+    std::string m_command;
626+
627+    CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {}

ryanofsky commented at 6:04 pm on October 18, 2019:

In commit “Refactor: split network transport deserializing from message container” (6294ecdb8bb4eb7049a18c721ee8cb4a53d80a06)

std::move here is misleading, and will result in a copy because recv_in is const. Would suggest dropping std::move or changing the argument type to non-const CDataStream recv_in (if you want to give the caller the option of moving or copying) or CDataStream&& recv_in (if you want to force the caller to move).

in src/net.cpp:713 in 6294ecdb8b outdated

709+    msg.m_command = hdr.GetCommand();
710+    msg.m_message_size = hdr.nMessageSize;
711+
712+    msg.m_valid_checksum = (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0);
713+    if (!msg.m_valid_checksum) {
714+        LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s\n",

ryanofsky commented at 6:23 pm on October 18, 2019:

In commit “Refactor: split network transport deserializing from message container” (6294ecdb8bb4eb7049a18c721ee8cb4a53d80a06)

It seems strange that this commit now logs two slightly different CHECKSUM ERROR line from different code locations instead of definitively logging in one place like the previous code.

It is also odd that the checksum error log print was copied here, but the invalid magic and invalid header errors were not copied.

Would suggest logging errors either here or ProcessMessages, but not both places. Or if there is some reason to log both places, at least being consistent about which errors are logged and which errors are ignored.

in src/net.h:628 in 6294ecdb8b outdated

624+    uint32_t m_message_size = 0;         // size of the payload
625+    std::string m_command;
626+
627+    CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {}
628+
629+    void SetVersion(int nVersionIn)

ryanofsky commented at 7:13 pm on October 18, 2019:

In commit “Refactor: split network transport deserializing from message container” (6294ecdb8bb4eb7049a18c721ee8cb4a53d80a06)

Why do CNetMessage and TransportDeserializer both have SetVersion methods now? It seems like only TransportDeserializer should have a SetVersion method, and it should be responsible for passing it the version on to CNetMessage objects as it constructs them.

Rght now both objects have SetVersion methods and the TransportDeserializer one is never called. I’d would suggest shifting more logic to TransportDeserializer, or at least deleting the SetVersion implementation that is never called.

in src/net.h:656 in 6294ecdb8b outdated

653+    TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
654+        Reset();
655+    }
656 
657-    CNetMessage(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
658+    void Reset() {

ryanofsky commented at 7:25 pm on October 18, 2019:

In commit “Refactor: split network transport deserializing from message container” (6294ecdb8bb4eb7049a18c721ee8cb4a53d80a06)

Reset method seems to reset every member except hdr, leaving it untouched. I guess this is because CMessageHeader doesn’t have a clear method or easily callable constructor, but it would be good to have a comment saying hdr is intentionally skipped, because this looks like a bug, and could potentially cause bugs in the future.

in src/net.h:687 in efecb74677 outdated

683@@ -664,25 +684,23 @@ class TransportDeserializer {
684         data_hash.SetNull();
685         hasher.Reset();
686     }
687-
688-    bool complete() const
689+    bool Complete() const

ryanofsky commented at 7:42 pm on October 18, 2019:

In commit “Use adapter pattern for the network deserializer” (efecb74677222f6c70adf7f860c315f430d39ec4)

Would be good to add override keyword to Complete, Read, and other overridden methods for clarity and compile errors if there are changes to superclass and subclass methods no longer match up.

ryanofsky approved

ryanofsky commented at 7:49 pm on October 18, 2019: member

Code review ACK efecb74677222f6c70adf7f860c315f430d39ec4. There are few odd things I made suggestions about which could be cleaned up later. Overall this seems like a clean simple approach that doesn’t change existing code very much.

sipa commented at 9:34 pm on October 18, 2019: member

ACK efecb74677222f6c70adf7f860c315f430d39ec4

I have a few suggested improvements at https://github.com/sipa/bitcoin/commits/jonasschnelli_net_refactor_1: getting rid of the special message for oversized messages (strange to deal with that specifically if even larger messages are ignored, and the interface is cleaner without it), simplifying the interface a bit, and adding final/override.

jonatack commented at 10:24 am on October 22, 2019: member

ACK efecb74677222f6c70adf7f860c315f430d39ec4. Code review, built, ran tests, ran node with net logging. The proposed improvements by @ryanofsky and @sipa at https://github.com/sipa/bitcoin/commits/jonasschnelli_net_refactor_1 look worthwhile for a follow-up.

Force CNetMessage::m_recv to use std::move b0e10ff4df

Remove oversized message detection from log and interface 6a91499496

Make resetting implicit in TransportDeserializer::Read() f342a5e61a

Add override/final modifiers to V1TransportDeserializer ed2dc5e48a

jonasschnelli commented at 7:31 am on October 23, 2019: contributor

Added one extra commit that fixes wrong std::move behaviour for the CNetMessages:m_recv (CDataStream).

Also added @sipa s three commits.

in test/functional/p2p_invalid_messages.py:104 in 6a91499496 outdated

100@@ -101,11 +101,10 @@ def run_test(self):
101             msg_over_size = msg_unrecognized(str_data="b" * (valid_data_limit + 1))
102             assert len(msg_over_size.serialize()) == (msg_limit + 1)
103 
104-            with node.assert_debug_log(["Oversized message from peer=4, disconnecting"]):

ryanofsky commented at 3:32 pm on October 23, 2019:

In commit “Remove oversized message detection from log and interface” (6a91499496d76c2b3e84489e9723b60514fb08db)

Is there a different log message this could check for? It would seem better if the test could continue checking the reason for disconnect.

in src/net.h:691 in ed2dc5e48a

688         if (!in_data)
689             return false;
690         return (hdr.nMessageSize == nDataPos);
691     }
692-    void SetVersion(int nVersionIn)
693+    void SetVersion(int nVersionIn) override

ryanofsky commented at 3:43 pm on October 23, 2019:

In commit “Add override/final modifiers to V1TransportDeserializer” (ed2dc5e48abed1cde6ab98025dc8212917d47d21)

This function is still never called. Isn’t it pointless for CNetMessage and TransportDeserializer to both have SetVersion methods, instead of just setting the version in a single place? See previous #16202 (review)

ryanofsky approved

ryanofsky commented at 3:46 pm on October 23, 2019: member

Code review ACK ed2dc5e48abed1cde6ab98025dc8212917d47d21. 4 cleanup commits added since last review. Unaddressed comments:

#16202 (review) #16202 (review) #16202 (review)

in src/net.cpp:707 in 1a5c656c31 outdated

706@@ -707,6 +707,7 @@ CNetMessage TransportDeserializer::GetMessage(const CMessageHeader::MessageStart
707     // store command string, payload size

ariard commented at 9:06 pm on October 23, 2019:

nit: “store … wire message size” has been dropped

in src/net_processing.cpp:3286 in 6294ecdb8b outdated

3291-    std::string strCommand = hdr.GetCommand();
3292+    const std::string& strCommand = msg.m_command;
3293 
3294     // Message size
3295-    unsigned int nMessageSize = hdr.nMessageSize;
3296+    unsigned int nMessageSize = msg.m_message_size;

ariard commented at 9:37 pm on October 23, 2019:

Why define nMessageSize and strCommand here, can’t we use CNetMessage members directly ? We don’t need to check anything on them

ariard approved

ariard commented at 9:41 pm on October 23, 2019: member

Code review and tested ACK ed2dc5e.

promag commented at 4:16 pm on October 27, 2019: member

Code review ACK ed2dc5e48abed1cde6ab98025dc8212917d47d21.

marcinja commented at 1:51 am on October 28, 2019: contributor

Code review ACK ed2dc5e48abed1cde6ab98025dc8212917d47d21

fanquake renamed this:
~~Refactor network message deserialization~~
p2p: Refactor network message deserialization
on Oct 28, 2019

fanquake referenced this in commit badca85e2c on Oct 28, 2019

fanquake merged this on Oct 28, 2019

fanquake closed this on Oct 28, 2019

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

fanquake moved this from the "In progress" to the "Done" column in a project

MarcoFalke referenced this in commit eca4d8ef6a on Feb 28, 2020

sidhujag referenced this in commit b6ff52dc4d on Feb 29, 2020

jasonbcox referenced this in commit 360f3ded8b on Nov 3, 2020

jasonbcox referenced this in commit e692ba5cc5 on Nov 3, 2020

deadalnix referenced this in commit a7040a762f on Nov 3, 2020

deadalnix referenced this in commit af5f2e8b2d on Nov 3, 2020

deadalnix referenced this in commit 06ae90a108 on Nov 3, 2020

jasonbcox referenced this in commit b6107fb39d on Nov 3, 2020

jasonbcox referenced this in commit 843cb65087 on Nov 3, 2020

sidhujag referenced this in commit 78a555639f on Nov 10, 2020

in src/net.h:685 in ed2dc5e48a

689+
690+    V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
691+        Reset();
692+    }
693+
694+    bool Complete() const override

rebroad commented at 8:00 am on May 8, 2021:

why is this function in the .h instead of the .cpp?

in src/net.h:696 in ed2dc5e48a

707         vRecv.SetVersion(nVersionIn);
708     }
709-
710-    int readHeader(const char *pch, unsigned int nBytes);
711-    int readData(const char *pch, unsigned int nBytes);
712+    int Read(const char *pch, unsigned int nBytes) override {

rebroad commented at 8:01 am on May 8, 2021:

why is this function in the .h instead of in the .cpp?

kittywhiskers referenced this in commit d2d220d6c3 on Aug 5, 2021

kittywhiskers referenced this in commit 40baae9dc5 on Aug 5, 2021

kittywhiskers referenced this in commit af374df9b8 on Aug 9, 2021

kittywhiskers referenced this in commit af67300377 on Aug 12, 2021

kittywhiskers referenced this in commit aab92247ed on Aug 22, 2021

kittywhiskers referenced this in commit afbafcb9cb on Aug 30, 2021

dhruv added this to the "Done" column in a project

kittywhiskers referenced this in commit 374958c513 on Nov 1, 2021

kittywhiskers referenced this in commit c0d4df0a06 on Nov 3, 2021

pravblockc referenced this in commit 51968c66cc on Nov 18, 2021

gades referenced this in commit 87e49b7bba on May 16, 2022

DrahtBot locked this on Aug 16, 2022

p2p: Refactor network message deserialization #16202

Conflicts