Upstream PR 471 (OrchardZSA) – internal review and fixes

[dmidem]

This PR mirrors zcash#471 (QED-it:zsa1 → zcash:ltf/zsa) on our fork for internal review and changes.

We’ll address upstream review comments here, then propagate the final commits and responses back to zcash#471.

[dmidem]

str4d (Link): Non-blocking: The sighash version needs to be passed into the method, because it is a property of the sighash (and only the caller knows which version was used). Given that this will need an API refactor across several crates to ensure consistency, let's not do it in this PR (which is complicated enough as it is).

[dmidem]

str4d (Link): A repr(u8) enum with unique values matching the sighashVersions cannnot work here, because:

• The v0 sighash for tx v6 will almost certainly be different from the v0 sighash for tx v7.
• No PCZT encoding has been defined yet.
• We do not know in the orchard crate what the actual encoding of the sighash version will be; it requires knowledge of the transaction version in which the sighash is used (in addition to the sighash algorithm).
• Future sighash versions may have associatedData, which cannot be conveyed in an enum like this.

Instead of using version numbers for the enum variants, they should be the semantic digest operation that is expected here; the mapping from that to actual serialized version numbers should happen in zcash_primitives. This also means the NoVersion variant is unnecessary (the difference is handled by the tx serializer).

Suggested change

	}
	/// The kind of data that a sighash commits to.
	///
	/// This is used to implement [sighash versioning] for transactions containing Orchard
	/// bundles.
	///
	/// [sighash versioning]: https://zips.z.cash/zip-0246#sighash-versioning
	#[derive(Debug, Clone, Eq, PartialEq)]
	pub enum OrchardSighashKind {
	/// The "default" sighash that commits to all effecting data of the transaction.
	///
	/// Corresponds to the Orchard parts of the following specifications:
	/// - [ZIP 244](https://zips.z.cash/zip-0244#s-4-orchard-digest)
	/// - [ZIP 246](https://zips.z.cash/zip-0246#s-4-orchard-digest)
	AllEffecting,
	}

(Then when a new digest algorithm is defined for a given tx version that commits to a subset of data, we'd add a new variant here representing the commitment to the subset of Orchard; likewise if a bug is found in a given tx version's AllEffecting committed data, we'd add a new variant representing the fixed commitment strategy.)

As a result, this entire module should be rewritten to avoid mentioning "versioning" (other than by motivating reference in documentation like above) as that is a transaction-level concern, not an Orchard bundle-level concern.

[dmidem]

str4d (Link): Document the change to this struct in the changelog.

[dmidem]

str4d (Link): This is in the wrong place. The asset for the action should be an argument of arb_unauthorized_action alongside the spend and output values, otherwise the caller cannot leverage those existing arguments in any meaningful way.

Suggested change

asset in arb_asset_base(),

[dmidem]

str4d (Link): Ditto:

Suggested change

asset in arb_asset_base(),

[dmidem]

str4d (Link): ```suggestion
//! Add chip implementation.

[dmidem]

str4d (Link): I think most of the same rationale applies here as for the Orchard sighash kind, i.e. this should be AllEffecting instead of V0.

I agree that it should be a separate enum from the Orchard sighash kind, at least for the current approach. This would in theory allow a TransactionData to be constructed that claimed to use incompatible OrchardSighashKind and IssueSighashKind values, but that should be detected and rejected at tx serialization time. I'll see if I can think of a cleaner approach...

[dmidem]

str4d (Link): ```suggestion
/// [notecommit]: https://zips.z.cash/zip-0226#note-structure-and-commitment

[dmidem]

str4d (Link): Do not remove the existing reference; it is the correct reference for $NoteCommit^Orchard$. Instead reference both locations, and also mention $NoteCommit^OrchardZSA$ in the docstring.

[dmidem]

str4d (Link): It feels very wrong for a CompactAction to be considered an "Orchard primitive", when it is actually derived from a more fundamental value (Action). This is a misunderstanding of the primitives module, and gets to a complaint I raised with you months ago that I didn't think the deletion of the note_encryption module was the right way to approach the refactor.

Non-blocking because it doesn't affect the correctness of the PR (and the history of this PR is more valuable to me than ease of introspecting the history of the note encryption logic), but be warned that I will likely go through after this PR is merged (and maybe after the corresponding librustzcash PR is merged) and undo a bunch of your refactoring (with the effect of having logically reduced the diff of this PR, even if it doesn't reduce the actual diff that gets merged).

