zcash_client_sqlite: Ensure that target and anchor heights are relati…

…ve 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

@@ -12,7 +12,7 @@ and this library adheres to Rust's notion of
      wallet::ReceivedSaplingNote,
    }`
 - `impl Debug` for `zcash_client_backend::{
-     data_api::wallet::input_selection::Proposal, 
+     data_api::wallet::input_selection::Proposal,
      wallet::ReceivedSaplingNote
    }`
 - `impl Eq for zcash_client_backend::{
@@ -27,7 +27,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,
     get_spendable_sapling_note, get_unspent_transparent_output}`
   - `WalletWrite::{put_blocks, update_chain_tip}`
   - `chain::CommitmentTreeRoot`
@@ -128,10 +128,14 @@ 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; the `blocks`
+    table contains information about scanned blocks, but the wallet may be aware
+    that the chain tip is at a greater height than is represented in the `blocks`
+    table without having downloaded or scanned the blocks at the chain tip.
+  - `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`

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.
@@ -628,7 +601,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,
@@ -677,7 +650,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)
         }
 
@@ -707,6 +687,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

@@ -65,9 +65,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};
@@ -624,6 +626,43 @@ pub(crate) fn block_height_extrema(
     })
 }
 
+/// Returns the minimum and maximum heights for blocks known to the wallet.
+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)?;
+            Ok(min_height
+                .map(BlockHeight::from)
+                .zip(max_height.map(BlockHeight::from)))
+        },
+    )
+}
+
+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| {
+        // Scan ranges are end-exclusive, so we can use the max block_range_end value directly as
+        // the target height as it is one greater than the last known height of the chain tip.
+        heights.map(|(min_height, target_height)| {
+            // 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> {
@@ -766,16 +805,31 @@ 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(
     conn: &rusqlite::Connection,
 ) -> 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)