This PR is a follow-up to #24078.
a message is not a command, but simply a message of some type
The first commit covers the message_command variable name and comments not addressed in the original PR in src/net* files.
The second commit goes beyond the original src/net* limit of #24078 and does similar changes in the src/rpc/net.cpp file.
Concept ACK. Making rename commits a “scripted-diff” will simplify reviewing a lot.
Link to documentation how to use scripted-diff https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs and random commit showing how the commit message should look like: eb04badcd6108e17c8ec78e9c316775c334030cb
I ran this script against your PR - https://raw.githubusercontent.com/shaavan/bitcoin/dd720d20d994bcac9974ca12db849baaff5adabe/24141-scripted-diff.sh
0#!/bin/bash -e
1
2function s1() { git grep -l "$1" src/net* | xargs sed -i .old "s/$1/$2/g"; }
3
4 s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
5 s1 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_OTHER'
6 s1 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsg'
7 s1 'mapMsgCmdSize' 'mapMsgSize'
8 s1 'sendPerMsgCmd' 'sendPerMsg'
9 s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
10 s1 'recvPerMsgCmd' 'recvPerMsg'
11
12function s2() { git grep -l "$1" src/rpc/net* | xargs sed -i .old "s/$1/$2/g"; }
13
14 s2 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
15 s2 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_OTHER'
16 s2 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsg'
17 s2 'mapMsgCmdSize' 'mapMsgSize'
18 s2 'sendPerMsgCmd' 'sendPerMsg'
19 s2 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
20 s2 'recvPerMsgCmd' 'recvPerMsg'
155@@ -156,7 +156,7 @@ static RPCHelpMan getpeerinfo()
156 {RPCResult::Type::NUM, "msg", "The total bytes received aggregated by message type\n"
157 "When a message type is not listed in this json object, the bytes received are 0.\n"
158 "Only known message types can appear as keys in the object and all bytes received\n"
159- "of unknown message types are listed under '"+NET_MESSAGE_COMMAND_OTHER+"'."}
604@@ -605,12 +605,12 @@ void CNode::CopyStats(CNodeStats& stats)
605 X(m_bip152_highbandwidth_from);
606 {
607 LOCK(cs_vSend);
608- X(mapSendBytesPerMsgCmd);
609+ X(mapSendBytesPerMsg);
mapSendBytesPerMsgCmd be renamed to mapSendBytesPerMsgType?
I think this would be a better naming convention for the variables.
Addressed this and the following comments in 8f31aadd25ad434536ba0d166b81324a18180acb
101@@ -102,7 +102,7 @@ enum BindFlags {
102 // The sleep time needs to be small to avoid new sockets stalling
103 static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50;
104
105-const std::string NET_MESSAGE_COMMAND_OTHER = "*other*";
106+const std::string NET_MESSAGE_OTHER = "*other*";
NET_MESSAGE_COMMAND_OTHER be renamed to NET_MESSAGE_TYPE_OTHER?
610 X(nSendBytes);
611 }
612 {
613 LOCK(cs_vRecv);
614- X(mapRecvBytesPerMsgCmd);
615+ X(mapRecvBytesPerMsg);
mapRecvBytesPerMsgCmd be renamed to mapRecvBytesPerMsgType?
232@@ -233,8 +233,8 @@ struct LocalServiceInfo {
233 extern Mutex g_maplocalhost_mutex;
234 extern std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(g_maplocalhost_mutex);
235
236-extern const std::string NET_MESSAGE_COMMAND_OTHER;
237-typedef std::map<std::string, uint64_t> mapMsgCmdSize; //command, total bytes
238+extern const std::string NET_MESSAGE_OTHER;
239+typedef std::map<std::string, uint64_t> mapMsgSize; //command, total bytes
mapMsgCmdSize be renamed to mapMsgTypeSize ?
254- obj.pushKV("bytessent_per_msg", sendPerMsgCmd);
255+ obj.pushKV("bytessent_per_msg", sendPerMsg);
256
257- UniValue recvPerMsgCmd(UniValue::VOBJ);
258- for (const auto& i : stats.mapRecvBytesPerMsgCmd) {
259+ UniValue recvPerMsg(UniValue::VOBJ);
recvPerMsgCmd be renamed to recvPerMsgType ?
242@@ -243,19 +243,19 @@ static RPCHelpMan getpeerinfo()
243 obj.pushKV("permissions", permissions);
244 obj.pushKV("minfeefilter", ValueFromAmount(stats.minFeeFilter));
245
246- UniValue sendPerMsgCmd(UniValue::VOBJ);
247- for (const auto& i : stats.mapSendBytesPerMsgCmd) {
248+ UniValue sendPerMsg(UniValue::VOBJ);
sendPerMsgCmd be renamed to sendPerMsgType ?
After some consideration - I generally agree with @w0xlt - the values held in the string are defined in
https://github.com/bitcoin/bitcoin/blob/9ec3991ad3b2ae91997c696a4c2f187fe538eff0/src/protocol.h#L64
&
The Cmd part of the naming convention seems to be taken from COMMAND_SIZE originally defined here -> https://github.com/bitcoin/bitcoin/blame/c729dbb6d2b63d9c0b32e3937be0ebf528d4c753/src/protocol.h#L52
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Updated from e80330b to 8f31aad (pr24141.01 -> pr24141.02) Addressed @hebasto @w0xlt suggestions.
Changes:
message_command -> message_type instead of message_command -> message.Thanks, @prusnak and @hebasto, for the scripted-diff example and documentation. And thanks, @RandyMacmillian, for the script message, which I have used as an inspiration for the commit message.
ACK 8f31aad
Lint is failing because the changed comments do not match the pattern of the sed command.
Change the comments in another commit and this CI error will be fixed.
Lint is failing because the changed comments do not match the pattern of the sed command.
Nice catch, @w0xlt! I reckoned the comment change is relatively small and wouldn’t require a separate commit. But let’s keep the lint intact. Let me update the PR with separating comment changes in a new commit.
Updated from 8f31aad to d077901 (pr24141.02 -> pr24141.03) Addressed @w0xlt suggestion.
Changes:
Updated from d077901 to ec50abf (pr24141.03 -> pr24141.04) Addressed @MarcoFalke comment.
Changes:
Thanks, @MarcoFalke, for catching it.
Concept ACK
Some more candidates for the second commit:
0diff --git a/src/net.cpp b/src/net.cpp
1index f3ae11ce1..04d9b318d 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -748,7 +748,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
5 // decompose a single CNetMessage from the TransportDeserializer
6 CNetMessage msg(std::move(vRecv));
7
8- // store command string, time, and sizes
9+ // store message type string, time, and sizes
10 msg.m_type = hdr.GetCommand();
11 msg.m_time = time;
12 msg.m_message_size = hdr.nMessageSize;
13@@ -759,7 +759,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
14 // We just received a message off the wire, harvest entropy from the time (and the message checksum)
15 RandAddEvent(ReadLE32(hash.begin()));
16
17- // Check checksum and header command string
18+ // Check checksum and header message type string
19 if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
20 LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
21 SanitizeString(msg.m_type), msg.m_message_size,
0diff --git a/src/net.h b/src/net.h
1index b1a19bcc4..62d23f6c4 100644
2--- a/src/net.h
3+++ b/src/net.h
4@@ -298,7 +298,7 @@ public:
5
6 /** The TransportDeserializer takes care of holding and deserializing the
7 * network receive buffer. It can deserialize the network buffer into a
8- * transport protocol agnostic CNetMessage (command & payload)
9+ * transport protocol agnostic CNetMessage (message type & payload)
10 */
11 class TransportDeserializer {
12 public:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index cc23876af..3bcf2d1b1 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -4078,8 +4078,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5 return;
6 }
7
8- // Ignore unknown commands for extensibility
9- LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
10+ // Ignore unknown messages types for extensibility
11+ LogPrint(BCLog::NET, "Unknown message type \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
12 return;
13 }
(Though changing a log message is not a refactor strictly speaking, so not sure if it’s appropriate in this PR.)
Updated from https://github.com/bitcoin/bitcoin/commit/ec50abf1d0666706093a004773cbfa5229c6f618 to 15eb7b94965827bb4e1ac11af9fc61fcbf044cd5 (pr24141.04 → pr24141.05, diff)
Addressed @theStack comments.
Included the suggestions suggested by @theStack here.
About the Log message: I thought about adding it in the 2nd commit, but then I inclined against it for the following reason:
But I think changing the log message would be essential to make it consistent with the other changes done in this PR. So I would require other reviewers’ suggestions on this matter, on whether it is better to edit the log message or not.
Code-review ACK 15eb7b94965827bb4e1ac11af9fc61fcbf044cd5
About the Log message: I thought about adding it in the 2nd commit, but then I inclined against it for the following reason:
- This will not strictly be a refactoring change
- This changes a file (net_processing.cpp) untouched in this PR.
But I think changing the log message would be essential to make it consistent with the other changes done in this PR. So I would require other reviewers’ suggestions on this matter, on whether it is better to edit the log message or not.
Probably it could be done with another follow-up (final) PR which replaces the remaining occurences of the “message command” terminology in src/protocol.{h,cpp} and src/net_processing.cpp?
Not sure about the scripted diff. It seems a bit bloated and throws errors (https://cirrus-ci.com/task/6681881558122496?logs=lint#L842). Try something like:
0 scripted-diff: Rename message command to message type
1
2 -BEGIN VERIFY SCRIPT-
3
4 s1() { sed -i "s/$1/$2/g" $(git grep -l "$1" ./); }
5
6 s1 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_TYPE_OTHER'
7 s1 'mapMsgCmdSize' 'mapMsgTypeSize'
8 s1 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsgType'
9 s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsgType'
10 s1 'recvPerMsgCmd' 'recvPerMsgType'
11 s1 'sendPerMsgCmd' 'sendPerMsgType'
12
13 -END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT-
s1() { sed -i "s/$1/$2/g" $(git grep -l "$1" ./); }
s1 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_TYPE_OTHER'
s1 'mapMsgCmdSize' 'mapMsgTypeSize'
s1 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsgType'
s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsgType'
s1 'recvPerMsgCmd' 'recvPerMsgType'
s1 'sendPerMsgCmd' 'sendPerMsgType'
-END VERIFY SCRIPT-Updated from pr24141.05 → pr24141.06
Changes:
Thanks, @MarcoFalke, for the script suggestion. Using your suggested script, I resolved the sed: no input files warning. I tried to add you as the Co-author of this commit; however, I could not find your public email ID, so I was unable to do so.
The current and the last version of the PR are identical (see the diff), except for the script used to create those changes, so I think it should be easy to review this update.
232@@ -233,8 +233,8 @@ struct LocalServiceInfo {
233 extern Mutex g_maplocalhost_mutex;
234 extern std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(g_maplocalhost_mutex);
235
236-extern const std::string NET_MESSAGE_COMMAND_OTHER;
237-typedef std::map<std::string, uint64_t> mapMsgCmdSize; //command, total bytes
238+extern const std::string NET_MESSAGE_TYPE_OTHER;
239+typedef std::map<std::string, uint64_t> mapMsgTypeSize; //message, total bytes
nit: Second commit s/message/message type/. Or:
0using mapMsgTypeSize = std::map</* msg type */ std::string, /* total bytes */ uint64_t>;
Thanks, @MarcoFalke, for taking another look. As per your suggestion, I was able to identify one more place in the second commit where your suggestion could be implemented.
0- // to prevent a memory DOS, only allow valid commands
1+ // to prevent a memory DOS, only allow valid messages
But I opted against changing the above from messages to message types, as that would have decreased the sentence’s logical soundness IMO.
Updated from pr24141.06 → pr24141.07
Addressed @MarcoFalke suggestion.
Changes:
typedef with using in defining mapMsgTypeSize.message with message type in the comment defining the above.659 }
660
661- // Store received bytes per message command
662- // to prevent a memory DOS, only allow valid commands
663+ // Store received bytes per message type
664+ // to prevent a memory DOS, only allow valid messages
I think this should say “known message types”?
For example, a reject message might be valid according to BIP 61, but the type is unknown to this version of Bitcoin Core.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Update from pr24141.07 → pr24141.08, diff
Addressed suggestion by @MarcoFalke
Changes:
reject, as pointed out by @MarcoFalke, may be valid but yet unknown to the current version of bitcoin core)
review ACK e71c51b27d420fbd6cc0a36f62e63e190e13473a 💥
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK e71c51b27d420fbd6cc0a36f62e63e190e13473a 💥
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhJ/Qv+KgTz8htAqBuJ6FnTHQdjyLswJvYHXukDceneF5E72Gifx7MaJw439PKt
8MjVpE6s1VCZIliOhSojkNrINaL07pLQu+MoDNSkdOHexfm6xlsDg29nBdba0ocLq
9meTNnDW+f+uUibpz+6NxY7Fur8YIpyz+Ney8X0EkOa73F01Dcy9rjAzRZRjCI/gk
10X9i7kWzByG7uTzAAvuNxV+F1Kg1qVX2GgMOtxmYj+qpbx2kVCd7YA2pUjYbLeuPL
119/SWmNdYnjFPznUUhby0HcSY0wwZ5urKIhWPgVeXpQgnQJu4WCNYum+ckBIThhor
12uS1IkAtylgZfwcEU4m8unW4dUoQkeCFyN8xVSsQztL1TinM3CmzKnsgZq1LkU2El
13i1HTy5qxZCybGh2y3R/bUuygFelDw9PZY2Qm/AtvCaf1rsowGAG6Xq0gAsKwON9V
14co1M/Qgh9fbizx5PJx3uIWRM3SwD0QEojRkLSikmnwdrc08OA2Eo3zN9kNzfpDaS
15CfbI81Qr
16=u23E
17-----END PGP SIGNATURE-----