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

Jump to bottom

Add stratum-cli project #1115

Closed
wants to merge 4 commits into from
Closed

Add stratum-cli project #1115

wants to merge 4 commits into from

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Aug 19, 2024 •
edited
Loading

Note: This PR is still a draft and I do not expect it to be merged in less than a month TBH as it depends on the integration test PRs, but I do think its a good point to start the discussion and try and reach a consensus.

Currently this only covers the translator and jdclient. In order to test it locally, you can build the branch cargo build and then ./target/debug/stratum-cli translator start-local. For a full list of commands, try ./target/debug/stratum-cli help

blocked by #1092, #1097 and #1095

.. this should allow us to remove main.rs files from all the roles and leave them as pure libraries

@github-actions GitHub Actions
Copy link
Contributor

🐰Bencher

ReportMon, August 19, 2024 at 16:49:52 UTC
ProjectStratum v2 (SRI)
Branch2024-08-19-stratum-cli
Testbedsv2
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
client_sv2_handle_message_common✅ (view plot)44.48 (-0.15%)45.22 (98.36%)
client_sv2_handle_message_mining✅ (view plot)74.23 (+1.55%)81.02 (91.62%)
client_sv2_mining_message_submit_standard✅ (view plot)14.63 (-0.16%)14.69 (99.56%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)267.12 (+1.04%)283.59 (94.19%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)578.75 (-2.54%)626.99 (92.31%)
client_sv2_open_channel✅ (view plot)164.21 (-0.86%)171.38 (95.82%)
client_sv2_open_channel_serialize✅ (view plot)270.15 (-4.49%)293.95 (91.90%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)370.35 (-2.06%)422.91 (87.57%)
client_sv2_setup_connection✅ (view plot)169.08 (+2.87%)175.07 (96.58%)
client_sv2_setup_connection_serialize✅ (view plot)445.37 (-5.97%)505.45 (88.11%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)960.54 (-1.20%)1,043.17 (92.08%)

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

@github-actions GitHub Actions
Copy link
Contributor

🐰Bencher

ReportMon, August 19, 2024 at 16:49:52 UTC
ProjectStratum v2 (SRI)
Branch2024-08-19-stratum-cli
Testbedsv2

