zcash_client_sqlite: Align handling of transparent UTXOs with that of…

… shielded notes. Co-authored-by: Daira-Emma Hopwood <[email protected]> Co-authored-by: Jack Grigg <[email protected]>
zcash · Jun 22, 2024 · 4ba438c · 4ba438c
1 parent 0e1f2ff
commit 4ba438c
Show file tree

Hide file tree

Showing 12 changed files with 956 additions and 432 deletions.
diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md
@@ -10,13 +10,14 @@ and this library adheres to Rust's notion of
 ### Added
 - `zcash_client_backend::data_api`:
   - `chain::BlockCache` trait, behind the `sync` feature flag.
+  - `WalletRead::get_spendable_transparent_outputs`.
 - `zcash_client_backend::fees`:
   - `ChangeValue::{transparent, shielded}`
   - `sapling::EmptyBundleView`
   - `orchard::EmptyBundleView`
 - `zcash_client_backend::scanning`:
   - `testing` module
-- `zcash_client_backend::sync` module, behind the `sync` feature flag.
+- `zcash_client_backend::sync` module, behind the `sync` feature flag
 
 ### Changed
 - MSRV is now 1.70.0.
@@ -52,6 +53,12 @@ and this library adheres to Rust's notion of
   `zcash_address::ZcashAddress`. This simplifies the process of tracking the
   original address to which value was sent.
 
+### Removed
+- `zcash_client_backend::data_api`:
+  - `WalletRead::get_unspent_transparent_outputs` has been removed because its
+    semantics were unclear and could not be clarified. Use
+    `WalletRead::get_spendable_transparent_outputs` instead.
+
 ## [0.12.1] - 2024-03-27
 
 ### Fixed

diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs
@@ -665,10 +665,10 @@ pub trait InputSource {
         exclude: &[Self::NoteRef],
     ) -> Result<SpendableNotes<Self::NoteRef>, Self::Error>;
 
-    /// Fetches a spendable transparent output.
+    /// Fetches the transparent output corresponding to the provided `outpoint`.
     ///
     /// Returns `Ok(None)` if the UTXO is not known to belong to the wallet or is not
