From 011d0f3bcedccc6effe4f5d5c2abfe8187759d1d Mon Sep 17 00:00:00 2001 From: yito88 Date: Tue, 21 May 2024 23:04:21 +0200 Subject: [PATCH 1/8] add diff_key_filter --- crates/node/src/shell/mod.rs | 10 +++- crates/node/src/storage/mod.rs | 12 ++++- crates/state/src/lib.rs | 91 ++++++++++++++++++++++++++++++++-- crates/state/src/wl_state.rs | 34 +++++++++---- crates/storage/src/types.rs | 13 +++++ 5 files changed, 144 insertions(+), 16 deletions(-) diff --git a/crates/node/src/shell/mod.rs b/crates/node/src/shell/mod.rs index b119c47bcd..ded2c27932 100644 --- a/crates/node/src/shell/mod.rs +++ b/crates/node/src/shell/mod.rs @@ -355,7 +355,14 @@ where /// 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 { +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 { !(token::storage_key::is_masp_key(key) && *key != token::storage_key::masp_convert_anchor_key() && *key != token::storage_key::masp_token_map_key() @@ -442,6 +449,7 @@ where native_token, config.shell.storage_read_past_height_limit, is_merklized_storage_key, + is_key_diff_storable, ); let vp_wasm_cache_dir = base_dir.join(chain_id.as_str()).join("vp_wasm_cache"); diff --git a/crates/node/src/storage/mod.rs b/crates/node/src/storage/mod.rs index 610e87b57b..06d4fbd0f2 100644 --- a/crates/node/src/storage/mod.rs +++ b/crates/node/src/storage/mod.rs @@ -79,7 +79,7 @@ mod tests { use tempfile::TempDir; use super::*; - use crate::shell::is_merklized_storage_key; + use crate::shell::{is_key_diff_storable, is_merklized_storage_key}; #[test] fn test_crud_value() { @@ -92,6 +92,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); let key = Key::parse("key").expect("cannot parse the key string"); let value: u64 = 1; @@ -144,6 +145,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); state .in_mem_mut() @@ -206,6 +208,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); let (loaded_root, height) = state.in_mem().get_state().expect("no block exists"); @@ -227,6 +230,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); let mut expected = Vec::new(); @@ -271,6 +275,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); state .in_mem_mut() @@ -343,6 +348,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); // 1. For each `blocks_write_value`, write the current block height if @@ -437,6 +443,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); let num_keys = 5; @@ -559,6 +566,7 @@ mod tests { address::testing::nam(), Some(5), is_merklized_storage_key, + is_key_diff_storable, ); let new_epoch_start = BlockHeight(1); let signed_root_key = bridge_pool::get_signed_root_key(); @@ -681,6 +689,7 @@ mod tests { address::testing::nam(), None, is_merklized_storage_key, + is_key_diff_storable, ); let prefix = storage::Key::parse("prefix").unwrap(); @@ -789,6 +798,7 @@ 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 2f136ca9d2..a5521f63ed 100644 --- a/crates/state/src/lib.rs +++ b/crates/state/src/lib.rs @@ -613,6 +613,7 @@ pub mod testing { db: MockDB::default(), in_mem: Default::default(), merkle_tree_key_filter: merklize_all_keys, + diff_key_filter: diff_all_keys, }) } } @@ -621,6 +622,10 @@ pub mod testing { true } + fn diff_all_keys(_key: &storage::Key) -> bool { + true + } + /// In memory State for testing. pub type InMemoryState = InMemory; @@ -895,7 +900,15 @@ 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() } @@ -905,11 +918,14 @@ mod tests { 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(); @@ -930,6 +946,13 @@ 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 = @@ -952,9 +975,21 @@ mod tests { let res2 = u64::try_from_slice(&res2).unwrap(); assert_eq!(res2, val2); - // Check explicitly that key-val-2 is not in merkle tree + // Check merkle tree inclusion of key-val-2 explicitly let is_merklized2 = state.in_mem().block.tree.has_key(&key2).unwrap(); - assert!(!is_merklized2); + 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); // Check that the proper diffs exist for key-val-1 let res1 = state @@ -986,9 +1021,25 @@ 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(); @@ -998,15 +1049,18 @@ mod tests { // Check the key-vals are removed from the storage subspace let res1 = state.read::(&key1).unwrap(); let res2 = state.read::(&key2).unwrap(); - assert!(res1.is_none() && res2.is_none()); + let res3 = state.read::(&key3).unwrap(); + assert!(res1.is_none() && res2.is_none() && res3.is_none()); let res1 = state.db().read_subspace_val(&key1).unwrap(); let res2 = state.db().read_subspace_val(&key2).unwrap(); - assert!(res1.is_none() && res2.is_none()); + let res3 = state.db().read_subspace_val(&key3).unwrap(); + assert!(res1.is_none() && res2.is_none() && res3.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(&key2).unwrap(); - assert!(!is_merklized1 && !is_merklized2); + let is_merklized3 = state.in_mem().block.tree.has_key(&key3).unwrap(); + assert!(!is_merklized1 && !is_merklized2 && !is_merklized3); // Check that key-val-1 diffs are properly updated for blocks 0 and 1 let res1 = state @@ -1049,6 +1103,18 @@ 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 @@ -1063,6 +1129,21 @@ 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 014e4541f1..dc975617e9 100644 --- a/crates/state/src/wl_state.rs +++ b/crates/state/src/wl_state.rs @@ -46,6 +46,8 @@ where 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, } /// State with a temporary write log. This is used for dry-running txs and ABCI @@ -102,6 +104,7 @@ where 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(); let db = D::open(db_path, cache); @@ -115,6 +118,7 @@ where db, in_mem, merkle_tree_key_filter, + diff_key_filter, }); state.load_last_state(); state @@ -264,6 +268,7 @@ where ) -> 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) { // The tree of the bridge pool stores the current height for the @@ -272,7 +277,9 @@ where self.in_mem.block.tree.update(key, height)?; } else { // Update the merkle tree - if is_key_merklized { + if is_key_merklized && !persist_diffs { + self.in_mem.commit_only_data.add_no_diff_data(key, value); + } else if is_key_merklized { self.in_mem.block.tree.update(key, value)?; } } @@ -281,7 +288,7 @@ where self.in_mem.block.height, key, value, - is_key_merklized, + persist_diffs, )?) } @@ -294,15 +301,18 @@ where 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 { + if is_key_merklized && !persist_diffs { + self.in_mem.commit_only_data.delete_no_diff_data(key); + } else if is_key_merklized { self.in_mem.block.tree.delete(key)?; } Ok(self.db.batch_delete_subspace_val( batch, self.in_mem.block.height, key, - is_key_merklized, + persist_diffs, )?) } @@ -668,7 +678,7 @@ where { self.db_read(key) } else { - if !(self.merkle_tree_key_filter)(key) { + if !(self.diff_key_filter)(key) { return Ok((None, 0)); } @@ -703,6 +713,7 @@ where 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) { // The tree of the bright pool stores the current height for the @@ -711,7 +722,9 @@ where self.in_mem.block.tree.update(key, height)?; } else { // Update the merkle tree - if is_key_merklized { + if is_key_merklized && !persist_diffs { + self.in_mem.commit_only_data.add_no_diff_data(key, value); + } else if is_key_merklized { self.in_mem.block.tree.update(key, value)?; } } @@ -723,7 +736,7 @@ where self.in_mem.block.height, key, value, - is_key_merklized, + persist_diffs, )?; Ok((gas, size_diff)) } @@ -742,13 +755,16 @@ where let mut deleted_bytes_len = 0; if self.db_has_key(key)?.0 { let is_key_merklized = (self.merkle_tree_key_filter)(key); - if is_key_merklized { + let persist_diffs = (self.diff_key_filter)(key); + if is_key_merklized && !persist_diffs { + self.in_mem.commit_only_data.delete_no_diff_data(key); + } else if is_key_merklized { self.in_mem.block.tree.delete(key)?; } deleted_bytes_len = self.db.delete_subspace_val( self.in_mem.block.height, key, - is_key_merklized, + persist_diffs, )?; } let gas = (key.len() + deleted_bytes_len as usize) as u64 diff --git a/crates/storage/src/types.rs b/crates/storage/src/types.rs index e5f323bf09..bf39212a1d 100644 --- a/crates/storage/src/types.rs +++ b/crates/storage/src/types.rs @@ -5,6 +5,7 @@ use std::collections::BTreeMap; use borsh::{BorshDeserialize, BorshSerialize}; use namada_core::borsh::BorshSerializeExt; use namada_core::hash::Hash; +use namada_core::storage::Key; use regex::Regex; /// A key-value pair as raw bytes @@ -69,6 +70,7 @@ impl std::fmt::Debug for PatternIterator { pub struct CommitOnlyData { /// Map from tx hashes to their gas cost pub tx_gas: BTreeMap, + pub no_diff_data: BTreeMap, } impl CommitOnlyData { @@ -76,4 +78,15 @@ impl CommitOnlyData { pub fn serialize(&self) -> Vec { self.serialize_to_vec() } + + pub fn add_no_diff_data(&mut self, key: &Key, value: impl AsRef<[u8]>) { + let key_hash = Hash::sha256(key.to_string()); + let val_hash = Hash::sha256(value); + self.no_diff_data.insert(key_hash, val_hash); + } + + pub fn delete_no_diff_data(&mut self, key: &Key) { + let key_hash = Hash::sha256(key.to_string()); + self.no_diff_data.remove(&key_hash); + } } From a2f007cd94ce4857ba292b287af804321de34900 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 01:42:55 +0200 Subject: [PATCH 2/8] add NoDiff subtree --- crates/merkle_tree/Cargo.toml | 1 + crates/merkle_tree/src/lib.rs | 57 ++++++++++++++++++++++++++- crates/node/src/storage/mod.rs | 63 ++++++++++++++++++++++++++---- crates/node/src/storage/rocksdb.rs | 23 +++++------ crates/state/Cargo.toml | 1 + crates/state/src/lib.rs | 17 +++++++- crates/state/src/wl_state.rs | 44 ++++++++++++--------- crates/storage/src/mockdb.rs | 23 +++++------ crates/storage/src/types.rs | 13 ------ 9 files changed, 171 insertions(+), 71 deletions(-) diff --git a/crates/merkle_tree/Cargo.toml b/crates/merkle_tree/Cargo.toml index 56657bd76f..a53ad24b06 100644 --- a/crates/merkle_tree/Cargo.toml +++ b/crates/merkle_tree/Cargo.toml @@ -17,6 +17,7 @@ migrations = [ "namada_migrations", "linkme", ] +testing = ["namada_core/testing"] [dependencies] namada_core = { path = "../core" } diff --git a/crates/merkle_tree/src/lib.rs b/crates/merkle_tree/src/lib.rs index 3465dd15b0..d447a4667a 100644 --- a/crates/merkle_tree/src/lib.rs +++ b/crates/merkle_tree/src/lib.rs @@ -48,6 +48,8 @@ use namada_macros::BorshDeserializer; use namada_migrations::*; use thiserror::Error; +pub const NO_DIFF_KEY_PREFIX: &str = "no_diff"; + /// Trait for reading from a merkle tree that is a sub-tree /// of the global merkle tree. pub trait SubTreeRead { @@ -177,6 +179,8 @@ pub enum StoreType { PoS, /// For the Ethereum bridge Pool transfers BridgePool, + /// For data not stored to diffs + NoDiff, /// For the commit only data CommitData, } @@ -193,6 +197,8 @@ pub enum Store { PoS(SmtStore), /// For the Ethereum bridge Pool transfers BridgePool(BridgePoolStore), + /// For data not stored to diffs + NoDiff(SmtStore), /// For the commit only data CommitData, } @@ -206,6 +212,7 @@ impl Store { Self::Ibc(store) => StoreRef::Ibc(store), Self::PoS(store) => StoreRef::PoS(store), Self::BridgePool(store) => StoreRef::BridgePool(store), + Self::NoDiff(store) => StoreRef::NoDiff(store), Self::CommitData => StoreRef::CommitData, } } @@ -223,6 +230,8 @@ pub enum StoreRef<'a> { PoS(&'a SmtStore), /// For the Ethereum bridge Pool transfers BridgePool(&'a BridgePoolStore), + /// For data not stored to diffs + NoDiff(&'a SmtStore), /// For commit only data CommitData, } @@ -236,6 +245,7 @@ impl<'a> StoreRef<'a> { Self::Ibc(store) => Store::Ibc(store.to_owned()), Self::PoS(store) => Store::PoS(store.to_owned()), Self::BridgePool(store) => Store::BridgePool(store.to_owned()), + Self::NoDiff(store) => Store::NoDiff(store.to_owned()), Self::CommitData => Store::CommitData, } } @@ -248,6 +258,7 @@ impl<'a> StoreRef<'a> { Self::Ibc(store) => store.serialize_to_vec(), Self::PoS(store) => store.serialize_to_vec(), Self::BridgePool(store) => store.serialize_to_vec(), + Self::NoDiff(store) => store.serialize_to_vec(), Self::CommitData => vec![], } } @@ -256,12 +267,13 @@ impl<'a> StoreRef<'a> { impl StoreType { /// Get an iterator for the base tree and subtrees pub fn iter() -> std::slice::Iter<'static, Self> { - static SUB_TREE_TYPES: [StoreType; 6] = [ + static SUB_TREE_TYPES: [StoreType; 7] = [ StoreType::Base, StoreType::Account, StoreType::PoS, StoreType::Ibc, StoreType::BridgePool, + StoreType::NoDiff, StoreType::CommitData, ]; SUB_TREE_TYPES.iter() @@ -269,11 +281,12 @@ impl StoreType { /// Get an iterator for subtrees pub fn iter_subtrees() -> std::slice::Iter<'static, Self> { - static SUB_TREE_TYPES: [StoreType; 5] = [ + static SUB_TREE_TYPES: [StoreType; 6] = [ StoreType::Account, StoreType::PoS, StoreType::Ibc, StoreType::BridgePool, + StoreType::NoDiff, StoreType::CommitData, ]; SUB_TREE_TYPES.iter() @@ -293,6 +306,14 @@ impl StoreType { SUB_TREE_TYPES.iter() } + /// Return true if the subtree should be saved in every block + pub fn is_stored_every_block(&self) -> bool { + matches!( + self, + StoreType::Base | StoreType::NoDiff | StoreType::CommitData + ) + } + /// Get the store type and the sub key pub fn sub_key(key: &Key) -> Result<(Self, Key)> { if key.is_empty() { @@ -320,6 +341,9 @@ impl StoreType { _ => Ok((StoreType::Account, key.clone())), } } + Some(DbKeySeg::StringSeg(data)) if data.eq(NO_DIFF_KEY_PREFIX) => { + Ok((StoreType::NoDiff, key.clone())) + } Some(DbKeySeg::StringSeg(data)) if data.eq("commit_data") => { Ok((StoreType::CommitData, key.clone())) } @@ -352,6 +376,7 @@ impl StoreType { Self::Ibc => Ok(Store::Ibc(decode(bytes)?)), Self::PoS => Ok(Store::PoS(decode(bytes)?)), Self::BridgePool => Ok(Store::BridgePool(decode(bytes)?)), + Self::NoDiff => Ok(Store::NoDiff(decode(bytes)?)), Self::CommitData => Ok(Store::CommitData), } } @@ -367,6 +392,7 @@ impl FromStr for StoreType { "ibc" => Ok(StoreType::Ibc), "pos" => Ok(StoreType::PoS), "eth_bridge_pool" => Ok(StoreType::BridgePool), + "no_diff" => Ok(StoreType::NoDiff), "commit_data" => Ok(StoreType::CommitData), _ => Err(Error::StoreType(s.to_string())), } @@ -381,6 +407,7 @@ impl fmt::Display for StoreType { StoreType::Ibc => write!(f, "ibc"), StoreType::PoS => write!(f, "pos"), StoreType::BridgePool => write!(f, "eth_bridge_pool"), + StoreType::NoDiff => write!(f, "no_diff"), StoreType::CommitData => write!(f, "commit_data"), } } @@ -427,6 +454,7 @@ pub struct MerkleTree { ibc: Amt, pos: Smt, bridge_pool: BridgePoolTree, + no_diff: Smt, commit_data: CommitDataRoot, } @@ -448,6 +476,7 @@ impl MerkleTree { let pos = Smt::new(stores.pos.0.into(), stores.pos.1); let bridge_pool = BridgePoolTree::new(stores.bridge_pool.0, stores.bridge_pool.1); + let no_diff = Smt::new(stores.no_diff.0.into(), stores.no_diff.1); let commit_data = stores.commit.into(); let tree = Self { @@ -456,6 +485,7 @@ impl MerkleTree { ibc, pos, bridge_pool, + no_diff, commit_data, }; @@ -468,6 +498,8 @@ impl MerkleTree { let pos_root = tree.base.get(&pos_key.into())?; let bp_key = H::hash(StoreType::BridgePool.to_string()); let bp_root = tree.base.get(&bp_key.into())?; + let no_diff_key = H::hash(StoreType::NoDiff.to_string()); + let no_diff_root = tree.base.get(&no_diff_key.into())?; let commit_data_key = H::hash(StoreType::CommitData.to_string()); let commit_data_root = tree.base.get(&commit_data_key.into())?; if tree.base.root().is_zero() @@ -475,11 +507,13 @@ impl MerkleTree { && tree.ibc.root().is_zero() && tree.pos.root().is_zero() && tree.bridge_pool.root().is_zero() + && tree.no_diff.root().is_zero() && tree.commit_data.0.is_zero() || (account_root == tree.account.root().into() && ibc_root == tree.ibc.root().into() && pos_root == tree.pos.root().into() && bp_root == tree.bridge_pool.root().into() + && no_diff_root == tree.no_diff.root().into() && commit_data_root == tree.commit_data.0) { Ok(tree) @@ -498,6 +532,7 @@ impl MerkleTree { let pos = Smt::new(stores.pos.0.into(), stores.pos.1); let bridge_pool = BridgePoolTree::new(stores.bridge_pool.0, stores.bridge_pool.1); + let no_diff = Smt::new(stores.no_diff.0.into(), stores.no_diff.1); let commit_data = stores.commit.into(); Self { @@ -506,6 +541,7 @@ impl MerkleTree { ibc, pos, bridge_pool, + no_diff, commit_data, } } @@ -517,6 +553,7 @@ impl MerkleTree { StoreType::Ibc => Box::new(&self.ibc), StoreType::PoS => Box::new(&self.pos), StoreType::BridgePool => Box::new(&self.bridge_pool), + StoreType::NoDiff => Box::new(&self.no_diff), StoreType::CommitData => Box::new(&self.commit_data), } } @@ -531,6 +568,7 @@ impl MerkleTree { StoreType::Ibc => Box::new(&mut self.ibc), StoreType::PoS => Box::new(&mut self.pos), StoreType::BridgePool => Box::new(&mut self.bridge_pool), + StoreType::NoDiff => Box::new(&mut self.no_diff), StoreType::CommitData => Box::new(&mut self.commit_data), } } @@ -553,6 +591,7 @@ impl MerkleTree { } /// Check if the key exists in the tree + #[cfg(any(test, feature = "testing"))] pub fn has_key(&self, key: &Key) -> Result { let (store_type, sub_key) = StoreType::sub_key(key)?; self.tree(&store_type).subtree_has_key(&sub_key) @@ -601,6 +640,7 @@ impl MerkleTree { && self.ibc.validate() && self.pos.validate() && self.bridge_pool.validate() + && self.no_diff.validate() { let mut reconstructed = Smt::::default(); reconstructed.update( @@ -619,6 +659,10 @@ impl MerkleTree { H::hash(StoreType::BridgePool.to_string()).into(), self.bridge_pool.root().into(), )?; + reconstructed.update( + H::hash(StoreType::NoDiff.to_string()).into(), + self.no_diff.root().into(), + )?; reconstructed.update( H::hash(StoreType::CommitData.to_string()).into(), self.commit_data.0, @@ -649,6 +693,7 @@ impl MerkleTree { self.bridge_pool.root().into(), self.bridge_pool.store(), ), + no_diff: (self.no_diff.root().into(), self.no_diff.store()), commit: self.commit_data.0, } } @@ -803,6 +848,7 @@ pub struct MerkleTreeStoresRead { ibc: (Hash, AmtStore), pos: (Hash, SmtStore), bridge_pool: (KeccakHash, BridgePoolStore), + no_diff: (Hash, SmtStore), commit: Hash, } @@ -815,6 +861,7 @@ impl MerkleTreeStoresRead { StoreType::Ibc => self.ibc.0 = root, StoreType::PoS => self.pos.0 = root, StoreType::BridgePool => self.bridge_pool.0 = root.into(), + StoreType::NoDiff => self.no_diff.0 = root, StoreType::CommitData => self.commit = root, } } @@ -827,6 +874,7 @@ impl MerkleTreeStoresRead { Store::Ibc(store) => self.ibc.1 = store, Store::PoS(store) => self.pos.1 = store, Store::BridgePool(store) => self.bridge_pool.1 = store, + Store::NoDiff(store) => self.no_diff.1 = store, Store::CommitData => (), } } @@ -839,6 +887,7 @@ impl MerkleTreeStoresRead { StoreType::Ibc => StoreRef::Ibc(&self.ibc.1), StoreType::PoS => StoreRef::PoS(&self.pos.1), StoreType::BridgePool => StoreRef::BridgePool(&self.bridge_pool.1), + StoreType::NoDiff => StoreRef::NoDiff(&self.no_diff.1), StoreType::CommitData => StoreRef::CommitData, } } @@ -851,6 +900,7 @@ impl MerkleTreeStoresRead { StoreType::Ibc => self.ibc.0, StoreType::PoS => self.pos.0, StoreType::BridgePool => Hash(self.bridge_pool.0.0), + StoreType::NoDiff => self.no_diff.0, StoreType::CommitData => Hash(self.commit.0), } } @@ -863,6 +913,7 @@ pub struct MerkleTreeStoresWrite<'a> { ibc: (Hash, &'a AmtStore), pos: (Hash, &'a SmtStore), bridge_pool: (Hash, &'a BridgePoolStore), + no_diff: (Hash, &'a SmtStore), commit: Hash, } @@ -875,6 +926,7 @@ impl<'a> MerkleTreeStoresWrite<'a> { StoreType::Ibc => &self.ibc.0, StoreType::PoS => &self.pos.0, StoreType::BridgePool => &self.bridge_pool.0, + StoreType::NoDiff => &self.no_diff.0, StoreType::CommitData => &self.commit, } } @@ -887,6 +939,7 @@ impl<'a> MerkleTreeStoresWrite<'a> { StoreType::Ibc => StoreRef::Ibc(self.ibc.1), StoreType::PoS => StoreRef::PoS(self.pos.1), StoreType::BridgePool => StoreRef::BridgePool(self.bridge_pool.1), + StoreType::NoDiff => StoreRef::NoDiff(self.no_diff.1), StoreType::CommitData => StoreRef::CommitData, } } diff --git a/crates/node/src/storage/mod.rs b/crates/node/src/storage/mod.rs index 06d4fbd0f2..2b57c06c6b 100644 --- a/crates/node/src/storage/mod.rs +++ b/crates/node/src/storage/mod.rs @@ -61,17 +61,19 @@ mod tests { use namada::core::ethereum_events::Uint; use namada::core::hash::Hash; use namada::core::keccak::KeccakHash; - use namada::core::storage::{BlockHeight, Key}; + use namada::core::storage::{BlockHeight, Key, KeySeg}; use namada::core::time::DurationSecs; use namada::core::{address, storage}; use namada::eth_bridge::storage::proof::BridgePoolRootProof; + use namada::ibc::storage::is_ibc_key; use namada::ledger::eth_bridge::storage::bridge_pool; use namada::ledger::gas::STORAGE_ACCESS_GAS_PER_BYTE; - use namada::ledger::ibc::storage::ibc_key; + use namada::ledger::ibc::storage::{client_counter_key, ibc_key}; use namada::ledger::parameters::{EpochDuration, Parameters}; use namada::state::{self, StorageRead, StorageWrite, StoreType, DB}; use namada::token::conversion::update_allowed_conversions; use namada::{decode, encode, parameters}; + use namada_sdk::state::merkle_tree::NO_DIFF_KEY_PREFIX; use namada_sdk::state::StateRead; use proptest::collection::vec; use proptest::prelude::*; @@ -445,14 +447,21 @@ mod tests { is_merklized_storage_key, is_key_diff_storable, ); + let make_key = |suffix: u64| { + match suffix % 3u64 { + 0 => Key::parse(format!("key{suffix}")).unwrap(), + 1 => ibc_key(format!("key{suffix}")).unwrap(), + // for no diff + _ => client_counter_key(), + } + }; let num_keys = 5; let blocks_write_type = blocks_write_type.into_iter().enumerate().map( |(index, write_type)| { // try to update some keys at each height let height = BlockHeight::from(index as u64 / num_keys + 1); - let key = - ibc_key(format!("key{}", index as u64 % num_keys)).unwrap(); + let key = make_key(index as u64 % num_keys); (height, key, write_type) }, ); @@ -461,7 +470,7 @@ mod tests { // write values at Height 0 like init_storage for i in 0..num_keys { - let key = ibc_key(format!("key{}", i)).unwrap(); + let key = make_key(i); let value_bytes = encode(&state.in_mem().block.height); state.db_write(&key, value_bytes)?; } @@ -525,11 +534,14 @@ mod tests { let mut current_state = HashMap::new(); for i in 0..num_keys { - let key = ibc_key(format!("key{}", i)).unwrap(); + let key = make_key(i); current_state.insert(key, true); } - // Check a Merkle tree - for (height, key, write_type) in blocks_write_type { + // Check IBC subtree + for (height, key, write_type) in blocks_write_type.clone() { + if !is_ibc_key(&key) || key == client_counter_key() { + continue; + } let tree = state.get_merkle_tree(height, Some(StoreType::Ibc))?; assert_eq!(tree.root().0, roots.get(&height).unwrap().0); match write_type { @@ -551,6 +563,41 @@ mod tests { } } + // Check NoDiff subtree + let mut current_state = HashMap::new(); + for i in 0..num_keys { + let key = make_key(i); + current_state.insert(key, true); + } + for (height, key, write_type) in blocks_write_type { + if key != client_counter_key() { + continue; + } + let merkle_key = + Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()) + .join(&key); + let tree = + state.get_merkle_tree(height, Some(StoreType::NoDiff))?; + assert_eq!(tree.root().0, roots.get(&height).unwrap().0); + match write_type { + 0 => { + if *current_state.get(&key).unwrap() { + assert!(tree.has_key(&merkle_key)?); + } else { + assert!(!tree.has_key(&merkle_key)?); + } + } + 1 | 3 => { + assert!(!tree.has_key(&merkle_key)?); + current_state.insert(key, false); + } + _ => { + assert!(tree.has_key(&merkle_key)?); + current_state.insert(key, true); + } + } + } + Ok(()) } diff --git a/crates/node/src/storage/rocksdb.rs b/crates/node/src/storage/rocksdb.rs index f178f7149f..c5b91e95c8 100644 --- a/crates/node/src/storage/rocksdb.rs +++ b/crates/node/src/storage/rocksdb.rs @@ -903,15 +903,11 @@ impl DB for RocksDB { // Merkle tree for st in StoreType::iter() { - if *st == StoreType::Base - || *st == StoreType::CommitData - || is_full_commit - { - let key_prefix = match st { - StoreType::Base | StoreType::CommitData => { - tree_key_prefix_with_height(st, height) - } - _ => tree_key_prefix_with_epoch(st, epoch), + if st.is_stored_every_block() || is_full_commit { + let key_prefix = if st.is_stored_every_block() { + tree_key_prefix_with_height(st, height) + } else { + tree_key_prefix_with_epoch(st, epoch) }; let root_key = format!("{key_prefix}/{MERKLE_TREE_ROOT_KEY_SEGMENT}"); @@ -979,11 +975,10 @@ impl DB for RocksDB { .map(|st| Either::Left(std::iter::once(st))) .unwrap_or_else(|| Either::Right(StoreType::iter())); for st in store_types { - let key_prefix = match st { - StoreType::Base | StoreType::CommitData => { - tree_key_prefix_with_height(st, base_height) - } - _ => tree_key_prefix_with_epoch(st, epoch), + let key_prefix = if st.is_stored_every_block() { + tree_key_prefix_with_height(st, base_height) + } else { + tree_key_prefix_with_epoch(st, epoch) }; let root_key = format!("{key_prefix}/{MERKLE_TREE_ROOT_KEY_SEGMENT}"); diff --git a/crates/state/Cargo.toml b/crates/state/Cargo.toml index 12d2c4ff2f..a469dd24c0 100644 --- a/crates/state/Cargo.toml +++ b/crates/state/Cargo.toml @@ -50,6 +50,7 @@ proptest = { workspace = true, optional = true } [dev-dependencies] namada_core = { path = "../core", features = ["testing"] } +namada_merkle_tree = { path = "../merkle_tree", features = ["testing"] } assert_matches.workspace = true chrono.workspace = true diff --git a/crates/state/src/lib.rs b/crates/state/src/lib.rs index a5521f63ed..850749eb53 100644 --- a/crates/state/src/lib.rs +++ b/crates/state/src/lib.rs @@ -676,6 +676,7 @@ mod tests { use std::collections::BTreeMap; use chrono::{TimeZone, Utc}; + use merkle_tree::NO_DIFF_KEY_PREFIX; use namada_core::address::InternalAddress; use namada_core::borsh::{BorshDeserialize, BorshSerializeExt}; use namada_core::storage::DbKeySeg; @@ -977,6 +978,11 @@ mod tests { // Check merkle tree inclusion of key-val-2 explicitly let is_merklized2 = state.in_mem().block.tree.has_key(&key2).unwrap(); + assert!(!is_merklized2); + let no_diff_key2 = + Key::from(NO_DIFF_KEY_PREFIX.to_string().to_db_key()).join(&key2); + let is_merklized2 = + state.in_mem().block.tree.has_key(&no_diff_key2).unwrap(); assert!(is_merklized2); // Key3 should be in storage. Confirm by reading from @@ -990,6 +996,11 @@ mod tests { // 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 @@ -1058,8 +1069,10 @@ mod tests { // 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(&key2).unwrap(); - let is_merklized3 = state.in_mem().block.tree.has_key(&key3).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); // Check that key-val-1 diffs are properly updated for blocks 0 and 1 diff --git a/crates/state/src/wl_state.rs b/crates/state/src/wl_state.rs index dc975617e9..e698894237 100644 --- a/crates/state/src/wl_state.rs +++ b/crates/state/src/wl_state.rs @@ -8,6 +8,7 @@ use namada_core::chain::ChainId; use namada_core::storage; use namada_core::time::DateTimeUtc; use namada_events::{EmitEvents, EventToEmit}; +use namada_merkle_tree::NO_DIFF_KEY_PREFIX; use namada_parameters::EpochDuration; use namada_replay_protection as replay_protection; use namada_storage::conversion_state::{ConversionState, WithConversionState}; @@ -16,9 +17,9 @@ use namada_storage::{BlockHeight, BlockStateRead, BlockStateWrite, ResultExt}; use crate::in_memory::InMemory; use crate::write_log::{StorageModification, WriteLog}; use crate::{ - is_pending_transfer_key, DBIter, Epoch, Error, Hash, Key, LastBlock, - MembershipProof, MerkleTree, MerkleTreeError, ProofOps, Result, State, - StateRead, StorageHasher, StorageResult, StoreType, DB, + is_pending_transfer_key, DBIter, Epoch, Error, Hash, Key, KeySeg, + LastBlock, MembershipProof, MerkleTree, MerkleTreeError, ProofOps, Result, + State, StateRead, StorageHasher, StorageResult, StoreType, DB, EPOCH_SWITCH_BLOCKS_DELAY, STORAGE_ACCESS_GAS_PER_BYTE, }; @@ -278,10 +279,12 @@ where } else { // Update the merkle tree if is_key_merklized && !persist_diffs { - self.in_mem.commit_only_data.add_no_diff_data(key, value); + 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 { self.in_mem.block.tree.update(key, value)?; - } + }; } Ok(self.db.batch_write_subspace_val( batch, @@ -304,7 +307,8 @@ where let persist_diffs = (self.diff_key_filter)(key); // Update the merkle tree if is_key_merklized && !persist_diffs { - self.in_mem.commit_only_data.delete_no_diff_data(key); + 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 { self.in_mem.block.tree.delete(key)?; } @@ -723,7 +727,9 @@ where } else { // Update the merkle tree if is_key_merklized && !persist_diffs { - self.in_mem.commit_only_data.add_no_diff_data(key, value); + 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 { self.in_mem.block.tree.update(key, value)?; } @@ -757,7 +763,9 @@ where let is_key_merklized = (self.merkle_tree_key_filter)(key); let persist_diffs = (self.diff_key_filter)(key); if is_key_merklized && !persist_diffs { - self.in_mem.commit_only_data.delete_no_diff_data(key); + 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 { self.in_mem.block.tree.delete(key)?; } @@ -876,12 +884,11 @@ where .pred_epochs .get_epoch(height) .unwrap_or_default(); - let start_height = if store_type == Some(StoreType::CommitData) { - // CommitData is stored every height - height - } else { + let start_height = match store_type { + // subtree is stored every height + Some(st) if st.is_stored_every_block() => height, // others are stored at the first height of each epoch - match self + _ => match self .in_mem .block .pred_epochs @@ -890,7 +897,7 @@ where Some(BlockHeight(0)) => BlockHeight(1), Some(height) => height, None => BlockHeight(1), - } + }, }; let stores = self .db @@ -994,7 +1001,7 @@ where } } - // Restore the base tree and CommitData tree + // Restore the base tree and subtrees match store_type { Some(st) => { // It is enough to get the base tree @@ -1012,15 +1019,16 @@ where tree = MerkleTree::::new_partial(stores); } None => { - // Get the base and CommitData trees + // Get the base and subtrees stored in every block let mut stores = self .db .read_merkle_tree_stores(epoch, height, None)? .ok_or(Error::NoMerkleTree { height })?; let restored_stores = tree.stores(); - // Set all rebuilt subtrees except for CommitData tree + // Set all rebuilt subtrees except for the subtrees stored in + // every block for st in StoreType::iter_subtrees() { - if *st != StoreType::CommitData { + if !st.is_stored_every_block() { stores.set_root(st, *restored_stores.root(st)); stores.set_store(restored_stores.store(st).to_owned()); } diff --git a/crates/storage/src/mockdb.rs b/crates/storage/src/mockdb.rs index 101829f434..20432d37ff 100644 --- a/crates/storage/src/mockdb.rs +++ b/crates/storage/src/mockdb.rs @@ -238,15 +238,11 @@ impl DB for MockDB { // Merkle tree for st in StoreType::iter() { - if *st == StoreType::Base - || *st == StoreType::CommitData - || is_full_commit - { - let key_prefix = match st { - StoreType::Base | StoreType::CommitData => { - tree_key_prefix_with_height(st, height) - } - _ => tree_key_prefix_with_epoch(st, epoch), + if st.is_stored_every_block() || is_full_commit { + let key_prefix = if st.is_stored_every_block() { + tree_key_prefix_with_height(st, height) + } else { + tree_key_prefix_with_epoch(st, epoch) }; let root_key = format!("{key_prefix}/{MERKLE_TREE_ROOT_KEY_SEGMENT}"); @@ -302,11 +298,10 @@ impl DB for MockDB { .map(|st| Either::Left(std::iter::once(st))) .unwrap_or_else(|| Either::Right(StoreType::iter())); for st in store_types { - let key_prefix = match st { - StoreType::Base | StoreType::CommitData => { - tree_key_prefix_with_height(st, base_height) - } - _ => tree_key_prefix_with_epoch(st, epoch), + let key_prefix = if st.is_stored_every_block() { + tree_key_prefix_with_height(st, base_height) + } else { + tree_key_prefix_with_epoch(st, epoch) }; let root_key = format!("{key_prefix}/{MERKLE_TREE_ROOT_KEY_SEGMENT}"); diff --git a/crates/storage/src/types.rs b/crates/storage/src/types.rs index bf39212a1d..e5f323bf09 100644 --- a/crates/storage/src/types.rs +++ b/crates/storage/src/types.rs @@ -5,7 +5,6 @@ use std::collections::BTreeMap; use borsh::{BorshDeserialize, BorshSerialize}; use namada_core::borsh::BorshSerializeExt; use namada_core::hash::Hash; -use namada_core::storage::Key; use regex::Regex; /// A key-value pair as raw bytes @@ -70,7 +69,6 @@ impl std::fmt::Debug for PatternIterator { pub struct CommitOnlyData { /// Map from tx hashes to their gas cost pub tx_gas: BTreeMap, - pub no_diff_data: BTreeMap, } impl CommitOnlyData { @@ -78,15 +76,4 @@ impl CommitOnlyData { pub fn serialize(&self) -> Vec { self.serialize_to_vec() } - - pub fn add_no_diff_data(&mut self, key: &Key, value: impl AsRef<[u8]>) { - let key_hash = Hash::sha256(key.to_string()); - let val_hash = Hash::sha256(value); - self.no_diff_data.insert(key_hash, val_hash); - } - - pub fn delete_no_diff_data(&mut self, key: &Key) { - let key_hash = Hash::sha256(key.to_string()); - self.no_diff_data.remove(&key_hash); - } } From 1a1c03ea9265e9a94ab0f6fefdda3bb4ed8feb48 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 02:20:54 +0200 Subject: [PATCH 3/8] fmt --- crates/merkle_tree/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/merkle_tree/src/lib.rs b/crates/merkle_tree/src/lib.rs index d447a4667a..cab4ae92c4 100644 --- a/crates/merkle_tree/src/lib.rs +++ b/crates/merkle_tree/src/lib.rs @@ -48,6 +48,7 @@ use namada_macros::BorshDeserializer; use namada_migrations::*; use thiserror::Error; +/// Key prefix for the data not stored to diffs pub const NO_DIFF_KEY_PREFIX: &str = "no_diff"; /// Trait for reading from a merkle tree that is a sub-tree From 6443f83df5ae6377a30e6b53b484403eadb1f1d0 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 02:22:12 +0200 Subject: [PATCH 4/8] add changelog --- .changelog/unreleased/features/3293-merklized-not-diffed.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/features/3293-merklized-not-diffed.md diff --git a/.changelog/unreleased/features/3293-merklized-not-diffed.md b/.changelog/unreleased/features/3293-merklized-not-diffed.md new file mode 100644 index 0000000000..b77ebc0d1c --- /dev/null +++ b/.changelog/unreleased/features/3293-merklized-not-diffed.md @@ -0,0 +1,2 @@ +- Enable to write data to be updated in the subspace and Merkle tree, but not to + be updated in diffs ([\#3293](https://github.com/anoma/namada/issues/3293)) \ No newline at end of file From d7660f3521d69308cfdaff4ec9c279b6ba32d8b1 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 03:04:00 +0200 Subject: [PATCH 5/8] fix for tests --- crates/state/Cargo.toml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/state/Cargo.toml b/crates/state/Cargo.toml index a469dd24c0..69c34303af 100644 --- a/crates/state/Cargo.toml +++ b/crates/state/Cargo.toml @@ -16,7 +16,11 @@ version.workspace = true default = [] # for integration tests and test utilities -testing = ["proptest", "namada_core/testing"] +testing = [ + "proptest", + "namada_core/testing", + "namada_merkle_tree/testing" +] migrations = [ "namada_migrations", "linkme", From cd441a2aaf15f54718f942d02b877f5915f87220 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 14:44:59 +0200 Subject: [PATCH 6/8] 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(); } From 44c9201512543f8d4eb73fbce7a50c386bdd83c1 Mon Sep 17 00:00:00 2001 From: yito88 Date: Wed, 22 May 2024 22:34:20 +0200 Subject: [PATCH 7/8] add comments --- crates/node/src/storage/mod.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/node/src/storage/mod.rs b/crates/node/src/storage/mod.rs index 4e400350a9..549fd5f3d4 100644 --- a/crates/node/src/storage/mod.rs +++ b/crates/node/src/storage/mod.rs @@ -443,8 +443,11 @@ mod tests { // Prepare written keys for non-probable data, probable data (IBC), and // no diffed data let make_key = |suffix: u64| { + // For three type keys match suffix % 3u64 { + // for non-probable data 0 => Key::parse(format!("key{suffix}")).unwrap(), + // for probable data 1 => ibc_key(format!("key{suffix}")).unwrap(), // for no diff _ => client_counter_key(), @@ -543,6 +546,7 @@ mod tests { assert_eq!(tree.root().0, roots.get(&height).unwrap().0); match write_type { 0 => { + // data was not updated if *current_state.get(&key).unwrap() { assert!(tree.has_key(&key)?); } else { @@ -550,10 +554,12 @@ mod tests { } } 1 | 3 => { + // data was deleted assert!(!tree.has_key(&key)?); current_state.insert(key, false); } _ => { + // data was updated assert!(tree.has_key(&key)?); current_state.insert(key, true); } @@ -579,6 +585,7 @@ mod tests { assert_eq!(tree.root().0, roots.get(&height).unwrap().0); match write_type { 0 => { + // data was not updated if *current_state.get(&key).unwrap() { assert!(tree.has_key(&merkle_key)?); } else { @@ -586,10 +593,12 @@ mod tests { } } 1 | 3 => { + // data was deleted assert!(!tree.has_key(&merkle_key)?); current_state.insert(key, false); } _ => { + // data was updated assert!(tree.has_key(&merkle_key)?); current_state.insert(key, true); } From fc483daa7afde8d5dd976b88d3b35518fec70a26 Mon Sep 17 00:00:00 2001 From: yito88 Date: Thu, 23 May 2024 14:52:37 +0200 Subject: [PATCH 8/8] fix typo --- crates/node/src/storage/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/node/src/storage/mod.rs b/crates/node/src/storage/mod.rs index 549fd5f3d4..5e3c030cf8 100644 --- a/crates/node/src/storage/mod.rs +++ b/crates/node/src/storage/mod.rs @@ -440,14 +440,14 @@ mod tests { None, is_key_diff_storable, ); - // Prepare written keys for non-probable data, probable data (IBC), and + // Prepare written keys for non-provable data, provable data (IBC), and // no diffed data let make_key = |suffix: u64| { // For three type keys match suffix % 3u64 { - // for non-probable data + // for non-provable data 0 => Key::parse(format!("key{suffix}")).unwrap(), - // for probable data + // for provable data 1 => ibc_key(format!("key{suffix}")).unwrap(), // for no diff _ => client_counter_key(),