zcash_client_sqlite: Use transactions.mined_height instead of `tran…

…sactions.block_height` for mined-ness determinations.

zcash_client_sqlite/src/lib.rs

@@ -430,9 +430,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> WalletRead for W
     }
 
     fn chain_height(&self) -> Result<Option<BlockHeight>, Self::Error> {
-        wallet::scan_queue_extrema(self.conn.borrow())
-            .map(|h| h.map(|range| *range.end()))
-            .map_err(SqliteClientError::from)
+        wallet::chain_tip_height(self.conn.borrow()).map_err(SqliteClientError::from)
     }
 
     fn get_block_hash(&self, block_height: BlockHeight) -> Result<Option<BlockHash>, Self::Error> {

zcash_client_sqlite/src/wallet.rs

@@ -490,7 +490,7 @@ pub(crate) fn add_account<P: consensus::Parameters>(
 
     // Rewrite the scan ranges from the birthday height up to the chain tip so that we'll ensure we
     // re-scan to find any notes that might belong to the newly added account.
-    if let Some(t) = scan_queue_extrema(conn)?.map(|range| *range.end()) {
+    if let Some(t) = chain_tip_height(conn)? {
         let rescan_range = birthday.height()..(t + 1);
 
         replace_queue_entries::<SqliteClientError>(
@@ -952,8 +952,8 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
     min_confirmations: u32,
     progress: &impl ScanProgress,
 ) -> Result<Option<WalletSummary<AccountId>>, SqliteClientError> {
-    let chain_tip_height = match scan_queue_extrema(tx)? {
-        Some(range) => *range.end(),
+    let chain_tip_height = match chain_tip_height(tx)? {
+        Some(h) => h,
         None => {
             return Ok(None);
         }
@@ -1498,30 +1498,23 @@ pub(crate) fn get_account<P: Parameters>(
 }
 
 /// Returns the minimum and maximum heights of blocks in the chain which may be scanned.
-pub(crate) fn scan_queue_extrema(
+pub(crate) fn chain_tip_height(
     conn: &rusqlite::Connection,
-) -> Result<Option<RangeInclusive<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
-                .zip(max_height.map(|h| h.saturating_sub(1)))
-                .map(|(min, max)| RangeInclusive::new(min.into(), max.into())))
-        },
-    )
+) -> Result<Option<BlockHeight>, rusqlite::Error> {
+    conn.query_row("SELECT MAX(block_range_end) FROM scan_queue", [], |row| {
+        let max_height: Option<u32> = row.get(0)?;
+
+        // Scan ranges are end-exclusive, so we subtract 1 from `max_height` to obtain the
+        // height of the last known chain tip;
+        Ok(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> {
-    match scan_queue_extrema(conn)?.map(|range| *range.end()) {
+    match chain_tip_height(conn)? {
         Some(chain_tip_height) => {
             let sapling_anchor_height = get_max_checkpointed_height(
                 conn,

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

@@ -11,7 +11,7 @@ use zcash_client_backend::data_api::scanning::ScanPriority;
 use zcash_protocol::consensus::{self, BlockHeight, NetworkUpgrade};
 
 use super::shardtree_support;
-use crate::wallet::{init::WalletMigrationError, scan_queue_extrema, scanning::priority_code};
+use crate::wallet::{chain_tip_height, init::WalletMigrationError, scanning::priority_code};
 
 pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x3a6487f7_e068_42bb_9d12_6bb8dbe6da00);
 
@@ -142,10 +142,10 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
 
         // Treat the current best-known chain tip height as the height to use for Orchard
         // initialization, bounded below by NU5 activation.
-        if let Some(orchard_init_height) = scan_queue_extrema(transaction)?.and_then(|r| {
+        if let Some(orchard_init_height) = chain_tip_height(transaction)?.and_then(|h| {
             self.params
                 .activation_height(NetworkUpgrade::Nu5)
-                .map(|orchard_activation| std::cmp::max(orchard_activation, *r.end()))
+                .map(|orchard_activation| std::cmp::max(orchard_activation, h))
         }) {
             // If a scan range exists that contains the Orchard init height, split it in two at the
             // init height.

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

@@ -28,9 +28,10 @@ use zcash_primitives::{
 
 use crate::{
     wallet::{
+        chain_tip_height,
         commitment_tree::SqliteShardStore,
         init::{migrations::shardtree_support, WalletMigrationError},
-        scan_queue_extrema, scope_code,
+        scope_code,
     },
     PRUNING_DEPTH, SAPLING_TABLES_PREFIX,
 };
@@ -154,7 +155,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
             let zip212_height = tx_height.map_or_else(
                 || {
                     tx_expiry.filter(|h| *h != 0).map_or_else(
-                        || scan_queue_extrema(transaction).map(|extrema| extrema.map(|r| *r.end())),
+                        || chain_tip_height(transaction),
                         |h| Ok(Some(BlockHeight::from(h))),
                     )
                 },

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

@@ -81,7 +81,7 @@ impl RusqliteMigration for Migration {
                 FOREIGN KEY (account_id) REFERENCES accounts(id),
                 CONSTRAINT transparent_output_unique UNIQUE (transaction_id, output_index)
             );
-            CREATE INDEX idx_transparent_received_outputs_account_id 
+            CREATE INDEX idx_transparent_received_outputs_account_id
             ON "transparent_received_outputs" (account_id);
 
             INSERT INTO transparent_received_outputs SELECT
@@ -191,12 +191,12 @@ impl RusqliteMigration for Migration {
                 WITH
                 notes AS (
                     -- Outputs received in this transaction
-                    SELECT ro.account_id            AS account_id,
-                           transactions.block       AS block,
-                           transactions.txid        AS txid,
-                           ro.pool                  AS pool,
+                    SELECT ro.account_id              AS account_id,
+                           transactions.mined_height  AS mined_height,
+                           transactions.txid          AS txid,
+                           ro.pool                    AS pool,
                            id_within_pool_table,
-                           ro.value                 AS value,
+                           ro.value                   AS value,
                            CASE
                                 WHEN ro.is_change THEN 1
                                 ELSE 0
@@ -215,15 +215,15 @@ impl RusqliteMigration for Migration {
                          ON transactions.id_tx = ro.transaction_id
                     UNION
                     -- Outputs spent in this transaction
-                    SELECT ro.account_id            AS account_id,
-                           transactions.block       AS block,
-                           transactions.txid        AS txid,
-                           ro.pool                  AS pool,
+                    SELECT ro.account_id              AS account_id,
+                           transactions.mined_height  AS mined_height,
+                           transactions.txid          AS txid,
+                           ro.pool                    AS pool,
                            id_within_pool_table,
-                           -ro.value                AS value,
-                           0                        AS is_change,
-                           0                        AS received_count,
-                           0                        AS memo_present
+                           -ro.value                  AS value,
+                           0                          AS is_change,
+                           0                          AS received_count,
+                           0                          AS memo_present
                     FROM v_received_outputs ro
                     JOIN v_received_output_spends ros
                          ON ros.pool = ro.pool
@@ -256,7 +256,7 @@ impl RusqliteMigration for Migration {
                     SELECT MAX(blocks.height) AS max_height FROM blocks
                 )
                 SELECT notes.account_id             AS account_id,
-                       notes.block                  AS mined_height,
+                       notes.mined_height           AS mined_height,
                        notes.txid                   AS txid,
                        transactions.tx_index        AS tx_index,
                        transactions.expiry_height   AS expiry_height,
@@ -276,7 +276,7 @@ impl RusqliteMigration for Migration {
                 LEFT JOIN transactions
                      ON notes.txid = transactions.txid
                 JOIN blocks_max_height
-                LEFT JOIN blocks ON blocks.height = notes.block
+                LEFT JOIN blocks ON blocks.height = notes.mined_height
                 LEFT JOIN sent_note_counts
                      ON sent_note_counts.account_id = notes.account_id
                      AND sent_note_counts.txid = notes.txid

zcash_client_sqlite/src/wallet/transparent.rs

@@ -22,7 +22,7 @@ use zcash_protocol::consensus::{self, BlockHeight};
 
 use crate::{error::SqliteClientError, AccountId, UtxoId};
 
-use super::get_account_ids;
+use super::{chain_tip_height, get_account_ids};
 
 pub(crate) fn detect_spending_accounts<'a>(
     conn: &Connection,
@@ -189,32 +189,48 @@ fn to_unspent_transparent_output(row: &Row) -> Result<WalletTransparentOutput, S
     })
 }
 
+/// Select an output to fund a new transaction that is targeting at least `chain_tip_height + 1`.
 pub(crate) fn get_unspent_transparent_output(
     conn: &rusqlite::Connection,
     outpoint: &OutPoint,
 ) -> Result<Option<WalletTransparentOutput>, SqliteClientError> {
+    let chain_tip_height = chain_tip_height(conn)?;
+
+    // This could, in very rare circumstances, return as unspent outputs that are actually not
+    // spendable, if they are the outputs of deshielding transactions where the spend anchors have
+    // been invalidated by a rewind. There isn't a way to detect this circumstance at present, but
+    // it should be vanishingly rare as the vast majority of rewinds are of a single block.
     let mut stmt_select_utxo = conn.prepare_cached(
         "SELECT t.txid, u.output_index, u.script,
                 u.value_zat, t.mined_height AS received_height
          FROM transparent_received_outputs u
          JOIN transactions t ON t.id_tx = u.transaction_id
          WHERE t.txid = :txid
-         AND t.mined_height IS NOT NULL
          AND u.output_index = :output_index
+         -- the transaction that created the output is mined or is definitely unexpired
+         AND (
+            t.mined_height IS NOT NULL -- tx is mined
+            -- TODO: uncomment the following two lines in order to enable zero-conf spends
+            -- OR t.expiry_height = 0 -- tx will not expire
+            -- OR t.expiry_height >= :mempool_height -- tx has not yet expired
+         )
+         -- and the output is unspent
          AND u.id NOT IN (
             SELECT txo_spends.transparent_received_output_id
             FROM transparent_received_output_spends txo_spends
             JOIN transactions tx ON tx.id_tx = txo_spends.transaction_id
-            WHERE tx.block IS NOT NULL  -- the spending tx is mined
-            OR tx.expiry_height IS NULL -- the spending tx will not expire
+            WHERE tx.mined_height IS NOT NULL  -- the spending tx is mined
+            OR tx.expiry_height = 0 -- the spending tx will not expire
+            OR tx.expiry_height >= :mempool_height -- the spending tx has not yet expired
          )",
     )?;
 
     let result: Result<Option<WalletTransparentOutput>, SqliteClientError> = stmt_select_utxo
         .query_and_then(
             named_params![
                 ":txid": outpoint.hash(),
-                ":output_index": outpoint.n()
+                ":output_index": outpoint.n(),
+                ":mempool_height": chain_tip_height.map(|h| u32::from(h) + 1),
             ],
             to_unspent_transparent_output,
         )?
@@ -234,21 +250,38 @@ pub(crate) fn get_unspent_transparent_outputs<P: consensus::Parameters>(
     max_height: BlockHeight,
     exclude: &[OutPoint],
 ) -> Result<Vec<WalletTransparentOutput>, SqliteClientError> {
+    //let chain_tip_height = chain_tip_height(conn)?.ok_or(SqliteClientError::ChainHeightUnknown)?;
+
+    // This could, in very rare circumstances, return as unspent outputs that are actually not
+    // spendable, if they are the outputs of deshielding transactions where the spend anchors have
+    // been invalidated by a rewind. There isn't a way to detect this circumstance at present, but
+    // it should be vanishingly rare as the vast majority of rewinds are of a single block.
     let mut stmt_utxos = conn.prepare(
         "SELECT t.txid, u.output_index, u.script,
                 u.value_zat, t.mined_height AS received_height
          FROM transparent_received_outputs u
          JOIN transactions t ON t.id_tx = u.transaction_id
          WHERE u.address = :address
-         AND t.mined_height <= :max_height
+         -- the transaction that created the output is mined or is definitely unexpired
+         AND (
+            t.mined_height <= :max_height -- tx is mined
+            -- TODO: uncomment the following lines in order to enable zero-conf spends
+            -- OR (
+            --     :max_height > :chain_tip_height
+            --     AND (
+            --         t.expiry_height = 0 -- tx will not expire
+            --         OR t.expiry_height >= :max_height
+            --     )
+            -- )
+         )
+         -- and the output is unspent
          AND u.id NOT IN (
             SELECT txo_spends.transparent_received_output_id
             FROM transparent_received_output_spends txo_spends
             JOIN transactions tx ON tx.id_tx = txo_spends.transaction_id
-            WHERE
-              tx.block IS NOT NULL -- the spending tx is mined
-              OR tx.expiry_height IS NULL -- the spending tx will not expire
-              OR tx.expiry_height > :max_height -- the spending tx is unexpired
+            WHERE tx.mined_height IS NOT NULL -- the spending tx is mined
+            OR tx.expiry_height = 0 -- the spending tx will not expire
+            OR tx.expiry_height >= :max_height -- the spending tx has not yet expired
          )",
     )?;
 
@@ -258,7 +291,7 @@ pub(crate) fn get_unspent_transparent_outputs<P: consensus::Parameters>(
     let mut rows = stmt_utxos.query(named_params![
         ":address": addr_str,
         ":max_height": u32::from(max_height),
-        ":max_height": u32::from(max_height),
+        //":chain_tip_height": u32::from(chain_tip_height)
     ])?;
     let excluded: BTreeSet<OutPoint> = exclude.iter().cloned().collect();
     while let Some(row) = rows.next()? {
@@ -282,21 +315,34 @@ pub(crate) fn get_transparent_address_balances<P: consensus::Parameters>(
     account: AccountId,
     max_height: BlockHeight,
 ) -> Result<HashMap<TransparentAddress, NonNegativeAmount>, SqliteClientError> {
+    // let chain_tip_height = chain_tip_height(conn)?.ok_or(SqliteClientError::ChainHeightUnknown)?;
+
     let mut stmt_address_balances = conn.prepare(
         "SELECT u.address, SUM(u.value_zat)
          FROM transparent_received_outputs u
          JOIN transactions t
          ON t.id_tx = u.transaction_id
          WHERE u.account_id = :account_id
-         AND t.mined_height <= :max_height
+         -- the transaction that created the output is mined or is definitely unexpired
+         AND (
+            t.mined_height <= :max_height -- tx is mined
+            -- TODO: uncomment the following lines in order to enable zero-conf spends
+            -- OR (
+            --     :max_height > :chain_tip_height
+            --     AND (
+            --         t.expiry_height = 0 -- tx will not expire
+            --         OR t.expiry_height >= :max_height
+            --     )
+            -- )
+         )
+         -- and the output is unspent
          AND u.id NOT IN (
             SELECT txo_spends.transparent_received_output_id
             FROM transparent_received_output_spends txo_spends
             JOIN transactions tx ON tx.id_tx = txo_spends.transaction_id
-            WHERE
-              tx.block IS NOT NULL -- the spending tx is mined
-              OR tx.expiry_height IS NULL -- the spending tx will not expire
-              OR tx.expiry_height > :max_height -- the spending tx is unexpired
+            WHERE tx.mined_height IS NOT NULL -- the spending tx is mined
+            OR tx.expiry_height IS NULL -- the spending tx will not expire
+            OR tx.expiry_height > :max_height -- the spending tx is unexpired
          )
          GROUP BY u.address",
     )?;
@@ -305,6 +351,7 @@ pub(crate) fn get_transparent_address_balances<P: consensus::Parameters>(
     let mut rows = stmt_address_balances.query(named_params![
         ":account_id": account.0,
         ":max_height": u32::from(max_height),
+        //":chain_tip_height": u32::from(chain_tip_height),
     ])?;
     while let Some(row) = rows.next()? {
         let taddr_str: String = row.get(0)?;
@@ -322,26 +369,41 @@ pub(crate) fn add_transparent_account_balances(
     summary_height: BlockHeight,
     account_balances: &mut HashMap<AccountId, AccountBalance>,
 ) -> Result<(), SqliteClientError> {
+    //let chain_tip_height = chain_tip_height(conn)?.ok_or(SqliteClientError::ChainHeightUnknown)?;
+
     let mut stmt_account_balances = conn.prepare(
         "SELECT u.account_id, SUM(u.value_zat)
          FROM transparent_received_outputs u
          JOIN transactions t
          ON t.id_tx = u.transaction_id
-         WHERE t.mined_height <= :summary_height
+         -- the transaction that created the output is mined or is definitely unexpired
+         WHERE (
+            t.mined_height <= :summary_height -- tx is mined
+            -- TODO: uncomment the following two lines in order to enable zero-conf spends
+            -- OR (
+            --     :summary_height > :chain_tip_height
+            --     AND (
+            --         t.expiry_height = 0 -- tx will not expire
+            --         OR t.expiry_height >= :summary_height
+            --     )
+            -- )
+         )
          -- and the received txo is unspent
          AND u.id NOT IN (
            SELECT transparent_received_output_id
            FROM transparent_received_output_spends txo_spends
            JOIN transactions tx
              ON tx.id_tx = txo_spends.transaction_id
-           WHERE tx.block IS NOT NULL -- the spending tx is mined
+           WHERE tx.mined_height IS NOT NULL -- the spending tx is mined
            OR tx.expiry_height IS NULL -- the spending tx will not expire
            OR tx.expiry_height > :summary_height -- the spending tx is unexpired
          )
          GROUP BY u.account_id",
     )?;
-    let mut rows =
-        stmt_account_balances.query(named_params![":summary_height": u32::from(summary_height)])?;
+    let mut rows = stmt_account_balances.query(named_params![
+        ":summary_height": u32::from(summary_height),
+        //":chain_tip_height": u32::from(chain_tip_height),
+    ])?;
 
     while let Some(row) = rows.next()? {
         let account = AccountId(row.get(0)?);