-
Notifications
You must be signed in to change notification settings - Fork 127
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.
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
Refactor Framing
crate
#969
Refactor Framing
crate
#969
Conversation
f9a97f9
to
6dc8e75
Compare
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 8 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
d786508
to
dee8b9c
Compare
Any tips about this failing CI https://github.com/stratum-mining/stratum/actions/runs/9515356472/job/26229428224?pr=969 ? it seems to be failing only on linux, not on mac |
Hey @jbesraa, could I collaborate on the PR by adding some unit tests? I'm still new to Stratum and believe this would be a great learning opportunity for me. Also, I'm using Linux (Ubuntu distro). |
Hello @Shourya742 Ofcourse! It would be best if you used this branch as a base and branched out of it and start adding commits. Note that I want to update this branch and split the single commit into smaller detailed commits, so keep an eye on that. It would be also very useful if you wrote down here what you are intending to test so we dont do the same work twice. Happy also to do pair programming here to get things started. Edit: A great first step would be to take this workflow https://github.com/stratum-mining/stratum/actions/runs/9515356472/workflow?pr=969 and convert it to a shell script so we can run it easily locally. Once we do this, its easier to debug the failing test. |
Cool @jbesraa |
Cool. A note from the work done: If you notice here https://github.com/stratum-mining/stratum/pull/969/files#diff-5cb0e1268eb47c0881cebb757eb5fbb44ce55cdb2d5640581ef3729b74483b88L127 I added this error handling and I think this is where the error first hits, maybe that could be helpful for your debugging. |
@@ -142,17 +139,17 @@ impl<'a, T: Serialize + GetSize + Deserialize<'a>, B: IsBuffer + AeadBuffer> Wit | |||
} | |||
self.sv2_buffer.danger_set_start(0); | |||
let src = self.sv2_buffer.get_data_owned(); | |||
let frame = Sv2Frame::<T, B::Slice>::from_bytes_unchecked(src); | |||
let frame = Sv2Frame::<T, B::Slice>::from_bytes(src)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use unchecked here, if you are concerned you can put a check in debug mode
let src = self.noise_buffer.get_data_owned().as_mut().to_vec(); | ||
|
||
// below is inffalible as noise frame length has been already checked | ||
let frame = HandShakeFrame::from_bytes_unchecked(src.into()); | ||
let frame = HandShakeFrame::from_bytes(src.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use unchecked here, if you are concerned you can put a check in debug mode
@@ -203,7 +200,7 @@ impl<T: Serialize + binary_sv2::GetSize, B: IsBuffer> WithoutNoise<B, T> { | |||
0 => { | |||
self.missing_b = Header::SIZE; | |||
let src = self.buffer.get_data_owned(); | |||
let frame = Sv2Frame::<T, B::Slice>::from_bytes_unchecked(src); | |||
let frame = Sv2Frame::<T, B::Slice>::from_bytes(src)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use unchecked here, if you are concerned you can put a check in debug mode
f87dc31
to
acc8aca
Compare
1. Removed `NoiseHeader` struct in favor of three constants defined at the top of the file. 2. Added documentation and changed visibility to `pub(crate)` where needed. 3. Removed `Header::Default` and `Sv2Frame::Default` impls as they are unused. 4. Removed `unwrap()`s
acc8aca
to
cdeb873
Compare
The `Frame` trait is used solely by a single struct and its only adding biolerplate to the code without benifits. We could consider re-adding it in the future if needed.
This commit aims to refactor the `framing-sv2` crate. List of changes: 1. Removed `Frame` trait 2. Changed `EitherFrame` enum to `Frame` 3. Removed unused functions from `HandShakeFrame` and `Sv2Frame` 4. Added documentation where missing 5. Made sure there is consistency in function naming in `Frame`, i.e, in order to get the header you call `Frame::header`, for payload `Frame::payload` and so on. 6. Made sure we dont return mut data from `Frame` get functions(leave it to the caller to decide). 7. Fixed all the modules in `protocols` based on the above changes 8. Fixed all the modules in `roles` based on the above changes For 7 & 8 its mainly removing the `Frame` trait import plus naming fixes where I renamed `StandardEitherFrame` to `StandardFrame` and other name fixes and also fixing the `header` and `payload` calls to align with the new API.
cdeb873
to
12bec35
Compare
Breaking this down into smaller PRs, created first one here #976 picking two first commits from here |
Hey @jbesraa, I found the CI hiccup—it's due to the let len = payload.len();
let ptr = payload.as_mut_ptr();
let payload = unsafe { std::slice::from_raw_parts_mut(ptr, len) }; to a safer approach: let mut payload = payload.to_owned();
let payload = payload.as_mut_slice(); This tweak ditches the unsafe code for something cleaner and should fix the issue. Give it a shot and let’s see if it clears up the CI! |
relates to #903
This commit aims to refactor the
framing-sv2
crate. List of changes:0. Cleand up
header.rs
Frame
traitEitherFrame
enum toFrame
HandShakeFrame
andSv2Frame
Frame
, i.e, inorder to get the header you call
Frame::header
, for payloadFrame::payload
and so on.Frame
get functions(leave itto the caller to decide).
protocols
based on the above changesroles
based on the above changesFor 7 & 8 its mainly removing the
Frame
trait import plus naming fixeswhere I renamed
StandardEitherFrame
toStandardFrame
and other namefixes and also fixing the
header
andpayload
calls to align with thenew API.