🚨 10 ALERTS: Threshold Boundary Limits exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
client_sv2_mining_message_submit_standardL2 Accesses (accesses)🚨 (view plot | view alert)23.00 (+31.16%)22.72 (101.24%)
client_sv2_mining_message_submit_standard_serialize_deserializeInstructions (instructions)🚨 (view plot | view alert)10,591.00 (+0.45%)10,568.85 (100.21%)
client_sv2_mining_message_submit_standard_serialize_deserializeL1 Accesses (accesses)🚨 (view plot | view alert)15,404.00 (+0.41%)15,374.49 (100.19%)
client_sv2_open_channel_serialize_deserializeInstructions (instructions)🚨 (view plot | view alert)8,027.00 (+0.53%)8,009.32 (100.22%)
client_sv2_open_channel_serialize_deserializeL1 Accesses (accesses)🚨 (view plot | view alert)11,669.00 (+0.46%)11,646.70 (100.19%)
client_sv2_open_channel_serialize_deserializeL2 Accesses (accesses)🚨 (view plot | view alert)83.00 (+12.93%)82.34 (100.80%)
client_sv2_setup_connectionL2 Accesses (accesses)🚨 (view plot | view alert)15.00 (+64.06%)13.85 (108.27%)
client_sv2_setup_connection_serializeL2 Accesses (accesses)🚨 (view plot | view alert)51.00 (+13.74%)49.67 (102.67%)
client_sv2_setup_connection_serialize_deserializeInstructions (instructions)🚨 (view plot | view alert)14,855.00 (+0.29%)14,836.63 (100.12%)
client_sv2_setup_connection_serialize_deserializeL1 Accesses (accesses)🚨 (view plot | view alert)21,812.00 (+0.28%)21,784.66 (100.13%)

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,111.00 (+2.85%)2,134.78 (98.89%)✅ (view plot)473.00 (+0.47%)486.21 (97.28%)✅ (view plot)731.00 (-0.17%)754.94 (96.83%)✅ (view plot)10.00 (+44.33%)10.59 (94.42%)✅ (view plot)38.00 (+3.45%)38.78 (97.99%)
client_sv2_handle_message_mining✅ (view plot)8,221.00 (+0.29%)8,334.45 (98.64%)✅ (view plot)2,143.00 (+0.71%)2,171.53 (98.69%)✅ (view plot)3,166.00 (+0.65%)3,216.28 (98.44%)✅ (view plot)38.00 (-1.02%)43.51 (87.34%)✅ (view plot)139.00 (+0.10%)141.92 (97.94%)
client_sv2_mining_message_submit_standard✅ (view plot)6,310.00 (+0.53%)6,389.89 (98.75%)✅ (view plot)1,756.00 (+0.36%)1,763.40 (99.58%)✅ (view plot)2,555.00 (+0.04%)2,575.70 (99.20%)🚨 (view plot | view alert)23.00 (+31.16%)22.72 (101.24%)✅ (view plot)104.00 (+0.14%)106.90 (97.29%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)14,775.00 (-0.04%)15,033.62 (98.28%)✅ (view plot)4,700.00 (+0.13%)4,707.40 (99.84%)✅ (view plot)6,760.00 (+0.08%)6,775.19 (99.78%)✅ (view plot)49.00 (+3.82%)51.67 (94.84%)✅ (view plot)222.00 (-0.26%)229.93 (96.55%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)27,599.00 (+0.42%)27,849.18 (99.10%)🚨 (view plot | view alert)10,591.00 (+0.45%)10,568.85 (100.21%)🚨 (view plot | view alert)15,404.00 (+0.41%)15,374.49 (100.19%)✅ (view plot)87.00 (+3.92%)89.20 (97.53%)✅ (view plot)336.00 (+0.30%)345.31 (97.31%)
client_sv2_open_channel✅ (view plot)4,353.00 (-3.07%)4,617.16 (94.28%)✅ (view plot)1,461.00 (+0.05%)1,474.22 (99.10%)✅ (view plot)2,158.00 (+0.24%)2,172.66 (99.33%)✅ (view plot)12.00 (-0.15%)15.21 (78.89%)✅ (view plot)61.00 (-6.28%)68.40 (89.18%)
client_sv2_open_channel_serialize✅ (view plot)14,092.00 (-0.89%)14,456.70 (97.48%)✅ (view plot)5,064.00 (+0.02%)5,077.22 (99.74%)✅ (view plot)7,322.00 (+0.06%)7,339.03 (99.77%)✅ (view plot)38.00 (+2.31%)41.40 (91.80%)✅ (view plot)188.00 (-2.02%)198.73 (94.60%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)22,689.00 (+0.19%)23,024.76 (98.54%)🚨 (view plot | view alert)8,027.00 (+0.53%)8,009.32 (100.22%)🚨 (view plot | view alert)11,669.00 (+0.46%)11,646.70 (100.19%)🚨 (view plot | view alert)83.00 (+12.93%)82.34 (100.80%)✅ (view plot)303.00 (-0.56%)314.85 (96.23%)
client_sv2_setup_connection✅ (view plot)4,693.00 (-0.15%)4,765.54 (98.48%)✅ (view plot)1,502.00 (+0.05%)1,515.22 (99.13%)✅ (view plot)2,273.00 (-0.17%)2,299.32 (98.86%)🚨 (view plot | view alert)15.00 (+64.06%)13.85 (108.27%)✅ (view plot)67.00 (-1.37%)69.61 (96.25%)
client_sv2_setup_connection_serialize✅ (view plot)16,188.00 (-0.52%)16,483.42 (98.21%)✅ (view plot)5,963.00 (+0.01%)5,976.22 (99.78%)✅ (view plot)8,653.00 (-0.02%)8,676.80 (99.73%)🚨 (view plot | view alert)51.00 (+13.74%)49.67 (102.67%)✅ (view plot)208.00 (-1.55%)217.21 (95.76%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)35,562.00 (+0.06%)35,750.35 (99.47%)🚨 (view plot | view alert)14,855.00 (+0.29%)14,836.63 (100.12%)🚨 (view plot | view alert)21,812.00 (+0.28%)21,784.66 (100.13%)✅ (view plot)104.00 (+4.28%)112.52 (92.42%)✅ (view plot)378.00 (-0.46%)384.38 (98.34%)

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

