-
Notifications
You must be signed in to change notification settings - Fork 141
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
docs roles_logic_sv2
#1263
docs roles_logic_sv2
#1263
Conversation
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1263 +/- ##
==========================================
- Coverage 19.29% 19.14% -0.15%
==========================================
Files 164 166 +2
Lines 10852 10987 +135
==========================================
+ Hits 2094 2104 +10
- Misses 8758 8883 +125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9734571
to
c3bf010
Compare
c0bbc4f
to
f9b2125
Compare
This comment was marked as outdated.
This comment was marked as outdated.
426e910
to
14649c1
Compare
e99baf1
to
736b05c
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. We're not adding documentation for some structs, enums, and methods right now because they're likely to change in the near future. This crate needs significant refactoring, which will involve architectural changes. While the current documentation isn't complete, it provides enough context to understand the crate. We'll add examples once the refactoring is finished.
736b05c
to
73b0561
Compare
thanks! I made one last round of modifications (based on The elements still missing docs are:
all of them are obvious and meaning is self-evident... writing them would be extremely laborious and chatGPT proved a bit error-prone, so IMO it's fine to leave them like this |
73b0561
to
573c626
Compare
/// Enables the selection of which transactions or templates the node will work on. It allows | ||
/// for more autonomy for downstream nodes to define specific version bits to roll, adjust the | ||
/// difficulty target, apply `timestamp` range constraints, etc. | ||
/// | ||
/// - `true`: The downstream node can modify or customize the mining job, such as choosing | ||
/// specific transactions to include in a block template. | ||
/// - `false`: The downstream node strictly follows the work provided by the upstream, such as | ||
/// pre-constructed templates from a pool. |
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.
this one also should be clarified:
this flag is kind of a "feature gate" for whether SetCustomMiningJob
will be allowed to flow within this connection or not (set via REQUIRES_WORK_SELECTION
bit flag when the connection is established)
the description is conflating it with version rolling (which is gated by the flag below) and other extra features
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.
also re-written and simplified in 63b2c2b
@plebhash rn roles-logic-sv2 is not even spec complaints I would merge my PRs before documenting it. |
What is not spec-compliant? This PR is the last step of our efforts in documenting all crates (see #845) before refactoring them. And it's pretty much ready to be merged since a while now. We are just waiting for a final ACK from @rrybarczyk. |
|
the PR on the link is only changing SRI, we haven't enacted this change in the spec repo yet I'm about to start working on a PR that will introduce the different JD spec changes (including shortId removal), but ideally we should merge this first: stratum-mining/sv2-spec#104 |
NACK spec changes have not yet been enacted on we only have social consensus, so let's not block this PR because of that |
7328cd4
to
db12082
Compare
close #1014