-
Notifications
You must be signed in to change notification settings - Fork 67
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
Inline value should never be recorded. #184
Conversation
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 you please add some tests?
trie-db/src/lookup.rs
Outdated
if value_recording_required { | ||
let is_inline = L::MAX_INLINE_VALUE | ||
.map(|max| data.as_ref().len() < max as usize) | ||
.unwrap_or(true); |
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.
.unwrap_or(true); | |
.unwrap_or(false); |
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's true
I think: if No inline limit, then this is the old trie version where all values are inline so is_inline should be true for MAX_INLINE_VALUE I think (I allways end up needing to think twice 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.
Ahh okay. This is really confusing. Could you please leave some comment?
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.
added, also inverse the comparison order (seems more readable this way).
was my morning plan 👍 |
@@ -452,7 +452,7 @@ fn test_recorder_with_cache_get_hash_internal<T: TrieLayout>() { | |||
assert!(cache.get_node(&root).is_some()); | |||
// Also the data should be cached. | |||
|
|||
if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize >= key_value[1].1.len()) { | |||
if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize > key_value[1].1.len()) { |
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.
this test was not correct
Will release as 0.25.1 |
When value are inline, they are encoded into their parent node and we shall not record them individually.
Should fix: paritytech/substrate#13357