Merge pull request #867 from nuttycom/feature/pre_dag_sync-require_sc…

…an_range

zcash_client_backend: Make scan range bounds required in `data_api::chain::scan_cached_blocks`

zcash_client_backend/CHANGELOG.md

@@ -11,14 +11,15 @@ and this library adheres to Rust's notion of
 - `impl Eq for zcash_client_backend::zip321::{Payment, TransactionRequest}`
 - `impl Debug` for `zcash_client_backend::{data_api::wallet::input_selection::Proposal, wallet::ReceivedSaplingNote}`
 - `zcash_client_backend::data_api`:
+  - `BlockMetadata`
+  - `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers`
+  - `ScannedBlock`
+  - `ShieldedProtocol`
   - `WalletRead::{block_metadata, block_fully_scanned, suggest_scan_ranges}`
   - `WalletWrite::put_block`
   - `WalletCommitmentTrees`
-  - `testing::MockWalletDb::new`
-  - `NullifierQuery` for use with `WalletRead::get_sapling_nullifiers`
-  - `BlockMetadata`
-  - `ScannedBlock`
   - `chain::CommitmentTreeRoot`
+  - `testing::MockWalletDb::new`
   - `wallet::input_sellection::Proposal::{min_target_height, min_anchor_height}`:
 - `zcash_client_backend::wallet::WalletSaplingOutput::note_commitment_tree_position`
 - `zcash_client_backend::scanning::ScanError`
@@ -35,7 +36,8 @@ and this library adheres to Rust's notion of
     and its signature has changed; it now subsumes the removed `WalletRead::get_all_nullifiers`.
   - `WalletRead::get_target_and_anchor_heights` now takes its argument as a `NonZeroU32`
   - `chain::scan_cached_blocks` now takes a `from_height` argument that
-    permits the caller to control the starting position of the scan range.
+    permits the caller to control the starting position of the scan range
+    In addition, the `limit` parameter is now required.
   - A new `CommitmentTree` variant has been added to `data_api::error::Error`
   - `data_api::wallet::{create_spend_to_address, create_proposed_transaction,
     shield_transparent_funds}` all now require that `WalletCommitmentTrees` be
@@ -49,6 +51,8 @@ and this library adheres to Rust's notion of
     now take their respective `min_confirmations` arguments as `NonZeroU32`
   - 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`.
+  - The variants of the `PoolType` enum have changed; the `PoolType::Sapling` variant has been 
+    removed in favor of a `PoolType::Shielded` variant that wraps a `ShieldedProtocol` value.
 - `zcash_client_backend::wallet`:
   - `SpendableNote` has been renamed to `ReceivedSaplingNote`.
   - Arguments to `WalletSaplingOutput::from_parts` have changed.

zcash_client_backend/src/data_api.rs

@@ -372,14 +372,21 @@ pub struct SentTransaction<'a> {
     pub utxos_spent: Vec<OutPoint>,
 }
 
+/// A shielded transfer protocol supported by the wallet.
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum ShieldedProtocol {
+    /// The Sapling protocol
+    Sapling,
+    // TODO: Orchard
+}
+
 /// A value pool to which the wallet supports sending transaction outputs.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum PoolType {
     /// The transparent value pool
     Transparent,
-    /// The Sapling value pool
-    Sapling,
-    // TODO: Orchard
+    /// A shielded value pool.
+    Shielded(ShieldedProtocol),
 }
 
 /// A type that represents the recipient of a transaction output; a recipient address (and, for
@@ -487,7 +494,6 @@ pub trait WalletWrite: WalletRead {
     /// Updates the state of the wallet database by persisting the provided block information,
     /// along with the note commitments that were detected when scanning the block for transactions
     /// pertaining to this wallet.
-    #[allow(clippy::type_complexity)]
     fn put_block(
         &mut self,
         block: ScannedBlock<sapling::Nullifier>,

zcash_client_backend/src/data_api/chain.rs

@@ -45,12 +45,13 @@
 //! // At this point, the cache and scanned data are locally consistent (though not
 //! // necessarily consistent with the latest chain tip - this would be discovered the
 //! // next time this codepath is executed after new blocks are received).
-//! scan_cached_blocks(&network, &block_source, &mut db_data, None, None)
+//! scan_cached_blocks(&network, &block_source, &mut db_data, BlockHeight::from(0), 10)
 //! # }
 //! # }
 //! ```
 
 use zcash_primitives::{
+    block::BlockHash,
     consensus::{self, BlockHeight},
     sapling::{self, note_encryption::PreparedIncomingViewingKey},
     zip32::Scope,
@@ -60,9 +61,11 @@ use crate::{
     data_api::{NullifierQuery, WalletWrite},
     proto::compact_formats::CompactBlock,
     scan::BatchRunner,
-    scanning::{add_block_to_runner, scan_block_with_runner},
+    scanning::{add_block_to_runner, check_continuity, scan_block_with_runner},
 };
 
+use super::BlockMetadata;
+
 pub mod error;
 use error::Error;
 
@@ -141,8 +144,8 @@ pub fn scan_cached_blocks<ParamsT, DbT, BlockSourceT>(
     params: &ParamsT,
     block_source: &BlockSourceT,
     data_db: &mut DbT,
-    from_height: Option<BlockHeight>,
-    limit: Option<u32>,
+    from_height: BlockHeight,
+    limit: u32,
 ) -> Result<(), Error<DbT::Error, BlockSourceT::Error>>
 where
     ParamsT: consensus::Parameters + Send + 'static,
@@ -178,61 +181,94 @@ where
             .map(|(tag, ivk)| (tag, PreparedIncomingViewingKey::new(&ivk))),
     );
 
