PR471 review: various comments (#228)

Address the following review comments:
r2542313926
r2550287842
r2602977967
r2543702226
r2542907997
r2585555720 
r2602558531

Author: ConstanceBeguier

book/src/design/actions.md

@@ -29,11 +29,9 @@ we do not verify that the corresponding spent note commitment is part of the Mer
 
 ## Split notes for OrchardZSA
 
-For OrchardZSA, if the number of inputs exceeds the number of outputs,
-we use dummy output notes (as in Orchard) to fill all actions.
-Conversely, if the number of outputs exceeds the number of inputs, we use split notes to fill the actions.
 In OrchardZSA, ensuring that the AssetBase is correctly created is crucial.
-For this reason, split notes are used instead of dummy spent notes.
+For this reason, when inputs and outputs are unbalanced, actions are completed using
+split notes for inputs or dummy notes for outputs.
 Split notes are essentially duplicates of actual spent notes,
 but with the following differences:
 - The nullifier is randomized to prevent it from being treated as double-spending.

src/builder.rs

@@ -22,7 +22,7 @@ use crate::{
     note::{AssetBase, ExtractedNoteCommitment, Note, Nullifier, Rho, TransmittedNoteCiphertext},
     primitives::redpallas::{self, Binding, SpendAuth},
     primitives::{OrchardDomain, OrchardPrimitives},
-    sighash_versioning::{VerBindingSig, VerSpendAuthSig},
+    sighash_versioning::{OrchardSighashVersion, VerBindingSig, VerSpendAuthSig},
     tree::{Anchor, MerklePath},
     value::{self, NoteValue, OverflowError, ValueCommitTrapdoor, ValueCommitment, ValueSum},
     Proof,
@@ -699,6 +699,11 @@ impl Builder {
             return Err(BuildError::Burn(BurnError::ZeroAmount));
         }
 
+        // Check that the burn amount fits in u63
+        if value.inner() >= (1u64 << 63) {
+            return Err(BuildError::Burn(BurnError::InvalidAmount));
+        }
+
         match self.burn.entry(asset) {
             Entry::Occupied(_) => Err(BuildError::Burn(BurnError::DuplicateAsset)),
             Entry::Vacant(entry) => {
@@ -1067,7 +1072,7 @@ fn build_bundle<B, R: RngCore>(
 /// Marker trait representing bundle signatures in the process of being created.
 pub trait InProgressSignatures: fmt::Debug {
     /// The authorization type of an Orchard action in the process of being authorized.
-    type SpendAuth: fmt::Debug + Clone;
+    type SpendAuth: fmt::Debug;
 }
 
 /// Marker for a bundle in the process of being built.
@@ -1296,6 +1301,9 @@ impl<P: fmt::Debug, V, Pr: OrchardPrimitives> Bundle<InProgress<P, PartiallyAuth
     }
 
     fn append_signature(self, signature: &VerSpendAuthSig) -> Result<Self, BuildError> {
+        if signature.version() != &OrchardSighashVersion::V0 {
+            return Err(BuildError::InvalidExternalSignature);
+        }
         let mut signature_valid_for = 0usize;
         let bundle = self.map_authorization(
             &mut signature_valid_for,

src/bundle.rs

@@ -202,7 +202,7 @@ impl Flags {
 /// Defines the authorization type of an Orchard bundle.
 pub trait Authorization: fmt::Debug {
     /// The authorization type of an Orchard action.
-    type SpendAuth: fmt::Debug + Clone;
+    type SpendAuth: fmt::Debug;
 }
 
 /// A bundle of actions to be applied to the ledger.

src/bundle/burn_validation.rs

@@ -17,9 +17,12 @@ pub enum BurnError {
     NativeAsset,
     /// Cannot burn an asset with a zero value.
     ZeroAmount,
+    /// Burn amount does not fit in u63.
+    InvalidAmount,
 }
 
-/// Validates burn for a bundle by ensuring each asset is unique, non-native, and has a non-zero value.
+/// Validates burn for a bundle by ensuring each asset is unique, non-native, fit in u63 and has a
+/// non-zero value.
 ///
 /// Each burn element is represented as a tuple of `AssetBase` and `NoteValue` (value for the burn).
 ///
@@ -32,6 +35,7 @@ pub enum BurnError {
 /// Returns a `BurnError` if:
 /// * Any asset in the `burn` vector is native (`BurnError::NativeAsset`).
 /// * Any asset in the `burn` vector has a zero value (`BurnError::ZeroAmount`).
+/// * Any burn amount in the `burn` vector is out of the u63 range (`BurnError::InvalidAmount`).
 /// * Any asset in the `burn` vector is not unique (`BurnError::DuplicateAsset`).
 pub fn validate_bundle_burn(burn: &[(AssetBase, NoteValue)]) -> Result<(), BurnError> {
     let mut burn_set = BTreeSet::new();
@@ -43,6 +47,9 @@ pub fn validate_bundle_burn(burn: &[(AssetBase, NoteValue)]) -> Result<(), BurnE
         if value.inner() == 0 {
             return Err(BurnError::ZeroAmount);
         }
+        if value.inner() >= (1u64 << 63) {
+            return Err(BurnError::InvalidAmount);
+        }
         if !burn_set.insert(*asset) {
             return Err(BurnError::DuplicateAsset);
         }
@@ -59,6 +66,9 @@ impl fmt::Display for BurnError {
             BurnError::ZeroAmount => {
                 write!(f, "Cannot burn an asset with a zero value.")
             }
+            BurnError::InvalidAmount => {
+                write!(f, "Burn amount must fit in u63.")
+            }
         }
     }
 }
@@ -148,4 +158,20 @@ mod tests {
 
         assert_eq!(result, Err(BurnError::ZeroAmount));
     }
+
+    #[test]
+    fn validate_bundle_burn_invalid_amount() {
+        let mut rng = OsRng;
+        let mut used = BTreeSet::new();
+
+        let bundle_burn = vec![
+            burn_tuple_unique(&mut rng, &mut used, 10),
+            burn_tuple_unique(&mut rng, &mut used, 1u64 << 63),
+            burn_tuple_unique(&mut rng, &mut used, 10),
+        ];
+
+        let result = validate_bundle_burn(&bundle_burn);
+
+        assert_eq!(result, Err(BurnError::InvalidAmount));
+    }
 }

src/note/asset_base.rs

@@ -1,6 +1,6 @@
 use core::cmp::Ordering;
 use core::hash::{Hash, Hasher};
-use group::GroupEncoding;
+use group::{Group, GroupEncoding};
 use pasta_curves::{arithmetic::CurveExt, pallas};
 use subtle::{Choice, ConstantTimeEq, CtOption};
 
@@ -9,9 +9,6 @@ use crate::constants::fixed_bases::{NATIVE_ASSET_BASE_V_BYTES, VALUE_COMMITMENT_
 #[cfg(test)]
 use rand_core::CryptoRngCore;
 
-#[cfg(any(test, feature = "zsa-issuance"))]
-use group::Group;
-
 #[cfg(feature = "zsa-issuance")]
 use {
     crate::constants::fixed_bases::ZSA_ASSET_BASE_PERSONALIZATION,
@@ -100,8 +97,12 @@ pub const ZSA_ASSET_DIGEST_PERSONALIZATION: &[u8; 16] = b"ZSA-Asset-Digest";
 
 impl AssetBase {
     /// Deserialize the AssetBase from a byte array.
+    ///
+    /// Returns `None` if the byte encoding is invalid or if it corresponds
+    /// to the identity point.
     pub fn from_bytes(bytes: &[u8; 32]) -> CtOption<Self> {
-        pallas::Point::from_bytes(bytes).map(AssetBase)
+        pallas::Point::from_bytes(bytes)
+            .and_then(|asset| CtOption::new(AssetBase(asset), !asset.is_identity()))
     }
 
     /// Serialize the AssetBase to its canonical byte representation.

src/value.rs

@@ -490,6 +490,7 @@ mod tests {
     };
     use crate::{
         note::asset_base::testing::arb_asset_base, note::AssetBase, primitives::redpallas,
+        value::NoteValue,
     };
 
     fn negate_value_sum(value: ValueSum) -> ValueSum {
@@ -503,20 +504,46 @@ mod tests {
         ValueSum::from_magnitude_sign(magnitude, neg_sign)
     }
 
+    fn arb_asset_values_balanced_per_asset(
+        bound: NoteValue,
+        n_assets: usize,
+    ) -> impl Strategy<Value = Vec<(ValueSum, ValueCommitTrapdoor, AssetBase)>> {
+        (
+            prop::collection::vec(arb_asset_base(), n_assets),
+            prop::collection::vec(arb_value_sum_bounded(bound), n_assets * 4),
+            prop::collection::vec(arb_trapdoor(), n_assets * 5),
+        )
+            .prop_map(move |(assets, vals, traps)| {
+                let mut traps = traps.into_iter();
+                let mut out = Vec::with_capacity(n_assets * 5);
+
+                for (asset, four_values) in assets.into_iter().zip(vals.chunks_exact(4)) {
+                    let sum = four_values
+                        .iter()
+                        .cloned()
+                        .sum::<Result<ValueSum, OverflowError>>()
+                        .expect("we generate values that won't overflow");
+
+                    let fifth = negate_value_sum(sum);
+
+                    // Four random entries
+                    for value in four_values.iter().cloned() {
+                        out.push((value, traps.next().expect("enough trapdoors"), asset));
+                    }
+
+                    // One balancing entry so that the per-asset balance is zero
+                    out.push((fifth, traps.next().expect("enough trapdoors"), asset));
+                }
+
+                out
+            })
+    }
+
     fn check_binding_signature(
         native_values: &[(ValueSum, ValueCommitTrapdoor)],
         arb_values: &[(ValueSum, ValueCommitTrapdoor, AssetBase)],
-        neg_trapdoors: &[ValueCommitTrapdoor],
         arb_values_to_burn: &[(ValueSum, ValueCommitTrapdoor, AssetBase)],
     ) {
-        // for each arb value, create a negative value with a different trapdoor
-        let neg_arb_values: Vec<_> = arb_values
-            .iter()
-            .cloned()
-            .zip(neg_trapdoors.iter().cloned())
-            .map(|((value, _, asset), rcv)| (negate_value_sum(value), rcv, asset))
-            .collect();
-
         let native_value_balance = native_values
             .iter()
             .map(|(value, _)| value)
@@ -529,13 +556,7 @@ mod tests {
                 .map(|(value_sum, trapdoor)| (*value_sum, trapdoor.clone(), AssetBase::native()))
                 .collect();
 
-        let values = [
-            &native_values_with_asset,
-            arb_values,
-            &neg_arb_values,
-            arb_values_to_burn,
-        ]
-        .concat();
+        let values = [&native_values_with_asset, arb_values, arb_values_to_burn].concat();
 
         let bsk = values
             .iter()
@@ -571,20 +592,23 @@ mod tests {
                     prop::collection::vec((arb_value_sum_bounded(bound), arb_trapdoor()), n_values)
                 )
             ),
-            (asset_values, neg_trapdoors) in (1usize..10).prop_flat_map(|n_values|
-                (arb_note_value_bounded(MAX_NOTE_VALUE / n_values as u64).prop_flat_map(move |bound|
-                    prop::collection::vec((arb_value_sum_bounded(bound), arb_trapdoor(), arb_asset_base()), n_values)
-                ), prop::collection::vec(arb_trapdoor(), n_values))
+            // An Orchard ZSA transaction is valid only if the balance of each custom asset is zero.
+            // Accordingly, for each custom asset we generate 5 random ValueSum values (possibly negative)
+            // that sum to zero.
+            asset_values in (1usize..10).prop_flat_map(|n_assets|
+                arb_note_value_bounded(MAX_NOTE_VALUE / (n_assets as u64 * 5)).prop_flat_map(move |bound|
+                    arb_asset_values_balanced_per_asset(bound, n_assets)
+                )
             ),
             burn_values in (1usize..10).prop_flat_map(|n_values|
                 arb_note_value_bounded(MAX_NOTE_VALUE / n_values as u64)
                 .prop_flat_map(move |bound| prop::collection::vec((arb_value_sum_bounded(bound), arb_trapdoor(), arb_asset_base()), n_values))
             )
         ) {
-            check_binding_signature(&native_values, &[], &[], &[]);
-            check_binding_signature(&native_values, &[], &[], &burn_values);
-            check_binding_signature(&native_values, &asset_values, &neg_trapdoors, &[]);
-            check_binding_signature(&native_values, &asset_values, &neg_trapdoors, &burn_values);
+            check_binding_signature(&native_values, &[], &[]);
+            check_binding_signature(&native_values, &[], &burn_values);
+            check_binding_signature(&native_values, &asset_values, &[]);
+            check_binding_signature(&native_values, &asset_values, &burn_values);
         }
     }
 }