scripted-diff: Replace strCommand with msg_type

[maflcko]

Receiving a message is not a command, but simply a message of some type

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Serialization improvements step 6 (all except wallet/gui) #18317 (Serialization improvements step 6 (all except wallet/gui) by sipa)
• Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
• Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479 (Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery)
• Serve BIP 157 compact filters #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.

[naumenkogs]

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.

[maflcko]

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]

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

[maflcko]

Addressed @promag's feedback

[promag]

ACK 7777e36.

[naumenkogs]

ACK 7777e36

[practicalswift]

ACK 7777e36 -- I've always thought the strCommand name is confusing :)

[theStack]

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?

[maflcko]

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]

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

• char pchCommand[COMMAND_SIZE];
• std::string GetCommand() const;
• .....

[maflcko]

Did all of the replacements suggested by @theStack in one scripted diff

[maflcko]

The full replacement commit was eeee135, 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.

[maflcko]

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.