Questions about codec_sv2 crate

[plebhash]

this discussion should guide the documentation of codec_sv2, as described on #845

[plebhash]

I'm looking at examples of how to use codec_sv2 without Noise. This seems to be a common pattern to decode frames:

stratum/examples/ping-pong-without-noise/src/node.rs

Lines 150 to 154 in 24b8b1d

	let mut decoder = StandardDecoder::new();
	loop {
	let writable = decoder.writable();
	reader.read_exact(writable).await.unwrap();

stratum/roles/roles-utils/network-helpers/src/plain_connection_tokio.rs

Lines 46 to 50 in 24b8b1d

	let mut decoder = StandardDecoder::<Message>::new();
	loop {
	let writable = decoder.writable();
	match reader.read_exact(writable).await {

---

This is how decoder is initialized:

stratum/protocols/v2/codec-sv2/src/decoder.rs

Lines 219 to 225 in 24b8b1d

	pub fn new() -> Self {
	Self {
	frame: PhantomData,
	missing_b: Header::SIZE,
	buffer: Buffer::new(2_usize.pow(16) * 5),
	}
	}

Which means the writeable buffer on the usage examples above always has Header::SIZE (6) bytes of size, because of this:

stratum/protocols/v2/codec-sv2/src/decoder.rs

Lines 213 to 215 in 24b8b1d

	pub fn writable(&mut self) -> &mut [u8] {
	self.buffer.get_writable(self.missing_b)
	}

As a consequence, when we call read_exact on the usage examples above, we can only fill the writable buffer with the header, not the entire frame.

---

So I'm actually having issues with decoding a frame on this little code snippet:

let message = Message::new();

let frame = StandardSv2Frame::<Message>::from_message(message, 0, 0, false)
    .unwrap();

let mut encoder = Encoder::<Message>::new();
let encoded_frame = encoder.encode(frame).unwrap();

// BufReader to emulate TcpStream
let mut buf_reader = std::io::BufReader::new(encoded_frame);

// initialize decoder
let mut decoder = StandardDecoder::<Message>::new();
let decoder_buf = decoder.writable();

// this prints 11
println!("{}", encoded_frame.len());

// this prints 6
println!("{}", decoder_buf.len());

// read_exact from buf_reader into decoder_buf
buf_reader.read_exact(decoder_buf).unwrap();

// get decoded frame
// fails with: "called `Result::unwrap()` on an `Err` value: MissingBytes(5)"
let decoded_frame = decoder.next_frame().unwrap();

It seems read_exact is a bad choice? What am I missing?

[Fi3]

decoder need to read the header first in order to know the len of the message so you have to call it 2 times. Look at how is done in network helpers

[rrybarczyk]

Error Variant Naming

Suggestions for codec_sv2::error::Error and ::CError refactor:

• Instead of importing at the top of the file:
  • Use noise_sv2 crate directly in the Error variants like AeadError(noise_sv2::AeadError) and NoiseSv2Error(noise_sv2::Error).
  • Use framing_sv2 crate directly in the Error variant like FramingSv2Error(framing_sv2::Error).
• There are two definitions of the framing_sv2::Error variant: FramingError and FramingSv2Error, remove one of these variant and handle any downstream effects.
• Rename (applies to both the codec_sv2::Error and codec_sv2::CError enums):
• AeadError -> NoiseSv2Aead or NoiseAead
• BinarySv2Error -> BinarySv2or Binary
• FramingSv2Error/FramingError -> FramingSv2 or Framing
• NoiseSv2Error -> Noise

[plebhash]

@Fi3 @jbesraa do you agree with these suggestions?

when we get consensus here, my suggestion is to create a new issue so the suggestions become an actionable item

we should also link this future issue under #845 (taking #903 as an example)

[rrybarczyk]

Also, can we alphabetize the error variants, the match arms of the implementations for the Error enums, and the order of the impl From<external-error> for Error?

I am doing the alphabetization in the Rust docs PR (#1040), but can undo if we don't like that or don't want to do that in that PR and save it for the refactor stage.

[jbesraa]

Ill have to look into the code a bit closer, but a few guidelines I have been following:

1. Each crate should expose a single Error (called Error). It can have multiple errors internally but pub only main errors enum that covers all the crate errors.
2. crates should not export other crates stuff, i.e. codec_sv2 shouldnt not export anything from framing, framing should be consumed who ever wants to use it.
3. when we import errors between crates we can use a longer name, for example: impl From<framing::Error> for Error
4. some of the code is redundant and needs to be deleted

[jbesraa]

@rrybarczyk your points makes sense, I would keep AeadError as is though.

[rrybarczyk]

Uniform Spelling

• We are using both HandShake and Handshake. Everything should be Handshake with a lowercase "s" (this is how it is defined in The Noise Protocol Framework) . This impacts:
  • codec_sv2:::
    • lib::State::HandShake -> lib::State::Handshake
    • error::Error::NotInHandShake -> error::Error::NotInHandshake
    • error::CError::NotInHandShake -> error::CError::NotInHandshake
  • framing_sv2::framing2:: (there may be more in framing_sv2, but these are what I have noticed so far):
    • HandShakeFrame -> HandshakeFrame
    • EitherFrame::HandShake -> EitherFrame::Handshake
• We have decoder::WithNoise::decode_noise_frame and then encoder::NoiseEncoder::encode. Would it make sense to make these methods uniform like decoder::WithNoise::decode and encoder::NoiseEncoder::encode?
• We have decoder::WithNoise and decoder::WithoutNoise. Then we have encoder::NoiseEncoder and encoder::Encoder. Should we make it decoder::WithNoise, decoder::WithoutNoise, encoder::WithNoise, and encoder::WithoutNoise? Or decoder::NoiseDecoder, decoder::Decoder, encoder::NoiseEncoder and encoder::Encoder?

[jbesraa]

yea I think we would need to remove some of those HandshakeFrame/EitherFrame/Frame .. too many (maybe slightly different) variants of the same thing

[rrybarczyk]

Imports

• Small thing, but in codec_sv2::lib, I am not a big fan of importing framing_sv2::framing2::handshake_message_to_frame as h2f. I think we should import framing_sv2::framing2 then use framing2::handshake_message_to_frame directly. The upside is it is clearer to the reader, the downside is it is more verbose.
• use core::cmp is imported in codec_sv2::error with the #[cfg(test)] declarator, but when running cargo test I get the following warning saying it is not used. Should it be removed?

warning: unused import: `core::cmp`
 --> v2/codec-sv2/src/error.rs:8:5
  |
8 | use core::cmp;
  |     ^^^^^^^^^

• In codec_sv2::decoder, some imports can be combined into one line. For example, use binary_sv2::{GetSize, Serialize}.

[rrybarczyk]

Use of Result types

The following methods use core::result::Result but should use codec_sv2::Result (unless there is a reason I am not seeing that requires using core::result::Result:

• State::step_0
• State::step_1
• State::step_2

[rrybarczyk]

Misc

• Should hard coded default buffer sizes be defined and constants?

[rrybarczyk]

Closing in favor of #1129.

[rrybarczyk]

Actually, will keep open. But please keep #1129 in sync.

[plebhash]

why are codec_sv2 and framing_sv2 separate crates, and not one single crate?

@Fi3

[Fi3]

no particular reason

[plebhash]

what is the motivation behind these type aliases?

• StandardEitherFrame<T>
• StandardSv2Frame<T>
• StandardNoiseDecoder<T>
• StandardDecoder<T>

it seems all they are doing is adding a Standard keyword, but it's not clear why.

another confusing aspect is why this is only applied to decoder.rs but not encoder.rs... why is there no StandardEncoder?

@Fi3

[plebhash]

I see your point, thanks for the clarification!

hiding the discussion to avoid spreading confusion

[plebhash]

@Fi3 still related to this, now I'm thinking:

doesn't it make sense to turn buffer_sv2 into a module inside codec_sv2?

these two crates are tightly coupled... the README of buffer_sv2 has multiple references to codec_sv2::Decoder and if I understand correctly there's no real usage of buffer_sv2 outside of codec_sv2

obviously, that would require a careful attention over many aspects, such as:

• keeping the with_buffer_pool feature flag inside codec_sv2 to allow the user to choose to enable it or not
• porting over the benches
• porting over the fuzzing

it just feels weird to have buffer_sv2 as a standalone crate that will never be used of anything outside of codec_sv2

this question came to my mind after @Shourya742 asked me why buffer_sv2 is not inside protocols

[Fi3]

nack buffer_sv2 can be used for anyhting that follow a specific pattern:

BufferPool is useful whenever we need to work with buffers sequentially (fill a buffer, get
the filled buffer, fill a new buffer, get the filled buffer, and so on).
To fill a buffer BufferPool returns an &mut [u8] with the requested len (the filling part).
When the buffer is filled and the owner needs to be changed, BufferPool returns a Slice that
implements Send and AsMut<u8> (the get part).
BufferPool pre-allocates a user-defined capacity in the heap and use it to allocate the buffers,
when a Slice is dropped BufferPool acknowledge it and reuse the freed space in the pre-allocated
memory.

This is in utils cause has nothing to do with sv2 and can be used with anything I put the sv2 in the name only cause it is used by SRI. Also porting it inside codec with fuzz and benches is a lot of work, will make codec_sv2 much harder to understand and it will introduce unsafe in codec_sv2.

[Fi3]

code_sv2 is mentioned several times cause the most likely user of the crate are SRI developer so it make sense to explain how it work with codec_sv2

[rrybarczyk]

@Fi3, can you help me understand the initialization of the missing_b member field in the unencrypted vs encrypted decoder?

The unencrypted decoder initializes missing_b as a constant Header::SIZE, which makes sense to me because we know for certain the initial number of missing bytes will at least be the header size, and in subsequent message chunk reads will likely expand to include a payload if it exists:

stratum/protocols/v2/codec-sv2/src/decoder.rs

Lines 219 to 225 in e65cb32

	pub fn new() -> Self {
	Self {
	frame: PhantomData,
	missing_b: Header::SIZE,
	buffer: Buffer::new(2_usize.pow(16) * 5),
	}
	}

What I do not understand is why the encrypted missing_noise_b is initialized as 0:

stratum/protocols/v2/codec-sv2/src/decoder.rs

Lines 168 to 174 in e65cb32

	pub fn new() -> Self {
	Self {
	frame: PhantomData,
	missing_noise_b: 0,
	noise_buffer: Buffer::new(2_usize.pow(16) * 5),
	sv2_buffer: Buffer::new(2_usize.pow(16) * 5),
	}

The header size of the noise protocol is constant, right? If so, why don't we initialize the encrypted missing_noise_b with a constant size like we do in the unencrypted initialization? Or, why don't we initialized both missing_b and missing_noise_b as 0?

[rrybarczyk]

• Why do we have WithoutNoise::default and WithNoise::default if they just call Self::new()?
• When should State::not_initialized be used?
• Do we reallly need to make this a public export in lib.rs:
  

  stratum/protocols/v2/codec-sv2/src/lib.rs

  Line 35 in e65cb32

  pub use framing_sv2::{self, framing::handshake_message_to_frame as h2f};