Skip to content

Fix iteration for prefixed storage #15

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.

Sign up for GitHub

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

Merged
arkpar merged 3 commits into from
Mar 28, 2019
Merged

Fix iteration for prefixed storage #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bede10
Fixed iteration for prefixed storage
arkpar Mar 28, 2019
4031493
Version bump
arkpar Mar 28, 2019
21ff06e
A bit of documentation
arkpar Mar 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion trie-db/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "trie-db"
version = "0.12.0"
version = "0.12.1"
authors = ["Parity Technologies <[email protected]>"]
description = "Merkle-Patricia Trie generic over key hasher and node encoding"
repository = "https://github.com/paritytech/parity-common"
Expand Down
50 changes: 32 additions & 18 deletions trie-db/src/triedb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,13 @@ where
.ok_or_else(|| Box::new(TrieError::InvalidStateRoot(*self.root)))
}

/// Given some node-describing data `node`, return the actual node RLP.
/// Given some node-describing data `node`, and node key return the actual node RLP.
/// This could be a simple identity operation in the case that the node is sufficiently small, but
/// may require a database lookup. If `is_root_data` then this is root-data and
/// is known to be literal.
/// `partial_key` is encoded nibble slice that addresses the node.
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

@rphmeier rphmeier Mar 28, 2019

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 •
edited
Loading

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

@rphmeier rphmeier Mar 28, 2019

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?

(false, Some(key)) => {
self.db
.get(&key, partial_key)
Expand Down Expand Up @@ -274,7 +275,7 @@ impl<'a, H: Hasher, C: NodeCodec<H>> TrieDBIterator<'a, H, C> {
let mut partial = key;
let mut full_key_nibbles = 0;
loop {
let (data, mid) = {
let data = {
let node = C::decode(&node_data).expect("encoded data read from db; qed");
match node {
Node::Leaf(slice, _) => {
Expand All @@ -300,8 +301,10 @@ impl<'a, H: Hasher, C: NodeCodec<H>> TrieDBIterator<'a, H, C> {
node: node.clone().into(),
});
self.key_nibbles.extend(slice.iter());
full_key_nibbles += slice.len();
partial = partial.mid(slice.len());
let data = self.db.get_raw_or_lookup(&*item, &key.encoded_leftmost(full_key_nibbles, false))?;
(data, slice.len())
data
} else {
self.descend(&node_data)?;
return Ok(())
Expand All @@ -322,9 +325,11 @@ impl<'a, H: Hasher, C: NodeCodec<H>> TrieDBIterator<'a, H, C> {
node: node.clone().into(),
});
self.key_nibbles.push(i);
full_key_nibbles += 1;
partial = partial.mid(1);
if let Some(ref child) = nodes[i as usize] {
let child = self.db.get_raw_or_lookup(&*child, &key.encoded_leftmost(full_key_nibbles, false))?;
(child, 1)
child
} else {
return Ok(())
}
Expand All @@ -335,14 +340,12 @@ impl<'a, H: Hasher, C: NodeCodec<H>> TrieDBIterator<'a, H, C> {
};

node_data = data;
full_key_nibbles += mid;
partial = partial.mid(mid);
}
}

/// Descend into a payload.
fn descend(&mut self, d: &[u8]) -> Result<(), H::Out, C::Error> {
let node_data = &self.db.get_raw_or_lookup(d, &self.key_nibbles)?;
let node_data = &self.db.get_raw_or_lookup(d, &self.encoded_key())?;
let node = C::decode(&node_data).expect("encoded node read from db; qed");
Ok(self.descend_into_node(node.into()))
}
Expand Down Expand Up @@ -371,6 +374,17 @@ impl<'a, H: Hasher, C: NodeCodec<H>> TrieDBIterator<'a, H, C> {
}
result
}

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

@rphmeier rphmeier Mar 28, 2019

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

@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.

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.

let key = self.key();
let slice = NibbleSlice::new(&key);
if self.key_nibbles.len() % 2 == 1 {
NibbleSlice::new_composed(&slice, &NibbleSlice::new_offset(&self.key_nibbles[(self.key_nibbles.len() - 1)..], 1)).encoded(false)
} else {
slice.encoded(false)
}
}
}

