Fix proof generation and verification code #412
Conversation
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
jsign
force-pushed
the
jsign-tree-reconstr-extra-checks
branch
from
ba74c56 to
96db99c
Compare
* proof: add proof of absence border case tests Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * Proof of absence and presence for the same path with nil proof of presence value (#414) * keep the extension statuses of absent stems Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * more improvements and tests Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * fix & extra test case Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * make steams and extStatuses be 1:1 Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * cleanup Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
jsign
changed the title
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
jsign
force-pushed
the
jsign-tree-reconstr-extra-checks
branch
from
156d92b to
386c596
Compare
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
| addedStems := map[string]struct{}{} | ||
| for i := 0; i < len(group); i++ { | ||
| if _, ok := addedStems[string(group[i][:StemSize])]; !ok { | ||
| // A question arises here: what if this proof of absence | ||
| // corresponds to several stems? Should the ext status be | ||
| // repeated as many times? It's wasteful, so consider if the | ||
| // decoding code can be aware of this corner case. | ||
| esses = append(esses, extStatusAbsentEmpty|((n.depth+1)<<3)) | ||
| addedStems[string(group[i][:StemSize])] = struct{}{} | ||
| } |
There was a problem hiding this comment.
I understand the point of the comment, but unfortunately it isn't that easy to do this without buying again all the corner cases.
I think the comment is still a fair take on the situation, so that's why I'm leaving it here, but if somebody wants to propose doing that optimization, it should:
- Do the optimization
- Add tests for that optimization
- Run all Kaustinen to triple-check stuff
Of course, the complicated point here is to asses if the amount of bytes we'd save in a real chain is meaningful for the complexity we'd be adding. (Since this is proposing breaking the 1:1 between stems and extension statuses).
tree.go
Outdated
| // Proof of absence: case of a missing suffix tree. | ||
| // | ||
| // The suffix tree for this value is missing, i.e. all | ||
| // values in the extension-and-suffix tree are grouped | ||
| // in the other suffix tree (e.g. C2 if we are looking | ||
| // at C1). | ||
| if count == 0 { | ||
| // TODO(gballet) maintain a count variable at LeafNode level | ||
| // so that we know not to build the polynomials in this case, | ||
| // as all the information is available before fillSuffixTreePoly | ||
| // has to be called, save the count. | ||
| esses = append(esses, extStatusAbsentEmpty|(n.depth<<3)) | ||
| pe.Vals = append(pe.Vals, nil) | ||
| continue | ||
| } | ||
| _ = count | ||
| // // Proof of absence: case of a missing suffix tree. | ||
| // // | ||
| // // The suffix tree for this value is missing, i.e. all | ||
| // // values in the extension-and-suffix tree are grouped | ||
| // // in the other suffix tree (e.g. C2 if we are looking | ||
| // // at C1). | ||
| // if count == 0 { | ||
| // // TODO(gballet) maintain a count variable at LeafNode level | ||
| // // so that we know not to build the polynomials in this case, | ||
| // // as all the information is available before fillSuffixTreePoly | ||
| // // has to be called, save the count. | ||
| // esses = append(esses, extStatusAbsentEmpty|(n.depth<<3)) | ||
| // pe.Vals = append(pe.Vals, nil) | ||
| // continue | ||
| // } |
There was a problem hiding this comment.
I'm not sure this was the cause of issues, but definitely I think we need to chat about this.
This was adding a extStatusAbsentEmpty, and that's very tricky.
I totally understand the intention that if we are proving a single key which lies in an empty half of the leaf node, then we could use extStatusAbsentEmpty since the verifier could assume there's no LeafNode here and get a nil.
Note that this is a trick and not representative of the real tree. The real tree has a LeafNode, but for this particular situation we can "simulate" that doesn't.
Let's assume for a moment we're OK with that.
Still, the code here isn't doing things correctly. We can add this extStatusAbsentEmpty if there's a single key proven in this way. As in, if for a key we did this addition of extStatusAbsentEmpty, and then the next key proves the presence in a non-empty half, then we should definitely removed that extStatusAbsentEmpty thing... if not, we'd have more than one extension status for keys of this leaf.
Maybe it's something that we can try to optimize. But it's not correct to simply add this empty extension status. We need to have some internal state to track that we did that, and if anything else happens in a next key (i.e: proof of absence other, or presence) then we need to revert that addition.
We can keep chatting in Matrix about this. I just wanted to leave some comment since I have this fresh in my mind now.
[I left this code commented to remember we need to take a decision. If we decide to remove this optimization idea, I will remove the code]
There was a problem hiding this comment.
Still, the code here isn't doing things correctly. We can add this extStatusAbsentEmpty if there's a single key proven in this way. As in, if for a key we did this addition of extStatusAbsentEmpty, and then the next key proves the presence in a non-empty half, then we should definitely removed that extStatusAbsentEmpty thing... if not, we'd have more than one extension status for keys of this leaf.
Yes, the extra empty was being removed if a proof of presence was found. That was line 1474, when setting esses = nil.
It's ok, let's keep things simple and have one extension status per stem.
| if len(esses) == 0 || esses[len(esses)-1] != extStatusPresent|(n.depth<<3) { | ||
|
|
||
| if _, ok := addedStems[string(key[:StemSize])]; !ok { | ||
| esses = append(esses, extStatusPresent|(n.depth<<3)) | ||
| addedStems[string(key[:StemSize])] = struct{}{} | ||
| } |
There was a problem hiding this comment.
This previous (deleted) if was wrong. Now we always need to add the extension status for the present stem. (That len(esses) == 0 is not correct, since we could have proof of absence extensions status now).
In short, we simply add once the extension status present.
This PR: