HEADERS processing does it manually because there's a discrepancy between the serialize and deserialize formats -- it's serialized as a vector of cblocks with no transactions since sending the message predated the CBlockHeader structure, but is deserialized as essentially a vector of pairs of CBlockHeader and an ignored compactsize value.
The difference would be something like:
try {
locator.LimitedRead<MAX_LOCATOR_SZ>(stream);
stream >> hash_stop;
return true;
} catch (LimitedVectorExceededError& e) {
LogDebug(BCLog::NET, "%s locator size %u > %u, %s", msg_type, e.m_size, MAX_LOCATOR_SZ, node.DisconnectMsg());
node.fDisconnect = true;
return false;
}
vs
stream.ignore(4); // dummy version
size_t loc_sz = ReadCompactSize(stream);
if (loc_sz > MAX_LOCATOR_SZ) {
LogDebug(BCLog::NET, "%s locator size %u > %u, %s", msg_type, e.m_size, MAX_LOCATOR_SZ, node.DisconnectMsg());
node.fDisconnect = true;
return false;
}
locator.vHave.reserve(loc_sz);
while (loc_sz-- > 0) {
locator.vHave.emplace_back();
stream >> locator.vHave.back();
}
return true;
which seems worse to me. I don't think the "exception" overhead versus "normal control flow" is a loss here, but losing the encapsulation of dummy-version and vector deserialization is something of a loss.
Just using LIMITED_VECTOR directly in the CBlockLocator serialization function would be slightly simpler than introducing the LimitedRead function, making the non-exceptional path just try { stream >> locator >> hash_stop; return true; }. Would probably change fuzz test behaviour though.
