-
Notifications
You must be signed in to change notification settings - Fork 140
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 for Template Distribution Messages #1257
Docs for Template Distribution Messages #1257
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1257 +/- ##
==========================================
+ Coverage 19.30% 19.63% +0.33%
==========================================
Files 164 153 -11
Lines 10849 10029 -820
==========================================
- Hits 2094 1969 -125
+ Misses 8755 8060 -695
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
|
a877fed
to
31e33f6
Compare
@GitGab19 this is ready for initial review |
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.
Overall LGTM, I left some minor comments.
protocols/v2/subprotocols/template-distribution/src/coinbase_output_data_size.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/new_template.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Outdated
Show resolved
Hide resolved
97b62d7
to
1c25a28
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.
I have a few general questions about structs with C memory layout, along with some minor nit. The description in the messaging is really detailed.
protocols/v2/subprotocols/template-distribution/src/coinbase_output_data_size.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/coinbase_output_data_size.rs
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/coinbase_output_data_size.rs
Show resolved
Hide resolved
pub coinbase_tx_locktime: u32, | ||
/// Merkle path hashes ordered from deepest. | ||
#[cfg_attr(feature = "with_serde", serde(borrow))] | ||
pub merkle_path: Seq0255<'decoder, U256<'decoder>>, | ||
} | ||
|
||
/// Message used by an upstream to provide a new template for downstream to mine on. | ||
#[repr(C)] | ||
#[cfg(not(feature = "with_serde"))] | ||
pub struct CNewTemplate { |
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.
Not related to doc this struct should be like this, to have more efficient memory layout, and not waste space in padding.
#[repr(C)]
#[cfg(not(feature = "with_serde"))]
pub struct CNewTemplate {
future_template: bool,
coinbase_tx_locktime: u32,
version: u32,
coinbase_tx_input_sequence: u32,
coinbase_tx_outputs_count: u32,
template_id: u64,
coinbase_tx_value_remaining: u64,
coinbase_prefix: CVec,
coinbase_tx_outputs: CVec,
merkle_path: CVec2,
}
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.
Same, this is for refactoring not documentation
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/submit_solution.rs
Outdated
Show resolved
Hide resolved
1c25a28
to
765772d
Compare
protocols/v2/subprotocols/template-distribution/src/coinbase_output_data_size.rs
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/submit_solution.rs
Outdated
Show resolved
Hide resolved
b618c7d
to
584cf99
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.
LGTM. Left few minor nits
protocols/v2/subprotocols/template-distribution/src/new_template.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/request_transaction_data.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/template-distribution/src/set_new_prev_hash.rs
Outdated
Show resolved
Hide resolved
95ed9a6
to
1826593
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.
1826593
to
9a44b3d
Compare
9a44b3d
to
7f388ce
Compare
@GitGab19 done |
resolves #1206