Questions about framing_sv2 crate

[plebhash]

I'm working on the documentation effort described on #845

I asked many questions to @Fi3 as DMs over Discord. But I thought I should transfer those Q&A over here so they can be also used as future reference to the community.

[plebhash]

Could you provide a 1 sentence description for the following features:

• with_serde
• with_buffer_pool

[plebhash]

answer by @Fi3 :

with_serde use binary_sv2 with serde
with_buffer_pool use a more efficient method to allocate instead of a vec to dig into it you should go in utils/buffer
in buffer/utils readme is explained if you are curios

[plebhash]

I'm quite confused by this:

/// A frame can be either
/// 1: Sv2Frame
/// 2: NoiseFrame
/// 3: HandashakeFrame
///
#[derive(Debug)]
pub enum EitherFrame<T, B> {
    HandShake(HandShakeFrame),
    Sv2(Sv2Frame<T, B>),
}

HandShakeFrame is just an alias for NoiseFrame, so why does this comment list 3 items?

[plebhash]

HandshakeFrame is an alias for NoiseFrame:

stratum/protocols/v2/framing-sv2/src/framing2.rs

Line 90 in 546ecbc

pub type HandShakeFrame = NoiseFrame;

btw this line is still marked as /// todo on my WIP because I'm struggling to understand the purpose of this alias too... but that is probably related to my lack of knowledge about Noise protocol... I'm watching some YouTube videos about Noise today to improve that

[plebhash]

answer by @Fi3 :

ok leave it as todo and then when I will be back in florence i can check if we have to remove it or no
can you send me where it is used? If I remeber correctly is to again for performance reason when you want to change the id but everything else is the same? (if that is the case is not the safest or best interface) Or maybe is there to make the compiler happy in some weird cases?

[plebhash]

