p2p: Move all header verification into the network layer, extend logging #19107

use nullopt?

thanks, fixed

It seems ReceiveMsgBytes returns two bool, (the function return and bool& complete), and both of them will have the same value at any execution (if I am not reading wrong). Is there any specific reason to make a function return the same boolean value twice?

There are a few cases where ReceiveMsgBytes will return true and complete will stay false. The most common is when Read finishes reading all of the bytes out of the buffer (so nBytes == 0), but we haven't reached the end of a single message. (Remember the socket buffer is max 64KiB but a bitcoin message can be up to 4MB). In that case complete will still be false, but the method will return true, as we should stay connected to the peer to keep receiving the rest of the message. Another case, added by this PR, is when a message is found to be invalid when deserialized (and when there isn't the end of a valid message in the buffer). Again complete will stay false, but the method will return true.

Ya that makes sense. Thanks for clarifying.

As it is a network magic error, the error code can be a little verbose for better understanding, something like this? Especially the command and size seems redundant.

        LogPrint(BCLog::NET, "HEADER ERROR - NETMAGIC : (%s, %u bytes), expected %s got %s\n", hdr.GetCommand(), hdr.nMessageSize + CMessageHeader::HEADER_SIZE chain_params.MessageStart(), hdr.GetMessageStart());

The command and size outputs are being kept to stay consistent with other error messages. Both the codebase and documentation aren't consistent between the terms "Message Start String" and "Network Magic", I'm preferring "Message Start String" here as I think it's the older one, and neither are sufficiently descriptive out of context anyways.

I don't think it's necessary to print out the expected MessageStart in the debug message. The MessageStart is set when bitcoind starts up and doesn't change after that. No one should be confused as to what the expected value is. Additionally I don't print out the message's MessageStart here, because I can't find a situation where it would be relevant. I'm open to changing my stance on that if someone can propose when that would be useful.

Should it be called HEADER ERROR given the error is due to the bigger message size? I think the error code can also be improved a little here specifying the message size is bigger the max permissible size. Something like

        LogPrint(BCLog::NET, "HEADER ERROR - MESSAGESIZE (%s, %u bytes), size %u bigger than max message size\n", hdr.GetCommand(), hdr.nMessageSize + CMessageHeader::HEADER_SIZE, hdr.nMessageSize);

The error is that the header is stating the message is larger than the maximum size. That's orthogonal to whether the message is actually larger than the maximum size. (a priori we have no way of determining that). The error is in the construction of the header, and the error is brought up when checking the header.

I don't think it's necessary to print out the current size, as it's already being done in the brackets. (That said, the error logs I've found aren't consistent between printing the "message size" and the "raw message size" (including the header). I've chosen the conservative approach and not changed that, though in this case I think you're right, it really should be the "message" or "payload" size.)

These new logs don't include the peer id, which makes them a lot less useful than the existing logs. Are you able to pass the failure reason up to CNode::ReceiveMsgBytes() so the peer id can be logged?

I've added a second LogPrint to ReceiveMsgBytes that will always print after an initial error. I think this is a better solution than having Read and Getmessage pass out a string. If we really want them on the same line, I could remove the trailing newline from the initial errors. (But that feels fragile)

This seems good enough. I think ideally Read and Getmessages would pass out an enum indicating deserialization failure reason, and ReceiveMsgBytes would know how to log based on the enum.

Definitely don't remove the newline from the initial errors! bitcoind is multithreaded, so another thread could log between the two messages.

I've found a way to consolidate these logs

A comment here specifying GetMessage can fail due to invalid header might help here.

Seems like an error message here might be appropriate similar to the checksum check?

The logging is done in IsValid and in ReadHeader. I agree, I'd love to have it in GetMessage instead, just like the checksum check, but that would require IsValid to pass the information about why it failed up to GetMessage. Ideally all of the logging would be in ReceiveMsgBytes (and indeed I had a version of this PR where that was the case) but seeing the complexity that added to Read, ReadHeader, IsValid, and GetHeader I didn't think it was worth it.

Since the only check in IsValid() is that the command name in the header is a valid ascii string, and the only place it's called is from here, perhaps that function should be moved to net.cpp and its name updated to IsCommandNameValid() or similar.

Chose IsCommandValid() as not to confuse the reader with the command name check done in ReceiveMsgBytes

Why remove this comment block?

