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.

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 crate refactor: cleanup header.rs and remove Frame trait #976

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Jun 18, 2024

I am breaking #969 into smaller PRs because its touching too many parts of the code and doing it all in a single PR could result in endless conflicts and make it also hard to review.

In this part the header.rs file is tied up with a few things removed and replaced by constants and adding docs. the second commit removes the Frame trait and adjust the code accordingly.

Partially resolves #903

@jbesraa jbesraa force-pushed the 2024-06-18-framing-refactor-p1 branch 5 times, most recently from 099aa8c to 3468e0f Compare June 18, 2024 12:56
@jbesraa jbesraa mentioned this pull request Jun 18, 2024
Copy link
Contributor

github-actions bot commented Jun 18, 2024

🐰Bencher

ReportTue, July 2, 2024 at 16:59:05 UTC
ProjectStratum v2 (SRI)
Branch976/merge
Testbedsv1
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client-submit-serialize✅ (view plot)6,664.80 (-2.81%)7,378.90 (90.32%)
client-submit-serialize-deserialize✅ (view plot)7,386.80 (-5.05%)8,332.06 (88.66%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle✅ (view plot)8,073.50 (-3.32%)8,853.34 (91.19%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle✅ (view plot)927.25 (+3.05%)930.19 (99.68%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize✅ (view plot)707.52 (+1.41%)717.59 (98.60%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize✅ (view plot)247.42 (-0.51%)257.68 (96.02%)
client-sv1-get-authorize/client-sv1-get-authorize✅ (view plot)156.26 (-0.74%)162.91 (95.92%)
client-sv1-get-submit✅ (view plot)6,293.80 (-5.01%)7,147.96 (88.05%)
client-sv1-get-subscribe/client-sv1-get-subscribe✅ (view plot)281.67 (+0.75%)293.08 (96.11%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle✅ (view plot)781.83 (+4.09%)784.23 (99.69%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize✅ (view plot)617.34 (+0.44%)636.40 (97.01%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize✅ (view plot)203.66 (-1.22%)218.73 (93.11%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Jun 18, 2024

🐰Bencher

ReportTue, July 2, 2024 at 17:00:02 UTC
ProjectStratum v2 (SRI)
Branch2024-06-18-framing-refactor-p1
Testbedsv1
Click to view all benchmark results
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
Estimated Cycles Upper Boundary
estimated cycles | (%)
InstructionsInstructions Results
instructions | (Δ%)
Instructions Upper Boundary
instructions | (%)
L1 AccessesL1 Accesses Results
accesses | (Δ%)
L1 Accesses Upper Boundary
accesses | (%)
L2 AccessesL2 Accesses Results
accesses | (Δ%)
L2 Accesses Upper Boundary
accesses | (%)
RAM AccessesRAM Accesses Results
accesses | (Δ%)
RAM Accesses Upper Boundary
accesses | (%)
get_authorize✅ (view plot)8,492.00 (+0.59%)8,719.84 (97.39%)✅ (view plot)3,738.00 (-0.08%)3,850.09 (97.09%)✅ (view plot)5,237.00 (-0.16%)5,394.06 (97.09%)✅ (view plot)7.00 (-10.86%)10.25 (68.28%)✅ (view plot)92.00 (+1.96%)94.21 (97.66%)
get_submit✅ (view plot)95,463.00 (-0.09%)96,106.76 (99.33%)✅ (view plot)59,431.00 (-0.06%)59,760.83 (99.45%)✅ (view plot)85,348.00 (-0.06%)85,811.60 (99.46%)✅ (view plot)49.00 (-10.11%)62.55 (78.34%)✅ (view plot)282.00 (-0.03%)287.41 (98.12%)
get_subscribe✅ (view plot)7,945.00 (-0.37%)8,253.99 (96.26%)✅ (view plot)2,833.00 (+0.08%)2,935.46 (96.51%)✅ (view plot)3,960.00 (+0.12%)4,094.26 (96.72%)✅ (view plot)13.00 (-18.08%)19.85 (65.50%)✅ (view plot)112.00 (-0.51%)116.68 (95.99%)
serialize_authorize✅ (view plot)12,245.00 (+0.28%)12,494.66 (98.00%)✅ (view plot)5,309.00 (-0.05%)5,421.09 (97.93%)✅ (view plot)7,400.00 (-0.11%)7,557.40 (97.92%)✅ (view plot)10.00 (-6.73%)13.22 (75.66%)✅ (view plot)137.00 (+0.95%)140.22 (97.70%)
serialize_deserialize_authorize✅ (view plot)24,542.00 (+0.26%)24,710.64 (99.32%)✅ (view plot)9,890.00 (-0.10%)10,019.69 (98.71%)✅ (view plot)13,947.00 (-0.13%)14,141.18 (98.63%)✅ (view plot)33.00 (-8.91%)41.50 (79.51%)✅ (view plot)298.00 (+0.96%)298.61 (99.79%)
serialize_deserialize_handle_authorize✅ (view plot)30,281.00 (+0.40%)30,369.18 (99.71%)✅ (view plot)12,093.00 (-0.02%)12,205.09 (99.08%)✅ (view plot)17,106.00 (-0.07%)17,271.88 (99.04%)✅ (view plot)59.00 (+0.36%)64.39 (91.63%)✅ (view plot)368.00 (+1.02%)368.34 (99.91%)
serialize_deserialize_handle_submit✅ (view plot)126,372.00 (-0.03%)126,996.54 (99.51%)✅ (view plot)73,224.00 (-0.03%)73,599.76 (99.49%)✅ (view plot)104,947.00 (-0.04%)105,482.73 (99.49%)✅ (view plot)120.00 (-0.38%)130.70 (91.81%)✅ (view plot)595.00 (+0.03%)598.97 (99.34%)
serialize_deserialize_handle_subscribe✅ (view plot)27,445.00 (-0.03%)27,592.93 (99.46%)✅ (view plot)9,635.00 (+0.02%)9,737.46 (98.95%)✅ (view plot)13,630.00 (+0.04%)13,770.06 (98.98%)✅ (view plot)61.00 (-6.70%)73.56 (82.93%)✅ (view plot)386.00 (+0.06%)388.40 (99.38%)
serialize_deserialize_submit✅ (view plot)114,999.00 (-0.06%)115,619.23 (99.46%)✅ (view plot)68,001.00 (-0.07%)68,382.17 (99.44%)✅ (view plot)97,559.00 (-0.08%)98,120.51 (99.43%)✅ (view plot)65.00 (-5.80%)75.21 (86.42%)✅ (view plot)489.00 (+0.16%)492.29 (99.33%)
serialize_deserialize_subscribe✅ (view plot)22,876.00 (+0.00%)23,100.70 (99.03%)✅ (view plot)8,187.00 (+0.00%)8,292.85 (98.72%)✅ (view plot)11,531.00 (+0.00%)11,676.54 (98.75%)✅ (view plot)36.00 (-7.85%)43.99 (81.84%)✅ (view plot)319.00 (+0.13%)321.40 (99.25%)
serialize_submit✅ (view plot)99,780.00 (-0.10%)100,434.92 (99.35%)✅ (view plot)61,475.00 (-0.06%)61,809.94 (99.46%)✅ (view plot)88,195.00 (-0.06%)88,664.79 (99.47%)✅ (view plot)49.00 (-10.99%)62.57 (78.31%)✅ (view plot)324.00 (-0.19%)328.91 (98.51%)
serialize_subscribe✅ (view plot)11,282.00 (-0.36%)11,589.91 (97.34%)✅ (view plot)4,180.00 (+0.05%)4,282.46 (97.61%)✅ (view plot)5,817.00 (+0.06%)5,953.26 (97.71%)✅ (view plot)15.00 (-7.58%)18.85 (79.59%)✅ (view plot)154.00 (-0.71%)159.48 (96.57%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Jun 18, 2024

🐰Bencher

ReportTue, July 2, 2024 at 16:59:04 UTC
ProjectStratum v2 (SRI)
Branch2024-06-18-framing-refactor-p1
Testbedsv2

🚨 1 ALERT: Threshold Boundary Limit exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
client_sv2_mining_message_submit_standard_serializeL2 Accesses (accesses)🚨 (view plot | view alert)56.00 (+15.90%)55.85 (100.26%)

Click to view all benchmark results
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
Estimated Cycles Upper Boundary
estimated cycles | (%)
InstructionsInstructions Results
instructions | (Δ%)
Instructions Upper Boundary
instructions | (%)
L1 AccessesL1 Accesses Results
accesses | (Δ%)
L1 Accesses Upper Boundary
accesses | (%)
L2 AccessesL2 Accesses Results
accesses | (Δ%)
L2 Accesses Upper Boundary
accesses | (%)
RAM AccessesRAM Accesses Results
accesses | (Δ%)
RAM Accesses Upper Boundary
accesses | (%)
client_sv2_handle_message_common✅ (view plot)2,077.00 (+1.14%)2,132.47 (97.40%)✅ (view plot)473.00 (+0.34%)488.36 (96.85%)✅ (view plot)732.00 (-0.11%)757.27 (96.66%)✅ (view plot)10.00 (+37.61%)11.32 (88.32%)✅ (view plot)37.00 (+0.82%)38.55 (95.98%)
client_sv2_handle_message_mining✅ (view plot)8,253.00 (+0.60%)8,342.20 (98.93%)✅ (view plot)2,137.00 (+0.41%)2,170.42 (98.46%)✅ (view plot)3,158.00 (+0.39%)3,214.00 (98.26%)✅ (view plot)39.00 (+0.95%)43.48 (89.69%)✅ (view plot)140.00 (+0.72%)142.07 (98.54%)
client_sv2_mining_message_submit_standard✅ (view plot)6,316.00 (+0.55%)6,393.74 (98.78%)✅ (view plot)1,750.00 (-0.00%)1,765.06 (99.15%)✅ (view plot)2,546.00 (-0.29%)2,576.99 (98.80%)✅ (view plot)26.00 (+40.29%)26.25 (99.03%)✅ (view plot)104.00 (+0.13%)106.77 (97.40%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)14,661.00 (-0.70%)15,017.91 (97.62%)✅ (view plot)4,694.00 (-0.00%)4,709.06 (99.68%)✅ (view plot)6,751.00 (-0.05%)6,776.88 (99.62%)🚨 (view plot | view alert)56.00 (+15.90%)55.85 (100.26%)✅ (view plot)218.00 (-1.78%)229.60 (94.95%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)27,469.00 (-0.04%)27,826.32 (98.72%)✅ (view plot)10,585.00 (+0.35%)10,585.62 (99.99%)✅ (view plot)15,399.00 (+0.34%)15,399.47 (100.00%)✅ (view plot)90.00 (+6.59%)91.19 (98.70%)✅ (view plot)332.00 (-0.76%)344.62 (96.34%)
client_sv2_open_channel✅ (view plot)4,417.00 (-1.49%)4,606.15 (95.89%)✅ (view plot)1,461.00 (+0.02%)1,476.30 (98.96%)✅ (view plot)2,157.00 (+0.15%)2,176.67 (99.10%)✅ (view plot)11.00 (-7.17%)15.03 (73.18%)✅ (view plot)63.00 (-2.90%)68.09 (92.52%)
client_sv2_open_channel_serialize✅ (view plot)14,006.00 (-1.32%)14,462.96 (96.84%)✅ (view plot)5,064.00 (+0.01%)5,079.30 (99.70%)✅ (view plot)7,321.00 (+0.03%)7,342.44 (99.71%)✅ (view plot)42.00 (+11.11%)43.23 (97.15%)✅ (view plot)185.00 (-3.15%)199.15 (92.90%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)22,523.00 (-0.47%)22,998.92 (97.93%)✅ (view plot)8,027.00 (+0.47%)8,027.91 (99.99%)✅ (view plot)11,673.00 (+0.44%)11,673.74 (99.99%)✅ (view plot)84.00 (+12.63%)85.55 (98.19%)✅ (view plot)298.00 (-1.92%)314.58 (94.73%)
client_sv2_setup_connection✅ (view plot)4,659.00 (-0.76%)4,762.95 (97.82%)✅ (view plot)1,502.00 (+0.02%)1,517.30 (98.99%)✅ (view plot)2,274.00 (-0.14%)2,301.26 (98.82%)✅ (view plot)15.00 (+54.11%)15.44 (97.12%)✅ (view plot)66.00 (-2.49%)69.73 (94.65%)
client_sv2_setup_connection_serialize✅ (view plot)15,942.00 (-1.76%)16,531.20 (96.44%)✅ (view plot)5,963.00 (+0.00%)5,978.30 (99.74%)✅ (view plot)8,662.00 (+0.06%)8,681.68 (99.77%)✅ (view plot)49.00 (+8.53%)50.28 (97.46%)✅ (view plot)201.00 (-4.23%)219.03 (91.77%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)35,382.00 (-0.38%)35,741.04 (99.00%)✅ (view plot)14,855.00 (+0.26%)14,855.67 (100.00%)✅ (view plot)21,812.00 (+0.24%)21,812.03 (100.00%)✅ (view plot)110.00 (+9.09%)115.04 (95.62%)✅ (view plot)372.00 (-1.78%)385.58 (96.48%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Jun 18, 2024

🐰Bencher

ReportTue, July 2, 2024 at 16:59:08 UTC
ProjectStratum v2 (SRI)
Branch2024-06-18-framing-refactor-p1
Testbedsv2
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client_sv2_handle_message_common✅ (view plot)44.88 (+0.65%)45.33 (99.02%)
client_sv2_handle_message_mining✅ (view plot)75.37 (+3.17%)80.29 (93.87%)
client_sv2_mining_message_submit_standard✅ (view plot)14.64 (-0.07%)14.69 (99.67%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)262.71 (-0.78%)284.05 (92.49%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)601.48 (+1.14%)627.61 (95.84%)
client_sv2_open_channel✅ (view plot)166.67 (+0.57%)171.19 (97.36%)
client_sv2_open_channel_serialize✅ (view plot)272.11 (-3.50%)293.79 (92.62%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)371.15 (-1.77%)421.19 (88.12%)
client_sv2_setup_connection✅ (view plot)158.72 (-3.21%)174.69 (90.86%)
client_sv2_setup_connection_serialize✅ (view plot)494.41 (+4.60%)506.24 (97.66%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)950.14 (-2.25%)1,042.09 (91.18%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@jbesraa jbesraa force-pushed the 2024-06-18-framing-refactor-p1 branch 4 times, most recently from 6f71021 to 7459165 Compare June 18, 2024 14:42
@plebhash plebhash requested a review from Fi3 June 21, 2024 23:29
@plebhash plebhash force-pushed the 2024-06-18-framing-refactor-p1 branch from 7459165 to e51885f Compare June 25, 2024 20:00
@jbesraa
Copy link
Contributor Author

jbesraa commented Jun 26, 2024

@plebhash did you change anything here? I see you committed but cant see diff

@plebhash
Copy link
Collaborator

@plebhash did you change anything here? I see you committed but cant see diff

I rebased to keep it up with dev branch

image

@plebhash plebhash requested a review from rrybarczyk June 26, 2024 17:49
@jbesraa jbesraa force-pushed the 2024-06-18-framing-refactor-p1 branch from e51885f to 9c9f009 Compare July 1, 2024 07:07
@jbesraa
Copy link
Contributor Author

jbesraa commented Jul 1, 2024

rebased without further changes

Copy link
Collaborator

@rrybarczyk rrybarczyk left a comment

Choose a reason for hiding this comment

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

This looks good to me. Any idea why the MG bad-pool-config-test is failing in the CI?

Screen Shot 2024-07-01 at 10 47 36 AM

@jbesraa
Copy link
Contributor Author

jbesraa commented Jul 1, 2024

This looks good to me. Any idea why the MG bad-pool-config-test is failing in the CI?
Screen Shot 2024-07-01 at 10 47 36 AM

I think its a false alarm

@plebhash
Copy link
Collaborator

plebhash commented Jul 1, 2024

This looks good to me. Any idea why the MG bad-pool-config-test is failing in the CI?
Screen Shot 2024-07-01 at 10 47 36 AM

I think its a false alarm

correct, this is a false alarm

unfortunately the state of our MG CI is that we get many of those, and the solution is to keep re-running them with this button until they're all green:

image

running message-generator-tests.sh locally is usually a good way to get a more deterministic behavior

only if some specific test keeps failing in a deterministic way we know that there's actually something wrong with it

so our CI is currelty extremely suboptimal, and something should be done about this... MG CI already takes a really long time to run, and by having to re-run it several times (due to false alarms or rebasing) is a terrible bottleneck that really slows down development

we have some issues for that:

@Fi3
Copy link
Collaborator

Fi3 commented Jul 1, 2024

Not a solution but running them locally should be more stable.

@plebhash plebhash force-pushed the 2024-06-18-framing-refactor-p1 branch from 9c9f009 to 441ae41 Compare July 2, 2024 16:55
@plebhash
Copy link
Collaborator

plebhash commented Jul 2, 2024

I just rebased this PR to get it ready for merging

@jbesraa jbesraa force-pushed the 2024-06-18-framing-refactor-p1 branch from 441ae41 to 6413865 Compare July 3, 2024 14:16
@jbesraa
Copy link
Contributor Author

jbesraa commented Jul 3, 2024

@plebhash I think this is good to go

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
The `Frame` trait is used solely by a single struct and it is only
adding biolerplate to the code without benifits. We could consider
re-adding it in the future if needed.
@jbesraa jbesraa force-pushed the 2024-06-18-framing-refactor-p1 branch from 6413865 to c62fa3d Compare July 3, 2024 14:18
@plebhash plebhash merged commit f0b672e into stratum-mining:dev Jul 3, 2024
13 checks passed
@plebhash plebhash mentioned this pull request Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants