-
Notifications
You must be signed in to change notification settings - Fork 619
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
fix(client): fix trie_node_touched bug #3025
Conversation
Add read snapshots to storage and do refactoring where Trie instances are no longer persisted. Test plan --------- existing tests
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.
Can we add a test that reproduces the failure and verify that it passes now?
let mut guard = self.cache.lock().expect(POISONED_LOCK_ERR); | ||
if let Some(val) = guard.cache_get(hash) { | ||
Self::vec_to_rc(val) | ||
// Ignore cache to be safe. retrieve_rc is used only when writing storage and cache is shared with readers. |
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.
but don't we write to storage quite frequently (every block)?
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.
most of the time retrieve_rc is called on new values, and we go to storage anyway
Is it possible to reproduce the panic from the linked issue within a test? |
Change some code so that Trie instances are no longer persisted. Test plan --------- existing tests, new regression test
Change some code so that Trie instances are no longer persisted. Test plan --------- existing tests, new regression test
@@ -226,6 +227,7 @@ impl DBTransaction { | |||
pub struct RocksDB { | |||
db: DB, | |||
cfs: Vec<*const ColumnFamily>, | |||
_pin: PhantomPinned, |
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.
What's was the intention of pining related changes here (adding PhantomPinned
and Pin<dyn ...>
)? On the first glance, it seems as if they don't do anything useful here.
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.
I'm simply worried that we mention such change the second time in two weeks. First was #5722.
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.
I am not sure 😅
Add read snapshots to storage and do refactoring where
Trie instances are no longer persisted.
Fixes #3020
Test plan
existing tests