Apply suggestions from code review

Co-authored-by: Jack Grigg <jack@electriccoin.co>

Cargo.toml

@@ -33,7 +33,7 @@ getset = "0.1"
 core2 = { version = "0.3", default-features = false, features = ["alloc"] }
 
 # Circuits
-bellman = { version = "0.14", features = ["groth16"], optional = true }
+bellman = { version = "0.14", default-features = false, features = ["groth16"], optional = true }
 
 # CSPRNG
 rand = { version = "0.8", default-features = false }
@@ -86,26 +86,26 @@ rand_xorshift = "0.3"
 pprof = { version = "0.11", features = ["criterion", "flamegraph"] } # MSRV 1.56
 
 [features]
-default = ["multicore", "std"]
+default = ["multicore", "circuit"]
 std = [
   "core2/std",
-  "document-features",
+  "dep:document-features",
   "group/wnaf-memuse",
   "redjubjub/std",
-  "circuit",
 ]
 
 ## Enables creation of Sapling proofs
 circuit = [
-  "bellman",
+  "dep:bellman",
   "bls12_381/bits",
   "bls12_381/groups",
   "bls12_381/pairings",
   "jubjub/bits",
+  "std"
 ]
 
 ## Enables multithreading support for creating proofs.
-multicore = ["circuit", "bellman/multicore"]
+multicore = ["bellman?/multicore"]
 
 ### A temporary feature flag that exposes granular APIs needed by `zcashd`. These APIs
 ### should not be relied upon and will be removed in a future release.

src/builder.rs

@@ -992,7 +992,7 @@ impl ProverProgress for () {
     fn update(&mut self, _: u32, _: u32) {}
 }
 
-#[cfg(feature = "circuit")]
+#[cfg(all(feature = "circuit", feature = "std"))]
 impl<U: From<(u32, u32)>> ProverProgress for std::sync::mpsc::Sender<U> {
     fn update(&mut self, cur: u32, end: u32) {
         // If the send fails, we should ignore the error, not crash.
@@ -1058,7 +1058,6 @@ impl<'a, SP: SpendProver, OP: OutputProver, R: RngCore, U: ProverProgress>
         OP::encode_proof(proof)
     }
 
-    #[cfg(feature = "circuit")]
     fn map_authorization<S: InProgressSignatures>(
         &mut self,
         a: InProgress<Unproven, S>,
@@ -1301,9 +1300,9 @@ impl<V> Bundle<InProgress<Proven, PartiallyAuthorized>, V> {
     }
 }
 
-#[cfg(any(test, feature = "test-dependencies"))]
+#[cfg(all(feature = "circuit", any(test, feature = "test-dependencies")))]
 pub(crate) mod testing {
-    use std::fmt;
+    use core::fmt;
 
     use proptest::collection::vec;
     use proptest::prelude::*;
@@ -1312,7 +1311,6 @@ pub(crate) mod testing {
     use crate::{
         bundle::{Authorized, Bundle},
         note_encryption::Zip212Enforcement,
-        prover::mock::{MockOutputProver, MockSpendProver},
         testing::{arb_node, arb_note},
         value::testing::arb_positive_note_value,
         zip32::testing::arb_extended_spending_key,
@@ -1324,7 +1322,11 @@ pub(crate) mod testing {
 
     use super::{Builder, BundleType};
 
+    #[cfg(feature = "circuit")]
+    use crate::prover::mock::{MockOutputProver, MockSpendProver};
+
     #[allow(dead_code)]
+    #[cfg(feature = "circuit")]
     fn arb_bundle<V: fmt::Debug + From<i64>>(
         max_money: u64,
         zip212_enforcement: Zip212Enforcement,

src/circuit.rs

@@ -562,7 +562,10 @@ impl SpendParameters {
     /// Only set `verify_point_encodings` to false if you are verifying the parameters in
     /// another way (such as checking the hash of the parameters file on disk).
     pub fn read<R: io::Read>(reader: R, verify_point_encodings: bool) -> io::Result<Self> {
-        groth16::Parameters::<Bls12>::read(reader, verify_point_encodings).map(Self)
+        Ok(Self(groth16::Parameters::<Bls12>::read(
+            reader,
+            verify_point_encodings,
+        )?))
     }
 
     /// Returns the verifying key for the Sapling Spend circuit.

src/keys.rs

@@ -27,7 +27,7 @@ use subtle::{Choice, ConditionallySelectable, ConstantTimeEq, CtOption};
 use zcash_note_encryption::EphemeralKeyBytes;
 use zcash_spec::PrfExpand;
 
-#[cfg(test)]
+#[cfg(all(feature = "circuit", test))]
 use rand_core::RngCore;
 
 /// Errors that can occur in the decoding of Sapling spending keys.
@@ -154,7 +154,7 @@ impl Eq for SpendValidatingKey {}
 
 impl SpendValidatingKey {
     /// For circuit tests only.
-    #[cfg(test)]
+    #[cfg(all(feature = "circuit", test))]
     pub(crate) fn fake_random<R: RngCore>(mut rng: R) -> Self {
         loop {
             if let Some(k) = Self::from_bytes(&jubjub::SubgroupPoint::random(&mut rng).to_bytes()) {

src/lib.rs

@@ -7,7 +7,7 @@
 //! shielded payment address; we implicitly mean it is an Sapling payment address (as
 //! opposed to e.g. an Orchard payment address, which is also shielded).
 //!
-//! ## Feature flags
+#![cfg_attr(feature = "std", doc = "## Feature flags")]
 #![cfg_attr(feature = "std", doc = document_features::document_features!())]
 //!
 

src/pedersen_hash.rs

@@ -93,9 +93,7 @@ where
         let num_limbs: usize = acc.as_ref().len() / 8;
         let mut limbs = vec![0u64; num_limbs + 1];
         for (src, dst) in acc.chunks_exact(8).zip(limbs[..num_limbs].iter_mut()) {
-            let mut limb_bytes = [0u8; 8];
+            *dst = u64::from_le_bytes(src.try_into().expect("correct length"));
-            limb_bytes.copy_from_slice(src);
-            *dst = u64::from_le_bytes(limb_bytes);
         }
 
         let mut tmp = jubjub::SubgroupPoint::identity();

src/zip32.rs

@@ -421,8 +421,7 @@ impl ExtendedSpendingKey {
     pub fn derive_child(&self, i: ChildIndex) -> Self {
         let fvk = FullViewingKey::from_expanded_spending_key(&self.expsk);
         let tmp = {
-            let mut le_i = [0; 4];
+            let le_i = i.index().to_le_bytes();
-            le_i.copy_from_slice(&i.index().to_le_bytes());
             PrfExpand::SAPLING_ZIP32_CHILD_HARDENED.with(
                 self.chain_code.as_bytes(),
                 &self.expsk.to_bytes(),