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

jonasschnelli · 2018-08-24T09:20:34Z

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.

src/net_message.h

promag · 2018-08-24T10:01:54Z

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

jonasschnelli · 2018-08-24T11:14:22Z

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

DrahtBot · 2018-08-24T12:16:01Z

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.

maflcko · 2018-08-24T13:27:36Z

utACK 59a7ce0f9de950b64c5e89dd29ebcd0b607b982e

gmaxwell · 2018-08-24T16:40:06Z

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 · 2018-08-24T17:15:38Z

@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.

src/net.cpp

src/net_processing.cpp

src/net_message.h

jonasschnelli · 2018-08-27T11:19:43Z

@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.

promag

Fix commit message "Use shared pointers for CNetMessage handling".

src/net.h

jonasschnelli · 2018-08-31T19:51:37Z

Fixed commit message issue reported by @promag

src/net.h

src/net_message.h

src/net_message.cpp

promag · 2018-10-19T07:14:22Z

utACK c2f9c04.

sipa · 2018-10-19T23:43:57Z

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.

maflcko · 2018-11-07T20:07:42Z

@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.

jonasschnelli · 2019-05-20T09:53:52Z

Rebased

sipa · 2019-05-20T18:56:57Z

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 · 2019-05-20T19:04:54Z

@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 · 2019-06-13T10:01:23Z

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

DrahtBot · 2019-08-14T16:21:18Z

Needs rebase

ed2dc5e Add override/final modifiers to V1TransportDeserializer (Pieter Wuille) f342a5e Make resetting implicit in TransportDeserializer::Read() (Pieter Wuille) 6a91499 Remove oversized message detection from log and interface (Pieter Wuille) b0e10ff Force CNetMessage::m_recv to use std::move (Jonas Schnelli) efecb74 Use adapter pattern for the network deserializer (Jonas Schnelli) 1a5c656 Remove transport protocol knowhow from CNetMessage / net processing (Jonas Schnelli) 6294ecd Refactor: split network transport deserializing from message container (Jonas Schnelli) Pull request description: **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](#14046 (comment)) from sipa ACKs for top commit: promag: Code review ACK ed2dc5e. marcinja: Code review ACK ed2dc5e ryanofsky: Code review ACK ed2dc5e. 4 cleanup commits added since last review. Unaddressed comments: ariard: Code review and tested ACK ed2dc5e. Tree-SHA512: bab8d87464e2e8742529e488ddcdc8650f0c2025c9130913df00a0b17ecdb9a525061cbbbd0de0251b76bf75a8edb72e3ad0dbf5b79e26f2ad05d61b4e4ded6d

jonasschnelli · 2020-05-07T07:38:25Z

Superseded. Closing.

jonasschnelli added the P2P label Aug 24, 2018

jonasschnelli mentioned this pull request Aug 24, 2018

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

Closed

1 task

scravy reviewed Aug 24, 2018

View reviewed changes

src/net_message.h Show resolved Hide resolved

maflcko changed the title ~~Refactor message parsing (CNetMessage), adds flexibility~~ net: Refactor message parsing (CNetMessage), adds flexibility Aug 24, 2018

maflcko reviewed Aug 24, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

l2a5b1 reviewed Aug 26, 2018

View reviewed changes

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch from 59a7ce0 to f63893d Compare August 27, 2018 11:01

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch 2 times, most recently from e43f3b7 to 837beb4 Compare August 27, 2018 11:33

promag reviewed Aug 29, 2018

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Aug 29, 2018

--- REFACTOR (bitcoin#14046) ---

bd9e18e

DrahtBot mentioned this pull request Aug 29, 2018

Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. #13766

Closed

laanwj added this to Blockers in High-priority for review Aug 30, 2018

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch from 837beb4 to 689e20d Compare August 31, 2018 19:51

Empact reviewed Sep 1, 2018

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

Empact reviewed Sep 1, 2018

View reviewed changes

src/net_message.h Show resolved Hide resolved

practicalswift reviewed Sep 10, 2018

View reviewed changes

src/net_message.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Sep 10, 2018

View reviewed changes

src/net_message.cpp Show resolved Hide resolved

This was referenced Sep 12, 2018

net: Refactor ThreadSocketHandler #14147

Closed

wip: net: Implement poll #14221

Closed

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch 2 times, most recently from 7c1f556 to c2f9c04 Compare September 18, 2018 07:52

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch from 562a51a to daa4562 Compare October 17, 2018 14:34

DrahtBot mentioned this pull request Oct 20, 2018

Constexpr Everything Part 1: Constants #14487

Closed

maflcko removed this from Blockers in High-priority for review Nov 7, 2018

DrahtBot added the Needs rebase label Nov 28, 2018

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch from daa4562 to 48d789f Compare March 11, 2019 15:52

DrahtBot removed the Needs rebase label Mar 11, 2019

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch from 48d789f to a4803d3 Compare March 11, 2019 16:35

DrahtBot added the Needs rebase label Apr 4, 2019

jonasschnelli added 4 commits May 20, 2019 11:37

Use unique pointers for CNetMessage handling

d67c076

Split off format specific details from CNetMessage

6efdd02

Move NetMessage (NetMessageBase) to net_message.h/cpp

76dacb6

Factor out RecordRecvBytesPerMsgCmd()

c5816dc

jonasschnelli force-pushed the 2018/08/bip151_pre_refactor branch from a4803d3 to c5816dc Compare May 20, 2019 09:52

DrahtBot removed the Needs rebase label May 20, 2019

jonasschnelli mentioned this pull request Jun 13, 2019

p2p: Refactor network message deserialization #16202

Merged

fanquake added this to In progress in P2P refactor Jun 17, 2019

DrahtBot added the Needs rebase label Aug 14, 2019

jonasschnelli closed this May 7, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

dhruv added this to Closed unmerged in P2P encryption Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonasschnelli commented Aug 24, 2018

promag commented Aug 24, 2018

jonasschnelli commented Aug 24, 2018

DrahtBot commented Aug 24, 2018 •

edited

maflcko commented Aug 24, 2018

gmaxwell commented Aug 24, 2018

sipa commented Aug 24, 2018 •

edited

jonasschnelli commented Aug 27, 2018

promag left a comment

jonasschnelli commented Aug 31, 2018

promag commented Oct 19, 2018

sipa commented Oct 19, 2018 •

edited

maflcko commented Nov 7, 2018

jonasschnelli commented May 20, 2019

sipa commented May 20, 2019

jonasschnelli commented May 20, 2019

jonasschnelli commented Jun 13, 2019

DrahtBot commented Aug 14, 2019

jonasschnelli commented May 7, 2020

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

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

Conversation

jonasschnelli commented Aug 24, 2018

promag commented Aug 24, 2018

jonasschnelli commented Aug 24, 2018

DrahtBot commented Aug 24, 2018 • edited

Conflicts

maflcko commented Aug 24, 2018

gmaxwell commented Aug 24, 2018

sipa commented Aug 24, 2018 • edited

jonasschnelli commented Aug 27, 2018

promag left a comment

Choose a reason for hiding this comment

jonasschnelli commented Aug 31, 2018

promag commented Oct 19, 2018

sipa commented Oct 19, 2018 • edited

maflcko commented Nov 7, 2018

jonasschnelli commented May 20, 2019

sipa commented May 20, 2019

jonasschnelli commented May 20, 2019

jonasschnelli commented Jun 13, 2019

DrahtBot commented Aug 14, 2019

jonasschnelli commented May 7, 2020

DrahtBot commented Aug 24, 2018 •

edited

sipa commented Aug 24, 2018 •

edited

sipa commented Oct 19, 2018 •

edited