From ea9d976bbb4bd20413f56d3686f2c4ce08421ba2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 27 Mar 2024 13:57:47 +0000 Subject: [PATCH 1/5] core/storage: add `BlockHeight::checked_prev` --- crates/core/src/storage.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/core/src/storage.rs b/crates/core/src/storage.rs index 51f5ce3d53..1e8acc9cee 100644 --- a/crates/core/src/storage.rs +++ b/crates/core/src/storage.rs @@ -376,6 +376,11 @@ impl BlockHeight { pub fn prev_height(&self) -> BlockHeight { BlockHeight(self.0 - 1) } + + /// Get the height of the previous block if it won't underflow + pub fn checked_prev(&self) -> Option { + Some(BlockHeight(self.0.checked_sub(1)?)) + } } impl TryFrom<&[u8]> for BlockHash { From 84b0d6cf1253c5a2ce88285d68afa0b817bb2592 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 27 Mar 2024 13:26:51 +0000 Subject: [PATCH 2/5] rocksdb: separate non-persisted diffs into new "rollback" column family --- .../src/lib/node/ledger/storage/rocksdb.rs | 162 ++++++++---------- crates/core/src/storage.rs | 6 + 2 files changed, 81 insertions(+), 87 deletions(-) diff --git a/crates/apps/src/lib/node/ledger/storage/rocksdb.rs b/crates/apps/src/lib/node/ledger/storage/rocksdb.rs index 9e7f642086..a42dab312f 100644 --- a/crates/apps/src/lib/node/ledger/storage/rocksdb.rs +++ b/crates/apps/src/lib/node/ledger/storage/rocksdb.rs @@ -22,9 +22,15 @@ //! - `conversion_state`: MASP conversion state //! - `subspace`: accounts sub-spaces //! - `{address}/{dyn}`: any byte data associated with accounts -//! - `diffs`: diffs in account subspaces' key-vals -//! - `new/{dyn}`: value set in block height `h` -//! - `old/{dyn}`: value from predecessor block height +//! - `diffs`: diffs in account subspaces' key-vals modified with `persist_diff +//! == true` +//! - `{height}/new/{dyn}`: value set in block height `h` +//! - `{height}/old/{dyn}`: value from predecessor block height +//! - `rollback`: diffs in account subspaces' key-vals for keys modified with +//! `persist_diff == false` which are only kept for 1 block to support +//! rollback +//! - `{height}/new/{dyn}`: value set in block height `h` +//! - `{height}/old/{dyn}`: value from predecessor block height //! - `block`: block state //! - `results/{h}`: block results at height `h` //! - `h`: for each block at height `h`: @@ -40,6 +46,7 @@ //! - `all`: the hashes included up to the last block //! - `last`: the hashes included in the last block +use std::collections::HashSet; use std::fs::File; use std::io::{BufWriter, Write}; use std::path::Path; @@ -67,7 +74,8 @@ use namada::state::{ StoreType, DB, }; use namada::storage::{ - DbColFam, BLOCK_CF, DIFFS_CF, REPLAY_PROTECTION_CF, STATE_CF, SUBSPACE_CF, + DbColFam, BLOCK_CF, DIFFS_CF, REPLAY_PROTECTION_CF, ROLLBACK_CF, STATE_CF, + SUBSPACE_CF, }; use namada::token::ConversionState; use namada_sdk::migrations::DBUpdateVisitor; @@ -160,6 +168,14 @@ pub fn open( diffs_cf_opts.set_block_based_table_factory(&table_opts); cfs.push(ColumnFamilyDescriptor::new(DIFFS_CF, diffs_cf_opts)); + // for non-persisted diffs for rollback (read/update-intensive) + let mut rollback_cf_opts = Options::default(); + rollback_cf_opts.set_compression_type(DBCompressionType::Zstd); + rollback_cf_opts.set_compression_options(0, 0, 0, 1024 * 1024); + rollback_cf_opts.set_compaction_style(DBCompactionStyle::Level); + rollback_cf_opts.set_block_based_table_factory(&table_opts); + cfs.push(ColumnFamilyDescriptor::new(ROLLBACK_CF, rollback_cf_opts)); + // for the ledger state (update-intensive) let mut state_cf_opts = Options::default(); // No compression since the size of the state is small @@ -217,7 +233,11 @@ impl RocksDB { new_value: Option<&[u8]>, persist_diffs: bool, ) -> Result<()> { - let cf = self.get_column_family(DIFFS_CF)?; + let cf = if persist_diffs { + self.get_column_family(DIFFS_CF)? + } else { + self.get_column_family(ROLLBACK_CF)? + }; let (old_val_key, new_val_key) = old_and_new_diff_key(key, height)?; if let Some(old_value) = old_value { @@ -231,39 +251,6 @@ impl RocksDB { .put_cf(cf, new_val_key, new_value) .map_err(|e| Error::DBError(e.into_string()))?; } - - // If not persisting the diffs, remove the last diffs. - if !persist_diffs && height > BlockHeight::first() { - let mut height = height.prev_height(); - while height >= BlockHeight::first() { - let (old_diff_key, new_diff_key) = - old_and_new_diff_key(key, height)?; - let has_old_diff = self - .0 - .get_cf(cf, &old_diff_key) - .map_err(|e| Error::DBError(e.into_string()))? - .is_some(); - let has_new_diff = self - .0 - .get_cf(cf, &new_diff_key) - .map_err(|e| Error::DBError(e.into_string()))? - .is_some(); - if has_old_diff { - self.0 - .delete_cf(cf, old_diff_key) - .map_err(|e| Error::DBError(e.into_string()))?; - } - if has_new_diff { - self.0 - .delete_cf(cf, new_diff_key) - .map_err(|e| Error::DBError(e.into_string()))?; - } - if has_old_diff || has_new_diff { - break; - } - height = height.prev_height(); - } - } Ok(()) } @@ -278,7 +265,11 @@ impl RocksDB { new_value: Option<&[u8]>, persist_diffs: bool, ) -> Result<()> { - let cf = self.get_column_family(DIFFS_CF)?; + let cf = if persist_diffs { + self.get_column_family(DIFFS_CF)? + } else { + self.get_column_family(ROLLBACK_CF)? + }; let (old_val_key, new_val_key) = old_and_new_diff_key(key, height)?; if let Some(old_value) = old_value { @@ -288,35 +279,6 @@ impl RocksDB { if let Some(new_value) = new_value { batch.0.put_cf(cf, new_val_key, new_value); } - - // If not persisting the diffs, remove the last diffs. - if !persist_diffs && height > BlockHeight::first() { - let mut height = height.prev_height(); - while height >= BlockHeight::first() { - let (old_diff_key, new_diff_key) = - old_and_new_diff_key(key, height)?; - let has_old_diff = self - .0 - .get_cf(cf, &old_diff_key) - .map_err(|e| Error::DBError(e.into_string()))? - .is_some(); - let has_new_diff = self - .0 - .get_cf(cf, &new_diff_key) - .map_err(|e| Error::DBError(e.into_string()))? - .is_some(); - if has_old_diff { - batch.0.delete_cf(cf, old_diff_key); - } - if has_new_diff { - batch.0.delete_cf(cf, new_diff_key); - } - if has_old_diff || has_new_diff { - break; - } - height = height.prev_height(); - } - } Ok(()) } @@ -602,6 +564,10 @@ impl RocksDB { }, )?; + let mut batch = batch.into_inner().unwrap(); + + let subspace_cf = self.get_column_family(SUBSPACE_CF)?; + let diffs_cf = self.get_column_family(DIFFS_CF)?; // Look for diffs in this block to find what has been deleted let diff_new_key_prefix = Key { segments: vec![ @@ -609,24 +575,42 @@ impl RocksDB { NEW_DIFF_PREFIX.to_string().to_db_key(), ], }; + for (key_str, val, _) in + iter_diffs_prefix(self, diffs_cf, last_block.height, None, true) { - let mut batch_guard = batch.lock().unwrap(); - let subspace_cf = self.get_column_family(SUBSPACE_CF)?; - for (key, val, _) in - iter_diffs_prefix(self, last_block.height, None, true) - { - let key = Key::parse(key).unwrap(); - let diff_new_key = diff_new_key_prefix.join(&key); - if self.read_subspace_val(&diff_new_key)?.is_none() { - // If there is no new value, it has been deleted in this - // block and we have to restore it - batch_guard.put_cf(subspace_cf, key.to_string(), val) - } + let key = Key::parse(&key_str).unwrap(); + let diff_new_key = diff_new_key_prefix.join(&key); + if self.read_subspace_val(&diff_new_key)?.is_none() { + // If there is no new value, it has been deleted in this + // block and we have to restore it + batch.put_cf(subspace_cf, key_str, val) + } + } + + // Look for non-persisted diffs for rollback + let rollback_cf = self.get_column_family(ROLLBACK_CF)?; + // Iterate the old keys first and keep a set of keys that have old val + let mut keys_with_old_value = HashSet::::new(); + for (key_str, val, _) in + iter_diffs_prefix(self, rollback_cf, last_block.height, None, true) + { + // If there is no new value, it has been deleted in this + // block and we have to restore it + keys_with_old_value.insert(key_str.clone()); + batch.put_cf(subspace_cf, key_str, val) + } + // Then the new keys + for (key_str, _val, _) in + iter_diffs_prefix(self, rollback_cf, last_block.height, None, false) + { + if !keys_with_old_value.contains(&key_str) { + // If there was no old value it means that the key was newly + // written in the last block and we have to delete it + batch.delete_cf(subspace_cf, key_str) } } tracing::info!("Deleting keys prepended with the last height"); - let mut batch = batch.into_inner().unwrap(); let prefix = last_block.height.to_string(); let mut delete_keys = |cf: &ColumnFamily| { let read_opts = make_iter_read_opts(Some(prefix.clone())); @@ -1765,7 +1749,10 @@ impl<'iter> DBIter<'iter> for RocksDB { height: BlockHeight, prefix: Option<&'iter Key>, ) -> PersistentPrefixIterator<'iter> { - iter_diffs_prefix(self, height, prefix, true) + let diffs_cf = self + .get_column_family(DIFFS_CF) + .expect("{DIFFS_CF} column family should exist"); + iter_diffs_prefix(self, diffs_cf, height, prefix, true) } fn iter_new_diffs( @@ -1773,7 +1760,10 @@ impl<'iter> DBIter<'iter> for RocksDB { height: BlockHeight, prefix: Option<&'iter Key>, ) -> PersistentPrefixIterator<'iter> { - iter_diffs_prefix(self, height, prefix, false) + let diffs_cf = self + .get_column_family(DIFFS_CF) + .expect("{DIFFS_CF} column family should exist"); + iter_diffs_prefix(self, diffs_cf, height, prefix, false) } fn iter_replay_protection(&'iter self) -> Self::PrefixIter { @@ -1820,13 +1810,11 @@ fn iter_subspace_pattern<'iter>( fn iter_diffs_prefix<'a>( db: &'a RocksDB, + cf: &'a ColumnFamily, height: BlockHeight, prefix: Option<&Key>, is_old: bool, ) -> PersistentPrefixIterator<'a> { - let diffs_cf = db - .get_column_family(DIFFS_CF) - .expect("{DIFFS_CF} column family should exist"); let kind = if is_old { OLD_DIFF_PREFIX } else { @@ -1838,7 +1826,7 @@ fn iter_diffs_prefix<'a>( .unwrap(), ); // get keys without the `stripped_prefix` - iter_prefix(db, diffs_cf, stripped_prefix.as_ref(), prefix) + iter_prefix(db, cf, stripped_prefix.as_ref(), prefix) } /// Create an iterator over key-vals in the given CF matching the given diff --git a/crates/core/src/storage.rs b/crates/core/src/storage.rs index 1e8acc9cee..5284da1039 100644 --- a/crates/core/src/storage.rs +++ b/crates/core/src/storage.rs @@ -95,6 +95,8 @@ pub enum DbColFam { STATE, /// Diffs DIFFS, + /// Diffs for rollback (only kept for 1 block) + ROLLBACK, /// Replay protection REPLAYPROT, } @@ -103,6 +105,8 @@ pub enum DbColFam { pub const SUBSPACE_CF: &str = "subspace"; /// Diffs column family name pub const DIFFS_CF: &str = "diffs"; +/// Diffs for rollback (only kept for 1 block) column family name +pub const ROLLBACK_CF: &str = "rollback"; /// State column family name pub const STATE_CF: &str = "state"; /// Block column family name @@ -118,6 +122,7 @@ impl DbColFam { DbColFam::BLOCK => BLOCK_CF, DbColFam::STATE => STATE_CF, DbColFam::DIFFS => DIFFS_CF, + DbColFam::ROLLBACK => ROLLBACK_CF, DbColFam::REPLAYPROT => REPLAY_PROTECTION_CF, } } @@ -130,6 +135,7 @@ impl FromStr for DbColFam { match s.to_lowercase().as_str() { SUBSPACE_CF => Ok(Self::SUBSPACE), DIFFS_CF => Ok(Self::DIFFS), + ROLLBACK_CF => Ok(Self::ROLLBACK), STATE_CF => Ok(Self::STATE), REPLAY_PROTECTION_CF => Ok(Self::REPLAYPROT), BLOCK_CF => Ok(Self::BLOCK), From a42924482369174c76f0f4cdf2501e8a4ff95d27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 27 Mar 2024 13:57:05 +0000 Subject: [PATCH 3/5] DB: prune non-persisted diffs from prev block on every new block commit --- .../src/lib/node/ledger/storage/rocksdb.rs | 33 +++++++++++++++++++ crates/state/src/wl_state.rs | 4 +++ crates/storage/src/db.rs | 7 ++++ crates/storage/src/mockdb.rs | 15 ++++++++- 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/crates/apps/src/lib/node/ledger/storage/rocksdb.rs b/crates/apps/src/lib/node/ledger/storage/rocksdb.rs index a42dab312f..43da101ec6 100644 --- a/crates/apps/src/lib/node/ledger/storage/rocksdb.rs +++ b/crates/apps/src/lib/node/ledger/storage/rocksdb.rs @@ -1556,6 +1556,39 @@ impl DB for RocksDB { Ok(()) } + fn prune_non_persisted_diffs( + &mut self, + batch: &mut Self::WriteBatch, + height: BlockHeight, + ) -> Result<()> { + let rollback_cf = self.get_column_family(ROLLBACK_CF)?; + + let diff_old_key_prefix = Key { + segments: vec![ + height.to_db_key(), + OLD_DIFF_PREFIX.to_string().to_db_key(), + ], + }; + for (key_str, _val, _) in + iter_prefix(self, rollback_cf, None, Some(&diff_old_key_prefix)) + { + batch.0.delete_cf(rollback_cf, key_str) + } + + let diff_new_key_prefix = Key { + segments: vec![ + height.to_db_key(), + NEW_DIFF_PREFIX.to_string().to_db_key(), + ], + }; + for (key_str, _val, _) in + iter_prefix(self, rollback_cf, None, Some(&diff_new_key_prefix)) + { + batch.0.delete_cf(rollback_cf, key_str) + } + Ok(()) + } + #[inline] fn overwrite_entry( &self, diff --git a/crates/state/src/wl_state.rs b/crates/state/src/wl_state.rs index ec20360bfb..ee48f0927f 100644 --- a/crates/state/src/wl_state.rs +++ b/crates/state/src/wl_state.rs @@ -598,6 +598,10 @@ where // prune old merkle tree stores self.prune_merkle_tree_stores(&mut batch)?; } + // If there's a previous block, prune non-persisted diffs from it + if let Some(height) = self.in_mem.block.height.checked_prev() { + self.db.prune_non_persisted_diffs(&mut batch, height)?; + } self.db.exec_batch(batch)?; Ok(()) } diff --git a/crates/storage/src/db.rs b/crates/storage/src/db.rs index ac0d0a32a9..3574bd36c0 100644 --- a/crates/storage/src/db.rs +++ b/crates/storage/src/db.rs @@ -268,6 +268,13 @@ pub trait DB: Debug { batch: &mut Self::WriteBatch, ) -> Result<()>; + /// Prune non-persisted diffs that are only kept for one block for rollback + fn prune_non_persisted_diffs( + &mut self, + batch: &mut Self::WriteBatch, + height: BlockHeight, + ) -> Result<()>; + /// Overwrite a new value in storage, taking into /// account values stored at a previous height fn overwrite_entry( diff --git a/crates/storage/src/mockdb.rs b/crates/storage/src/mockdb.rs index 90cf1bccac..7f64eda47b 100644 --- a/crates/storage/src/mockdb.rs +++ b/crates/storage/src/mockdb.rs @@ -526,7 +526,8 @@ impl DB for MockDB { let diff_prefix = Key::from(height.to_db_key()); let mut db = self.0.borrow_mut(); - // Diffs + // Diffs - Note that this is different from RocksDB that has a separate + // CF for non-persisted diffs (ROLLBACK_CF) let size_diff = match db.insert(subspace_key.to_string(), value.to_owned()) { Some(prev_value) => { @@ -585,6 +586,8 @@ impl DB for MockDB { let diff_prefix = Key::from(height.to_db_key()); let mut db = self.0.borrow_mut(); + // Diffs - Note that this is different from RocksDB that has a separate + // CF for non-persisted diffs (ROLLBACK_CF) let size_diff = match db.remove(&subspace_key.to_string()) { Some(value) => { let old_key = diff_prefix @@ -691,6 +694,16 @@ impl DB for MockDB { Ok(()) } + fn prune_non_persisted_diffs( + &mut self, + _batch: &mut Self::WriteBatch, + _height: BlockHeight, + ) -> Result<()> { + // No-op - Note that this is different from RocksDB that has a separate + // CF for non-persisted diffs (ROLLBACK_CF) + Ok(()) + } + fn overwrite_entry( &self, _batch: &mut Self::WriteBatch, From f1d7471584671e65313a44002be8e6deb6e6c248 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Tue, 2 Apr 2024 14:49:27 +0100 Subject: [PATCH 4/5] test/DB: update rocksdb diffs and rollback tests --- .../apps/src/lib/node/ledger/storage/mod.rs | 19 +- .../src/lib/node/ledger/storage/rocksdb.rs | 355 ++++++++++-------- 2 files changed, 209 insertions(+), 165 deletions(-) diff --git a/crates/apps/src/lib/node/ledger/storage/mod.rs b/crates/apps/src/lib/node/ledger/storage/mod.rs index 2e1c383ca4..c9e9c2d7a5 100644 --- a/crates/apps/src/lib/node/ledger/storage/mod.rs +++ b/crates/apps/src/lib/node/ledger/storage/mod.rs @@ -779,10 +779,6 @@ mod tests { Key::parse("testing2").unwrap() } - fn merkle_tree_key_filter(key: &Key) -> bool { - key == &test_key_1() - } - #[test] fn test_persistent_storage_writing_without_merklizing_or_diffs() { let db_path = @@ -793,7 +789,8 @@ mod tests { ChainId::default(), address::testing::nam(), None, - merkle_tree_key_filter, + // Only merkelize and persist diffs for `test_key_1` + |key: &Key| -> bool { key == &test_key_1() }, ); // Start the first block let first_height = BlockHeight::first(); @@ -869,12 +866,12 @@ mod tests { // need to have diffs for at least 1 block for rollback purposes let res2 = state .db() - .read_diffs_val(&key2, first_height, true) + .read_rollback_val(&key2, first_height, true) .unwrap(); assert!(res2.is_none()); let res2 = state .db() - .read_diffs_val(&key2, first_height, false) + .read_rollback_val(&key2, first_height, false) .unwrap() .unwrap(); let res2 = u64::try_from_slice(&res2).unwrap(); @@ -932,12 +929,12 @@ mod tests { // Check that key-val-2 diffs don't exist for block 0 anymore let res2 = state .db() - .read_diffs_val(&key2, first_height, true) + .read_rollback_val(&key2, first_height, true) .unwrap(); assert!(res2.is_none()); let res2 = state .db() - .read_diffs_val(&key2, first_height, false) + .read_rollback_val(&key2, first_height, false) .unwrap(); assert!(res2.is_none()); @@ -945,14 +942,14 @@ mod tests { // val2 and no "new" value let res2 = state .db() - .read_diffs_val(&key2, second_height, true) + .read_rollback_val(&key2, second_height, true) .unwrap() .unwrap(); let res2 = u64::try_from_slice(&res2).unwrap(); assert_eq!(res2, val2); let res2 = state .db() - .read_diffs_val(&key2, second_height, false) + .read_rollback_val(&key2, second_height, false) .unwrap(); assert!(res2.is_none()); } diff --git a/crates/apps/src/lib/node/ledger/storage/rocksdb.rs b/crates/apps/src/lib/node/ledger/storage/rocksdb.rs index 43da101ec6..28a15b74fb 100644 --- a/crates/apps/src/lib/node/ledger/storage/rocksdb.rs +++ b/crates/apps/src/lib/node/ledger/storage/rocksdb.rs @@ -636,6 +636,27 @@ impl RocksDB { tracing::info!("Flushing restored state to disk"); self.exec_batch(batch) } + + /// Read diffs of non-persisted key-vals that are only kept for rollback of + /// one block height. + #[cfg(test)] + pub fn read_rollback_val( + &self, + key: &Key, + height: BlockHeight, + is_old: bool, + ) -> Result>> { + let rollback_cf = self.get_column_family(ROLLBACK_CF)?; + let key = if is_old { + old_and_new_diff_key(key, height)?.0 + } else { + old_and_new_diff_key(key, height)?.1 + }; + + self.0 + .get_cf(rollback_cf, key) + .map_err(|e| Error::DBError(e.into_string())) + } } impl DB for RocksDB { @@ -2258,174 +2279,191 @@ mod test { #[test] fn test_rollback() { - let dir = tempdir().unwrap(); - let mut db = open(dir.path(), None).unwrap(); - - // A key that's gonna be added on a second block - let add_key = Key::parse("add").unwrap(); - // A key that's gonna be deleted on a second block - let delete_key = Key::parse("delete").unwrap(); - // A key that's gonna be overwritten on a second block - let overwrite_key = Key::parse("overwrite").unwrap(); - - // Write first block - let mut batch = RocksDB::batch(); - let height_0 = BlockHeight(100); - let mut pred_epochs = Epochs::default(); - pred_epochs.new_epoch(height_0); - let conversion_state_0 = ConversionState::default(); - let to_delete_val = vec![1_u8, 1, 0, 0]; - let to_overwrite_val = vec![1_u8, 1, 1, 0]; - db.batch_write_subspace_val( - &mut batch, - height_0, - &delete_key, - &to_delete_val, - true, - ) - .unwrap(); - db.batch_write_subspace_val( - &mut batch, - height_0, - &overwrite_key, - &to_overwrite_val, - true, - ) - .unwrap(); - for tx in [b"tx1", b"tx2"] { - db.write_replay_protection_entry( + for persist_diffs in [true, false] { + println!("Running with persist_diffs: {persist_diffs}"); + + let dir = tempdir().unwrap(); + let mut db = open(dir.path(), None).unwrap(); + + // A key that's gonna be added on a second block + let add_key = Key::parse("add").unwrap(); + // A key that's gonna be deleted on a second block + let delete_key = Key::parse("delete").unwrap(); + // A key that's gonna be overwritten on a second block + let overwrite_key = Key::parse("overwrite").unwrap(); + + // Write first block + let mut batch = RocksDB::batch(); + let height_0 = BlockHeight(100); + let mut pred_epochs = Epochs::default(); + pred_epochs.new_epoch(height_0); + let conversion_state_0 = ConversionState::default(); + let to_delete_val = vec![1_u8, 1, 0, 0]; + let to_overwrite_val = vec![1_u8, 1, 1, 0]; + db.batch_write_subspace_val( &mut batch, - &replay_protection::all_key(&Hash::sha256(tx)), + height_0, + &delete_key, + &to_delete_val, + persist_diffs, ) .unwrap(); - db.write_replay_protection_entry( + db.batch_write_subspace_val( &mut batch, - &replay_protection::buffer_key(&Hash::sha256(tx)), + height_0, + &overwrite_key, + &to_overwrite_val, + persist_diffs, ) .unwrap(); - } + for tx in [b"tx1", b"tx2"] { + db.write_replay_protection_entry( + &mut batch, + &replay_protection::all_key(&Hash::sha256(tx)), + ) + .unwrap(); + db.write_replay_protection_entry( + &mut batch, + &replay_protection::buffer_key(&Hash::sha256(tx)), + ) + .unwrap(); + } - for tx in [b"tx3", b"tx4"] { - db.write_replay_protection_entry( + for tx in [b"tx3", b"tx4"] { + db.write_replay_protection_entry( + &mut batch, + &replay_protection::last_key(&Hash::sha256(tx)), + ) + .unwrap(); + } + + add_block_to_batch( + &db, &mut batch, - &replay_protection::last_key(&Hash::sha256(tx)), + height_0, + Epoch(1), + pred_epochs.clone(), + &conversion_state_0, ) .unwrap(); - } - - add_block_to_batch( - &db, - &mut batch, - height_0, - Epoch(1), - pred_epochs.clone(), - &conversion_state_0, - ) - .unwrap(); - db.exec_batch(batch.0).unwrap(); - - // Write second block - let mut batch = RocksDB::batch(); - let height_1 = BlockHeight(101); - pred_epochs.new_epoch(height_1); - let conversion_state_1 = ConversionState::default(); - let add_val = vec![1_u8, 0, 0, 0]; - let overwrite_val = vec![1_u8, 1, 1, 1]; - db.batch_write_subspace_val( - &mut batch, height_1, &add_key, &add_val, true, - ) - .unwrap(); - db.batch_write_subspace_val( - &mut batch, - height_1, - &overwrite_key, - &overwrite_val, - true, - ) - .unwrap(); - db.batch_delete_subspace_val(&mut batch, height_1, &delete_key, true) + db.exec_batch(batch.0).unwrap(); + + // Write second block + let mut batch = RocksDB::batch(); + let height_1 = BlockHeight(101); + pred_epochs.new_epoch(height_1); + let conversion_state_1 = ConversionState::default(); + let add_val = vec![1_u8, 0, 0, 0]; + let overwrite_val = vec![1_u8, 1, 1, 1]; + db.batch_write_subspace_val( + &mut batch, + height_1, + &add_key, + &add_val, + persist_diffs, + ) .unwrap(); - - db.prune_replay_protection_buffer(&mut batch).unwrap(); - db.write_replay_protection_entry( - &mut batch, - &replay_protection::all_key(&Hash::sha256(b"tx3")), - ) - .unwrap(); - - for tx in [b"tx3", b"tx4"] { - db.delete_replay_protection_entry( + db.batch_write_subspace_val( &mut batch, - &replay_protection::last_key(&Hash::sha256(tx)), + height_1, + &overwrite_key, + &overwrite_val, + persist_diffs, ) .unwrap(); - db.write_replay_protection_entry( + db.batch_delete_subspace_val( &mut batch, - &replay_protection::buffer_key(&Hash::sha256(tx)), + height_1, + &delete_key, + persist_diffs, ) .unwrap(); - } - for tx in [b"tx5", b"tx6"] { + db.prune_replay_protection_buffer(&mut batch).unwrap(); db.write_replay_protection_entry( &mut batch, - &replay_protection::last_key(&Hash::sha256(tx)), + &replay_protection::all_key(&Hash::sha256(b"tx3")), ) .unwrap(); - } - - add_block_to_batch( - &db, - &mut batch, - height_1, - Epoch(2), - pred_epochs, - &conversion_state_1, - ) - .unwrap(); - db.exec_batch(batch.0).unwrap(); - - // Check that the values are as expected from second block - let added = db.read_subspace_val(&add_key).unwrap(); - assert_eq!(added, Some(add_val)); - let overwritten = db.read_subspace_val(&overwrite_key).unwrap(); - assert_eq!(overwritten, Some(overwrite_val)); - let deleted = db.read_subspace_val(&delete_key).unwrap(); - assert_eq!(deleted, None); - for tx in [b"tx1", b"tx2", b"tx3", b"tx5", b"tx6"] { - assert!(db.has_replay_protection_entry(&Hash::sha256(tx)).unwrap()); - } - assert!( - !db.has_replay_protection_entry(&Hash::sha256(b"tx4")) - .unwrap() - ); + for tx in [b"tx3", b"tx4"] { + db.delete_replay_protection_entry( + &mut batch, + &replay_protection::last_key(&Hash::sha256(tx)), + ) + .unwrap(); + db.write_replay_protection_entry( + &mut batch, + &replay_protection::buffer_key(&Hash::sha256(tx)), + ) + .unwrap(); + } - // Rollback to the first block height - db.rollback(height_0).unwrap(); - - // Check that the values are back to the state at the first block - let added = db.read_subspace_val(&add_key).unwrap(); - assert_eq!(added, None); - let overwritten = db.read_subspace_val(&overwrite_key).unwrap(); - assert_eq!(overwritten, Some(to_overwrite_val)); - let deleted = db.read_subspace_val(&delete_key).unwrap(); - assert_eq!(deleted, Some(to_delete_val)); - // Check the conversion state - let state_cf = db.get_column_family(STATE_CF).unwrap(); - let conversion_state = - db.0.get_cf(state_cf, "conversion_state".as_bytes()) - .unwrap() + for tx in [b"tx5", b"tx6"] { + db.write_replay_protection_entry( + &mut batch, + &replay_protection::last_key(&Hash::sha256(tx)), + ) .unwrap(); - assert_eq!(conversion_state, encode(&conversion_state_0)); - for tx in [b"tx1", b"tx2", b"tx3", b"tx4"] { - assert!(db.has_replay_protection_entry(&Hash::sha256(tx)).unwrap()); - } + } - for tx in [b"tx5", b"tx6"] { + add_block_to_batch( + &db, + &mut batch, + height_1, + Epoch(2), + pred_epochs, + &conversion_state_1, + ) + .unwrap(); + db.exec_batch(batch.0).unwrap(); + + // Check that the values are as expected from second block + let added = db.read_subspace_val(&add_key).unwrap(); + assert_eq!(added, Some(add_val)); + let overwritten = db.read_subspace_val(&overwrite_key).unwrap(); + assert_eq!(overwritten, Some(overwrite_val)); + let deleted = db.read_subspace_val(&delete_key).unwrap(); + assert_eq!(deleted, None); + + for tx in [b"tx1", b"tx2", b"tx3", b"tx5", b"tx6"] { + assert!( + db.has_replay_protection_entry(&Hash::sha256(tx)).unwrap() + ); + } assert!( - !db.has_replay_protection_entry(&Hash::sha256(tx)).unwrap() + !db.has_replay_protection_entry(&Hash::sha256(b"tx4")) + .unwrap() ); + + // Rollback to the first block height + db.rollback(height_0).unwrap(); + + // Check that the values are back to the state at the first block + let added = db.read_subspace_val(&add_key).unwrap(); + assert_eq!(added, None); + let overwritten = db.read_subspace_val(&overwrite_key).unwrap(); + assert_eq!(overwritten, Some(to_overwrite_val)); + let deleted = db.read_subspace_val(&delete_key).unwrap(); + assert_eq!(deleted, Some(to_delete_val)); + // Check the conversion state + let state_cf = db.get_column_family(STATE_CF).unwrap(); + let conversion_state = + db.0.get_cf(state_cf, "conversion_state".as_bytes()) + .unwrap() + .unwrap(); + assert_eq!(conversion_state, encode(&conversion_state_0)); + for tx in [b"tx1", b"tx2", b"tx3", b"tx4"] { + assert!( + db.has_replay_protection_entry(&Hash::sha256(tx)).unwrap() + ); + } + + for tx in [b"tx5", b"tx6"] { + assert!( + !db.has_replay_protection_entry(&Hash::sha256(tx)).unwrap() + ); + } } } @@ -2463,18 +2501,21 @@ mod test { { let diffs_cf = db.get_column_family(DIFFS_CF).unwrap(); + let rollback_cf = db.get_column_family(ROLLBACK_CF).unwrap(); - // Diffs new key for `key_with_diffs` at height_0 must be present + // Diffs new key for `key_with_diffs` at height_0 must be + // present let (old_with_h0, new_with_h0) = old_and_new_diff_key(&key_with_diffs, height_0).unwrap(); assert!(db.0.get_cf(diffs_cf, old_with_h0).unwrap().is_none()); assert!(db.0.get_cf(diffs_cf, new_with_h0).unwrap().is_some()); - // Diffs new key for `key_without_diffs` at height_0 must be present + // Diffs new key for `key_without_diffs` at height_0 must be + // present let (old_wo_h0, new_wo_h0) = old_and_new_diff_key(&key_without_diffs, height_0).unwrap(); - assert!(db.0.get_cf(diffs_cf, old_wo_h0).unwrap().is_none()); - assert!(db.0.get_cf(diffs_cf, new_wo_h0).unwrap().is_some()); + assert!(db.0.get_cf(rollback_cf, old_wo_h0).unwrap().is_none()); + assert!(db.0.get_cf(rollback_cf, new_wo_h0).unwrap().is_some()); } // Write second block @@ -2496,10 +2537,12 @@ mod test { false, ) .unwrap(); + db.prune_non_persisted_diffs(&mut batch, height_0).unwrap(); db.exec_batch(batch.0).unwrap(); { let diffs_cf = db.get_column_family(DIFFS_CF).unwrap(); + let rollback_cf = db.get_column_family(ROLLBACK_CF).unwrap(); // Diffs keys for `key_with_diffs` at height_0 must be present let (old_with_h0, new_with_h0) = @@ -2510,8 +2553,8 @@ mod test { // Diffs keys for `key_without_diffs` at height_0 must be gone let (old_wo_h0, new_wo_h0) = old_and_new_diff_key(&key_without_diffs, height_0).unwrap(); - assert!(db.0.get_cf(diffs_cf, old_wo_h0).unwrap().is_none()); - assert!(db.0.get_cf(diffs_cf, new_wo_h0).unwrap().is_none()); + assert!(db.0.get_cf(rollback_cf, old_wo_h0).unwrap().is_none()); + assert!(db.0.get_cf(rollback_cf, new_wo_h0).unwrap().is_none()); // Diffs keys for `key_with_diffs` at height_1 must be present let (old_with_h1, new_with_h1) = @@ -2519,11 +2562,12 @@ mod test { assert!(db.0.get_cf(diffs_cf, old_with_h1).unwrap().is_some()); assert!(db.0.get_cf(diffs_cf, new_with_h1).unwrap().is_some()); - // Diffs keys for `key_without_diffs` at height_1 must be present + // Diffs keys for `key_without_diffs` at height_1 must be + // present let (old_wo_h1, new_wo_h1) = old_and_new_diff_key(&key_without_diffs, height_1).unwrap(); - assert!(db.0.get_cf(diffs_cf, old_wo_h1).unwrap().is_some()); - assert!(db.0.get_cf(diffs_cf, new_wo_h1).unwrap().is_some()); + assert!(db.0.get_cf(rollback_cf, old_wo_h1).unwrap().is_some()); + assert!(db.0.get_cf(rollback_cf, new_wo_h1).unwrap().is_some()); } // Write third block @@ -2545,10 +2589,12 @@ mod test { false, ) .unwrap(); + db.prune_non_persisted_diffs(&mut batch, height_1).unwrap(); db.exec_batch(batch.0).unwrap(); { let diffs_cf = db.get_column_family(DIFFS_CF).unwrap(); + let rollback_cf = db.get_column_family(ROLLBACK_CF).unwrap(); // Diffs keys for `key_with_diffs` at height_1 must be present let (old_with_h1, new_with_h1) = @@ -2559,8 +2605,8 @@ mod test { // Diffs keys for `key_without_diffs` at height_1 must be gone let (old_wo_h1, new_wo_h1) = old_and_new_diff_key(&key_without_diffs, height_1).unwrap(); - assert!(db.0.get_cf(diffs_cf, old_wo_h1).unwrap().is_none()); - assert!(db.0.get_cf(diffs_cf, new_wo_h1).unwrap().is_none()); + assert!(db.0.get_cf(rollback_cf, old_wo_h1).unwrap().is_none()); + assert!(db.0.get_cf(rollback_cf, new_wo_h1).unwrap().is_none()); // Diffs keys for `key_with_diffs` at height_2 must be present let (old_with_h2, new_with_h2) = @@ -2568,11 +2614,12 @@ mod test { assert!(db.0.get_cf(diffs_cf, old_with_h2).unwrap().is_some()); assert!(db.0.get_cf(diffs_cf, new_with_h2).unwrap().is_some()); - // Diffs keys for `key_without_diffs` at height_2 must be present + // Diffs keys for `key_without_diffs` at height_2 must be + // present let (old_wo_h2, new_wo_h2) = old_and_new_diff_key(&key_without_diffs, height_2).unwrap(); - assert!(db.0.get_cf(diffs_cf, old_wo_h2).unwrap().is_some()); - assert!(db.0.get_cf(diffs_cf, new_wo_h2).unwrap().is_some()); + assert!(db.0.get_cf(rollback_cf, old_wo_h2).unwrap().is_some()); + assert!(db.0.get_cf(rollback_cf, new_wo_h2).unwrap().is_some()); } } From 58883a3109f83c0fee3cc3414dcc3260b9f22e5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 3 Apr 2024 11:10:39 +0100 Subject: [PATCH 5/5] changelog: add #2964 --- .../unreleased/bug-fixes/2964-fix-non-persisted-diffs.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/2964-fix-non-persisted-diffs.md diff --git a/.changelog/unreleased/bug-fixes/2964-fix-non-persisted-diffs.md b/.changelog/unreleased/bug-fixes/2964-fix-non-persisted-diffs.md new file mode 100644 index 0000000000..a8bb541d41 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/2964-fix-non-persisted-diffs.md @@ -0,0 +1,4 @@ +- Replaced DB key-val diffs pruning of non-persisted keys that searched for the + last diffs and was degrading throughput with a separate DB column family that + is pruned on every block. + ([\#2964](https://github.com/anoma/namada/pull/2964)) \ No newline at end of file