Rename message_command variables in src/net* and src/rpc/net.cpp #24141

shaavan · 2022-01-24T17:05:12Z

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.

hebasto · 2022-01-24T17:15:45Z

Concept ACK. Making rename commits a "scripted-diff" will simplify reviewing a lot.

prusnak · 2022-01-24T18:55:33Z

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: eb04bad

hebasto · 2022-01-24T19:33:37Z

... and random commit showing how the commit message should look like: eb04bad

Another example -- c4a31ca -- with git grep --files-with-matches usage.

RandyMcMillan · 2022-01-24T21:12:33Z

I ran this script against your PR - https://raw.githubusercontent.com/shaavan/bitcoin/dd720d20d994bcac9974ca12db849baaff5adabe/24141-scripted-diff.sh

#!/bin/bash -e

function s1() { git grep -l "$1" src/net* | xargs sed -i .old "s/$1/$2/g"; }

 s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
 s1 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_OTHER'
 s1 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsg'
 s1 'mapMsgCmdSize' 'mapMsgSize'
 s1 'sendPerMsgCmd' 'sendPerMsg'
 s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
 s1 'recvPerMsgCmd' 'recvPerMsg'

function s2() { git grep -l "$1" src/rpc/net* | xargs sed -i .old "s/$1/$2/g"; }

 s2 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
 s2 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_OTHER'
 s2 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsg'
 s2 'mapMsgCmdSize' 'mapMsgSize'
 s2 'sendPerMsgCmd' 'sendPerMsg'
 s2 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsg'
 s2 'recvPerMsgCmd' 'recvPerMsg'

RandyMcMillan · 2022-01-24T21:15:16Z

Concept ACK

w0xlt

Approach ACK

src/rpc/net.cpp

src/net.cpp

w0xlt · 2022-01-24T21:32:52Z

src/net.cpp

