refactor: add FlatStateValue constructor for on-disk values (#9269)

This logic is used in multiple places, so it makes sense to abstract it behind a method in `FlatStateValue`. `on_disk` name is chosen over `new` since this logic is only applicable for values to be persisted on disk. Later we will also add inlining for values as part of Flat Storage Delta and those will have lower inlining threshold, so we will have another constructor.
near · Jun 30, 2023 · 283c2bb · 283c2bb
1 parent 7dea4bd
commit 283c2bb
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 35 deletions.
diff --git a/core/primitives/src/state.rs b/core/primitives/src/state.rs
@@ -69,6 +69,22 @@ pub enum FlatStateValue {
 }
 
 impl FlatStateValue {
+    /// Defines value size threshold for flat state inlining.
+    /// It means that values having size greater than the threshold will be stored
+    /// in FlatState as `FlatStateValue::Ref`, otherwise the whole value will be
+    /// stored as `FlatStateValue::Inlined`.
+    /// See the following comment for reasoning behind the threshold value:
+    /// https://github.com/near/nearcore/issues/8243#issuecomment-1523049994
+    pub const INLINE_DISK_VALUE_THRESHOLD: usize = 4000;
+
+    pub fn on_disk(value: &[u8]) -> Self {
+        if value.len() <= Self::INLINE_DISK_VALUE_THRESHOLD {
+            Self::inlined(value)
+        } else {
+            Self::value_ref(value)
+        }
+    }
+
     pub fn value_ref(value: &[u8]) -> Self {
         Self::Ref(ValueRef::new(value))
     }

diff --git a/core/store/src/flat/delta.rs b/core/store/src/flat/delta.rs
@@ -7,7 +7,6 @@ use near_primitives::types::RawStateChangesWithTrieKey;
 use std::collections::HashMap;
 use std::sync::Arc;
 
-use super::types::INLINE_DISK_VALUE_THRESHOLD;
 use super::{store_helper, BlockInfo};
 use crate::{CryptoHash, StoreUpdate};
 
@@ -94,13 +93,7 @@ impl FlatStateChanges {
                 .last()
                 .expect("Committed entry should have at least one change")
                 .data;
-            let flat_state_value = last_change.as_ref().map(|value| {
-                if value.len() <= INLINE_DISK_VALUE_THRESHOLD {
-                    FlatStateValue::inlined(value)
-                } else {
-                    FlatStateValue::value_ref(value)
-                }
-            });
+            let flat_state_value = last_change.as_ref().map(|value| FlatStateValue::on_disk(value));
             delta.insert(key, flat_state_value);
         }
         Self(delta)

diff --git a/core/store/src/flat/inlining_migration.rs b/core/store/src/flat/inlining_migration.rs
@@ -22,7 +22,7 @@ use crate::{DBCol, Store, TrieDBStorage, TrieStorage};
 use super::store_helper::{
     decode_flat_state_db_key, get_flat_state_values_inlining_migration_status,
 };
-use super::types::{FlatStateValuesInliningMigrationStatus, INLINE_DISK_VALUE_THRESHOLD};
+use super::types::FlatStateValuesInliningMigrationStatus;
 use super::FlatStorageManager;
 
 pub struct FlatStateValuesInliningMigrationHandle {
@@ -159,7 +159,7 @@ impl StateValueReader {
     }
 }
 
-/// Inlines all FlatState values having length below `INLINE_DISK_VALUE_THRESHOLD`.
+/// Inlines all FlatState values having length below `FlatStateValue::INLINE_DISK_VALUE_THRESHOLD`.
 /// Migration is safe to be executed in parallel with block processing, which
 /// is achieved by temporary preventing FlatState updates with
 /// `FlatStorageManager::set_flat_state_updates_mode`.
@@ -216,7 +216,7 @@ pub fn inline_flat_state_values(
             };
             PROCESSED_TOTAL_VALUES_SIZE.inc_by(value_size);
             if let FlatStateValue::Ref(value_ref) = fs_value {
-                if value_ref.length as usize <= INLINE_DISK_VALUE_THRESHOLD {
+                if value_ref.length as usize <= FlatStateValue::INLINE_DISK_VALUE_THRESHOLD {
                     if min_key.is_none() {
                         min_key = Some(key.to_vec());
                     }
@@ -311,7 +311,6 @@ fn log_skipped(reason: &str, err: impl std::error::Error) {
 mod tests {
     use super::inline_flat_state_values;
     use crate::flat::store_helper::encode_flat_state_db_key;
-    use crate::flat::types::INLINE_DISK_VALUE_THRESHOLD;
     use crate::flat::{FlatStateValuesInliningMigrationHandle, FlatStorageManager};
     use crate::{DBCol, NodeStorage, Store, TrieCachingStorage};
     use borsh::{BorshDeserialize, BorshSerialize};
@@ -326,8 +325,14 @@ mod tests {
     fn full_migration() {
         let store = NodeStorage::test_opener().1.open().unwrap().get_hot_store();
         let shard_uid = ShardLayout::v0_single_shard().get_shard_uids()[0];
-        let values =
-            [vec![0], vec![1], vec![2; INLINE_DISK_VALUE_THRESHOLD + 1], vec![3], vec![4], vec![5]];
+        let values = [
+            vec![0],
+            vec![1],
+            vec![2; FlatStateValue::INLINE_DISK_VALUE_THRESHOLD + 1],
+            vec![3],
+            vec![4],
+            vec![5],
+        ];
         populate_flat_store(&store, shard_uid, &values);
 
         let flat_storage_manager =
@@ -363,8 +368,14 @@ mod tests {
         init_test_logger();
         let store = NodeStorage::test_opener().1.open().unwrap().get_hot_store();
         let shard_uid = ShardLayout::v0_single_shard().get_shard_uids()[0];
-        let values =
-            [vec![0], vec![1], vec![2; INLINE_DISK_VALUE_THRESHOLD + 1], vec![3], vec![4], vec![5]];
+        let values = [
+            vec![0],
+            vec![1],
+            vec![2; FlatStateValue::INLINE_DISK_VALUE_THRESHOLD + 1],
+            vec![3],
+            vec![4],
+            vec![5],
+        ];
         populate_flat_store(&store, shard_uid, &values);
 
         let flat_storage_manager =
@@ -398,8 +409,14 @@ mod tests {
         init_test_logger();
         let store = NodeStorage::test_opener().1.open().unwrap().get_hot_store();
         let shard_uid = ShardLayout::v0_single_shard().get_shard_uids()[0];
-        let values =
-            [vec![0], vec![1], vec![2; INLINE_DISK_VALUE_THRESHOLD + 1], vec![3], vec![4], vec![5]];
+        let values = [
+            vec![0],
+            vec![1],
+            vec![2; FlatStateValue::INLINE_DISK_VALUE_THRESHOLD + 1],
+            vec![3],
+            vec![4],
+            vec![5],
+        ];
         populate_flat_store(&store, shard_uid, &values);
 
         let flat_storage_manager =

diff --git a/core/store/src/flat/mod.rs b/core/store/src/flat/mod.rs
@@ -42,7 +42,7 @@ pub use metrics::FlatStorageCreationMetrics;
 pub use storage::FlatStorage;
 pub use types::{
     BlockInfo, FetchingStateStatus, FlatStateIterator, FlatStorageCreationStatus, FlatStorageError,
-    FlatStorageReadyStatus, FlatStorageStatus, INLINE_DISK_VALUE_THRESHOLD,
+    FlatStorageReadyStatus, FlatStorageStatus,
 };
 
 pub(crate) const POISONED_LOCK_ERR: &str = "The lock was poisoned.";

diff --git a/core/store/src/flat/types.rs b/core/store/src/flat/types.rs
@@ -4,14 +4,6 @@ use near_primitives::hash::CryptoHash;
 use near_primitives::state::FlatStateValue;
 use near_primitives::types::BlockHeight;
 
-/// Defines value size threshold for flat state inlining.
-/// It means that values having size greater than the threshold will be stored
-/// in FlatState as `FlatStateValue::Ref`, otherwise the whole value will be
-/// stored as `FlatStateValue::Inlined`.
-/// See the following comment for reasoning behind the threshold value:
-/// https://github.com/near/nearcore/issues/8243#issuecomment-1523049994
-pub const INLINE_DISK_VALUE_THRESHOLD: usize = 4000;
-
 #[derive(BorshSerialize, BorshDeserialize, Debug, Copy, Clone, PartialEq, Eq)]
 pub struct BlockInfo {
     pub hash: CryptoHash,

diff --git a/core/store/src/trie/state_parts.rs b/core/store/src/trie/state_parts.rs
@@ -16,7 +16,7 @@
 //! Moreover, we include all left siblings for each path, because they are
 //! necessary to prove its position in the list of prefix sums.
 
-use crate::flat::{FlatStateChanges, FlatStateIterator, INLINE_DISK_VALUE_THRESHOLD};
+use crate::flat::{FlatStateChanges, FlatStateIterator};
 use crate::trie::iterator::TrieTraversalItem;
 use crate::trie::nibble_slice::NibbleSlice;
 use crate::trie::trie_storage::TrieMemoryPartialStorage;
@@ -463,13 +463,7 @@ impl Trie {
             let value = trie.storage.retrieve_raw_bytes(&hash)?;
             map.entry(hash).or_insert_with(|| (value.to_vec(), 0)).1 += 1;
             if let Some(trie_key) = key {
-                // TODO: Refactor to hide this condition in an abstraction.
-                //       For example, it could be `FlatStateValue::new(&value)`.
-                let flat_state_value = if value.len() <= INLINE_DISK_VALUE_THRESHOLD {
-                    FlatStateValue::inlined(&value)
-                } else {
-                    FlatStateValue::value_ref(&value)
-                };
+                let flat_state_value = FlatStateValue::on_disk(&value);
                 flat_state_delta.insert(trie_key.clone(), Some(flat_state_value));
                 if is_contract_code_key(&trie_key) {
                     contract_codes.push(ContractCode::new(value.to_vec(), None));