Replication response size cap

[jadalsmail]

This PR introduces a size cap for the replication response messages so that avalanchego clients doesn't reject messages above the size limit.
Also added unit tests to test for the size limit.

[yacovm]

Instead of Bytes() we can just have an instance method Size for all these sub-message types (Notarization, EmptyNotarization, etc.) and we can just call the Size() and it will recursively compute the size.

We don't want to serialize the block to bytes just to know the size of the block.

[yacovm]

I don't think we should have this method return an error. Ideally we would be able to estimate the size of a message without an error.

If we don't marshal the fields to bytes we can remove the error returned.

[yacovm]

type ParsedBlock struct {
	metadata.StateMachineBlock
	msm *metadata.StateMachine
}

Shouldn't the Size() just be a method of StateMachineBlock ?

If StateMachineBlock implements Size() then since ParsedBlock embeds StateMachineBlock, we get it implicitly.

[jadalsmail]

But here Size() doesn't measure a stateMachineBlock, it measure the RawBlock encoding of one, which is defined in the root package, not in msm (where stateMachineBlock is).
We can't move the Size() method to the msm package, because it doesn't import canotoTag_RawBlock__InnerBlockBytes and canotoTag_RawBlock__Metadata, and msm can't import them because the root already imports msm. (see https://github.com/ava-labs/Simplex/blob/main/external.go#L10)

[yacovm]

but the raw block is just an msm reference which isn't serialized, and the block.

So the Size() of the block should be an instance method of the block, in my opinion.

We can't move the Size() method to the msm package, because it doesn't import canotoTag_RawBlock__InnerBlockBytes and canotoTag_RawBlock__Metadata,

I don't understand.

[jadalsmail]

ok, in order to do that I just moved the Bytes() and the Size() methods to msm/block.go and they are a method of StateMachineBlock, also moved the RawBlock type there as well. And updated the RawBlock calls to be metadata.RawBlock.

What do you think? Does this work?

[yacovm]

I don't really care where the RawBlock lives, that's fine moving it there.

However, I'm not entirely convinced that using the canoto size cache is the right way to go, it seems to me very ad-hoc and overly dependent on canoto.

I think maybe a better solution would be to pre-compute the size of the StateMachineMetadata by marshaling it once the RawBlock is instantiated, and then Size() can just return the cached version.

[yacovm]

This would mean that Size can return to be in RawBlock as initially.

[jadalsmail]

Ok now I understand. I removed the dependency on canoto entirely, now the size is precomputed when the block is first serialized in Bytes() and we return the cached version. we are serializing the whole block tho not only the StateMachineMetadata (because if we were only marshaling the StateMachineMetadata we would still need to combine with the inner block size and the canoto tags).

[yacovm]

This change technically works, but - do we really need the code to be like this?

The only reason the code works is because ParsedBlock embeds StateMachineBlock.

ParsedBlock is what implements VerifiedBlock, right? It implements all methods of VerifiedBlock.

If we're adding Size() to the interface of VerifiedBlock then it makes sense that ParsedBlock will implement Size() and not StateMachineBlock.

I think that size can be in ParsedBlock and Size() can be there too, and we can have the lazy caching of size under a lock that can be a field in ParsedBlock, so that we can be invoked Size() concurrently.

[yacovm]

I realize that I initially asked to move Size() from ParsedBlock to StateMachineBlock, sorry about that 🤦

However, the way the code was back then made it odd that we access the fields of StateMachineBlock in such a way:

(&p.Metadata).CalculateCanotoCache()
metadataSize := (&p.Metadata).CachedCanotoSize()

So I thought it would make sense to have the instance method touch its own fields.

However, now when I thought about it some more, I think it actually makes sense to have it in ParsedBlock and reduce the complexity of StateMachineBlock

[jadalsmail]

Ok that makes sense. Let me know what you think of it now.