-    // Start at either the provided height, or where we synced up to previously.
-    let (scan_from, mut prior_block_metadata) = match from_height {
-        Some(h) => {
-            // if we are provided with a starting height, obtain the metadata for the previous
-            // block (if any is available)
-            (
-                Some(h),
-                if h > BlockHeight::from(0) {
-                    data_db.block_metadata(h - 1).map_err(Error::Wallet)?
-                } else {
-                    None
-                },
-            )
-        }
-        None => {
-            let last_scanned = data_db.block_fully_scanned().map_err(Error::Wallet)?;
-            last_scanned.map_or_else(|| (None, None), |m| (Some(m.block_height + 1), Some(m)))
-        }
+    let mut prior_block_metadata = if from_height > BlockHeight::from(0) {
+        data_db
+            .block_metadata(from_height - 1)
+            .map_err(Error::Wallet)?
+    } else {
+        None
     };
 
-    block_source.with_blocks::<_, DbT::Error>(scan_from, limit, |block: CompactBlock| {
-        add_block_to_runner(params, block, &mut batch_runner);
-        Ok(())
-    })?;
+    let mut continuity_check_metadata = prior_block_metadata;
+    block_source.with_blocks::<_, DbT::Error>(
+        Some(from_height),
+        Some(limit),
+        |block: CompactBlock| {
+            // check block continuity
+            if let Some(scan_error) = check_continuity(&block, continuity_check_metadata.as_ref()) {
+                return Err(Error::Scan(scan_error));
+            }
 
-    batch_runner.flush();
+            if from_height == BlockHeight::from(0) {
+                // We can always derive a valid `continuity_check_metadata` for the
+                // genesis block, even if the block source doesn't have
+                // `sapling_commitment_tree_size`. So briefly set it to a dummy value that
+                // ensures the `map` below produces the correct genesis block value.
+                assert!(continuity_check_metadata.is_none());
+                continuity_check_metadata = Some(BlockMetadata::from_parts(
+                    BlockHeight::from(0),
+                    BlockHash([0; 32]),
+                    0,
+                ));
+            }
+            continuity_check_metadata = continuity_check_metadata.as_ref().map(|m| {
+                BlockMetadata::from_parts(
+                    block.height(),
+                    block.hash(),
+                    block
+                        .chain_metadata
+                        .as_ref()
+                        .map(|m| m.sapling_commitment_tree_size)
+                        .unwrap_or_else(|| {
+                            m.sapling_tree_size()
+                                + u32::try_from(
+                                    block.vtx.iter().map(|tx| tx.outputs.len()).sum::<usize>(),
+                                )
+                                .unwrap()
+                        }),
+                )
+            });
 
-    block_source.with_blocks::<_, DbT::Error>(scan_from, limit, |block: CompactBlock| {
-        let scanned_block = scan_block_with_runner(
-            params,
-            block,
-            &dfvks,
-            &sapling_nullifiers,
-            prior_block_metadata.as_ref(),
-            Some(&mut batch_runner),
-        )
-        .map_err(Error::Scan)?;
+            add_block_to_runner(params, block, &mut batch_runner);
 
-        let spent_nf: Vec<&sapling::Nullifier> = scanned_block
-            .transactions
-            .iter()
-            .flat_map(|tx| tx.sapling_spends.iter().map(|spend| spend.nf()))
-            .collect();
+            Ok(())
+        },
+    )?;
 
-        sapling_nullifiers.retain(|(_, nf)| !spent_nf.contains(&nf));
-        sapling_nullifiers.extend(scanned_block.transactions.iter().flat_map(|tx| {
-            tx.sapling_outputs
+    batch_runner.flush();
+
+    block_source.with_blocks::<_, DbT::Error>(
+        Some(from_height),
+        Some(limit),
+        |block: CompactBlock| {
+            let scanned_block = scan_block_with_runner(
+                params,
+                block,
+                &dfvks,
+                &sapling_nullifiers,
+                prior_block_metadata.as_ref(),
+                Some(&mut batch_runner),
+            )
+            .map_err(Error::Scan)?;
+
+            let spent_nf: Vec<&sapling::Nullifier> = scanned_block
+                .transactions
                 .iter()
-                .map(|out| (out.account(), *out.nf()))
-        }));
+                .flat_map(|tx| tx.sapling_spends.iter().map(|spend| spend.nf()))
+                .collect();
+
+            sapling_nullifiers.retain(|(_, nf)| !spent_nf.contains(&nf));
+            sapling_nullifiers.extend(scanned_block.transactions.iter().flat_map(|tx| {
+                tx.sapling_outputs
+                    .iter()
+                    .map(|out| (out.account(), *out.nf()))
+            }));
 
-        prior_block_metadata = Some(*scanned_block.metadata());
-        data_db.put_block(scanned_block).map_err(Error::Wallet)?;
-        Ok(())
-    })?;
+            prior_block_metadata = Some(*scanned_block.metadata());
+            data_db.put_block(scanned_block).map_err(Error::Wallet)?;
+            Ok(())
+        },
+    )?;
 
     Ok(())
 }

