-
Notifications
You must be signed in to change notification settings - Fork 722
Feat/marf compression #6654
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
base: develop
Are you sure you want to change the base?
Feat/marf compression #6654
Changes from 1 commit
0a3fa71
b49e29c
c6929bf
e2bbdaa
bcad6ed
045dc0a
f6e7014
4dfce7e
a112f52
e5d6ec5
1d492bd
adad821
00c6c5b
295d227
83831c7
ab778c0
4420530
eecc737
73a2a9d
dc944e3
ed3475a
756d79e
bad16e8
40908f4
27de3b6
f989667
d481601
fdc3e60
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 |
|---|---|---|
|
|
@@ -2180,17 +2180,22 @@ impl<'a, T: MarfTrieId> TrieStorageTransaction<'a, T> { | |
| if real_bhh != &bhh { | ||
| // note: this was moved from the block_retarget function | ||
| // to avoid stepping on the borrow checker. | ||
| debug!("Retarget block {} to {}", bhh, real_bhh); | ||
| debug!( | ||
| "Retarget block {} to {}. Current block ID is {:?}", | ||
| bhh, real_bhh, &self.data.cur_block_id | ||
| ); | ||
| // switch over state | ||
| self.data.retarget_block(real_bhh.clone()); | ||
| } | ||
| self.with_trie_blobs(|db, blobs| match blobs { | ||
| let new_block_id = self.with_trie_blobs(|db, blobs| match blobs { | ||
| Some(blobs) => blobs.store_trie_blob(db, real_bhh, &buffer), | ||
| None => { | ||
| test_debug!("Stored trie blob {} to db", real_bhh); | ||
| trie_sql::write_trie_blob(db, real_bhh, &buffer) | ||
| } | ||
| })? | ||
| })?; | ||
| self.data.set_block(real_bhh.clone(), Some(new_block_id)); | ||
| new_block_id | ||
| } | ||
| FlushOptions::MinedTable(real_bhh) => { | ||
| if self.unconfirmed() { | ||
|
|
@@ -2610,11 +2615,14 @@ impl<T: MarfTrieId> TrieStorageConnection<'_, T> { | |
| /// when following a backptr, which stores the block identifier directly. | ||
| pub fn open_block_known_id(&mut self, bhh: &T, id: u32) -> Result<(), Error> { | ||
| trace!( | ||
| "open_block_known_id({},{}) (unconfirmed={:?},{})", | ||
| "open_block_known_id({},{}) (unconfirmed={:?},{}) from {},{:?} in {}", | ||
| bhh, | ||
| id, | ||
| &self.unconfirmed_block_id, | ||
| self.unconfirmed() | ||
| self.unconfirmed(), | ||
| &self.data.cur_block, | ||
| &self.data.cur_block_id, | ||
| self.db_path, | ||
| ); | ||
| if *bhh == self.data.cur_block && self.data.cur_block_id.is_some() { | ||
| // no-op | ||
|
|
@@ -2636,10 +2644,11 @@ impl<T: MarfTrieId> TrieStorageConnection<'_, T> { | |
| /// that all node reads will occur relative to it. | ||
| pub fn open_block(&mut self, bhh: &T) -> Result<(), Error> { | ||
| trace!( | ||
| "open_block({}) (unconfirmed={:?},{})", | ||
| "open_block({}) (unconfirmed={:?},{}) in {}", | ||
| bhh, | ||
| &self.unconfirmed_block_id, | ||
| self.unconfirmed() | ||
| self.unconfirmed(), | ||
| self.db_path | ||
| ); | ||
| self.bench.open_block_start(); | ||
|
|
||
|
|
@@ -3059,8 +3068,9 @@ impl<T: MarfTrieId> TrieStorageConnection<'_, T> { | |
| self.unconfirmed() | ||
| ); | ||
|
|
||
| let (saved_block_hash, saved_block_id) = self.get_cur_block_and_id(); | ||
|
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. Managing the block restore appears unnecessary, since none of the downstream code (including the invoked methods) modifies the currently opened block. It’s harmless to keep the restore in place, but it doesn’t have any observable effect, so we could safely remove it to simplify the logic. I also ran the consensus tests and some integration tests without the block-restore logic, and they all passed. |
||
|
|
||
| let cur_block_id = block_id; | ||
| let cur_block = self.get_block_hash_caching(cur_block_id)?.to_owned(); | ||
| let mut node_hash_opt = None; | ||
| let mut patches: Vec<(u32, TriePtr, TrieNodePatch)> = vec![]; | ||
| for _ in 0..MAX_PATCH_DEPTH { | ||
|
|
@@ -3070,7 +3080,7 @@ impl<T: MarfTrieId> TrieStorageConnection<'_, T> { | |
| let node = node.apply_patches(&patches, cur_block_id).ok_or_else(|| { | ||
| Error::CorruptionError("Failed to apply patches to node".to_string()) | ||
| })?; | ||
| self.open_block(&cur_block)?; | ||
| self.open_block_maybe_id(&saved_block_hash, saved_block_id)?; | ||
| return Ok((node, node_hash_opt.unwrap_or(hash))); | ||
| } | ||
| Err(Error::Patch(hash_opt, node_patch)) => { | ||
|
|
@@ -3087,12 +3097,12 @@ impl<T: MarfTrieId> TrieStorageConnection<'_, T> { | |
| } | ||
| } | ||
| Err(e) => { | ||
| self.open_block(&cur_block)?; | ||
| self.open_block_maybe_id(&saved_block_hash, saved_block_id)?; | ||
| return Err(e); | ||
| } | ||
| } | ||
| } | ||
| self.open_block(&cur_block)?; | ||
| self.open_block_maybe_id(&saved_block_hash, saved_block_id)?; | ||
| return Err(Error::NodeTooDeep); | ||
| } | ||
|
|
||
|
|
||
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.
With this change, where all block data are always set at once via
set_block, it seems to supersede the retarget management, so that logic could likely be removed. I wrote some local unit tests to validate this behavior, and also checked the behaviour running integration testsThere 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.
added unit tests: bad16e8