Address comments from code review.

zcash_client_backend/CHANGELOG.md

@@ -46,7 +46,7 @@ and this library adheres to Rust's notion of
     respective `min_confirmations` arguments as `NonZeroU32`
   - `data_api::wallet::input_selection::InputSelector::{propose_transaction, propose_shielding}`
     now take their respective `min_confirmations` arguments as `NonZeroU32`
-  - A new `Sync` variant has been added to `data_api::chain::error::Error`.
+  - A new `Scan` variant has been added to `data_api::chain::error::Error`.
   - A new `SyncRequired` variant has been added to `data_api::wallet::input_selection::InputSelectorError`.
 - `zcash_client_backend::wallet`:
   - `SpendableNote` has been renamed to `ReceivedSaplingNote`.
@@ -61,8 +61,7 @@ and this library adheres to Rust's notion of
 - Arguments to `zcash_client_backend::scanning::scan_block` have changed. This
   method now takes an optional `BlockMetadata` argument instead of a base commitment
   tree and incremental witnesses for each previously-known note. In addition, the
-  return type has now been updated to return a `Result<ScannedBlock, ScanError>` 
-  in the case of scan failure.
+  return type has now been updated to return a `Result<ScannedBlock, ScanError>`.
 
 
 ### Removed

zcash_client_backend/build.rs