Net Processing is no longer aware of the format of the header. That's all dealt with in net, so that net processing can just process the payload.

3a9d277 Is there somewhere suitable this documentation can be moved to rather than be tossed out?

A little more info in error message might help?

I don't see any more that we could add here.

Same comment for a little more info in the error message.

Same as above

This seems like a very interesting observation, especially for new reviewers. Is it possible to combine the explanation with the question below and elucidate it more clearly? It took me some time to figure out what these statements were referring to.

Good point. This PR is getting a little crowded, I'm going to move the test changes to a new one and I'll clear things up there.

This change to the CMessageHeader ctor should be pulled out into its own commit, with the commit message explaining why it's ok to initialize a CMessageHeader without the start bytes.

Done

No need for hdr() in the initializer list. It'll use the default construction anyway if you leave it out.

Done

in commit Give V1TransportDeserializer a ref to CChainParams member, and use it in GetMessage, you should switch out the calls to the global Params for the local chain_params

Missed this one, good eye. Done

Member variables in new code should be named m_name. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c for our style guide.

What's the reason for storing a reference to the chain params, rather than the message start bytes themselves, which is all we actually need?

I don't have a great justification for this. The chain_params is a singleton object and it somehow feels better to have a reference to a singleton object than a reference to a member of it. Maybe one day V1TransportDeserializer will need another parameter, if even only for a test. If anyone can propose even a weak reason why this is a bad idea than I'm happy to change it.

Done

I don't think commit Create getter CMessageHeader::GetMessageStart will build. You're changing the IsValid() signature in this commit, but don't change the callers until a later commit.

Fixed

Did you mean to commit all of this? It seems unfinished.

Test changes moved to a separate PR

This commit should be squashed with the code changes.

Done

I understand you are trying to repeat the errors here after the initial error from the actual function call, but can this make the errors clumsy? Especially when errors will occur from multiple peers simultaneously. It's expected that an entire error will be in a single line for any context, instead here the error with peer id is printing on a new line.

I agree. I've found a way to consolidate these logs, let me know what you think

Indication of checksum check already happening inside GetMessage() in this comment might be helpful?

Nit: Mentioning it "optionally" decomposes the message might make the comment complete. Feel free to ignore.

Referring back to the review club discussion. Previously we were dropping connections here because of any header error. Going by the logic of not dropping the connection due to wrong checksum (naughty firewall) cant those same things happen in other parts of the header too? Orthogonal to this PR, but curious about the rationale here.

This PR doesn't change behaviour at all, so I think questions about whether or not to drop connections for different errors is off-topic and should be saved for PRs that change behaviour!

Nit: Keeping in the spirit of the original comment here, maybe add a comment saying returning staring string?

The diff algorithm is confused, there wasn't ever a comment here. This check is now in ReadHeader, where I've preserved the comment.

no need for this use-once local variable. Combine the lines:

    if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
        // Invalid checksum. Log and exit
        ...

Done

It's a slight smell that you're calling Reset() for both exit branches here. I think this would be safer if this branch just set msg to be a nullopt and an else branch set it to be the Optional<CNetMessage>.

I'd also prefer to avoid the implicit conversion from CNetMessage to Optional<CNetMessage> in the return statement below.

Consolidated all of the checks to the bottom

in commit 5ad7ce1:

I still see Reset in both exit branches. Has this been addressed?

I think a comment here to say "Message deserialization failed. Drop the message and log the failure, but don't disconnect the peer" would be useful

Done

You can remove this include from net.cpp now that it's included in the header file.

Good catch, done

