fix(security): fix concurrency issues in tree key formats, and CPU usage in genesis tree roots

book/src/dev/state-db-upgrades.md

@@ -40,6 +40,28 @@ This means that:
 
 If there is an upgrade failure, it can panic and tell the user to delete their cached state and re-launch Zebra.
 
+### Performance Constraints
+
+Some column family access patterns can lead to very poor performance.
+
+Known performance issues include:
+- using an iterator on a column family which also deletes keys
+- creating large numbers of iterators
+- holding an iterator for a long time
+
+See the performance notes and bug reports in:
+- https://github.com/facebook/rocksdb/wiki/Iterator#iterating-upper-bound-and-lower-bound
+- https://tracker.ceph.com/issues/55324
+- https://jira.mariadb.org/browse/MDEV-19670
+
+But we need to use iterators for some operations, so our alternatives are (in preferred order):
+1. Minimise the number of keys we delete, and how often we delete them
+2. Avoid using iterators on column families where we delete keys
+3. If we must use iterators on those column families, set read bounds to minimise the amount of deleted data that is read
+
+Currently only UTXOs require key deletion, and only `utxo_loc_by_transparent_addr_loc` requires
+deletion and iterators.
+
 ### Implementation Steps
 
 - [ ] update the [database format](https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/state-db-upgrades.md#current) in the Zebra docs
@@ -87,7 +109,7 @@ We use the following rocksdb column families:
 | *Sprout*                           |                        |                               |         |
 | `sprout_nullifiers`                | `sprout::Nullifier`    | `()`                          | Create  |
 | `sprout_anchors`                   | `sprout::tree::Root`   | `sprout::NoteCommitmentTree`  | Create  |
-| `sprout_note_commitment_tree`      | `block::Height`        | `sprout::NoteCommitmentTree`  | Delete  |
+| `sprout_note_commitment_tree`      | `()`                   | `sprout::NoteCommitmentTree`  | Update  |
 | *Sapling*                          |                        |                               |         |
 | `sapling_nullifiers`               | `sapling::Nullifier`   | `()`                          | Create  |
 | `sapling_anchors`                  | `sapling::tree::Root`  | `()`                          | Create  |
@@ -99,7 +121,7 @@ We use the following rocksdb column families:
 | `orchard_note_commitment_tree`     | `block::Height`        | `orchard::NoteCommitmentTree` | Create  |
 | `orchard_note_commitment_subtree`  | `block::Height`        | `NoteCommitmentSubtreeData`   | Create  |
 | *Chain*                            |                        |                               |         |
-| `history_tree`                     | `block::Height`        | `NonEmptyHistoryTree`         | Delete  |
+| `history_tree`                     | `()`                   | `NonEmptyHistoryTree`         | Update  |
 | `tip_chain_value_pool`             | `()`                   | `ValueBalance`                | Update  |
 
 Zcash structures are encoded using `ZcashSerialize`/`ZcashDeserialize`.
@@ -131,6 +153,7 @@ Amounts:
 
 Derived Formats:
 - `*::NoteCommitmentTree`: `bincode` using `serde`
+  - stored note commitment trees always have cached roots
 - `NonEmptyHistoryTree`: `bincode` using `serde`, using `zcash_history`'s `serde` implementation
 
 

zebra-state/src/constants.rs

@@ -49,11 +49,11 @@ pub(crate) const DATABASE_FORMAT_VERSION: u64 = 25;
 /// - adding new column families,
 /// - changing the format of a column family in a compatible way, or
 /// - breaking changes with compatibility code in all supported Zebra versions.
-pub(crate) const DATABASE_FORMAT_MINOR_VERSION: u64 = 2;
+pub(crate) const DATABASE_FORMAT_MINOR_VERSION: u64 = 3;
 
 /// The database format patch version, incremented each time the on-disk database format has a
 /// significant format compatibility fix.
-pub(crate) const DATABASE_FORMAT_PATCH_VERSION: u64 = 2;
+pub(crate) const DATABASE_FORMAT_PATCH_VERSION: u64 = 0;
 
 /// Returns the highest database version that modifies the subtree index format.
 ///

zebra-state/src/lib.rs

@@ -66,7 +66,7 @@ pub use response::GetBlockTemplateChainInfo;
 pub use service::{
     arbitrary::{populated_state, CHAIN_TIP_UPDATE_WAIT_LIMIT},
     chain_tip::{ChainTipBlock, ChainTipSender},
-    finalized_state::MAX_ON_DISK_HEIGHT,
+    finalized_state::{DiskWriteBatch, MAX_ON_DISK_HEIGHT},
     init_test, init_test_services, ReadStateService,
 };
 

zebra-state/src/service/check/tests/anchors.rs

@@ -6,8 +6,9 @@ use zebra_chain::{
     amount::Amount,
     block::{Block, Height},
     primitives::Groth16Proof,
+    sapling,
     serialization::ZcashDeserializeInto,
-    sprout::JoinSplit,
+    sprout::{self, JoinSplit},
     transaction::{JoinSplitData, LockTime, Transaction, UnminedTx},
 };
 
@@ -18,7 +19,7 @@ use crate::{
         write::validate_and_commit_non_finalized,
     },
     tests::setup::{new_state_with_mainnet_genesis, transaction_v4_from_coinbase},
-    SemanticallyVerifiedBlock, ValidateContextError,
+    DiskWriteBatch, SemanticallyVerifiedBlock, ValidateContextError,
 };
 
 // Sprout