-    /// spendable.
+    /// spendable as of the chain tip height.
     #[cfg(feature = "transparent-inputs")]
     fn get_unspent_transparent_output(
         &self,
@@ -677,14 +677,18 @@ pub trait InputSource {
         Ok(None)
     }
 
-    /// Returns a list of unspent transparent UTXOs that appear in the chain at heights up to and
-    /// including `max_height`.
+    /// Returns the list of transparent outputs received at `address` such that:
+    /// * The transaction that produced these outputs is mined or mineable as of `max_height`.
+    /// * Each returned output is unspent as of the current chain tip.
+    ///
+    /// The caller should filter these outputs to ensure they respect the desired number of
+    /// confirmations before attempting to spend them.
     #[cfg(feature = "transparent-inputs")]
-    fn get_unspent_transparent_outputs(
+    fn get_spendable_transparent_outputs(
         &self,
         _address: &TransparentAddress,
-        _max_height: BlockHeight,
-        _exclude: &[OutPoint],
+        _target_height: BlockHeight,
+        _min_confirmations: u32,
     ) -> Result<Vec<WalletTransparentOutput>, Self::Error> {
         Ok(vec![])
     }

diff --git a/zcash_client_backend/src/data_api/wallet/input_selection.rs b/zcash_client_backend/src/data_api/wallet/input_selection.rs
@@ -580,11 +580,7 @@ where
         let mut transparent_inputs: Vec<WalletTransparentOutput> = source_addrs
             .iter()
             .map(|taddr| {
-                wallet_db.get_unspent_transparent_outputs(
-                    taddr,
-                    target_height - min_confirmations,
-                    &[],
-                )
+                wallet_db.get_spendable_transparent_outputs(taddr, target_height, min_confirmations)
             })
             .collect::<Result<Vec<Vec<_>>, _>>()
             .map_err(InputSelectorError::DataSource)?

diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs
@@ -276,18 +276,18 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
     }
 
     #[cfg(feature = "transparent-inputs")]
-    fn get_unspent_transparent_outputs(
+    fn get_spendable_transparent_outputs(
         &self,
         address: &TransparentAddress,
-        max_height: BlockHeight,
-        exclude: &[OutPoint],
+        target_height: BlockHeight,
+        min_confirmations: u32,
     ) -> Result<Vec<WalletTransparentOutput>, Self::Error> {
-        wallet::transparent::get_unspent_transparent_outputs(
+        wallet::transparent::get_spendable_transparent_outputs(
             self.conn.borrow(),
             &self.params,
             address,
-            max_height,
-            exclude,
+            target_height,
+            min_confirmations,
         )
     }
 }
@@ -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> {

diff --git a/zcash_client_sqlite/src/wallet.rs b/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);
         }
@@ -1019,7 +1019,7 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
         ) -> Result<(), SqliteClientError>,
     {
         // 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.
+        // including that height, none of our shielded balance is currently spendable.
         #[tracing::instrument(skip_all)]
         fn is_any_spendable(
             conn: &rusqlite::Connection,
@@ -1167,12 +1167,7 @@ pub(crate) fn get_wallet_summary<P: consensus::Parameters>(
     drop(sapling_trace);
 
     #[cfg(feature = "transparent-inputs")]
-    transparent::add_transparent_account_balances(
-        tx,
-        chain_tip_height,
-        min_confirmations,
-        &mut account_balances,
-    )?;
+    transparent::add_transparent_account_balances(tx, chain_tip_height + 1, &mut account_balances)?;
 
     // The approach used here for Sapling and Orchard subtree indexing was a quick hack
     // that has not yet been replaced. TODO: Make less hacky.
@@ -1503,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,
@@ -1861,9 +1849,29 @@ pub(crate) fn truncate_to_height<P: consensus::Parameters>(
         named_params![":new_end_height": u32::from(block_height + 1)],
     )?;
 
-    // If we're removing scanned blocks, we need to truncate the note commitment tree, un-mine
-    // transactions, and remove received transparent outputs and affected block records from the
-    // database.
+    // Mark transparent utxos as un-mined. Since the TXO is now not mined, it would ideally be
+    // considered to have been returned to the mempool; it _might_ be spendable in this state, but
+    // we must also set its max_observed_unspent_height field to NULL because the transaction may
+    // be rendered entirely invalid by a reorg that alters anchor(s) used in constructing shielded
+    // spends in the transaction.
+    conn.execute(
+        "UPDATE transparent_received_outputs
+         SET max_observed_unspent_height = NULL
+         WHERE max_observed_unspent_height > :height",
+        named_params![":height": u32::from(block_height)],
+    )?;
+
+    // Un-mine transactions. This must be done outside of the last_scanned_height check because
+    // transaction entries may be created as a consequence of receiving transparent TXOs.
+    conn.execute(
+        "UPDATE transactions
+         SET block = NULL, mined_height = NULL, tx_index = NULL
+         WHERE block > ?",
+        [u32::from(block_height)],
+    )?;
+
+    // If we're removing scanned blocks, we need to truncate the note commitment tree and remove
+    // affected block records from the database.
     if block_height < last_scanned_height {
         // Truncate the note commitment trees
         let mut wdb = WalletDb {
@@ -1885,20 +1893,6 @@ pub(crate) fn truncate_to_height<P: consensus::Parameters>(
         // not recoverable; balance APIs must ensure that un-mined received notes
         // do not count towards spendability or transaction balalnce.
 
-        // Rewind utxos. It is currently necessary to delete these because we do
-        // not have the full transaction data for the received output.
-        conn.execute(
-            "DELETE FROM utxos WHERE height > ?",
-            [u32::from(block_height)],
-        )?;
-
-        // Un-mine transactions.
-        conn.execute(
-            "UPDATE transactions SET block = NULL, tx_index = NULL
-            WHERE block IS NOT NULL AND block > ?",
-            [u32::from(block_height)],
-        )?;
-
         // Now that they aren't depended on, delete un-mined blocks.
         conn.execute(
             "DELETE FROM blocks WHERE height > ?",
@@ -2010,6 +2004,25 @@ pub(crate) fn put_block(
         ":orchard_action_count": orchard_action_count,
     ])?;
 
+    // If we now have a block corresponding to a received transparent output that had not been
+    // scanned at the time the UTXO was discovered, update the associated transaction record to
+    // refer to that block.
+    //
+    // NOTE: There's a small data corruption hazard here, in that we're relying exclusively upon
+    // the block height to associate the transaction to the block. This is because CompactBlock
+    // values only contain CompactTx entries for transactions that contain shielded inputs or
+    // outputs, and the GetAddressUtxosReply data does not contain the block hash. As such, it's
+    // necessary to ensure that any chain rollback to below the received height causes that height
+    // to be set to NULL.
+    let mut stmt_update_transaction_block_reference = conn.prepare_cached(
+        "UPDATE transactions
+         SET block = :height
+         WHERE mined_height = :height",
+    )?;
+
+    stmt_update_transaction_block_reference
+        .execute(named_params![":height": u32::from(block_height),])?;
+
     Ok(())
 }
 
@@ -2022,10 +2035,11 @@ pub(crate) fn put_tx_meta(
 ) -> Result<i64, SqliteClientError> {
     // It isn't there, so insert our transaction into the database.
     let mut stmt_upsert_tx_meta = conn.prepare_cached(
-        "INSERT INTO transactions (txid, block, tx_index)
-        VALUES (:txid, :block, :tx_index)
+        "INSERT INTO transactions (txid, block, mined_height, tx_index)
+        VALUES (:txid, :block, :block, :tx_index)
         ON CONFLICT (txid) DO UPDATE
         SET block = :block,
+            mined_height = :block,
             tx_index = :tx_index
         RETURNING id_tx",
     )?;