Trie (non)-membership proofs #7509
Conversation
There was a problem hiding this comment.
The two things we still need is the change to iterator I’ve mentioned which would replace seek with something like seek_prefix and cause the iterator to stop as soon as it’s done with the prefix and addition of include_proof field to the request. Those two could be done with separate PRs to make it simpler to manage.
| fn retrieve_node(&self, hash: &CryptoHash) -> Result<TrieNodeWithSize, StorageError> { | ||
| /// Retrieves decoded node alongside with its raw bytes representation. | ||
| /// | ||
| /// Note that because Empty nodes (those which are referenced by | ||
| /// [`Self::EMPTY_ROOT`] hash) aren’t stored in the database, they don’t | ||
| /// have a bytes representation. For those nodes the first return value | ||
| /// will be `None`. | ||
| fn retrieve_node( | ||
| &self, | ||
| hash: &CryptoHash, | ||
| ) -> Result<(Option<std::sync::Arc<[u8]>>, TrieNodeWithSize), StorageError> { | ||
| match self.retrieve_raw_node(hash)? { | ||
| None => Ok(TrieNodeWithSize::empty()), | ||
| Some((_bytes, node)) => Ok(TrieNodeWithSize::from_raw(node)), | ||
| None => Ok((None, TrieNodeWithSize::empty())), | ||
| Some((bytes, node)) => Ok((Some(bytes), TrieNodeWithSize::from_raw(node))), | ||
| } | ||
| } |
There was a problem hiding this comment.
It would be best to push this change as separate PR. That would make this PR smaller and easier to understand.
There was a problem hiding this comment.
I'm unsure of what to do here.
There was a problem hiding this comment.
It’s just a matter of separating this diff hunk and all the places where retrieve_node is called into a new commit and submitting as separate PR. It will get this PR smaller.
There was a problem hiding this comment.
it's not super trivial as far as I see. Need to also modify descend_into_node. Can't we just keep it as it is?
There was a problem hiding this comment.
It should be rather simple mechanical change. In that separate PR, the change to desdend_into_node will be change from self.trie.retrieve_node(hash)? to self.trie.retrieve_node(hash)?.1.
There was a problem hiding this comment.
That will happen in this PR.
There was a problem hiding this comment.
sorry @mina86 but I'm truly not following you on this one
There was a problem hiding this comment.
There are three separate PRs here:
- PR changing retrieve_node to return (bytes, TrieNodeWithSize). All that PR does is changes the return type and updates all the call sites to do
retrieve_node(...)?.1. This is quite mechanical and boring change but thanks to that it should be easy to write, review and merge. This PR is not concerned with the overall feature. - PR changing TrieIterator::seek to TrieIterator::seek_prefix and makes changes the iterator such that it stops once everything within the prefix has been traversed. This can be done by adding a
prefix_boundary: booltoCrumband changing Crumb::increment such that if that flag is set, it always moves to Exiting status. The purpose of this change is to make sure that when iterating over a prefix, the iterator does not fetch spurious nodes. Currently I believe that returned proof includes unnecessary nodes. There may be some smarter way of dealing with this but I’m not seeing it just now. - Once 1 and 2 are done, this PR can be rebased and will be ready to go.
1 and 2 can be done in parallel.
There was a problem hiding this comment.
For (2) I've created ComposableFi#11 Can you please review it there?
Required to merge near#7509
This will be needed in future commit where we will need access to the raw bytes of the node. See #7509.
|
Finally, I think we’re on the home straight. \o/ Once #7585 gets merged this can be merged or rebased and I think everything here should be fine. There’s only one detail I’m still wondering about. JSON RPC is public API so I want to get it right the first time around ;) I think we do want an option to enable or disable including the proof. So that would be done by adding |
|
You’re gonna need to allow maintainers to edit here as well. It’s just how our merge bot operates. Otherwise it’s kinda annoying to merge manually. |
|
Opening it up again from my own repo |
This will be needed in future commit where we will need access to the raw bytes of the node. See #7509.
Adding changes suggested by @mina86
Following up the discussion from #7205