From 71833cd55808bec3b181873d1a5f50c23cf416cf Mon Sep 17 00:00:00 2001 From: brentstone Date: Tue, 16 Jan 2024 15:04:59 -0500 Subject: [PATCH] 1-block diffs for keys without persisting all diffs --- apps/src/lib/node/ledger/storage/mod.rs | 49 ++-- apps/src/lib/node/ledger/storage/rocksdb.rs | 235 ++++++++++++-------- core/src/ledger/storage/mockdb.rs | 133 +++++++---- core/src/ledger/storage/mod.rs | 46 ++-- core/src/ledger/storage/write_log.rs | 1 + 5 files changed, 287 insertions(+), 177 deletions(-) diff --git a/apps/src/lib/node/ledger/storage/mod.rs b/apps/src/lib/node/ledger/storage/mod.rs index c772f80400..a6fd74ac87 100644 --- a/apps/src/lib/node/ledger/storage/mod.rs +++ b/apps/src/lib/node/ledger/storage/mod.rs @@ -820,7 +820,7 @@ mod tests { let res = wls.read::(&key1).unwrap().unwrap(); assert_eq!(res, val1); - // Read from Storage shouldn't return val1 bc the block hasn't been + // Read from Storage shouldn't return val1 because the block hasn't been // committed let (res, _) = wls.storage.read(&key1).unwrap(); assert!(res.is_none()); @@ -836,28 +836,22 @@ mod tests { wls.commit_block().unwrap(); wls.storage.block.height = wls.storage.block.height.next_height(); - // Read key1 from Storage should return val1 (also implies it is - // merklized) + // Read key1 from Storage should return val1 let (res1, _) = wls.storage.read(&key1).unwrap(); let res1 = u64::try_from_slice(&res1.unwrap()).unwrap(); - assert_eq!(res1, 1); + assert_eq!(res1, val1); // Check merkle tree inclusion of key-val-1 explicitly let is_merklized1 = wls.storage.block.tree.has_key(&key1).unwrap(); assert!(is_merklized1); - // Read key2 from Storage - should actually fail because `Storage::read` - // checks that the key is in merkle tree first - let (res2, _) = wls.storage.read(&key2).unwrap(); - assert!(res2.is_none()); - - // But key2 should actually be in storage. Confirm by reading from + // Key2 should be in storage. Confirm by reading from // WlStorage and also by reading Storage subspace directly let res2 = wls.read::(&key2).unwrap().unwrap(); assert_eq!(res2, val2); let res2 = wls.storage.db.read_subspace_val(&key2).unwrap().unwrap(); let res2 = u64::try_from_slice(&res2).unwrap(); - assert_eq!(res2, 2); + assert_eq!(res2, val2); // Check explicitly that key-val-2 is not in merkle tree let is_merklized2 = wls.storage.block.tree.has_key(&key2).unwrap(); @@ -867,60 +861,67 @@ mod tests { let res1 = wls .storage .db - .read_diffs_val(&key1, Default::default(), true) + .read_diffs_val(&key1, BlockHeight(0), true) .unwrap(); assert!(res1.is_none()); let res1 = wls .storage .db - .read_diffs_val(&key1, Default::default(), false) + .read_diffs_val(&key1, BlockHeight(0), false) .unwrap() .unwrap(); let res1 = u64::try_from_slice(&res1).unwrap(); assert_eq!(res1, val1); - // Check that there are no diffs for key-val-2 + // Check that there are diffs for key-val-2 in block 0, since all keys + // need to have diffs for at least 1 block for rollback purposes let res2 = wls .storage .db - .read_diffs_val(&key2, Default::default(), true) + .read_diffs_val(&key2, BlockHeight(0), true) .unwrap(); assert!(res2.is_none()); let res2 = wls .storage .db - .read_diffs_val(&key2, Default::default(), false) + .read_diffs_val(&key2, BlockHeight(0), false) + .unwrap() .unwrap(); - assert!(res2.is_none()); + let res2 = u64::try_from_slice(&res2).unwrap(); + assert_eq!(res2, val2); // Delete the data then commit the block wls.delete(&key1).unwrap(); wls.delete_without_diffs(&key2).unwrap(); wls.commit_block().unwrap(); + wls.storage.block.height = wls.storage.block.height.next_height(); // Check the key-vals are removed from the storage subspace let res1 = wls.read::(&key1).unwrap(); let res2 = wls.read::(&key2).unwrap(); assert!(res1.is_none() && res2.is_none()); + let res1 = wls.storage.db.read_subspace_val(&key1).unwrap(); + let res2 = wls.storage.db.read_subspace_val(&key2).unwrap(); + assert!(res1.is_none() && res2.is_none()); // Check that the key-vals don't exist in the merkle tree anymore let is_merklized1 = wls.storage.block.tree.has_key(&key1).unwrap(); - let is_merklized2 = wls.storage.block.tree.has_key(&key1).unwrap(); + let is_merklized2 = wls.storage.block.tree.has_key(&key2).unwrap(); assert!(!is_merklized1 && !is_merklized2); // Check that key-val-1 diffs are properly updated for blocks 0 and 1 let res1 = wls .storage .db - .read_diffs_val(&key1, Default::default(), true) + .read_diffs_val(&key1, BlockHeight(0), true) .unwrap(); assert!(res1.is_none()); let res1 = wls .storage .db - .read_diffs_val(&key1, Default::default(), false) + .read_diffs_val(&key1, BlockHeight(0), false) .unwrap() .unwrap(); let res1 = u64::try_from_slice(&res1).unwrap(); @@ -942,7 +943,7 @@ mod tests { .unwrap(); assert!(res1.is_none()); - // Check that key-val-2 diffs don't exist + // Check that key-val-2 diffs don't exist for block 0 anymore let res2 = wls .storage .db @@ -956,12 +957,16 @@ mod tests { .unwrap(); assert!(res2.is_none()); + // Check that the block 1 diffs for key-val-2 include an "old" value of + // val2 and no "new" value let res2 = wls .storage .db .read_diffs_val(&key2, BlockHeight(1), true) + .unwrap() .unwrap(); - assert!(res2.is_none()); + let res2 = u64::try_from_slice(&res2).unwrap(); + assert_eq!(res2, val2); let res2 = wls .storage .db diff --git a/apps/src/lib/node/ledger/storage/rocksdb.rs b/apps/src/lib/node/ledger/storage/rocksdb.rs index a900791562..1f2f8180cc 100644 --- a/apps/src/lib/node/ledger/storage/rocksdb.rs +++ b/apps/src/lib/node/ledger/storage/rocksdb.rs @@ -220,6 +220,7 @@ impl RocksDB { key: &Key, old_value: Option<&[u8]>, new_value: Option<&[u8]>, + persist_diffs: bool, ) -> Result<()> { let cf = self.get_column_family(DIFFS_CF)?; let key_prefix = Key::from(height.to_db_key()); @@ -245,6 +246,30 @@ 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 existing diffs from two block + // heights earlier than `height` + if !persist_diffs { + if let Some(pruned_height) = height.0.checked_sub(1) { + let pruned_key_prefix = Key::from(pruned_height.to_db_key()); + let old_val_key = pruned_key_prefix + .push(&OLD_DIFF_PREFIX.to_owned()) + .map_err(Error::KeyError)? + .join(key) + .to_string(); + self.0 + .delete_cf(cf, old_val_key) + .map_err(|e| Error::DBError(e.into_string()))?; + let new_val_key = pruned_key_prefix + .push(&NEW_DIFF_PREFIX.to_owned()) + .map_err(Error::KeyError)? + .join(key) + .to_string(); + self.0 + .delete_cf(cf, new_val_key) + .map_err(|e| Error::DBError(e.into_string()))?; + } + } Ok(()) } @@ -257,6 +282,7 @@ impl RocksDB { key: &Key, old_value: Option<&[u8]>, new_value: Option<&[u8]>, + persist_diffs: bool, ) -> Result<()> { let cf = self.get_column_family(DIFFS_CF)?; let key_prefix = Key::from(height.to_db_key()); @@ -278,6 +304,26 @@ impl RocksDB { .to_string(); batch.0.put_cf(cf, new_val_key, new_value); } + + // If not persisting the diffs, remove the existing diffs from two block + // heights earlier than `height` + if !persist_diffs { + if let Some(pruned_height) = height.0.checked_sub(1) { + let pruned_key_prefix = Key::from(pruned_height.to_db_key()); + let old_val_key = pruned_key_prefix + .push(&OLD_DIFF_PREFIX.to_owned()) + .map_err(Error::KeyError)? + .join(key) + .to_string(); + batch.0.delete_cf(cf, old_val_key); + let new_val_key = pruned_key_prefix + .push(&NEW_DIFF_PREFIX.to_owned()) + .map_err(Error::KeyError)? + .join(key) + .to_string(); + batch.0.delete_cf(cf, new_val_key); + } + } Ok(()) } @@ -1317,29 +1363,34 @@ impl DB for RocksDB { ) -> Result { let subspace_cf = self.get_column_family(SUBSPACE_CF)?; let value = value.as_ref(); - let size_diff = if !action.contains(WriteOpts::WRITE_DIFFS) { - 0 - } else { - match self - .0 - .get_cf(subspace_cf, key.to_string()) - .map_err(|e| Error::DBError(e.into_string()))? - { - Some(prev_value) => { - let size_diff = - value.len() as i64 - prev_value.len() as i64; - self.write_subspace_diff( - height, - key, - Some(&prev_value), - Some(value), - )?; - size_diff - } - None => { - self.write_subspace_diff(height, key, None, Some(value))?; - value.len() as i64 - } + + let persist_diffs = action.contains(WriteOpts::WRITE_DIFFS); + + let size_diff = match self + .0 + .get_cf(subspace_cf, key.to_string()) + .map_err(|e| Error::DBError(e.into_string()))? + { + Some(prev_value) => { + let size_diff = value.len() as i64 - prev_value.len() as i64; + self.write_subspace_diff( + height, + key, + Some(&prev_value), + Some(value), + persist_diffs, + )?; + size_diff + } + None => { + self.write_subspace_diff( + height, + key, + None, + Some(value), + persist_diffs, + )?; + value.len() as i64 } }; @@ -1359,27 +1410,26 @@ impl DB for RocksDB { ) -> Result { let subspace_cf = self.get_column_family(SUBSPACE_CF)?; + let persists_diffs = action.contains(WriteOpts::WRITE_DIFFS); + // Check the length of previous value, if any - let prev_len = if !action.contains(WriteOpts::WRITE_DIFFS) { - 0 - } else { - match self - .0 - .get_cf(subspace_cf, key.to_string()) - .map_err(|e| Error::DBError(e.into_string()))? - { - Some(prev_value) => { - let prev_len = prev_value.len() as i64; - self.write_subspace_diff( - height, - key, - Some(&prev_value), - None, - )?; - prev_len - } - None => 0, + let prev_len = match self + .0 + .get_cf(subspace_cf, key.to_string()) + .map_err(|e| Error::DBError(e.into_string()))? + { + Some(prev_value) => { + let prev_len = prev_value.len() as i64; + self.write_subspace_diff( + height, + key, + Some(&prev_value), + None, + persists_diffs, + )?; + prev_len } + None => 0, }; // Delete the key-val @@ -1409,37 +1459,37 @@ impl DB for RocksDB { let value = value.as_ref(); let subspace_cf = self.get_column_family(SUBSPACE_CF)?; + let persist_diffs = action.contains(WriteOpts::WRITE_DIFFS); + // Diffs - let size_diff = if !action.contains(WriteOpts::WRITE_DIFFS) { - 0 - } else { - match self - .0 - .get_cf(subspace_cf, key.to_string()) - .map_err(|e| Error::DBError(e.into_string()))? - { - Some(old_value) => { - let size_diff = value.len() as i64 - old_value.len() as i64; - // Persist the previous value - self.batch_write_subspace_diff( - batch, - height, - key, - Some(&old_value), - Some(value), - )?; - size_diff - } - None => { - self.batch_write_subspace_diff( - batch, - height, - key, - None, - Some(value), - )?; - value.len() as i64 - } + let size_diff = match self + .0 + .get_cf(subspace_cf, key.to_string()) + .map_err(|e| Error::DBError(e.into_string()))? + { + Some(old_value) => { + let size_diff = value.len() as i64 - old_value.len() as i64; + // Persist the previous value + self.batch_write_subspace_diff( + batch, + height, + key, + Some(&old_value), + Some(value), + persist_diffs, + )?; + size_diff + } + None => { + self.batch_write_subspace_diff( + batch, + height, + key, + None, + Some(value), + persist_diffs, + )?; + value.len() as i64 } }; @@ -1458,29 +1508,28 @@ impl DB for RocksDB { ) -> Result { let subspace_cf = self.get_column_family(SUBSPACE_CF)?; + let persist_diffs = action.contains(WriteOpts::WRITE_DIFFS); + // Check the length of previous value, if any - let prev_len = if !action.contains(WriteOpts::WRITE_DIFFS) { - 0 - } else { - match self - .0 - .get_cf(subspace_cf, key.to_string()) - .map_err(|e| Error::DBError(e.into_string()))? - { - Some(prev_value) => { - let prev_len = prev_value.len() as i64; - // Persist the previous value - self.batch_write_subspace_diff( - batch, - height, - key, - Some(&prev_value), - None, - )?; - prev_len - } - None => 0, + let prev_len = match self + .0 + .get_cf(subspace_cf, key.to_string()) + .map_err(|e| Error::DBError(e.into_string()))? + { + Some(prev_value) => { + let prev_len = prev_value.len() as i64; + // Persist the previous value + self.batch_write_subspace_diff( + batch, + height, + key, + Some(&prev_value), + None, + persist_diffs, + )?; + prev_len } + None => 0, }; // Delete the key-val diff --git a/core/src/ledger/storage/mockdb.rs b/core/src/ledger/storage/mockdb.rs index 64cd2fcc89..9061c8e4d8 100644 --- a/core/src/ledger/storage/mockdb.rs +++ b/core/src/ledger/storage/mockdb.rs @@ -30,6 +30,11 @@ use crate::types::storage::{ }; use crate::types::time::DateTimeUtc; +const SUBSPACE_CF: &str = "subspace"; + +const OLD_DIFF_PREFIX: &str = "old"; +const NEW_DIFF_PREFIX: &str = "new"; + /// An in-memory DB for testing. #[derive(Debug, Default)] pub struct MockDB( @@ -467,7 +472,11 @@ impl DB for MockDB { height: BlockHeight, is_old: bool, ) -> Result>> { - let old_new_seg = if is_old { "old" } else { "new" }; + let old_new_seg = if is_old { + OLD_DIFF_PREFIX + } else { + NEW_DIFF_PREFIX + }; let prefix = Key::from(height.to_db_key()) .push(&old_new_seg.to_string().to_db_key()) @@ -478,7 +487,7 @@ impl DB for MockDB { } fn read_subspace_val(&self, key: &Key) -> Result>> { - let key = Key::parse("subspace").map_err(Error::KeyError)?.join(key); + let key = Key::parse(SUBSPACE_CF).map_err(Error::KeyError)?.join(key); Ok(self.0.borrow().get(&key.to_string()).cloned()) } @@ -554,36 +563,54 @@ impl DB for MockDB { let current_len = value.len() as i64; let diff_prefix = Key::from(height.to_db_key()); let mut db = self.0.borrow_mut(); - if !action.contains(WriteOpts::WRITE_DIFFS) { - db.insert(subspace_key.to_string(), value.to_owned()); - Ok(0) - } else { - Ok( - match db.insert(subspace_key.to_string(), value.to_owned()) { - Some(prev_value) => { - let old_key = diff_prefix - .push(&"old".to_string().to_db_key()) - .unwrap() - .join(key); - db.insert(old_key.to_string(), prev_value.clone()); - let new_key = diff_prefix - .push(&"new".to_string().to_db_key()) - .unwrap() - .join(key); - db.insert(new_key.to_string(), value.to_owned()); - current_len - prev_value.len() as i64 - } - None => { - let new_key = diff_prefix - .push(&"new".to_string().to_db_key()) - .unwrap() - .join(key); - db.insert(new_key.to_string(), value.to_owned()); - current_len - } - }, - ) + + let persist_diffs = action.contains(WriteOpts::WRITE_DIFFS); + + // Diffs + let size_diff = + match db.insert(subspace_key.to_string(), value.to_owned()) { + Some(prev_value) => { + let old_key = diff_prefix + .push(&OLD_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key); + db.insert(old_key.to_string(), prev_value.clone()); + let new_key = diff_prefix + .push(&NEW_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key); + db.insert(new_key.to_string(), value.to_owned()); + current_len - prev_value.len() as i64 + } + None => { + let new_key = diff_prefix + .push(&NEW_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key); + db.insert(new_key.to_string(), value.to_owned()); + current_len + } + }; + + if !persist_diffs { + if let Some(pruned_height) = height.0.checked_sub(1) { + let pruned_key_prefix = Key::from(pruned_height.to_db_key()); + let old_val_key = pruned_key_prefix + .push(&NEW_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key) + .to_string(); + db.remove(&old_val_key); + let new_val_key = pruned_key_prefix + .push(&NEW_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key) + .to_string(); + db.remove(&new_val_key); + } } + + Ok(size_diff) } fn batch_delete_subspace_val( @@ -594,24 +621,44 @@ impl DB for MockDB { action: WriteOpts, ) -> Result { let subspace_key = - Key::parse("subspace").map_err(Error::KeyError)?.join(key); + Key::parse(SUBSPACE_CF).map_err(Error::KeyError)?.join(key); let diff_prefix = Key::from(height.to_db_key()); let mut db = self.0.borrow_mut(); - Ok(match db.remove(&subspace_key.to_string()) { + + let persist_diffs = action.contains(WriteOpts::WRITE_DIFFS); + + let size_diff = match db.remove(&subspace_key.to_string()) { Some(value) => { - if !action.contains(WriteOpts::WRITE_DIFFS) { - 0 - } else { - let old_key = diff_prefix - .push(&"old".to_string().to_db_key()) - .unwrap() - .join(key); - db.insert(old_key.to_string(), value.clone()); - value.len() as i64 + let old_key = diff_prefix + .push(&OLD_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key); + db.insert(old_key.to_string(), value.clone()); + + if !persist_diffs { + if let Some(pruned_height) = height.0.checked_sub(1) { + let pruned_key_prefix = + Key::from(pruned_height.to_db_key()); + let old_val_key = pruned_key_prefix + .push(&NEW_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key) + .to_string(); + db.remove(&old_val_key); + let new_val_key = pruned_key_prefix + .push(&NEW_DIFF_PREFIX.to_string().to_db_key()) + .unwrap() + .join(key) + .to_string(); + db.remove(&new_val_key); + } } + value.len() as i64 } None => 0, - }) + }; + + Ok(size_diff) } fn prune_merkle_tree_store( diff --git a/core/src/ledger/storage/mod.rs b/core/src/ledger/storage/mod.rs index a32219ed20..7a1e3b7a4a 100644 --- a/core/src/ledger/storage/mod.rs +++ b/core/src/ledger/storage/mod.rs @@ -1707,28 +1707,22 @@ mod tests { wls.commit_block().unwrap(); wls.storage.block.height = wls.storage.block.height.next_height(); - // Read key1 from Storage should return val1 (also implies it is - // merklized) + // Read key1 from Storage should return val1 let (res1, _) = wls.storage.read(&key1).unwrap(); let res1 = u64::try_from_slice(&res1.unwrap()).unwrap(); - assert_eq!(res1, 1); + assert_eq!(res1, val1); // Check merkle tree inclusion of key-val-1 explicitly let is_merklized1 = wls.storage.block.tree.has_key(&key1).unwrap(); assert!(is_merklized1); - // Read key2 from Storage - should actually fail because `Storage::read` - // checks that the key is in merkle tree first - let (res2, _) = wls.storage.read(&key2).unwrap(); - assert!(res2.is_none()); - - // But key2 should actually be in storage. Confirm by reading from + // Key2 should be in storage. Confirm by reading from // WlStorage and also by reading Storage subspace directly let res2 = wls.read::(&key2).unwrap().unwrap(); assert_eq!(res2, val2); let res2 = wls.storage.db.read_subspace_val(&key2).unwrap().unwrap(); let res2 = u64::try_from_slice(&res2).unwrap(); - assert_eq!(res2, 2); + assert_eq!(res2, val2); // Check explicitly that key-val-2 is not in merkle tree let is_merklized2 = wls.storage.block.tree.has_key(&key2).unwrap(); @@ -1751,46 +1745,56 @@ mod tests { let res1 = u64::try_from_slice(&res1).unwrap(); assert_eq!(res1, val1); - // Check that there are no diffs for key-val-2 + // Check that there are diffs for key-val-2 in block 0, since all keys + // need to have diffs for at least 1 block for rollback purposes let res2 = wls .storage .db - .read_diffs_val(&key2, Default::default(), true) + .read_diffs_val(&key2, BlockHeight(0), true) .unwrap(); assert!(res2.is_none()); let res2 = wls .storage .db - .read_diffs_val(&key2, Default::default(), false) + .read_diffs_val(&key2, BlockHeight(0), false) + .unwrap() .unwrap(); - assert!(res2.is_none()); + let res2 = u64::try_from_slice(&res2).unwrap(); + assert_eq!(res2, val2); + // Now delete the keys properly wls.delete(&key1).unwrap(); wls.delete_without_diffs(&key2).unwrap(); + + // Commit the block again wls.commit_block().unwrap(); + wls.storage.block.height = wls.storage.block.height.next_height(); // Check the key-vals are removed from the storage subspace let res1 = wls.read::(&key1).unwrap(); let res2 = wls.read::(&key2).unwrap(); assert!(res1.is_none() && res2.is_none()); + let res1 = wls.storage.db.read_subspace_val(&key1).unwrap(); + let res2 = wls.storage.db.read_subspace_val(&key2).unwrap(); + assert!(res1.is_none() && res2.is_none()); // Check that the key-vals don't exist in the merkle tree anymore let is_merklized1 = wls.storage.block.tree.has_key(&key1).unwrap(); - let is_merklized2 = wls.storage.block.tree.has_key(&key1).unwrap(); + let is_merklized2 = wls.storage.block.tree.has_key(&key2).unwrap(); assert!(!is_merklized1 && !is_merklized2); // Check that key-val-1 diffs are properly updated for blocks 0 and 1 let res1 = wls .storage .db - .read_diffs_val(&key1, Default::default(), true) + .read_diffs_val(&key1, BlockHeight(0), true) .unwrap(); assert!(res1.is_none()); let res1 = wls .storage .db - .read_diffs_val(&key1, Default::default(), false) + .read_diffs_val(&key1, BlockHeight(0), false) .unwrap() .unwrap(); let res1 = u64::try_from_slice(&res1).unwrap(); @@ -1812,7 +1816,7 @@ mod tests { .unwrap(); assert!(res1.is_none()); - // Check that key-val-2 diffs don't exist + // Check that key-val-2 diffs don't exist for block 0 anymore let res2 = wls .storage .db @@ -1826,12 +1830,16 @@ mod tests { .unwrap(); assert!(res2.is_none()); + // Check that the block 1 diffs for key-val-2 include an "old" value of + // val2 and no "new" value let res2 = wls .storage .db .read_diffs_val(&key2, BlockHeight(1), true) + .unwrap() .unwrap(); - assert!(res2.is_none()); + let res2 = u64::try_from_slice(&res2).unwrap(); + assert_eq!(res2, val2); let res2 = wls .storage .db diff --git a/core/src/ledger/storage/write_log.rs b/core/src/ledger/storage/write_log.rs index 7d1d7fbf5f..46610b9140 100644 --- a/core/src/ledger/storage/write_log.rs +++ b/core/src/ledger/storage/write_log.rs @@ -579,6 +579,7 @@ impl WriteLog { } } + // Replay protections specifically for (hash, entry) in self.replay_protection.iter() { match entry { ReProtStorageModification::Write => storage