Skip to content

Commit

Permalink
fix: sparse Merkle tree key querying (tari-project#5566)
Browse files Browse the repository at this point in the history
Description
---
Fixes key querying on sparse Merkle trees. Adds a test. Closes tari-project#5565.

Motivation and Context
---
It's possible to ask a sparse Merkle tree if it
[contains](https://github.com/tari-project/tari/blob/87c070305951152c62a0179e13fadc55065cc318/base_layer/mmr/src/sparse_merkle_tree/tree.rs#L256-L259)
a particular key. However, the query returns `true` whether or not the
key is in the tree.

This PR fixes the incorrect behavior. The query returns `true` if and
only if the tree search yields a leaf node with the given key. If the
tree is malformed such that the search returns an error, the query fails
safe and returns `false`.

Interestingly, the existing unit tests did not detect the incorrect
behavior, as no test checked for the case where a key did not exist in a
tree. We add a test that checks this for a few cases of interest.

How Has This Been Tested?
---
Existing tests pass. A new test asserts the expected behavior (and would
have caught the previous incorrect behavior).

What process can a PR reviewer use to test or verify this change?
---
Confirm that the detection logic in the `contains` function is correct.

Breaking Changes
---
None.
  • Loading branch information
AaronFeickert committed Jul 3, 2023
1 parent 61a1d4d commit 623839f
Showing 1 changed file with 42 additions and 1 deletion.
43 changes: 42 additions & 1 deletion base_layer/mmr/src/sparse_merkle_tree/tree.rs
Expand Up @@ -255,7 +255,14 @@ impl<H: Digest<OutputSize = U32>> SparseMerkleTree<H> {

/// Returns true if the tree contains the key `key`.
pub fn contains(&self, key: &NodeKey) -> bool {
self.search_node(key).ok().is_some()
match self.search_node(key) {
// In the case of a malformed tree where the search fails unexpectedly, play it safe
Err(_) => false,
// The node is either empty or is a leaf with an unexpected key
Ok(None) => false,
// The node is a leaf with the expected key
Ok(Some(_)) => true,
}
}

/// Returns the value at location `key` if it exists, or `None` otherwise.
Expand Down Expand Up @@ -806,4 +813,38 @@ mod test {
let _ = tree.delete(&short_key(65)).unwrap();
assert!(tree.is_empty());
}

#[test]
fn contains() {
let mut tree = SparseMerkleTree::<Blake256>::default();

// An empty tree contains no keys
assert!(!tree.contains(&short_key(0)));
assert!(!tree.contains(&short_key(1)));

// Add a key, which the tree must then contain
tree.upsert(short_key(1), ValueHash::from([1u8; 32])).unwrap();
assert!(!tree.contains(&short_key(0)));
assert!(tree.contains(&short_key(1)));

// Delete the key, which the tree must not contain
tree.delete(&short_key(1)).unwrap();
assert!(!tree.contains(&short_key(0)));
assert!(!tree.contains(&short_key(1)));

// Build a more complex tree with two keys, which the tree must then contain
tree.upsert(short_key(0), ValueHash::from([0u8; 32])).unwrap();
tree.upsert(short_key(1), ValueHash::from([1u8; 32])).unwrap();
assert!(tree.contains(&short_key(0)));
assert!(tree.contains(&short_key(1)));

// Delete each key in turn
tree.delete(&short_key(0)).unwrap();
assert!(!tree.contains(&short_key(0)));
tree.delete(&short_key(1)).unwrap();
assert!(!tree.contains(&short_key(1)));

// Sanity check that the tree is now empty
assert!(tree.is_empty());
}
}

0 comments on commit 623839f

Please sign in to comment.