Refactor message transport packaging #16562

jonasschnelli · 2019-08-07T14:42:32Z

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.

DrahtBot · 2019-08-07T18:29:15Z

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

Conflicts

No conflicts as of last run.

promag

This is more a concept comment than code review. Looks like this approach causes reallocations? If the goal is to factor out from CConnman::PushMessage have you considered something like class MessageWriterV1 : public MessageWriter? The message writer V1 would still push 2 buffers to vSendMsg (vSendMsg should be accessible to the message writer). In other words, CConnman wouldn't be responsible to push to vSendMsg).

src/net.h

jonasschnelli · 2019-08-08T07:09:59Z

@promag: yes. You'r right. The insert/reallocation was stupid. Slighly changed the approach. It is now also avoiding the behavior change.

Also removed the unused return value (will eventually come back with the v2 implementation).

ariard

ACK b840e42, code reviewed, tests ok. Feel free to add comments if you rebase.

src/net.h

src/net.cpp

dongcarl · 2020-01-22T20:01:10Z

Rebased this PR for you as well: master...dongcarl:2020-01-net_refactor_2-rebased

jonasschnelli · 2020-01-30T10:32:31Z

Thanks @dongcarl. Pushed your rebased version.

elichai

semiACK 16d6113 ran functional+unit tests.

While reading the code I found one observable difference which is the replacement of HEADER_SIZE with serializedHeader.size(). I'm not exactly sure which should be used here(they should be the same but maybe there's some DOS vector i'm missing)
other than that the pointer is allocated in the constructor so there shouldn't be a way to call it when null.

dongcarl · 2020-02-04T18:18:30Z

ACK 16d6113 FWIW

Possible followup:

Add properly formatted Doxygen comments (something like what's already in the abstract class) for, and/or rename TransportSerializer::prepareForTransport as the name can be misinterpreted as: "do everything that we need to do before we send it to transport layer."

ariard · 2020-02-25T01:34:43Z

Code review ACK 16d6113

I'm not exactly sure which should be used here(they should be the same but maybe there's some DOS vector i'm missing)

@elichai I don't think there is a risk of DoS here, serializerHeader as all CMessageHeader components have a fixed-length. Bandwidth-DoS is already left unbounded (see default of -maxuploadtarget)

maflcko

ACK 16d6113 🙎

Show signature and timestamp

Signature:

Timestamp of file with hash 2ae5b6b8bcff0b2640a243d5665c57ba947e1d6ef581e8eb4aa04cfd1b160d31 -

maflcko · 2020-02-27T18:09:48Z

src/net.cpp

@@ -2703,6 +2716,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
    }

    m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));
+    m_serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());


Any reason this needs to call the copy(or move) constructor?

Suggested change

m_serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());

m_serializer = MakeUnique<V1TransportSerializer>();

practicalswift · 2020-02-27T18:29:05Z

Any chance we can move forward with the V1TransportSerializer fuzzer in #17771 to get additional robustness testing of this code before proceeding with the V1TransportSerializer refactoring(s)?

It should hopefully be trivial to review :)

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

jonasschnelli · 2020-03-02T10:00:13Z

@practicalswift: are you working on fuzz testing the serializing part?

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: ``` 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. ``` Backport of core [[bitcoin/bitcoin#16562 | PR16562]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK, jasonbcox Reviewed By: #bitcoin_abc, PiRK, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8374

dhruv · 2021-05-24T14:20:20Z

@practicalswift: are you working on fuzz testing the serializing part?

@jonasschnelli: I've attempt to extend the test to serialization in #22029. I've also discovered that the deserialization does not proceed 60-70% of the time due to a failed checksum test. Will try to address that and increase coverage in another commit this week.

jonasschnelli added the P2P label Aug 7, 2019

promag reviewed Aug 7, 2019

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

jonasschnelli force-pushed the 2019/06/net_refactor_2 branch from 586b0b3 to b840e42 Compare August 8, 2019 07:09

ariard approved these changes Aug 14, 2019

View reviewed changes

src/net.h Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

practicalswift reviewed Aug 15, 2019

View reviewed changes

src/net.cpp Show resolved Hide resolved

jonasschnelli force-pushed the 2019/06/net_refactor_2 branch from b840e42 to 88ddebb Compare August 15, 2019 13:53

laanwj added the Refactoring label Oct 2, 2019

DrahtBot added the Needs rebase label Oct 28, 2019

Refactor message transport packaging

16d6113

jonasschnelli force-pushed the 2019/06/net_refactor_2 branch from 88ddebb to 16d6113 Compare January 30, 2020 10:31

DrahtBot removed the Needs rebase label Jan 30, 2020

elichai reviewed Feb 3, 2020

View reviewed changes

maflcko approved these changes Feb 27, 2020

View reviewed changes

maflcko merged commit eca4d8e into bitcoin:master Feb 28, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

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

merge bitcoin#16562: Refactor message transport packaging

4bfe1da

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

merge bitcoin#16562: Refactor message transport packaging

31cdcb6

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

merge bitcoin#16562: Refactor message transport packaging

e375835

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

merge bitcoin#16562: Refactor message transport packaging

04cef82

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

	m_serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());
	m_serializer = MakeUnique<V1TransportSerializer>();

Refactor message transport packaging #16562

Refactor message transport packaging #16562

Uh oh!

Conversation

jonasschnelli commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag 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!

Uh oh!

jonasschnelli commented Aug 8, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongcarl commented Jan 22, 2020

Uh oh!

jonasschnelli commented Jan 30, 2020

Uh oh!

elichai left a comment

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Feb 25, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Mar 2, 2020

Uh oh!

dhruv commented May 24, 2021

Uh oh!

Uh oh!

jonasschnelli commented Aug 7, 2019 •

edited

Loading

DrahtBot commented Aug 7, 2019 •

edited

Loading

promag left a comment •

edited

Loading

dongcarl commented Feb 4, 2020 •

edited

Loading

practicalswift commented Feb 27, 2020 •

edited

Loading