In principle, the checksums of P2P packets are simply 4-byte blobs which are the first bytes of SHA256(SHA256(payload)).
Currently they are handled as little-endian 32-bit integers half of the time, as blobs the other half, sometimes copying the one to the other, resulting in somewhat confused code.
This PR changes the handling to be consistent both at packet creation and receiving, making it (I think) easier to understand.
utACK 0a1229617d647d8d288f2afa3006d6a6a5d942a1. This inconsistency has bothered me before.
In principle, the checksums of P2P packets are simply 4-byte blobs which
are the first four bytes of SHA256(SHA256(payload)).
Currently they are handled as little-endian 32-bit integers half of the
time, as blobs the other half, sometimes copying the one to the other,
resulting in somewhat confused code.
This PR changes the handling to be consistent both at packet creation
and receiving, making it (I think) easier to understand.
I've slightly changed the error message to no longer mention nChecksum, as the name of the field changed and is not relevant for troubleshooting anyway:
2016-09-28 10:42:19.666628 ProcessMessages(ping, 8 bytes): CHECKSUM ERROR expected 7ef0ca62 was 12234556
In some later pull we'd likely want to move all these messages to the net debug category. I'm also going to add a test later that will actually exercise the P2P network error logic.
utACK 41e58faf043864a64a6db08a8df527fa5fd1ec5b
Any reason to keep CHECKSUM_SIZE = sizeof(int) (protocol.h) instead of just CHECKSUM_SIZE = 4?
6252 | @@ -6253,11 +6253,12 @@ bool ProcessMessages(CNode* pfrom, CConnman& connman) 6253 | // Checksum 6254 | CDataStream& vRecv = msg.vRecv; 6255 | uint256 hash = Hash(vRecv.begin(), vRecv.begin() + nMessageSize); 6256 | - unsigned int nChecksum = ReadLE32((unsigned char*)&hash); 6257 | - if (nChecksum != hdr.nChecksum) 6258 | + if (memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) != 0)
A size check of hash.size() <= CMessageHeader::CHECKSUM_SIZE is probably a total overkill...
A compile-time assertion could be added... But I don't think there is any risk of someone increasing CHECKSUM_SIZE larger than 32 in practice.
Any reason to keep
CHECKSUM_SIZE = sizeof(int)(protocol.h) instead of justCHECKSUM_SIZE = 4?
Yes, I thought about that too, we know that the checksum is always four bytes and it shouldn't change based on what happens to be the architectures int type. WIll do.
Edit: Done. Also changed the SIZE_SIZE to be a fixed-size uint32_t the same go. We already use WriteLE32 at the packet-construction side.
The P2P network uses a fixed protocol, these sizes shouldn't change
based on what happens to be the architecture.
utACK 305087bdf640a5c0b3eeeb90d33ee255d2456eba
6260 | - LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR nChecksum=%08x hdr.nChecksum=%08x\n", __func__, 6261 | - SanitizeString(strCommand), nMessageSize, nChecksum, hdr.nChecksum); 6262 | + LogPrintf("%s(%s, %u bytes): CHECKSUM ERROR expected %s was %s\n", __func__, 6263 | + SanitizeString(strCommand), nMessageSize, 6264 | + HexStr(hash.begin(), hash.begin()+CMessageHeader::CHECKSUM_SIZE), 6265 | + HexStr(hdr.pchChecksum, hdr.pchChecksum+CMessageHeader::CHECKSUM_SIZE));
are these the wrong way around? i.e. the expected checksum is supposed to be displayed before the "was"?