Merge pull request #1122 from zcash/light-client-perf

Improve light client performance

zcash_client_backend/CHANGELOG.md

@@ -21,6 +21,7 @@ and this library adheres to Rust's notion of
       with_orchard_balance_mut,
       add_unshielded_value
     }`
+  - `WalletSummary::next_sapling_subtree_index`
   - `wallet::propose_standard_transfer_to_address`
   - `wallet::input_selection::Proposal::{from_parts, shielded_inputs}`
   - `wallet::input_selection::ShieldedInputs`
@@ -81,6 +82,8 @@ and this library adheres to Rust's notion of
   - Fields of `Balance` and `AccountBalance` have been made private and the values
     of these fields have been made available via methods having the same names
     as the previously-public fields.
+  - `WalletSummary::new` now takes an additional `next_sapling_subtree_index`
+    argument.
   - `WalletWrite::get_next_available_address` now takes an additional 
     `UnifiedAddressRequest` argument.
   - `chain::scan_cached_blocks` now returns a `ScanSummary` containing metadata

zcash_client_backend/src/data_api.rs

@@ -288,6 +288,7 @@ pub struct WalletSummary {
     chain_tip_height: BlockHeight,
     fully_scanned_height: BlockHeight,
     scan_progress: Option<Ratio<u64>>,
+    next_sapling_subtree_index: u64,
 }
 
 impl WalletSummary {
@@ -297,12 +298,14 @@ impl WalletSummary {
         chain_tip_height: BlockHeight,
         fully_scanned_height: BlockHeight,
         scan_progress: Option<Ratio<u64>>,
+        next_sapling_subtree_idx: u64,
     ) -> Self {
         Self {
             account_balances,
             chain_tip_height,
             fully_scanned_height,
             scan_progress,
+            next_sapling_subtree_index: next_sapling_subtree_idx,
         }
     }
 
@@ -332,6 +335,12 @@ impl WalletSummary {
         self.scan_progress
     }
 
+    /// Returns the Sapling subtree index that should start the next range of subtree
+    /// roots passed to [`WalletCommitmentTrees::put_sapling_subtree_roots`].
+    pub fn next_sapling_subtree_index(&self) -> u64 {
+        self.next_sapling_subtree_index
+    }
+
     /// Returns whether or not wallet scanning is complete.
     pub fn is_synced(&self) -> bool {
         self.chain_tip_height == self.fully_scanned_height

zcash_client_sqlite/src/lib.rs

@@ -302,8 +302,10 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
         &self,
         min_confirmations: u32,
     ) -> Result<Option<WalletSummary>, Self::Error> {
+        // This will return a runtime error if we call `get_wallet_summary` from two
+        // threads at the same time, as transactions cannot nest.
         wallet::get_wallet_summary(
-            self.conn.borrow(),
+            &self.conn.borrow().unchecked_transaction()?,
             &self.params,
             min_confirmations,
             &SubtreeScanProgress,

zcash_client_sqlite/src/testing.rs

@@ -683,15 +683,7 @@ impl<Cache> TestState<Cache> {
         min_confirmations: u32,
         f: F,
     ) -> T {
-        let binding = get_wallet_summary(
+        let binding = self.get_wallet_summary(min_confirmations).unwrap();
-            &self.wallet().conn,
-            &self.wallet().params,
-            min_confirmations,
-            &SubtreeScanProgress,
-        )
-        .unwrap()
-        .unwrap();
-
         f(binding.account_balances().get(&account).unwrap())
     }
 
@@ -734,7 +726,7 @@ impl<Cache> TestState<Cache> {
 
     pub(crate) fn get_wallet_summary(&self, min_confirmations: u32) -> Option<WalletSummary> {
         get_wallet_summary(
-            &self.wallet().conn,
+            &self.wallet().conn.unchecked_transaction().unwrap(),
             &self.wallet().params,
             min_confirmations,
             &SubtreeScanProgress,

zcash_client_sqlite/src/wallet.rs

@@ -66,7 +66,7 @@
 
 use incrementalmerkletree::Retention;
 use rusqlite::{self, named_params, OptionalExtension};
-use shardtree::ShardTree;
+use shardtree::{error::ShardTreeError, store::ShardStore, ShardTree};
 use std::collections::{BTreeMap, HashMap};
 use std::convert::TryFrom;
 use std::io::{self, Cursor};
@@ -104,7 +104,7 @@ use crate::{
     SAPLING_TABLES_PREFIX,
 };
 
-use self::scanning::{parse_priority_code, replace_queue_entries};
+use self::scanning::{parse_priority_code, priority_code, replace_queue_entries};
 
 #[cfg(feature = "transparent-inputs")]
 use {
@@ -486,9 +486,11 @@ pub(crate) trait ScanProgress {
     ) -> Result<Option<Ratio<u64>>, SqliteClientError>;
 }
 
+#[derive(Debug)]
 pub(crate) struct SubtreeScanProgress;
 
 impl ScanProgress for SubtreeScanProgress {
+    #[tracing::instrument(skip(conn))]
     fn sapling_scan_progress(
         &self,
         conn: &rusqlite::Connection,
@@ -572,13 +574,14 @@ impl ScanProgress for SubtreeScanProgress {
 ///
 /// `min_confirmations` can be 0, but that case is currently treated identically to
 /// `min_confirmations == 1` for shielded notes. This behaviour may change in the future.
+#[tracing::instrument(skip(tx, params, progress))]
 pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
-    conn: &rusqlite::Connection,
+    tx: &rusqlite::Transaction,
     params: &P,
     min_confirmations: u32,
     progress: &impl ScanProgress,
 ) -> Result<Option<WalletSummary>, SqliteClientError> {
-    let chain_tip_height = match scan_queue_extrema(conn)? {
+    let chain_tip_height = match scan_queue_extrema(tx)? {
         Some(range) => *range.end(),
         None => {
             return Ok(None);
@@ -586,14 +589,14 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
     };
 
     let birthday_height =
-        wallet_birthday(conn)?.expect("If a scan range exists, we know the wallet birthday.");
+        wallet_birthday(tx)?.expect("If a scan range exists, we know the wallet birthday.");
 
     let fully_scanned_height =
-        block_fully_scanned(conn, params)?.map_or(birthday_height - 1, |m| m.block_height());
+        block_fully_scanned(tx, params)?.map_or(birthday_height - 1, |m| m.block_height());
     let summary_height = (chain_tip_height + 1).saturating_sub(std::cmp::max(min_confirmations, 1));
 
     let sapling_scan_progress = progress.sapling_scan_progress(
-        conn,
+        tx,
         birthday_height,
         fully_scanned_height,
         chain_tip_height,
@@ -601,19 +604,27 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
 
     // If the shard containing the summary height contains any unscanned ranges that start below or
     // including that height, none of our balance is currently spendable.
-    let any_spendable = conn.query_row(
+    #[tracing::instrument(skip_all)]
-        "SELECT NOT EXISTS(
+    fn is_any_spendable(
-             SELECT 1 FROM v_sapling_shard_unscanned_ranges
+        conn: &rusqlite::Connection,
-             WHERE :summary_height
+        summary_height: BlockHeight,
-                BETWEEN subtree_start_height
+    ) -> Result<bool, SqliteClientError> {
-                AND IFNULL(subtree_end_height, :summary_height)
+        conn.query_row(
-             AND block_range_start <= :summary_height
+            "SELECT NOT EXISTS(
-         )",
+                 SELECT 1 FROM v_sapling_shard_unscanned_ranges
-        named_params![":summary_height": u32::from(summary_height)],
+                 WHERE :summary_height
-        |row| row.get::<_, bool>(0),
+                    BETWEEN subtree_start_height
-    )?;
+                    AND IFNULL(subtree_end_height, :summary_height)
+                 AND block_range_start <= :summary_height
+             )",
+            named_params![":summary_height": u32::from(summary_height)],
+            |row| row.get::<_, bool>(0),
+        )
+        .map_err(|e| e.into())
+    }
+    let any_spendable = is_any_spendable(tx, summary_height)?;
 
-    let mut stmt_accounts = conn.prepare_cached("SELECT account FROM accounts")?;
+    let mut stmt_accounts = tx.prepare_cached("SELECT account FROM accounts")?;
     let mut account_balances = stmt_accounts
         .query([])?
         .and_then(|row| {
@@ -623,7 +634,8 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
         })
         .collect::<Result<BTreeMap<AccountId, AccountBalance>, _>>()?;
 
-    let mut stmt_select_notes = conn.prepare_cached(
+    let sapling_trace = tracing::info_span!("stmt_select_notes").entered();
+    let mut stmt_select_notes = tx.prepare_cached(
         "SELECT n.account, n.value, n.is_change, scan_state.max_priority, t.block
          FROM sapling_received_notes n
          JOIN transactions t ON t.id_tx = n.tx
@@ -695,13 +707,15 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
             })?;
         }
     }
+    drop(sapling_trace);
 
     #[cfg(feature = "transparent-inputs")]
     {
+        let transparent_trace = tracing::info_span!("stmt_transparent_balances").entered();
         let zero_conf_height = (chain_tip_height + 1).saturating_sub(min_confirmations);
         let stable_height = chain_tip_height.saturating_sub(PRUNING_DEPTH);
 
-        let mut stmt_transparent_balances = conn.prepare(
+        let mut stmt_transparent_balances = tx.prepare(
             "SELECT u.received_by_account, SUM(u.value_zat)
              FROM utxos u
              LEFT OUTER JOIN transactions tx
@@ -727,13 +741,34 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
                 balances.add_unshielded_value(value)?;
             }
         }
+        drop(transparent_trace);
     }
 
+    let next_sapling_subtree_index = {
+        let shard_store =
+            SqliteShardStore::<_, ::sapling::Node, SAPLING_SHARD_HEIGHT>::from_connection(
+                tx,
+                SAPLING_TABLES_PREFIX,
+            )?;
+
+        // The last shard will be incomplete, and we want the next range to overlap with
+        // the last complete shard, so return the index of the second-to-last shard root.
+        shard_store
+            .get_shard_roots()
+            .map_err(ShardTreeError::Storage)?
+            .iter()
+            .rev()
+            .nth(1)
+            .map(|addr| addr.index())
+            .unwrap_or(0)
+    };
+
     let summary = WalletSummary::new(
         account_balances,
         chain_tip_height,
         fully_scanned_height,
         sapling_scan_progress,
+        next_sapling_subtree_index,
     );
 
     Ok(Some(summary))
@@ -1011,6 +1046,7 @@ fn parse_block_metadata<P: consensus::Parameters>(
     ))
 }
 
+#[tracing::instrument(skip(conn, params))]
 pub(crate) fn block_metadata<P: consensus::Parameters>(
     conn: &rusqlite::Connection,
     params: &P,
@@ -1041,6 +1077,7 @@ pub(crate) fn block_metadata<P: consensus::Parameters>(
     .and_then(|meta_row| meta_row.map(|r| parse_block_metadata(params, r)).transpose())
 }
 
+#[tracing::instrument(skip_all)]
 pub(crate) fn block_fully_scanned<P: consensus::Parameters>(
     conn: &rusqlite::Connection,
     params: &P,
@@ -1052,42 +1089,46 @@ pub(crate) fn block_fully_scanned<P: consensus::Parameters>(
         // `put_block`, and the effective combination of intra-range linear scanning and the nullifier
         // map ensures that we discover all wallet-related information within the contiguous range.
         //
-        // The fully-scanned height is therefore the greatest height in the first contiguous range of
+        // We also assume that every contiguous range of block heights in the `blocks` table has a
-        // block rows, which is a combined case of the "gaps and islands" and "greatest N per group"
+        // single matching entry in the `scan_queue` table with priority "Scanned". This requires no
+        // bugs in the scan queue update logic, which we have had before. However, a bug here would
+        // mean that we return a more conservative fully-scanned height, which likely just causes a
+        // performance regression.
+        //
+        // The fully-scanned height is therefore the last height that falls within the first range in
+        // the scan queue with priority "Scanned".
         // SQL query problems.
-        conn.query_row(
+        let fully_scanned_height = match conn
-            "SELECT height, hash, sapling_commitment_tree_size, sapling_tree, orchard_commitment_tree_size
+            .query_row(
-            FROM blocks
+                "SELECT block_range_start, block_range_end
-            INNER JOIN (
+                FROM scan_queue
-                WITH contiguous AS (
+                WHERE priority = :priority
-                    SELECT height, ROW_NUMBER() OVER (ORDER BY height) - height AS grp
+                ORDER BY block_range_start ASC
-                    FROM blocks
+                LIMIT 1",
-                )
+                named_params![":priority": priority_code(&ScanPriority::Scanned)],
-                SELECT MIN(height) AS group_min_height, MAX(height) AS group_max_height
+                |row| {
-                FROM contiguous
+                    let block_range_start = BlockHeight::from_u32(row.get(0)?);
-                GROUP BY grp
+                    let block_range_end = BlockHeight::from_u32(row.get(1)?);
-                HAVING :birthday_height BETWEEN group_min_height AND group_max_height
+
+                    // If the start of the earliest scanned range is greater than
+                    // the birthday height, then there is an unscanned range between
+                    // the wallet birthday and that range, so there is no fully
+                    // scanned height.
+                    Ok(if block_range_start <= birthday_height {
+                        // Scan ranges are end-exclusive.
+                        Some(block_range_end - 1)
+                    } else {
+                        None
+                    })
+                },
             )
-            ON height = group_max_height",
+            .optional()?
-            named_params![":birthday_height": u32::from(birthday_height)],
+        {
-            |row| {
+            Some(Some(h)) => h,
-                let height: u32 = row.get(0)?;
+            _ => return Ok(None),
-                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)?;
+        block_metadata(conn, params, fully_scanned_height)
-                let orchard_tree_size: Option<u32> = row.get(4)?;
-                Ok((
-                    BlockHeight::from(height),
-                    block_hash,
-                    sapling_tree_size,
-                    sapling_tree,
-                    orchard_tree_size
-                ))
-            },
-        )
-        .optional()
-        .map_err(SqliteClientError::from)
-        .and_then(|meta_row| meta_row.map(|r| parse_block_metadata(params, r)).transpose())
     } else {
         Ok(None)
     }
@@ -1997,17 +2038,18 @@ pub(crate) fn prune_nullifier_map(
 mod tests {
     use std::num::NonZeroU32;
 
+    use sapling::zip32::ExtendedSpendingKey;
     use zcash_client_backend::data_api::{AccountBirthday, WalletRead};
+    use zcash_primitives::{block::BlockHash, transaction::components::amount::NonNegativeAmount};
 
-    use crate::{testing::TestBuilder, AccountId};
+    use crate::{
+        testing::{AddressType, BlockCache, TestBuilder, TestState},
+        AccountId,
+    };
 
     #[cfg(feature = "transparent-inputs")]
     use {
-        crate::{
+        crate::PRUNING_DEPTH,
-            testing::{AddressType, TestState},
-            PRUNING_DEPTH,
-        },
-        sapling::zip32::ExtendedSpendingKey,
         zcash_client_backend::{
             data_api::{wallet::input_selection::GreedyInputSelector, InputSource, WalletWrite},
             encoding::AddressCodec,
@@ -2017,7 +2059,7 @@ mod tests {
         zcash_primitives::{
             consensus::BlockHeight,
             transaction::{
-                components::{amount::NonNegativeAmount, Amount, OutPoint, TxOut},
+                components::{Amount, OutPoint, TxOut},
                 fees::fixed::FeeRule as FixedFeeRule,
             },
         },
@@ -2300,4 +2342,64 @@ mod tests {
         check_balance(&st, 1, value);
         check_balance(&st, 2, value);
     }
+
+    #[test]
+    fn block_fully_scanned() {
+        let mut st = TestBuilder::new()
+            .with_block_cache()
+            .with_test_account(AccountBirthday::from_sapling_activation)
+            .build();
+
+        let block_fully_scanned = |st: &TestState<BlockCache>| {
+            st.wallet()
+                .block_fully_scanned()
+                .unwrap()
+                .map(|meta| meta.block_height())
+        };
+
+        // A fresh wallet should have no fully-scanned block.
+        assert_eq!(block_fully_scanned(&st), None);
+
+        // Scan a block above the wallet's birthday height.
+        let not_our_key = ExtendedSpendingKey::master(&[]).to_diversifiable_full_viewing_key();
+        let not_our_value = NonNegativeAmount::const_from_u64(10000);
+        let end_height = st.sapling_activation_height() + 2;
+        let _ = st.generate_block_at(
+            end_height,
+            BlockHash([37; 32]),
+            &not_our_key,
+            AddressType::DefaultExternal,
+            not_our_value,
+            17,
+        );
+        st.scan_cached_blocks(end_height, 1);
+
+        // The wallet should still have no fully-scanned block, as no scanned block range
+        // overlaps the wallet's birthday.
+        assert_eq!(block_fully_scanned(&st), None);
+
+        // Scan the block at the wallet's birthday height.
+        let start_height = st.sapling_activation_height();
+        let _ = st.generate_block_at(
+            start_height,
+            BlockHash([0; 32]),
+            &not_our_key,
+            AddressType::DefaultExternal,
+            not_our_value,
+            0,
+        );
+        st.scan_cached_blocks(start_height, 1);
+
+        // The fully-scanned height should now be that of the scanned block.
+        assert_eq!(block_fully_scanned(&st), Some(start_height));
+
+        // Scan the block in between the two previous blocks.
+        let (h, _, _) =
+            st.generate_next_block(&not_our_key, AddressType::DefaultExternal, not_our_value);
+        st.scan_cached_blocks(h, 1);
+
+        // The fully-scanned height should now be the latest block, as the two disjoint
+        // ranges have been connected.
+        assert_eq!(block_fully_scanned(&st), Some(end_height));
+    }
 }

zcash_client_sqlite/src/wallet/commitment_tree.rs

@@ -391,6 +391,7 @@ pub(crate) fn last_shard<H: HashSer>(
 /// Returns an error iff the proposed insertion range
 /// for the tree shards would create a discontinuity
 /// in the database.
+#[tracing::instrument(skip(conn))]
 fn check_shard_discontinuity(
     conn: &rusqlite::Connection,
     table_prefix: &'static str,
@@ -517,6 +518,7 @@ pub(crate) fn truncate(
     .map(|_| ())
 }
 
+#[tracing::instrument(skip(conn))]
 pub(crate) fn get_cap<H: HashSer>(
     conn: &rusqlite::Connection,
     table_prefix: &'static str,
@@ -534,6 +536,7 @@ pub(crate) fn get_cap<H: HashSer>(
     )
 }
 
+#[tracing::instrument(skip(conn, cap))]
 pub(crate) fn put_cap<H: HashSer>(
     conn: &rusqlite::Connection,
     table_prefix: &'static str,
@@ -949,6 +952,7 @@ pub(crate) fn truncate_checkpoints(
     Ok(())
 }
 
+#[tracing::instrument(skip(conn, roots))]
 pub(crate) fn put_shard_roots<
     H: Hashable + HashSer + Clone + Eq,
     const DEPTH: u8,
@@ -1004,6 +1008,7 @@ pub(crate) fn put_shard_roots<
             .map_err(ShardTreeError::Storage)?,
     );
 
+    let insert_into_cap = tracing::info_span!("insert_into_cap").entered();
     let cap_result = cap
         .batch_insert(
             Position::from(start_index),
@@ -1019,6 +1024,7 @@ pub(crate) fn put_shard_roots<
         )
         .map_err(ShardTreeError::Insert)?
         .expect("slice of inserted roots was verified to be nonempty");
+    drop(insert_into_cap);
 
     put_cap(conn, table_prefix, cap_result.subtree.take_root()).map_err(ShardTreeError::Storage)?;
 
@@ -1029,6 +1035,7 @@ pub(crate) fn put_shard_roots<
     )
     .map_err(ShardTreeError::Storage)?;
 
+    let put_roots = tracing::info_span!("write_shards").entered();
     for (root, i) in roots.iter().zip(0u64..) {
         // We want to avoid deserializing the subtree just to annotate its root node, so we simply
         // cache the downloaded root alongside of any already-persisted subtree. We will update the
@@ -1063,6 +1070,7 @@ pub(crate) fn put_shard_roots<
         ])
         .map_err(|e| ShardTreeError::Storage(Error::Query(e)))?;
     }
+    drop(put_roots);
 
     Ok(())
 }

zcash_client_sqlite/src/wallet/scanning.rs

@@ -636,6 +636,16 @@ pub(crate) mod tests {
                 scan_range((sap_active + 300)..(sap_active + 310), ChainTip)
             ]
         );
+
+        // The wallet summary should be requesting the second-to-last root, as the last
+        // shard is incomplete.
+        assert_eq!(
+            st.wallet()
+                .get_wallet_summary(0)
+                .unwrap()
+                .map(|s| s.next_sapling_subtree_index()),
+            Some(2),
+        );
     }
 
     pub(crate) fn test_with_canopy_birthday() -> (
@@ -970,6 +980,10 @@ pub(crate) mod tests {
         // We have scanned a block, so we now have a starting tree position, 500 blocks above the
         // wallet birthday but before the end of the shard.
         let summary = st.get_wallet_summary(1);
+        assert_eq!(
+            summary.as_ref().map(|s| s.next_sapling_subtree_index()),
+            Some(0),
+        );
         assert_eq!(
             summary.and_then(|s| s.scan_progress()),
             Some(Ratio::new(1, 0x1 << SAPLING_SHARD_HEIGHT))