[net] Don't return an optional from TransportDeserializer::GetMessage()

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:

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.

fanquake added the label P2P on Aug 18, 2021

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.

[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

[net] Replace GetID() with id in TransportDeserializer constructor f3e451bebf

jnewbery force-pushed on Aug 19, 2021

practicalswift commented at 9:32 pm on August 19, 2021: contributor

Concept ACK

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.

theStack commented at 11:30 am on August 20, 2021: member

Concept ACK

theStack approved

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.

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.

ryanofsky approved

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.

MarcoFalke merged this on Nov 2, 2021

MarcoFalke closed this on Nov 2, 2021

jnewbery deleted the branch on Nov 2, 2021

sidhujag referenced this in commit 62dfca15f3 on Nov 2, 2021

DrahtBot locked this on Nov 2, 2022

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

Conflicts