scripted-diff: Replace strCommand with msg_type #18533

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2004-netMsgType changing 2 files +36 −36

MarcoFalke commented at 6:47 PM on April 5, 2020: member

Receiving a message is not a command, but simply a message of some type
DrahtBot commented at 7:35 PM on April 5, 2020: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #18317 (Serialization improvements step 6 (all except wallet/gui) by sipa)
- #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
- #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
- #16442 (Serve BIP 157 compact filters by jimpo)
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.
DrahtBot added the label P2P on Apr 5, 2020
DrahtBot added the label Refactoring on Apr 5, 2020
DrahtBot cross-referenced this on Apr 5, 2020 from issue Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery
naumenkogs commented at 10:40 PM on April 5, 2020: member

Not sure how we go about refactoring like this, but I do like this change. ACK!

I was thinking maybe we should make a type stronger than a string since we're touching it, but I guess there's not much we can do here.
DrahtBot cross-referenced this on Apr 5, 2020 from issue Serve BIP 157 compact filters by jimpo
MarcoFalke commented at 11:11 PM on April 5, 2020: member

Not sure how we go about refactoring like this, but I do like this change. ACK!

For purely stylistic matters, I think the rule is to not change the style unless you touch that line for other reasons. However, in this case, I think it is a problem of code (self) documentation. A command and message type are two completely different things and should not be confused with each other.

I was thinking maybe we should make a type stronger than a string since we're touching it, but I guess there's not much we can do here.

Indeed, string is the best we can do here. On the wire it is a string and practically you'll always have to do string comparison. Even if it is to convert the string to an enum (#10145).
promag commented at 11:54 PM on April 5, 2020: member

ACK, also update src/test/fuzz/process_message.cpp?

scripted-diff: Replace strCommand with msg_type

-BEGIN VERIFY SCRIPT-
sed -i 's/\<strCommand\>/msg_type/g' ./src/net_processing.cpp ./src/test/fuzz/process_message.cpp
-END VERIFY SCRIPT-

7777e3624f

MarcoFalke force-pushed on Apr 6, 2020
MarcoFalke commented at 12:11 AM on April 6, 2020: member

Addressed @promag's feedback
promag commented at 8:35 AM on April 6, 2020: member

ACK 7777e3624fabe4718675b2be8b088697b7ad4d0d.
naumenkogs commented at 12:59 PM on April 6, 2020: member

ACK 7777e36
practicalswift commented at 1:36 PM on April 6, 2020: contributor

ACK 7777e3624fabe4718675b2be8b088697b7ad4d0d -- I've always thought the strCommand name is confusing :)
theStack approved
theStack commented at 2:29 PM on April 6, 2020: contributor

ACK 7777e36

A command and message type are two completely different things and should not be confused with each other.

Concerning this point, a follow-up PR renaming also CNetMessage.m_command would make sense I guess?
MarcoFalke commented at 2:59 PM on April 6, 2020: member

Concerning this point, a follow-up PR renaming also CNetMessage.m_command would make sense I guess?

There is some active work going on in CNetMessage, so it can be done as part of one of those changes that touch that code anyway.
theStack commented at 3:21 PM on April 6, 2020: contributor
Another place where the "command" naming is still used is in the class CMessageHeader:
- char pchCommand[COMMAND_SIZE];
- std::string GetCommand() const;
- .....
MarcoFalke force-pushed on Apr 7, 2020
MarcoFalke force-pushed on Apr 7, 2020
MarcoFalke force-pushed on Apr 7, 2020
MarcoFalke force-pushed on Apr 7, 2020
DrahtBot cross-referenced this on Apr 7, 2020 from issue Serialization improvements step 6 (all except wallet/gui) by sipa
DrahtBot cross-referenced this on Apr 7, 2020 from issue Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli
MarcoFalke commented at 12:16 AM on April 8, 2020: member

Did all of the replacements suggested by @theStack in one scripted diff
MarcoFalke force-pushed on Apr 8, 2020
MarcoFalke commented at 3:50 PM on April 8, 2020: member

The full replacement commit was eeee135a9f517d65075f0f5b66f450e79368fbc4, but I've reverted this to the initial version, which was focussed on just net_processing and was less invasive and has 4 ACKs. Someone else can pick up the full replacement later.
MarcoFalke commented at 4:12 PM on April 8, 2020: member

I've also checked that on my system with clang and gcc on -O2 it produces the same binaries. So I will go ahead and merge this.
MarcoFalke merged this on Apr 8, 2020
MarcoFalke closed this on Apr 8, 2020
MarcoFalke deleted the branch on Apr 8, 2020
theStack cross-referenced this on Apr 9, 2020 from issue net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear') by theStack
theStack cross-referenced this on Apr 12, 2020 from issue scripted-diff: test: replace command with msgtype (naming) by theStack
sidhujag referenced this in commit 313b344e67 on Apr 13, 2020
MarcoFalke referenced this in commit d2882a012b on Apr 19, 2020
sidhujag referenced this in commit ab8d4f3851 on Apr 20, 2020
theStack cross-referenced this on May 10, 2020 from issue refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg by theStack
MarcoFalke referenced this in commit 62948caf44 on Jun 19, 2020
sidhujag referenced this in commit c47c194b1a on Jul 7, 2020
theStack cross-referenced this on Aug 6, 2020 from issue Per-Peer Message Capture by troygiorshev
jasonbcox referenced this in commit 8d54f58d4f on Nov 19, 2020
hebasto cross-referenced this on Jan 15, 2022 from issue net, refactor: Rename CNetMessage::m_command with CNetMessage::m_type by hebasto
MarcoFalke referenced this in commit 973c390298 on Jan 24, 2022
sidhujag referenced this in commit b135edda87 on Jan 28, 2022
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#10145 Separate Contextual checks and handling & switch on enum in net_processing.cpp #18544 net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear')#18610 scripted-diff: test: replace command with msgtype (naming)#18937 refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg #19509 Per-Peer Message Capture #24078 net, refactor: Rename CNetMessage::m_command with CNetMessage::m_type #31163 scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}#34203 doc: p2p: replace last remaining "command" terminology with "message type"

scripted-diff: Replace strCommand with msg_type #18533

Conflicts