Address review comments from zcash PR #471 (2)

.github/workflows/ci.yml

@@ -84,6 +84,7 @@ jobs:
         run: |
           cargo update -p unicode-ident --precise 1.0.12
           cargo update -p syn --precise 2.0.114
+          cargo update -p quote --precise 1.0.44
       - name: Build no_std for target
         working-directory: ./ci-build
         run: cargo build --verbose --target ${{ matrix.target }}

.github/workflows/lints-stable.yml

@@ -61,6 +61,7 @@ jobs:
         run: |
           cargo update -p unicode-ident --precise 1.0.12
           cargo update -p syn --precise 2.0.114
+          cargo update -p quote --precise 1.0.44
       - name: Clippy no_std for target
         working-directory: ./ci-build
         run: cargo clippy --verbose --target ${{ matrix.target }}

src/bundle/commitments/issuance.rs

@@ -44,7 +44,7 @@ pub(crate) fn hash_issue_bundle_txid_data<A: IssueAuth>(bundle: &IssueBundle<A>)
             ind.update(note.rseed().as_bytes());
         }
         ia.update(ind.finalize().as_bytes());
-        ia.update(&[u8::from(action.is_finalized())]);
+        ia.update(&[action.flags().to_byte()]);
     }
     h.update(ia.finalize().as_bytes());
     h.finalize()

src/circuit.rs