there's some instances of usage of HandshakeFrame on codec_sv2:

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

  Line 49 in 546ecbc

  pub fn step_0(&mut self) -> core::result::Result<HandShakeFrame, Error> {

• stratum/protocols/v2/codec-sv2/src/encoder.rs

  Line 105 in 546ecbc

  let i: HandShakeFrame = item.try_into().map_err(|e| {

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

  Line 155 in 546ecbc

  let frame = HandShakeFrame::from_bytes_unchecked(src.into());

and also network_helpers:

• stratum/roles/roles-utils/network-helpers/src/lib.rs

  Line 73 in 546ecbc

  let second_message: HandShakeFrame = second_message

[Fi3]

Right now Sv2Frame is used both for noise and not noise context and NoiseFrame aka HandShakeFrame is used only during the handshake. Before the changes to the noise part of the protocol we used NoiseFrame for the noise context and the handshake part and Sv2Frame for the not noise context.
Right now the only thing that NoiseFrame does is to return the inner bytes without changing it (it can just be a Vec)

Given that EitherFrame could be something like that:

#[derive(Debug)]
pub enum EitherFrame<T, B> {
    HandShake(Vec<u8>) // no need to use buffer_sv2::Slice here imo
    Sv2(Sv2Frame<T, B>),
}

The type Sv2Frame represent an unencrypted stratum v2 frame.

[plebhash]

OK thanks, I'll make sure the documentation for the current state of the crate provides some description of this.

But IMO even with docs, it will be quite confusing for the user, and framing_sv2 should be eventually refactored to make it easier to use and understand.

This is a future discussion, but I'm taking notes for a future refactoring.

Along with the total removal of Frame trait, I would propose that we:

• remove HandShakeFrame alias
• rename NoiseFrame to NoiseHandShakeFrame
• refactor APIs of NoiseHandShakeFrame to remove the impositions from Frame trait, making it more intuitive

[plebhash]

I'm trying to understand the implementation of Sv2Frame::from_bytes and Sv2Frame::from_bytes_unchecked.

Do we assume the bytes parameter is already serialized?

[plebhash]

answer by @Fi3 :

exactly bytes is already serialized you build the frame from bytes when you receive a raw header + payload from the network. This do not deserialize the bytes and just build a frame that have the correct bytes len given an header. We do lazy deserialization cause a valid use case for a proxy is to just relay a frame as it is and would be a waste of resource to deserialize and reserilaize it.
of course header need to be deserilized and the message to be decrypted so this thing are always done
btw deserilizing header is very fast as it is just a memcopy

[plebhash]

I understand most of what's happening on Sv2Frame::serialize.

But since this function is called serialize, shouldn't it be populating self.serialized on the else if let Some(payload) = self.payload branch?

If not here, where is self.serialized initialized?

[jbesraa]

@Fi3 Do you think it makes sense to change Sv2Frame to an enum?
Current version

/// Abstraction for a SV2 Frame.
#[derive(Debug, Clone)]
pub struct Sv2Frame<T, B> {
    header: Header,
    payload: Option<T>,
    /// Serialized header + payload
    serialized: Option<B>,
}

Enum version:

/// Abstraction for a SV2 Frame.
#[derive(Debug, Clone)]
pub enum Sv2Frame<T, B> {
    Raw(serialized: B),
    Frame {
        header: Header,
        payload: T,
    },
}

[Fi3]

I guess that is not an enum for performance reason but I don't remember (could be for other reasons also reasons that now do not exist anymore). I don't see why we need to change it since it work and is not public. If you want to try I can take a look at the PR, if you can put it in a separate PR or commit I will really appreciate it.

[jbesraa]

here you go jbesraa@29f5e44

I think its much more cleaner with enum. What performance advantages structs has over enums?

[Fi3]

it is just a guess in fact I say also could be for other reasons also reasons that now do not exist anymore

[Fi3]

and ofc is not enum vs struct is that in that particular case would have made sense to structure the code in that way. I want to be clear that I'm not saying that enum are in someway magically slower the struct.

[plebhash]

what is the purpose of fn build_noise_frame_header( ?

it is not used anywhere

[plebhash]

answer by @Fi3:

if is not used anywhere is likley something that we used and the we stop using and no one notice we can remove it in that case

[plebhash]

what is the purpose of Sv2Frame::map?

I see some use-cases inside roles (e.g.: this) but it seems weird that you would only modify payload whithout modifying serialized or header. If we modify the payload but serialized and header stay the same, wouldn't that break the frame? I mean, now payload represents some new information.

[plebhash]

answer by @Fi3 :

you never have both serialized and payload or one or the other. If you don't modify the len of the payload and the message type is the same you don't have to modify the header
btw yes is you have both serialized and payload you can not change just one
maybe we should at least check for it in the map and call it try_map? but before lets' understand the real reason fro why is there maybe we can remove it or change it with somthing else

[plebhash]

do you know the history behind this unreachable! macro?

stratum/protocols/v2/framing-sv2/src/framing2.rs

Lines 92 to 97 in 24b8b1d

	#[cfg(feature = "with_buffer_pool")]
	impl<A> From<EitherFrame<A, Vec<u8>>> for Sv2Frame<A, buffer_sv2::Slice> {
	fn from(_: EitherFrame<A, Vec<u8>>) -> Self {
	unreachable!()
	}
	}

I commented out this code block and built the roles workspace without issues. Maybe it could be removed?

[plebhash]

answer by @Fi3 :

if everything build with all feature (look at message generator) we can remove it

[plebhash]

I successfully executed message-generator-tests.sh after commenting out this code block.

I guess that's solid confirmation it can be removed

[Fi3]

yep is that is the case I guess that we can safely remove it

[plebhash]

the usage of generic B and the type alias Slice is a bit confusing... it seems both are aiming to achieve very similar goals but maybe in a redundant way

• Sv2Frame<T, B> has generic B to allow the user to decide which kind of buffer they want (which could be anything, as long as it satisfies the trait contraints).
• HandShakeFrame uses the Slice alias to choose between Vec<u8> vs buffer_sv2::Slice, and this choice is enacted via the with_buffer_pool

wouldn't it be better if we simply replaced B with Slice everywhere?

generic B feels a bit overkill, since we already have two options available

@Fi3

[plebhash]

or alternatively, if keeping generic B is highly desirable for some reason, we could also make HandShakeFrame use generic B instead of type alias Slice

it just feels wrong to mix the two approaches for Sv2Frame and HandShakeFrame... since both are flavors of enum Frame, the API should allow the user to choose the buffer strategy with the same mechanism (either generic B or feature flag with_buffer_pool, but not both)

[Fi3]

rust best practice is to constrains generics only where needed. For example:

// You do not constrain G cause you don't need it to be constrained
struct Something<G> {
    first: G
}

// Here you need to add it so you do it and you constrain to Addable that is a trait that I just made up
impl<G: Addable> Something<G> {
    fn double(&mut self) {
        *self.first += self.first
    }
}

The example above do not make sense but I want to use it explain what I mean.

Another best practice is to use explicative generics names. If Sv2Frame<T, B> is not clead you can use something like Sv2Frame<Sv2Message, Buffer> but to me it seems an overkill

[plebhash]

no but my main question is not really around generic constrains, that's secondary

the point I'm making is about using generic B/Buffer vs with_buffer_pool

it is confusing to have two different methods of choice in the same crate

I feel we should aim to have only one way that the user can choose which flavor of Buffer they prefer (either generic OR feature flag, ideally not both)

[Fi3]

in that case I would go with the feature flag only

[plebhash]

great, I also feel this is the best choice

cross referencing: #903 (comment)