thoughts on deserializing into an iterator (and not a simple vec)

[antiochp]

Currently we read a full vec of elements during deserialization.

https://github.com/mimblewimble/grin/blob/4d2cbe6596462e609a528182e57a965ca4d5aa8d/core/src/ser.rs#L452-L461

We use this when handling a Headers msg during header sync.

I think this could potentially be reworked (with a bit of work) to avoid reading the full vec in, returning an iterator instead.

Presumably we'd need to keep a ref to the underlying reader in the iterator.

But this would give us a ton of flexibility around exactly how/when we read the elements of the vec.

So we'd be able to do things like (code might look something like this) -

headers = msg.body_iter()?;
let chunks = headers.chunks(8);
while let Some(header_chunk) = chunks.next() {
    adapter.headers_received(header_chunk);
}

This would allow us to avoid the custom headers deserialization logic where we estimate header size in bytes etc. We'd be able to effectively deserialize them one at a time or several at a time then hand them off for further processing.

The actual header deserialization would be hidden behind the body_iter() (or something similar).

Thoughts @garyyu ?

[ignopeverell]

That would be nice as the underlying OS buffers would allow a nice optimal balance between CPU processing and data availability.

Along the same lines, and perhaps for more gains against DDoS, there's still a TODO to not need reading the whole block before validating its header.

[garyyu]

@antiochp great! I love it 😄

@ignopeverell

there's still a TODO to not need reading the whole block before validating its header.

propose to create an issue on this, to avoid forget. Perhaps just call a shutdown if header is already known or fail validation.

[antiochp]

An issue to track that TODO would be good. We should ban the peer and continue if the peer sends us an invalid header. Shutdown would be extreme (unless you just meant disconnect the peer connection).

You could just send bad headers to everyone and cause network wide havoc if peers shutdown in response.

[garyyu]

Shutdown would be extreme (unless you just meant disconnect the peer connection).

I mean the shutdown() function of TcpStream 😄 Yes, it means disconnect.

[garyyu]

@antiochp You plan to do this now or want me to try it?