@@ -290,7 +290,7 @@ impl VerifyingKey {
 
 /// The proving key for the Orchard Action circuit.
 ///
-/// In the current type system, this could be a verifying key for either
+/// In the current type system, this could be a proving key for either
 /// the original Orchard Action circuit, or the OrchardZSA circuit.
 #[derive(Debug)]
 pub struct ProvingKey {

src/issuance.rs

@@ -829,7 +829,7 @@ impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             IssueActionNotFound => {
-                write!(f, "the requested IssueAction not exists in the bundle.")
+                write!(f, "the requested IssueAction does not exist in the bundle.")
             }
             IssueBundleIkMismatchAssetBase => {
                 write!(

src/issuance/sighash_kind.rs

@@ -8,6 +8,8 @@ use alloc::vec::Vec;
 /// The kind of data that a sighash commits to.
 ///
 /// This is used to implement [sighash versioning] for issuance transactions.
+///
+/// [sighashversioning]: https://zips.z.cash/zip-0246#sighash-versioning
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub enum IssueSighashKind {
     /// The "default" sighash that commits to all effecting data of the transaction, as defined in
@@ -48,7 +50,7 @@ pub type BIP340IssueAuthSig = IssueSig<ZSASchnorr>;
 ///
 /// This helper is only intended for use in tests.
 #[cfg(test)]
-pub fn test_sighash_info_for_kind(kind: &IssueSighashKind) -> Vec<u8> {
+pub(crate) fn test_sighash_info_for_kind(kind: &IssueSighashKind) -> Vec<u8> {
     match kind {
         IssueSighashKind::AllEffecting => vec![0],
     }

src/keys.rs

@@ -1,7 +1,6 @@
 //! Key structures for Orchard.
 
 use alloc::vec::Vec;
-use core::fmt::Debug;
 use core2::io::{self, Read, Write};
 
 use ::zip32::ChildIndex;

src/note.rs

@@ -360,7 +360,7 @@ impl Note {
     }
 
     #[cfg(test)]
-    pub fn has_rho(&self) -> bool {
+    pub(crate) fn has_rho(&self) -> bool {
         self.rho.is_some()
     }
 

src/note/asset_base.rs

@@ -59,7 +59,7 @@ impl<'a> AssetId<'a> {
         }
     }
 
-    /// Derives the Asset Digest for the given ZSA asset.
+    /// Derives the Asset Digest for this ZSA asset.
     ///
     /// Defined in [ZIP-227: Issuance of Zcash Shielded Assets][assetdigest].
     ///
@@ -195,6 +195,10 @@ pub mod testing {
 
     use proptest::prelude::*;
 
+    use crate::constants::fixed_bases::ZSA_ASSET_BASE_PERSONALIZATION;
+    use group::Group;
+    use pasta_curves::{arithmetic::CurveExt, pallas};
+
     prop_compose! {
         /// Generate a uniformly distributed asset base.
         pub fn arb_asset_base()
@@ -215,22 +219,15 @@ pub mod testing {
         /// it is sufficient to use a random asset digest. This allows generating a random
         /// `AssetBase` even when `zsa-issuance` feature is disabled.
         pub fn arb_zsa_asset_base()(
-            asset_digest in any::<[u8; 64]>(),
+            asset_digest in any::<[u8; 64]>().prop_filter("hash_to_curve must not be identity", |digest| {
+                // Reject the extremely unlikely case where `hash_to_curve` returns the
+                // identity point. `prop_filter` makes proptest discard such inputs and
+                // regenerate new ones instead of failing the test.
+                let asset_base = pallas::Point::hash_to_curve(ZSA_ASSET_BASE_PERSONALIZATION)(digest);
+                !bool::from(asset_base.is_identity())
+            })
         ) -> AssetBase {
-            use crate::constants::fixed_bases::ZSA_ASSET_BASE_PERSONALIZATION;
-            use group::Group;
-            use pasta_curves::{arithmetic::CurveExt, pallas};
-
-            let asset_base = loop {
-                let asset_base =
-                pallas::Point::hash_to_curve(ZSA_ASSET_BASE_PERSONALIZATION)(&asset_digest);
-
-                // Extremely unlikely, but explicitly reject the identity point.
-                if bool::from(!asset_base.is_identity()) {
-                    break asset_base;
-                }
-            };
-
+            let asset_base = pallas::Point::hash_to_curve(ZSA_ASSET_BASE_PERSONALIZATION)(&asset_digest);
             AssetBase(asset_base)
         }
     }

src/primitives/orchard_primitives_vanilla.rs

@@ -94,6 +94,12 @@ impl OrchardPrimitives for OrchardVanilla {
     /// Evaluate `orchard_auth_digest` for the bundle as defined in
     /// [ZIP-244: Transaction Identifier Non-Malleability][zip244]
     ///
+    /// # Panics
+    ///
+    /// Panics if any signature in the bundle uses a sighash kind different from
+    /// `OrchardSighashKind::AllEffecting`. In Orchard v5 transactions, this is the
+    /// only defined sighash kind.
+    ///
     /// [zip244]: https://zips.z.cash/zip-0244
     fn hash_bundle_auth_data<V>(
         bundle: &Bundle<Authorized, V, OrchardVanilla>,
@@ -102,8 +108,16 @@ impl OrchardPrimitives for OrchardVanilla {
         let mut h = hasher(ZCASH_ORCHARD_SIGS_HASH_PERSONALIZATION);
         h.update(bundle.authorization().proof().as_ref());
         for action in bundle.actions().iter() {
+            assert_eq!(
+                *action.authorization().sighash_kind(),
+                OrchardSighashKind::AllEffecting
+            );
             h.update(&<[u8; 64]>::from(action.authorization().sig()));
         }
+        assert_eq!(
+            *bundle.authorization().binding_signature().sighash_kind(),
+            OrchardSighashKind::AllEffecting
+        );
         h.update(&<[u8; 64]>::from(
             bundle.authorization().binding_signature().sig(),
         ));

src/primitives/zcash_note_encryption_domain.rs

@@ -1,8 +1,6 @@
 //! This module implements `Domain` and `BatchDomain` traits from the `zcash_note_encryption`
 //! crate and contains the common logic for `OrchardVanilla` and `OrchardZSA` flavors.
 
-// Review hint: this file is largely derived from src/note_encryption.rs
-
 use alloc::vec::Vec;
 use blake2b_simd::{Hash, Params};
 use group::ff::PrimeField;

src/sighash_kind.rs

@@ -9,6 +9,8 @@ use alloc::vec::Vec;
 ///
 /// This is used to implement [sighash versioning] for transactions containing Orchard
 /// bundles.
+///
+/// [sighashversioning]: 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, as defined in
@@ -52,7 +54,7 @@ pub type OrchardBindingSig = OrchardSig<Binding>;
 ///
 /// This helper is only intended for use in tests.
 #[cfg(test)]
-pub fn test_sighash_info_for_kind(kind: &OrchardSighashKind) -> Vec<u8> {
+pub(crate) fn test_sighash_info_for_kind(kind: &OrchardSighashKind) -> Vec<u8> {
     match kind {
         OrchardSighashKind::AllEffecting => vec![0],
     }