scripted-diff: Replace strCommand with msg_type #18533

1. 
   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
2. 
   DrahtBot commented at 7:35 pm on April 5, 2020: member

   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.

3. DrahtBot added the label P2P on Apr 5, 2020
4. DrahtBot added the label Refactoring on Apr 5, 2020
5. 
   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.

6. 
   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).

7. 
   promag commented at 11:54 pm on April 5, 2020: member

   ACK, also update src/test/fuzz/process_message.cpp?
8. 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
9. MarcoFalke force-pushed on Apr 6, 2020
10. 
    MarcoFalke commented at 0:11 am on April 6, 2020: member

    Addressed @promag’s feedback
11. 
    promag commented at 8:35 am on April 6, 2020: member

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

    ACK 7777e36
13. 
    practicalswift commented at 1:36 pm on April 6, 2020: contributor

    ACK 7777e3624fabe4718675b2be8b088697b7ad4d0d – I’ve always thought the strCommand name is confusing :)
14. theStack approved
15. 
    theStack commented at 2:29 pm on April 6, 2020: member

    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?

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

17. 
    theStack commented at 3:21 pm on April 6, 2020: member

    Another place where the “command” naming is still used is in the class CMessageHeader:

    • char pchCommand[COMMAND_SIZE];
    • std::string GetCommand() const;
    • …..
18. MarcoFalke force-pushed on Apr 7, 2020
19. MarcoFalke force-pushed on Apr 7, 2020
20. MarcoFalke force-pushed on Apr 7, 2020
21. MarcoFalke force-pushed on Apr 7, 2020
22. 
    MarcoFalke commented at 0:16 am on April 8, 2020: member

    Did all of the replacements suggested by @theStack in one scripted diff
23. MarcoFalke force-pushed on Apr 8, 2020
24. 
    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.
25. 
    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.
26. MarcoFalke merged this on Apr 8, 2020
27. MarcoFalke closed this on Apr 8, 2020

    ---

28. MarcoFalke deleted the branch on Apr 8, 2020
29. sidhujag referenced this in commit 313b344e67 on Apr 13, 2020
30. MarcoFalke referenced this in commit d2882a012b on Apr 19, 2020
31. sidhujag referenced this in commit ab8d4f3851 on Apr 20, 2020
32. MarcoFalke referenced this in commit 62948caf44 on Jun 19, 2020
33. sidhujag referenced this in commit c47c194b1a on Jul 7, 2020
34. jasonbcox referenced this in commit 8d54f58d4f on Nov 19, 2020
35. MarcoFalke referenced this in commit 973c390298 on Jan 24, 2022
36. sidhujag referenced this in commit b135edda87 on Jan 28, 2022
37. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke DrahtBot naumenkogs promag practicalswift theStack

Labels
Refactoring P2P