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

ZSA Protocol: Transfer, Issuance and Burn: ZIPs 226, 227, and 230 #960

Closed
wants to merge 2 commits into from

Conversation

Copy link
Collaborator

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Reviewed 2ef0f1a in ZIP Sync with @nuttycom, @arya2, @conradoplg, @daira, and @SamHSmith. The "Note to ZIP Editors" comments are not blocking and should not be addressed in this PR.

Comment on lines 229 to 226
The encoding of Sapling Spends and Outputs has changed relative to prior versions in order
to better separate data that describe the effects of the transaction from the proofs of and
commitments to those effects, and for symmetry with this separation in the Orchard-related parts
of the transaction format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to ZIP Editors: this paragraph is copy-pasta-ed from the v5 tx format ZIP where this change occurred; it isn't relevant here, and we will reword it when we start defining the final v6 tx format.

+------------------------------------+--------------------------+--------------------------------------------------+---------------------------------------------------------------------+
|``sizeProofsOrchard`` |``proofsOrchard`` |``byte[sizeProofsOrchard]`` |The aggregated zk-SNARK proof for all Actions in the Action Group. |
+------------------------------------+--------------------------+--------------------------------------------------+---------------------------------------------------------------------+
|``4`` |``nAGExpiryHeight`` |``uint32`` |A block height (in the future) after which the Actions of the |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to ZIP Editors: this is effecting data, not authorizing data, so it will be moved to earlier in the ActionGroup format.

- It MUST be the case that $0 < \mathtt{assetDescSize} \leq 512$.
- It MUST be the case that $\mathsf{asset\_desc}$ is a string of length $\mathtt{assetDescSize}$ bytes.
- Every Issue Note in ``IssueAction`` MUST be a valid encoding of the $\mathsf{Note^{Issue}}$ type, and MUST encode the same $\mathsf{AssetBase}$.
- This $\mathsf{AssetBase}$ MUST satisfy the derivation from the issuance validating key and asset description described in the `Specification: Asset Identifier, Asset Digest, and Asset Base`_ section.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to ZIP Editors: we will remove the explicit AssetBase from the v6 tx format, along with this consensus rule and the MUST encode from the previous rule.

Issuance bundles include both ik and asset_desc, meaning that AssetBase can always be derived. No computation is saved in consensus rules from encoding it due to the need to check the validity, so we may as well just use the output of the computation directly.


- For every issue note description ($\mathsf{note}_{\mathsf{j}},\ 1 \leq j \leq \mathtt{nNotes}$) in ``IssueAction``:

- The $\text{ρ}$ field of the issue note MUST have been computed as described in the `Computation of ρ`_ section.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to ZIP Editors: consider whether rho should just be derived instead of encoded and checked (like will be done for AssetBase).

In general, the places we have regretted not explicitly encoding something is where the ability to derive it depends on non-local information (e.g. fees depend on looking up prior transparent transaction outputs). Here, both assetBase and rho can be computed solely from information present in the transaction. (There might however be a question wrt compact blocks as to whether there is sufficient information; consider this when deciding the final v6 tx format.)

- The issuance key structure is independent of the original key tree, but derived in an analogous manner (via ZIP 32). This is in order to keep the issuance details and the Asset Identifiers consistent across multiple shielded pools.
- The design decision is not to have a chosen name to describe the Custom Asset, but to delegate it to an off-chain mapping, as this would imply a land-grab “war”.
- The issuance key structure is independent of the original key tree, but derived in an analogous manner (via ZIP 32). This keeps the issuance details and the Asset Identifiers consistent across multiple shielded pools. It also separates the issuance authority from the spend authority, allowing for the potential transfer of issuance authority without compromising the spend authority.
- The Custom Asset is described via a combination of the issuance validating key and an asset description string, to preclude the possibility of two different issuers creating colliding Custom Assets.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably want to mention wallet-level considerations here (e.g. problems with an issue issuing two assets that only differ in JSON whitespace in asset_desc), as well as maybe allude to future issuance designs where collision / sharing of an asset ID is desired.

TODO for @daira to write a Security Considerations / Rationale section for how asset_desc is included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have a Security Considerations section at the end of the ZIP, see L692-710. Also, some considerations for wallets to follow are provided L229-232, and linked in the Security and Privacy Considerations section as well.

vivek-arte added a commit to QED-it/zips that referenced this pull request Feb 4, 2025
This makes various changes based on the comments on zcash#960 made
during ZIP Editor syncs.
This shifts the information pertaining to the burning of assets into
the OrchardZSA Action Group -- this occurs both in the Transaction
Format and the computation of the TxID digest.
@str4d
Copy link
Collaborator

str4d commented Feb 4, 2025

The ZIP Editors have decided to not merge the changes from QED-it#97 yet as they need further discussion (as it's another semantic change to the ZSA ZIPs beyond the semantic stability point). As we can't alter this PR directly, we're merging the previous commit (b5b434c) into main to "close" this PR (and unblock us from v6 tx work), and then cc5b418 can be considered separately.

@str4d str4d mentioned this pull request Feb 4, 2025
str4d added a commit that referenced this pull request Feb 4, 2025
@str4d
Copy link
Collaborator

str4d commented Feb 4, 2025

The commits up to and including b5b434c were merged in #974. This PR should either be closed, or rebased to just contain the new semantic change.

In general, please open separate PRs for new semantic changes rather than pushing them to an existing PR that is in the middle of review (unless the semantic change is requested as part of that review).

This merges the changes to the repository upstream into the `zsa1`
branch
@vivek-arte
Copy link
Contributor Author

vivek-arte commented Feb 6, 2025

I am closing this in favour of #976

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.

4 participants