In the meantime:

• Document these additions to the primitives module in the changelog.

[dmidem]

str4d (Link): Even though self.0 cannot be the identity, I do not like that the .unwrap()s here. We should instead just compare the byte representations of self and other.

[dmidem]

str4d (Link): Bug: this needs to reject the identity, in order to correctly enforce the requirements on AssetBase specified in ZIP 226 when parsing note plaintexts.

[dmidem]

str4d (Link): ```suggestion
/// Derives the Asset Digest for the given ZSA asset.
///
/// Defined in [ZIP-227: Issuance of Zcash Shielded Assets][assetdigest].
///
/// [assetdigest]: https://zips.z.cash/zip-0227#asset-digests

[dmidem]

str4d (Link): This function mis-represents the relationship between its arguments. version is not an arbitrary parameter independent from ik and asset_desc_hash: by definition, if version != 0, the ik and asset_desc_hash parameters might not exist.

Either remove the version parameter and internally hard-code it to 0 (making this function solely for the version 0 protocol), or replace all of the arguments with an AssetId enum:

pub enum AssetId {
    V0 {
        ik: IssueValidatingKey<ZSASchnorr>,
        asset_desc_hash: [u8; 32],
    },
}

and then make this function a method of AssetId (as well as asset_digest).

[dmidem]

str4d (Link): While you're making changes here, update the variable names to match ZIP 227:

Suggested change

	asset_id.extend(ik_encoding);
	let issuer = ik.encode();
	let mut asset_id = Vec::with_capacity(1 + issuer.len() + asset_desc_hash.len());
	asset_id.push(version);
	asset_id.extend(issuer);

[dmidem]

str4d (Link): I don't like this being a public method and taking an arbitrary unconstrained slice. If you take my earlier suggestion of introducing an AssetId enum with this as one of its methods, then this would instead take &self which is fine.

[dmidem]

str4d (Link): This should be named AssetBase::custom rather than AssetBase::derive, to match the "Custom Asset" terminology from ZIP 227.

If my enum suggestion is taken:

Suggested change

	let asset_digest = asset_digest(&asset_id);
	pub fn custom(asset_id: &AssetId) -> Self
	let asset_digest = asset_id.asset_digest();

[dmidem]

str4d (Link): ```suggestion
/// Defined in [ZIP 227: Issuance of Zcash Shielded Assets][assetbase].
///
/// [assetbase]: https://zips.z.cash/zip-0227#asset-bases

[dmidem]

str4d (Link): I just noticed that ZIP 227's terminology section defines "Native Asset" as "a Custom Asset with issuance defined on the Zcash blockchain" (first introduced in QED-it/zips@97eacab), where "Custom Asset" is "any Asset other than ZEC and TAZ" (first introduced in QED-it/zips@8ed51c2). Thus the entire usage of native / is_native in this PR is inconsistent with ZIP 227, and should be reworked to use a different term.

Non-blocking for this PR, but blocking the first orchard release containing this PR; open an issue for addressing this.

[dmidem]

str4d (Link): The slice notation should not be necessary hereh (it isn't present in the existing equivalent call that is deleted in this PR):

Suggested change

	)(&NATIVE_ASSET_BASE_V_BYTES[..]))
	)(&NATIVE_ASSET_BASE_V_BYTES))

[dmidem]

str4d (Link): Nit: we should only get arbitrary (isk, asset_desc_hash) if is_native; the generation should be conditional. I think you do this with a second sequence of arguments? We essentially want the equivalent of (!is_native).then(|| arb_zsa_asset_base()).

[dmidem]

From upstream review (reply; parent not copied inline)

str4d (Link): In a pairing with daira, we also noted that currently ZIP 230 defines valueBurn: u64, but allowing that full range now is annoying because every other value balance is an i64, and it would force us to add a bunch of extra handling logic that would only be used for burns (and would be incompatible with future asset-specific value balances when we eventually have a subsequent pool that supports ZSAs via a turnstile).

For simplicity, we should update ZIP 230 to only allow burn amounts that fit into both a u64 and an i64, i.e. u63.

[dmidem]

Closed in favour of #200