Block response size check is not correct

[nazar-pc]

I was re-reviewing my PR paritytech/substrate#12146 and noticed that the size calculation (both before and after) is likely incorrect and can cause unexplainable errors.

Here estimation of total size accounts for some fields of block_data, but not others:

polkadot-sdk/substrate/client/network/sync/src/block_request_handler.rs

Lines 419 to 430 in 0400ed9

	let new_total_size = total_size +
	block_data.body.iter().map(|ex| ex.len()).sum::<usize>() +
	block_data.indexed_body.iter().map(|ex| ex.len()).sum::<usize>();
	// Send at least one block, but make sure to not exceed the limit.
	if !blocks.is_empty() && new_total_size > MAX_BODY_BYTES {
	break
	}
	total_size = new_total_size;
	blocks.push(block_data);

I'm not aware of what are the historical reasons for this, but if my understanding is correct, response will always be strictly bigger than total_size estimated. The message, annoyingly, is in protobuf format, so suspect there is no encoded_size method there, but I think it should push an entry into blocks, check encoded size and in case size is exceeded pop the value before breaking the loop.

[altonen]

Requests/responses are encoded using unsigned-varint which just prepends a length (at most 9 bytes) to the message. This message length tag is not counted in so the block response has all of the 16 MB to use.

It looks like right now the length is not checked for outbound messages which could cause "unexplainable errors" but I don't think exposing the details of the encoding should be exposed to the block request protocol and instead it should try to create the response within the limits that were provided when the protocol was initialized.

[nazar-pc]

If I understand correctly, we're encoding BlockData, while only checking the size of body and indexed_body fields, ignoring all others. This is where I think the problem is, every field and length for each vector should be accounted for.

[altonen]

Fair enough. I think all the information is available to BlockRequestHandler though so I don't think there is a need to expose any details about the implementation of the request-response protocol when crafting the response and the handler should just calculate the response size correctly based on the information it has.