Refactor message transport packaging #16562

jonasschnelli commented at 2:42 PM on August 7, 2019: contributor

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.

jonasschnelli added the label P2P on Aug 7, 2019

DrahtBot commented at 6:29 PM on August 7, 2019: member

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

Conflicts

No conflicts as of last run.

in src/net.h:622 in 586b0b3bf9 outdated

 613 | @@ -614,12 +614,27 @@ class CNetMessage {
 614 |      int readData(const char *pch, unsigned int nBytes);
 615 |  };
 616 |  
 617 | +/** The TransportSerializer prepares messages for the network transport
 618 | + */
 619 | +class TransportSerializer {
 620 | +public:
 621 | +    // prepare message for transport (insert header, etc.)
 622 | +    virtual bool prepareForTransport(CSerializedNetMsg& msg) = 0;

promag commented at 11:12 PM on August 7, 2019:

Return value is currently ignored, either make it void or assert(result)?

promag commented at 11:21 PM on August 7, 2019: member

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

jonasschnelli force-pushed on Aug 8, 2019

jonasschnelli commented at 7:09 AM on August 8, 2019: contributor

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

in src/net.h:708 in b840e424f5 outdated

 618 | + */
 619 | +class TransportSerializer {
 620 | +public:
 621 | +    // prepare message for transport (insert header, etc.)
 622 | +    virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) = 0;
 623 | +    virtual ~TransportSerializer() {}

ariard commented at 10:32 PM on August 14, 2019:

Do you expect serializer to keep state between call and so need to clean it with a destructor? (or just a good practice)

jonasschnelli commented at 3:13 PM on August 15, 2019:

Since we keep the instances in an std::unique_ptr, we need to have a virtual destructor.

in src/net.h:722 in b840e424f5 outdated

 631 |  /** Information about a peer */
 632 |  class CNode
 633 |  {
 634 |      friend class CConnman;
 635 |  public:
 636 | +    std::unique_ptr<TransportSerializer> m_serializer;

ariard commented at 10:36 PM on August 14, 2019:

This change would mean we initialize serializer at ConnectNode/AcceptConnection, if we make an outbound connection, thanks to NODE_P2P_V2 we can be sure to have right serializer, but in case of inbound we may need to update it. Maybe comment this logic here?

jonasschnelli commented at 1:21 PM on August 15, 2019:

Thanks for the comment. Your concerns are valid but not part of this PR. For a BIP324 (NODE_P2P_V2) inbound connection (also outbound) we always start with a V1 serializer/deserializer to perform the handshake. I think the handshake/a.k.a. protocol upgrade belongs to the V1 protocol.

Once the handshake has been completed, we exchange the node objects serializer/deserializer with a V2 instance.

ariard commented at 2:34 PM on August 15, 2019:

You mean the handshake belongs to the V1 protocol because we are using V1 messages to exchange 32-byte public keys between initiator/responder as defined in V2 BIP ? That makes sense and it's already implicit in the BIP, "After a successful handshake, messages MUST use the "v2 messages structure", but maybe put a word about this at the beginning of handshake section, just to be really obvious.

in src/net.h:621 in b840e424f5 outdated

 613 | @@ -614,12 +614,27 @@ class CNetMessage {
 614 |      int readData(const char *pch, unsigned int nBytes);
 615 |  };
 616 |  
 617 | +/** The TransportSerializer prepares messages for the network transport
 618 | + */
 619 | +class TransportSerializer {
 620 | +public:
 621 | +    // prepare message for transport (insert header, etc.)

ariard commented at 10:43 PM on August 14, 2019:

nit: Comments could be better for public interfaces, parent class could list the abstract operations a transport protocol may realize like header construction, error-correction computation, payload encryption and then for transport subclass would list the concrete sequence of operations implemented?

jonasschnelli commented at 1:53 PM on August 15, 2019:

Slightly improved...

ariard approved

ariard commented at 10:45 PM on August 14, 2019: member

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

in src/net.cpp:719 in b840e424f5 outdated

 689 | @@ -690,6 +690,19 @@ const uint256& CNetMessage::GetMessageHash() const
 690 |      return data_hash;
 691 |  }
 692 |  
 693 | +void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) {

practicalswift commented at 8:03 AM on August 15, 2019:

Should be override?

jonasschnelli commented at 1:54 PM on August 15, 2019:

Thanks. Fixed.

jonasschnelli force-pushed on Aug 15, 2019

laanwj added the label Refactoring on Oct 2, 2019

DrahtBot added the label Needs rebase on Oct 28, 2019

Refactor message transport packaging 16d6113f4f

dongcarl commented at 8:01 PM on January 22, 2020: member

Rebased this PR for you as well: https://github.com/bitcoin/bitcoin/compare/master...dongcarl:2020-01-net_refactor_2-rebased

jonasschnelli force-pushed on Jan 30, 2020

jonasschnelli commented at 10:32 AM on January 30, 2020: contributor

Thanks @dongcarl. Pushed your rebased version.

DrahtBot removed the label Needs rebase on Jan 30, 2020

elichai commented at 4:37 PM on February 3, 2020: contributor

semiACK 16d6113f4faa901e248adb693d4768a9e5019a16 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 commented at 6:18 PM on February 4, 2020: member

ACK 16d6113f4faa901e248adb693d4768a9e5019a16 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 commented at 1:34 AM on February 25, 2020: member

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)

in src/net.cpp:2719 in 16d6113f4f

2715 | @@ -2703,6 +2716,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
2716 |      }
2717 |  
2718 |      m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));
2719 | +    m_serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());

MarcoFalke commented at 6:09 PM on February 27, 2020:

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

    m_serializer = MakeUnique<V1TransportSerializer>();

MarcoFalke approved

MarcoFalke commented at 6:14 PM on February 27, 2020: member

ACK 16d6113f4faa901e248adb693d4768a9e5019a16 🙎

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 2ae5b6b8bcff0b2640a243d5665c57ba947e1d6ef581e8eb4aa04cfd1b160d31 -

</details>

practicalswift commented at 6:29 PM on February 27, 2020: contributor

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

MarcoFalke merged this on Feb 28, 2020

MarcoFalke closed this on Feb 28, 2020

sidhujag referenced this in commit b6ff52dc4d on Feb 29, 2020

jonasschnelli commented at 10:00 AM on March 2, 2020: contributor

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

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

jasonbcox referenced this in commit 06ce66282a on Nov 13, 2020

dhruv commented at 2:20 PM on May 24, 2021: member

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

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

kittywhiskers referenced this in commit 4bfe1da5e8 on Nov 1, 2021

kittywhiskers referenced this in commit 31cdcb64b8 on Nov 3, 2021

pravblockc referenced this in commit e375835d7d on Nov 18, 2021

gades referenced this in commit 04cef827d9 on May 17, 2022

DrahtBot locked this on Aug 16, 2022