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:
This is #20364, rebased on master.
Requesting rereview from @ryanofsky @MarcoFalke and @ajtowns, who reviewed the original PR.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
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
Concept ACK
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);
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) { ... })
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.
Concept ACK
Code-review ACK f3e451bebfe2e2d8de901d8ac29c064a51d3b746
Also rebased on master + verified that the build is clean and all tests pass.
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));
f3e451bebfe2e2d8de901d8ac29c064a51d3b746:
Seem redundant to construct the type twice, fist by calling the constructor, then by calling the copy constructor.
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.
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.