Skip to content

upgrade Job Declaration and Mining Protocols #121

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

Merged
merged 12 commits into from
Apr 8, 2025

Conversation

plebhash
Copy link
Member

@plebhash plebhash commented Jan 17, 2025

close #120

changes:

  • rename JDP variant of SubmitSolution to PushSolution (to avoid confusion with TDP SubmitSolution)
  • replace "async" terminology with new JDP "modes": Coinbase-only and Full-Template
  • drop ShortIds and SipHash (as discussed in Make more clear how we use the block header to calculate the txs short ids (job declarator) #99)
  • add prev_hash field to DeclareMiningJob message
  • add version field to PushSolution message (fka SubmitSolution)
  • clarify description of nbits field of PushSolution message (fka SubmitSolution)
  • remove extranonce_size, min_ntime and version fields from SetCustomMiningJob message

for easier review, I recommend this Markdown rendering on my fork

@plebhash plebhash force-pushed the upgrade-jdp branch 3 times, most recently from d7dbd0b to 22fb563 Compare January 17, 2025 23:22
@plebhash plebhash force-pushed the upgrade-jdp branch 6 times, most recently from c3baaca to 5583c11 Compare January 23, 2025 14:59
@plebhash plebhash changed the title [WIP] upgrade Job Declaration Protocol [WIP] upgrade Job Declaration and Mining Protocols Jan 23, 2025
@Sjors
Copy link
Contributor

Sjors commented Jan 24, 2025

I don't think it's wise to have two messages in the spec called SubmitSolution, defined in two different places and with different content. Imo they should either be unified and defined in one place, or one needs a new name.

@plebhash
Copy link
Member Author

I don't think it's wise to have two messages in the spec called SubmitSolution, defined in two different places and with different content. Imo they should either be unified and defined in one place, or one needs a new name.

I don't think it's possible to unify them, as one is meant for TP and the other for JDS (and each needs different parts of the solution)

I'm not opposed to renaming either one of them or both, but it would be good to get input from @Fi3 and @TheBlueMatt on this

@plebhash plebhash marked this pull request as ready for review January 24, 2025 13:57
@plebhash plebhash changed the title [WIP] upgrade Job Declaration and Mining Protocols upgrade Job Declaration and Mining Protocols Jan 24, 2025
@@ -75,45 +125,40 @@ Notably, if the pool intends to change the space it requires for coinbase transa
| request_id | U32 | Unique identifier for pairing the response |
| mining_job_token | B0_255 | Token that makes the client eligible for committing a mining job for approval/transaction declaration or for identifying custom mining job on mining connection. |
| coinbase_output_max_additional_size | U32 | The maximum additional serialized bytes which the pool will add in coinbase transaction outputs. See discussion in the Template Distribution Protocol's CoinbaseOutputDataSize message for more details. |
| async_mining_allowed | BOOL | If true, the mining_job_token can be used immediately on a mining connection in the SetCustomMiningJob message, even before DeclareMiningJob and DeclareMiningJob.Success messages have been sent and received. If false, Job Declarator MUST use this token for DeclareMiningJob only. <br>This MUST be true when SetupConnection.flags had REQUIRES_ASYNC_JOB_MINING set. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how the upstream communicate if the token is for coinbase-only mode or for full-template mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by simply allowing the connection to be established or not

when JDC sends SetupConnection, the DECLARE_TX_DATA bit flag is used to signal that it consents to revealing the txs... then it's up to JDS to decide whether this is ok or not

so we have 4 possible scenarios:

JDS allows Coinbase-only mode JDS does NOT allow Coinbase-only mode
JDC sets DECLARE_TX_DATA SetupConnection.Success SetupConnection.Success
JDC does NOT set DECLARE_TX_DATA SetupConnection.Success SetupConnection.Error (unsupported-feature-flags)

do you feel this still leaves room for unspecified scenarios?

Copy link
Member Author

@plebhash plebhash Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is heavily based on the assumption that JDS will operate EITHER under Coinbase-only OR Full-Template, but not BOTH at the same time.

If we want to allow for JDS to operate under both at the same time, then I see how DECLARE_TX_DATA is not sufficient, as JDC wouldn't know under which mode the token is supposed to be used.

So I guess the question is: does this scenario make sense? Can JDS ever allow for both Coinbase-only and Full-Template at the same time? TBH it doesn't really make much sense in my head, but maybe I have some blindspot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GitGab19 what are your thoughts here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think it doesn't make too much sense, but maybe @Fi3 meant something different so it's better to get clarifications from him on this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to allow for JDS to operate under both at the same time, then I see how DECLARE_TX_DATA is not sufficient, as JDC wouldn't know under which mode the token is supposed to be used.

So I guess the question is: does this scenario make sense? Can JDS ever allow for both Coinbase-only and Full-Template at the same time? TBH it doesn't really make much sense in my head, but maybe I have some blindspot.

I guess I should retract what I said here.

it doesn't matter that JDS supports both modes.

if JDC sends SetupConnection with DECLARE_TX_DATA bit enabled, then it assumes the connection is operating under Full Template mode (and all tokens are allocated are allocated with that in mind)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep before it was a token property so you could mix them but I guess it make more sense to define it connectionwise.

@plebhash plebhash force-pushed the upgrade-jdp branch 2 times, most recently from 251067a to b16de1e Compare January 28, 2025 13:26
@Fi3
Copy link
Contributor

Fi3 commented Jan 28, 2025

I don't think it's wise to have two messages in the spec called SubmitSolution, defined in two different places and with different content. Imo they should either be unified and defined in one place, or one needs a new name.

make sense

@plebhash

This comment was marked as resolved.

@plebhash

This comment was marked as resolved.

Copy link
Member

@GitGab19 GitGab19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK on my end 👍

Let's maybe wait a bit for @Fi3 input on the last opened discussion.

After that, it's good to go imo.

@plebhash plebhash force-pushed the upgrade-jdp branch 6 times, most recently from 7e20c54 to 9b41e0e Compare April 2, 2025 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spec Changes for Job Declaration + Mining Protocols
7 participants