Skip to content

Conversation

@arkpar
Copy link
Member

@arkpar arkpar commented Mar 28, 2019

Trie iteration was not implemented correctly in #12
Also switched tests to use prefixed storage.

/// is known to be literal.
fn get_raw_or_lookup(&'db self, node: &[u8], partial_key: &[u8]) -> Result<Cow<'db, DBValue>, H::Out, C::Error> {
match (partial_key.is_empty(), C::try_decode_hash(node)) {
match (partial_key == nibbleslice::EMPTY_ENCODED, C::try_decode_hash(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit beyond the scope of the PR, but is this match assuming that only empty partial-keys could lead to nodes small enough to fit in the hash? A small partial key would easily break that if so

Copy link
Member Author

@arkpar arkpar Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously (before #12) there was a check for root node here. partial_key is the key up to the node. Only the root node has it empty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, maybe a comment about that?

}

/// Encoded key for storage lookup
fn encoded_key(&self) -> ElasticArray36<u8> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not also encoded somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a method of TrieDBIterator, which manages current key as a vector of nibbles. I considered switching to NibbleVec instead, that uses compact form and can be trivially converted to NibbleSlice. Unfortunately the conversion would only work for even number of nibbles, and it is not trivial to fix that.

@arkpar arkpar merged commit 0d914ac into master Mar 28, 2019
@arkpar arkpar deleted the a-fix-iteration branch March 28, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants