-
Notifications
You must be signed in to change notification settings - Fork 142
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
doc binary-sv2 #1231
doc binary-sv2 #1231
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1231 +/- ##
=======================================
Coverage 19.30% 19.30%
=======================================
Files 164 164
Lines 10849 10849
=======================================
Hits 2094 2094
Misses 8755 8755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@Shourya742 I think you mentioned the wrong issues in the description |
Thanks for pointing it out... Now pointing to correct issues.. |
0744189
to
9185560
Compare
Bencher Report
🚨 4 Alerts
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
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.
Here is the first chunk of a review.
Some of the changes I suggested in decodable.rs
can be extrapolated to other files. Specifically we need to avoid language like "This method", "This implementation", etc. and just directly say what it is (the language does not really need to change beyond the removal of the qualifiers). Also, we need to make sure each line goes up to 100 characters.
Do we have examples to add?
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/encodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
c3bfe3b
to
e452f0c
Compare
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/mod.rs
Outdated
Show resolved
Hide resolved
4e15631
to
32606e0
Compare
I approved let's aim to merge this as soon as we fix CI issues |
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.
While reading this I got the right context about what is this about and how things are working together. great job!
Added few comments and will be doing another review once they are addressed.
In regards to the commit history, I think having 15 commits is a bit too much for this pull request. Think about the history of the main
branch once we merge this. There will too many commits addressing the same-ish thing.
[![rustc+](https://img.shields.io/badge/rustc-1.75.0%2B-lightgrey.svg)](https://blog.rust-lang.org/2023/12/28/Rust-1.75.0.html) | ||
[![license](https://img.shields.io/badge/license-MIT%2FApache--2.0-blue.svg)](https://github.com/stratum-mining/stratum/blob/main/LICENSE.md) | ||
|
||
`binary-sv2` is a Rust crate that helps serialize and deserialize binary data into Stratum V2 messages — either through `serde` or custom trait-based setup. Allowing it to be used in environment(s) where std or/and serde are not available. |
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.
Is there an example on how to use the "custom trait-based setup" ?
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.
The example in the example folder is for the custom-trait based one.
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/decodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/encodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/encodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/encodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/encodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/codec/encodable.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/mod.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/mod.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
853f776
to
1c6d3fa
Compare
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.
ACK
Please just fixup last commit to the appropriate commit
3a15226
to
d8960cf
Compare
d8960cf
to
8bf2780
Compare
Done |
cc68035
to
0ba42e4
Compare
old review, everything was addressed, now it's just blocking the PR
as a consequence of stratum-mining#1231
as a consequence of stratum-mining#1231
* tp_authority_public_key update * lock version for cargo semver-checks * Use shorter relative path .. and fix some styling inconsistency * Rewrite docs in `common_messages_sv2` .. As part of the effort to improve Stratum V2 protocols docs, this commit aims to improves and make the documentation more comprehensive and accessible for contributors and end users alike. * Add README.md .. Use the template README used across the different Stratum V2 protocol crates to `common_messages_sv2` crate. * modify sv2.h to make it consistent with comments changes * Use shorter relative path * Add README file * Improve Job Declaration Documentation * Add README file * Use shorter paths * Improve `Template Distribution` subprotocol docs * modify sv2.h to make it consistent with comments changes * add binary-sv2 docs * add binary-sv2 readme.md * add binary-sv2 custom trait example * add binary-sv2 no-serde derive-codec docs * add binary-sv2 no-serde derive-codec readme * add binary-sv2 no-serde codec readme * add binary-sv2 no-serde codec docs * update sv2.h for binary_sv2 docs * change doc test in derive_codec * Minor warning and error grammar and typo fixes * bump jdc * Slice docs * Buffer docs * Lib WriteError, Write, Buffer docs * Back buffer pool docs * Front buffer pool docs * InnerMemory buffer pool docs * PoolMode docs * BufferPool docs * Top buffer_pool mod docs * Buffer pool examples * Top level crate docs + clean up * Update README * Sniffer::wait_for_message_type * fix unwrap on sv1-mining-device tcp connection.. Update roles/test-utils/mining-device-sv1/src/client.rs Co-authored-by: jbesraa <[email protected]> * remove redundant Drop implementation from ITF TemplateProvider * Add `translator_sv2` test * use macos-14 as macos runner * disable MG tproxy CI * Have JDS ping local mempool less frequently Otherwise Bitcoin Core -debug=rpc logging becomes too noisy. * Use shorter paths * Add README mining subprotocol * Rewrite mining subprotocol docs * Create a lib for `mining-sv2-proxy` * Add `mining-sv2-proxy` initializer * lock semver-checks to 37 while replacing MSRV with stable * bump serde_v2 major to 2.0.0.. since v37, cargo semver-checks enforces that removal of features are breaking changes therefore we need to update serde_v2 as a follow up to stratum-mining#1230 * bump binary_sv2 for serde_sv2 dependency.. since we bumpbed serde_sv2 to 2.0.0 * bump sv2_ffi for serde_sv2 dependency.. since we bumpted serde_sv2 to 2.0.0 * bump roles/Cargo.lock.. since we bumped serde_sv2 and binary_sv2 * bump const_sv2 major to 3.0.0.. since v37, cargo semver-checks enforces that removal of features are breaking changes therefore we need to update serde_v2 as a follow up to stratum-mining#1230 * bump noise_sv2 for const_sv2 dependency.. since we bumpbed const_sv2 to 3.0.0 * bump framing_sv2 for const_sv2 dependency.. since we bumpbed const_sv2 to 3.0.0 * bump codec_sv2 for const_sv2 dependency.. since we bumpbed const_sv2 to 3.0.0 * bump subprotocols for const_sv2 dependency.. since we bumpbed const_sv2 to 3.0.0 * bump sv2_ffi for const_sv2 dependency.. since we bumpbed const_sv2 to 3.0.0 * bump roles_logic_sv2 for const_sv2 dependency.. since we bumpbed const_sv2 to 3.0.0 * bump roles due to const_sv2 bump to 3.0.0 * bump framing_sv2 major to 3.0.0.. since v37, cargo semver-checks enforces that removal of features are breaking changes therefore we need to update framing_v2 as a follow up to stratum-mining#1230 * bump codec_sv2 for framing_sv2 dependency.. since we bumped framing_sv2 to 3.0.0 * bump framing_sv2 dendency version on roles_logic_sv2.. crate version has already been bumped before release. no need to bump again * bump roles due to framing_sv2 bump * bump benches due to framing_sv2 bump * bump message_generator_sv2 due to const_sv2 * change macos-14 to macos-13 * bump buffer_sv2.. as a follow up to stratum-mining#1230 * bump buffer_sv2 dep on roles/Cargo.lock * bump buffer_sv2 dep on utils/Cargo.lock * bump derive_codec_sv2.. as a consequence of stratum-mining#1231 * bump derive_codec_sv2 dep on roles/Cargo.lock * Add framing sv2 frame and handshake frame ex * /// -> // for private types+fns * framing mod doc cmts + clean * Add framing README * add codecov.yml to remove ci error * feat: update println! and eprintln! to info using tracing cargo. * Wait for `NewTemplate` message to arrive instead of failing if it is not there yet when we first check * ci: bump TP to 0.1.13 * test: increase TP log timestamp precision Also log rpc calls. * `common_messages_sv2` `job_declaration_sv2` `mining_sv2` `template_distribution_sv2`: remove the `no_std` feature and make them `#![no_std]` as they never use `std` anywhere. - bump their MAJOR version because of feature removal - bump the dependant crates PATCH version - updates docs * fix: clarify logging * remove readme field from Cargo.toml of crates that dont have a README.md * add release-libs.sh * release-libs.yaml use 1.75.0 toolchain * remove readme field from Cargo.toml of crates that dont have a README.md * avoid publishing protocols crates with all-features.. publishing with all-features is overengineering: - no_std is being deprecated - with_serde will be deprecated soon - for crates with relevant features, we specifically list them for publishing * min_ntime fix to use the one sent by TP * header_timestamp_value_assertion_in_new_extended_mining_job test addition * bump PATCH after timestamp-bug fix (stratum-mining#1324) * update lockfile after roles_logic patch * Test Pool role behavior if bad `coinbase_output`.. is provided * Remove bad pool config MG test * Move sniffer test to a separate file * Derive Clone+Debug for `JobDeclaratorClient` * Derive Clone+Debug for `JobDeclaratorServer` * Align all integration test start_* .. functions return signature * Make Sniffer::drop output more verbose * Add README.md to integration-tests * Move integration tests to separate GH action * Rename integration tests crate.. it's important to have a descriptive name for when this is published to crates.io * Modularize integration tests APIs.. into the following modules: - lib: with general purpose functions (e.g.: starters) - sniffer - template_provider * Remove redundant `TestPoolSv2`.. this struct is redundant and no other roles follow this pattern we can do the initialization inside `start_pool` * Fix variable naming on `wait_for_client`.. the `SocketAddr` is not a client, but the socket where we will listen to also, `listner` is a typo * Rename sniffer channel variables.. the original naming was confusing copypasta * Create `utils.rs` in `tests-integration` ..to hold utility functions used internaly only. * noise_sv2: keep current (std) API unchanged, add a no_std compliant API with `*_with_rng` and `*_with_now` * generate cargo lock --------- Co-authored-by: GitGab19 <[email protected]> Co-authored-by: plebhash <[email protected]> Co-authored-by: plebhash <[email protected]> Co-authored-by: jbesraa <[email protected]> Co-authored-by: bit-aloo <[email protected]> Co-authored-by: Gabriele Vernetti <[email protected]> Co-authored-by: Pavlenex <[email protected]> Co-authored-by: RJ Rybarczyk <[email protected]> Co-authored-by: Sjors Provoost <[email protected]> Co-authored-by: devworlds <[email protected]> Co-authored-by: Georges Palauqui <[email protected]> Co-authored-by: Gary Krause <[email protected]> Co-authored-by: Oleg Akulov <[email protected]>
Closes: #1008 , #1009 , #1010