-
Notifications
You must be signed in to change notification settings - Fork 62
Auditing improvements #470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ use crate::storage::types::{ | |
| DbRecord, KeyData, StorageType, ValueState, ValueStateKey, ValueStateRetrievalFlag, | ||
| }; | ||
| use crate::storage::{Database, Storable, StorageUtil}; | ||
| use crate::tree_node::{NodeKey, TreeNodeWithPreviousValue}; | ||
| use crate::{AkdLabel, AkdValue}; | ||
| use async_trait::async_trait; | ||
| use dashmap::DashMap; | ||
|
|
@@ -30,15 +31,42 @@ type UserValueMap = HashMap<Epoch, ValueState>; | |
| pub struct AsyncInMemoryDatabase { | ||
| db: Arc<DashMap<Vec<u8>, DbRecord>>, | ||
| user_info: Arc<DashMap<Vec<u8>, UserValueMap>>, | ||
| /// This flag is used to determine whether the database will automatically | ||
| /// (and aggressively) remove entries corresponding to left and right | ||
| /// children of a tree node when the node is inserted. The purpose behind this | ||
| /// is to reduce the size of the in-memory database by culling the child enries | ||
| /// once the parent node's hash has been calculated. The primary use case for this | ||
| /// is to improve auditing memory usage and time (since during auditing, we no longer | ||
| /// care about the child node hashes once its parent has been computed). Note that this | ||
| /// technique takes advantage of the way batch insertion of nodes into the tree works, | ||
| /// since we always process all of the children of a particular subtree before processing | ||
| /// the root of that subtree. | ||
| remove_child_nodes_on_insertion: bool, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm generally not the biggest fan of using a boolean to differentiate behavior, as I'd like to use something like a different type to reflect that the in-memory store we're using doesn't store everything, but I think that's a bit more of a rework than what we have now. As such, I think what we have here is sufficient since we need to influence how inner workings of |
||
| } | ||
|
|
||
| unsafe impl Send for AsyncInMemoryDatabase {} | ||
| unsafe impl Sync for AsyncInMemoryDatabase {} | ||
|
|
||
| impl AsyncInMemoryDatabase { | ||
| /// Returns the size of the in-memory database | ||
| pub fn size_of_db(&self) -> usize { | ||
| self.db.len() | ||
| } | ||
|
|
||
| /// Creates a new in memory db | ||
| pub fn new() -> Self { | ||
| Self::default() | ||
| Self { | ||
| remove_child_nodes_on_insertion: false, | ||
| ..Self::default() | ||
| } | ||
| } | ||
|
|
||
| /// Creates a new in memory db with the flag set to remove child nodes on insertion | ||
| pub fn new_with_remove_child_nodes_on_insertion() -> Self { | ||
| Self { | ||
| remove_child_nodes_on_insertion: true, | ||
| ..Self::default() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
|
@@ -103,6 +131,22 @@ impl Database for AsyncInMemoryDatabase { | |
| } | ||
| } | ||
| } else { | ||
| if self.remove_child_nodes_on_insertion { | ||
| if let DbRecord::TreeNode(node) = record.clone() { | ||
| if let Some(left_child) = node.latest_node.left_child { | ||
| self.db | ||
| .remove(&TreeNodeWithPreviousValue::get_full_binary_key_id( | ||
| &NodeKey(left_child), | ||
| )); | ||
| } | ||
| if let Some(right_child) = node.latest_node.right_child { | ||
| self.db | ||
| .remove(&TreeNodeWithPreviousValue::get_full_binary_key_id( | ||
| &NodeKey(right_child), | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| self.db.insert(record.get_full_binary_id(), record); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we're mostly doing the same thing in the
StorageManagerinstances we're creating here (i.e. dmanager1andmanager2). That is:With the above in mind, do you think it might make sense to define a helper-like function which captures the common? E.g.
If we do something like that, then I think this function essentially becomes:
Note: I didn't run, nor did I format, any of the code above. It's just meant to reflect an idea to reduce some duplication, but please feel free to ignore if you prefer what's here.