From c7a7e6190739d7188f43d0c6b64fb27760c181f5 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 8 Feb 2024 23:24:17 +0100 Subject: [PATCH 1/8] Refactor obtaining of activation heights --- zebra-chain/src/parameters/network.rs | 13 ++++++++++++ zebra-scan/src/service.rs | 2 +- zebra-scan/src/service/scan_task/commands.rs | 4 +++- zebra-scan/src/service/scan_task/scan.rs | 4 ++-- .../src/service/scan_task/scan/scan_range.rs | 2 +- zebra-scan/src/storage.rs | 20 +------------------ zebra-scan/src/storage/db/sapling.rs | 2 +- 7 files changed, 22 insertions(+), 25 deletions(-) diff --git a/zebra-chain/src/parameters/network.rs b/zebra-chain/src/parameters/network.rs index c90ce860798..753e8230dbf 100644 --- a/zebra-chain/src/parameters/network.rs +++ b/zebra-chain/src/parameters/network.rs @@ -129,6 +129,19 @@ impl Network { pub fn is_a_test_network(&self) -> bool { *self != Network::Mainnet } + + /// Returns the minimum Sapling birthday height. + pub fn sapling_activation_height(self) -> Height { + // Assume that the genesis block never contains shielded inputs or outputs. + // + // # Consensus + // + // For Zcash mainnet and the public testnet, Sapling activates above genesis, + // so this is always true. + super::NetworkUpgrade::Sapling + .activation_height(self) + .expect("Sapling activation height needs to be set") + } } impl FromStr for Network { diff --git a/zebra-scan/src/service.rs b/zebra-scan/src/service.rs index 3dee20f787d..4f86c108929 100644 --- a/zebra-scan/src/service.rs +++ b/zebra-scan/src/service.rs @@ -86,7 +86,7 @@ impl Service for ScanService { return async move { Ok(Response::Info { - min_sapling_birthday_height: db.min_sapling_birthday_height(), + min_sapling_birthday_height: db.network().sapling_activation_height(), }) } .boxed(); diff --git a/zebra-scan/src/service/scan_task/commands.rs b/zebra-scan/src/service/scan_task/commands.rs index 58d9bd88d7e..6ce10015a5e 100644 --- a/zebra-scan/src/service/scan_task/commands.rs +++ b/zebra-scan/src/service/scan_task/commands.rs @@ -12,7 +12,7 @@ use color_eyre::{eyre::eyre, Report}; use tokio::sync::oneshot; use zcash_primitives::{sapling::SaplingIvk, zip32::DiversifiableFullViewingKey}; -use zebra_chain::{block::Height, transaction::Transaction}; +use zebra_chain::{block::Height, parameters::Network, transaction::Transaction}; use zebra_state::SaplingScanningKey; use super::ScanTask; @@ -61,11 +61,13 @@ impl ScanTask { SaplingScanningKey, (Vec, Vec), >, + network: Network, ) -> Result< HashMap, Vec, Height)>, Report, > { let mut new_keys = HashMap::new(); + let sapling_activation_height = network.sapling_activation_height(); loop { let cmd = match cmd_receiver.try_recv() { diff --git a/zebra-scan/src/service/scan_task/scan.rs b/zebra-scan/src/service/scan_task/scan.rs index 9af1e3ef97d..9654735960e 100644 --- a/zebra-scan/src/service/scan_task/scan.rs +++ b/zebra-scan/src/service/scan_task/scan.rs @@ -76,7 +76,7 @@ pub async fn start( cmd_receiver: Receiver, ) -> Result<(), Report> { let network = storage.network(); - let sapling_activation_height = storage.min_sapling_birthday_height(); + let sapling_activation_height = network.sapling_activation_height(); // Do not scan and notify if we are below sapling activation height. wait_for_height( @@ -126,7 +126,7 @@ pub async fn start( } } - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; // TODO: Check if the `start_height` is at or above the current height if !new_keys.is_empty() { diff --git a/zebra-scan/src/service/scan_task/scan/scan_range.rs b/zebra-scan/src/service/scan_task/scan/scan_range.rs index 4e16d4ccdf1..8d5f6c025e9 100644 --- a/zebra-scan/src/service/scan_task/scan/scan_range.rs +++ b/zebra-scan/src/service/scan_task/scan/scan_range.rs @@ -71,7 +71,7 @@ pub async fn scan_range( state: State, storage: Storage, ) -> Result<(), BoxError> { - let sapling_activation_height = storage.min_sapling_birthday_height(); + let sapling_activation_height = storage.network().sapling_activation_height(); // Do not scan and notify if we are below sapling activation height. wait_for_height( sapling_activation_height, diff --git a/zebra-scan/src/storage.rs b/zebra-scan/src/storage.rs index 61a51a29d3e..2372957014e 100644 --- a/zebra-scan/src/storage.rs +++ b/zebra-scan/src/storage.rs @@ -2,10 +2,7 @@ use std::collections::{BTreeMap, HashMap}; -use zebra_chain::{ - block::Height, - parameters::{Network, NetworkUpgrade}, -}; +use zebra_chain::{block::Height, parameters::Network}; use zebra_state::TransactionIndex; use crate::config::Config; @@ -126,19 +123,4 @@ impl Storage { ) -> BTreeMap> { self.sapling_results_for_key(sapling_key) } - - // Parameters - - /// Returns the minimum sapling birthday height for the configured network. - pub fn min_sapling_birthday_height(&self) -> Height { - // Assume that the genesis block never contains shielded inputs or outputs. - // - // # Consensus - // - // For Zcash mainnet and the public testnet, Sapling activates above genesis, - // so this is always true. - NetworkUpgrade::Sapling - .activation_height(self.network()) - .unwrap_or(Height(0)) - } } diff --git a/zebra-scan/src/storage/db/sapling.rs b/zebra-scan/src/storage/db/sapling.rs index 8ad0fcce72b..6892fc30af9 100644 --- a/zebra-scan/src/storage/db/sapling.rs +++ b/zebra-scan/src/storage/db/sapling.rs @@ -213,7 +213,7 @@ impl Storage { sapling_key: &SaplingScanningKey, birthday_height: Option, ) { - let min_birthday_height = self.min_sapling_birthday_height(); + let min_birthday_height = self.network().sapling_activation_height(); // The birthday height must be at least the minimum height for that pool. let birthday_height = birthday_height From c4a7246959b20a5ad1c7bc4675eb58cbe70142e0 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 8 Feb 2024 23:36:32 +0100 Subject: [PATCH 2/8] Impl the `RegisterKeys` service request --- .../src/scan_service/request.rs | 4 +- .../src/scan_service/response.rs | 15 +++-- zebra-scan/src/service.rs | 13 ++-- zebra-scan/src/service/scan_task/commands.rs | 65 ++++++++++++------- 4 files changed, 64 insertions(+), 33 deletions(-) diff --git a/zebra-node-services/src/scan_service/request.rs b/zebra-node-services/src/scan_service/request.rs index 6069baf25be..802876ac7a0 100644 --- a/zebra-node-services/src/scan_service/request.rs +++ b/zebra-node-services/src/scan_service/request.rs @@ -14,8 +14,8 @@ pub enum Request { /// TODO: Accept `KeyHash`es and return key hashes that are registered CheckKeyHashes(Vec<()>), - /// TODO: Accept `ViewingKeyWithHash`es and return Ok(()) if successful or an error - RegisterKeys(Vec<()>), + /// Submits viewing keys with their optional birth-heights for scanning. + RegisterKeys(Vec<(String, Option)>), /// Deletes viewing keys and their results from the database. DeleteKeys(Vec), diff --git a/zebra-node-services/src/scan_service/response.rs b/zebra-node-services/src/scan_service/response.rs index 3a04de94218..081200dcdbf 100644 --- a/zebra-node-services/src/scan_service/response.rs +++ b/zebra-node-services/src/scan_service/response.rs @@ -10,23 +10,28 @@ use zebra_chain::{block::Height, transaction::Hash}; #[derive(Debug)] /// Response types for `zebra_scan::service::ScanService` pub enum Response { - /// Response to the `Info` request + /// Response to the [`Info`](super::request::Request::Info) request Info { /// The minimum sapling birthday height for the shielded scanner min_sapling_birthday_height: Height, }, - /// Response to Results request + /// Response to [`RegisterKeys`](super::request::Request::RegisterKeys) request + /// + /// Contains the keys that the scanner accepted. + RegisteredKeys(Vec), + + /// Response to [`Results`](super::request::Request::Results) request /// /// We use the nested `BTreeMap` so we don't repeat any piece of response data. Results(BTreeMap>>), - /// Response to DeleteKeys request + /// Response to [`DeleteKeys`](super::request::Request::DeleteKeys) request DeletedKeys, - /// Response to ClearResults request + /// Response to [`ClearResults`](super::request::Request::ClearResults) request ClearedResults, - /// Response to SubscribeResults request + /// Response to `SubscribeResults` request SubscribeResults(mpsc::Receiver>), } diff --git a/zebra-scan/src/service.rs b/zebra-scan/src/service.rs index 4f86c108929..4d00b757728 100644 --- a/zebra-scan/src/service.rs +++ b/zebra-scan/src/service.rs @@ -96,10 +96,15 @@ impl Service for ScanService { // TODO: check that these entries exist in db } - Request::RegisterKeys(_viewing_key_with_hashes) => { - // TODO: - // - add these keys as entries in db - // - send new keys to scan task + Request::RegisterKeys(keys) => { + let mut scan_task = self.scan_task.clone(); + + return async move { + Ok(Response::RegisteredKeys( + scan_task.register_keys(keys)?.await?, + )) + } + .boxed(); } Request::DeleteKeys(keys) => { diff --git a/zebra-scan/src/service/scan_task/commands.rs b/zebra-scan/src/service/scan_task/commands.rs index 6ce10015a5e..b37aaa57981 100644 --- a/zebra-scan/src/service/scan_task/commands.rs +++ b/zebra-scan/src/service/scan_task/commands.rs @@ -15,6 +15,8 @@ use zcash_primitives::{sapling::SaplingIvk, zip32::DiversifiableFullViewingKey}; use zebra_chain::{block::Height, parameters::Network, transaction::Transaction}; use zebra_state::SaplingScanningKey; +use crate::scan::sapling_key_to_scan_block_keys; + use super::ScanTask; #[derive(Debug)] @@ -23,10 +25,9 @@ pub enum ScanTaskCommand { /// Start scanning for new viewing keys RegisterKeys { /// New keys to start scanning for - keys: HashMap< - SaplingScanningKey, - (Vec, Vec, Height), - >, + keys: Vec<(String, Option)>, + /// Returns the set of keys the scanner accepted. + rsp_tx: oneshot::Sender>, }, /// Stop scanning for deleted viewing keys @@ -81,25 +82,44 @@ impl ScanTask { }; match cmd { - ScanTaskCommand::RegisterKeys { keys } => { + ScanTaskCommand::RegisterKeys { keys, rsp_tx } => { + // Determine what keys we pass to the scanner. let keys: Vec<_> = keys .into_iter() - .filter(|(key, _)| { - !parsed_keys.contains_key(key) || new_keys.contains_key(key) + .filter_map(|key| { + // Don't accept keys that: + // 1. the scanner already has, and + // 2. were already submitted. + if parsed_keys.contains_key(&key.0) && !new_keys.contains_key(&key.0) { + return None; + } + + let birth_height = if let Some(height) = key.1 { + match Height::try_from(height) { + Ok(height) => height, + // Don't accept the key if its birth height is not a valid height. + Err(_) => return None, + } + } else { + // Use the Sapling activation height if the key has no birth height. + sapling_activation_height + }; + + sapling_key_to_scan_block_keys(&key.0, network) + .ok() + .map(|parsed| (key.0, (parsed.0, parsed.1, birth_height))) }) .collect(); - if !keys.is_empty() { - new_keys.extend(keys.clone()); + // Send the accepted keys back. + let _ = rsp_tx.send(keys.iter().map(|key| key.0.clone()).collect()); - let keys = - keys.into_iter() - .map(|(key, (decoded_dfvks, decoded_ivks, _h))| { - (key, (decoded_dfvks, decoded_ivks)) - }); + new_keys.extend(keys.clone()); - parsed_keys.extend(keys); - } + parsed_keys.extend( + keys.into_iter() + .map(|(key, (dfvks, ivks, _))| (key, (dfvks, ivks))), + ); } ScanTaskCommand::RemoveKeys { done_tx, keys } => { @@ -145,11 +165,12 @@ impl ScanTask { /// Sends a message to the scan task to start scanning for the provided viewing keys. pub fn register_keys( &mut self, - keys: HashMap< - SaplingScanningKey, - (Vec, Vec, Height), - >, - ) -> Result<(), mpsc::SendError> { - self.send(ScanTaskCommand::RegisterKeys { keys }) + keys: Vec<(String, Option)>, + ) -> Result>, mpsc::SendError> { + let (rsp_tx, rsp_rx) = oneshot::channel(); + + self.send(ScanTaskCommand::RegisterKeys { keys, rsp_tx })?; + + Ok(rsp_rx) } } From 8ed93dda7fad09942b6cf42fd951d1f47e0a4b29 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 8 Feb 2024 23:38:22 +0100 Subject: [PATCH 3/8] Mock viewing keys for tests --- zebra-scan/src/tests.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/zebra-scan/src/tests.rs b/zebra-scan/src/tests.rs index 55eef898019..fa2dcaf0e4e 100644 --- a/zebra-scan/src/tests.rs +++ b/zebra-scan/src/tests.rs @@ -51,6 +51,28 @@ pub const ZECPAGES_SAPLING_VIEWING_KEY: &str = "zxviews1q0duytgcqqqqpqre26wkl45g /// A fake viewing key in an incorrect format. pub const FAKE_SAPLING_VIEWING_KEY: &str = "zxviewsfake"; +/// Generates fake Sapling viewing keys. +pub fn mock_sapling_viewing_keys(i: u8) -> Vec { + let mut keys: Vec = vec![]; + + for i in 0..i { + keys.push(encode_extended_full_viewing_key( + "zxviews", + &mock_sapling_efvk(&[i]), + )); + } + + keys +} + +/// Generates an [`zip32::sapling::ExtendedFullViewingKey`] from `seed` for tests. +#[allow(deprecated)] +pub fn mock_sapling_efvk(seed: &[u8]) -> zip32::sapling::ExtendedFullViewingKey { + // TODO: Use `to_diversifiable_full_viewing_key` since `to_extended_full_viewing_key` is + // deprecated. + zip32::sapling::ExtendedSpendingKey::master(seed).to_extended_full_viewing_key() +} + /// Generates a fake block containing a Sapling output decryptable by `dfvk`. /// /// The fake block has the following transactions in this order: From 9eb1cab8213f992cc3cc59987ca743fd9a0f5eb7 Mon Sep 17 00:00:00 2001 From: Marek Date: Thu, 8 Feb 2024 23:39:36 +0100 Subject: [PATCH 4/8] Refactor tests --- .../src/service/scan_task/tests/vectors.rs | 89 +++++++------------ zebra-scan/src/tests.rs | 13 +-- zebra-scan/src/tests/vectors.rs | 21 +++-- .../common/shielded_scan/scans_for_new_key.rs | 7 +- 4 files changed, 54 insertions(+), 76 deletions(-) diff --git a/zebra-scan/src/service/scan_task/tests/vectors.rs b/zebra-scan/src/service/scan_task/tests/vectors.rs index 2d9d46063e2..21920423724 100644 --- a/zebra-scan/src/service/scan_task/tests/vectors.rs +++ b/zebra-scan/src/service/scan_task/tests/vectors.rs @@ -4,25 +4,23 @@ use std::collections::HashMap; use color_eyre::Report; -use zebra_chain::block::Height; - -use crate::service::ScanTask; +use crate::{service::ScanTask, tests::mock_sapling_viewing_keys}; /// Test that [`ScanTask::process_messages`] adds and removes keys as expected for `RegisterKeys` and `DeleteKeys` command #[tokio::test] async fn scan_task_processes_messages_correctly() -> Result<(), Report> { let (mut mock_scan_task, cmd_receiver) = ScanTask::mock(); let mut parsed_keys = HashMap::new(); + let network = Default::default(); // Send some keys to be registered let num_keys = 10; - mock_scan_task.register_keys( - (0..num_keys) - .map(|i| (i.to_string(), (vec![], vec![], Height::MIN))) - .collect(), - )?; + let sapling_keys = mock_sapling_viewing_keys(num_keys as u8); + let sapling_keys_with_birth_heights: Vec<(String, Option)> = + sapling_keys.into_iter().zip((0..).map(Some)).collect(); + mock_scan_task.register_keys(sapling_keys_with_birth_heights.clone())?; - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; // Check that it updated parsed_keys correctly and returned the right new keys when starting with an empty state @@ -38,15 +36,11 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { "should add all received keys to parsed keys" ); - mock_scan_task.register_keys( - (0..num_keys) - .map(|i| (i.to_string(), (vec![], vec![], Height::MIN))) - .collect(), - )?; + mock_scan_task.register_keys(sapling_keys_with_birth_heights.clone())?; // Check that no key should be added if they are all already known and the heights are the same - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; assert_eq!( parsed_keys.len(), @@ -59,21 +53,19 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { "should not return known keys as new keys" ); - // Check that it returns the last seen start height for a key as the new key when receiving 2 register key messages + // Check that keys can't be overridden. - mock_scan_task.register_keys( - (10..20) - .map(|i| (i.to_string(), (vec![], vec![], Height::MIN))) - .collect(), - )?; + let sapling_keys = mock_sapling_viewing_keys(20); + let sapling_keys_with_birth_heights: Vec<(String, Option)> = sapling_keys + .clone() + .into_iter() + .map(|key| (key, Some(0))) + .collect(); - mock_scan_task.register_keys( - (10..15) - .map(|i| (i.to_string(), (vec![], vec![], Height::MAX))) - .collect(), - )?; + mock_scan_task.register_keys(sapling_keys_with_birth_heights[10..20].to_vec())?; + mock_scan_task.register_keys(sapling_keys_with_birth_heights[10..15].to_vec())?; - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; assert_eq!( parsed_keys.len(), @@ -87,22 +79,13 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { "should add 10 of received keys to new keys" ); - for (new_key, (_, _, start_height)) in new_keys { - if (10..15).contains(&new_key.parse::().expect("should parse into int")) { - assert_eq!( - start_height, - Height::MAX, - "these key heights should have been overwritten by the second message" - ); - } - } - // Check that it removes keys correctly - let done_rx = - mock_scan_task.remove_keys(&(0..200).map(|i| i.to_string()).collect::>())?; + let sapling_keys = mock_sapling_viewing_keys(30); + + let done_rx = mock_scan_task.remove_keys(&sapling_keys)?; - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; // Check that it sends the done notification successfully before returning and dropping `done_tx` done_rx.await?; @@ -116,15 +99,11 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { // Check that it doesn't return removed keys as new keys when processing a batch of messages - mock_scan_task.register_keys( - (0..200) - .map(|i| (i.to_string(), (vec![], vec![], Height::MAX))) - .collect(), - )?; + mock_scan_task.register_keys(sapling_keys_with_birth_heights.clone())?; - mock_scan_task.remove_keys(&(0..200).map(|i| i.to_string()).collect::>())?; + mock_scan_task.remove_keys(&sapling_keys)?; - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; assert!( new_keys.is_empty(), @@ -133,21 +112,13 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { // Check that it does return registered keys if they were removed in a prior message when processing a batch of messages - mock_scan_task.register_keys( - (0..200) - .map(|i| (i.to_string(), (vec![], vec![], Height::MAX))) - .collect(), - )?; + mock_scan_task.register_keys(sapling_keys_with_birth_heights.clone())?; - mock_scan_task.remove_keys(&(0..200).map(|i| i.to_string()).collect::>())?; + mock_scan_task.remove_keys(&sapling_keys)?; - mock_scan_task.register_keys( - (0..2) - .map(|i| (i.to_string(), (vec![], vec![], Height::MAX))) - .collect(), - )?; + mock_scan_task.register_keys(sapling_keys_with_birth_heights[..2].to_vec())?; - let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys)?; + let new_keys = ScanTask::process_messages(&cmd_receiver, &mut parsed_keys, network)?; assert_eq!( new_keys.len(), diff --git a/zebra-scan/src/tests.rs b/zebra-scan/src/tests.rs index fa2dcaf0e4e..3aa5fd51080 100644 --- a/zebra-scan/src/tests.rs +++ b/zebra-scan/src/tests.rs @@ -12,8 +12,11 @@ use ff::{Field, PrimeField}; use group::GroupEncoding; use rand::{rngs::OsRng, thread_rng, RngCore}; -use zcash_client_backend::proto::compact_formats::{ - ChainMetadata, CompactBlock, CompactSaplingOutput, CompactSaplingSpend, CompactTx, +use zcash_client_backend::{ + encoding::encode_extended_full_viewing_key, + proto::compact_formats::{ + ChainMetadata, CompactBlock, CompactSaplingOutput, CompactSaplingSpend, CompactTx, + }, }; use zcash_note_encryption::Domain; use zcash_primitives::{ @@ -27,7 +30,7 @@ use zcash_primitives::{ value::NoteValue, Note, Nullifier, }, - zip32::DiversifiableFullViewingKey, + zip32, }; use zebra_chain::{ @@ -83,7 +86,7 @@ pub fn mock_sapling_efvk(seed: &[u8]) -> zip32::sapling::ExtendedFullViewingKey pub fn fake_block( height: BlockHeight, nf: Nullifier, - dfvk: &DiversifiableFullViewingKey, + dfvk: &zip32::sapling::DiversifiableFullViewingKey, value: u64, tx_after: bool, initial_sapling_tree_size: Option, @@ -157,7 +160,7 @@ pub fn fake_compact_block( height: BlockHeight, prev_hash: BlockHash, nf: Nullifier, - dfvk: &DiversifiableFullViewingKey, + dfvk: &zip32::sapling::DiversifiableFullViewingKey, value: u64, tx_after: bool, initial_sapling_tree_size: Option, diff --git a/zebra-scan/src/tests/vectors.rs b/zebra-scan/src/tests/vectors.rs index 1b0dfd6456c..3d772873071 100644 --- a/zebra-scan/src/tests/vectors.rs +++ b/zebra-scan/src/tests/vectors.rs @@ -5,7 +5,8 @@ use std::sync::Arc; use color_eyre::Result; use zcash_client_backend::{ - encoding::decode_extended_full_viewing_key, proto::compact_formats::ChainMetadata, + encoding::{decode_extended_full_viewing_key, encode_extended_full_viewing_key}, + proto::compact_formats::ChainMetadata, }; use zcash_primitives::{ constants::mainnet::HRP_SAPLING_EXTENDED_FULL_VIEWING_KEY, @@ -24,7 +25,7 @@ use zebra_state::{SaplingScannedResult, TransactionIndex}; use crate::{ scan::{block_to_compact, scan_block}, storage::db::tests::new_test_storage, - tests::{fake_block, ZECPAGES_SAPLING_VIEWING_KEY}, + tests::{fake_block, mock_sapling_efvk, ZECPAGES_SAPLING_VIEWING_KEY}, }; /// This test: @@ -146,18 +147,16 @@ async fn scanning_zecpages_from_populated_zebra_state() -> Result<()> { /// /// The purpose of this test is to check if our database and our scanning code are compatible. #[test] -#[allow(deprecated)] fn scanning_fake_blocks_store_key_and_results() -> Result<()> { + let network = Network::Mainnet; + // Generate a key - let extsk = ExtendedSpendingKey::master(&[]); - // TODO: find out how to do it with `to_diversifiable_full_viewing_key` as `to_extended_full_viewing_key` is deprecated. - let extfvk = extsk.to_extended_full_viewing_key(); - let dfvk: DiversifiableFullViewingKey = extsk.to_diversifiable_full_viewing_key(); - let key_to_be_stored = - zcash_client_backend::encoding::encode_extended_full_viewing_key("zxviews", &extfvk); + let efvk = mock_sapling_efvk(&[]); + let dfvk = efvk.to_diversifiable_full_viewing_key(); + let key_to_be_stored = encode_extended_full_viewing_key("zxviews", &efvk); // Create a database - let mut s = new_test_storage(Network::Mainnet); + let mut s = new_test_storage(network); // Insert the generated key to the database s.add_sapling_key(&key_to_be_stored, None); @@ -170,7 +169,7 @@ fn scanning_fake_blocks_store_key_and_results() -> Result<()> { .expect("height is stored") .next() .expect("height is not maximum"), - s.min_sapling_birthday_height() + network.sapling_activation_height() ); let nf = Nullifier([7; 32]); diff --git a/zebrad/tests/common/shielded_scan/scans_for_new_key.rs b/zebrad/tests/common/shielded_scan/scans_for_new_key.rs index 35f0c320f07..99bd4b15315 100644 --- a/zebrad/tests/common/shielded_scan/scans_for_new_key.rs +++ b/zebrad/tests/common/shielded_scan/scans_for_new_key.rs @@ -107,7 +107,12 @@ pub(crate) async fn run() -> Result<()> { tracing::info!("started scan task, sending register keys message with zecpages key to start scanning for a new key",); - scan_task.register_keys(parsed_keys)?; + scan_task.register_keys( + parsed_keys + .into_iter() + .map(|key| (key.0, Some(key.1 .2 .0))) + .collect(), + )?; tracing::info!( ?WAIT_FOR_RESULTS_DURATION, From f1ef371a052095d601a8cceb0809d9d78ed24a9f Mon Sep 17 00:00:00 2001 From: Marek Date: Fri, 9 Feb 2024 12:28:24 +0100 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Arya --- zebra-chain/src/parameters/network.rs | 2 +- zebra-scan/src/service/scan_task/tests/vectors.rs | 2 +- zebrad/tests/common/shielded_scan/scans_for_new_key.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/zebra-chain/src/parameters/network.rs b/zebra-chain/src/parameters/network.rs index 753e8230dbf..6901cb2b8be 100644 --- a/zebra-chain/src/parameters/network.rs +++ b/zebra-chain/src/parameters/network.rs @@ -130,7 +130,7 @@ impl Network { *self != Network::Mainnet } - /// Returns the minimum Sapling birthday height. + /// Returns the Sapling activation height for this network. pub fn sapling_activation_height(self) -> Height { // Assume that the genesis block never contains shielded inputs or outputs. // diff --git a/zebra-scan/src/service/scan_task/tests/vectors.rs b/zebra-scan/src/service/scan_task/tests/vectors.rs index 21920423724..e8cdfac0cfd 100644 --- a/zebra-scan/src/service/scan_task/tests/vectors.rs +++ b/zebra-scan/src/service/scan_task/tests/vectors.rs @@ -15,7 +15,7 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { // Send some keys to be registered let num_keys = 10; - let sapling_keys = mock_sapling_viewing_keys(num_keys as u8); + let sapling_keys = mock_sapling_viewing_keys(num_keys.try_into().expect("should fit in u8")); let sapling_keys_with_birth_heights: Vec<(String, Option)> = sapling_keys.into_iter().zip((0..).map(Some)).collect(); mock_scan_task.register_keys(sapling_keys_with_birth_heights.clone())?; diff --git a/zebrad/tests/common/shielded_scan/scans_for_new_key.rs b/zebrad/tests/common/shielded_scan/scans_for_new_key.rs index 95749ea0284..9064b54c975 100644 --- a/zebrad/tests/common/shielded_scan/scans_for_new_key.rs +++ b/zebrad/tests/common/shielded_scan/scans_for_new_key.rs @@ -108,7 +108,7 @@ pub(crate) async fn run() -> Result<()> { scan_task.register_keys( parsed_keys .into_iter() - .map(|key| (key.0, Some(key.1 .2 .0))) + .map(|(key, (_, _, Height(h)))| (key, Some(h))) .collect(), )?; From fda282b5edcb3a3e5dfc589e217ca54a3ea048aa Mon Sep 17 00:00:00 2001 From: Marek Date: Fri, 9 Feb 2024 14:21:12 +0100 Subject: [PATCH 6/8] Remove a redundant comment I don't think we need to assume the genesis block doesn't contain shielded data as the comment says. --- zebra-chain/src/parameters/network.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/zebra-chain/src/parameters/network.rs b/zebra-chain/src/parameters/network.rs index 6901cb2b8be..89eedfd3cd3 100644 --- a/zebra-chain/src/parameters/network.rs +++ b/zebra-chain/src/parameters/network.rs @@ -132,12 +132,6 @@ impl Network { /// Returns the Sapling activation height for this network. pub fn sapling_activation_height(self) -> Height { - // Assume that the genesis block never contains shielded inputs or outputs. - // - // # Consensus - // - // For Zcash mainnet and the public testnet, Sapling activates above genesis, - // so this is always true. super::NetworkUpgrade::Sapling .activation_height(self) .expect("Sapling activation height needs to be set") From 5118f76a9199c2158f20fe9e91c915e538fcc88e Mon Sep 17 00:00:00 2001 From: Marek Date: Fri, 9 Feb 2024 14:22:49 +0100 Subject: [PATCH 7/8] Avoid using a single-letter variable --- zebra-scan/src/tests/vectors.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/zebra-scan/src/tests/vectors.rs b/zebra-scan/src/tests/vectors.rs index 3d772873071..7af0d5dedb3 100644 --- a/zebra-scan/src/tests/vectors.rs +++ b/zebra-scan/src/tests/vectors.rs @@ -156,15 +156,16 @@ fn scanning_fake_blocks_store_key_and_results() -> Result<()> { let key_to_be_stored = encode_extended_full_viewing_key("zxviews", &efvk); // Create a database - let mut s = new_test_storage(network); + let mut storage = new_test_storage(network); // Insert the generated key to the database - s.add_sapling_key(&key_to_be_stored, None); + storage.add_sapling_key(&key_to_be_stored, None); // Check key was added - assert_eq!(s.sapling_keys_last_heights().len(), 1); + assert_eq!(storage.sapling_keys_last_heights().len(), 1); assert_eq!( - s.sapling_keys_last_heights() + storage + .sapling_keys_last_heights() .get(&key_to_be_stored) .expect("height is stored") .next() @@ -185,7 +186,7 @@ fn scanning_fake_blocks_store_key_and_results() -> Result<()> { SaplingScannedResult::from_bytes_in_display_order(*result.transactions()[0].txid.as_ref()); // Add result to database - s.add_sapling_results( + storage.add_sapling_results( &key_to_be_stored, Height(1), [(TransactionIndex::from_usize(0), result)].into(), @@ -193,7 +194,7 @@ fn scanning_fake_blocks_store_key_and_results() -> Result<()> { // Check the result was added assert_eq!( - s.sapling_results(&key_to_be_stored).get(&Height(1)), + storage.sapling_results(&key_to_be_stored).get(&Height(1)), Some(&vec![result]) ); From e634f192ded2efb3ee51076d1400300723385bc0 Mon Sep 17 00:00:00 2001 From: Marek Date: Fri, 9 Feb 2024 14:23:21 +0100 Subject: [PATCH 8/8] Refactor mocking Sapling scanning keys --- zebra-scan/src/service/scan_task/tests/vectors.rs | 8 ++++---- zebra-scan/src/tests.rs | 14 +++++++++----- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/zebra-scan/src/service/scan_task/tests/vectors.rs b/zebra-scan/src/service/scan_task/tests/vectors.rs index e8cdfac0cfd..8899ddbbd7f 100644 --- a/zebra-scan/src/service/scan_task/tests/vectors.rs +++ b/zebra-scan/src/service/scan_task/tests/vectors.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use color_eyre::Report; -use crate::{service::ScanTask, tests::mock_sapling_viewing_keys}; +use crate::{service::ScanTask, tests::mock_sapling_scanning_keys}; /// Test that [`ScanTask::process_messages`] adds and removes keys as expected for `RegisterKeys` and `DeleteKeys` command #[tokio::test] @@ -15,7 +15,7 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { // Send some keys to be registered let num_keys = 10; - let sapling_keys = mock_sapling_viewing_keys(num_keys.try_into().expect("should fit in u8")); + let sapling_keys = mock_sapling_scanning_keys(num_keys.try_into().expect("should fit in u8")); let sapling_keys_with_birth_heights: Vec<(String, Option)> = sapling_keys.into_iter().zip((0..).map(Some)).collect(); mock_scan_task.register_keys(sapling_keys_with_birth_heights.clone())?; @@ -55,7 +55,7 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { // Check that keys can't be overridden. - let sapling_keys = mock_sapling_viewing_keys(20); + let sapling_keys = mock_sapling_scanning_keys(20); let sapling_keys_with_birth_heights: Vec<(String, Option)> = sapling_keys .clone() .into_iter() @@ -81,7 +81,7 @@ async fn scan_task_processes_messages_correctly() -> Result<(), Report> { // Check that it removes keys correctly - let sapling_keys = mock_sapling_viewing_keys(30); + let sapling_keys = mock_sapling_scanning_keys(30); let done_rx = mock_scan_task.remove_keys(&sapling_keys)?; diff --git a/zebra-scan/src/tests.rs b/zebra-scan/src/tests.rs index 3aa5fd51080..72272cc5978 100644 --- a/zebra-scan/src/tests.rs +++ b/zebra-scan/src/tests.rs @@ -44,6 +44,7 @@ use zebra_chain::{ transparent::{CoinbaseData, Input}, work::{difficulty::CompactDifficulty, equihash::Solution}, }; +use zebra_state::SaplingScanningKey; #[cfg(test)] mod vectors; @@ -54,14 +55,17 @@ pub const ZECPAGES_SAPLING_VIEWING_KEY: &str = "zxviews1q0duytgcqqqqpqre26wkl45g /// A fake viewing key in an incorrect format. pub const FAKE_SAPLING_VIEWING_KEY: &str = "zxviewsfake"; -/// Generates fake Sapling viewing keys. -pub fn mock_sapling_viewing_keys(i: u8) -> Vec { - let mut keys: Vec = vec![]; +/// Generates `num_keys` of [`SaplingScanningKey`]s for tests. +/// +/// The keys are seeded only from their index in the returned `Vec`, so repeated calls return same +/// keys at a particular index. +pub fn mock_sapling_scanning_keys(num_keys: u8) -> Vec { + let mut keys: Vec = vec![]; - for i in 0..i { + for seed in 0..num_keys { keys.push(encode_extended_full_viewing_key( "zxviews", - &mock_sapling_efvk(&[i]), + &mock_sapling_efvk(&[seed]), )); }