From 77b638012b3704082364377f00f87aea1e45c74d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Sat, 1 Jul 2023 17:58:01 -0600 Subject: [PATCH] Remove `zcash_client_backend::data_api::chain::validate_chain` Local chain validation will be performed internal to `scan_cached_blocks`, and as handling of chain reorgs will need to change to support out-of-order scanning, the `validate_chain` method will be superfluous. It is removed in advance of other changes in order to avoid updating it to reflect the forthcoming changes. --- zcash_client_backend/CHANGELOG.md | 2 + zcash_client_backend/src/data_api/chain.rs | 101 +-------------------- zcash_client_sqlite/src/chain.rs | 40 ++------ 3 files changed, 16 insertions(+), 127 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index fa7d27cc9..8a6fc7f36 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -75,6 +75,8 @@ and this library adheres to Rust's notion of feature flag, has been modified by the addition of a `sapling_tree` property. - `wallet::input_selection`: - `Proposal::target_height` (use `Proposal::min_target_height` instead). +- `zcash_client_backend::data_api::chain::validate_chain` TODO: document how + to handle validation given out-of-order blocks. - `zcash_client_backend::wallet::WalletSaplingOutput::{witness, witness_mut}` have been removed as individual incremental witnesses are no longer tracked on a per-note basis. The global note commitment tree for the wallet should be used diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index b09432198..a55470307 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -36,43 +36,12 @@ //! let mut db_data = testing::MockWalletDb::new(Network::TestNetwork); //! //! // 1) Download new CompactBlocks into block_source. -//! -//! // 2) Run the chain validator on the received blocks. //! // -//! // Given that we assume the server always gives us correct-at-the-time blocks, any -//! // errors are in the blocks we have previously cached or scanned. -//! let max_height_hash = db_data.get_max_height_hash().map_err(Error::Wallet)?; -//! if let Err(e) = validate_chain(&block_source, max_height_hash, None) { -//! match e { -//! Error::Chain(e) => { -//! // a) Pick a height to rewind to. -//! // -//! // This might be informed by some external chain reorg information, or -//! // heuristics such as the platform, available bandwidth, size of recent -//! // CompactBlocks, etc. -//! let rewind_height = e.at_height() - 10; -//! -//! // b) Rewind scanned block information. -//! db_data.truncate_to_height(rewind_height); -//! -//! // c) Delete cached blocks from rewind_height onwards. -//! // -//! // This does imply that assumed-valid blocks will be re-downloaded, but it -//! // is also possible that in the intervening time, a chain reorg has -//! // occurred that orphaned some of those blocks. -//! -//! // d) If there is some separate thread or service downloading -//! // CompactBlocks, tell it to go back and download from rewind_height -//! // onwards. -//! }, -//! e => { -//! // handle or return other errors -//! -//! } -//! } -//! } -//! -//! // 3) Scan (any remaining) cached blocks. +//! // 2) FIXME: Obtain necessary block metadata for continuity checking? +//! // +//! // 3) Scan cached blocks. +//! // +//! // FIXME: update documentation on how to detect when a rewind is required. //! // //! // At this point, the cache and scanned data are locally consistent (though not //! // necessarily consistent with the latest chain tip - this would be discovered the @@ -82,10 +51,7 @@ //! # } //! ``` -use std::convert::Infallible; - use zcash_primitives::{ - block::BlockHash, consensus::{self, BlockHeight}, sapling::{self, note_encryption::PreparedIncomingViewingKey}, zip32::Scope, @@ -143,63 +109,6 @@ pub trait BlockSource { F: FnMut(CompactBlock) -> Result<(), error::Error>; } -/// Checks that the scanned blocks in the data database, when combined with the recent -/// `CompactBlock`s in the block_source database, form a valid chain. -/// -/// This function is built on the core assumption that the information provided in the -/// block source is more likely to be accurate than the previously-scanned information. -/// This follows from the design (and trust) assumption that the `lightwalletd` server -/// provides accurate block information as of the time it was requested. -/// -/// Arguments: -/// - `block_source` Source of compact blocks -/// - `validate_from` Height & hash of last validated block; -/// - `limit` specified number of blocks that will be valididated. Callers providing -/// a `limit` argument are responsible of making subsequent calls to `validate_chain()` -/// to complete validating the remaining blocks stored on the `block_source`. If `none` -/// is provided, there will be no limit set to the validation and upper bound of the -/// validation range will be the latest height present in the `block_source`. -/// -/// Returns: -/// - `Ok(())` if the combined chain is valid up to the given height -/// and block hash. -/// - `Err(Error::Chain(cause))` if the combined chain is invalid. -/// - `Err(e)` if there was an error during validation unrelated to chain validity. -pub fn validate_chain( - block_source: &BlockSourceT, - mut validate_from: Option<(BlockHeight, BlockHash)>, - limit: Option, -) -> Result<(), Error> -where - BlockSourceT: BlockSource, -{ - // The block source will contain blocks above the `validate_from` height. Validate from that - // maximum height up to the chain tip, returning the hash of the block found in the block - // source at the `validate_from` height, which can then be used to verify chain integrity by - // comparing against the `validate_from` hash. - - block_source.with_blocks::<_, Infallible, Infallible>( - validate_from.map(|(h, _)| h + 1), - limit, - move |block| { - if let Some((valid_height, valid_hash)) = validate_from { - if block.height() != valid_height + 1 { - return Err(ChainError::block_height_discontinuity( - valid_height + 1, - block.height(), - ) - .into()); - } else if block.prev_hash() != valid_hash { - return Err(ChainError::prev_hash_mismatch(block.height()).into()); - } - } - - validate_from = Some((block.height(), block.hash())); - Ok(()) - }, - ) -} - /// Scans at most `limit` new blocks added to the block source for any transactions received by the /// tracked accounts. /// diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index 191b2a6b0..4d968bb36 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -279,7 +279,7 @@ mod tests { use zcash_client_backend::{ address::RecipientAddress, data_api::{ - chain::{error::Error, scan_cached_blocks, validate_chain}, + chain::scan_cached_blocks, wallet::{input_selection::GreedyInputSelector, spend}, WalletRead, WalletWrite, }, @@ -329,21 +329,9 @@ mod tests { insert_into_cache(&db_cache, &cb); - // Cache-only chain should be valid - let validate_chain_result = validate_chain( - &db_cache, - Some((fake_block_height, fake_block_hash)), - Some(1), - ); - - assert_matches!(validate_chain_result, Ok(())); - // Scan the cache scan_cached_blocks(&tests::network(), &db_cache, &mut db_data, None, None).unwrap(); - // Data-only chain should be valid - validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap(); - // Create a second fake CompactBlock sending more value to the address let (cb2, _) = fake_compact_block( sapling_activation_height() + 1, @@ -355,14 +343,8 @@ mod tests { ); insert_into_cache(&db_cache, &cb2); - // Data+cache chain should be valid - validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap(); - // Scan the cache again scan_cached_blocks(&tests::network(), &db_cache, &mut db_data, None, None).unwrap(); - - // Data-only chain should be valid - validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap(); } #[test] @@ -401,9 +383,6 @@ mod tests { // Scan the cache scan_cached_blocks(&tests::network(), &db_cache, &mut db_data, None, None).unwrap(); - // Data-only chain should be valid - validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap(); - // Create more fake CompactBlocks that don't connect to the scanned ones let (cb3, _) = fake_compact_block( sapling_activation_height() + 2, @@ -425,9 +404,10 @@ mod tests { insert_into_cache(&db_cache, &cb4); // Data+cache chain should be invalid at the data/cache boundary - let val_result = validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None); - - assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 2); + assert_matches!( + scan_cached_blocks(&tests::network(), &db_cache, &mut db_data, None, None), + Err(_) // FIXME: check error result more closely + ); } #[test] @@ -466,9 +446,6 @@ mod tests { // Scan the cache scan_cached_blocks(&tests::network(), &db_cache, &mut db_data, None, None).unwrap(); - // Data-only chain should be valid - validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None).unwrap(); - // Create more fake CompactBlocks that contain a reorg let (cb3, _) = fake_compact_block( sapling_activation_height() + 2, @@ -490,9 +467,10 @@ mod tests { insert_into_cache(&db_cache, &cb4); // Data+cache chain should be invalid inside the cache - let val_result = validate_chain(&db_cache, db_data.get_max_height_hash().unwrap(), None); - - assert_matches!(val_result, Err(Error::Chain(e)) if e.at_height() == sapling_activation_height() + 3); + assert_matches!( + scan_cached_blocks(&tests::network(), &db_cache, &mut db_data, None, None), + Err(_) // FIXME: check error result more closely + ); } #[test]