p2p: Refactor network message deserialization #16202

jonasschnelli · 2019-06-13T09:59:53Z

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

DrahtBot · 2019-06-13T11:13:05Z

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.

ariard

+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)

src/net.h

src/net.cpp

src/net.h

src/net_processing.cpp

jonasschnelli · 2019-07-25T11:09:33Z

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

ariard

utACK e85d88d

src/net.h

sipa · 2019-07-29T22:56:05Z

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 · 2019-07-30T14:22:47Z

Thanks @sipa.
Added sipa's 6e4d183 to this PR

promag · 2019-08-06T01:24:59Z

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

dongcarl

ACK 7370bed
Reviewed code, ran unit tests and functional tests successfully.

dongcarl · 2019-09-05T17:24:31Z

src/net_processing.cpp

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


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.

etscrivner · 2019-09-24T15:17:30Z

ACK 7370bed

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

jkczyz · 2019-09-25T15:26:46Z

src/net.h

+    std::string m_command;
+
+    CNetMessage(const CDataStream& recv_in) : m_recv(std::move(recv_in)) {
+        m_time = 0;


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

d8c3e95

Agree with @jkczyz.

src/net.h

jkczyz · 2019-09-25T18:30:17Z

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:

StatusOr<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

ACK 7370bed, reviewed code, ran unit and functional tests both at the mentioned commit and also rebased on current master fdfaeb6, 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 · 2019-09-30T17:16:36Z

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.

marcinja · 2019-10-04T00:56:30Z

src/net_processing.cpp

-           SanitizeString(strCommand), nMessageSize,
-           HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE),
-           HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE));
+        LogPrint(BCLog::NET, "%s(%s, %u bytes): CHECKSUM ERROR\n", __func__,


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

marcinja · 2019-10-04T01:00:41Z

ACK 7370bed

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.

16d6113 Refactor message transport packaging (Jonas Schnelli) Pull request description: This PR factors out transport packaging logic from `CConnman::PushMessage()`. It's similar to #16202 (where we refactor deserialization). This allows implementing a new message transport protocol like BIP324. ACKs for top commit: dongcarl: ACK 16d6113 FWIW ariard: Code review ACK 16d6113 elichai: semiACK 16d6113 ran functional+unit tests. MarcoFalke: ACK 16d6113 🙎 Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1

16d6113 Refactor message transport packaging (Jonas Schnelli) Pull request description: This PR factors out transport packaging logic from `CConnman::PushMessage()`. It's similar to bitcoin#16202 (where we refactor deserialization). This allows implementing a new message transport protocol like BIP324. ACKs for top commit: dongcarl: ACK 16d6113 FWIW ariard: Code review ACK 16d6113 elichai: semiACK 16d6113 ran functional+unit tests. MarcoFalke: ACK 16d6113 🙎 Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1

Summary: Partial backport 1/7 of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@6294ecd Adapted to match our codebase. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8222

Summary: Partial backport 2/7 of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@1a5c656 Depends on D8222. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8223

Summary: Partial backport 3/7 of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@efecb74 Depends on D8223. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8224

Summary: Partial backport 4/7 of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@b0e10ff Depends on D8224. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8225

Summary: Partial backport 5/7 of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@6a91499 Depends on D8225. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8226

Summary: Partial backport 6/7 of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@f342a5e Depends on D8226. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8227

Summary: Completes backport (7/7) of core [[bitcoin/bitcoin#16202 | PR16202]]: bitcoin/bitcoin@ed2dc5e Depends on D8227. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8228

16d6113 Refactor message transport packaging (Jonas Schnelli) Pull request description: This PR factors out transport packaging logic from `CConnman::PushMessage()`. It's similar to bitcoin#16202 (where we refactor deserialization). This allows implementing a new message transport protocol like BIP324. ACKs for top commit: dongcarl: ACK 16d6113 FWIW ariard: Code review ACK 16d6113 elichai: semiACK 16d6113 ran functional+unit tests. MarcoFalke: ACK 16d6113 🙎 Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1

rebroad · 2021-05-08T08:00:48Z

src/net.h

+        Reset();
+    }
+
+    bool Complete() const override


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

rebroad · 2021-05-08T08:01:07Z

src/net.h

-
-    int readHeader(const char *pch, unsigned int nBytes);
-    int readData(const char *pch, unsigned int nBytes);
+    int Read(const char *pch, unsigned int nBytes) override {


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

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

jonasschnelli added the P2P label Jun 13, 2019

jonasschnelli mentioned this pull request Jun 13, 2019

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

Closed

fanquake added the Refactoring label Jun 13, 2019

jonasschnelli force-pushed the 2019/06/net_refactor_1 branch 2 times, most recently from 3fa744d to 7bd4dd2 Compare June 14, 2019 15:50

ariard reviewed Jul 23, 2019

View reviewed changes

jonasschnelli force-pushed the 2019/06/net_refactor_1 branch 4 times, most recently from 4cbe0af to e85d88d Compare July 25, 2019 11:07

ariard approved these changes Jul 25, 2019

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

jonasschnelli force-pushed the 2019/06/net_refactor_1 branch 2 times, most recently from 5f34746 to 1396bd4 Compare August 5, 2019 11:54

jonasschnelli force-pushed the 2019/06/net_refactor_1 branch from 1396bd4 to 7370bed Compare August 7, 2019 13:38

jonasschnelli mentioned this pull request Aug 7, 2019

Refactor message transport packaging #16562

Merged

dongcarl self-assigned this Aug 22, 2019

dongcarl approved these changes Sep 5, 2019

View reviewed changes

jonatack mentioned this pull request Sep 17, 2019

Future PRs bitcoin-core-review-club/website#14

Closed

jkczyz reviewed Sep 25, 2019

View reviewed changes

jonatack reviewed Sep 27, 2019

View reviewed changes

marcinja reviewed Oct 4, 2019

View reviewed changes

fanquake merged commit ed2dc5e into bitcoin:master Oct 28, 2019

ryanofsky mentioned this pull request Oct 28, 2019

Replace Connman and BanMan globals with NodeContext local #16839

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

dongcarl mentioned this pull request Apr 15, 2020

net: Reduce TransportDeserializer interface to 2 methods #18644

Closed

jkczyz mentioned this pull request May 20, 2020

Immediately disconnect on invalid net message checksum #15206

Closed

rebroad reviewed May 8, 2021

View reviewed changes

src/net.h

Reset();

}

bool Complete() const override

Copy link

Contributor

rebroad May 8, 2021

Choose a reason for hiding this comment

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

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

rebroad reviewed May 8, 2021

View reviewed changes

rebroad mentioned this pull request May 9, 2021

Enable reading of pchCommand before Complete() #21896

Closed

kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021

merge bitcoin#16202: Refactor network message deserialization

d2d220d

kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021

merge bitcoin#16202: Refactor network message deserialization

40baae9

kwvg added a commit to kwvg/dash that referenced this pull request Aug 9, 2021

merge bitcoin#16202: Refactor network message deserialization

af374df

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Aug 12, 2021

merge bitcoin#16202: Refactor network message deserialization

af67300

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Aug 22, 2021

merge bitcoin#16202: Refactor network message deserialization

aab9224

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Aug 30, 2021

merge bitcoin#16202: Refactor network message deserialization

afbafcb

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021

merge bitcoin#16202: Refactor network message deserialization

374958c

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

kwvg added a commit to kwvg/dash that referenced this pull request Nov 3, 2021

merge bitcoin#16202: Refactor network message deserialization

c0d4df0

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

merge bitcoin#16202: Refactor network message deserialization

51968c6

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 16, 2022

merge bitcoin#16202: Refactor network message deserialization

87e49b7

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

jnewbery mentioned this pull request May 27, 2022

net/net_processing: Add thread safety related annotations for CNode and Peer #25174

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

p2p: Refactor network message deserialization #16202

p2p: Refactor network message deserialization #16202

Uh oh!

Conversation

jonasschnelli commented Jun 13, 2019 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonasschnelli commented Jul 25, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Jul 29, 2019

Uh oh!

jonasschnelli commented Jul 30, 2019

Uh oh!

promag commented Aug 6, 2019

Uh oh!

dongcarl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongcarl Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etscrivner commented Sep 24, 2019

Uh oh!

jkczyz Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

promag Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz commented Sep 25, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Sep 30, 2019

Uh oh!

marcinja Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

marcinja commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebroad May 8, 2021

Choose a reason for hiding this comment

Uh oh!

rebroad May 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonasschnelli commented Jun 13, 2019 •

edited by fanquake

Loading

DrahtBot commented Jun 13, 2019 •

edited

Loading

dongcarl left a comment •

edited

Loading

dongcarl Sep 5, 2019 •

edited

Loading

marcinja commented Oct 4, 2019 •

edited

Loading