net: have GetReceivedMessage() return bytes #30213

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2024/05/transport-sans-netmsg changing 2 files +90 −58
  1. Sjors commented at 2:39 pm on May 31, 2024: member

    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:

    • clean up
    • fix tests
    • implement SetMessageToSend
  2. net: have GetReceivedMessage() return bytes
    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.
    11bec52511
  3. DrahtBot commented at 2:39 pm on May 31, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

  4. DrahtBot added the label P2P on May 31, 2024
  5. Sjors commented at 2:43 pm on May 31, 2024: member
    v2 is fairly straight-forward, but v1 is not.
  6. in src/net.cpp:670 in 11bec52511
    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) {
    


    sipa commented at 2:44 pm on May 31, 2024:
    Please don’t do this; the whole point of having separate transport classes is hiding the complexity of dealing with the difference from the rest of the network code.
  7. sipa commented at 2:48 pm on May 31, 2024: member

    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?

  8. Sjors commented at 3:16 pm on May 31, 2024: member

    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?

  9. DrahtBot added the label CI failed on May 31, 2024
  10. DrahtBot commented at 6:18 pm on May 31, 2024: contributor

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

    Debug: https://github.com/bitcoin/bitcoin/runs/25655524124

  11. Sjors commented at 6:51 pm on May 31, 2024: member
    As discussed in #30209 (comment) I’m going to try a different approach first.
  12. Sjors closed this on May 31, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me