From 5ad3205e585d2be244b6a2139d26dc447d0518cb Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 26 Jul 2024 13:19:54 -0600 Subject: [PATCH 1/2] zcash_client_sqlite: Demonstrate the failure of store_decrypted_tx to persist transparent outputs. Related to #1434 --- zcash_client_backend/CHANGELOG.md | 15 +++-- zcash_client_backend/src/wallet.rs | 10 +-- zcash_client_sqlite/src/lib.rs | 2 +- zcash_client_sqlite/src/testing/pool.rs | 17 ++++- zcash_client_sqlite/src/wallet/transparent.rs | 67 +++++++++++-------- 5 files changed, 70 insertions(+), 41 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 607207685..ee29ab2e4 100644 --- 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` rather than + `BlockHeight`. ### Removed - `zcash_client_backend::data_api`: @@ -101,12 +106,14 @@ 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 `WalletTransparentOutput::mined_height` instead). ## [0.12.1] - 2024-03-27 ### Fixed - This release fixes a problem in note selection when sending to a transparent - recipient, whereby available funds were being incorrectly excluded from + recipient, whereby available funds were being incorrectly excluded from input selection. ## [0.12.0] - 2024-03-25 @@ -176,11 +183,11 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `get_transaction` now returns `Result, _>` rather than returning an `Err` if the `txid` parameter does not correspond to a transaction in the database. - - `WalletWrite::create_account` now takes its `AccountBirthday` argument by + - `WalletWrite::create_account` now takes its `AccountBirthday` argument by reference. - Changes to the `InputSource` trait: - `select_spendable_notes` now takes its `target_value` argument as a - `NonNegativeAmount`. Also, it now returns a `SpendableNotes` data + `NonNegativeAmount`. Also, it now returns a `SpendableNotes` data structure instead of a vector. - Fields of `DecryptedTransaction` are now private. Use `DecryptedTransaction::new` and the newly provided accessors instead. diff --git a/zcash_client_backend/src/wallet.rs b/zcash_client_backend/src/wallet.rs index 1bc4dae3f..f3bca04ba 100644 --- a/zcash_client_backend/src/wallet.rs +++ b/zcash_client_backend/src/wallet.rs @@ -249,7 +249,7 @@ impl WalletTx { pub struct WalletTransparentOutput { outpoint: OutPoint, txout: TxOut, - height: BlockHeight, + mined_height: Option, recipient_address: TransparentAddress, } @@ -257,14 +257,14 @@ impl WalletTransparentOutput { pub fn from_parts( outpoint: OutPoint, txout: TxOut, - height: BlockHeight, + mined_height: Option, ) -> Option { 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 { + 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 index afb3d170b..69f8ebac8 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -290,7 +290,7 @@ impl, P: consensus::Parameters> InputSource for &self, outpoint: &OutPoint, ) -> Result, 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 index 76d172749..fbd06e6a6 100644 --- 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() { }; 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() { }, ); 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() { &zip317::FeeRule::standard(), ) .unwrap(); + let txid = build_result.transaction().txid(); let decrypted_tx = DecryptedTransaction::::new( build_result.transaction(), vec![], @@ -585,6 +589,13 @@ pub(crate) fn send_multi_step_proposed_transfer() { ); 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() { 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 index 000c515e2..0db0fca04 100644 --- 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 = 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 Result Result, 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, 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( 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,7 +742,7 @@ 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. @@ -740,7 +750,7 @@ mod tests { 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(); From 3cec9ee4a7de366d20586ad8ee4da101055824f2 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 26 Jul 2024 08:47:01 -0600 Subject: [PATCH 2/2] zcash_client_sqlite: Store received UTXOs in `store_decrypted_tx`. This fixes an issue wherein transparent outputs of transactions added to the wallet via `decrypt_and_store_transaction` would not be properly recorded as UTXOs belonging to the wallet. Part of #1434 --- zcash_client_backend/CHANGELOG.md | 4 +++- zcash_client_backend/src/data_api.rs | 7 +++++++ zcash_client_backend/src/decrypt.rs | 1 + zcash_client_sqlite/src/lib.rs | 21 +++++++++++++++++++++ zcash_client_sqlite/src/testing/pool.rs | 1 + zcash_primitives/src/legacy.rs | 17 +++++++++++++++++ 6 files changed, 50 insertions(+), 1 deletion(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index ee29ab2e4..bc7f3a4c5 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -26,7 +26,8 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail ### Added - `zcash_client_backend::data_api`: - `chain::BlockCache` trait, behind the `sync` feature flag. - - `WalletRead::get_spendable_transparent_outputs`. + - `WalletRead::get_spendable_transparent_outputs` + - `DecryptedTransaction::mined_height` - `zcash_client_backend::fees`: - `EphemeralBalance` - `ChangeValue::shielded, is_ephemeral` @@ -68,6 +69,7 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail - `error::Error` has new `Address` and (when the "transparent-inputs" feature is enabled) `PaysEphemeralTransparentAddress` variants. - `wallet::input_selection::InputSelectorError` has a new `Address` variant. + - `DecryptedTransaction::new` takes an additional `mined_height` argument. - `zcash_client_backend::data_api::fees` - When the "transparent-inputs" feature is enabled, `ChangeValue` can also represent an ephemeral transparent output in a proposal. Accordingly, the diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 9c8e5a932..cd80d169b 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1263,6 +1263,7 @@ impl ScannedBlock { /// The purpose of this struct is to permit atomic updates of the /// wallet database when transactions are successfully decrypted. pub struct DecryptedTransaction<'a, AccountId> { + mined_height: Option, tx: &'a Transaction, sapling_outputs: Vec>, #[cfg(feature = "orchard")] @@ -1272,6 +1273,7 @@ pub struct DecryptedTransaction<'a, AccountId> { impl<'a, AccountId> DecryptedTransaction<'a, AccountId> { /// Constructs a new [`DecryptedTransaction`] from its constituent parts. pub fn new( + mined_height: Option, tx: &'a Transaction, sapling_outputs: Vec>, #[cfg(feature = "orchard")] orchard_outputs: Vec< @@ -1279,6 +1281,7 @@ impl<'a, AccountId> DecryptedTransaction<'a, AccountId> { >, ) -> Self { Self { + mined_height, tx, sapling_outputs, #[cfg(feature = "orchard")] @@ -1286,6 +1289,10 @@ impl<'a, AccountId> DecryptedTransaction<'a, AccountId> { } } + /// Returns the height at which the transaction was mined, if known. + pub fn mined_height(&self) -> Option { + self.mined_height + } /// Returns the raw transaction data. pub fn tx(&self) -> &Transaction { self.tx diff --git a/zcash_client_backend/src/decrypt.rs b/zcash_client_backend/src/decrypt.rs index 51f9abcdb..b48d077b5 100644 --- a/zcash_client_backend/src/decrypt.rs +++ b/zcash_client_backend/src/decrypt.rs @@ -215,6 +215,7 @@ pub fn decrypt_transaction<'a, P: consensus::Parameters, AccountId: Copy>( .collect(); DecryptedTransaction::new( + Some(height), tx, sapling_outputs, #[cfg(feature = "orchard")] diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 69f8ebac8..7496eb7c3 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -124,6 +124,9 @@ use wallet::{ SubtreeScanProgress, }; +#[cfg(feature = "transparent-inputs")] +use wallet::transparent::{find_account_for_transparent_address, put_transparent_output}; + #[cfg(test)] mod testing; @@ -1320,6 +1323,24 @@ impl WalletWrite for WalletDb #[cfg(feature = "transparent-inputs")] wallet::transparent::ephemeral::mark_ephemeral_address_as_seen(wdb, &address, tx_ref)?; + // If the output belongs to the wallet, add it to `transparent_received_outputs`. + #[cfg(feature = "transparent-inputs")] + if let Some(account_id) = find_account_for_transparent_address( + wdb.conn.0, + &wdb.params, + &address + )? { + put_transparent_output( + wdb.conn.0, + &wdb.params, + &OutPoint::new(d_tx.tx().txid().into(), u32::try_from(output_index).unwrap()), + txout, + d_tx.mined_height(), + &address, + account_id + )?; + } + // If a transaction we observe contains spends from our wallet, we will // store its transparent outputs in the same way they would be stored by // create_spend_to_address. diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index fbd06e6a6..ae6f37197 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -582,6 +582,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { .unwrap(); let txid = build_result.transaction().txid(); let decrypted_tx = DecryptedTransaction::::new( + None, build_result.transaction(), vec![], #[cfg(feature = "orchard")] diff --git a/zcash_primitives/src/legacy.rs b/zcash_primitives/src/legacy.rs index defe8e376..23ba7cf83 100644 --- a/zcash_primitives/src/legacy.rs +++ b/zcash_primitives/src/legacy.rs @@ -1,6 +1,7 @@ //! Support for legacy transparent addresses and scripts. use byteorder::{ReadBytesExt, WriteBytesExt}; +use zcash_address::TryFromRawAddress; use std::fmt; use std::io::{self, Read, Write}; @@ -407,6 +408,22 @@ impl TransparentAddress { } } +impl TryFromRawAddress for TransparentAddress { + type Error = (); + + fn try_from_raw_transparent_p2pkh( + data: [u8; 20], + ) -> Result> { + Ok(TransparentAddress::PublicKeyHash(data)) + } + + fn try_from_raw_transparent_p2sh( + data: [u8; 20], + ) -> Result> { + Ok(TransparentAddress::ScriptHash(data)) + } +} + #[cfg(any(test, feature = "test-dependencies"))] pub mod testing { use proptest::prelude::{any, prop_compose};