nit: space between ) and { please.

Since you're touching this line, can you also give the surrounding for loop braces to comply with current code style guide?

Gladly! done

nit: space between ) and { please.

Is it possible to move this logging up into V1TransportDeserializer::GetMessage() so you don't need to duplicate it?

I agree, done. Logging is now exclusively in GetMessage and ReadHeader

            if (result) {

            if (!result) {

        if (handled < 0) {

I think something has gone wrong here

oof, thanks

@jonatack the correct fix here is below but I'll hold off on touching anything until you're ready.

    V1TransportDeserializer(const CChainParams& chain_params_in, const NodeId node_id_in, int nTypeIn, int nVersionIn) : m_chain_params(chain_params_in), node_id(node_id_in), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn)
    {

thanks @troygiorshev

e0d316d nit: s/Set True/True|Set to true/

Could squash this commit into one of the others.

1596652 style nit, feel free to ignore

            Optional<CNetMessage> result{m_deserializer->GetMessage(nTimeMicros)};

3a9d277f All this reformatting of the section seems ok but I'm not sure if it's a good idea to change the formatting of critical code that's not being directly changed.

This is done in response to #19107 (review)

3a9d277

        with self.nodes[0].assert_debug_log(['HEADER ERROR - SIZE (badmsg, 4000001 bytes)']):

3a9d277

        with self.nodes[0].assert_debug_log(['HEADER ERROR - MESSAGESTART (badmsg, 2 bytes), got ffffffff']):

3a9d277

        with self.nodes[0].assert_debug_log(['HEADER ERROR - COMMAND (bad, 2 bytes)']):

I'm going to leave this one. The reason the command is wrong is not because it's an unrecognized command, it's because it isn't validly constructed. It ends up being bad\x00sg\x00\x00\x00\x00\x00\x00 which doesn't pass the "once you have a \x00 all remaining chars must be \x00" test. One day it might be nice to change all of these tests so that they send recognized commands, and then check for that separately.

style nit, feel free to ignore

            CNetMessage msg{*result};

5ad7ce1 and 3a9d277 I agree with @jnewbery here about the multiple Resets. Maybe something like this would be cleaner (tested example):

-    // Check checksum
-    // Always reset the network deserializer (prepare for the next message)
+    Optional<CNetMessage> result{};
     if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
         LogPrint(BCLog::NET, "CHECKSUM ERROR ... ;
-        Reset();
-        return nullopt;
-    }
-    // Check header command string
-    if (!hdr.IsCommandValid()) {
+    } else if (!hdr.IsCommandValid()) {
         LogPrint(BCLog::NET, "HEADER ERROR - COMMAND ... ;
-        Reset();
-        return nullopt;
+    } else {
+        result = msg;
     }
-
+    // Reset the network deserializer (prepare for the next message).
     Reset();
-    return msg;
+    return result;

Thanks for the tested example! I'm looking deeper into how Optional is implemented. It looks like we don't have move semantics until before Boost 1.56.0. I'm worried about the performance hit if we're copying a CNetMessage every time.

If I don't find anything important there, then I'll use this suggestion for sure.

IIRC return result; should always move if it is possible. All recent OS as well as the binaries on the website come with a version of boost greater than 1.56. I think we don't care about reduced performance when someone compiles on a historic OS.

See also commit ff9c671b11d40e5d0623eff3dd12e48cbaafb34e

Sounds good to me!

        LogPrint(BCLog::NET, "HEADER ERROR - MESSAGESTART (%s, %u bytes), received %s, peer=%d\n", hdr.GetCommand(), hdr.nMessageSize, HexStr(hdr.GetMessageStart()), node_id);

in commit 5ad7ce11027536fb6e48098db9659744514a7fc2:

Why is this copy and alias of the message needed?

• msg. occurs only twice, so I think simply replacing msg. with msg-> should be preferable to copying the whole message (or even adding a convenience alias (reference) to it).

In my mind this was the safe way to use an optional (immediately check the optional and resolve it to a real object). In this case though I've checked and what you've suggested both works and looks clear, so I'll go with it.

Since msg is not an rvalue, I think this will always do a copy assignment. Have you tried:

result = std::move(msg)

?

Tested and I can confirm that you're right on both accounts. Additionally, since std::is_move_constructible<CNetMessage>::value == true we can be reasonably sure that the Optional is being move constructed (from this boost doc)

Furthermore, by checking msg.m_recv.data() and result->m_recv.data() before the assignment, after the assignment, and after the method return, we can be absolutely sure that it's move constructing.

(and indeed I have, and indeed it is!)

nit: name m_node_id to comply with style guidelines

One day I'll remember, thanks!

Alternatively, rather than having both a CNetMessage and Optional<CNetMessage>:

diff --git a/src/net.cpp b/src/net.cpp
index c23cf54c75..7cc5068a75 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -726,14 +726,13 @@ const uint256& V1TransportDeserializer::GetMessageHash() const
 Optional<CNetMessage> V1TransportDeserializer::GetMessage(int64_t time)
 {
     // decompose a single CNetMessage from the TransportDeserializer
-    CNetMessage msg(std::move(vRecv));
-    Optional<CNetMessage> result{};
+    Optional<CNetMessage> msg(std::move(vRecv));
 
     // store command string, time, and sizes
-    msg.m_command = hdr.GetCommand();
-    msg.m_time = time;
-    msg.m_message_size = hdr.nMessageSize;
-    msg.m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
+    msg->m_command = hdr.GetCommand();
+    msg->m_time = time;
+    msg->m_message_size = hdr.nMessageSize;
+    msg->m_raw_message_size = hdr.nMessageSize + CMessageHeader::HEADER_SIZE;
 
     uint256 hash = GetMessageHash();
 
@@ -743,20 +742,20 @@ Optional<CNetMessage> V1TransportDeserializer::GetMessage(int64_t time)
     // Check checksum and header command string
     if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0) {
         LogPrint(BCLog::NET, "CHECKSUM ERROR (%s, %u bytes), expected %s was %s, peer=%d\n",
-            SanitizeString(msg.m_command), msg.m_message_size,
+            SanitizeString(msg->m_command), msg->m_message_size,
             HexStr(hash.begin(), hash.begin() + CMessageHeader::CHECKSUM_SIZE),
             HexStr(hdr.pchChecksum, hdr.pchChecksum + CMessageHeader::CHECKSUM_SIZE),
             node_id);
+        msg = nullopt;
     } else if (!hdr.IsCommandValid()) {
         LogPrint(BCLog::NET, "HEADER ERROR - COMMAND (%s, %u bytes), peer=%d\n",
-            hdr.GetCommand(), msg.m_message_size, node_id);
-    } else {
-        result = std::move(msg);
+            hdr.GetCommand(), msg->m_message_size, node_id);
+        msg = nullopt;
     }
 
     // Always reset the network deserializer (prepare for the next message)
     Reset();
-    return result;
+    return msg;
 }

I'm happy with this pattern if everyone else is (@jonatack). It's the "reverse" of before. Now we're assuming success and have to set failure. I think this is clearer.

LGTM

LGTM

in commit cecb5e329a26455f44dde39a4ffac82c4869ce2a

It appears that the peer id is no longer logged for invalid checksums. Doesn't this make it harder to debug checksum issues because it is now impossible to determine the peer id?

Previous log:

[net] CHECKSUM ERROR (msg_type, baz bytes), expected foo was bar
[msghand] ProcessMessages(msg_type, baz bytes): CHECKSUM ERROR peer=id

I see in the last commit you add + const NodeId m_node_id; // Only for logging

Maybe this addition could be moved to the first commit? It will be slightly painful due to the conflicts that will need to be solved, but I think it will be worth it

Moving it to the beginning was well worth it. The peer id is now present in all of this logging (and everything is just a little cleaner).

nit in commit cecb5e329a26455f44dde39a4ffac82c4869ce2a

                const CNetMessage& msg = *result;

can be a reference to a avoid the copy

I've changed this so that it uses list initialization instead and then reaches inside the optional. Now it's consistent with the "real" code in net.cpp.

in commit 9d5ba871fcba17038236a660e231022d11b769bc:

I don't think this method cares where the data is from. It can be any pointer that points to initialized data.

 * [@param](/bitcoin-bitcoin/contributor/param/)[in]   pch         A pointer to the raw data

Side note: (in the future) this could probably be changed to a Span, so that the pointer and size are passed in the same argument for clarity

+1 to one day making this a span

in commit 9d5ba87:

 * [@param](/bitcoin-bitcoin/contributor/param/)[out]  complete    Set True if at least one message has been deserialized and is

Could make this a bit more verbose for clarity?

in commit 8a492c8aef

This is unused, no?

    V1TransportDeserializer(int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn)

Also, if you decide to change this, maybe this very long line could be split up into two (or more) lines:

E.g.

diff --git a/src/net.h b/src/net.h
index b461470f1f..ba2d2fad27 100644
--- a/src/net.h
+++ b/src/net.h
@@ -674,8 +674,11 @@ private:
     }
 
 public:
-
-    V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
+    V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn)
+        : hdrbuf(nTypeIn, nVersionIn),
+          hdr(pchMessageStartIn),
+          vRecv(nTypeIn, nVersionIn)
+    {
         Reset();
     }
 

or

diff --git a/src/net.h b/src/net.h
index b461470f1f..1d99f2608b 100644
--- a/src/net.h
+++ b/src/net.h
@@ -674,8 +674,9 @@ private:
     }
 
 public:
