Make height optional in the utxos table (using a -1 sentinel to avoid a

migration).

Signed-off-by: Daira-Emma Hopwood <[email protected]>

zcash_client_backend/CHANGELOG.md

@@ -32,10 +32,14 @@ and this library adheres to Rust's notion of
     `ShieldedProtocol` to `zcash_protocol::PoolType`.
 - `zcash_client_backend::input_selection::GreedyInputSelectorError` has a
   new variant `UnsupportedTexAddress`.
-- `zcash_client_backend::wallet::Recipient` variants have changed. Instead of
-  wrapping protocol-address types, the `Recipient` type now wraps a
-  `zcash_address::ZcashAddress`. This simplifies the process of tracking the
-  original address to which value was sent.
+- `zcash_client_backend::wallet`:
+  - `Recipient` variants have changed. Instead of wrapping protocol-address
+    types, the `External` and `InternalAccount` variants now wrap a
+    `zcash_address::ZcashAddress`. This simplifies the process of tracking
+    the original address to which value was sent.
+  - `WalletTransparentOutput::{from_parts, height}` now take and return the
+    height as an `Option<BlockHeight>`, accounting for cases where the height
+    is not known.
 - MSRV is now 1.66.0 (except for the `lightwalletd-tonic` feature flag which
   requires 1.70.0).
 

zcash_client_backend/src/wallet.rs