@@ -45,6 +45,10 @@ fn build() -> io::Result<()> {
         // Build the gRPC types and client.
         tonic_build::configure()
             .build_server(false)
+            .extern_path(
+                ".cash.z.wallet.sdk.rpc.ChainMetadata",
+                "crate::proto::compact_formats::ChainMetadata",
+            )
             .extern_path(
                 ".cash.z.wallet.sdk.rpc.CompactBlock",
                 "crate::proto::compact_formats::CompactBlock",

zcash_client_backend/proto/compact_formats.proto

@@ -10,9 +10,7 @@ option swift_prefix = "";
 // Remember that proto3 fields are all optional. A field that is not present will be set to its zero value.
 // bytes fields of hashes are in canonical little-endian format.
 
-// BlockMetadata represents information about a block that may not be
-// represented directly in the block data, but is instead derived from chain
-// data or other external sources.
+// ChainMetadata represents information about the state of the chain as of a given block.
 message ChainMetadata {
     uint32 saplingCommitmentTreeSize = 1; // the size of the Sapling note commitment tree as of the end of this block
     uint32 orchardCommitmentTreeSize = 2; // the size of the Orchard note commitment tree as of the end of this block

zcash_client_backend/src/data_api.rs

@@ -340,7 +340,7 @@ impl<Nf> ScannedBlock<Nf> {
         &self.sapling_commitments
     }
 
-    pub fn take_sapling_commitments(self) -> Vec<(sapling::Node, Retention<BlockHeight>)> {
+    pub fn into_sapling_commitments(self) -> Vec<(sapling::Node, Retention<BlockHeight>)> {
         self.sapling_commitments
     }
 }

zcash_client_backend/src/data_api/chain.rs

@@ -92,10 +92,11 @@ pub trait BlockSource {
 /// Scans at most `limit` new blocks added to the block source for any transactions received by the
 /// tracked accounts.
 ///
-/// If the `from_height` argument is not `None`, then the block source will begin requesting blocks
-/// from the provided block source at the specified height; if `from_height` is `None then this
-/// will begin scanning at first block after the position to which the wallet has previously
-/// fully scanned the chain, thereby beginning or continuing a linear scan over all blocks.
+/// If the `from_height` argument is not `None`, then this method block source will begin
+/// requesting blocks from the provided block source at the specified height; if `from_height` is
+/// `None then this will begin scanning at first block after the position to which the wallet has
+/// previously fully scanned the chain, thereby beginning or continuing a linear scan over all
+/// blocks.
 ///
 /// This function will return without error after scanning at most `limit` new blocks, to enable
 /// the caller to update their UI with scanning progress. Repeatedly calling this function with

zcash_client_backend/src/scanning.rs

@@ -285,7 +285,7 @@ pub(crate) fn scan_block_with_runner<
             at_height: cur_height,
         })?;
 
-    let block_tx_count = block.vtx.len();
+    let compact_block_tx_count = block.vtx.len();
     for (tx_idx, tx) in block.vtx.into_iter().enumerate() {
         let txid = tx.txid();
 
@@ -392,7 +392,8 @@ pub(crate) fn scan_block_with_runner<
             {
                 // Collect block note commitments
                 let node = sapling::Node::from_cmu(&output.cmu);
-                let is_checkpoint = output_idx + 1 == decoded.len() && tx_idx + 1 == block_tx_count;
+                let is_checkpoint =
+                    output_idx + 1 == decoded.len() && tx_idx + 1 == compact_block_tx_count;
                 let retention = match (dec_output.is_some(), is_checkpoint) {
                     (is_marked, true) => Retention::Checkpoint {
                         id: cur_height,

zcash_client_sqlite/src/error.rs

@@ -9,7 +9,7 @@ use shardtree::ShardTreeError;
 use zcash_client_backend::encoding::{Bech32DecodeError, TransparentCodecError};
 use zcash_primitives::{consensus::BlockHeight, zip32::AccountId};
 
-use crate::PRUNING_HEIGHT;
+use crate::PRUNING_DEPTH;
 
 #[cfg(feature = "transparent-inputs")]
 use zcash_primitives::legacy::TransparentAddress;
@@ -108,7 +108,7 @@ impl fmt::Display for SqliteClientError {
             SqliteClientError::InvalidNoteId =>
                 write!(f, "The note ID associated with an inserted witness must correspond to a received note."),
             SqliteClientError::RequestedRewindInvalid(h, r) =>
-                write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_HEIGHT, h, r),
+                write!(f, "A rewind must be either of less than {} blocks, or at least back to block {} for your wallet; the requested height was {}.", PRUNING_DEPTH, h, r),
             SqliteClientError::Bech32DecodeError(e) => write!(f, "{}", e),
             #[cfg(feature = "transparent-inputs")]
             SqliteClientError::HdwalletError(e) => write!(f, "{:?}", e),

zcash_client_sqlite/src/lib.rs

@@ -82,7 +82,7 @@ pub mod wallet;
 /// The maximum number of blocks the wallet is allowed to rewind. This is
 /// consistent with the bound in zcashd, and allows block data deeper than
 /// this delta from the chain tip to be pruned.
-pub(crate) const PRUNING_HEIGHT: u32 = 100;
+pub(crate) const PRUNING_DEPTH: u32 = 100;
 
 pub(crate) const SAPLING_TABLES_PREFIX: &str = "sapling";
 
@@ -428,7 +428,7 @@ impl<P: consensus::Parameters> WalletWrite for WalletDb<rusqlite::Connection, P>
             let block_height = block.height();
             let sapling_tree_size = block.metadata().sapling_tree_size();
             let sapling_commitments_len = block.sapling_commitments().len();
-            let mut sapling_commitments = block.take_sapling_commitments().into_iter();
+            let mut sapling_commitments = block.into_sapling_commitments().into_iter();
             wdb.with_sapling_tree_mut::<_, _, SqliteClientError>(move |sapling_tree| {
                 let start_position = Position::from(u64::from(sapling_tree_size))
                     - u64::try_from(sapling_commitments_len).unwrap();
@@ -640,7 +640,7 @@ impl<P: consensus::Parameters> WalletCommitmentTrees for WalletDb<rusqlite::Conn
         let shard_store = SqliteShardStore::from_connection(&tx, SAPLING_TABLES_PREFIX)
             .map_err(|e| ShardTreeError::Storage(Either::Right(e)))?;
         let result = {
-            let mut shardtree = ShardTree::new(shard_store, PRUNING_HEIGHT.try_into().unwrap());
+            let mut shardtree = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap());
             callback(&mut shardtree)?
         };
         tx.commit()
@@ -668,7 +668,7 @@ impl<'conn, P: consensus::Parameters> WalletCommitmentTrees for WalletDb<SqlTran
         let mut shardtree = ShardTree::new(
             SqliteShardStore::from_connection(self.conn.0, SAPLING_TABLES_PREFIX)
                 .map_err(|e| ShardTreeError::Storage(Either::Right(e)))?,
-            PRUNING_HEIGHT.try_into().unwrap(),
+            PRUNING_DEPTH.try_into().unwrap(),
         );
         let result = callback(&mut shardtree)?;
 

zcash_client_sqlite/src/wallet.rs

@@ -90,7 +90,7 @@ use zcash_client_backend::{
 };
 
 use crate::{
-    error::SqliteClientError, SqlTransaction, WalletCommitmentTrees, WalletDb, PRUNING_HEIGHT,
+    error::SqliteClientError, SqlTransaction, WalletCommitmentTrees, WalletDb, PRUNING_DEPTH,
 };
 
 #[cfg(feature = "transparent-inputs")]
@@ -543,94 +543,91 @@ pub(crate) fn block_height_extrema(
 
 fn parse_block_metadata(
     row: (BlockHeight, Vec<u8>, Option<u32>, Vec<u8>),
-) -> Option<Result<BlockMetadata, SqliteClientError>> {
+) -> Result<BlockMetadata, SqliteClientError> {
     let (block_height, hash_data, sapling_tree_size_opt, sapling_tree) = row;
-    let sapling_tree_size = sapling_tree_size_opt.map(Ok).or_else(|| {
+    let sapling_tree_size = sapling_tree_size_opt.map_or_else(|| {
         if sapling_tree == BLOCK_SAPLING_FRONTIER_ABSENT {
-            None
+            Err(SqliteClientError::CorruptedData("One of either the Sapling tree size or the legacy Sapling commitment tree must be present.".to_owned()))
         } else {
             // parse the legacy commitment tree data
             read_commitment_tree::<
                 zcash_primitives::sapling::Node,
                 _,
                 { zcash_primitives::sapling::NOTE_COMMITMENT_TREE_DEPTH },
             >(Cursor::new(sapling_tree))
-            .map(|tree| Some(tree.size().try_into().unwrap()))
+            .map(|tree| tree.size().try_into().unwrap())
             .map_err(SqliteClientError::from)
-            .transpose()
         }
-    })?;
+    }, Ok)?;
 
     let block_hash = BlockHash::try_from_slice(&hash_data).ok_or_else(|| {
         SqliteClientError::from(io::Error::new(
             io::ErrorKind::InvalidData,
             format!("Invalid block hash length: {}", hash_data.len()),
         ))
-    });
+    })?;
 
-    Some(sapling_tree_size.and_then(|sapling_tree_size| {
-        block_hash.map(|block_hash| {
-            BlockMetadata::from_parts(block_height, block_hash, sapling_tree_size)
-        })
-    }))
+    Ok(BlockMetadata::from_parts(
+        block_height,
+        block_hash,
+        sapling_tree_size,
+    ))
 }
 
 pub(crate) fn block_metadata(
     conn: &rusqlite::Connection,
     block_height: BlockHeight,
 ) -> Result<Option<BlockMetadata>, SqliteClientError> {
-    let res_opt = conn
-        .query_row(
-            "SELECT height, hash, sapling_commitment_tree_size, sapling_tree
+    conn.query_row(
+        "SELECT height, hash, sapling_commitment_tree_size, sapling_tree
             FROM blocks
             WHERE height = :block_height",
-            named_params![":block_height": u32::from(block_height)],
-            |row| {
-                let height: u32 = row.get(0)?;
-                let block_hash: Vec<u8> = row.get(1)?;
-                let sapling_tree_size: Option<u32> = row.get(2)?;
-                let sapling_tree: Vec<u8> = row.get(3)?;
-                Ok((
-                    BlockHeight::from(height),
-                    block_hash,
-                    sapling_tree_size,
-                    sapling_tree,
-                ))
-            },
-        )
-        .optional()?;
-
-    res_opt.and_then(parse_block_metadata).transpose()
+        named_params![":block_height": u32::from(block_height)],
+        |row| {
+            let height: u32 = row.get(0)?;
+            let block_hash: Vec<u8> = row.get(1)?;
+            let sapling_tree_size: Option<u32> = row.get(2)?;
+            let sapling_tree: Vec<u8> = row.get(3)?;
+            Ok((
+                BlockHeight::from(height),
+                block_hash,
+                sapling_tree_size,
+                sapling_tree,
+            ))
+        },
+    )
+    .optional()
+    .map_err(SqliteClientError::from)
+    .and_then(|meta_row| meta_row.map(parse_block_metadata).transpose())
 }
 
 pub(crate) fn block_fully_scanned(
     conn: &rusqlite::Connection,
 ) -> Result<Option<BlockMetadata>, SqliteClientError> {
     // FIXME: this will need to be rewritten once out-of-order scan range suggestion
     // is implemented.
-    let res_opt = conn
-        .query_row(
-            "SELECT height, hash, sapling_commitment_tree_size, sapling_tree
+    conn.query_row(
+        "SELECT height, hash, sapling_commitment_tree_size, sapling_tree
             FROM blocks
             ORDER BY height DESC
             LIMIT 1",
-            [],
-            |row| {
-                let height: u32 = row.get(0)?;
-                let block_hash: Vec<u8> = row.get(1)?;
-                let sapling_tree_size: Option<u32> = row.get(2)?;
-                let sapling_tree: Vec<u8> = row.get(3)?;
-                Ok((
-                    BlockHeight::from(height),
-                    block_hash,
-                    sapling_tree_size,
-                    sapling_tree,
-                ))
-            },
-        )
-        .optional()?;
-
-    res_opt.and_then(parse_block_metadata).transpose()
+        [],
+        |row| {
+            let height: u32 = row.get(0)?;
+            let block_hash: Vec<u8> = row.get(1)?;
+            let sapling_tree_size: Option<u32> = row.get(2)?;
+            let sapling_tree: Vec<u8> = row.get(3)?;
+            Ok((
+                BlockHeight::from(height),
+                block_hash,
+                sapling_tree_size,
+                sapling_tree,
+            ))
+        },
+    )
+    .optional()
+    .map_err(SqliteClientError::from)
+    .and_then(|meta_row| meta_row.map(parse_block_metadata).transpose())
 }
 
 /// Returns the block height at which the specified transaction was mined,
@@ -704,7 +701,7 @@ pub(crate) fn truncate_to_height<P: consensus::Parameters>(
             .map(|opt| opt.map_or_else(|| sapling_activation_height - 1, BlockHeight::from))
     })?;
 
-    if block_height < last_scanned_height - PRUNING_HEIGHT {
+    if block_height < last_scanned_height - PRUNING_DEPTH {
         if let Some(h) = get_min_unspent_height(conn)? {
             if block_height > h {
                 return Err(SqliteClientError::RequestedRewindInvalid(h, block_height));

zcash_client_sqlite/src/wallet/init.rs

@@ -21,7 +21,7 @@ use zcash_primitives::{
 
 use zcash_client_backend::{data_api::SAPLING_SHARD_HEIGHT, keys::UnifiedFullViewingKey};
 
-use crate::{error::SqliteClientError, wallet, WalletDb, SAPLING_TABLES_PREFIX};
+use crate::{error::SqliteClientError, wallet, WalletDb, PRUNING_DEPTH, SAPLING_TABLES_PREFIX};
 
 use super::commitment_tree::SqliteShardStore;
 
@@ -327,7 +327,7 @@ pub fn init_blocks_table<P: consensus::Parameters>(
                 _,
                 { sapling::NOTE_COMMITMENT_TREE_DEPTH },
                 SAPLING_SHARD_HEIGHT,
-            > = ShardTree::new(shard_store, 100);
+            > = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap());
             shard_tree.insert_frontier_nodes(
                 nonempty_frontier.clone(),
                 Retention::Checkpoint {

zcash_client_sqlite/src/wallet/init/migrations/shardtree_support.rs

@@ -23,7 +23,7 @@ use crate::{
         commitment_tree::SqliteShardStore,
         init::{migrations::received_notes_nullable_nf, WalletMigrationError},
     },
-    SAPLING_TABLES_PREFIX,
+    PRUNING_DEPTH, SAPLING_TABLES_PREFIX,
 };
 
 pub(super) const MIGRATION_ID: Uuid = Uuid::from_fields(
@@ -103,7 +103,7 @@ impl RusqliteMigration for Migration {
             _,
             { sapling::NOTE_COMMITMENT_TREE_DEPTH },
             SAPLING_SHARD_HEIGHT,
-        > = ShardTree::new(shard_store, 100);
+        > = ShardTree::new(shard_store, PRUNING_DEPTH.try_into().unwrap());
         // Insert all the tree information that we can get from block-end commitment trees
         {
             let mut stmt_blocks = transaction.prepare("SELECT height, sapling_tree FROM blocks")?;