-
-    V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn) {
+    V1TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn)
+        : hdrbuf(nTypeIn, nVersionIn), hdr(pchMessageStartIn), vRecv(nTypeIn, nVersionIn)
+    {
         Reset();
     }
 

Good eye. Fixed and cleaned.

nit In commit ef99b3bfbca87ef727c34f9ffedf50139a223086

Why is this removed? I think every file needs to include everything it uses. Otherwise, removing it from the header file might break compilation in the cpp file.

in commit 3f5a7306e941bd9bcd50afeb0e0a9fce46f22fd1

It seems confusing to make the magic bytes variable length. Unlike the message type they are not padded with zeros. See also https://btcinformation.org/en/developer-reference#message-headers

What do you think about returning a span (or a raw byte vector if there are concerns about life-time)?

Thanks for bringing this up, I agree. I've removed this commit and changed the logging to use HexStr(hdr.pchMessageStart, hdr.pchMessageStart + CMessageHeader::MESSAGE_START_SIZE), like how the checksum is logged later in the file.

I think a span would be a great idea, and a future PR should continue the work that #19373 started. (Maybe once someone converts HexStr to use span, as suggested there)

in commit 2ac16b110b

Adding logging is great, but I don't think this counts as a refactor. Also, the tests seem to require some changes, so maybe change the "without changing behavior" to something like "add logging" in the pull request title?