@@ -191,15 +191,15 @@ impl<AccountId> WalletTx<AccountId> {
 pub struct WalletTransparentOutput {
     outpoint: OutPoint,
     txout: TxOut,
-    height: BlockHeight,
+    height: Option<BlockHeight>,
     recipient_address: TransparentAddress,
 }
 
 impl WalletTransparentOutput {
     pub fn from_parts(
         outpoint: OutPoint,
         txout: TxOut,
-        height: BlockHeight,
+        height: Option<BlockHeight>,
     ) -> Option<WalletTransparentOutput> {
         txout
             .recipient_address()
@@ -219,7 +219,7 @@ impl WalletTransparentOutput {
         &self.txout
     }
 
-    pub fn height(&self) -> BlockHeight {
+    pub fn height(&self) -> Option<BlockHeight> {
         self.height
     }
 

zcash_client_sqlite/CHANGELOG.md

@@ -7,6 +7,8 @@ and this library adheres to Rust's notion of
 
 ## [Unreleased]
 ### Changed
+- The result of the `v_tx_outputs` SQL query could now include transparent outputs
+  with unknown height.
 - MSRV is now 1.66.0.
 
 ## [0.10.3] - 2024-04-08

zcash_client_sqlite/src/testing/pool.rs

@@ -1208,7 +1208,7 @@ pub(crate) fn shield_transparent<T: ShieldedPoolTester>() {
             value: NonNegativeAmount::const_from_u64(10000),
             script_pubkey: taddr.script(),
         },
-        h,
+        Some(h),
     )
     .unwrap();
 

zcash_client_sqlite/src/wallet.rs

@@ -1299,6 +1299,7 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
         let zero_conf_height = (chain_tip_height + 1).saturating_sub(min_confirmations);
         let stable_height = chain_tip_height.saturating_sub(PRUNING_DEPTH);
 
+        // FIXME: This can select utxos with unknown height (sentinel -1); is that correct?
         let mut stmt_transparent_balances = tx.prepare(
             "SELECT u.received_by_account_id, SUM(u.value_zat)
              FROM utxos u
@@ -2091,7 +2092,14 @@ fn to_unspent_transparent_output(row: &Row) -> Result<WalletTransparentOutput, S
     let value = NonNegativeAmount::from_nonnegative_i64(raw_value).map_err(|_| {
         SqliteClientError::CorruptedData(format!("Invalid UTXO value: {}", raw_value))
     })?;
-    let height: u32 = row.get("height")?;
+    let height = match row.get("height")? {
+        -1i64 => Ok(None),
+        h @ 0..=0xFFFFFFFFi64 => Ok(Some(BlockHeight::from(h as u32))),
+        raw_value => Err(SqliteClientError::CorruptedData(format!(
+            "Invalid height: {}",
+            raw_value
+        ))),
+    }?;
 
     let outpoint = OutPoint::new(txid_bytes, index);
     WalletTransparentOutput::from_parts(
@@ -2100,7 +2108,7 @@ fn to_unspent_transparent_output(row: &Row) -> Result<WalletTransparentOutput, S
             value,
             script_pubkey,
         },
-        BlockHeight::from(height),
+        height,
     )
     .ok_or_else(|| {
         SqliteClientError::CorruptedData(
@@ -2114,6 +2122,7 @@ pub(crate) fn get_unspent_transparent_output(
     conn: &rusqlite::Connection,
     outpoint: &OutPoint,
 ) -> Result<Option<WalletTransparentOutput>, SqliteClientError> {
+    // FIXME: This can select utxos with unknown height (sentinel -1); is that correct?
     let mut stmt_select_utxo = conn.prepare_cached(
         "SELECT u.prevout_txid, u.prevout_idx, u.script, u.value_zat, u.height
          FROM utxos u
@@ -2158,6 +2167,7 @@ pub(crate) fn get_unspent_transparent_outputs<P: consensus::Parameters>(
         .unwrap_or(max_height)
         .saturating_sub(PRUNING_DEPTH);
 
+    // FIXME: This can select utxos with unknown height (sentinel -1); is that correct?
     let mut stmt_utxos = conn.prepare(
         "SELECT u.prevout_txid, u.prevout_idx, u.script,
                 u.value_zat, u.height
@@ -2211,6 +2221,7 @@ pub(crate) fn get_transparent_balances<P: consensus::Parameters>(
         .unwrap_or(max_height)
         .saturating_sub(PRUNING_DEPTH);
 
+    // FIXME: This can select utxos with unknown height (sentinel -1); is that correct?
     let mut stmt_blocks = conn.prepare(
         "SELECT u.address, SUM(u.value_zat)
          FROM utxos u
@@ -2453,6 +2464,7 @@ pub(crate) fn mark_transparent_utxo_spent(
     tx_ref: i64,
     outpoint: &OutPoint,
 ) -> Result<(), SqliteClientError> {
+    // FIXME: This can select utxos with unknown height (sentinel -1); is that correct?
     let mut stmt_mark_transparent_utxo_spent = conn.prepare_cached(
         "INSERT INTO transparent_received_output_spends (transparent_received_output_id, transaction_id)
          SELECT txo.id, :spent_in_tx
@@ -2527,7 +2539,7 @@ pub(crate) fn put_legacy_transparent_utxo<P: consensus::Parameters>(
     output: &WalletTransparentOutput,
     received_by_account: AccountId,
 ) -> Result<UtxoId, rusqlite::Error> {
-    #[cfg(feature = "transparent-inputs")]
+    // This can insert utxos with unknown height (sentinel -1).
     let mut stmt_upsert_legacy_transparent_utxo = conn.prepare_cached(
         "INSERT INTO utxos (
             prevout_txid, prevout_idx,
@@ -2553,7 +2565,7 @@ pub(crate) fn put_legacy_transparent_utxo<P: consensus::Parameters>(
         ":address": &output.recipient_address().encode(params),
         ":script": &output.txout().script_pubkey.0,
         ":value_zat": &i64::from(Amount::from(output.txout().value)),
-        ":height": &u32::from(output.height()),
+        ":height": &output.height().map_or(-1, i64::from),
     ];
 
     stmt_upsert_legacy_transparent_utxo.query_row(sql_args, |row| row.get::<_, i64>(0).map(UtxoId))
@@ -2928,7 +2940,8 @@ mod tests {
 
         // Pretend the output's transaction was mined at `height_1`.
         let utxo =
-            WalletTransparentOutput::from_parts(outpoint.clone(), txout.clone(), height_1).unwrap();
+            WalletTransparentOutput::from_parts(outpoint.clone(), txout.clone(), Some(height_1))
+                .unwrap();
         let res0 = st.wallet_mut().put_received_transparent_utxo(&utxo);
         assert_matches!(res0, Ok(_));
 
@@ -2939,17 +2952,17 @@ mod tests {
                 height_1,
                 &[]
             ).as_deref(),
-            Ok([ret]) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), height_1)
+            Ok([ret]) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), Some(height_1))
         );
         assert_matches!(
             st.wallet().get_unspent_transparent_output(utxo.outpoint()),
-            Ok(Some(ret)) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), height_1)
+            Ok(Some(ret)) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), Some(height_1))
         );
 
         // Change the mined height of the UTXO and upsert; we should get back
         // the same `UtxoId`.
         let height_2 = BlockHeight::from_u32(34567);
-        let utxo2 = WalletTransparentOutput::from_parts(outpoint, txout, height_2).unwrap();
+        let utxo2 = WalletTransparentOutput::from_parts(outpoint, txout, Some(height_2)).unwrap();
         let res1 = st.wallet_mut().put_received_transparent_utxo(&utxo2);
         assert_matches!(res1, Ok(id) if id == res0.unwrap());
 
@@ -2964,15 +2977,15 @@ mod tests {
         // We can still look up the specific output, and it has the expected height.
         assert_matches!(
             st.wallet().get_unspent_transparent_output(utxo2.outpoint()),
-            Ok(Some(ret)) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo2.outpoint(), utxo2.txout(), height_2)
+            Ok(Some(ret)) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo2.outpoint(), utxo2.txout(), Some(height_2))
         );
 
         // If we include `height_2` then the output is returned.
         assert_matches!(
             st.wallet()
                 .get_unspent_transparent_outputs(taddr, height_2, &[])
                 .as_deref(),
-            Ok([ret]) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), height_2)
+            Ok([ret]) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), Some(height_2))
         );
 
         assert_matches!(
@@ -3107,7 +3120,7 @@ mod tests {
 
         // Pretend the output was received in the chain tip.
         let height = st.wallet().chain_height().unwrap().unwrap();
-        let utxo = WalletTransparentOutput::from_parts(outpoint, txout, height).unwrap();
+        let utxo = WalletTransparentOutput::from_parts(outpoint, txout, Some(height)).unwrap();
         st.wallet_mut()
             .put_received_transparent_utxo(&utxo)
             .unwrap();

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

@@ -90,6 +90,7 @@ impl<P: consensus::Parameters> RusqliteMigration for Migration<P> {
             }
         }
 
+        // Although height is INTEGER NOT NULL, we use -1 as a sentinel for an unknown height.
         transaction.execute_batch(
             "CREATE TABLE utxos_new (
                 id_utxo INTEGER PRIMARY KEY,

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

@@ -30,6 +30,7 @@ impl RusqliteMigration for Migration {
     type Error = WalletMigrationError;
 
     fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), WalletMigrationError> {
+        // Although height is INTEGER NOT NULL, we use -1 as a sentinel for an unknown height.
         transaction.execute_batch(
             "CREATE TABLE IF NOT EXISTS utxos (
                 id_utxo INTEGER PRIMARY KEY,

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

@@ -34,6 +34,8 @@ impl RusqliteMigration for Migration {
     type Error = WalletMigrationError;
 
     fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> {
+        // FIXME: This will *not* include a transaction by virtue of it involving a utxo
+        // that is in the utxos table with unknown height. Is that correct?
         transaction.execute_batch(
             "DROP VIEW v_transactions;
             CREATE VIEW v_transactions AS
@@ -134,6 +136,8 @@ impl RusqliteMigration for Migration {
             GROUP BY notes.account_id, notes.txid;"
         )?;
 
+        // FIXME: This *will* include outputs that are in the utxos table with unknown height.
+        // Is that correct?
         transaction.execute_batch(
             "DROP VIEW v_tx_outputs;
             CREATE VIEW v_tx_outputs AS