Apply suggestions from code review

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: str4d <thestr4d@gmail.com>

zcash_client_backend/src/data_api.rs

@@ -27,7 +27,7 @@ pub mod chain;
 pub mod error;
 pub mod wallet;
 
-/// Read-only operations require for light wallet functions.
+/// Read-only operations required for light wallet functions.
 ///
 /// This trait defines the read-only portion of the storage
 /// interface atop which higher-level wallet operations are
@@ -53,8 +53,10 @@ pub trait WalletRead {
     /// Returns the minimum and maximum block heights for stored blocks.
     fn block_height_extrema(&self) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error>;
 
-    /// Returns the default target height and anchor height, given the
-    /// range of block heights that the backend knows about.
+    /// Returns the default target height (for the block in which a new
+    /// transaction would be mined) and anchor height (to use for a new
+    /// transaction), given the range of block heights that the backend
+    /// knows about.
     fn get_target_and_anchor_heights(
         &self,
     ) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
@@ -74,7 +76,7 @@ pub trait WalletRead {
         })
     }
 
-    /// Returns the block hash for the block at the given height
+    /// Returns the block hash for the block at the given height.
     fn get_block_hash(&self, block_height: BlockHeight) -> Result<Option<BlockHash>, Self::Error>;
 
     /// Returns the block hash for the block at the maximum height known
@@ -92,14 +94,15 @@ pub trait WalletRead {
             .map(|oo| oo.flatten())
     }
 
-    /// Returns the block height in which the specified transaction was mined.
+    /// Returns the block height in which the specified transaction was mined,
+    /// or `None` if the transaction is not mined in the main chain.
     fn get_tx_height(&self, txid: TxId) -> Result<Option<BlockHeight>, Self::Error>;
 
     /// Returns the payment address for the specified account, if the account
     /// identifier specified refers to a valid account for this wallet.
     fn get_address(&self, account: AccountId) -> Result<Option<PaymentAddress>, Self::Error>;
 
-    /// Returns all extended full viewing keys known about by this wallet
+    /// Returns all extended full viewing keys known about by this wallet.
     fn get_extended_full_viewing_keys(
         &self,
     ) -> Result<HashMap<AccountId, ExtendedFullViewingKey>, Self::Error>;
@@ -127,7 +130,7 @@ pub trait WalletRead {
     }
 
     /// Returns the wallet balance for an account as of the specified block
-    /// height. and
+    /// height.
     ///
     /// This may be used to obtain a balance that ignores notes that have been
     /// received so recently that they are not yet deemed spendable.
@@ -200,6 +203,8 @@ pub trait WalletWrite: WalletRead {
         commitment_tree: &CommitmentTree<Node>,
     ) -> Result<(), Self::Error>;
 
+    /// Rewinds the wallet database to the specified height.
+    ///
     /// This method assumes that the state of the underlying data store is
     /// consistent up to a particular block height. Since it is possible that
     /// a chain reorg might invalidate some stored state, this method must be

zcash_client_backend/src/data_api/chain.rs

@@ -339,8 +339,7 @@ where
                     .retain(|(_, nf)| !tx.shielded_spends.iter().any(|spend| &spend.nf == nf));
 
                 for output in tx.shielded_outputs {
-                    match &extfvks.get(&output.account) {
-                        Some(extfvk) => {
+                    if let Some(extfvk) = &extfvks.get(&output.account) {
                             let nf = output
                                 .note
                                 .nf(&extfvk.fvk.vk, output.witness.position() as u64);
@@ -352,8 +351,6 @@ where
 
                             // Cache nullifier for note (to detect subsequent spends in this scan).
                             nullifiers.push((output.account, nf));
-                        }
-                        None => (),
                     }
                 }
             }

zcash_client_backend/src/data_api/error.rs