@@ -102,7 +102,7 @@ enum BindFlags {
 // The sleep time needs to be small to avoid new sockets stalling
 static const uint64_t SELECT_TIMEOUT_MILLISECONDS = 50;

-const std::string NET_MESSAGE_COMMAND_OTHER = "*other*";
+const std::string NET_MESSAGE_OTHER = "*other*";


Shouldn't NET_MESSAGE_COMMAND_OTHER be renamed to NET_MESSAGE_TYPE_OTHER?

src/net.cpp

src/net.h

src/rpc/net.cpp

RandyMcMillan · 2022-01-24T23:29:26Z

After some consideration - I generally agree with @w0xlt - the values held in the string are defined in

bitcoin/src/protocol.h

Line 64 in 9ec3991

namespace NetMsgType {

&

bitcoin/src/protocol.cpp

Line 12 in 9ec3991

namespace NetMsgType {

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

DrahtBot · 2022-01-24T23:49:03Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

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.

shaavan · 2022-01-25T13:41:42Z

Updated from e80330b to 8f31aad (pr24141.01 -> pr24141.02)
Addressed @hebasto @w0xlt suggestions.

Changes:

Renamed commit message to a scripted-diff message.
Renamed variables from 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.

w0xlt

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.

RandyMcMillan · 2022-01-28T17:52:23Z

@w0xlt - Interesting nuance to scripted diffs - thanks for the insight. 😄 - basically each change needs its own commit in incremental steps.

shaavan · 2022-01-29T13:17:57Z

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.

shaavan · 2022-01-29T17:37:22Z

Updated from 8f31aad to d077901 (pr24141.02 -> pr24141.03)
Addressed @w0xlt suggestion.

Changes:

Separated comment changes in a separate commit, allowing lint test to pass successfully on scripted-diff commits.

w0xlt

reACK d077901

maflcko · 2022-01-30T16:26:39Z

Does each commit compile on its own?

shaavan · 2022-01-31T10:32:05Z

Updated from d077901 to ec50abf (pr24141.03 -> pr24141.04)
Addressed @MarcoFalke comment.

Changes:

Merged scripted diff commits (commit f85a0fc and d077901) together. This was done to make the individual commits compilable.

Thanks, @MarcoFalke, for catching it.

theStack

Concept ACK

Some more candidates for the second commit:

diff --git a/src/net.cpp b/src/net.cpp
index f3ae11ce1..04d9b318d 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -748,7 +748,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
     // decompose a single CNetMessage from the TransportDeserializer
     CNetMessage msg(std::move(vRecv));

-    // store command string, time, and sizes
+    // store message type string, time, and sizes
     msg.m_type = hdr.GetCommand();
     msg.m_time = time;
     msg.m_message_size = hdr.nMessageSize;
@@ -759,7 +759,7 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
     // We just received a message off the wire, harvest entropy from the time (and the message checksum)
     RandAddEvent(ReadLE32(hash.begin()));

-    // Check checksum and header command string
+    // Check checksum and header message type string
     if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
         LogPrint(BCLog::NET, "Header error: Wrong checksum (%s, %u bytes), expected %s was %s, peer=%d\n",
                  SanitizeString(msg.m_type), msg.m_message_size,

diff --git a/src/net.h b/src/net.h
index b1a19bcc4..62d23f6c4 100644
--- a/src/net.h
+++ b/src/net.h
@@ -298,7 +298,7 @@ public:

 /** The TransportDeserializer takes care of holding and deserializing the
  * network receive buffer. It can deserialize the network buffer into a
- * transport protocol agnostic CNetMessage (command & payload)
+ * transport protocol agnostic CNetMessage (message type & payload)
  */
 class TransportDeserializer {
 public:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index cc23876af..3bcf2d1b1 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4078,8 +4078,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         return;
     }

-    // Ignore unknown commands for extensibility
-    LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
+    // Ignore unknown messages types for extensibility
+    LogPrint(BCLog::NET, "Unknown message type \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
     return;
 }

(Though changing a log message is not a refactor strictly speaking, so not sure if it's appropriate in this PR.)

shaavan · 2022-03-07T04:35:29Z

Updated from ec50abf to 15eb7b9 (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:

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.

theStack

Code-review ACK 15eb7b9

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?

maflcko · 2022-03-08T14:26:32Z

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:

    scripted-diff: Rename message command to message type
    
    -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-

laanwj · 2022-04-06T11:25:38Z

Concept ACK

maflcko · 2022-04-06T11:29:55Z

Are you still working on this? #24141 (comment)

-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-

shaavan · 2022-04-07T12:05:43Z

Updated from pr24141.05 → pr24141.06

Changes:

Used the scripted-diff commit message as suggested by @MarcoFalke in this comment.

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.

maflcko

ACK

maflcko · 2022-04-07T12:15:27Z

src/net.h

-extern const std::string NET_MESSAGE_COMMAND_OTHER;
-typedef std::map<std::string, uint64_t> mapMsgCmdSize; //command, total bytes
+extern const std::string NET_MESSAGE_TYPE_OTHER;
+typedef std::map<std::string, uint64_t> mapMsgTypeSize; //message, total bytes


nit: Second commit s/message/message type/. Or:

Suggested change

typedef std::map<std::string, uint64_t> mapMsgTypeSize; //message, total bytes

using 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.

- // to prevent a memory DOS, only allow valid commands + // 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.

shaavan · 2022-04-08T11:53:58Z

Updated from pr24141.06 → pr24141.07

Addressed @MarcoFalke suggestion.

Changes:

Replaced usage of typedef with using in defining mapMsgTypeSize.
Replaced message with message type in the comment defining the above.

maflcko · 2022-04-08T12:26:00Z

src/net.cpp

-            // Store received bytes per message command
-            // to prevent a memory DOS, only allow valid commands
+            // Store received bytes per message type
+            // 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.

Thanks for seeing it. I think you are right. Using "known message types" would be a better wording for this sentence.
Let me address it by updating the second commit.

jonatack · 2022-04-08T12:41:10Z

Approach ACK

…files Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

shaavan · 2022-04-16T11:37:25Z

Update from pr24141.07 → pr24141.08, diff

Addressed suggestion by @MarcoFalke

Changes:

Change the wording of the comment from “allow valid messages” to “allow known message types” as some of the messages (like reject, as pointed out by @MarcoFalke, may be valid but yet unknown to the current version of bitcoin core)
Also did some punctuation corrections to make the comment grammatically sound.

maflcko

review ACK e71c51b 💥

Show signature

Signature:

… src/rpc/net.cpp e71c51b refactor: rename command -> message type in comments in the src/net* files (Shashwat) 2b09593 scripted-diff: Rename message command to message type (Shashwat) Pull request description: This PR is a follow-up to bitcoin#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 bitcoin#24078 and does similar changes in the `src/rpc/net.cpp` file. ACKs for top commit: MarcoFalke: review ACK e71c51b 💥 Tree-SHA512: 24015d132c00f15239e5d3dc7aedae904ae3103a90920bb09e984ff57723402763f697d886322f78e42a0cb46808cb6bc9d4905561dc6ddee9961168f8324b05

DrahtBot added P2P RPC/REST/ZMQ labels Jan 24, 2022

w0xlt reviewed Jan 24, 2022

View reviewed changes

RandyMcMillan mentioned this pull request Jan 25, 2022

WIP: net/p2p:rename command*/Command/* to message*/Message* #24143

Closed

DrahtBot mentioned this pull request Jan 25, 2022

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

shaavan force-pushed the 220124-message branch from e80330b to 8f31aad Compare January 25, 2022 13:40

w0xlt approved these changes Jan 28, 2022

View reviewed changes

shaavan force-pushed the 220124-message branch from 8f31aad to d077901 Compare January 29, 2022 17:36

w0xlt approved these changes Jan 30, 2022

View reviewed changes

shaavan force-pushed the 220124-message branch from d077901 to ec50abf Compare January 31, 2022 10:30

theStack reviewed Feb 2, 2022

View reviewed changes

shaavan force-pushed the 220124-message branch from ec50abf to 15eb7b9 Compare March 7, 2022 04:32

shaavan requested a review from theStack March 8, 2022 07:49

theStack approved these changes Mar 8, 2022

View reviewed changes

maflcko removed RPC/REST/ZMQ P2P labels Mar 8, 2022

maflcko added the Refactoring label Mar 8, 2022

DrahtBot mentioned this pull request Mar 12, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

DrahtBot mentioned this pull request Apr 5, 2022

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

shaavan force-pushed the 220124-message branch from 15eb7b9 to 07ecabf Compare April 7, 2022 12:02

maflcko approved these changes Apr 7, 2022

View reviewed changes

shaavan force-pushed the 220124-message branch from 07ecabf to 26b7f9f Compare April 8, 2022 11:53

maflcko reviewed Apr 8, 2022

View reviewed changes

refactor: rename command -> message type in comments in the src/net* …

e71c51b

…files Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

shaavan force-pushed the 220124-message branch from 26b7f9f to e71c51b Compare April 16, 2022 11:37

maflcko approved these changes May 5, 2022

View reviewed changes

maflcko merged commit 0d080a1 into bitcoin:master May 5, 2022

bitcoin locked and limited conversation to collaborators May 5, 2023

	typedef std::map<std::string, uint64_t> mapMsgTypeSize; //message, total bytes
	using mapMsgTypeSize = std::map</* msg type / std::string, / total bytes */ uint64_t>;

Rename message_command variables in src/net* and src/rpc/net.cpp #24141

Rename message_command variables in src/net* and src/rpc/net.cpp #24141

Uh oh!

Conversation

shaavan commented Jan 24, 2022

Uh oh!

hebasto commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prusnak commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 24, 2022

Uh oh!

RandyMcMillan commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RandyMcMillan commented Jan 24, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

w0xlt Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RandyMcMillan commented Jan 24, 2022

Uh oh!

DrahtBot commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

shaavan commented Jan 25, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Jan 28, 2022

Uh oh!

shaavan commented Jan 29, 2022

Uh oh!

shaavan commented Jan 29, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 30, 2022

Uh oh!

shaavan commented Jan 31, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Mar 7, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 8, 2022

Uh oh!

laanwj commented Apr 6, 2022

Uh oh!

maflcko commented Apr 6, 2022

Uh oh!

shaavan commented Apr 7, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

shaavan Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jan 24, 2022 •

edited

Loading

prusnak commented Jan 24, 2022 •

edited

Loading

RandyMcMillan commented Jan 24, 2022 •

edited

Loading

DrahtBot commented Jan 24, 2022 •

edited

Loading

maflcko Apr 8, 2022 •

edited

Loading