Skip to content
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

[ZIP 230, ZIP 246] v6 transaction format and sighash #987

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

str4d
Copy link
Collaborator

@str4d str4d commented Feb 26, 2025

Incomplete todo list:

  • Specify sighash versioning process (it is currently just a rough summary).

@str4d str4d marked this pull request as draft February 26, 2025 23:13
Signed-off-by: Daira-Emma Hopwood <[email protected]>
clean up some formatting.

Signed-off-by: Daira-Emma Hopwood <[email protected]>
Copy link
Contributor

@vivek-arte vivek-arte left a comment

Choose a reason for hiding this comment

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

One of the changes relevant for the sighash and transaction format is the move of the burn field information into the action groups.

Note that this is independent of any subsequent versioning etc for Swaps specific changes, it is natural to have the burn information a part of each Action Group.

I've made those changes as suggestions in this review, along with a few smaller suggestions.

Comment on lines +152 to +156
| varies | ``nAssetBurn`` | ``compactSize`` |The number of Assets burnt. |
+-----------------------------+------------------------------+------------------------------------------------+---------------------------------------------------------------------+
| 40 × ``nAssetBurn`` | ``vAssetBurn`` | ``AssetBurn[nAssetBurn]`` |A sequence of Asset Burn descriptions, encoded per |
| | | | encoded per `OrchardZSA Asset Burn Description`_. |
+-----------------------------+------------------------------+------------------------------------------------+---------------------------------------------------------------------+
Copy link
Contributor

Choose a reason for hiding this comment

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

This corresponds to the move of the asset burn fields into the Action Group. We move this down into the OrchardZSA Action Group Description.

Suggested change
| varies | ``nAssetBurn`` | ``compactSize`` |The number of Assets burnt. |
+-----------------------------+------------------------------+------------------------------------------------+---------------------------------------------------------------------+
| 40 × ``nAssetBurn`` | ``vAssetBurn`` | ``AssetBurn[nAssetBurn]`` |A sequence of Asset Burn descriptions, encoded per |
| | | | encoded per `OrchardZSA Asset Burn Description`_. |
+-----------------------------+------------------------------+------------------------------------------------+---------------------------------------------------------------------+

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As noted in #960 (comment), this change requires further discussion. In particular, no rationale has been provided for this move (which should be in ZIP 227), and there's been no discussion anywhere I'm aware of about the privacy implications of this change, or how it impacts the ability of the ActionGroup bundler to decide which ActionGroups to bundle (AFAICT after this change, the bundler would need to contribute their own ActionGroup in order to handle any mismatch that requires altering burns, which increases both the size and fee of the transaction beyond what the bundled ActionGroups would otherwise cost).

Copy link
Contributor

Choose a reason for hiding this comment

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

  • For the NU7 setting, there is no impact as such since the bundling continues to be done by the party creating the component action groups.
  • For the subsequent Swaps addition, we already expect the bundler to create their own Action Group in order to collect any "matching fees", so this is not a cost that is not already part of the Swaps mechanism.

any mismatch that requires altering burns

