refactor: SMT path iter uses leaf key #482
Conversation
|
Converted back to draft, since I forgot to update the CHANGELOG. Will do that now! |
| return Ok(()); | ||
| } | ||
|
|
||
| let leaf_node = Node::create_leaf(key, data); | ||
| self.storage.insert(&leaf_node.hash(), &leaf_node.as_ref().into())?; | ||
| self.storage.insert(leaf_node.leaf_key(), &leaf_node.as_ref().into())?; |
There was a problem hiding this comment.
This was the line that introduced the potential for malicious collision
| if self.root() == *Self::empty_root() { | ||
| // The zero root signifies that all leaves are empty, including the | ||
| // given key. | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if let Some(primitive) = self.storage.get(key)? { | ||
| let leaf_node = Node::create_leaf(key, data); | ||
| if let Some(primitive) = self.storage.get(&leaf_node.hash())? { |
There was a problem hiding this comment.
Actually, I checked the code, and it seems you don't even need a value. Because self.path_set and self.delete_with_path_set use only leaf_key inside the logic.
There was a problem hiding this comment.
Does this mean we wouldn't need to pass the data here?
There was a problem hiding this comment.
Interesting, I can explore that further
There was a problem hiding this comment.
Unfortunately, this doesn't work: We can't avoid the storage lookup here because we need to know if the deleted item is in the set of leaves or not. Whenever we call delete, we will modify all items along the leaf path. If we never added the item, we end up "reverting" a leaf path that was never actually changed, thus creating a false root. I got far enough into the changes in a PR here #483, but the tests with delete are failing for this reason.
Specifically:
failures:
---- sparse::merkle_tree::test::test_delete_non_existent_key stdout ----
thread 'sparse::merkle_tree::test::test_delete_non_existent_key' panicked at 'assertion failed: `(left == right)`
left: `"8e87e7b530e86dda2bd6705754af66f4320105e20b5fc8c5a5347064d7cc5bf2"`,
right: `"108f731f2414e33ae57e584dc26bd276db07874436b2264ca6e520c658185c6b"`', fuel-merkle/src/sparse/merkle_tree.rs:552:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- sparse::merkle_tree::test::test_interleaved_update_delete stdout ----
thread 'sparse::merkle_tree::test::test_interleaved_update_delete' panicked at 'assertion failed: `(left == right)`
left: `"c37ab7fc94e589545364e797b952a2eadbbbdcf182fe770fc2253fa2d2d36f5d"`,
right: `"7e6643325042cfe0fc76626c043b97062af51c7e9fc56665f12b479034bce326"`', fuel-merkle/src/sparse/merkle_tree.rs:592:9
failures:
sparse::merkle_tree::test::test_delete_non_existent_key
sparse::merkle_tree::test::test_interleaved_update_delete
We need to be able to short-circuit the delete call before modifying the nodes along the leaf path. As far as I am aware, the only way to do this is by checking the storage. In order to check the storage, we need to know the leaf data.
If there is another way we can determine whether or not a leaf is in the set, we can do that.
Maybe we can try:
- Storing the list of leaves
- Bloom filter (won't be reliable though)
- ???
There was a problem hiding this comment.
@xgreenx observed that we can check for the leaf by querying the path set, allowing us to properly check the delete 👍
* Refactor path iter to use leaf key * compile/clippy * Fix * Improve --------- Co-authored-by: green <xgreenx9999@gmail.com>
bvrooman
changed the title
|
|
Related issues:
This PR removes the dependency on storage lookups for the leaf key during deletion. During deletion, we no longer need to look up leafs by leaf keys in storage because we can perform path iteration without the whole leaf: the leaf key is sufficient. This PR refactors the iterator to use a leaf key rather than the entire leaf.