From cd441a2aaf15f54718f942d02b877f5915f87220 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 14:44:59 +0200 Subject: [PATCH] remove is_merklized_storage_key --- crates/node/src/shell/mod.rs | 8 --- crates/node/src/storage/mod.rs | 17 +++--- crates/state/src/lib.rs | 95 ++-------------------------------- crates/state/src/wl_state.rs | 87 ++++++++++++------------------- 4 files changed, 44 insertions(+), 163 deletions(-) diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index ded2c27932..610f43d0c6 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -353,13 +353,6 @@ where event_log: EventLog, } -/// Merkle tree storage key filter. Return `false` for keys that shouldn't be -/// merklized. -pub fn is_merklized_storage_key(_key: &namada_sdk::storage::Key) -> bool { - // We need to merklize all keys for now - true -} - /// Storage key filter to store the diffs into the storage. Return `false` for /// keys whose diffs shouldn't be stored. pub fn is_key_diff_storable(key: &namada_sdk::storage::Key) -> bool { @@ -448,7 +441,6 @@ where chain_id.clone(), native_token, config.shell.storage_read_past_height_limit, - is_merklized_storage_key, is_key_diff_storable, ); let vp_wasm_cache_dir = diff --git a/crates/node/src/storage/mod.rs b/crates/node/src/storage/mod.rs index 2b57c06c6b..4e400350a9 100644 --- a/crates/node/src/storage/mod.rs +++ b/crates/node/src/storage/mod.rs @@ -81,7 +81,7 @@ mod tests { use tempfile::TempDir; use super::*; - use crate::shell::{is_key_diff_storable, is_merklized_storage_key}; + use crate::shell::is_key_diff_storable; #[test] fn test_crud_value() { @@ -93,7 +93,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); let key = Key::parse("key").expect("cannot parse the key string"); @@ -146,7 +145,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); state @@ -209,7 +207,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); let (loaded_root, height) = @@ -231,7 +228,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); @@ -276,7 +272,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); state @@ -349,7 +344,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); @@ -444,9 +438,10 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); + // Prepare written keys for non-probable data, probable data (IBC), and + // no diffed data let make_key = |suffix: u64| { match suffix % 3u64 { 0 => Key::parse(format!("key{suffix}")).unwrap(), @@ -529,6 +524,7 @@ mod tests { } } } + // save the last root roots.insert(state.in_mem().block.height, state.in_mem().merkle_root()); state.commit_block_from_batch(batch)?; @@ -543,6 +539,7 @@ mod tests { continue; } let tree = state.get_merkle_tree(height, Some(StoreType::Ibc))?; + // Check if the rebuilt tree's root is the same as the saved one assert_eq!(tree.root().0, roots.get(&height).unwrap().0); match write_type { 0 => { @@ -578,6 +575,7 @@ mod tests { .join(&key); let tree = state.get_merkle_tree(height, Some(StoreType::NoDiff))?; + // Check if the rebuilt tree's root is the same as the saved one assert_eq!(tree.root().0, roots.get(&height).unwrap().0); match write_type { 0 => { @@ -612,7 +610,6 @@ mod tests { ChainId::default(), address::testing::nam(), Some(5), - is_merklized_storage_key, is_key_diff_storable, ); let new_epoch_start = BlockHeight(1); @@ -735,7 +732,6 @@ mod tests { ChainId::default(), address::testing::nam(), None, - is_merklized_storage_key, is_key_diff_storable, ); @@ -845,7 +841,6 @@ mod tests { None, // Only merkelize and persist diffs for `test_key_1` |key: &Key| -> bool { key == &test_key_1() }, - |key: &Key| -> bool { key == &test_key_1() }, ); // Start the first block let first_height = BlockHeight::first(); diff --git a/crates/state/src/lib.rs b/crates/state/src/lib.rs index 850749eb53..b7ff50f837 100644 --- a/crates/state/src/lib.rs +++ b/crates/state/src/lib.rs @@ -612,16 +612,11 @@ pub mod testing { write_log: Default::default(), db: MockDB::default(), in_mem: Default::default(), - merkle_tree_key_filter: merklize_all_keys, diff_key_filter: diff_all_keys, }) } } - fn merklize_all_keys(_key: &storage::Key) -> bool { - true - } - fn diff_all_keys(_key: &storage::Key) -> bool { true } @@ -901,32 +896,21 @@ mod tests { Key::parse("testing2").unwrap() } - fn test_key_3() -> Key { - Key::parse("testing3").unwrap() - } - - fn merkle_tree_key_filter(key: &Key) -> bool { - key == &test_key_1() || key == &test_key_2() - } - fn diff_key_filter(key: &Key) -> bool { key == &test_key_1() } #[test] - fn test_writing_without_merklizing_or_diffs() { + fn test_writing_without_diffs() { let mut state = TestState::default(); assert_eq!(state.in_mem().block.height.0, 0); - (state.0.merkle_tree_key_filter) = merkle_tree_key_filter; (state.0.diff_key_filter) = diff_key_filter; let key1 = test_key_1(); let val1 = 1u64; let key2 = test_key_2(); let val2 = 2u64; - let key3 = test_key_3(); - let val3 = 3u64; // Standard write of key-val-1 state.write(&key1, val1).unwrap(); @@ -947,13 +931,6 @@ mod tests { let res = state.read::(&key2).unwrap().unwrap(); assert_eq!(res, val2); - // Write key-val-3 without merklizing or diffs - state.write(&key3, val3).unwrap(); - - // Read from state should return val3 - let res = state.read::(&key3).unwrap().unwrap(); - assert_eq!(res, val3); - // Commit block and storage changes state.commit_block().unwrap(); state.in_mem_mut().block.height = @@ -985,23 +962,6 @@ mod tests { state.in_mem().block.tree.has_key(&no_diff_key2).unwrap(); assert!(is_merklized2); - // Key3 should be in storage. Confirm by reading from - // state and also by reading DB subspace directly - let res3 = state.read::(&key3).unwrap().unwrap(); - assert_eq!(res3, val3); - let res3 = state.db().read_subspace_val(&key3).unwrap().unwrap(); - let res3 = u64::try_from_slice(&res3).unwrap(); - assert_eq!(res3, val3); - - // Check explicitly that key-val-3 is not in merkle tree - let is_merklized3 = state.in_mem().block.tree.has_key(&key3).unwrap(); - assert!(!is_merklized3); - let no_diff_key3 = - Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()).join(&key3); - let is_merklized3 = - state.in_mem().block.tree.has_key(&no_diff_key3).unwrap(); - assert!(!is_merklized3); - // Check that the proper diffs exist for key-val-1 let res1 = state .db() @@ -1032,25 +992,9 @@ mod tests { let res2 = u64::try_from_slice(&res2).unwrap(); assert_eq!(res2, val2); - // Check that there are diffs for key-val-3 in block 0, since all keys - // need to have diffs for at least 1 block for rollback purposes - let res3 = state - .db() - .read_diffs_val(&key3, BlockHeight(0), true) - .unwrap(); - assert!(res3.is_none()); - let res3 = state - .db() - .read_diffs_val(&key3, BlockHeight(0), false) - .unwrap() - .unwrap(); - let res3 = u64::try_from_slice(&res3).unwrap(); - assert_eq!(res3, val3); - // Now delete the keys properly state.delete(&key1).unwrap(); state.delete(&key2).unwrap(); - state.delete(&key3).unwrap(); // Commit the block again state.commit_block().unwrap(); @@ -1060,20 +1004,16 @@ mod tests { // Check the key-vals are removed from the storage subspace let res1 = state.read::(&key1).unwrap(); let res2 = state.read::(&key2).unwrap(); - let res3 = state.read::(&key3).unwrap(); - assert!(res1.is_none() && res2.is_none() && res3.is_none()); + assert!(res1.is_none() && res2.is_none()); let res1 = state.db().read_subspace_val(&key1).unwrap(); let res2 = state.db().read_subspace_val(&key2).unwrap(); - let res3 = state.db().read_subspace_val(&key3).unwrap(); - assert!(res1.is_none() && res2.is_none() && res3.is_none()); + assert!(res1.is_none() && res2.is_none()); // Check that the key-vals don't exist in the merkle tree anymore let is_merklized1 = state.in_mem().block.tree.has_key(&key1).unwrap(); let is_merklized2 = state.in_mem().block.tree.has_key(&no_diff_key2).unwrap(); - let is_merklized3 = - state.in_mem().block.tree.has_key(&no_diff_key3).unwrap(); - assert!(!is_merklized1 && !is_merklized2 && !is_merklized3); + assert!(!is_merklized1 && !is_merklized2); // Check that key-val-1 diffs are properly updated for blocks 0 and 1 let res1 = state @@ -1116,18 +1056,6 @@ mod tests { .unwrap(); assert!(res2.is_none()); - // Check that key-val-3 diffs don't exist for block 0 anymore - let res3 = state - .db() - .read_diffs_val(&key3, BlockHeight(0), true) - .unwrap(); - assert!(res3.is_none()); - let res3 = state - .db() - .read_diffs_val(&key3, BlockHeight(0), false) - .unwrap(); - assert!(res3.is_none()); - // Check that the block 1 diffs for key-val-2 include an "old" value of // val2 and no "new" value let res2 = state @@ -1142,21 +1070,6 @@ mod tests { .read_diffs_val(&key2, BlockHeight(1), false) .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 res3 = state - .db() - .read_diffs_val(&key3, BlockHeight(1), true) - .unwrap() - .unwrap(); - let res3 = u64::try_from_slice(&res3).unwrap(); - assert_eq!(res3, val3); - let res3 = state - .db() - .read_diffs_val(&key3, BlockHeight(1), false) - .unwrap(); - assert!(res3.is_none()); } proptest! { diff --git a/crates/state/src/wl_state.rs b/crates/state/src/wl_state.rs index e698894237..89e0d84c55 100644 --- a/crates/state/src/wl_state.rs +++ b/crates/state/src/wl_state.rs @@ -45,8 +45,6 @@ where pub(crate) db: D, /// State in memory pub(crate) in_mem: InMemory, - /// Static merkle tree storage key filter - pub merkle_tree_key_filter: fn(&storage::Key) -> bool, /// Static diff storage key filter pub diff_key_filter: fn(&storage::Key) -> bool, } @@ -104,7 +102,6 @@ where chain_id: ChainId, native_token: Address, storage_read_past_height_limit: Option, - merkle_tree_key_filter: fn(&storage::Key) -> bool, diff_key_filter: fn(&storage::Key) -> bool, ) -> Self { let write_log = WriteLog::default(); @@ -118,7 +115,6 @@ where write_log, db, in_mem, - merkle_tree_key_filter, diff_key_filter, }); state.load_last_state(); @@ -268,7 +264,6 @@ where value: impl AsRef<[u8]>, ) -> Result { let value = value.as_ref(); - let is_key_merklized = (self.merkle_tree_key_filter)(key); let persist_diffs = (self.diff_key_filter)(key); if is_pending_transfer_key(key) { @@ -278,11 +273,11 @@ where self.in_mem.block.tree.update(key, height)?; } else { // Update the merkle tree - if is_key_merklized && !persist_diffs { + if !persist_diffs { let prefix = Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()); self.in_mem.block.tree.update(&prefix.join(key), value)?; - } else if is_key_merklized { + } else { self.in_mem.block.tree.update(key, value)?; }; } @@ -303,13 +298,12 @@ where batch: &mut D::WriteBatch, key: &Key, ) -> Result { - let is_key_merklized = (self.merkle_tree_key_filter)(key); let persist_diffs = (self.diff_key_filter)(key); // Update the merkle tree - if is_key_merklized && !persist_diffs { + if !persist_diffs { let prefix = Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()); self.in_mem.block.tree.delete(&prefix.join(key))?; - } else if is_key_merklized { + } else { self.in_mem.block.tree.delete(key)?; } Ok(self.db.batch_delete_subspace_val( @@ -716,7 +710,6 @@ where // but with gas and storage bytes len diff accounting tracing::debug!("storage write key {}", key,); let value = value.as_ref(); - let is_key_merklized = (self.merkle_tree_key_filter)(key); let persist_diffs = (self.diff_key_filter)(key); if is_pending_transfer_key(key) { @@ -726,11 +719,11 @@ where self.in_mem.block.tree.update(key, height)?; } else { // Update the merkle tree - if is_key_merklized && !persist_diffs { + if !persist_diffs { let prefix = Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()); self.in_mem.block.tree.update(&prefix.join(key), value)?; - } else if is_key_merklized { + } else { self.in_mem.block.tree.update(key, value)?; } } @@ -760,13 +753,12 @@ where // but with gas and storage bytes len diff accounting let mut deleted_bytes_len = 0; if self.db_has_key(key)?.0 { - let is_key_merklized = (self.merkle_tree_key_filter)(key); let persist_diffs = (self.diff_key_filter)(key); - if is_key_merklized && !persist_diffs { + if !persist_diffs { let prefix = Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()); self.in_mem.block.tree.delete(&prefix.join(key))?; - } else if is_key_merklized { + } else { self.in_mem.block.tree.delete(key)?; } deleted_bytes_len = self.db.delete_subspace_val( @@ -931,38 +923,32 @@ where match old.0.cmp(&new.0) { Ordering::Equal => { // the value was updated - if (self.merkle_tree_key_filter)(&new_key) { - tree.update( - &new_key, - if is_pending_transfer_key(&new_key) { - target_height.serialize_to_vec() - } else { - new.1.clone() - }, - )?; - } + tree.update( + &new_key, + if is_pending_transfer_key(&new_key) { + target_height.serialize_to_vec() + } else { + new.1.clone() + }, + )?; old_diff = old_diff_iter.next(); new_diff = new_diff_iter.next(); } Ordering::Less => { // the value was deleted - if (self.merkle_tree_key_filter)(&old_key) { - tree.delete(&old_key)?; - } + tree.delete(&old_key)?; old_diff = old_diff_iter.next(); } Ordering::Greater => { // the value was inserted - if (self.merkle_tree_key_filter)(&new_key) { - tree.update( - &new_key, - if is_pending_transfer_key(&new_key) { - target_height.serialize_to_vec() - } else { - new.1.clone() - }, - )?; - } + tree.update( + &new_key, + if is_pending_transfer_key(&new_key) { + target_height.serialize_to_vec() + } else { + new.1.clone() + }, + )?; new_diff = new_diff_iter.next(); } } @@ -971,10 +957,7 @@ where // the value was deleted let key = Key::parse(old.0.clone()) .expect("the key should be parsable"); - - if (self.merkle_tree_key_filter)(&key) { - tree.delete(&key)?; - } + tree.delete(&key)?; old_diff = old_diff_iter.next(); } @@ -983,16 +966,14 @@ where let key = Key::parse(new.0.clone()) .expect("the key should be parsable"); - if (self.merkle_tree_key_filter)(&key) { - tree.update( - &key, - if is_pending_transfer_key(&key) { - target_height.serialize_to_vec() - } else { - new.1.clone() - }, - )?; - } + tree.update( + &key, + if is_pending_transfer_key(&key) { + target_height.serialize_to_vec() + } else { + new.1.clone() + }, + )?; new_diff = new_diff_iter.next(); }