@@ -27,7 +27,7 @@ pub enum Error<NoteId> {
     InsufficientBalance(Amount, Amount),
     /// Chain validation detected an error in the block at the specified block height.
     InvalidChain(BlockHeight, ChainInvalid),
-    /// A provided extfvk is not associated with the specified account.
+    /// A provided extsk is not associated with the specified account.
     InvalidExtSK(AccountId),
     /// The root of an output's witness tree in a newly arrived transaction does
     /// not correspond to root of the stored commitment tree at the recorded height.

zcash_client_backend/src/wallet.rs

@@ -1,6 +1,8 @@
 //! Structs representing transaction data scanned from the block chain by a wallet or
 //! light client.
 
+use subtle::{ConditionallySelectable, Choice};
+
 use zcash_primitives::{
     keys::OutgoingViewingKey,
     merkle_tree::IncrementalWitness,
@@ -13,6 +15,18 @@ use zcash_primitives::{
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
 pub struct AccountId(pub u32);
 
+impl Default for AccountId {
+    fn default() -> Self {
+        AccountId(0)
+    }
+}
+
+impl ConditionallySelectable for AccountId {
+    fn conditional_select(a0: &Self, a1: &Self, c: Choice) -> Self {
+        AccountId(u32::conditional_select(&a0.0, &a1.0, c))
+    }
+}
+
 /// A subset of a [`Transaction`] relevant to wallets and light clients.
 ///
 /// [`Transaction`]: zcash_primitives::transaction::Transaction

zcash_client_backend/src/welding_rig.rs

@@ -108,19 +108,20 @@ pub fn scan_block<P: consensus::Parameters>(
             .into_iter()
             .enumerate()
             .map(|(index, spend)| {
-                let spend_nf = Nullifier::from_slice(&spend.nf[..]);
+                let spend_nf = Nullifier::from_slice(&spend.nf[..]).unwrap();
                 // Find the first tracked nullifier that matches this spend, and produce
                 // a WalletShieldedSpend if there is a match, in constant time.
                 nullifiers
                     .iter()
-                    .map(|&(account, nf)| CtOption::new(account.0 as u64, nf.0.ct_eq(&spend_nf.0)))
-                    .fold(CtOption::new(0, 0.into()), |first, next| {
-                        CtOption::conditional_select(&next, &first, first.is_some())
-                    })
+                    .map(|&(account, nf)| CtOption::new(account, nf.ct_eq(&spend_nf)))
+                    .fold(
+                        CtOption::new(AccountId::default(), 0.into()), 
+                        |first, next| CtOption::conditional_select(&next, &first, first.is_some())
+                    )
                     .map(|account| WalletShieldedSpend {
                         index,
                         nf: spend_nf,
-                        account: AccountId(account as u32),
+                        account,
                     })
             })
             .filter(|spend| spend.is_some().into())

zcash_client_sqlite/src/wallet.rs

@@ -431,7 +431,7 @@ pub fn get_nullifiers<P>(
     let nullifiers = stmt_fetch_nullifiers.query_map(NO_PARAMS, |row| {
         let account = AccountId(row.get(1)?);
         let nf_bytes: Vec<u8> = row.get(2)?;
-        Ok((account, Nullifier::from_slice(&nf_bytes)))
+        Ok((account, Nullifier::from_slice(&nf_bytes).unwrap()))
     })?;
 
     let res: Vec<_> = nullifiers.collect::<Result<_, _>>()?;

zcash_primitives/src/primitives.rs

@@ -2,7 +2,9 @@
 
 use ff::PrimeField;
 use group::{Curve, Group, GroupEncoding};
+use std::array::TryFromSliceError;
 use std::convert::TryInto;
+use subtle::{ConstantTimeEq, Choice};
 
 use crate::constants;
 
@@ -201,10 +203,8 @@ pub enum Rseed {
 pub struct Nullifier(pub [u8; 32]);
 
 impl Nullifier {
-    pub fn from_slice(bytes: &[u8]) -> Nullifier {
-        let mut nf = Nullifier([0u8; 32]);
-        nf.0.copy_from_slice(bytes);
-        nf
+    pub fn from_slice(bytes: &[u8]) -> Result<Nullifier, TryFromSliceError> {
+        bytes.try_into().map(Nullifier)
     }
 
     pub fn to_vec(&self) -> Vec<u8> {
@@ -212,6 +212,13 @@ impl Nullifier {
     }
 }
 
+impl ConstantTimeEq for Nullifier {
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.ct_eq(&other.0)
+    }
+}
+
+
 #[derive(Clone, Debug)]
 pub struct Note {
     /// The value of the note
@@ -287,7 +294,7 @@ impl Note {
                 .update(&rho.to_bytes())
                 .finalize()
                 .as_bytes(),
-        )
+        ).unwrap()
     }
 
     /// Computes the note commitment