PR471 review: various fixes (1)

.github/workflows/ci.yml

@@ -1,27 +1,18 @@
 name: CI checks
 
-#on: [push, pull_request]
-on:
-  push:
-    branches: [ main ]  # Only runs on push to main
-  pull_request:       # Runs on any PR to any branch
+on: [push, pull_request]
 
 jobs:
   test:
     name: Test on ${{ matrix.os }}
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
-        os: [ ubuntu-latest, windows-latest ]
-    #        os: [ubuntu-latest, windows-latest, macOS-latest]
-
+        os: [ubuntu-latest, windows-latest, macOS-latest]
     steps:
       - uses: actions/checkout@v4
       - name: Run tests
-        env:
-          # Disable LTO in CI to avoid very slow/hanging release test builds on macOS (rustc/LLVM).
-          CARGO_PROFILE_RELEASE_LTO: "off"
-        run: cargo test --verbose --release
+        run: cargo test --verbose
       - name: Verify working directory is clean
         run: git diff --exit-code
 
@@ -120,7 +111,7 @@ jobs:
     steps:
       - uses: actions/checkout@v4
       - name: Generate coverage report
-        run: timeout --preserve-status 300s cargo tarpaulin --engine llvm --timeout 600 --out xml --skip-clean || true
+        run: cargo tarpaulin --engine llvm --all-features --release --timeout 600 --out xml
       - name: Upload coverage to Codecov
         uses: codecov/codecov-action@v3.1.4
 

Cargo.toml

@@ -9,7 +9,7 @@ authors = [
     "Kris Nuttycombe <kris@electriccoin.co>",
 ]
 edition = "2021"
-rust-version = "1.71"
+rust-version = "1.70"
 description = "The Orchard shielded transaction protocol"
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/zcash/orchard"

README.md

@@ -1,7 +1,6 @@
-# orchard [![Crates.io](https://img.shields.io/crates/v/orchard.svg)](https://crates.io/crates/orchard) [![CI checks](https://github.com/QED-it/orchard/actions/workflows/ci.yml/badge.svg?branch=zsa1)](https://github.com/QED-it/orchard/actions/workflows/ci.yml)
-#
+# orchard [![Crates.io](https://img.shields.io/crates/v/orchard.svg)](https://crates.io/crates/orchard) #
 
-Requires Rust 1.71+.
+Requires Rust 1.70+.
 
 ## Documentation
 

benches/circuit.rs

@@ -7,11 +7,11 @@ use criterion::{BenchmarkId, Criterion};
 use pprof::criterion::{Output, PProfProfiler};
 
 use orchard::{
-    builder::{Builder, BundleType},
+    builder::Builder,
     circuit::{ProvingKey, VerifyingKey},
+    flavor::{OrchardVanilla, OrchardZSA},
     keys::{FullViewingKey, Scope, SpendingKey},
     note::AssetBase,
-    orchard_flavor::{OrchardVanilla, OrchardZSA},
     value::NoteValue,
     Anchor, Bundle,
 };
