zcash_client_sqlite: Demonstrate the failure of store_decrypted_tx to…

… persist transparent outputs. Related to zcash#1434
nuttycom · Jul 26, 2024 · 46efb80 · 46efb80
1 parent 001f278
commit 46efb80
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 38 deletions.
diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md
@@ -39,7 +39,9 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
   - `testing` module
 - `zcash_client_backend::sync` module, behind the `sync` feature flag.
 - `zcash_client_backend::tor` module, behind the `tor` feature flag.
-- `zcash_client_backend::wallet::Recipient::map_ephemeral_transparent_outpoint`
+- `zcash_client_backend::wallet`:
+    - `Recipient::map_ephemeral_transparent_outpoint`
+    - `WalletTransparentOutput::mined_height`
 
 ### Changed
 - MSRV is now 1.70.0.
@@ -93,6 +95,9 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
   tracking the original address to which value was sent. There is also a new
   `EphemeralTransparent` variant, and an additional generic parameter for the
   type of metadata associated with an ephemeral transparent outpoint.
+- `zcash_client_backend::wallet::WalletTransparentOutput::from_parts`
+  now takes its height argument as `Option<BlockHeight>` rather than
+  `BlockHeight`.
 
 ### Removed
 - `zcash_client_backend::data_api`:
@@ -101,6 +106,8 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
     `WalletRead::get_spendable_transparent_outputs` instead.
 - `zcash_client_backend::fees::ChangeValue::new`. Use `ChangeValue::shielded`
   or `ChangeValue::ephemeral_transparent` instead.
+- `zcash_client_backend::wallet::WalletTransparentOutput::height` use the
+  `mined_height` method instead.
 
 ## [0.12.1] - 2024-03-27
 

diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs
@@ -249,22 +249,22 @@ impl<AccountId> WalletTx<AccountId> {
 pub struct WalletTransparentOutput {
     outpoint: OutPoint,
     txout: TxOut,
-    height: BlockHeight,
+    mined_height: Option<BlockHeight>,
     recipient_address: TransparentAddress,
 }
 
 impl WalletTransparentOutput {
     pub fn from_parts(
         outpoint: OutPoint,
         txout: TxOut,
-        height: BlockHeight,
+        mined_height: Option<BlockHeight>,
     ) -> Option<WalletTransparentOutput> {
         txout
             .recipient_address()
             .map(|recipient_address| WalletTransparentOutput {
                 outpoint,
                 txout,
-                height,
+                mined_height,
                 recipient_address,
             })
     }
@@ -277,8 +277,8 @@ impl WalletTransparentOutput {
         &self.txout
     }
 
-    pub fn height(&self) -> BlockHeight {
-        self.height
+    pub fn mined_height(&self) -> Option<BlockHeight> {
+        self.mined_height
     }
 
     pub fn recipient_address(&self) -> &TransparentAddress {

diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs
@@ -290,7 +290,7 @@ impl<C: Borrow<rusqlite::Connection>, P: consensus::Parameters> InputSource for
         &self,
         outpoint: &OutPoint,
     ) -> Result<Option<WalletTransparentOutput>, Self::Error> {
-        wallet::transparent::get_unspent_transparent_output(self.conn.borrow(), outpoint)
+        wallet::transparent::get_wallet_transparent_output(self.conn.borrow(), outpoint, false)
     }
 
     #[cfg(feature = "transparent-inputs")]

diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs
@@ -315,7 +315,9 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
     };
     use zcash_protocol::value::ZatBalance;
 
-    use crate::wallet::{sapling::tests::test_prover, GAP_LIMIT};
+    use crate::wallet::{
+        sapling::tests::test_prover, transparent::get_wallet_transparent_output, GAP_LIMIT,
+    };
 
     let mut st = TestBuilder::new()
         .with_block_cache()
@@ -553,8 +555,9 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
         },
     );
     let (colliding_addr, _) = &known_addrs[10];
+    let utxo_value = (value - zip317::MINIMUM_FEE).unwrap();
     assert_matches!(
-        builder.add_transparent_output(colliding_addr, (value - zip317::MINIMUM_FEE).unwrap()),
+        builder.add_transparent_output(colliding_addr, utxo_value),
         Ok(_)
     );
     let sk = account
