Skip to content

Conversation

@ccdle12
Copy link

@ccdle12 ccdle12 commented Oct 18, 2022

Adds a new field to the NewTemplate message to contain an optional 32 byte witness reserve value from the coinbase tx scriptWitness field. If the NewTemplate does not contain segwit inputs then we could simply just send 0 for the byte length or omit it entirely.

The witness reserve value is used for future extensions and is required to compute the witness commitment SHA256^2(witness root, witness reserve value).

My current understanding is the witness reserve value is currently always set to [0x00; 32] but this may change with future use of this field.

* Adds a new optional field for the TemplateProvider to send the
  witness reserve value in a NewTemplate if it contains segwit inputs.
  The downstream device will use the witness reserve value to compute
  a valid witness commitment.
* Explicitly state the purpose of additional coinbase tx outputs since
  the downstream device will construct the first output to pay fee +
  block subsidy to itself/interested parties.
ccdle12 added a commit to ccdle12/stratum that referenced this pull request Nov 2, 2022
* The tmp hardcoded WITNESS_RESERVE_VALUE allows the pool to process
  NewTemplates that cointain segwit inputs. The WITNESS_RESERVE_VALUE
value is placed in the witness field of the generated coinbase
transaction and is used to generate a correct Witness commitment hash to
be placed as one of the outputs in the coinbase.

* The hardcoding is tmp in nature as spec change is under review to send
  this field from the TemplateProvider: stratum-mining/sv2-spec#15
ccdle12 added a commit to ccdle12/stratum that referenced this pull request Nov 7, 2022
* The tmp hardcoded WITNESS_RESERVE_VALUE allows the pool to process
  NewTemplates that cointain segwit inputs. The WITNESS_RESERVE_VALUE
value is placed in the witness field of the generated coinbase
transaction and is used to generate a correct Witness commitment hash to
be placed as one of the outputs in the coinbase.

* The hardcoding is tmp in nature as spec change is under review to send
  this field from the TemplateProvider: stratum-mining/sv2-spec#15
@Fi3
Copy link
Contributor

Fi3 commented Nov 18, 2022

LGTM

@pavlenex
Copy link
Contributor

@jakubtrnka Can you also take a peek at this one when you get a chance?

Also #17 😄

@rrybarczyk
Copy link
Contributor

Looks good!

+-----------------------------+----------------+-----------------------------------------------------------------------+
| coinbase_tx_outputs | B0_64K | Bitcoin transaction outputs to be included as the last outputs in |
| | | the coinbase transaction |
| | | the coinbase transaction. An additional coinbase tx output (other than|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit.
there is no space at the end of line before the finel | bar

| | | the coinbase transaction |
| | | the coinbase transaction. An additional coinbase tx output (other than|
| | | the output which pays the fees + block subsidy), will most likely be |
| | | the coinbase witness commitment output. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is bit unclear to me. Are there any outputs except for these?
Is there any first output since we are talking about "last outputs"?

Copy link
Collaborator

@jakubtrnka jakubtrnka Dec 20, 2022

Choose a reason for hiding this comment

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

I think I understand now.

the coinbase_tx_outputs are constructed from
bytes inserted by the client (pool) that have maximum size coinbase_output_max_additional_size.
Those two arrays are concatenated <client-specified-outputs> || coinbase_tx_outputs

This statement An additional coinbase tx output will most likely be the coinbase witness commitment output is quite confusing. It sounds like there would be yet-another transaction output for witness stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

+-----------------------------+----------------+-----------------------------------------------------------------------+
| merkle_path | SEQ0_255[U256] | Merkle path hashes ordered from deepest |
+-----------------------------+----------------+-----------------------------------------------------------------------+
| witness_reserve_value | B0_255 | Optional value when a NewTemplate contains segwit inputs. The witness |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of this field.

Why would the client side need to the know witness_reserved_value?
Will it ever recalculate the witness commitment hash?

As far as I understand the client has a freedom to add additional output to the coinbase transaction with the maximum size of coinbase_output_max_additional_size. Nothing more. The structure of the block, witness data, and specifically, the witness commitment is fixed by the bitcoin core and cant be manipulated. Sending the w_reserved_value to the client then has no purpuse. Or if the client was ever to recalculate witness commitment hash. It would also need the entire witness tree.

https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#commitment-structure

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the most simple case is:

server will provide coinbase_tx_outputs (an output that pay 0 and contain the witness commitment hash).

client will create the coinbase using || coinbase_tx_outputs adding the output/s that pay the pool, so witness commitment will be the output with the highest index.

There are also cases where server want to add more than one output and in that case we need to use coinbase_tx_value_remaining.

Not sure why we need witness_reserve_value @ccdle12 ?

@pavlenex
Copy link
Contributor

pavlenex commented Jan 7, 2023

@ccdle12 Could you address @jakubtrnka and @Fi3 suggestions here please?

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.

5 participants