I agree, this has grown and there is enough context in the PR description. Changed.

test-nit in commit 2616399cca2e9c1a4c03605241c7964d53b360bf

Could extend the test?

diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py
index d9a9ae5188..04d86d3bc5 100755
--- a/test/functional/p2p_invalid_messages.py
+++ b/test/functional/p2p_invalid_messages.py
@@ -93,7 +93,9 @@ class InvalidMessagesTest(BitcoinTestFramework):
     def test_checksum(self):
         self.log.info("Test message with invalid checksum logs an error")
         conn = self.nodes[0].add_p2p_connection(P2PDataStore())
-        with self.nodes[0].assert_debug_log(['CHECKSUM ERROR (badmsg, 2 bytes), expected 78df0a04 was ffffffff']):
+        log_msg = 'CHECKSUM ERROR (badmsg, 2 bytes), expected 78df0a04 was ffffffff, peer={}'.format(
+            self.nodes[0].getpeerinfo()[0]['id'])
+        with self.nodes[0].assert_debug_log([log_msg]):
             msg = conn.build_message(msg_unrecognized(str_data="d"))
             # Checksum is after start bytes (4B), message type (12B), len (4B)
             cut_len = 4 + 12 + 4

This could be done for all of the tests, and can mostly be done separate from the changes in this PR. I'll do this in another PR.

Additionally I'm not sure that we have consensus that assert_debug_log is really a good thing.

No need to use the public getter GetId() since this is a member function of CNode. You can just use id directly.

I agree this is better, but I'll leave it for now.

I suggest you use a new category NET_MESSAGE_COMMAND_CORRUPT here rather than overloading _OTHER.

I'm worried about how adding a new category will affect people who have scripts that parse getpeerinfo. They will either:

1. Crash (bad)
2. Ignore it (also bad, loses the "sum of messages in bytesrecv_per_msg" == "bytesrecv" invariant)
3. Treat it like "_OTHER" (same as what we have here)

As it is right now in the PR, the bytes received for corrupt messages have been moved from their respective command to _OTHER. This is definitely an accuracy improvement. I'm not sure that adding a new category is worth the scope creep.

I've implemented your suggested change at this branch.

We do occasionally add new message types (most recently in #18876), so a client should tolerate unrecognized message types (crashing/requiring a software update for the client every time we introduce a new message type would be bad).

I agree that this isn't a critical issue and could be changed in a follow-up if necessary. For now, _OTHER seems fine.

In commit "Move checksum check from net_processing to net" (5cecad30d9ce37ba500eb1e06743d213bb911379)

Minor:

mapRecvBytesPerMsgCmd.at(NET_MESSAGE_COMMAND_OTHER) += out_err;

Would be shorter and avoid undefined behavior if there's no COMMAND_OTHER entry.

In commit "Move checksum check from net_processing to net" (5cecad30d9ce37ba500eb1e06743d213bb911379)

jonatack commented on the out_err name and I agree it's not descriptive, and I think additionally it's awkward that caller can't just refer to one variable for the raw message size, but has use look at different variables (out_err or result->m_raw_message_size) depending on whether the checksum fails. I think a better API for the function would be:

-Optional<CNetMessage> V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, uint32_t& out_err)
+CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds time, bool& reject_message)

