Worker Specific Hashrate Tracking Extension #113
Conversation
GitGab19
force-pushed
the
extend-submit-shares-extended
branch
from
81852c2 to
1f5150f
Compare
| | channel_id | U32 | Channel identification. | | ||
| | sequence_number | U32 | Unique sequential identifier of the submit within the channel. | | ||
| | job_id | U32 | Identifier of the job as provided by NewMiningJob or NewExtendedMiningJob message. | | ||
| | nonce | U32 | Nonce leading to the hash being submitted. | | ||
| | ntime | U32 | The nTime field in the block header. This MUST be greater than or equal to the header_timestamp field in the latest SetNewPrevHash message and lower than or equal to that value plus the number of seconds since the receipt of that message. | | ||
| | version | U32 | Full nVersion field. | | ||
| | extranonce | B0_32 | Extranonce bytes which need to be added to coinbase to form a fully valid submission (full coinbase = coinbase_tx_prefix + extranonce_prefix + extranonce + coinbase_tx_suffix). The size of the provided extranonce MUST be equal to the negotiated extranonce size from channel opening. | |
There was a problem hiding this comment.
I wouldn't re-state this, just write "existing message", cause this is gonna get stale.
| | ntime | U32 | The nTime field in the block header. This MUST be greater than or equal to the header_timestamp field in the latest SetNewPrevHash message and lower than or equal to that value plus the number of seconds since the receipt of that message. | | ||
| | version | U32 | Full nVersion field. | | ||
| | extranonce | B0_32 | Extranonce bytes which need to be added to coinbase to form a fully valid submission (full coinbase = coinbase_tx_prefix + extranonce_prefix + extranonce + coinbase_tx_suffix). The size of the provided extranonce MUST be equal to the negotiated extranonce size from channel opening. | | ||
| | user_identity | STR0_255 | Up to 32 bytes (not including the length byte), unique string identity for the worker | |
There was a problem hiding this comment.
Not a big fan of defining this here. As written, this implies that the negotiated extension set has to be passed through to the message serialization layer, which is pretty gross. Instead, IMO, we should define this field in the main spec, but then include "this field MUST NOT be set to a length other than zero, or be elided, unless the extension is negotiated". That way the message serialization layer knows how to serialize always, its just the higher level code can reject/ignore the field if the extension was not negotiated.
There was a problem hiding this comment.
I don't see how this approach could scale. Maybe I don't get it. But this means that for every extension that we define we need to "reserve" fields in the main spec?
I mean this should not be a modified message but a completely new messages with his own message id under the extension namespace ofc.
There was a problem hiding this comment.
I think that an extension should never be allowed to modify format of a message defined somewhere else. But I also think that we should not modify the main spec to have special field that get activated only when an extension is on: (1) I really don't see how this can scale to more the an handfull of extension; (2) every time that we add an extension we need to release a new version of sv2 that is crazy.
I'm ok btw with having optional field at the end of a message in order to allow extending the main spec without breaking compatibility with old versions. But IMO these fields should be used to extend the main spec only not to add extensions.
There was a problem hiding this comment.
I mean this should not be a modified message but a completely new messages with his own message id under the extension namespace ofc.
This way doesn't scale and would make things very hard for parsers in general. What if a single message (let's say the SubmitSharesExtended) is going to add 5 different new fields in the future with 5 different extensions?
That's the reason why it's better to define the new fields in the main spec, and then enable them only in the case a specific set of extensions have been negotiated.
There was a problem hiding this comment.
That's the reason why it's better to define the new fields in the main spec, and then enable them only in the case a specific set of extensions have been negotiated.
No, adding field in main spec specifically for extension no not scale for the reasons that I stated above:
- You will likely have conflicting extension (they both need a field in a message) so you end up adding a lot of field.
- You will need to change main spec (not backward comaptible version) every time that you need an extension. This completely defeat the purpose of having an extension.
What if a single message (let's say the SubmitSharesExtended) is going to add 5 different new fields in the future with 5 different extensions?
absolutely, otherwise you will have 5 different spec that is just crazy.
make things very hard for parsers in general
No is super easy for adding a new message type in the parser that support the extension.
There was a problem hiding this comment.
if extensions can add field at the end of the messages we either end up with incompatible extensions or we need to have an authority that authorize an extensions. On the other side if every extension must define is own messages anyone can create extensions that are compatible with each other. To me it seems that the pros of the latter outweigh the cons (having to send 2 message).
We also have the additional field free for protocol patches but I think that we can argue that is true also with the former case without an autority where we allow incompatible extension. Since you can ship a patch as an extension. But that seems a little bit worse to me.
There was a problem hiding this comment.
If you add new message for each new extension, you are limited to use only a single extension for each message.
For example if there are two extensions and both change SubmitSharesExtended, you will be able to use only one of them.
There was a problem hiding this comment.
no each extension has his own namespace, in that case you will have SubmitSharesExtendedExtension1 SubmitSharesExtendedExtension2 of course every message will have only the data needed for that extension.
Look at that message for example https://github.com/demand-open-source/share-accounting-ext/blob/master/extension.md#shareok-server---client
Here the share accounting extension define a new message that upstream send when downstream send a good share, this message have 2 fields that are needed by the extension (is outdated we need also a signature).
This is the description:
Sent by the pool for each SubmitShare submitted by the pool.
When this extension is active the pool MAY not send SubmitShares.Success
If downstream have only this extension activated the pool will send only ShareOk if have other extension can decide to send also SubmitShares.Success
There was a problem hiding this comment.
I see. Tbh none of the proposed solutions sounds compelling enough. Did we consider the TLV route or something similar?
|
After yesterday's call, I noticed that there were some misunderstandings about this PR. So I just pushed a new commit to clarify this point of discussion. Please @Fi3 @TheBlueMatt review the last commit: 9260a15 |
This PR adds an extension to directly send a
user_identityfield inside aSubmitSharesExtended. Theuser_identityfield is limited to a maximum of 32 bytes-length to not increase too much the additional bandwidth consumption for extended shares submissions.This PR follows #112.