zcash_client_sqlite: Ensure that target and anchor heights are relative to the chain tip.

Prior to the scan-before-sync changes, the wallet was able to assume
that the maximum scanned block height at the time of the spend was
within a few blocks of the chain tip. However, under linear scanning
after the spend-before-sync changes this invariant no longer holds,
resulting in a situation where in linear sync conditions the wallet
could attempt to create transactions with already-past expiry heights.

This change separates the notion of "chain tip" from "max scanned
height", relying upon the `scan_queue` table to maintain the wallet's
view of the consensus chain height and using information from the
`blocks` table only in situations where the latest and/or earliest
scanned height is required.

As part of this change, the `WalletRead` interface is also modified to
disambiguate these concepts.

zcash_client_backend/CHANGELOG.md

@@ -17,7 +17,7 @@ and this library adheres to Rust's notion of
   - `ScannedBlock`
   - `ShieldedProtocol`
   - `WalletCommitmentTrees`
-  - `WalletRead::{block_metadata, block_fully_scanned, suggest_scan_ranges}`
+  - `WalletRead::{chain_height, block_metadata, block_fully_scanned, suggest_scan_ranges}`
   - `WalletWrite::{put_blocks, update_chain_tip}`
   - `chain::CommitmentTreeRoot`
   - `scanning` A new module containing types required for `suggest_scan_ranges`
@@ -89,10 +89,13 @@ and this library adheres to Rust's notion of
 
 ### Removed
 - `zcash_client_backend::data_api`:
-  - `WalletRead::get_all_nullifiers`
-  - `WalletRead::{get_commitment_tree, get_witnesses}` have been removed 
-    without replacement. The utility of these methods is now subsumed
-    by those available from the `WalletCommitmentTrees` trait.
+  - `WalletRead::block_height_extrema` has been removed. Use `chain_height`
+    instead to obtain the wallet's view of the chain tip instead, or
+    `suggest_scan_ranges` to obtain information about blocks that need to be
+    scanned.
+  - `WalletRead::{get_all_nullifiers, get_commitment_tree, get_witnesses}` have
+    been removed without replacement. The utility of these methods is now
+    subsumed by those available from the `WalletCommitmentTrees` trait.
   - `WalletWrite::advance_by_block` (use `WalletWrite::put_blocks` instead).
   - `PrunedBlock` has been replaced by `ScannedBlock`
   - `testing::MockWalletDb`, which is available under the `test-dependencies`
@@ -107,6 +110,9 @@ and this library adheres to Rust's notion of
   have been removed as individual incremental witnesses are no longer tracked on a
   per-note basis. The global note commitment tree for the wallet should be used
   to obtain witnesses for spend operations instead.
+- Default implementations of `zcash_client_backend::data_api::WalletRead::{
+    get_target_and_anchor_heights, get_max_height_hash
+  }` have been removed. These should be implemented in a backend-specific fashion.
 
 
 ## [0.9.0] - 2023-04-28

zcash_client_backend/src/data_api.rs

@@ -1,6 +1,5 @@
 //! Interfaces for wallet data persistence & low-level wallet utilities.
 
-use std::cmp;
 use std::collections::HashMap;
 use std::fmt::Debug;
 use std::num::NonZeroU32;
@@ -60,10 +59,11 @@ pub trait WalletRead {
     /// or a UUID.
     type NoteRef: Copy + Debug + Eq + Ord;
 
-    /// Returns the minimum and maximum block heights for stored blocks.
+    /// Returns the height of the chain as known to the wallet as of the most recent call to
+    /// [`WalletWrite::update_chain_tip`].
     ///
-    /// This will return `Ok(None)` if no block data is present in the database.
-    fn block_height_extrema(&self) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error>;
+    /// This will return `Ok(None)` if the height of the current consensus chain tip is unknown.
+    fn chain_height(&self) -> Result<Option<BlockHeight>, Self::Error>;
 
     /// Returns the available block metadata for the block at the specified height, if any.
     fn block_metadata(&self, height: BlockHeight) -> Result<Option<BlockMetadata>, Self::Error>;
@@ -98,22 +98,7 @@ pub trait WalletRead {
     fn get_target_and_anchor_heights(
         &self,
         min_confirmations: NonZeroU32,
-    ) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
-        self.block_height_extrema().map(|heights| {
-            heights.map(|(min_height, max_height)| {
-                let target_height = max_height + 1;
-
-                // Select an anchor min_confirmations back from the target block,
-                // unless that would be before the earliest block we have.
-                let anchor_height = BlockHeight::from(cmp::max(
-                    u32::from(target_height).saturating_sub(min_confirmations.into()),
-                    u32::from(min_height),
-                ));
-
-                (target_height, anchor_height)
-            })
-        })
-    }
+    ) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error>;
 
     /// Returns the minimum block height corresponding to an unspent note in the wallet.
     fn get_min_unspent_height(&self) -> Result<Option<BlockHeight>, Self::Error>;