impl<'a, H: Hasher, C: NodeCodec<H>> TrieIterator<H, C> for TrieDBIterator<'a, H, C> {
Expand Down Expand Up @@ -417,7 +431,7 @@ impl<'a, H: Hasher, C: NodeCodec<H>> Iterator for TrieDBIterator<'a, H, C> {
return Some(Ok((self.key(), v.clone())));
},
(Status::At, &OwnedNode::Extension(_, ref d)) => {
IterStep::Descend::<H::Out, C::Error>(self.db.get_raw_or_lookup(&*d, &self.key_nibbles))
IterStep::Descend::<H::Out, C::Error>(self.db.get_raw_or_lookup(&*d, &self.encoded_key()))
},
(Status::At, &OwnedNode::Branch(_)) => IterStep::Continue,
(Status::AtChild(i), &OwnedNode::Branch(ref branch)) if branch.index(i).is_some() => {
Expand All @@ -428,7 +442,7 @@ impl<'a, H: Hasher, C: NodeCodec<H>> Iterator for TrieDBIterator<'a, H, C> {
}
IterStep::Descend::<H::Out, C::Error>(self.db.get_raw_or_lookup(
&branch.index(i).expect("this arm guarded by branch[i].is_some(); qed"),
&self.key_nibbles))
&self.encoded_key()))
},
(Status::AtChild(i), &OwnedNode::Branch(_)) => {
if i == 0 {
Expand Down Expand Up @@ -459,7 +473,7 @@ impl<'a, H: Hasher, C: NodeCodec<H>> Iterator for TrieDBIterator<'a, H, C> {

#[cfg(test)]
mod tests {
use memory_db::{MemoryDB, HashKey};
use memory_db::{MemoryDB, PrefixedKey};
use keccak_hasher::KeccakHasher;
use DBValue;
use reference_trie::{RefTrieDB, RefTrieDBMut, RefLookup, Trie, TrieMut, NibbleSlice};
Expand All @@ -471,7 +485,7 @@ mod tests {
(hex!("0103000000000000000469").to_vec(), hex!("ffffffffff").to_vec()),
];

let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
{
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand Down Expand Up @@ -499,7 +513,7 @@ mod tests {
(hex!("0103000000000000000469").to_vec(), hex!("ffffffffff").to_vec()),
];

let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
{
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand All @@ -523,7 +537,7 @@ mod tests {
fn iterator() {
let d = vec![DBValue::from_slice(b"A"), DBValue::from_slice(b"AA"), DBValue::from_slice(b"AB"), DBValue::from_slice(b"B")];

let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
{
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand All @@ -541,7 +555,7 @@ mod tests {
fn iterator_seek() {
let d = vec![ DBValue::from_slice(b"A"), DBValue::from_slice(b"AA"), DBValue::from_slice(b"AB"), DBValue::from_slice(b"B") ];

let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
{
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand Down Expand Up @@ -580,7 +594,7 @@ mod tests {

#[test]
fn get_len() {
let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
{
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand All @@ -598,7 +612,7 @@ mod tests {
fn debug_output_supports_pretty_print() {
let d = vec![ DBValue::from_slice(b"A"), DBValue::from_slice(b"AA"), DBValue::from_slice(b"AB"), DBValue::from_slice(b"B") ];

let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
let root = {
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand Down Expand Up @@ -666,7 +680,7 @@ mod tests {
fn test_lookup_with_corrupt_data_returns_decoder_error() {
use std::marker::PhantomData;

let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, DBValue>::default();
let mut memdb = MemoryDB::<KeccakHasher, PrefixedKey<_>, DBValue>::default();
let mut root = Default::default();
{
let mut t = RefTrieDBMut::new(&mut memdb, &mut root);
Expand Down