Switching to this would also make the commit much smaller because msg. could stay msg. everywhere and not have to change to msg-> and result->.

In commit "Give V1TransportDeserializer an m_node_id member" (90fc3ffa82e398a6c95dbaf5374b6a478f17ae88)

It would be helpful for this commit message to say "This commit doesn't change behavior except for logging". It's harder to know if commit is doing the right thing if commit message doesn't say what intended behavior change is.

In commit "Move checksum check from net_processing to net" (5cecad30d9ce37ba500eb1e06743d213bb911379)

I think commit message here should say to what extent new behavior is equivalent to old behavior, because just looking at the current message, it's not clear what behavior changes are intended, and how fPauseRecv and mapRecvBytesPerMsgCmd and nReceiveFloodSize are involved. This information is hidden away in review discussion:

#19107 (comment) #19107 (comment) #19107 (comment) #19107 (comment) #19107 (comment)

After reading the discussion, it's still not clear to me why this change doesn't make it easier for a peer to flood the connection with bad checksum messages that cause log spam but are otherwise ignored. Is the takeaway that this change makes it slightly easier to flood but not significantly easier? Or that there's some other flood protection? Or that the new and old behavior are equivalent some other way? This is the type of information that would be good to have in the commit message.

    m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));

Yup, I prefer this too. I don't think it's unclear enough to merit review effort changing it now though, I'll incorporate it into a follow-up.

IIUC, out_err isn't an error number like its name suggests, but rather the size in bytes of the message. Perhaps its naming could be clearer.

I named it this way because TransportDeserializer is such a general interface. It's possible that some other implementation of it will use out_err as some sort of magic number.

Right now it is only set when the method fails. If we're going to go down the route of renaming this to something like msg_size, then I would prefer to always set it.

fixed

This is one of several places where the out_err naming seems odd to me; out_err suggests an error number more than the message size in bytes.

fixed

Rather than just "size", the logged error message could state that the message is too large and state what is expected, like for the checksum error logging

     // reject messages larger than MAX_SIZE or MAX_PROTOCOL_MESSAGE_LENGTH
-    if (hdr.nMessageSize > MAX_SIZE || hdr.nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) {
-        LogPrint(BCLog::NET, "HEADER ERROR - SIZE (%s, %u bytes), peer=%d\n", hdr.GetCommand(), hdr.nMessageSize, m_node_id);
+    unsigned int max_size{std::max(MAX_SIZE, MAX_PROTOCOL_MESSAGE_LENGTH)};
+    if (hdr.nMessageSize > max_size) {
+        LogPrint(BCLog::NET, "HEADER ERROR - SIZE (%s, %u bytes, %u expected), peer=%d\n",
+                 hdr.GetCommand(), hdr.nMessageSize, max_size, m_node_id);
         return -1;
     }

I like this, I'll leave it for a follow-up PR.

        LogPrint(BCLog::NET, "HEADER ERROR - INVALID COMMAND (%s, %u bytes), peer=%d\n",

I'll do a run through and decide between "ERROR - X" and "ERROR - INVALID X" (and make consistent) in a follow-up PR

In commit "Change CMessageHeader Constructor" (878d829b5bff9db5140c9547400ec024a08c0105)

I guess this is the change explained by "This messagestart should always be replaced when deserializing an actual message header so that we can run checks on it." in the commit message.

Would suggest making that a little more explicit like "This messagestart is overwritten by data from incoming messages during deserialization, so there is no change in behavior when processing messages."

In commit "Remove header checks out of net_processing" (e72c5b2e27b6ac3a1303b133f551bf2afcff1c0d)

Again commit message should say what intended behavior change is because it's hard to verify this is doing the right thing without know what it's supposed to do. It seems like dropping fDisconnect = true would no longer cause disconnects but I see from #19107 (comment) that there will still be disconnects because ReceiveMsgBytes will disconnect after readHeader now returns -1.

Would suggest commit message saying explicitly there is no change in behavior except for logging, that invalid magic bytes cause disconnects earlier in ReceiveMsgBytes, and that invalid header messages are ignored earlier but don't make flooding easier for whatever reason they don't make flooding easier