@@ -123,22 +108,10 @@ pub trait WalletRead {
     /// is not found in the database.
     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
-    /// in stored data.
+    /// Returns the block height and hash for the block at the maximum scanned block height.
     ///
-    /// This will return `Ok(None)` if no block data is present in the database.
-    fn get_max_height_hash(&self) -> Result<Option<(BlockHeight, BlockHash)>, Self::Error> {
-        self.block_height_extrema()
-            .and_then(|extrema_opt| {
-                extrema_opt
-                    .map(|(_, max_height)| {
-                        self.get_block_hash(max_height)
-                            .map(|hash_opt| hash_opt.map(move |hash| (max_height, hash)))
-                    })
-                    .transpose()
-            })
-            .map(|oo| oo.flatten())
-    }
+    /// This will return `Ok(None)` if no blocks have been scanned.
+    fn get_max_height_hash(&self) -> Result<Option<(BlockHeight, BlockHash)>, Self::Error>;
 
     /// Returns the block height in which the specified transaction was mined, or `Ok(None)` if the
     /// transaction is not in the main chain.
@@ -613,7 +586,7 @@ pub mod testing {
     use incrementalmerkletree::Address;
     use secrecy::{ExposeSecret, SecretVec};
     use shardtree::{error::ShardTreeError, store::memory::MemoryShardStore, ShardTree};
-    use std::{collections::HashMap, convert::Infallible};
+    use std::{collections::HashMap, convert::Infallible, num::NonZeroU32};
 
     use zcash_primitives::{
         block::BlockHash,
@@ -662,7 +635,14 @@ pub mod testing {
         type Error = ();
         type NoteRef = u32;
 
-        fn block_height_extrema(&self) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
+        fn chain_height(&self) -> Result<Option<BlockHeight>, Self::Error> {
+            Ok(None)
+        }
+
+        fn get_target_and_anchor_heights(
+            &self,
+            _min_confirmations: NonZeroU32,
+        ) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
             Ok(None)
         }
 
@@ -692,6 +672,10 @@ pub mod testing {
             Ok(None)
         }
 
+        fn get_max_height_hash(&self) -> Result<Option<(BlockHeight, BlockHash)>, Self::Error> {
+            Ok(None)
+        }
+
         fn get_tx_height(&self, _txid: TxId) -> Result<Option<BlockHeight>, Self::Error> {
             Ok(None)
         }

zcash_client_backend/src/data_api/wallet.rs

@@ -66,9 +66,7 @@ where
     // for mempool transactions.
     let height = data
         .get_tx_height(tx.txid())?
-        .or(data
-            .block_height_extrema()?
-            .map(|(_, max_height)| max_height + 1))
+        .or(data.chain_height()?.map(|max_height| max_height + 1))
         .or_else(|| params.activation_height(NetworkUpgrade::Sapling))
         .expect("Sapling activation height must be known.");
 

zcash_client_sqlite/src/chain.rs

@@ -370,7 +370,7 @@ mod tests {
         let (dfvk, _taddr) = init_test_accounts_table(&mut db_data);
 
         // Empty chain should return None
-        assert_matches!(db_data.get_max_height_hash(), Ok(None));
+        assert_matches!(db_data.chain_height(), Ok(None));
 
         // Create a fake CompactBlock sending value to the address
         let (cb, _) = fake_compact_block(

zcash_client_sqlite/src/lib.rs

@@ -38,7 +38,10 @@ use maybe_rayon::{
 };
 use rusqlite::{self, Connection};
 use secrecy::{ExposeSecret, SecretVec};
-use std::{borrow::Borrow, collections::HashMap, convert::AsRef, fmt, ops::Range, path::Path};
+use std::{
+    borrow::Borrow, collections::HashMap, convert::AsRef, fmt, num::NonZeroU32, ops::Range,
+    path::Path,
+};
 
 use incrementalmerkletree::Position;
 use shardtree::{error::ShardTreeError, ShardTree};
@@ -157,8 +160,10 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
     type Error = SqliteClientError;
     type NoteRef = ReceivedNoteId;
 
-    fn block_height_extrema(&self) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
-        wallet::block_height_extrema(self.conn.borrow()).map_err(SqliteClientError::from)
+    fn chain_height(&self) -> Result<Option<BlockHeight>, Self::Error> {
+        wallet::scan_queue_extrema(self.conn.borrow())
+            .map(|h| h.map(|(_, max)| max))
+            .map_err(SqliteClientError::from)
     }
 
     fn block_metadata(&self, height: BlockHeight) -> Result<Option<BlockMetadata>, Self::Error> {
@@ -174,6 +179,14 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
             .map_err(SqliteClientError::from)
     }
 
+    fn get_target_and_anchor_heights(
+        &self,
+        min_confirmations: NonZeroU32,
+    ) -> Result<Option<(BlockHeight, BlockHeight)>, Self::Error> {
+        wallet::get_target_and_anchor_heights(self.conn.borrow(), min_confirmations)
+            .map_err(SqliteClientError::from)
+    }
+
     fn get_min_unspent_height(&self) -> Result<Option<BlockHeight>, Self::Error> {
         wallet::get_min_unspent_height(self.conn.borrow()).map_err(SqliteClientError::from)
     }
@@ -182,6 +195,10 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
         wallet::get_block_hash(self.conn.borrow(), block_height).map_err(SqliteClientError::from)
     }
 
+    fn get_max_height_hash(&self) -> Result<Option<(BlockHeight, BlockHash)>, Self::Error> {
+        wallet::get_max_height_hash(self.conn.borrow()).map_err(SqliteClientError::from)
+    }
+
     fn get_tx_height(&self, txid: TxId) -> Result<Option<BlockHeight>, Self::Error> {
         wallet::get_tx_height(self.conn.borrow(), txid).map_err(SqliteClientError::from)
     }

zcash_client_sqlite/src/wallet.rs

@@ -65,9 +65,11 @@
 //! - `memo` the shielded memo associated with the output, if any.
 
 use rusqlite::{self, named_params, OptionalExtension, ToSql};
+use std::cmp;
 use std::collections::HashMap;
 use std::convert::TryFrom;
 use std::io::{self, Cursor};
+use std::num::NonZeroU32;
 use zcash_client_backend::data_api::scanning::{ScanPriority, ScanRange};
 
 use zcash_client_backend::data_api::{NoteId, ShieldedProtocol};
@@ -623,6 +625,45 @@ pub(crate) fn block_height_extrema(
     })
 }
 
+/// Returns the minimum and maximum heights of blocks in the chain which may be scanned.
+pub(crate) fn scan_queue_extrema(
+    conn: &rusqlite::Connection,
+) -> Result<Option<(BlockHeight, BlockHeight)>, rusqlite::Error> {
+    conn.query_row(
+        "SELECT MIN(block_range_start), MAX(block_range_end) FROM scan_queue",
+        [],
+        |row| {
+            let min_height: Option<u32> = row.get(0)?;
+            let max_height: Option<u32> = row.get(1)?;
+
+            // Scan ranges are end-exclusive, so we subtract 1 from `max_height` to obtain the
+            // height of the last known chain tip;
+            Ok(min_height
+                .map(BlockHeight::from)
+                .zip(max_height.map(|h| BlockHeight::from(h.saturating_sub(1)))))
+        },
+    )
+}
+
+pub(crate) fn get_target_and_anchor_heights(
+    conn: &rusqlite::Connection,
+    min_confirmations: NonZeroU32,
+) -> Result<Option<(BlockHeight, BlockHeight)>, rusqlite::Error> {
+    scan_queue_extrema(conn).map(|heights| {
+        heights.map(|(min_height, max_height)| {
+            let target_height = max_height + 1;
+            // Select an anchor min_confirmations back from the target block,
+            // unless that would be before the earliest block we have.
+            let anchor_height = BlockHeight::from(cmp::max(
+                u32::from(target_height).saturating_sub(min_confirmations.into()),
+                u32::from(min_height),
+            ));
+
+            (target_height, anchor_height)
+        })
+    })
+}
+
 fn parse_block_metadata(
     row: (BlockHeight, Vec<u8>, Option<u32>, Vec<u8>),
 ) -> Result<BlockMetadata, SqliteClientError> {
@@ -765,6 +806,21 @@ pub(crate) fn get_block_hash(
     .optional()
 }
 
+pub(crate) fn get_max_height_hash(
+    conn: &rusqlite::Connection,
+) -> Result<Option<(BlockHeight, BlockHash)>, rusqlite::Error> {
+    conn.query_row(
+        "SELECT height, hash FROM blocks ORDER BY height DESC LIMIT 1",
+        [],
+        |row| {
+            let height = row.get::<_, u32>(0).map(BlockHeight::from)?;
+            let row_data = row.get::<_, Vec<_>>(1)?;
+            Ok((height, BlockHash::from_slice(&row_data)))
+        },
+    )
+    .optional()
+}
+
 /// Gets the height to which the database must be truncated if any truncation that would remove a
 /// number of blocks greater than the pruning height is attempted.
 pub(crate) fn get_min_unspent_height(
@@ -772,9 +828,9 @@ pub(crate) fn get_min_unspent_height(
 ) -> Result<Option<BlockHeight>, SqliteClientError> {
     conn.query_row(
         "SELECT MIN(tx.block)
-             FROM sapling_received_notes n
-             JOIN transactions tx ON tx.id_tx = n.tx
-             WHERE n.spent IS NULL",
+         FROM sapling_received_notes n
+         JOIN transactions tx ON tx.id_tx = n.tx
+         WHERE n.spent IS NULL",
         [],
         |row| {
             row.get(0)

zcash_client_sqlite/src/wallet/sapling.rs

@@ -368,12 +368,14 @@ pub(crate) mod tests {
 
     use zcash_primitives::{
         block::BlockHash,
-        consensus::{BlockHeight, BranchId},
+        consensus::BranchId,
         legacy::TransparentAddress,
         memo::Memo,
-        sapling::{note_encryption::try_sapling_output_recovery, prover::TxProver},
+        sapling::{
+            note_encryption::try_sapling_output_recovery, prover::TxProver, Note, PaymentAddress,
+        },
         transaction::{
-            components::Amount,
+            components::{amount::BalanceError, Amount},
             fees::{fixed::FeeRule as FixedFeeRule, zip317::FeeRule as Zip317FeeRule},
             Transaction,
         },
@@ -388,7 +390,8 @@ pub(crate) mod tests {
             error::Error,
             wallet::{
                 create_proposed_transaction, create_spend_to_address,
-                input_selection::GreedyInputSelector, propose_transfer, spend,
+                input_selection::{GreedyInputSelector, GreedyInputSelectorError},
+                propose_transfer, spend,
             },
             ShieldedProtocol, WalletRead, WalletWrite,
         },
@@ -401,15 +404,13 @@ pub(crate) mod tests {
 
     use crate::{
         chain::init::init_cache_database,
+        error::SqliteClientError,
         tests::{
             self, fake_compact_block, insert_into_cache, network, sapling_activation_height,
             AddressType,
         },
-        wallet::{
-            get_balance, get_balance_at,
-            init::{init_blocks_table, init_wallet_db},
-        },
-        AccountId, BlockDb, NoteId, WalletDb,
+        wallet::{commitment_tree, get_balance, get_balance_at, init::init_wallet_db},
+        AccountId, BlockDb, NoteId, ReceivedNoteId, WalletDb,
     };
 
     #[cfg(feature = "transparent-inputs")]
@@ -651,6 +652,12 @@ pub(crate) mod tests {
         let dfvk = usk.sapling().to_diversifiable_full_viewing_key();
         let to = dfvk.default_address().1.into();
 
+        // Account balance should be zero
+        assert_eq!(
+            get_balance(&db_data.conn, AccountId::from(0)).unwrap(),
+            Amount::zero()
+        );
+
         // We cannot do anything if we aren't synchronised
         assert_matches!(
             create_spend_to_address(
@@ -668,53 +675,6 @@ pub(crate) mod tests {
         );
     }
 
-    #[test]
-    fn create_to_address_fails_on_insufficient_balance() {
-        let data_file = NamedTempFile::new().unwrap();
-        let mut db_data = WalletDb::for_path(data_file.path(), tests::network()).unwrap();
-        init_wallet_db(&mut db_data, None).unwrap();
-        init_blocks_table(
-            &mut db_data,
-            BlockHeight::from(1u32),
-            BlockHash([1; 32]),
-            1,
-            &[0x0, 0x0, 0x0],
-        )
-        .unwrap();
-
-        // Add an account to the wallet
-        let seed = Secret::new([0u8; 32].to_vec());
-        let (_, usk) = db_data.create_account(&seed).unwrap();
-        let dfvk = usk.sapling().to_diversifiable_full_viewing_key();
-        let to = dfvk.default_address().1.into();
-
-        // Account balance should be zero
-        assert_eq!(
-            get_balance(&db_data.conn, AccountId::from(0)).unwrap(),
-            Amount::zero()
-        );
-
-        // We cannot spend anything
-        assert_matches!(
-            create_spend_to_address(
-                &mut db_data,
-                &tests::network(),
-                test_prover(),
-                &usk,
-                &to,
-                Amount::from_u64(1).unwrap(),
-                None,
-                OvkPolicy::Sender,
-                NonZeroU32::new(1).unwrap(),
-            ),
-            Err(data_api::error::Error::InsufficientFunds {
-                available,
-                required
-            })
-            if available == Amount::zero() && required == Amount::from_u64(10001).unwrap()
-        );
-    }
-
     #[test]
     fn create_to_address_fails_on_unverified_notes() {
         let cache_file = NamedTempFile::new().unwrap();
@@ -1101,7 +1061,19 @@ pub(crate) mod tests {
         let addr2 = extsk2.default_address().1;
         let to = addr2.into();
 
-        let send_and_recover_with_policy = |db_data: &mut WalletDb<Connection, _>, ovk_policy| {
+        #[allow(clippy::type_complexity)]
+        let send_and_recover_with_policy = |db_data: &mut WalletDb<Connection, _>,
+                                            ovk_policy|
+         -> Result<
+            Option<(Note, PaymentAddress, MemoBytes)>,
+            Error<
+                SqliteClientError,
+                commitment_tree::Error,
+                GreedyInputSelectorError<BalanceError, ReceivedNoteId>,
+                Infallible,
+                ReceivedNoteId,
+            >,
+        > {
             let txid = create_spend_to_address(
                 db_data,
                 &tests::network(),
@@ -1112,8 +1084,7 @@ pub(crate) mod tests {
                 None,
                 ovk_policy,
                 NonZeroU32::new(1).unwrap(),
-            )
-            .unwrap();
+            )?;
 
             // Fetch the transaction from the database
             let raw_tx: Vec<_> = db_data
@@ -1137,18 +1108,19 @@ pub(crate) mod tests {
                 );
 
                 if result.is_some() {
-                    return result;
+                    return Ok(result);
                 }
             }
 
-            None
+            Ok(None)
         };
 
         // Send some of the funds to another address, keeping history.
         // The recipient output is decryptable by the sender.
-        let (_, recovered_to, _) =
-            send_and_recover_with_policy(&mut db_data, OvkPolicy::Sender).unwrap();
-        assert_eq!(&recovered_to, &addr2);
+        assert_matches!(
+            send_and_recover_with_policy(&mut db_data, OvkPolicy::Sender),
+            Ok(Some((_, recovered_to, _))) if recovered_to == addr2
+        );
 
         // Mine blocks SAPLING_ACTIVATION_HEIGHT + 1 to 42 (that don't send us funds)
         // so that the first transaction expires
@@ -1175,7 +1147,10 @@ pub(crate) mod tests {
 
         // Send the funds again, discarding history.
         // Neither transaction output is decryptable by the sender.
-        assert!(send_and_recover_with_policy(&mut db_data, OvkPolicy::Discard).is_none());
+        assert_matches!(
+            send_and_recover_with_policy(&mut db_data, OvkPolicy::Discard),
+            Ok(None)
+        );
     }
 
     #[test]

zcash_client_sqlite/src/wallet/scanning.rs

@@ -619,12 +619,12 @@ pub(crate) fn update_chain_tip<P: consensus::Parameters>(
     params: &P,
     new_tip: BlockHeight,
 ) -> Result<(), SqliteClientError> {
-    // Read the previous tip height from the blocks table.
+    // Read the previous max scanned height from the blocks table
     let prior_tip = block_height_extrema(conn)?.map(|(_, prior_tip)| prior_tip);
 
-    // If the chain tip is below the prior tip height, then the caller has caught the
-    // chain in the middle of a reorg. Do nothing; the caller will continue using the old
-    // scan ranges and either:
+    // If the chain tip is below the prior max scanned height, then the caller has caught
+    // the chain in the middle of a reorg. Do nothing; the caller will continue using the
+    // old scan ranges and either:
     // - encounter an error trying to fetch the blocks (and thus trigger the same handling
     //   logic as if this happened with the old linear scanning code); or
     // - encounter a discontinuity error in `scan_cached_blocks`, at which point they will