@@ -31,20 +32,30 @@ fn check_sprout_anchors() {
 
     let (finalized_state, mut non_finalized_state, _genesis) = new_state_with_mainnet_genesis();
 
-    // Bootstrap a block at height == 1.
+    // Delete the empty anchor from the database
+    let mut batch = DiskWriteBatch::new();
+    batch.delete_sprout_anchor(
+        &finalized_state,
+        &sprout::tree::NoteCommitmentTree::default().root(),
+    );
+    finalized_state
+        .write_batch(batch)
+        .expect("unexpected I/O error");
+
+    // Create a block at height 1.
     let block_1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
         .zcash_deserialize_into::<Block>()
         .expect("block should deserialize");
 
-    // Bootstrap a block just before the first Sprout anchors.
+    // Create a block just before the first Sprout anchors.
     let block_395 = zebra_test::vectors::BLOCK_MAINNET_395_BYTES
         .zcash_deserialize_into::<Block>()
         .expect("block should deserialize");
 
     // Add initial transactions to [`block_1`].
     let block_1 = prepare_sprout_block(block_1, block_395);
 
-    // Bootstrap a block at height == 2 that references the Sprout note commitment tree state
+    // Create a block at height == 2 that references the Sprout note commitment tree state
     // from [`block_1`].
     let block_2 = zebra_test::vectors::BLOCK_MAINNET_2_BYTES
         .zcash_deserialize_into::<Block>()
@@ -74,10 +85,13 @@ fn check_sprout_anchors() {
         )
     });
 
-    assert!(matches!(
-        check_unmined_tx_anchors_result,
-        Err(ValidateContextError::UnknownSproutAnchor { .. })
-    ));
+    assert!(
+        matches!(
+            check_unmined_tx_anchors_result,
+            Err(ValidateContextError::UnknownSproutAnchor { .. }),
+        ),
+        "unexpected result: {check_unmined_tx_anchors_result:?}",
+    );
 
     // Validate and commit [`block_1`]. This will add an anchor referencing the
     // empty note commitment tree to the state.