zcash_client_backend/src/data_api/wallet.rs

@@ -37,6 +37,8 @@ use crate::{
 pub mod input_selection;
 use input_selection::{GreedyInputSelector, GreedyInputSelectorError, InputSelector};
 
+use super::ShieldedProtocol;
+
 #[cfg(feature = "transparent-inputs")]
 use {
     crate::wallet::WalletTransparentOutput,
@@ -550,7 +552,7 @@ where
                     payment.memo.clone().unwrap_or_else(MemoBytes::empty),
                 )?;
                 sapling_output_meta.push((
-                    Recipient::Unified(ua.clone(), PoolType::Sapling),
+                    Recipient::Unified(ua.clone(), PoolType::Shielded(ShieldedProtocol::Sapling)),
                     payment.amount,
                     payment.memo.clone(),
                 ));
@@ -589,7 +591,10 @@ where
                     MemoBytes::empty(),
                 )?;
                 sapling_output_meta.push((
-                    Recipient::InternalAccount(account, PoolType::Sapling),
+                    Recipient::InternalAccount(
+                        account,
+                        PoolType::Shielded(ShieldedProtocol::Sapling),
+                    ),
                     *amount,
                     change_memo.clone(),
                 ))
@@ -610,20 +615,23 @@ where
                     .output_index(i)
                     .expect("An output should exist in the transaction for each shielded payment.");
 
-                let received_as =
-                    if let Recipient::InternalAccount(account, PoolType::Sapling) = recipient {
-                        tx.sapling_bundle().and_then(|bundle| {
-                            try_sapling_note_decryption(
-                                params,
-                                proposal.min_target_height(),
-                                &internal_ivk,
-                                &bundle.shielded_outputs()[output_index],
-                            )
-                            .map(|(note, _, _)| (account, note))
-                        })
-                    } else {
-                        None
-                    };
+                let received_as = if let Recipient::InternalAccount(
+                    account,
+                    PoolType::Shielded(ShieldedProtocol::Sapling),
+                ) = recipient
+                {
+                    tx.sapling_bundle().and_then(|bundle| {
+                        try_sapling_note_decryption(
+                            params,
+                            proposal.min_target_height(),
+                            &internal_ivk,
+                            &bundle.shielded_outputs()[output_index],
+                        )
+                        .map(|(note, _, _)| (account, note))
+                    })
+                } else {
+                    None
+                };
 
                 SentTransactionOutput::from_parts(output_index, recipient, value, memo, received_as)
             });