-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Support for null keys #39
Conversation
src/lib.rs
Outdated
); | ||
assert_eq!( | ||
proof.keyvals, | ||
vec![[210, 144, 49, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 128]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the leaf with key 11111111111111110000000000000000
should be sent in proof.keyvals
. That's the only way the verifier can make sure that the top branch node is pointing only to one leaf, which is not the leaf we're looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, it seems for supporting null keys #25 is a requirement.
Update: For now though we can pass in a list of keys along with the multiproof and assume these keys can be trusted. Then rebuild
can check that the leaf's partial key diverges from the expected key and can assume that the leaf we're looking for doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, 11111111111111110000000000000000
should be present, and that's (part of) the bug.
I didn't get this part, can you maybe re-phrase? |
Let's assume that you have two keys key in the tree: You want to prove that when it could have created this tree: |
Showing off that diagram generator, are we 😎 😄 Hm, in my opinion both of those last two diagrams are invalid (i.e. verifier can't verify non-existence of |
src/lib.rs
Outdated
.to_string()); | ||
} | ||
instructions.push(Instruction::LEAF(keys[0].len())); | ||
rlp::encode(&Leaf(keys[0].clone(), vec![])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, 11111111111111110000000000000000 should be present, and that's (part of) the bug.
Well here it could be (similar to above) rlp::encode(&Leaf(leafkey.clone(), leafval.clone()))
to include 11..00..
Maybe this if *leafkey == keys[0] {
condition is not necessary at all. Whatever the path, we include this leaf fully.
You bet, I'm proud 😊
The first one is a simplification, with the idea that 0x1111 yields an
Well that's already what it does in the current state (more accurately: this is what it should be doing if it wasn't buggy) but why would you want to increase the proof size by adding some extra info? Is there something I'm missing on the verification side? |
So for example in this case of this rebuilt trie, the root hash will be different than expected by the verifier: The verifier should have the branch node and see for itself that there's no child with nibble 1, and that the hashes also match (i.e. this is the valid trie). |
I think I get the argument on why you should add all the data, however in the case of this example we can't use the Then it has a different root than that of In fact, in that case the original tree is the correct proof (if you assume that In the case of terminal nodes in which the value is missing (so in that case, if |
Hey I was tinkering with this and saw you've added a lot of good stuff! I think rebuilding the trie and then querying to see that a key is null was a good trick. Without that the verification gets harder and it is full of edge cases... I'll have to prototype a version of turbo-token which simply rebuilds the trie and queries the keys and see how that performs |
This is a rewrite after many discussions, of how make_multiproof handles keys that are not present in the tree. * If a search encounters an EmptySlot then the key is considered missing and an (empty key, empty value) is inserted to point out that the key is not present in the tree. * In the case of an extension where the key and the extension part differ in at least one nibble, the hash of the extension's child is added and the full extension node is added to the proof, so prove that the tree has a different path and therefore not the one pointed by the missing key. If the missing key and extension have the same nibbles, the algorithm recurses. * In the case of a branch node, either the first byte isn't available in which case nothing is done since an EmptySlot clearly indicates that the key is missing. Otherwise, the algorithm recurses. * In the case of a leaf, the leaf that is present is added to prove that the other key is missing. Rebuilding the tree will work like before, except in the case of an empty (key, value): in that case an EmptySlot is inserted instead of raising an error. A function that checks if a key is present in the tree is also added. There is also the start of an implementation of the [] accessor for Node, which currently only works with Branch nodes.
let factor_length = extkey.factor_length(k); | ||
// If a key has a prefix that differs from that of the extension, | ||
// then it is missing in this tree and is not added to the list | ||
// of shortened keys to be recursively passed down. This special |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This special case is handled after the loop.
Where? 😄
This addresses #28 and adds the possibility to provide
make_multiproof
with keys that are not present in the tree. The result will be a leaf with no value, meant to represent anull
key. The tree will be rebuilt by replacing that key with anEmptySlot
.There are currently two problems with this PR, which is created for discussion purposes:
extension node > branch node with only empty slots
. This is actually a valid representation, yet it's not the most efficient one.