-
Notifications
You must be signed in to change notification settings - Fork 130
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
base: main
Are you sure you want to change the base?
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.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. 🚨 Try these New Features:
|
Bencher Report
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.
[![docs.rs](https://docs.rs/common_messages_sv2/badge.svg)](https://docs.rs/common_messages_sv2) | ||
[![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) | ||
[![codecov](https://codecov.io/gh/stratum-mining/stratum/branch/main/graph/badge.svg)](https://app.codecov.io/gh/stratum-mining/stratum/tree/main/protocols%2Fv2%2Fcommon_messages_sv2) |
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.
[![codecov](https://codecov.io/gh/stratum-mining/stratum/branch/main/graph/badge.svg)](https://app.codecov.io/gh/stratum-mining/stratum/tree/main/protocols%2Fv2%2Fcommon_messages_sv2) | |
[![codecov](https://codecov.io/gh/stratum-mining/stratum/branch/main/graph/badge.svg)](https://app.codecov.io/gh/stratum-mining/stratum/tree/main/protocols%2Fv2%2Ftemplate_distribution_sv2) |
[![license](https://img.shields.io/badge/license-MIT%2FApache--2.0-blue.svg)](https://github.com/stratum-mining/stratum/blob/main/LICENSE.md) | ||
[![codecov](https://codecov.io/gh/stratum-mining/stratum/branch/main/graph/badge.svg)](https://app.codecov.io/gh/stratum-mining/stratum/tree/main/protocols%2Fv2%2Fcommon_messages_sv2) | ||
|
||
`template_distribution_sv2` is a Rust crate that implements the set of messages used by multiple Stratum V2 roles. |
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.
Isn't this a bit too much generic? It's the same of common_messages_sv2
, but maybe you still need to work on the README. In any case, I think here the description should contain some reference to template distribution protocol at least.
/// will be added by the pools it intends to use this work. It then **must** communicate the sum of | ||
/// such size to the Template Provider via this message. | ||
/// | ||
/// The Template Provider **must not** provide `NewMiningJob` or `NewExtendedMiningJob` messages |
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 Template Provider **must not** provide `NewMiningJob` or `NewExtendedMiningJob` messages | |
/// The Template Provider **must not** provide `NewTemplate` messages |
@@ -1,23 +1,14 @@ | |||
#![cfg_attr(feature = "no_std", no_std)] | |||
|
|||
//! # Template Distribution Protocol |
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.
//! # Template Distribution Protocol | |
//! # Stratum V2 Template Distribution Protocol Messages Crate |
/// current UNIX time may be used in place of an ID. | ||
/// Upstream’s identification of the template. | ||
/// | ||
/// Strictly increasing, the current UNIX time may be used in place of an ID. |
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 current UNIX time may be used in place of an ID.
I don't know if I would leave this tbh
/// Up to 8 bytes (not including the length byte) which are to be placed at the beginning of | ||
/// the coinbase field in the coinbase transaction. |
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 is wrong (cc @plebhash), since they are the bip34 bytes, we should put a proper description for it.
pub template_id: u64, | ||
} | ||
|
||
/// ## RequestTransactionData.Success (Server->Client) | ||
/// Message used by an upstream to respond successfuly to a [`RequestTransactionData`] message. |
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.
/// Message used by an upstream to respond successfuly to a [`RequestTransactionData`] message. | |
/// Message used by an upstream to respond successfully to a [`RequestTransactionData`] message. |
@@ -58,6 +66,38 @@ pub struct RequestTransactionDataSuccess<'decoder> { | |||
pub transaction_list: Seq064K<'decoder, B016M<'decoder>>, | |||
} | |||
|
|||
/// Message used by an upstream to respond successfuly to a [`RequestTransactionData`] message. |
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.
/// Message used by an upstream to respond successfuly to a [`RequestTransactionData`] message. | |
/// Message used by an upstream to respond successfully to a [`RequestTransactionData`] message. |
/// | ||
/// If a `NewMiningJob` or `NewExtendedMiningJob` message has previously been sent with the | ||
/// Future Job flag set, the [`SetNewPrevHash::template_id`] field **should** be set to the | ||
/// `job_id` present in that message. |
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.
/// `job_id` present in that message. | |
/// `template_id` present in that message. |
I think this should be the right description, I just asked on discord to be 100% sure
/// | ||
/// If a `NewMiningJob` or `NewExtendedMiningJob` message has previously been sent with the | ||
/// Future Job flag set, the [`SetNewPrevHash::template_id`] field **should** be set to the | ||
/// `job_id` present in that message. |
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.
/// `job_id` present in that message. | |
/// `template_id` present in that message. |
I think this should be the right description, I just asked on discord to be 100% sure
31e33f6
to
97b62d7
Compare
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.
/// coinbase transaction outputs. | ||
/// | ||
/// As the pool is responsible for adding coinbase transaction outputs for payouts and other uses, | ||
/// the Template Provider will need to consider this additional block size when selecting |
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 Template Provider will need to consider this additional block size when selecting | |
/// the Template Provider will need to consider this reserved space when selecting |
/// additional bytes required in the output count variable-length integer in the coinbase | ||
/// transaction when complying with the size limits. | ||
/// | ||
/// [`NewTemplate`]: crate::NewTemplate | ||
#[derive(Serialize, Deserialize, Copy, Clone, Debug, PartialEq, Eq)] | ||
#[repr(C)] |
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.
A C-compatible memory layout (#[repr(C)]
) seems unnecessary in this case, as the struct contains only a single field, which will always occupy 4 bytes without any padding.
/// additional bytes required in the output count variable-length integer in the coinbase | ||
/// transaction when complying with the size limits. | ||
/// | ||
/// [`NewTemplate`]: crate::NewTemplate | ||
#[derive(Serialize, Deserialize, Copy, Clone, Debug, PartialEq, Eq)] |
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.
#[derive(Serialize, Deserialize, Copy, Clone, Debug, PartialEq, Eq)] | |
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq)] |
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,
}
/// ensure backward compatibility, the transaction data **may** be encoded in a way that is | ||
/// different from the consensus serialization of Bitcoin transactions. | ||
/// | ||
/// The [`RequestTransactionDataSuccess`] sender **must** ensure that provided data is forward and |
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 [`RequestTransactionDataSuccess`] sender **must** ensure that provided data is forward and | |
/// The [`CRequestTransactionDataSuccess`] sender **must** ensure that provided data is forward and |
/// ignored by versions which do not understand it. | ||
/// | ||
/// To work around the limitation of not being able to negotiate e.g. a transaction compression | ||
/// scheme, the format of the opaque data in [`RequestTransactionDataSuccess`] messages **may** be |
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.
/// scheme, the format of the opaque data in [`RequestTransactionDataSuccess`] messages **may** be | |
/// scheme, the format of the opaque data in [`CRequestTransactionDataSuccess`] messages **may** be |
/// scheme, the format of the opaque data in [`RequestTransactionDataSuccess`] messages **may** be | ||
/// changed in a non-compatible way at the time of fork activation, given sufficient time from | ||
/// code-release to activation and there being in protocol(Template Declaration) signaling of | ||
/// support for the new fork (e.g. for soft-forks activated using [BIP 9]). |
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.
Can we just remove this description, and just add C version of RequestTransactionDataSuccess..
/// | ||
/// Upon validating a new best block, the server **must** immediately send this message. | ||
/// | ||
/// If a `NewMiningJob` or `NewExtendedMiningJob` message has previously been sent with the |
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.
/// If a `NewMiningJob` or `NewExtendedMiningJob` message has previously been sent with the | |
/// If a `NewTemplate` message has previously been sent with the |
/// TODO: Define how many previous works the client has to track (2? 3?), and require that the | ||
/// server reference one of those in SetNewPrevHash. |
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.
Why do we have this as a doc comment.?
/// Message used by a downstream to submit a successful solution to a previously provided template. | ||
/// | ||
/// Upon finding a Coinbase transaction and nonce pair which double-SHA256 hashes at or below | ||
/// [`crate::SetNewPrevHash::target`], the downstream **must** immediately send this message. | ||
/// | ||
/// Upon receiving this message, upstream **must** immediately construct the corresponding full | ||
/// block and attempt to propagate it to the Bitcoin network. |
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.
Can be removed, and can be detailed as c implementation for SubmitSolution
resolves #1206