@@ -182,7 +196,17 @@ fn check_sapling_anchors() {
 
     let (finalized_state, mut non_finalized_state, _genesis) = new_state_with_mainnet_genesis();
 
-    // Bootstrap a block at height == 1 that has the first Sapling note commitments
+    // Delete the empty anchor from the database
+    let mut batch = DiskWriteBatch::new();
+    batch.delete_sapling_anchor(
+        &finalized_state,
+        &sapling::tree::NoteCommitmentTree::default().root(),
+    );
+    finalized_state
+        .write_batch(batch)
+        .expect("unexpected I/O error");
+
+    // Create a block at height 1 that has the first Sapling note commitments
     let mut block1 = zebra_test::vectors::BLOCK_MAINNET_1_BYTES
         .zcash_deserialize_into::<Block>()
         .expect("block should deserialize");
@@ -227,7 +251,7 @@ fn check_sapling_anchors() {
 
     let block1 = Arc::new(block1).prepare();
 
-    // Bootstrap a block at height == 2 that references the Sapling note commitment tree state
+    // Create a block at height == 2 that references the Sapling note commitment tree state
     // from earlier block
     let mut block2 = zebra_test::vectors::BLOCK_MAINNET_2_BYTES
         .zcash_deserialize_into::<Block>()
@@ -315,3 +339,5 @@ fn check_sapling_anchors() {
         Ok(())
     );
 }
+
+// TODO: create a test for orchard anchors

zebra-state/src/service/finalized_state.rs

@@ -42,7 +42,10 @@ pub use disk_format::{OutputIndex, OutputLocation, TransactionLocation, MAX_ON_D
 
 pub(super) use zebra_db::ZebraDb;
 
-pub(super) use disk_db::DiskWriteBatch;
+#[cfg(any(test, feature = "proptest-impl"))]
+pub use disk_db::DiskWriteBatch;
+#[cfg(not(any(test, feature = "proptest-impl")))]
+use disk_db::DiskWriteBatch;
 
 /// The finalized part of the chain state, stored in the db.
 ///
@@ -88,14 +91,33 @@ pub struct FinalizedState {
 }
 
 impl FinalizedState {
-    /// Returns an on-disk database instance for `config` and `network`.
+    /// Returns an on-disk database instance for `config`, `network`, and `elastic_db`.
     /// If there is no existing database, creates a new database on disk.
     pub fn new(
         config: &Config,
         network: Network,
         #[cfg(feature = "elasticsearch")] elastic_db: Option<elasticsearch::Elasticsearch>,
     ) -> Self {
-        let db = ZebraDb::new(config, network, false);
+        Self::new_with_debug(
+            config,
+            network,
+            false,
+            #[cfg(feature = "elasticsearch")]
+            elastic_db,
+        )
+    }
+
+    /// Returns an on-disk database instance with the supplied production and debug settings.
+    /// If there is no existing database, creates a new database on disk.
+    ///
+    /// This method is intended for use in tests.
+    pub(crate) fn new_with_debug(
+        config: &Config,
+        network: Network,
+        debug_skip_format_upgrades: bool,
+        #[cfg(feature = "elasticsearch")] elastic_db: Option<elasticsearch::Elasticsearch>,
+    ) -> Self {
+        let db = ZebraDb::new(config, network, debug_skip_format_upgrades);
 
         #[cfg(feature = "elasticsearch")]
         let new_state = Self {

zebra-state/src/service/finalized_state/disk_db.rs

@@ -98,7 +98,7 @@ pub struct DiskDb {
 //       and make them accessible via read-only methods
 #[must_use = "batches must be written to the database"]
 #[derive(Default)]
-pub(crate) struct DiskWriteBatch {
+pub struct DiskWriteBatch {
     /// The inner RocksDB write batch.
     batch: rocksdb::WriteBatch,
 }

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/empty_column_families@mainnet_0.snap

@@ -5,13 +5,10 @@ expression: empty_column_families
 [
   "balance_by_transparent_addr: no entries",
   "history_tree: no entries",
-  "orchard_anchors: no entries",
   "orchard_note_commitment_subtree: no entries",
   "orchard_nullifiers: no entries",
-  "sapling_anchors: no entries",
   "sapling_note_commitment_subtree: no entries",
   "sapling_nullifiers: no entries",
-  "sprout_anchors: no entries",
   "sprout_nullifiers: no entries",
   "tip_chain_value_pool: no entries",
   "tx_loc_by_transparent_addr_loc: no entries",

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/empty_column_families@testnet_0.snap

@@ -5,13 +5,10 @@ expression: empty_column_families
 [
   "balance_by_transparent_addr: no entries",
   "history_tree: no entries",
-  "orchard_anchors: no entries",
   "orchard_note_commitment_subtree: no entries",
   "orchard_nullifiers: no entries",
-  "sapling_anchors: no entries",
   "sapling_note_commitment_subtree: no entries",
   "sapling_nullifiers: no entries",
-  "sprout_anchors: no entries",
   "sprout_nullifiers: no entries",
   "tip_chain_value_pool: no entries",
   "tx_loc_by_transparent_addr_loc: no entries",

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/orchard_anchors_raw_data@mainnet_0.snap

@@ -0,0 +1,10 @@
+---
+source: zebra-state/src/service/finalized_state/disk_format/tests/snapshot.rs
+expression: cf_data
+---
+[
+  KV(
+    k: "ae2935f1dfd8a24aed7c70df7de3a668eb7a49b1319880dde2bbd9031ae5d82f",
+    v: "",
+  ),
+]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/orchard_anchors_raw_data@testnet_0.snap

@@ -0,0 +1,10 @@
+---
+source: zebra-state/src/service/finalized_state/disk_format/tests/snapshot.rs
+expression: cf_data
+---
+[
+  KV(
+    k: "ae2935f1dfd8a24aed7c70df7de3a668eb7a49b1319880dde2bbd9031ae5d82f",
+    v: "",
+  ),
+]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sapling_anchors_raw_data@mainnet_0.snap

@@ -0,0 +1,10 @@
+---
+source: zebra-state/src/service/finalized_state/disk_format/tests/snapshot.rs
+expression: cf_data
+---
+[
+  KV(
+    k: "fbc2f4300c01f0b7820d00e3347c8da4ee614674376cbc45359daa54f9b5493e",
+    v: "",
+  ),
+]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sapling_anchors_raw_data@testnet_0.snap

@@ -0,0 +1,10 @@
+---
+source: zebra-state/src/service/finalized_state/disk_format/tests/snapshot.rs
+expression: cf_data
+---
+[
+  KV(
+    k: "fbc2f4300c01f0b7820d00e3347c8da4ee614674376cbc45359daa54f9b5493e",
+    v: "",
+  ),
+]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_anchors_raw_data@mainnet_0.snap

@@ -0,0 +1,10 @@
+---
+source: zebra-state/src/service/finalized_state/disk_format/tests/snapshot.rs
+expression: cf_data
+---
+[
+  KV(
+    k: "d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
+    v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
+  ),
+]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_anchors_raw_data@testnet_0.snap

@@ -0,0 +1,10 @@
+---
+source: zebra-state/src/service/finalized_state/disk_format/tests/snapshot.rs
+expression: cf_data
+---
+[
+  KV(
+    k: "d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
+    v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
+  ),
+]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_note_commitment_tree_raw_data@mainnet_0.snap

@@ -4,7 +4,7 @@ expression: cf_data
 ---
 [
   KV(
-    k: "000000",
+    k: "",
     v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
   ),
 ]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_note_commitment_tree_raw_data@mainnet_1.snap

@@ -4,7 +4,7 @@ expression: cf_data
 ---
 [
   KV(
-    k: "000001",
+    k: "",
     v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
   ),
 ]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_note_commitment_tree_raw_data@mainnet_2.snap

@@ -4,7 +4,7 @@ expression: cf_data
 ---
 [
   KV(
-    k: "000002",
+    k: "",
     v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
   ),
 ]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_note_commitment_tree_raw_data@testnet_0.snap

@@ -4,7 +4,7 @@ expression: cf_data
 ---
 [
   KV(
-    k: "000000",
+    k: "",
     v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
   ),
 ]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_note_commitment_tree_raw_data@testnet_1.snap

@@ -4,7 +4,7 @@ expression: cf_data
 ---
 [
   KV(
-    k: "000001",
+    k: "",
     v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
   ),
 ]

zebra-state/src/service/finalized_state/disk_format/tests/snapshots/sprout_note_commitment_tree_raw_data@testnet_2.snap

@@ -4,7 +4,7 @@ expression: cf_data
 ---
 [
   KV(
-    k: "000002",
+    k: "",
     v: "0001d7c612c817793191a1e68652121876d6b3bde40f4fa52bc314145ce6e5cdd259",
   ),
 ]