An example is when someone creates an Action Group with burn fields corresponding to Assets they don’t have. If the bundler wants to accept such an Action Group by adding the necessary Assets to the transaction themselves, they are free to do so (though I don't see why they would?). If not, they are free to ignore the Action Group that doesn’t balance out, so such Action Groups would never be included. Did you have a different scenario in mind?

The motivation for having burn information inside the Action Group is to allow parties to be able to burn Assets as well, otherwise the burning ability remains only with the bundler. (I agree this rationale should be added)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I suppose it is better to have this discussion in its own PR rather than here?

We already have this change up in #976 building on top of main, though it does not have the ZIP 246 changes which are part of this PR. I also opened #991 that builds on top of this PR (#987) instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, these changes should have initially been proposed in their own PR (#976 was not such a PR, as it also included other unrelated changes). The discussion continues here: https://github.com/zcash/zips/pull/991/files#r1990147143

Comment on lines +250 to +252
T.4a.iii : flagsOrchard (1 byte)
T.4a.iv : anchorOrchard (32 bytes)
T.4a.v : nAGExpiryHeight (4 bytes)
Copy link
Contributor

Choose a reason for hiding this comment

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

This corresponds to the move of the asset burn fields into the Action Group.

Suggested change
T.4a.iii : flagsOrchard (1 byte)
T.4a.iv : anchorOrchard (32 bytes)
T.4a.v : nAGExpiryHeight (4 bytes)
T.4a.iii : orchard_burn_digest (32-byte hash output)
T.4a.iv : flagsOrchard (1 byte)
T.4a.v : anchorOrchard (32 bytes)
T.4a.vi : nAGExpiryHeight (4 bytes)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is now proposed and discussed in #991

.. [#zip-0317-fee-calc] `ZIP 317: Proportional Transfer Fee Mechanism, Fee calculation <zip-0317.html#fee-calculation>`_
.. [#zip-0246] `ZIP 246: Digests for the Version 6 Transaction Format <zip-0246.html>`_
.. [#zip-0317] `ZIP 317: Proportional Transfer Fee Mechanism <zip-0317.html>`_
.. [#zip-0317-fee-calculation] `ZIP 317: Proportional Transfer Fee Mechanism - Fee calculation <zip-0317.html#fee-calculation>`_
.. [#bip-0043] `BIP 43: Purpose Field for Deterministic Wallets <https://github.com/bitcoin/bips/blob/master/bip-0043.mediawiki>`_
.. [#bip-0340] `BIP 340: Schnorr Signatures for secp256k1 <https://github.com/bitcoin/bips/blob/200f9b26fe0a2f235a2af8b30c4be9f12f6bc9cb/bip-0340.mediawiki>`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this is referring to a specific commit? We usually just reference master (in this case it looks like it there are only editorial changes since that commit).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That question is irrelevant to this PR. Please open a separate issue and tag qedit into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can switch to referencing master if that is the usual convention.

I used the commit for similar reasons to referencing a specific commit in the zcash-test-vectors repository when using the bip340.py code from the bips repository. The specification BIP though should not need that.

This can be switched here, as suggested, or in a separate PR, or via opening a separate issue, as the ZIP Editors feel fit.

Suggested change
.. [#bip-0340] `BIP 340: Schnorr Signatures for secp256k1 <https://github.com/bitcoin/bips/blob/200f9b26fe0a2f235a2af8b30c4be9f12f6bc9cb/bip-0340.mediawiki>`_
.. [#bip-0340] `BIP 340: Schnorr Signatures for secp256k1 <https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki>`_

- Sighash versions are numbered starting from 0 for each tx version.
- v0 is by convention the "commit to everything" sighash digest. Other versions can commit to whatever makes sense for desired functionality within that tx version.
- Have a single byte encoded alongside the signature (not appended the way transparent does) that permits the signer to specify which sighash version they are using.
- Consensus rules choose the digest algorithm for each signer based on that byte.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should clarify that the consensus rules choose from the available digest algorithms for the (consensus_branch_id, transaction_version) tuple under question (both of which are committed to by the transaction, and can therefore be checked non-contextually). For the NU7 consensus branch ID, there will only be one available digest version for v6 transactions (the v0 digests).


- Sighash versions are numbered starting from 0 for each tx version.
- v0 is by convention the "commit to everything" sighash digest. Other versions can commit to whatever makes sense for desired functionality within that tx version.
- Have a single byte encoded alongside the signature (not appended the way transparent does) that permits the signer to specify which sighash version they are using.
Copy link
Collaborator Author

@str4d str4d Mar 11, 2025

Choose a reason for hiding this comment

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

In ZIP Sync, we discussed how this should be represented. We identified that one of the issues we have with how transparent sighash types are handled is that they are considered authorizing data (they are appended to signatures and are thus part of the signature, chosen at signing time by the signer). We decided that for sighash versioning, we'd prefer them to be effecting data. This means:

  • For Sapling spendAuthSig, this would be an extra byte on the Spend Description.
  • For Orchard spendAuthSig, this would be an extra byte on the Action Description.
  • For bindingSigs, we place an extra byte in the corresponding bundles.
  • For transparent, due to the lack of a 1:1 between inputs and signatures (as an arbitrary number of signatures can exist within a scriptSig), this would be an extra byte on the CTxIn that is used for every signature authorizing the input.
    • @daira doesn't think we need it at the per-input level for transparent (Ya Ain't Gonna Need It), while @str4d says this is useful for symmetry (the effecting data is then consistently at the per-input level) and that we are trying to create a design that doesn't restrict what kinds of sighash digest algorithms we might want to create in future. @nuttycom is happy to have it at the per-input level for now, and we can adjust if we discover difficulties.
    • Note that there is no requirement for future sighash digest algorithms to pay any attention to the sighash_type appended to each signature. It is entirely fine for us to treat that as a v0-specific behaviour (or even decide to phase it out in future transaction versions, though we are explicitly choosing to keep it for the v6 transaction's v0 digest algorithm).

Copy link
Contributor

@vivek-arte vivek-arte left a comment

Choose a reason for hiding this comment

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

Ran into some issues generating the rendered HTML files in #991. The suggestions in this review are those relevant to this PR (#987).

.. [#zip-0230-issue-notes-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: Issue Note Description <zip-0230.rst#issue-note-description>`_
.. [#zip-0244] `ZIP 244: Transaction Identifier Non-Malleability <zip-0244.rst>`_
.. [#zip-0244-sigdigest] `ZIP 244: Transaction Identifier Non-Malleability: Signature Digest <zip-0244.html#signature-digest>`_
.. [#zip-0244-authcommitment] `ZIP 244: Transaction Identifier Non-Malleability: Authorizing Data Commitment <zip-0244.html#authorizing-data-commitment>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

There are links to citations of ZIP 307 on L195, L213, L262 and L281 which do not have a reference here.

Suggested change
.. [#zip-0244-authcommitment] `ZIP 244: Transaction Identifier Non-Malleability: Authorizing Data Commitment <zip-0244.html#authorizing-data-commitment>`_
.. [#zip-0244-authcommitment] `ZIP 244: Transaction Identifier Non-Malleability: Authorizing Data Commitment <zip-0244.html#authorizing-data-commitment>`_
.. [#zip-0307] `ZIP 307: Light Client Protocol for Payment Detection <zip-0307.rst>`_


"ZTxId6SOutC_Hash" (1 underscore character)

The field encodings are specified in ZIP 230 [#zip-0230-issuance-field-encodings]_.
Copy link
Contributor

Choose a reason for hiding this comment

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

This link is not present in the references. Should it instead be:

Suggested change
The field encodings are specified in ZIP 230 [#zip-0230-issuance-field-encodings]_.
The field encodings are specified in §7.4 ‘Output Description Encoding and Consensus’ of the Zcash Protocol Specification [#protocol-outputdesc]_.


"ZTxId6SOutN_Hash" (1 underscore character)

The field encodings are specified in ZIP 230 [#zip-0230-issuance-field-encodings]_.
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

Suggested change
The field encodings are specified in ZIP 230 [#zip-0230-issuance-field-encodings]_.
The field encodings are specified in §7.4 ‘Output Description Encoding and Consensus’ of the Zcash Protocol Specification [#protocol-outputdesc]_.


BLAKE2b-256("ZTxIdMemo___Hash", [])

The field encodings are specified in ZIP 230 [#zip-0230-issuance-field-encodings]_.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should correspond to this section of ZIP 231 (which defines nonceOrHash rather than nonce though)

Suggested change
The field encodings are specified in ZIP 230 [#zip-0230-issuance-field-encodings]_.
The field encodings are specified in ZIP 231 [#zip-0231-encoding-in-transactions]_.

.. [#zip-0230-orchard-action-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: OrchardZSA Action Description <zip-0230.rst#orchardzsa-action-description>`_
.. [#zip-0230-orchard-asset-burn-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: OrchardZSA Asset Burn Description <zip-0230.rst#orchardzsa-asset-burn-description>`_
.. [#zip-0230-issue-actions-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: Issuance Action Description <zip-0230.rst#issuance-action-description>`_
.. [#zip-0230-issue-notes-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: Issue Note Description <zip-0230.rst#issue-note-description>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. [#zip-0230-issue-notes-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: Issue Note Description <zip-0230.rst#issue-note-description>`_
.. [#zip-0230-issue-notes-field-encodings] `ZIP 230: Version 6 Transaction Format. Specification: Issue Note Description <zip-0230.rst#issue-note-description>`_
.. [#zip-0231-encoding-in-transactions] `ZIP 231: Memo Bundles. Encoding in transactions <zip-0231.md#encodingintransactions>


S.5: issuance_digest
````````````````````
Identical to the ``issuance_digest`` specified for the transaction identifier in ZIP 227 [zip-0227-txiddigest]_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since issuance_digest is now in the same file (i.e. ZIP 246):

Suggested change
Identical to the ``issuance_digest`` specified for the transaction identifier in ZIP 227 [zip-0227-txiddigest]_.
Identical to that specified for the transaction identifier.

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.

3 participants