[net] Don’t return an optional from TransportDeserializer::GetMessage() #22735

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:2021-08-20364-rebased changing 3 files +35 −36
  1. jnewbery commented at 12:55 pm on August 18, 2021: member

    Also, access mapRecvBytesPerMsgCmd with at() not find(). This throws an error if COMMAND_OTHER doesn’t exist, which should never happen. find() instead just accessed the last element, which could make debugging more difficult.

    Resolves review comments from PR19107:

  2. jnewbery commented at 12:56 pm on August 18, 2021: member

    This is #20364, rebased on master.

    Requesting rereview from @ryanofsky @MarcoFalke and @ajtowns, who reviewed the original PR.

  3. fanquake added the label P2P on Aug 18, 2021
  4. DrahtBot commented at 8:37 am on August 19, 2021: member

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

    Conflicts

    No conflicts as of last run.

  5. [net] Don't return an optional from TransportDeserializer::GetMessage()
    Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This
    throws an error if COMMAND_OTHER doesn't exist, which should never
    happen. `find()` instead just accessed the last element, which could make
    debugging more difficult.
    
    Resolves review comments from PR19107:
    
    - https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436
    - https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497
    8c96008ab1
  6. [net] Replace GetID() with id in TransportDeserializer constructor f3e451bebf
  7. jnewbery force-pushed on Aug 19, 2021
  8. practicalswift commented at 9:32 pm on August 19, 2021: contributor
    Concept ACK
  9. in src/test/fuzz/p2p_transport_serialization.cpp:73 in 8c96008ab1 outdated
    75-                assert(result->m_raw_message_size <= mutable_msg_bytes.size());
    76-                assert(result->m_raw_message_size == CMessageHeader::HEADER_SIZE + result->m_message_size);
    77-                assert(result->m_time == m_time);
    78+            bool reject_message{false};
    79+            CNetMessage msg = deserializer.GetMessage(m_time, reject_message);
    80+            assert(msg.m_command.size() <= CMessageHeader::COMMAND_SIZE);
    


    theStack commented at 11:29 am on August 20, 2021:
    Just to get sure, is it intended that the block following the GetMessage is now not conditional any more? (i.e. within a if (!reject_message) { ... })

    jnewbery commented at 9:16 am on September 30, 2021:
    Yes, it’s essentially reverting the change here: https://github.com/bitcoin/bitcoin/commit/890b1d7c2b8312d41d048d2db124586c5dbc8a49#diff-7f9b91d8cb2eaad7b4c0e1a9a4650d9a089c0908faa0502703f3b13a920558afR37, when the return type of GetMessage() was changed to an optional.
  10. theStack commented at 11:30 am on August 20, 2021: member
    Concept ACK
  11. theStack approved
  12. theStack commented at 8:36 pm on October 5, 2021: member

    Code-review ACK f3e451bebfe2e2d8de901d8ac29c064a51d3b746

    Also rebased on master + verified that the build is clean and all tests pass.

  13. in src/net.cpp:3010 in f3e451bebf
    3006@@ -3007,7 +3007,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, SOCKET hSocketIn, const
    3007         LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
    3008     }
    3009 
    3010-    m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION));
    3011+    m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));
    


    MarcoFalke commented at 8:24 am on October 11, 2021:

    f3e451bebfe2e2d8de901d8ac29c064a51d3b746:

    Seem redundant to construct the type twice, fist by calling the constructor, then by calling the copy constructor.


    jnewbery commented at 11:05 am on October 11, 2021:
    Definitely true, but unrelated to this PR. Very happy to open/review a PR that changes the initialization of these members. They could also be const and initialized in the initializer list, and made private.
  14. ryanofsky approved
  15. ryanofsky commented at 8:48 pm on November 1, 2021: member

    Code review ACK f3e451bebfe2e2d8de901d8ac29c064a51d3b746. Changes since last review in #20364#pullrequestreview-534369904 were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit.

    The cleanups here are all small and straightforward, but I think the best one is getting rid of the unchecked map.find iterator dereference, and getting rid of the redundant & sometimes initialized out_err_raw_size output parameter.

  16. MarcoFalke merged this on Nov 2, 2021
  17. MarcoFalke closed this on Nov 2, 2021

  18. jnewbery deleted the branch on Nov 2, 2021
  19. sidhujag referenced this in commit 62dfca15f3 on Nov 2, 2021
  20. DrahtBot locked this on Nov 2, 2022

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-23 00:13 UTC

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