Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framing refactor: framing.rs api #1033

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+198 −203
Draft

Framing refactor: framing.rs api #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
005e6c9
Constrain `T` and `B` generics
jbesraa Jul 4, 2024
e613a46
Change `payload` fn signature to return Option
jbesraa Jul 4, 2024
d0aae41
Change `Sv2Frame::get_header` to `header` and ..
jbesraa Jul 4, 2024
d1d9247
Change `Sv2Frame` from struct to enum
jbesraa Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions benches/benches/src/sv2/criterion_sv2_benchmark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,11 @@ fn client_sv2_setup_connection_serialize_deserialize(c: &mut Criterion) {
let mut dst = vec![0; size];
let _serialized = frame.serialize(&mut dst);
b.iter(|| {
let mut frame = StdFrame::from_bytes(black_box(dst.clone().into())).unwrap();
let type_ = frame.get_header().unwrap().msg_type().clone();
let payload = frame.payload();
let _ = AnyMessage::try_from((type_, payload)).unwrap();
if let Ok(mut frame) = StdFrame::from_bytes(black_box(dst.clone().into())) {
let msg_type = frame.header().msg_type().clone();
let payload = frame.payload().unwrap();
let _ = AnyMessage::try_from((msg_type, payload)).unwrap();
}
});
});
}
Expand Down Expand Up @@ -94,10 +95,11 @@ fn client_sv2_open_channel_serialize_deserialize(c: &mut Criterion) {
let mut dst = vec![0; size];
frame.serialize(&mut dst);
b.iter(|| {
let mut frame = StdFrame::from_bytes(black_box(dst.clone().into())).unwrap();
let type_ = frame.get_header().unwrap().msg_type().clone();
let payload = frame.payload();
black_box(AnyMessage::try_from((type_, payload)).unwrap());
if let Ok(mut frame) = StdFrame::from_bytes(black_box(dst.clone().into())) {
let msg_type = frame.header().msg_type().clone();
let payload = frame.payload().unwrap();
black_box(AnyMessage::try_from((msg_type, payload)).unwrap());
}
});
});
}
Expand Down Expand Up @@ -150,10 +152,11 @@ fn client_sv2_mining_message_submit_standard_serialize_deserialize(c: &mut Crite
"client_sv2_mining_message_submit_standard_serialize_deserialize",
|b| {
b.iter(|| {
let mut frame = StdFrame::from_bytes(black_box(dst.clone().into())).unwrap();
let type_ = frame.get_header().unwrap().msg_type().clone();
let payload = frame.payload();
black_box(AnyMessage::try_from((type_, payload)).unwrap());
if let Ok(mut frame) = StdFrame::from_bytes(black_box(dst.clone().into())) {
let msg_type = frame.header().msg_type().clone();
let payload = frame.payload().unwrap();
black_box(AnyMessage::try_from((msg_type, payload)).unwrap());
}
});
},
);
Expand Down
12 changes: 6 additions & 6 deletions benches/benches/src/sv2/iai_sv2_benchmark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ fn client_sv2_setup_connection_serialize_deserialize() {
let mut dst = vec![0; size];
frame.serialize(&mut dst);
let mut frame = StdFrame::from_bytes(black_box(dst.clone().into())).unwrap();
let type_ = frame.get_header().unwrap().msg_type().clone();
let payload = frame.payload();
let type_ = frame.header().msg_type().clone();
let payload = frame.payload().unwrap();
black_box(AnyMessage::try_from((type_, payload)));
}

Expand Down Expand Up @@ -77,8 +77,8 @@ fn client_sv2_open_channel_serialize_deserialize() {
let mut dst = vec![0; size];
frame.serialize(&mut dst);
let mut frame = StdFrame::from_bytes(black_box(dst.clone().into())).unwrap();
let type_ = frame.get_header().unwrap().msg_type().clone();
let payload = frame.payload();
let type_ = frame.header().msg_type().clone();
let payload = frame.payload().unwrap();
black_box(AnyMessage::try_from((type_, payload)));
}

