This PR contains a few follow-ups to #19107.
Most notable is a change to GetMessage, where it no longer returns an optional. Ultimately this was done in a heavy-handed way that didn't improve readability and would probably cause more problems than it would save.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
CI fails with "Timed out!" when running the fuzz tests, at exactly 2h of running time. Looks to be unrelated to this PR.
You can fix that by rebasing on current master
750 | @@ -751,16 +751,16 @@ const uint256& V1TransportDeserializer::GetMessageHash() const 751 | return data_hash; 752 | } 753 | 754 | -Optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err_raw_size) 755 | +CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, bool& reject_message) 756 | { 757 | // decompose a single CNetMessage from the TransportDeserializer
In commit "Remove Optional from GetMessage" (eb21969fe3fcd845d4060d890f7c46d703238661)
Might be good to initialize reject_message = false here just to be clear this is an output parameter, not an in/out parameter
I hesitated away from this just in case the caller wanted to reject the message for some other reason before calling GetMessage. (So reject_message would already be true)
However, now writing that out into words, I think that's an unlikely scenario, and I agree that setting it at the beginning makes things clearer.
Thanks, made the change!
Code review ACK ec4fd8a7fd6efb60938adb21f6764ce67e12be50. Thanks for followups #19107 (review) and #19107 (review)!
781 | } else if (!hdr.IsCommandValid()) {
782 | - LogPrint(BCLog::NET, "HEADER ERROR - COMMAND (%s, %u bytes), peer=%d\n",
783 | - hdr.GetCommand(), msg->m_message_size, m_node_id);
784 | - out_err_raw_size = msg->m_raw_message_size;
785 | - msg = nullopt;
786 | + LogPrint(BCLog::NET, "HEADER ERROR - INVALID COMMAND (%s, %u bytes), peer=%d\n",
LogPrint(BCLog::NET, "HEADER ERROR - INVALID MSGTYPE (%s, %u bytes), peer=%d\n",
style-nit: When changing the log message, might as well change command->msg type, which is preferred in new code.
git range-diff master ec4fd8a a10135e
GetMesage no longer returns an optional.
Additionally, we now access mapRecvBytesPerMsgCmd with `.at` not
`.find`.
This is shorter and more clear.
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.
For this part, I think this is a definite improvement, but I'm not sure
it's the best solution. Open to suggestions.
git range-diff master a10135e 78fb468
reject_message2979 | @@ -2980,7 +2980,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn 2980 | LogPrint(BCLog::NET, "Added connection peer=%d\n", id); 2981 | } 2982 | 2983 | - m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params(), GetId(), SER_NETWORK, INIT_PROTO_VERSION)); 2984 | + m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));
GetId() is an inline method that returns id already; this change seems pretty pointless?
We should remove GetId() entirely. id is a const member so should just be made public.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-get
We certainly shouldn't be using trivial getters inside CNode methods. They can access member variables directly.
675 | //store received bytes per message command 676 | //to prevent a memory DOS, only allow valid commands 677 | - mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(result->m_command); 678 | + mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command); 679 | if (i == mapRecvBytesPerMsgCmd.end()) 680 | i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
Should change this .find() to .at() as well?
std::map::at returns a reference to the value, not an iterator on the map, which is what is required here for the i->second call below. Alternatively:
diff --git a/src/net.cpp b/src/net.cpp
index 9c2a79b809..e6a3d69810 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -670,11 +670,11 @@ bool CNode::ReceiveMsgBytes(const char *pch, unsigned int nBytes, bool& complete
//store received bytes per message command
//to prevent a memory DOS, only allow valid commands
- mapMsgCmdSize::iterator i = mapRecvBytesPerMsgCmd.find(msg.m_command);
- if (i == mapRecvBytesPerMsgCmd.end())
- i = mapRecvBytesPerMsgCmd.find(NET_MESSAGE_COMMAND_OTHER);
- assert(i != mapRecvBytesPerMsgCmd.end());
- i->second += msg.m_raw_message_size;
+ if (auto i = mapRecvBytesPerMsgCmd.find(msg.m_command); i != mapRecvBytesPerMsgCmd.end()) {
+ i->second += msg.m_raw_message_size;
+ } else {
+ mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += msg.m_raw_message_size;
+ }
PR title/description should summarise what was actually being done in this PR -- I think it's "[refactor] net.cpp: various minor cleanups" or similar?
I think you could split out ".find() to .at()" and "drop redundant out_err_raw_size" as separate commits; they seem pretty independent. I'm not sure having "this function might throw, but we're sure it never will" is better than "this function might deref end() but we're sure it never will" is better. Maybe an Assert() would be better, or maybe using &map[NET_MESSAGE_COMMAND_OTHER] (which creates it if missing) would be better.
I'm not seeing what the benefit of dropping the optional is though, it just changes:
Optional<CNetMessage> x = GetMessage(time);
if (!x) { ...; return; }
do_stuff(*x);
to
bool reject_message;
CNetMessage x = GetMessage(time, reject_message);
if (reject_message) { ...; return; }
do_stuff(x);
which doesn't seem like an improvement to me?
I'm not seeing what the benefit of dropping the optional is though, it just changes:
The benefit of dropping optional is that the calling code can reliably use msg.m_raw_message_size, instead of sometimes using m_raw_message_size and sometimes having to look in out_err_raw_size depending on the code path. It is easy to imagine a future change changing the logic a little and causing the wrong size to be read from the wrong location. Getting rid of optional also would have reduced the size of #19107 diff, which was the original reason for the suggestion, but that ship already sailed.
I'm not sure having "this function might throw, but we're sure it never will" is better than "this function might deref end() but we're sure it never will" is better.
I won't die on this hill but I do think an error is better than undefined behavior
Code review ACK 78fb468c7a1e5942cb1dc16864a4bbf8c85381ba. Only changes since last review are resetting output parameter and tweaking log messages
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
Code review ACK 78fb468c7a1e5942cb1dc16864a4bbf8c85381ba
Needs rebase
Suggest you remove "For this part, I think this is a definite improvement, but I'm not sure it's the best solution. Open to suggestions." from the first commit log. The commit messages should be used to explain the code changes, and github should be used for review/meta-commentary.
Closing this as up for grabs.