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
Draft

Framing refactor: framing.rs api #1033

Show file tree
Hide file tree
Changes from 3 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
27 changes: 15 additions & 12 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,13 +30,13 @@ 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)
}
Expand Down Expand Up @@ -78,18 +82,17 @@ 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] {
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

if let Some(serialized) = self.serialized.as_mut() {
&mut serialized.as_mut()[Header::SIZE..]
Some(&mut serialized.as_mut()[Header::SIZE..])
} else {
// panic here is the expected behaviour
panic!("Sv2Frame is not yet serialized.")
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 {
self.header
}

/// Tries to build a `Sv2Frame` from raw bytes, assuming they represent a serialized `Sv2Frame` frame (`Self.serialized`).
Expand Down Expand Up @@ -175,7 +178,7 @@ impl<T: Serialize + GetSize, B: AsMut<[u8]> + AsRef<[u8]>> Sv2Frame<T, B> {
}
}

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> {
Expand All @@ -190,7 +193,7 @@ impl<A, B> Sv2Frame<A, B> {
}
}

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 +235,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
50 changes: 25 additions & 25 deletions protocols/v2/sv2-ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,11 +465,11 @@ pub extern "C" fn next_frame(decoder: *mut DecoderWrapper) -> CResult<CSv2Messag

match decoder.0.next_frame() {
Ok(mut f) => {
let msg_type = match f.get_header() {
Some(header) => header.msg_type(),
let msg_type = f.header().msg_type();
let payload = match f.payload() {
Some(payload) => payload,
None => return CResult::Err(Sv2Error::InvalidSv2Frame),
};
let payload = f.payload();
let len = payload.len();
let ptr = payload.as_mut_ptr();
let payload = unsafe { std::slice::from_raw_parts_mut(ptr, len) };
Expand Down Expand Up @@ -760,8 +760,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::CoinbaseOutputDataSize(m) => m,
Expand Down Expand Up @@ -812,8 +812,8 @@ mod tests {
let mut decoded = decoder.next_frame().unwrap();

// Extract payload of the frame which is the NewTemplate message
let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::NewTemplate(m) => m,
Expand Down Expand Up @@ -860,8 +860,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::RequestTransactionData(m) => m,
Expand Down Expand Up @@ -910,8 +910,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::RequestTransactionDataError(m) => m,
Expand Down Expand Up @@ -960,8 +960,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::RequestTransactionDataSuccess(m) => m,
Expand Down Expand Up @@ -1005,8 +1005,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::SetNewPrevHash(m) => m,
Expand Down Expand Up @@ -1050,8 +1050,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::SubmitSolution(m) => m,
Expand Down Expand Up @@ -1108,8 +1108,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::ChannelEndpointChanged(m) => m,
Expand Down Expand Up @@ -1144,8 +1144,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::SetupConnection(m) => m,
Expand Down Expand Up @@ -1193,8 +1193,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::SetupConnectionError(m) => m,
Expand Down Expand Up @@ -1242,8 +1242,8 @@ mod tests {

let mut decoded = decoder.next_frame().unwrap();

let msg_type = decoded.get_header().unwrap().msg_type();
let payload = decoded.payload();
let msg_type = decoded.header().msg_type();
let payload = decoded.payload().unwrap();
let decoded_message: Sv2Message = (msg_type, payload).try_into().unwrap();
let decoded_message = match decoded_message {
Sv2Message::SetupConnectionSuccess(m) => m,
Expand Down
8 changes: 4 additions & 4 deletions roles/jd-client/src/lib/downstream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ impl DownstreamMiningNode {

/// Parse the received message and relay it to the right upstream
pub async fn next(self_mutex: &Arc<Mutex<Self>>, mut incoming: StdFrame) {
let message_type = incoming.get_header().unwrap().msg_type();
let payload = incoming.payload();
let message_type = incoming.header().msg_type();
let payload = incoming.payload().unwrap();

let routing_logic = roles_logic_sv2::routing_logic::MiningRoutingLogic::None;

Expand Down Expand Up @@ -707,8 +707,8 @@ pub async fn listen_for_downstream_mining(
);

let mut incoming: StdFrame = node.receiver.recv().await.unwrap().try_into().unwrap();
let message_type = incoming.get_header().unwrap().msg_type();
let payload = incoming.payload();
let message_type = incoming.header().msg_type();
let payload = incoming.payload().unwrap();
let routing_logic = roles_logic_sv2::routing_logic::CommonRoutingLogic::None;
let node = Arc::new(Mutex::new(node));
if let Some(upstream) = upstream {
Expand Down
4 changes: 2 additions & 2 deletions roles/jd-client/src/lib/job_declarator/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,8 @@ impl JobDeclarator {
let receiver = self_mutex.safe_lock(|d| d.receiver.clone()).unwrap();
loop {
let mut incoming: StdFrame = receiver.recv().await.unwrap().try_into().unwrap();
let message_type = incoming.get_header().unwrap().msg_type();
let payload = incoming.payload();
let message_type = incoming.header().msg_type();
let payload = incoming.payload().unwrap();
let next_message_to_send =
ParseServerJobDeclarationMessages::handle_message_job_declaration(
self_mutex.clone(),
Expand Down
4 changes: 2 additions & 2 deletions roles/jd-client/src/lib/job_declarator/setup_connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ impl SetupConnectionHandler {

let mut incoming: StdFrame = receiver.recv().await.unwrap().try_into().unwrap();

let message_type = incoming.get_header().unwrap().msg_type();
let payload = incoming.payload();
let message_type = incoming.header().msg_type();
let payload = incoming.payload().unwrap();
ParseUpstreamCommonMessages::handle_message_common(
Arc::new(Mutex::new(SetupConnectionHandler {})),
message_type,
Expand Down
Loading