Correct checksum error message (and debug node id) #8962

rebroad commented at 3:56 PM on October 18, 2016: contributor

Order of variables was incorrect. Also nodeid was not included. Fixed.

MarcoFalke added the label Docs and Output on Oct 18, 2016

in src/main.cpp:None in 1751499a80 outdated

6288 | @@ -6289,10 +6289,11 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman)
6289 |          uint256 hash = Hash(vRecv.begin(), vRecv.begin() + nMessageSize);
6290 |          if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0)
6291 |          {
6292 | -            LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR expected %s was %s\n", __func__,
6293 | +            LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR expected=%s was=%s peer=%d\n", __func__,
6294 |                 SanitizeString(strCommand), nMessageSize,
6295 | +               HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE),

laanwj commented at 4:45 PM on October 18, 2016:

I don't think this change is correct. Expected is hash, the actual value is hdr.pchChecksum. Which are in the right order already, or am I missing something?

paveljanik commented at 7:42 PM on October 18, 2016:

No, you are correct of course. As hash is computed locally a few lines above, it is the expected value, thus the first one. And the current code is OK.

But 'was' is strange here (it was received). There is similar code a few lines above:

%x expected, %x received

laanwj commented at 9:25 PM on October 18, 2016:

Agreed.

Also it makes sense to move this to net debug category I think.

rebroad commented at 12:26 AM on October 19, 2016:

hdr.pchChecksum is the checksum we are told it will be once received, therefore this is the "expected" (as the real checksum has not been calculated yet) checksum. When we receive the message, the computed checksum is the actual checksum. Is this correct?

paveljanik commented at 5:05 AM on October 19, 2016:

This is the "was" I mentioned. The message was received with some declared checksum. When checking the checksum, we calculate the real checksum and expect it will be the same. It is not. Expected (calculated here, locally - hash) doesn't match the received one (declared by the sender, actual one in the message).

We do not expect the checksum received (provided by the peer) will match. We do not trust it, we check it.

rebroad commented at 5:40 AM on October 19, 2016:

@paveljanik I see what you are saying now. Ok, so "was" and "expected" are both ambiguous it seems. Perhaps "declared" and "calculated"?

paveljanik commented at 6:03 AM on October 19, 2016:

expected and received used in other places...

in src/main.cpp:None in 1751499a80 outdated

6293 | +            LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR expected=%s was=%s peer=%d\n", __func__,
6294 |                 SanitizeString(strCommand), nMessageSize,
6295 | +               HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE),
6296 |                 HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE),
6297 | -               HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE));
6298 | +	       pfrom->id);

paveljanik commented at 7:42 PM on October 18, 2016:

TAB here?

rebroad commented at 12:25 AM on October 19, 2016:

will fix. thanks.

Correct checksum error message (and debug node id) 270cce1d60

rebroad force-pushed on Oct 19, 2016

in src/main.cpp:None in 270cce1d60

6347 | @@ -6348,10 +6348,11 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman)
6348 |          uint256 hash = Hash(vRecv.begin(), vRecv.begin() + nMessageSize);
6349 |          if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0)
6350 |          {
6351 | -            LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR expected %s was %s\n", __func__,
6352 | +            LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR declared=%s calculated=%s peer=%d\n", __func__,

laanwj commented at 7:06 AM on October 19, 2016:

Still NACK on swapping them,

laanwj commented at 7:06 AM on October 19, 2016:

Still NACK on swapping them,

rebroad commented at 9:13 AM on October 19, 2016:

Why do you want them to be the wrong way around?

laanwj closed this on Oct 19, 2016

MarcoFalke locked this on Sep 8, 2021