@@ -31,7 +31,10 @@ fn criterion_benchmark<FL: OrchardFlavorBench>(c: &mut Criterion) {
     let pk = ProvingKey::build::<FL>();
 
     let create_bundle = |num_recipients| {
-        let mut builder = Builder::new(BundleType::DEFAULT, Anchor::from_bytes([0; 32]).unwrap());
+        let mut builder = Builder::new(
+            FL::DEFAULT_BUNDLE_TYPE,
+            Anchor::from_bytes([0; 32]).unwrap(),
+        );
         for _ in 0..num_recipients {
             builder
                 .add_output(

benches/note_decryption.rs

@@ -1,10 +1,10 @@
 use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
 use orchard::{
-    builder::{Builder, BundleType},
+    builder::Builder,
     circuit::ProvingKey,
+    flavor::{OrchardVanilla, OrchardZSA},
     keys::{FullViewingKey, PreparedIncomingViewingKey, Scope, SpendingKey},
     note::AssetBase,
-    orchard_flavor::{OrchardVanilla, OrchardZSA},
     primitives::{CompactAction, OrchardDomain},
     value::NoteValue,
     Anchor, Bundle,
@@ -50,7 +50,10 @@ fn bench_note_decryption<FL: OrchardFlavorBench>(c: &mut Criterion) {
         .collect();
 
     let bundle = {
-        let mut builder = Builder::new(BundleType::DEFAULT, Anchor::from_bytes([0; 32]).unwrap());
+        let mut builder = Builder::new(
+            FL::DEFAULT_BUNDLE_TYPE,
+            Anchor::from_bytes([0; 32]).unwrap(),
+        );
         // The builder pads to two actions, and shuffles their order. Add two recipients
         // so the first action is always decryptable.
         builder

benches/utils.rs

@@ -1,15 +1,22 @@
 use criterion::{measurement::Measurement, BenchmarkGroup, Criterion};
 
-use orchard::orchard_flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA};
+use orchard::{
+    builder::BundleType,
+    flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
+};
 
 pub(crate) trait OrchardFlavorBench: OrchardFlavor {
+    const DEFAULT_BUNDLE_TYPE: BundleType;
+
     fn benchmark_group<'a, M: Measurement>(
         c: &'a mut Criterion<M>,
         group_name: &str,
     ) -> BenchmarkGroup<'a, M>;
 }
 
 impl OrchardFlavorBench for OrchardVanilla {
+    const DEFAULT_BUNDLE_TYPE: BundleType = BundleType::DEFAULT;
+
     fn benchmark_group<'a, M: Measurement>(
         c: &'a mut Criterion<M>,
         group_name: &str,
@@ -19,6 +26,8 @@ impl OrchardFlavorBench for OrchardVanilla {
 }
 
 impl OrchardFlavorBench for OrchardZSA {
+    const DEFAULT_BUNDLE_TYPE: BundleType = BundleType::DEFAULT_ZSA;
+
     fn benchmark_group<'a, M: Measurement>(
         c: &'a mut Criterion<M>,
         group_name: &str,

book/src/design/actions.md

@@ -21,7 +21,6 @@ balance the transaction without doubling its size.
 ## Dummy notes for Orchard
 
 For Orchard, a transaction is a bundle of actions. Each action is composed of one spend and one output.
-This means we have the same amount of "spends" and "outputs" in one transaction.
 If we would like to create a transaction with a different number of spends and outputs,
 we need to add "dummy" spends or outputs to balance their count.
 A dummy spend or output is a note with a value of zero and a random recipient address.
@@ -53,4 +52,3 @@ Each Orchard action has a memo field for its corresponding output, as with Sprou
 Sapling. We did at one point consider having a single Orchard memo field per transaction,
 and/or having a mechanism for enabling multiple recipients to decrypt the same memo, but
 these were decided against in order to keep the overall design simpler.
-

rust-toolchain.toml

@@ -1,3 +1,3 @@
 [toolchain]
-channel = "1.71.0"
+channel = "1.70.0"
 components = [ "clippy", "rustfmt" ]

src/action.rs

@@ -2,9 +2,9 @@ use memuse::DynamicUsage;
 
 use crate::{
     note::{ExtractedNoteCommitment, Nullifier, Rho, TransmittedNoteCiphertext},
-    orchard_sighash_versioning::VerSpendAuthSig,
     primitives::redpallas::{self, SpendAuth},
     primitives::OrchardPrimitives,
+    sighash_versioning::VerSpendAuthSig,
     value::ValueCommitment,
 };
 
@@ -136,12 +136,12 @@ pub(crate) mod testing {
             commitment::ExtractedNoteCommitment, nullifier::testing::arb_nullifier,
             testing::arb_note, AssetBase, Note, TransmittedNoteCiphertext,
         },
-        orchard_sighash_versioning::VerSpendAuthSig,
         primitives::redpallas::{
             self,
             testing::{arb_spendauth_signing_key, arb_spendauth_verification_key},
         },
         primitives::{OrchardDomain, OrchardPrimitives},
+        sighash_versioning::VerSpendAuthSig,
         value::{NoteValue, ValueCommitTrapdoor, ValueCommitment},
     };
 

src/builder.rs

@@ -15,15 +15,15 @@ use crate::{
     address::Address,
     builder::BuildError::{BurnNative, BurnZero},
     bundle::{Authorization, Authorized, Bundle, Flags},
+    flavor::OrchardVanilla,
     keys::{
         FullViewingKey, OutgoingViewingKey, Scope, SpendAuthorizingKey, SpendValidatingKey,
         SpendingKey,
     },
     note::{AssetBase, ExtractedNoteCommitment, Note, Nullifier, Rho, TransmittedNoteCiphertext},
-    orchard_flavor::OrchardVanilla,
-    orchard_sighash_versioning::{VerBindingSig, VerSpendAuthSig},
     primitives::redpallas::{self, Binding, SpendAuth},
     primitives::{OrchardDomain, OrchardPrimitives},
+    sighash_versioning::{VerBindingSig, VerSpendAuthSig},
     tree::{Anchor, MerklePath},
     value::{self, NoteValue, OverflowError, ValueCommitTrapdoor, ValueCommitment, ValueSum},
     Proof,
@@ -35,7 +35,7 @@ use {
         action::Action,
         bundle::derive_bvk,
         circuit::{Circuit, Instance, OrchardCircuit, ProvingKey, Witnesses},
-        orchard_flavor::OrchardFlavor,
+        flavor::OrchardFlavor,
     },
     nonempty::NonEmpty,
 };
@@ -517,7 +517,7 @@ impl ActionInfo {
         mut rng: impl RngCore,
     ) -> (Action<SigningMetadata, FL>, Witnesses) {
         let v_net = self.value_sum();
-        let cv_net = ValueCommitment::derive(v_net, self.rcv, self.output.asset);
+        let cv_net = ValueCommitment::derive(v_net, self.rcv.clone(), self.output.asset);
 
         let (nf_old, ak, alpha, rk) = self.spend.build(&mut rng);
         let (note, cmx, encrypted_note) = self.output.build(&cv_net, nf_old, &mut rng);
@@ -540,7 +540,7 @@ impl ActionInfo {
 
     fn build_for_pczt(self, mut rng: impl RngCore) -> crate::pczt::Action {
         let v_net = self.value_sum();
-        let cv_net = ValueCommitment::derive(v_net, self.rcv, self.spend.note.asset());
+        let cv_net = ValueCommitment::derive(v_net, self.rcv.clone(), self.spend.note.asset());
 
         let spend = self.spend.into_pczt(&mut rng);
         let output = self.output.into_pczt(&cv_net, spend.nullifier, &mut rng);
@@ -1398,9 +1398,9 @@ pub mod testing {
         address::testing::arb_address,
         bundle::{Authorized, Bundle},
         circuit::ProvingKey,
+        flavor::OrchardFlavor,
         keys::{testing::arb_spending_key, FullViewingKey, SpendAuthorizingKey, SpendingKey},
         note::{testing::arb_note, AssetBase},
-        orchard_flavor::OrchardFlavor,
         primitives::OrchardPrimitives,
         tree::{Anchor, MerkleHashOrchard, MerklePath},
         value::{testing::arb_positive_note_value, NoteValue, MAX_NOTE_VALUE},
@@ -1546,9 +1546,9 @@ mod tests {
         bundle::{Authorized, Bundle},
         circuit::ProvingKey,
         constants::MERKLE_DEPTH_ORCHARD,
+        flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
         keys::{FullViewingKey, Scope, SpendingKey},
         note::AssetBase,
-        orchard_flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
         tree::EMPTY_ROOTS,
         value::NoteValue,
     };

src/bundle.rs

@@ -26,9 +26,9 @@ use crate::{
     bundle::commitments::{hash_bundle_auth_data, hash_bundle_txid_data},
     keys::{IncomingViewingKey, OutgoingViewingKey, PreparedIncomingViewingKey},
     note::{AssetBase, Note},
-    orchard_sighash_versioning::{OrchardSighashVersion, VerBindingSig, VerSpendAuthSig},
     primitives::redpallas::{self, Binding},
     primitives::{OrchardDomain, OrchardPrimitives},
+    sighash_versioning::{OrchardSighashVersion, VerBindingSig, VerSpendAuthSig},
     tree::Anchor,
     value::{NoteValue, ValueCommitTrapdoor, ValueCommitment, ValueSum},
     Proof,
@@ -639,8 +639,8 @@ pub mod testing {
             asset_base::testing::{arb_asset_base, arb_zsa_asset_base},
             AssetBase,
         },
-        orchard_sighash_versioning::{VerBindingSig, VerSpendAuthSig},
         primitives::{redpallas::testing::arb_binding_signing_key, OrchardPrimitives},
+        sighash_versioning::{VerBindingSig, VerSpendAuthSig},
         value::{
             testing::{arb_note_value, arb_note_value_bounded},
             NoteValue, ValueSum, MAX_NOTE_VALUE,

src/bundle/burn_validation.rs

@@ -66,7 +66,10 @@ impl fmt::Display for BurnError {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{issuance::compute_asset_desc_hash, issuance_auth::ZSASchnorr, value::NoteValue};
+    use crate::{
+        issuance::{auth::ZSASchnorr, compute_asset_desc_hash},
+        value::NoteValue,
+    };
     use nonempty::NonEmpty;
 
     /// Creates an item of bundle burn list for a given asset description hash and value.
@@ -84,7 +87,7 @@ mod tests {
     /// A tuple `(AssetBase, Amount)` representing the burn list item.
     ///
     fn get_burn_tuple(asset_desc_hash: &[u8; 32], value: u64) -> (AssetBase, NoteValue) {
-        use crate::issuance_auth::{IssueAuthKey, IssueValidatingKey};
+        use crate::issuance::auth::{IssueAuthKey, IssueValidatingKey};
 
         let isk = IssueAuthKey::<ZSASchnorr>::from_bytes(&[1u8; 32]).unwrap();
 

src/bundle/commitments.rs

@@ -5,10 +5,12 @@ use blake2b_simd::{Hash as Blake2bHash, Params, State};
 
 use crate::{
     bundle::{Authorization, Authorized, Bundle},
-    issuance::{IssueAuth, IssueBundle, Signed},
-    issuance_sighash_versioning::IssueSighashVersion,
-    orchard_sighash_versioning::OrchardSighashVersion,
+    issuance::{
+        sighash_versioning::IssueSighashVersion,
+        {IssueAuth, IssueBundle, Signed},
+    },
     primitives::OrchardPrimitives,
+    sighash_versioning::OrchardSighashVersion,
 };
 
 pub(crate) const ZCASH_ORCHARD_HASH_PERSONALIZATION: &[u8; 16] = b"ZTxIdOrchardHash";
@@ -192,13 +194,15 @@ mod tests {
             Authorized, Bundle,
         },
         circuit::ProvingKey,
-        issuance::{compute_asset_desc_hash, AwaitingSighash, IssueBundle, IssueInfo},
-        issuance_auth::{IssueAuthKey, IssueValidatingKey, ZSASchnorr},
-        issuance_sighash_versioning::IssueSighashVersion,
+        flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
+        issuance::{
+            auth::{IssueAuthKey, IssueValidatingKey, ZSASchnorr},
+            sighash_versioning::IssueSighashVersion,
+            {compute_asset_desc_hash, AwaitingSighash, IssueBundle, IssueInfo},
+        },
         keys::{FullViewingKey, Scope, SpendingKey},
         note::{AssetBase, Nullifier},
-        orchard_flavor::{OrchardFlavor, OrchardVanilla, OrchardZSA},
-        orchard_sighash_versioning::OrchardSighashVersion,
+        sighash_versioning::OrchardSighashVersion,
         value::NoteValue,
         Anchor,
     };
@@ -328,7 +332,7 @@ mod tests {
             compute_asset_desc_hash(&NonEmpty::from_slice(b"second asset").unwrap());
 
         let (mut bundle, asset) = IssueBundle::new(
-            ik.clone(),
+            ik,
             asset_desc_hash_1,
             Some(IssueInfo {
                 recipient,

src/circuit.rs

@@ -267,6 +267,8 @@ impl Witnesses {
 }
 
 /// The verifying key for the Orchard Action circuit.
+///
+/// In practice, this is constructed for either `OrchardVanilla` or `OrchardZSA`.
 #[derive(Debug, Clone)]
 pub struct VerifyingKey {
     pub(crate) params: halo2_proofs::poly::commitment::Params<vesta::Affine>,
@@ -286,6 +288,8 @@ impl VerifyingKey {
 }
 
 /// The proving key for the Orchard Action circuit.
+///
+/// In practice, this is constructed for either `OrchardVanilla` or `OrchardZSA`.
 #[derive(Debug, Clone)]
 pub struct ProvingKey {
     params: halo2_proofs::poly::commitment::Params<vesta::Affine>,

src/circuit/circuit_vanilla.rs

@@ -36,8 +36,8 @@ use crate::{
         ENABLE_OUTPUT, ENABLE_SPEND, NF_OLD, RK_X, RK_Y,
     },
     constants::{OrchardFixedBases, OrchardFixedBasesFull, OrchardHashDomains},
+    flavor::OrchardVanilla,
     note::AssetBase,
-    orchard_flavor::OrchardVanilla,
 };
 
 impl OrchardCircuit for OrchardVanilla {
@@ -641,9 +641,9 @@ mod tests {
     use crate::{
         bundle::Flags,
         circuit::{Circuit, Instance, Proof, ProvingKey, VerifyingKey, Witnesses, K},
+        flavor::OrchardVanilla,
         keys::SpendValidatingKey,
         note::{AssetBase, Note, Rho},
-        orchard_flavor::OrchardVanilla,
         tree::MerklePath,
         value::{ValueCommitTrapdoor, ValueCommitment},
     };
@@ -665,7 +665,7 @@ mod tests {
 
         let value = spent_note.value() - output_note.value();
         let rcv = ValueCommitTrapdoor::random(&mut rng);
-        let cv_net = ValueCommitment::derive(value, rcv, AssetBase::native());
+        let cv_net = ValueCommitment::derive(value, rcv.clone(), AssetBase::native());
 
         let path = MerklePath::dummy(&mut rng);
         let anchor = path.root(spent_note.commitment().into());