Expand Down Expand Up @@ -127,8 +127,8 @@ fn client_sv2_mining_message_submit_standard_serialize_deserialize() {
let mut dst = vec![0; size];
frame.serialize(&mut dst);
let mut frame = StdFrame::from_bytes(black_box(dst.clone().into())).unwrap();
let type_ = frame.get_header().unwrap().msg_type().clone();
let payload = frame.payload();
let type_ = frame.header().msg_type().clone();
let payload = frame.payload().unwrap();
black_box(AnyMessage::try_from((type_, payload)));
}

Expand Down
4 changes: 2 additions & 2 deletions examples/interop-cpp/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ mod main_ {
let buffer = decoder.writable();
stream.read_exact(buffer).unwrap();
if let Ok(mut f) = decoder.next_frame() {
let msg_type = f.get_header().unwrap().msg_type();
let payload = f.payload();
let msg_type = f.header().msg_type();
let payload = f.payload().unwrap();
let message: Sv2Message = (msg_type, payload).try_into().unwrap();
match message {
Sv2Message::SetupConnection(_) => panic!(),
Expand Down
4 changes: 2 additions & 2 deletions examples/ping-pong-with-noise/src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl Node {
) -> Message<'static> {
match self.expected {
Expected::Ping => {
let ping: Result<Ping, _> = from_bytes(frame.payload());
let ping: Result<Ping, _> = from_bytes(frame.payload().unwrap());
match ping {
Ok(ping) => {
println!("Node {} received:", self.name);
Expand All @@ -118,7 +118,7 @@ impl Node {
}
}
Expected::Pong => {
let pong: Result<Pong, _> = from_bytes(frame.payload());
let pong: Result<Pong, _> = from_bytes(frame.payload().unwrap());
match pong {
Ok(pong) => {
println!("Node {} received:", self.name);
Expand Down
4 changes: 2 additions & 2 deletions examples/ping-pong-without-noise/src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl Node {
) -> Message<'static> {
match self.expected {
Expected::Ping => {
let ping: Result<Ping, _> = from_bytes(frame.payload());
let ping: Result<Ping, _> = from_bytes(frame.payload().unwrap());
match ping {
Ok(ping) => {
println!("Node {} received:", self.name);
Expand All @@ -107,7 +107,7 @@ impl Node {
}
}
Expected::Pong => {
let pong: Result<Pong, _> = from_bytes(frame.payload());
let pong: Result<Pong, _> = from_bytes(frame.payload().unwrap());
match pong {
Ok(pong) => {
println!("Node {} received:", self.name);
Expand Down
113 changes: 56 additions & 57 deletions protocols/v2/framing-sv2/src/framing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ type Slice = buffer_sv2::Slice;
/// A wrapper to be used in a context we need a generic reference to a frame
/// but it doesn't matter which kind of frame it is (`Sv2Frame` or `HandShakeFrame`)
#[derive(Debug)]
pub enum Frame<T, B> {
pub enum Frame<T, B>
where
T: Serialize + GetSize,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best practice is to constrain generics only where you need it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the impl block where you have function that need the generics to be constrained

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having naked generics makes it really hard to read the enum

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. Why is harder?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you want to use InnerType and Buffer instead of T and B ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously it was only Frame<T,B>{ HandShake(HandShakeFrame), Sv2(Sv2Frame<T,B>) without any info about T and B

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont feel very strongly about it, but I would appreciate reviewing the full PR (:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went trough the code everything look very good to me.

Copy link
Collaborator

@plebhash plebhash Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the outcome here?

@Fi3 do you still feel need we shouldn't be constraining generics here? or looking at the rest of the PR change your mind?

should we drop this commit from the PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constrain generics where not needed is bad, so my opinion is the same. If you are concerned about readability you can use Frame<InnerType,Buffer> instead of Frame<T,B>

B: AsMut<[u8]> + AsRef<[u8]>,
{
HandShake(HandShakeFrame),
Sv2(Sv2Frame<T, B>),
}
Expand All @@ -26,25 +30,23 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Frame<T, B> {
}
}

impl<T, B> From<HandShakeFrame> for Frame<T, B> {
impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> From<HandShakeFrame> for Frame<T, B> {
fn from(v: HandShakeFrame) -> Self {
Self::HandShake(v)
}
}

impl<T, B> From<Sv2Frame<T, B>> for Frame<T, B> {
impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> From<Sv2Frame<T, B>> for Frame<T, B> {
fn from(v: Sv2Frame<T, B>) -> Self {
Self::Sv2(v)
}
}

/// Abstraction for a SV2 Frame.
Copy link
Collaborator

@plebhash plebhash Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should expand this comment to make it clear what each enum variant is

maybe something like:

/// Both `Payload` and `Raw` variants carry the `Header` as metadata
/// `Payload` means the `Sv2Frame` carries a payload, but it has not yet been serialized
/// `Raw` means the `Sv2Frame` has been fully serialized, where the `serialized` field contains both `header` + `payload`

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

#[derive(Debug, Clone)]
pub struct Sv2Frame<T, B> {
header: Header,
payload: Option<T>,
/// Serialized header + payload
serialized: Option<B>,
pub enum Sv2Frame<T, B> {
Payload { header: Header, payload: T },
Raw { header: Header, serialized: B },
}

impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
Expand All @@ -53,23 +55,23 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
/// When called on a non serialized frame, it is not so cheap (because it serializes it).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

#[inline]
pub fn serialize(self, dst: &mut [u8]) -> Result<(), Error> {
if let Some(mut serialized) = self.serialized {
dst.swap_with_slice(serialized.as_mut());
Ok(())
} else if let Some(payload) = self.payload {
#[cfg(not(feature = "with_serde"))]
to_writer(self.header, dst).map_err(Error::BinarySv2Error)?;
#[cfg(not(feature = "with_serde"))]
to_writer(payload, &mut dst[Header::SIZE..]).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&self.header, dst.as_mut()).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&payload, &mut dst.as_mut()[Header::SIZE..])
.map_err(Error::BinarySv2Error)?;
Ok(())
} else {
// Sv2Frame always has a payload or a serialized payload
panic!("Impossible state")
match self {
Sv2Frame::Raw { mut serialized, .. } => {
dst.swap_with_slice(serialized.as_mut());
Ok(())
}
Sv2Frame::Payload { header, payload } => {
#[cfg(not(feature = "with_serde"))]
to_writer(header, dst).map_err(Error::BinarySv2Error)?;
#[cfg(not(feature = "with_serde"))]
to_writer(payload, &mut dst[Header::SIZE..]).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&header, dst.as_mut()).map_err(Error::BinarySv2Error)?;
#[cfg(feature = "with_serde")]
to_writer(&payload, &mut dst.as_mut()[Header::SIZE..])
.map_err(Error::BinarySv2Error)?;
Ok(())
}
}
}

Expand All @@ -78,18 +80,19 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
/// This function is only intended as a fast way to get a reference to an
/// already serialized payload. If the frame has not yet been
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

/// serialized, this function should never be used (it will panic).
pub fn payload(&mut self) -> &mut [u8] {
if let Some(serialized) = self.serialized.as_mut() {
&mut serialized.as_mut()[Header::SIZE..]
} else {
// panic here is the expected behaviour
panic!("Sv2Frame is not yet serialized.")
pub fn payload(&mut self) -> Option<&mut [u8]> {
Copy link
Collaborator

@plebhash plebhash Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we rename this method?

with the new enum we are sort of establishing a convention where the Payload variant signals that Sv2Frame has not yet been serialized

so calling a method that is named payload for a Sv2Frame::Payload and getting a None is extremely counter intuitive!

I feel we would create a more sane user experience if this method was called raw or serialized

match self {
Sv2Frame::Raw { serialized, .. } => Some(&mut serialized.as_mut()[Header::SIZE..]),
Sv2Frame::Payload { .. } => None,
}
}

/// `Sv2Frame` always returns `Some(self.header)`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

pub fn get_header(&self) -> Option<crate::header::Header> {
Some(self.header)
pub fn header(&self) -> crate::header::Header {
match self {
Self::Payload { header, .. } => *header,
Self::Raw { header, .. } => *header,
}
}

/// Tries to build a `Sv2Frame` from raw bytes, assuming they represent a serialized `Sv2Frame` frame (`Self.serialized`).
Expand All @@ -110,10 +113,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
pub fn from_bytes_unchecked(mut bytes: B) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github doesn't allow me to add this comment on the lines above. Ideally this comment should be placed on line 98, but placing it here should be sufficient

the comments on from_bytes are outdated with regards to the new enum-based Sv2Frame

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

// Unchecked function caller is supposed to already know that the passed bytes are valid
let header = Header::from_bytes(bytes.as_mut()).expect("Invalid header");
Self {
Self::Raw {
header,
payload: None,
serialized: Some(bytes),
serialized: bytes,
}
}

Expand Down Expand Up @@ -147,13 +149,9 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
/// otherwise, returns the length of `self.payload`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update these comments so that they are coherent with the new behavior of this function

updating the comments is especially important given that these are used on the Rust Docs, so they need to follow these changes accordingly

#[inline]
pub fn encoded_length(&self) -> usize {
if let Some(serialized) = self.serialized.as_ref() {
serialized.as_ref().len()
} else if let Some(payload) = self.payload.as_ref() {
payload.get_size() + Header::SIZE
} else {
// Sv2Frame always has a payload or a serialized payload
panic!("Impossible state")
match self {
Sv2Frame::Raw { serialized, .. } => serialized.as_ref().len(),
Sv2Frame::Payload { payload, .. } => payload.get_size() + Header::SIZE,
}
}

Expand All @@ -167,30 +165,31 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
) -> Option<Self> {
let extension_type = update_extension_type(extension_type, channel_msg);
let len = message.get_size() as u32;
Header::from_len(len, message_type, extension_type).map(|header| Self {
Header::from_len(len, message_type, extension_type).map(|header| Self::Payload {
header,
payload: Some(message),
serialized: None,
payload: message,
})
}
}

impl<A, B> Sv2Frame<A, B> {
impl<A: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<A, B> {
/// Maps a `Sv2Frame<A, B>` to `Sv2Frame<C, B>` by applying `fun`,
/// which is assumed to be a closure that converts `A` to `C`
pub fn map<C>(self, fun: fn(A) -> C) -> Sv2Frame<C, B> {
let serialized = self.serialized;
let header = self.header;
let payload = self.payload.map(fun);
Sv2Frame {
header,
payload,
serialized,
pub fn map<C>(self, fun: fn(A) -> C) -> Sv2Frame<C, B>
where
C: Serialize + GetSize,
{
match self {
Sv2Frame::Raw { header, serialized } => Sv2Frame::Raw { header, serialized },
Sv2Frame::Payload { header, payload } => Sv2Frame::Payload {
header,
payload: fun(payload),
},
}
}
}

impl<T, B> TryFrom<Frame<T, B>> for Sv2Frame<T, B> {
impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> TryFrom<Frame<T, B>> for Sv2Frame<T, B> {
type Error = Error;

fn try_from(v: Frame<T, B>) -> Result<Self, Error> {
Expand Down Expand Up @@ -232,7 +231,7 @@ impl HandShakeFrame {
}
}

impl<T, B> TryFrom<Frame<T, B>> for HandShakeFrame {
impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> TryFrom<Frame<T, B>> for HandShakeFrame {
type Error = Error;

fn try_from(v: Frame<T, B>) -> Result<Self, Error> {
Expand Down
Loading
Loading