From 9e4fa96dd7b927476640a0f2bfe96c136e793fb8 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 21 Sep 2023 22:24:46 +0000 Subject: [PATCH 1/6] zcash_client_sqlite: Clean up existing transparent input test --- zcash_client_sqlite/src/wallet.rs | 90 +++++++++++++++---------------- 1 file changed, 44 insertions(+), 46 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index bcc1d4f9f..c1872ab47 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1918,7 +1918,6 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { - secrecy::Secret, zcash_client_backend::{ data_api::WalletWrite, encoding::AddressCodec, wallet::WalletTransparentOutput, }, @@ -1963,12 +1962,11 @@ mod tests { fn put_received_transparent_utxo() { use crate::testing::TestBuilder; - let mut st = TestBuilder::new().build(); + let mut st = TestBuilder::new() + .with_test_account(AccountBirthday::from_sapling_activation) + .build(); - // Add an account to the wallet - let seed = Secret::new([0u8; 32].to_vec()); - let birthday = AccountBirthday::from_sapling_activation(&st.network()); - let (account_id, _usk) = st.wallet_mut().create_account(&seed, birthday).unwrap(); + let (account_id, _, _) = st.test_account().unwrap(); let uaddr = st .wallet() .get_current_address(account_id) @@ -1976,63 +1974,63 @@ mod tests { .unwrap(); let taddr = uaddr.transparent().unwrap(); + let height_1 = BlockHeight::from_u32(12345); let bal_absent = st .wallet() - .get_transparent_balances(account_id, BlockHeight::from_u32(12345)) + .get_transparent_balances(account_id, height_1) .unwrap(); assert!(bal_absent.is_empty()); - let utxo = WalletTransparentOutput::from_parts( - OutPoint::new([1u8; 32], 1), - TxOut { - value: Amount::from_u64(100000).unwrap(), - script_pubkey: taddr.script(), - }, - BlockHeight::from_u32(12345), - ) - .unwrap(); + // Create a fake transparent output. + let value = Amount::from_u64(100000).unwrap(); + let outpoint = OutPoint::new([1u8; 32], 1); + let txout = TxOut { + value, + script_pubkey: taddr.script(), + }; + // Pretend the output's transaction was mined at `height_1`. + let utxo = + WalletTransparentOutput::from_parts(outpoint.clone(), txout.clone(), height_1).unwrap(); let res0 = st.wallet_mut().put_received_transparent_utxo(&utxo); assert_matches!(res0, Ok(_)); + // Confirm that we see the output unspent as of `height_1`. + assert_matches!( + st.wallet().get_unspent_transparent_outputs( + taddr, + height_1, + &[] + ).as_deref(), + Ok(&[ref ret]) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), height_1) + ); + // Change the mined height of the UTXO and upsert; we should get back - // the same utxoid - let utxo2 = WalletTransparentOutput::from_parts( - OutPoint::new([1u8; 32], 1), - TxOut { - value: Amount::from_u64(100000).unwrap(), - script_pubkey: taddr.script(), - }, - BlockHeight::from_u32(34567), - ) - .unwrap(); + // the same `UtxoId`. + let height_2 = BlockHeight::from_u32(34567); + let utxo2 = WalletTransparentOutput::from_parts(outpoint, txout, height_2).unwrap(); let res1 = st.wallet_mut().put_received_transparent_utxo(&utxo2); assert_matches!(res1, Ok(id) if id == res0.unwrap()); + // Confirm that we no longer see any unspent outputs as of `height_1`. assert_matches!( - st.wallet().get_unspent_transparent_outputs( - taddr, - BlockHeight::from_u32(12345), - &[] - ), - Ok(utxos) if utxos.is_empty() + st.wallet() + .get_unspent_transparent_outputs(taddr, height_1, &[]) + .as_deref(), + Ok(&[]) + ); + + // If we include `height_2` then the output is returned. + assert_matches!( + st.wallet() + .get_unspent_transparent_outputs(taddr, height_2, &[]) + .as_deref(), + Ok(&[ref ret]) if (ret.outpoint(), ret.txout(), ret.height()) == (utxo.outpoint(), utxo.txout(), height_2) ); assert_matches!( - st.wallet().get_unspent_transparent_outputs( - taddr, - BlockHeight::from_u32(34567), - &[] - ), - Ok(utxos) if { - utxos.len() == 1 && - utxos.iter().any(|rutxo| rutxo.height() == utxo2.height()) - } - ); - - assert_matches!( - st.wallet().get_transparent_balances(account_id, BlockHeight::from_u32(34567)), - Ok(h) if h.get(taddr) == Amount::from_u64(100000).ok().as_ref() + st.wallet().get_transparent_balances(account_id, height_2), + Ok(h) if h.get(taddr) == Some(&value) ); // Artificially delete the address from the addresses table so that From 71e38fe190857ddcc742f517b787b7763bb4fd30 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Sep 2023 00:09:00 +0000 Subject: [PATCH 2/6] zcash_client_sqlite: Enable `TestState` to mine wallet transactions --- zcash_client_backend/src/proto.rs | 27 +++++++-- zcash_client_sqlite/src/testing.rs | 70 +++++++++++++++++------ zcash_client_sqlite/src/wallet/sapling.rs | 12 ++-- 3 files changed, 80 insertions(+), 29 deletions(-) diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index ee21a8e9f..a93817404 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -116,10 +116,8 @@ impl compact_formats::CompactSaplingOutput { } } -impl From> - for compact_formats::CompactSaplingOutput -{ - fn from(out: sapling::OutputDescription) -> compact_formats::CompactSaplingOutput { +impl From<&sapling::OutputDescription> for compact_formats::CompactSaplingOutput { + fn from(out: &sapling::OutputDescription) -> compact_formats::CompactSaplingOutput { compact_formats::CompactSaplingOutput { cmu: out.cmu().to_bytes().to_vec(), ephemeral_key: out.ephemeral_key().as_ref().to_vec(), @@ -146,6 +144,27 @@ impl compact_formats::CompactSaplingSpend { } } +impl From<&sapling::SpendDescription> + for compact_formats::CompactSaplingSpend +{ + fn from(spend: &sapling::SpendDescription) -> compact_formats::CompactSaplingSpend { + compact_formats::CompactSaplingSpend { + nf: spend.nullifier().to_vec(), + } + } +} + +impl From<&orchard::Action> for compact_formats::CompactOrchardAction { + fn from(action: &orchard::Action) -> compact_formats::CompactOrchardAction { + compact_formats::CompactOrchardAction { + nullifier: action.nullifier().to_bytes().to_vec(), + cmx: action.cmx().to_bytes().to_vec(), + ephemeral_key: action.encrypted_note().epk_bytes.to_vec(), + ciphertext: action.encrypted_note().enc_ciphertext[..COMPACT_NOTE_SIZE].to_vec(), + } + } +} + impl service::TreeState { /// Deserializes and returns the Sapling note commitment tree field of the tree state. pub fn sapling_tree(&self) -> io::Result> { diff --git a/zcash_client_sqlite/src/testing.rs b/zcash_client_sqlite/src/testing.rs index a9c5cffe0..5bc160ba0 100644 --- a/zcash_client_sqlite/src/testing.rs +++ b/zcash_client_sqlite/src/testing.rs @@ -14,7 +14,7 @@ use tempfile::NamedTempFile; #[cfg(feature = "unstable")] use tempfile::TempDir; -use zcash_client_backend::data_api::AccountBalance; +use zcash_client_backend::data_api::{AccountBalance, WalletRead}; #[allow(deprecated)] use zcash_client_backend::{ address::RecipientAddress, @@ -35,7 +35,7 @@ use zcash_client_backend::{ wallet::OvkPolicy, zip321, }; -use zcash_note_encryption::{Domain, COMPACT_NOTE_SIZE}; +use zcash_note_encryption::Domain; use zcash_primitives::{ block::BlockHash, consensus::{self, BlockHeight, Network, NetworkUpgrade, Parameters}, @@ -270,14 +270,34 @@ where (height, res) } + /// Creates a fake block at the expected next height containing only the wallet + /// transaction with the given txid, and inserts it into the cache. + /// + /// This generated block will be treated as the latest block, and subsequent calls to + /// [`Self::generate_next_block`] (or similar) will build on it. + pub(crate) fn generate_next_block_including( + &mut self, + txid: TxId, + ) -> (BlockHeight, Cache::InsertResult) { + let tx = self + .wallet() + .get_transaction(txid) + .expect("TxId should exist in the wallet"); + + // Index 0 is by definition a coinbase transaction, and the wallet doesn't + // construct coinbase transactions. So we pretend here that the block has a + // coinbase transaction that does not have shielded coinbase outputs. + self.generate_next_block_from_tx(1, &tx) + } + /// Creates a fake block at the expected next height containing only the given /// transaction, and inserts it into the cache. - /// This assumes that the transaction only has Sapling spends and outputs. /// /// This generated block will be treated as the latest block, and subsequent calls to /// [`Self::generate_next_block`] will build on it. pub(crate) fn generate_next_block_from_tx( &mut self, + tx_index: usize, tx: &Transaction, ) -> (BlockHeight, Cache::InsertResult) { let (height, prev_hash, initial_sapling_tree_size) = self @@ -285,7 +305,14 @@ where .map(|(prev_height, prev_hash, end_size)| (prev_height + 1, prev_hash, end_size)) .unwrap_or_else(|| (self.sapling_activation_height(), BlockHash([0; 32]), 0)); - let cb = fake_compact_block_from_tx(height, prev_hash, tx, initial_sapling_tree_size); + let cb = fake_compact_block_from_tx( + height, + prev_hash, + tx_index, + tx, + initial_sapling_tree_size, + 0, + ); let res = self.cache.insert(&cb); self.latest_cached_block = Some(( @@ -728,35 +755,42 @@ pub(crate) fn fake_compact_block( } /// Create a fake CompactBlock at the given height containing only the given transaction. -/// This assumes that the transaction only has Sapling spends and outputs. pub(crate) fn fake_compact_block_from_tx( height: BlockHeight, prev_hash: BlockHash, + tx_index: usize, tx: &Transaction, initial_sapling_tree_size: u32, + initial_orchard_tree_size: u32, ) -> CompactBlock { - // Create a fake CompactTx + // Create a fake CompactTx containing the transaction. let mut ctx = CompactTx { + index: tx_index as u64, hash: tx.txid().as_ref().to_vec(), ..Default::default() }; if let Some(bundle) = tx.sapling_bundle() { for spend in bundle.shielded_spends() { - ctx.spends.push(CompactSaplingSpend { - nf: spend.nullifier().to_vec(), - }); + ctx.spends.push(spend.into()); } for output in bundle.shielded_outputs() { - ctx.outputs.push(CompactSaplingOutput { - cmu: output.cmu().to_bytes().to_vec(), - ephemeral_key: output.ephemeral_key().0.to_vec(), - ciphertext: output.enc_ciphertext()[..COMPACT_NOTE_SIZE].to_vec(), - }); + ctx.outputs.push(output.into()); + } + } + if let Some(bundle) = tx.orchard_bundle() { + for action in bundle.actions() { + ctx.actions.push(action.into()); } } - fake_compact_block_from_compact_tx(ctx, height, prev_hash, initial_sapling_tree_size) + fake_compact_block_from_compact_tx( + ctx, + height, + prev_hash, + initial_sapling_tree_size, + initial_orchard_tree_size, + ) } /// Create a fake CompactBlock at the given height, spending a single note from the @@ -833,7 +867,7 @@ pub(crate) fn fake_compact_block_spending( } }); - fake_compact_block_from_compact_tx(ctx, height, prev_hash, initial_sapling_tree_size) + fake_compact_block_from_compact_tx(ctx, height, prev_hash, initial_sapling_tree_size, 0) } pub(crate) fn fake_compact_block_from_compact_tx( @@ -841,6 +875,7 @@ pub(crate) fn fake_compact_block_from_compact_tx( height: BlockHeight, prev_hash: BlockHash, initial_sapling_tree_size: u32, + initial_orchard_tree_size: u32, ) -> CompactBlock { let mut rng = OsRng; let mut cb = CompactBlock { @@ -857,7 +892,8 @@ pub(crate) fn fake_compact_block_from_compact_tx( cb.chain_metadata = Some(compact::ChainMetadata { sapling_commitment_tree_size: initial_sapling_tree_size + cb.vtx.iter().map(|tx| tx.outputs.len() as u32).sum::(), - ..Default::default() + orchard_commitment_tree_size: initial_orchard_tree_size + + cb.vtx.iter().map(|tx| tx.actions.len() as u32).sum::(), }); cb } diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index e5ccb7d65..6bf84cf00 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -821,9 +821,8 @@ pub(crate) mod tests { NonZeroU32::new(10).unwrap(), ) .unwrap(); - let tx = &st.wallet().get_transaction(txid).unwrap(); - let (h, _) = st.generate_next_block_from_tx(tx); + let (h, _) = st.generate_next_block_including(txid); st.scan_cached_blocks(h, 1); // TODO: send to an account so that we can check its balance. @@ -938,9 +937,8 @@ pub(crate) mod tests { NonZeroU32::new(1).unwrap(), ) .unwrap(); - let tx2 = &st.wallet().get_transaction(txid2).unwrap(); - let (h, _) = st.generate_next_block_from_tx(tx2); + let (h, _) = st.generate_next_block_including(txid2); st.scan_cached_blocks(h, 1); // TODO: send to an account so that we can check its balance. @@ -1200,7 +1198,6 @@ pub(crate) mod tests { NonZeroU32::new(1).unwrap(), ) .unwrap(); - let tx = &st.wallet().get_transaction(txid).unwrap(); let amount_left = (value - (amount_sent + fee_rule.fixed_fee().try_into().unwrap()).unwrap()).unwrap(); @@ -1211,7 +1208,7 @@ pub(crate) mod tests { // We spent the only note so we only have pending change. assert_eq!(st.get_total_balance(AccountId::from(0)), pending_change); - let (h, _) = st.generate_next_block_from_tx(tx); + let (h, _) = st.generate_next_block_including(txid); st.scan_cached_blocks(h, 1); assert_eq!(st.get_total_balance(AccountId::from(1)), amount_sent); @@ -1330,9 +1327,8 @@ pub(crate) mod tests { NonZeroU32::new(1).unwrap(), ) .unwrap(); - let tx = &st.wallet().get_transaction(txid).unwrap(); - let (h, _) = st.generate_next_block_from_tx(tx); + let (h, _) = st.generate_next_block_including(txid); st.scan_cached_blocks(h, 1); // TODO: send to an account so that we can check its balance. From cd6c9627195e98c40f001f15c3b312c67554d8f6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Sep 2023 01:00:34 +0000 Subject: [PATCH 3/6] zcash_client_sqlite: Write a test for transparent balance behaviour --- zcash_client_sqlite/src/wallet.rs | 146 +++++++++++++++++++++++++++++- 1 file changed, 144 insertions(+), 2 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index c1872ab47..d33eeb2ec 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1918,12 +1918,21 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { + crate::testing::{AddressType, TestState}, zcash_client_backend::{ - data_api::WalletWrite, encoding::AddressCodec, wallet::WalletTransparentOutput, + data_api::{wallet::input_selection::GreedyInputSelector, WalletWrite}, + encoding::AddressCodec, + fees::{fixed, DustOutputPolicy}, + wallet::WalletTransparentOutput, }, zcash_primitives::{ consensus::BlockHeight, - transaction::components::{Amount, OutPoint, TxOut}, + memo::MemoBytes, + transaction::{ + components::{amount::NonNegativeAmount, Amount, OutPoint, TxOut}, + fees::fixed::FeeRule as FixedFeeRule, + }, + zip32::ExtendedSpendingKey, }, }; @@ -2046,4 +2055,137 @@ mod tests { let res2 = st.wallet_mut().put_received_transparent_utxo(&utxo2); assert_matches!(res2, Err(_)); } + + #[test] + #[cfg(feature = "transparent-inputs")] + fn transparent_balance_across_shielding() { + let mut st = TestBuilder::new() + .with_block_cache() + .with_test_account(AccountBirthday::from_sapling_activation) + .build(); + + let (account_id, usk, _) = st.test_account().unwrap(); + let uaddr = st + .wallet() + .get_current_address(account_id) + .unwrap() + .unwrap(); + let taddr = uaddr.transparent().unwrap(); + + // Initialize the wallet with chain data that has no shielded notes for us. + let not_our_key = ExtendedSpendingKey::master(&[]).to_diversifiable_full_viewing_key(); + let not_our_value = Amount::const_from_i64(10000); + let (start_height, _, _) = + st.generate_next_block(¬_our_key, AddressType::DefaultExternal, not_our_value); + for _ in 1..10 { + st.generate_next_block(¬_our_key, AddressType::DefaultExternal, not_our_value); + } + st.scan_cached_blocks(start_height, 10); + + let check_balance = |st: &TestState<_>, min_confirmations: u32, expected| { + // Check the wallet summary returns the expected transparent balance. + let summary = st + .wallet() + .get_wallet_summary(min_confirmations) + .unwrap() + .unwrap(); + let balance = summary.account_balances().get(&account_id).unwrap(); + assert_eq!(balance.unshielded, expected); + + // Check the older APIs for consistency. + let max_height = st.wallet().chain_height().unwrap().unwrap() + 1 - min_confirmations; + assert_eq!( + st.wallet() + .get_transparent_balances(account_id, max_height) + .unwrap() + .get(taddr) + .cloned() + .unwrap_or(Amount::zero()), + Amount::from(expected), + ); + assert_eq!( + st.wallet() + .get_unspent_transparent_outputs(taddr, max_height, &[]) + .unwrap() + .into_iter() + .map(|utxo| utxo.value()) + .sum::>(), + Some(Amount::from(expected)), + ); + }; + + // The wallet starts out with zero balance. + check_balance(&st, 0, NonNegativeAmount::ZERO); + check_balance(&st, 1, NonNegativeAmount::ZERO); + + // Create a fake transparent output. + let value = NonNegativeAmount::from_u64(100000).unwrap(); + let outpoint = OutPoint::new([1u8; 32], 1); + let txout = TxOut { + value: value.into(), + script_pubkey: taddr.script(), + }; + + // 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(); + st.wallet_mut() + .put_received_transparent_utxo(&utxo) + .unwrap(); + + // The wallet should detect the balance as having 1 confirmation. + check_balance(&st, 0, value); + check_balance(&st, 1, value); + check_balance(&st, 2, NonNegativeAmount::ZERO); + + // Shield the output. + let input_selector = GreedyInputSelector::new( + fixed::SingleOutputChangeStrategy::new(FixedFeeRule::non_standard(Amount::zero())), + DustOutputPolicy::default(), + ); + let txid = st + .shield_transparent_funds( + &input_selector, + value, + &usk, + &[*taddr], + &MemoBytes::empty(), + NonZeroU32::new(1).unwrap(), + ) + .unwrap(); + + // The wallet should have zero transparent balance, because the shielding + // transaction can be mined. + check_balance(&st, 0, NonNegativeAmount::ZERO); + check_balance(&st, 1, NonNegativeAmount::ZERO); + check_balance(&st, 2, NonNegativeAmount::ZERO); + + // Mine the shielding transaction. + let (mined_height, _) = st.generate_next_block_including(txid); + st.scan_cached_blocks(mined_height, 1); + + // The wallet should still have zero transparent balance. + check_balance(&st, 0, NonNegativeAmount::ZERO); + check_balance(&st, 1, NonNegativeAmount::ZERO); + check_balance(&st, 2, NonNegativeAmount::ZERO); + + // Unmine the shielding transaction via a reorg. + st.wallet_mut() + .truncate_to_height(mined_height - 1) + .unwrap(); + + // The wallet should still have zero transparent balance. + check_balance(&st, 0, NonNegativeAmount::ZERO); + check_balance(&st, 1, NonNegativeAmount::ZERO); + check_balance(&st, 2, NonNegativeAmount::ZERO); + + // Expire the shielding transaction. + let expiry_height = st.wallet().get_transaction(txid).unwrap().expiry_height(); + st.wallet_mut().update_chain_tip(expiry_height).unwrap(); + + // The transparent output should be spendable again, with more confirmations. + check_balance(&st, 0, value); + check_balance(&st, 1, value); + check_balance(&st, 2, value); + } } From 625a5ff59450d4cd48e6b5e121e927e36e962b90 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Sep 2023 01:19:49 +0000 Subject: [PATCH 4/6] zcash_client_sqlite: Remove is-mined checks from transparent balance The `LEFT OUTER JOIN` was causing the `tx.block IS NULL` check to alias two cases: an unspent transparent output, and a transparent output spent in an unmined transaction. The latter only makes sense to include in the UTXO count if the transaction is expired, and (due to limitations of the transparent data model in the current wallet) if that expiry won't be undone by a reorg. We now handle these two cases directly. Partly reverts 8828276361e02155f49da31d2cf9515231f4d26b. Closes zcash/librustzcash#983. Co-authored-by: Kris Nuttycombe --- zcash_client_sqlite/src/wallet.rs | 50 +++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index d33eeb2ec..69fa9e6e3 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -713,17 +713,21 @@ pub(crate) fn get_wallet_summary( #[cfg(feature = "transparent-inputs")] { let zero_conf_height = (chain_tip_height + 1).saturating_sub(min_confirmations); + let stable_height = chain_tip_height.saturating_sub(PRUNING_DEPTH); + let mut stmt_transparent_balances = conn.prepare( "SELECT u.received_by_account, SUM(u.value_zat) FROM utxos u LEFT OUTER JOIN transactions tx ON tx.id_tx = u.spent_in_tx WHERE u.height <= :max_height - AND tx.block IS NULL + AND (u.spent_in_tx IS NULL OR (tx.block IS NULL AND tx.expiry_height <= :stable_height)) GROUP BY u.received_by_account", )?; - let mut rows = stmt_transparent_balances - .query(named_params![":max_height": u32::from(zero_conf_height)])?; + let mut rows = stmt_transparent_balances.query(named_params![ + ":max_height": u32::from(zero_conf_height), + ":stable_height": u32::from(stable_height) + ])?; while let Some(row) = rows.next()? { let account = AccountId::from(row.get::<_, u32>(0)?); @@ -1302,15 +1306,20 @@ pub(crate) fn get_unspent_transparent_outputs( max_height: BlockHeight, exclude: &[OutPoint], ) -> Result, SqliteClientError> { + let chain_tip_height = scan_queue_extrema(conn)?.map(|(_, max)| max); + let stable_height = chain_tip_height + .unwrap_or(max_height) + .saturating_sub(PRUNING_DEPTH); + let mut stmt_blocks = conn.prepare( "SELECT u.prevout_txid, u.prevout_idx, u.script, - u.value_zat, u.height, tx.block as block + u.value_zat, u.height FROM utxos u LEFT OUTER JOIN transactions tx ON tx.id_tx = u.spent_in_tx WHERE u.address = :address AND u.height <= :max_height - AND tx.block IS NULL", + AND (u.spent_in_tx IS NULL OR (tx.block IS NULL AND tx.expiry_height <= :stable_height))", )?; let addr_str = address.encode(params); @@ -1318,7 +1327,8 @@ pub(crate) fn get_unspent_transparent_outputs( let mut utxos = Vec::::new(); let mut rows = stmt_blocks.query(named_params![ ":address": addr_str, - ":max_height": u32::from(max_height) + ":max_height": u32::from(max_height), + ":stable_height": u32::from(stable_height), ])?; let excluded: BTreeSet = exclude.iter().cloned().collect(); while let Some(row) = rows.next()? { @@ -1367,6 +1377,11 @@ pub(crate) fn get_transparent_balances( account: AccountId, max_height: BlockHeight, ) -> Result, SqliteClientError> { + let chain_tip_height = scan_queue_extrema(conn)?.map(|(_, max)| max); + let stable_height = chain_tip_height + .unwrap_or(max_height) + .saturating_sub(PRUNING_DEPTH); + let mut stmt_blocks = conn.prepare( "SELECT u.address, SUM(u.value_zat) FROM utxos u @@ -1374,14 +1389,15 @@ pub(crate) fn get_transparent_balances( ON tx.id_tx = u.spent_in_tx WHERE u.received_by_account = :account_id AND u.height <= :max_height - AND tx.block IS NULL + AND (u.spent_in_tx IS NULL OR (tx.block IS NULL AND tx.expiry_height <= :stable_height)) GROUP BY u.address", )?; let mut res = HashMap::new(); let mut rows = stmt_blocks.query(named_params![ ":account_id": u32::from(account), - ":max_height": u32::from(max_height) + ":max_height": u32::from(max_height), + ":stable_height": u32::from(stable_height), ])?; while let Some(row) = rows.next()? { let taddr_str: String = row.get(0)?; @@ -1918,7 +1934,10 @@ mod tests { #[cfg(feature = "transparent-inputs")] use { - crate::testing::{AddressType, TestState}, + crate::{ + testing::{AddressType, TestState}, + PRUNING_DEPTH, + }, zcash_client_backend::{ data_api::{wallet::input_selection::GreedyInputSelector, WalletWrite}, encoding::AddressCodec, @@ -2183,6 +2202,19 @@ mod tests { let expiry_height = st.wallet().get_transaction(txid).unwrap().expiry_height(); st.wallet_mut().update_chain_tip(expiry_height).unwrap(); + // TODO: Making the transparent output spendable in this situation requires + // changes to the transparent data model, so for now the wallet should still have + // zero transparent balance. https://github.com/zcash/librustzcash/issues/986 + check_balance(&st, 0, NonNegativeAmount::ZERO); + check_balance(&st, 1, NonNegativeAmount::ZERO); + check_balance(&st, 2, NonNegativeAmount::ZERO); + + // Roll forward the chain tip until the transaction's expiry height is in the + // stable block range (so a reorg won't make it spendable again). + st.wallet_mut() + .update_chain_tip(expiry_height + PRUNING_DEPTH) + .unwrap(); + // The transparent output should be spendable again, with more confirmations. check_balance(&st, 0, value); check_balance(&st, 1, value); From 513abf8b9758ca7062a6d80c45c55ff7829a8eae Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Sep 2023 01:24:03 +0000 Subject: [PATCH 5/6] rustfmt --- zcash_client_backend/src/data_api.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 18646c44d..875ac614b 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -490,7 +490,7 @@ impl ScannedBlock { } /// Returns the metadata describing the state of the note commitment trees as of the end of the - /// scanned block. + /// scanned block. /// /// The metadata returned from this method is guaranteed to be consistent with what is returned /// by [`Self::height`] and [`Self::block_hash`]. From b76b028b3a84d58318a9f6433f0d3ff95468ac6b Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Sep 2023 15:04:51 +0000 Subject: [PATCH 6/6] zcash_client_sqlite: Set chain tip to truncation height when truncating We don't know at truncation time what the latest chain tip is; the chain might have reorged to a shorter heavier chain, or the reorg depth might only be a few blocks. `WalletDb::chain_height` uses the scan queue as its source of truth, so the `Verify` range we add during truncation (to prioritise determining whether the rewind was sufficient) can't extend beyond the block height we know to exist. The next call to `WalletDb::update_chain_tip` will add additional ranges beyond this height, which might include a `Verify` range that ends up merging with the one added during truncation. --- zcash_client_sqlite/src/wallet.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 69fa9e6e3..70f52d97b 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -105,8 +105,8 @@ use zcash_client_backend::{ use crate::wallet::commitment_tree::SqliteShardStore; use crate::{ error::SqliteClientError, SqlTransaction, WalletCommitmentTrees, WalletDb, PRUNING_DEPTH, + SAPLING_TABLES_PREFIX, }; -use crate::{SAPLING_TABLES_PREFIX, VERIFY_LOOKAHEAD}; use self::scanning::{parse_priority_code, replace_queue_entries}; @@ -1281,8 +1281,8 @@ pub(crate) fn truncate_to_height( named_params![":end_height": u32::from(block_height + 1)], )?; - // Prioritize the range starting at the height we just rewound to for verification - let query_range = block_height..(block_height + VERIFY_LOOKAHEAD); + // Prioritize the height we just rewound to for verification. + let query_range = block_height..(block_height + 1); let scan_range = ScanRange::from_parts(query_range.clone(), ScanPriority::Verify); replace_queue_entries::( conn, @@ -2192,6 +2192,7 @@ mod tests { st.wallet_mut() .truncate_to_height(mined_height - 1) .unwrap(); + assert_eq!(st.wallet().chain_height().unwrap(), Some(mined_height - 1)); // The wallet should still have zero transparent balance. check_balance(&st, 0, NonNegativeAmount::ZERO);