Fixes #30209.
Previously GetReceivedMessage() returned a CNetMessage. Have the caller process the bytes instead. Similarly SetMessageToSend needed a CSerializedNetMsg, give it bytes instead.
This makes the Transport class usable for future message types that don’t inherit from CNetMessage.
TODO:
SetMessageToSend
Previously it returned a CNetMessage. Have the caller process the bytes instead. This makes the Transport class usable for future message types that don't inherit from CNetMessage.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
663@@ -664,7 +664,65 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
664 if (m_transport->ReceivedMessageComplete()) {
665 // decompose a transport agnostic CNetMessage from the deserializer
666 bool reject_message{false};
667- CNetMessage msg = m_transport->GetReceivedMessage(time, reject_message);
668+ CNetMessage msg{DataStream{}};
669+ auto contents{m_transport->GetReceivedMessage()};
670+
671+ switch (m_transport->GetInfo().transport_type) {
Would an alternative approach be that V1Transport and V2Transport always just treat the first 12 bytes of a message as the command type (as in on decode they’d produce a message with “length” 12+payload_len, etc). That would mean the Transport interface works purely on bytes, and could be usable for protocols that don’t deal with things that have a command type.
Another option is having two layers: a “raw transport” interface that works on bytes, and a layer “command-based transport” interface on top?
In Sv2Transport (see here ) the encrypted header contains both the message length and size. In V2Transport we first get the size and then get the type and its content. And in V1Transport we first get the message type and then the size followed by the content.
At the very minimum Transport needs to know the message size. Currently the V1Transport and V2Transport implementation figure that out. They also perform all the additional steps to construct a CNetMessage.
In my initial attempt here I moved some of that work to CNode, but it’s a mess.
and a layer “command-based transport” interface on top
I’m not entirely sure what you mean here, but I guess we’d want some new class that tells Transport how many bytes to fetch and hands off a CNetMessage to CNode::ReceiveMsgBytes?
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.