@github-actions GitHub Actions
Copy link
Contributor

🐰Bencher

ReportMon, August 19, 2024 at 16:49:54 UTC
ProjectStratum v2 (SRI)
Branch1115/merge
Testbedsv1
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
client-submit-serialize✅ (view plot)6,450.60 (-6.95%)7,358.10 (87.67%)
client-submit-serialize-deserialize✅ (view plot)7,464.30 (-5.02%)8,346.20 (89.43%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle✅ (view plot)7,960.90 (-5.46%)8,857.48 (89.88%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle✅ (view plot)893.75 (-0.50%)926.09 (96.51%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize✅ (view plot)718.37 (+2.99%)719.41 (99.85%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize✅ (view plot)245.23 (-1.10%)255.10 (96.13%)
client-sv1-get-authorize/client-sv1-get-authorize✅ (view plot)156.20 (-0.58%)162.35 (96.21%)
client-sv1-get-submit✅ (view plot)6,308.20 (-5.85%)7,146.12 (88.27%)
client-sv1-get-subscribe/client-sv1-get-subscribe✅ (view plot)277.42 (-0.40%)291.01 (95.33%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle✅ (view plot)756.16 (+1.19%)776.06 (97.44%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize✅ (view plot)621.86 (+1.19%)637.70 (97.52%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize✅ (view plot)205.93 (-0.39%)219.85 (93.67%)

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

@github-actions GitHub Actions
Copy link
Contributor

🐰Bencher

ReportMon, August 19, 2024 at 16:49:59 UTC
ProjectStratum v2 (SRI)
Branch2024-08-19-stratum-cli
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,507.00 (+0.93%)8,746.61 (97.26%)✅ (view plot)3,772.00 (+0.90%)3,864.85 (97.60%)✅ (view plot)5,297.00 (+1.03%)5,414.11 (97.84%)✅ (view plot)5.00 (-36.10%)10.53 (47.49%)✅ (view plot)91.00 (+1.23%)94.24 (96.56%)
get_submit✅ (view plot)95,654.00 (+0.11%)96,173.49 (99.46%)✅ (view plot)59,522.00 (+0.09%)59,782.21 (99.56%)✅ (view plot)85,499.00 (+0.11%)85,841.46 (99.60%)✅ (view plot)50.00 (-9.21%)62.50 (80.00%)✅ (view plot)283.00 (+0.41%)288.76 (98.00%)
get_subscribe✅ (view plot)8,068.00 (+1.20%)8,300.43 (97.20%)✅ (view plot)2,848.00 (+0.75%)2,949.78 (96.55%)✅ (view plot)3,983.00 (+0.84%)4,112.50 (96.85%)✅ (view plot)12.00 (-25.65%)19.89 (60.34%)✅ (view plot)115.00 (+2.12%)117.42 (97.94%)
serialize_authorize✅ (view plot)12,358.00 (+1.27%)12,535.77 (98.58%)✅ (view plot)5,343.00 (+0.63%)5,435.85 (98.29%)✅ (view plot)7,458.00 (+0.71%)7,577.21 (98.43%)✅ (view plot)7.00 (-34.37%)13.60 (51.48%)✅ (view plot)139.00 (+2.55%)140.79 (98.73%)
serialize_deserialize_authorize✅ (view plot)24,580.00 (+0.47%)24,722.40 (99.42%)✅ (view plot)9,950.00 (+0.51%)10,035.49 (99.15%)✅ (view plot)14,050.00 (+0.59%)14,163.84 (99.20%)✅ (view plot)34.00 (-7.18%)41.62 (81.68%)✅ (view plot)296.00 (+0.43%)297.62 (99.46%)
serialize_deserialize_handle_authorize✅ (view plot)30,180.00 (+0.15%)30,370.67 (99.37%)✅ (view plot)12,127.00 (+0.28%)12,219.85 (99.24%)✅ (view plot)17,165.00 (+0.29%)17,291.71 (99.27%)✅ (view plot)62.00 (+5.30%)64.83 (95.64%)✅ (view plot)363.00 (-0.15%)367.11 (98.88%)
serialize_deserialize_handle_submit✅ (view plot)126,443.00 (+0.04%)127,073.37 (99.50%)✅ (view plot)73,307.00 (+0.08%)73,628.21 (99.56%)✅ (view plot)105,098.00 (+0.11%)105,523.91 (99.60%)✅ (view plot)111.00 (-7.49%)131.80 (84.22%)✅ (view plot)594.00 (-0.09%)600.26 (98.96%)
serialize_deserialize_handle_subscribe✅ (view plot)27,482.00 (+0.13%)27,641.39 (99.42%)✅ (view plot)9,650.00 (+0.22%)9,751.78 (98.96%)✅ (view plot)13,652.00 (+0.24%)13,788.01 (99.01%)✅ (view plot)64.00 (-3.00%)73.66 (86.89%)✅ (view plot)386.00 (+0.10%)388.77 (99.29%)
serialize_deserialize_submit✅ (view plot)115,239.00 (+0.14%)115,676.54 (99.62%)✅ (view plot)68,167.00 (+0.15%)68,409.59 (99.65%)✅ (view plot)97,844.00 (+0.19%)98,163.24 (99.67%)✅ (view plot)63.00 (-9.25%)75.55 (83.39%)✅ (view plot)488.00 (+0.03%)493.20 (98.95%)
serialize_deserialize_subscribe✅ (view plot)22,986.00 (+0.50%)23,136.04 (99.35%)✅ (view plot)8,209.00 (+0.32%)8,307.33 (98.82%)✅ (view plot)11,566.00 (+0.35%)11,695.59 (98.89%)✅ (view plot)37.00 (-6.22%)43.98 (84.12%)✅ (view plot)321.00 (+0.78%)321.82 (99.74%)
serialize_submit✅ (view plot)100,065.00 (+0.17%)100,506.53 (99.56%)✅ (view plot)61,566.00 (+0.08%)61,831.38 (99.57%)✅ (view plot)88,345.00 (+0.10%)88,694.81 (99.61%)✅ (view plot)48.00 (-13.85%)62.42 (76.89%)✅ (view plot)328.00 (+1.06%)330.70 (99.18%)
serialize_subscribe✅ (view plot)11,507.00 (+1.60%)11,646.59 (98.80%)✅ (view plot)4,195.00 (+0.51%)4,296.78 (97.63%)✅ (view plot)5,837.00 (+0.50%)5,971.60 (97.75%)✅ (view plot)14.00 (-14.38%)19.04 (73.54%)✅ (view plot)160.00 (+3.00%)160.55 (99.66%)

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

@jbesraa jbesraa changed the title 2024 08 19 stratum cli Add stratum-cli project Aug 19, 2024
@plebhash
Copy link
Collaborator

plebhash commented Aug 19, 2024 •
edited
Loading

as much as I think would be a positive change, I feel this scope is starting to move in the direction of optimizations that are not really needed with regards to where the project is right now

we agreed on #1066 (and later #1093) because that was a strict requirement for #1120, while I'm not really convinced on the need for a unified binary just yet

as I already stated in our dev calls, I feel that deeper changes to the application layer should be left for a more distant future, once we are already done with #845 and we have a more clear outline on the ideal architecture for the application layer that are still under conceptual discussion under #1069

don't get me wrong, I do feel a unified CLI would be ideal for the long term, as I already proposed via #1069 (comment)

but for now, I feel our time and efforts would be better spent if we focused on either:

@jbesraa can you elaborate on what you see as the immediate benefits of this change? why should we focus on this sooner rather than later?

@plebhash plebhash mentioned this pull request Aug 19, 2024
Merged
@jbesraa
Copy link
Contributor Author

jbesraa commented Aug 20, 2024 •
edited
Loading

as much as I think would be a positive change, I feel this scope is starting to move in the direction of optimizations that are not really needed with regards to where the project is right now

Why do you think so? Our project does not have a proper entry point, which is one of the first things usually done in projects.

we agreed on #1066 (and later #1093) because that was a strict requirement for #1120, while I'm not really convinced on the need for a unified binary just yet

Adding a cli tool for a "plug and play" access to our code-base is not a "unified binary", its just an API for the end user to interact with the roles.

as I already stated in our dev calls, I feel that deeper changes to the application layer should be left for a more distant future, once we are already done with #845 and we have a more clear outline on the ideal architecture for the application layer that are still under conceptual discussion under #1069

How would #845 affect this change? I do not think its related in anyway. Adding a cli app could be discarded if you think there is a better way to expose our libraries to the end user, but I do not see how #845 can be a factor here. Moreover, the work on #845 is gonna take at least a year if not more, I have a few open PRs that are waiting for review for a bit and they are not moving forward. #845 Only changes the protocols code which I think should be in general the lowest priority for the following reasons:

  1. Its tricky to review and approve
  2. Its working and kinda fine, not as bad as the roles
  3. The end user will see minor benefits from months of work of multiple developers

don't get me wrong, I do feel a unified CLI would be ideal for the long term, as I already proposed via #1069 (comment)

but for now, I feel our time and efforts would be better spent if we focused on either:

* [Rust Docs + refactor: `protocols` crates #845](https://github.com/stratum-mining/stratum/issues/845)

* [SRI Integration Test Framework #1120](https://github.com/stratum-mining/stratum/issues/1120)

Both #845 and #1120 are work in progress and have open PRs that should be reviewed so I am not sure why adding CLI should be blocked by those?

@jbesraa can you elaborate on what you see as the immediate benefits of this change? why should we focus on this sooner rather than later?

  1. Low effort: Creating the initial CLI with just the functionality exposed from the binaries is easy and quick. This PR should just cherry-pick the other commits from the Move bin code to lib and add another few cases in the code to cover them. It can be a quick win with minimum(actually, none) footprint on the current code
  2. It will allow us to remove all the binary related things in the roles which is quite a distraction
  3. In the last few weeks you spent time fixing lock files and other problems related to binary/library, bit-aloo also did a similar effort with removing the toml package, our best developers are spending the time putting plasters on a bleeding heart with changes that will be discarded sooner or later.
  4. 100x better user experience
  5. No breaking changes
  6. No actual code changes in roles or anywhere else, this is just adding new crate stratum-cli, we can decide to remove the main.rs files in a later PR

@jbesraa
Copy link
Contributor Author

jbesraa commented Aug 20, 2024

In general, it would be better to stop thinking about everything as a refactor. Even when adding this 'stratum-cli' there is none refactoring, its merely a code organization putting things in there correct place.

@plebhash
Copy link
Collaborator

Both #845 and #1120 are work in progress and have open PRs that should be reviewed so I am not sure why adding CLI should be blocked by those?

it's not a matter of blocking, but a matter of how we strategize about where our collective efforts are focused

the reason progress on #845 and #1120 have been slow is the fact that we are a small number of contributors, and unless we keep our scope under reasonable and manageable constrains, our efforts will be scattered and progress will be slow everywhere

with 1.0.2 out of the way, I hope we will be able to spend more time on those activities and make meaningful progress on #845 and #1120

In the last few weeks you spent time fixing lock files and other problems related to binary/library

the lockfiles issue could be mitigated via CI, as proposed via #1102

we can decide to remove the main.rs files in a later PR

removing main.rs files from roles will have implications on the entire MG CI stack, which relies heavily on the current CLI structure of the different roles binaries

so we can't break the current roles CLI until #1121 is fully addressed, otherwise we will loose our current CI

we would also need to adapt docs and communicate those changes to adopters who are currently experimenting with the current roles binaries

which brings me back to my original point: this might seem like a good direction at a first glance, but it opens up many cans of worms that will inevitably drain our energy into new unforeseen directions... in an ideal world where all SRI contributors had infinite bandwidth for tackling all those implications, sure, that would be ok... but unfortunately we need to be very strategic about how we focus our efforts

my bottom line here is: I'm not strictly opposed to UI enhancements for roles, I just think this is a bit premature, and our efforts will be more optimally allocated if we could move #1120 forward

@jbesraa jbesraa mentioned this pull request Aug 23, 2024
Merged
@plebhash plebhash changed the base branch from dev to main August 28, 2024 20:14
@jbesraa jbesraa closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbesraa @plebhash