Skip to content

Commit

Permalink
Add hash of the node to the MissingTrieValue
Browse files Browse the repository at this point in the history
  • Loading branch information
staffik committed Sep 29, 2023
1 parent 65af455 commit 3c10e68
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 40 deletions.
3 changes: 1 addition & 2 deletions core/primitives/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ pub enum StorageError {
/// Key-value db internal failure
StorageInternalError,
/// Requested trie value by its hash which is missing in storage.
/// TODO (#8997): consider including hash of trie node.
MissingTrieValue,
MissingTrieValue(String),
/// Found trie node which shouldn't be part of state. Raised during
/// validation of state sync parts where incorrect node was passed.
/// TODO (#8997): consider including hash of trie node.
Expand Down
2 changes: 1 addition & 1 deletion core/store/src/trie/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ impl<'a> TrieIterator<'a> {
fn descend_into_node(&mut self, hash: &CryptoHash) -> Result<(), StorageError> {
let (bytes, node) = self.trie.retrieve_node(hash)?;
if let Some(ref mut visited) = self.visited_nodes {
visited.push(bytes.ok_or(StorageError::MissingTrieValue)?);
visited.push(bytes.ok_or(StorageError::MissingTrieValue(format!("Trie value missing in storage by node hash {hash}")))?);
}
self.trail.push(Crumb { status: CrumbStatus::Entering, node, prefix_boundary: false });
Ok(())
Expand Down
8 changes: 7 additions & 1 deletion core/store/src/trie/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1459,7 +1459,13 @@ mod tests {

assert_eq!(trie3.get(b"dog"), Ok(Some(b"puppy".to_vec())));
assert_eq!(trie3.get(b"horse"), Ok(Some(b"stallion".to_vec())));
assert_eq!(trie3.get(b"doge"), Err(StorageError::MissingTrieValue));

match trie3.get(b"doge") {
Err(StorageError::MissingTrieValue(msg)) => {
assert!(msg.starts_with("Trie memory recorded storage does not have node by hash"))
}
_ => panic!("Unexpected error type"),
}
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion core/store/src/trie/prefetching_trie_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ impl TrieStorage for TriePrefetchingStorage {
// Releasing the lock here to unstuck main thread if it
// was blocking on this value, but it will also fail on its read.
self.prefetching.release(hash);
Err(StorageError::MissingTrieValue)
Err(StorageError::MissingTrieValue(format!("Hash found in the trie without node: {hash}")))
}
Err(e) => {
// This is an unrecoverable IO error.
Expand Down
67 changes: 39 additions & 28 deletions core/store/src/trie/state_parts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1070,10 +1070,13 @@ mod tests {
let mut trie_values_missing = trie_values.clone();
trie_values_missing.remove(num_trie_values / 2);
let wrong_state_part = PartialState::TrieValues(trie_values_missing);
assert_eq!(
Trie::validate_state_part(&root, part_id, wrong_state_part),
Err(StorageError::MissingTrieValue)
);

match Trie::validate_state_part(&root, part_id, wrong_state_part) {
Err(StorageError::MissingTrieValue(msg)) => {
assert!(msg.starts_with("Trie memory recorded storage does not have node by hash"))
}
_ => panic!("Unexpected error type"),
}

// Add extra value to the state part, check that validation fails.
let mut trie_values_extra = trie_values.clone();
Expand Down Expand Up @@ -1176,16 +1179,19 @@ mod tests {

let view_chunk_trie =
tries.get_trie_with_block_hash_for_shard(shard_uid, root, &block_hash, true);
assert_eq!(
view_chunk_trie.get_trie_nodes_for_part_with_flat_storage(
part_id,
partial_state,
nibbles_begin,
nibbles_end,
&trie_without_flat,
),
Err(StorageError::MissingTrieValue)
);

match view_chunk_trie.get_trie_nodes_for_part_with_flat_storage(
part_id,
partial_state,
nibbles_begin,
nibbles_end,
&trie_without_flat,
) {
Err(StorageError::MissingTrieValue(msg)) => {
assert!(msg.starts_with("Trie memory recorded storage does not have node by hash"))
}
_ => panic!("Unexpected error type"),
}

// Fill flat storage and check that state part creation succeeds.
let changes_for_delta =
Expand Down Expand Up @@ -1219,10 +1225,13 @@ mod tests {
store_update.decrement_refcount(DBCol::State, &store_key);
store_update.commit().unwrap();

assert_eq!(
trie_without_flat.get_trie_nodes_for_part_without_flat_storage(part_id),
Err(StorageError::MissingTrieValue)
);
match trie_without_flat.get_trie_nodes_for_part_without_flat_storage(part_id) {
Err(StorageError::MissingTrieValue(msg)) => {
assert_eq!(msg, format!("Database missing trie node by hash {value_hash}"));
}
_ => panic!("Unexpected error type"),
}

assert_eq!(
view_chunk_trie.get_trie_nodes_for_part_with_flat_storage(
part_id,
Expand All @@ -1242,15 +1251,17 @@ mod tests {
delta.apply_to_flat_state(&mut store_update, shard_uid);
store_update.commit().unwrap();

assert_eq!(
view_chunk_trie.get_trie_nodes_for_part_with_flat_storage(
part_id,
partial_state,
nibbles_begin,
nibbles_end,
&trie_without_flat,
),
Err(StorageError::MissingTrieValue)
);
match view_chunk_trie.get_trie_nodes_for_part_with_flat_storage(
part_id,
partial_state,
nibbles_begin,
nibbles_end,
&trie_without_flat,
) {
Err(StorageError::MissingTrieValue(msg)) => {
assert!(msg.starts_with("Trie memory recorded storage does not have node by hash"))
}
_ => panic!("Unexpected error type"),
}
}
}
6 changes: 4 additions & 2 deletions core/store/src/trie/trie_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,9 @@ pub struct TrieMemoryPartialStorage {

impl TrieStorage for TrieMemoryPartialStorage {
fn retrieve_raw_bytes(&self, hash: &CryptoHash) -> Result<Arc<[u8]>, StorageError> {
let result = self.recorded_storage.get(hash).cloned().ok_or(StorageError::MissingTrieValue);
let result = self.recorded_storage.get(hash).cloned().ok_or(
StorageError::MissingTrieValue(format!("Trie memory recorded storage does not have node by hash {hash}"))
);
if result.is_ok() {
self.visited_nodes.borrow_mut().insert(*hash);
}
Expand Down Expand Up @@ -532,7 +534,7 @@ fn read_node_from_db(
let val = store
.get(DBCol::State, key.as_ref())
.map_err(|_| StorageError::StorageInternalError)?
.ok_or_else(|| StorageError::MissingTrieValue)?;
.ok_or_else(|| StorageError::MissingTrieValue(format!("Database missing trie node by hash {hash}")))?;
Ok(val.into())
}

Expand Down
18 changes: 13 additions & 5 deletions core/store/src/trie/trie_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl TrieStorage for IncompletePartialStorage {
self.visited_nodes.borrow_mut().insert(*hash);

if self.visited_nodes.borrow().len() > self.node_count_to_fail_after {
Err(StorageError::MissingTrieValue)
Err(StorageError::MissingTrieValue(format!("Recorded storage is missing hash {hash}")))
} else {
Ok(result)
}
Expand Down Expand Up @@ -77,9 +77,17 @@ where
for i in 0..(size + 1) {
let storage = IncompletePartialStorage::new(storage.clone(), i);
let new_trie = Trie::new(Rc::new(storage), *trie.get_root(), None);
let expected_result =
if i < size { Err(&StorageError::MissingTrieValue) } else { Ok(&expected) };
assert_eq!(test(new_trie).map(|v| v.1).as_ref(), expected_result);
let result = test(new_trie).map(|v| v.1);
if i < size {
match result {
Err(StorageError::MissingTrieValue(msg)) => {
assert!(msg.starts_with("Recorded storage is missing hash"));
}
_ => panic!("Unexpected error type"),
}
} else {
assert_eq!(result.as_ref(), Ok(&expected));
}
}
println!("Success");
}
Expand Down Expand Up @@ -274,7 +282,7 @@ mod trie_storage_tests {
let key = hash(&value);

let result = trie_caching_storage.retrieve_raw_bytes(&key);
assert_matches!(result, Err(StorageError::MissingTrieValue));
assert_matches!(result, Err(StorageError::MissingTrieValue(_)));
}

/// Check that large values does not fall into shard cache, but fall into accounting cache.
Expand Down

0 comments on commit 3c10e68

Please sign in to comment.