@@ -577,6 +580,7 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
             &zip317::FeeRule::standard(),
         )
         .unwrap();
+    let txid = build_result.transaction().txid();
     let decrypted_tx = DecryptedTransaction::<AccountId>::new(
         build_result.transaction(),
         vec![],
@@ -585,6 +589,13 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
     );
     st.wallet_mut().store_decrypted_tx(decrypted_tx).unwrap();
 
+    // We call get_wallet_transparent_output with `allow_unspendable = true` to verify
+    // storage because the decrypted transaction has not yet been mined.
+    let utxo =
+        get_wallet_transparent_output(&st.db_data.conn, &OutPoint::new(txid.into(), 0), true)
+            .unwrap();
+    assert_matches!(utxo, Some(v) if v.value() == utxo_value);
+
     // That should have advanced the start of the gap to index 11.
     let new_known_addrs = st
         .wallet()
@@ -1532,7 +1543,7 @@ pub(crate) fn shield_transparent<T: ShieldedPoolTester>() {
             value: NonNegativeAmount::const_from_u64(10000),
             script_pubkey: taddr.script(),
         },
-        h,
+        Some(h),
     )
     .unwrap();
 

diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs
@@ -169,7 +169,7 @@ 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("received_height")?;
+    let height: Option<u32> = row.get("received_height")?;
 
     let outpoint = OutPoint::new(txid_bytes, index);
     WalletTransparentOutput::from_parts(
@@ -178,7 +178,7 @@ fn to_unspent_transparent_output(row: &Row) -> Result<WalletTransparentOutput, S
             value,
             script_pubkey,
         },
-        BlockHeight::from(height),
+        height.map(BlockHeight::from),
     )
     .ok_or_else(|| {
         SqliteClientError::CorruptedData(
@@ -188,16 +188,18 @@ 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(
+pub(crate) fn get_wallet_transparent_output(
     conn: &rusqlite::Connection,
     outpoint: &OutPoint,
+    allow_unspendable: bool,
 ) -> 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.
+    // This could 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 or spent in a transaction that has not been observed by this wallet. There isn't a
+    // way to detect the circumstance related to anchor invalidation 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
@@ -207,20 +209,26 @@ pub(crate) fn get_unspent_transparent_output(
          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
+             :allow_unspendable
+             OR (
+                 (
+                    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.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
+                 )
+             )
          )
-         -- 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.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
@@ -229,6 +237,7 @@ pub(crate) fn get_unspent_transparent_output(
                 ":txid": outpoint.hash(),
                 ":output_index": outpoint.n(),
                 ":mempool_height": chain_tip_height.map(|h| u32::from(h) + 1),
+                ":allow_unspendable": allow_unspendable
             ],
             to_unspent_transparent_output,
         )?
@@ -456,7 +465,7 @@ pub(crate) fn put_received_transparent_utxo<P: consensus::Parameters>(
             params,
             output.outpoint(),
             output.txout(),
-            Some(output.height()),
+            output.mined_height(),
             address,
             receiving_account,
         )
@@ -695,7 +704,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(_));
 
@@ -706,18 +716,18 @@ mod tests {
                 height_1,
                 0
             ).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.mined_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.mined_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 = birthday + 34567;
         st.wallet_mut().update_chain_tip(height_2).unwrap();
-        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());
 
@@ -732,15 +742,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.mined_height()) == (utxo2.outpoint(), utxo2.txout(), Some(height_2))
         );
 
         // If we include `height_2` then the output is returned.
         assert_matches!(
             st.wallet()
                 .get_spendable_transparent_outputs(taddr, height_2, 0)
                 .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.mined_height()) == (utxo.outpoint(), utxo.txout(), Some(height_2))
         );
 
         assert_matches!(
@@ -842,7 +852,8 @@ 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::fake(), txout, height).unwrap();
+        let utxo =
+            WalletTransparentOutput::from_parts(OutPoint::fake(), txout, Some(height)).unwrap();
         st.wallet_mut()
             .put_received_transparent_utxo(&utxo)
             .unwrap();