diff --git a/zebra-state/src/service/finalized_state/disk_format/chain.rs b/zebra-state/src/service/finalized_state/disk_format/chain.rs index df47c595e89..512424ec16f 100644 --- a/zebra-state/src/service/finalized_state/disk_format/chain.rs +++ b/zebra-state/src/service/finalized_state/disk_format/chain.rs @@ -10,8 +10,12 @@ use std::collections::BTreeMap; use bincode::Options; use zebra_chain::{ - amount::NonNegative, block::Height, history_tree::NonEmptyHistoryTree, parameters::Network, - primitives::zcash_history, value_balance::ValueBalance, + amount::NonNegative, + block::Height, + history_tree::{HistoryTree, NonEmptyHistoryTree}, + parameters::Network, + primitives::zcash_history, + value_balance::ValueBalance, }; use crate::service::finalized_state::disk_format::{FromDisk, IntoDisk}; @@ -78,3 +82,10 @@ impl FromDisk for NonEmptyHistoryTree { .expect("deserialization format should match the serialization format used by IntoDisk") } } + +// We don't write empty history trees to disk, so we know this one is non-empty. +impl FromDisk for HistoryTree { + fn from_bytes(bytes: impl AsRef<[u8]>) -> Self { + NonEmptyHistoryTree::from_bytes(bytes).into() + } +} diff --git a/zebra-state/src/service/finalized_state/zebra_db/chain.rs b/zebra-state/src/service/finalized_state/zebra_db/chain.rs index a0fd331b582..1e31741b0a0 100644 --- a/zebra-state/src/service/finalized_state/zebra_db/chain.rs +++ b/zebra-state/src/service/finalized_state/zebra_db/chain.rs @@ -14,59 +14,74 @@ use std::{borrow::Borrow, collections::HashMap, sync::Arc}; use zebra_chain::{ - amount::NonNegative, - history_tree::{HistoryTree, NonEmptyHistoryTree}, - transparent, - value_balance::ValueBalance, + amount::NonNegative, history_tree::HistoryTree, transparent, value_balance::ValueBalance, }; use crate::{ request::SemanticallyVerifiedBlockWithTrees, service::finalized_state::{ disk_db::{DiskDb, DiskWriteBatch, ReadDisk, WriteDisk}, - disk_format::RawBytes, zebra_db::ZebraDb, }, BoxError, SemanticallyVerifiedBlock, }; impl ZebraDb { - /// Returns the ZIP-221 history tree of the finalized tip or `None` - /// if it does not exist yet in the state (pre-Heartwood). + /// Returns the ZIP-221 history tree of the finalized tip. + /// + /// If history trees have not been activated yet (pre-Heartwood), or the state is empty, + /// returns an empty history tree. pub fn history_tree(&self) -> Arc { - if self.finalized_tip_height().is_some() { - let history_tree_cf = self.db.cf_handle("history_tree").unwrap(); - - // # Concurrency - // - // There is only one tree in this column family, which is atomically updated by a block - // write batch (database transaction). If this update runs between the height read and - // the tree read, the height will be wrong, and the tree will be missing. - // That could cause consensus bugs. - // - // Instead, always read the last tree in the column family, regardless of height. - // - // See ticket #7581 for more details. - // - // TODO: this concurrency bug will be permanently fixed in PR #7392, - // by changing the block update to overwrite the tree, rather than deleting it. - // - // # Forwards Compatibility - // - // This code can read the column family format in 1.2.0 and earlier (tip height key), - // and after PR #7392 is merged (empty key). - let history_tree: Option = self - .db - .zs_last_key_value(&history_tree_cf) - // RawBytes will deserialize both Height and `()` (empty) keys. - .map(|(_key, value): (RawBytes, _)| value); - - if let Some(non_empty_tree) = history_tree { - return Arc::new(HistoryTree::from(non_empty_tree)); - } + if self.is_empty() { + return Arc::::default(); + } + + // # Performance + // + // Using `zs_last_key_value()` on this column family significantly reduces sync performance + // (#7618). But it seems to work for other column families. This is probably because + // `zs_delete()` is also used on the same column family: + // + // + // + // See also the performance notes in: + // + // + // This bug will be fixed by PR #7392, because it changes this column family to update the + // existing key, rather than deleting old keys. + let history_tree_cf = self.db.cf_handle("history_tree").unwrap(); + + // # Forwards Compatibility + // + // This code can read the column family format in 1.2.0 and earlier (tip height key), + // and after PR #7392 is merged (empty key). The height-based code can be removed when + // versions 1.2.0 and earlier are no longer supported. + // + // # Concurrency + // + // There is only one tree in this column family, which is atomically updated by a block + // write batch (database transaction). If this update runs between the height read and + // the tree read, the height will be wrong, and the tree will be missing. + // That could cause consensus bugs. + // + // Instead, try reading the new empty-key format (from PR #7392) first, + // then read the old format if needed. + // + // See ticket #7581 for more details. + // + // TODO: this concurrency bug will be permanently fixed in PR #7392, + // by changing the block update to overwrite the tree, rather than deleting it. + let mut history_tree: Option> = self.db.zs_get(&history_tree_cf, &()); + + if history_tree.is_none() { + let tip_height = self + .finalized_tip_height() + .expect("just checked for an empty database"); + + history_tree = self.db.zs_get(&history_tree_cf, &tip_height); } - Default::default() + history_tree.unwrap_or_default() } /// Returns the stored `ValueBalance` for the best chain at the finalized tip height. diff --git a/zebra-state/src/service/finalized_state/zebra_db/shielded.rs b/zebra-state/src/service/finalized_state/zebra_db/shielded.rs index b74ed6d2ce1..fc0cca9d5a4 100644 --- a/zebra-state/src/service/finalized_state/zebra_db/shielded.rs +++ b/zebra-state/src/service/finalized_state/zebra_db/shielded.rs @@ -30,7 +30,6 @@ use crate::{ request::SemanticallyVerifiedBlockWithTrees, service::finalized_state::{ disk_db::{DiskDb, DiskWriteBatch, ReadDisk, WriteDisk}, - disk_format::RawBytes, zebra_db::ZebraDb, }, BoxError, SemanticallyVerifiedBlock, @@ -85,34 +84,49 @@ impl ZebraDb { /// Returns the Sprout note commitment tree of the finalized tip /// or the empty tree if the state is empty. pub fn sprout_tree(&self) -> Arc { - if self.finalized_tip_height().is_none() { - return Default::default(); + if self.is_empty() { + return Arc::::default(); } - let sprout_nct_handle = self.db.cf_handle("sprout_note_commitment_tree").unwrap(); + // # Performance + // + // Using `zs_last_key_value()` on this column family significantly reduces sync performance + // (#7618). This is probably because `zs_delete()` is also used on the same column family. + // See the comment in `ZebraDb::history_tree()` for details. + // + // This bug will be fixed by PR #7392, because it changes this column family to update the + // existing key, rather than deleting old keys. + let sprout_tree_cf = self.db.cf_handle("sprout_note_commitment_tree").unwrap(); + // # Forwards Compatibility + // + // This code can read the column family format in 1.2.0 and earlier (tip height key), + // and after PR #7392 is merged (empty key). The height-based code can be removed when + // versions 1.2.0 and earlier are no longer supported. + // // # Concurrency // // There is only one tree in this column family, which is atomically updated by a block - // write batch (database transaction). If this update runs between the height read and the - // tree read, the height will be wrong, and the tree will be missing. + // write batch (database transaction). If this update runs between the height read and + // the tree read, the height will be wrong, and the tree will be missing. + // That could cause consensus bugs. // - // Instead, always read the last tree in the column family, regardless of height. - // - // See ticket #7581 for more details. + // See the comment in `ZebraDb::history_tree()` for details. // // TODO: this concurrency bug will be permanently fixed in PR #7392, // by changing the block update to overwrite the tree, rather than deleting it. - // - // # Forwards Compatibility - // - // This code can read the column family format in 1.2.0 and earlier (tip height key), - // and after PR #7392 is merged (empty key). - self.db - .zs_last_key_value(&sprout_nct_handle) - // RawBytes will deserialize both Height and `()` (empty) keys. - .map(|(_key, value): (RawBytes, _)| Arc::new(value)) - .expect("Sprout note commitment tree must exist if there is a finalized tip") + let mut sprout_tree: Option> = + self.db.zs_get(&sprout_tree_cf, &()); + + if sprout_tree.is_none() { + let tip_height = self + .finalized_tip_height() + .expect("just checked for an empty database"); + + sprout_tree = self.db.zs_get(&sprout_tree_cf, &tip_height); + } + + sprout_tree.expect("Sprout note commitment tree must exist if there is a finalized tip") } /// Returns the Sprout note commitment tree matching the given anchor.