Skip to content
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

Proof of absence border case test #413

Merged
merged 2 commits into from
Nov 3, 2023

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented Nov 1, 2023

This PR adds an extra tests case that proves, what I consider, an unfortunate flaw in an optimization included a while ago ~around #188.

The explanation is that if we have a proof of absence and a proof of presence in the same leaf node, and that proof of absence is for a nil value, we get in a situation in which we can't figure out the appropriate stem.

@jsign jsign changed the base branch from master to jsign-tree-reconstr-extra-checks November 1, 2023 13:51
@@ -1088,3 +1088,40 @@ func TestGenerateProofWithOnlyAbsentKeys(t *testing.T) {
}
}
}

func TestProofOfPresenceWithEmptyValue(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is pretty simple, and I believe shows a potentially fundamental flaw of the optimization done compared to the spec reference regarding "leaving out" the extension statuses for stems that have both proof of presence and absence.

The test case is simple:

  • Add a key with a value.
  • Prove that another key that corresponds to that LeafNode has value nil.

This fails for what looks like a simple to fix bug, but after you do the obvious fix it will be apparent that that will break another test and the fundamental issue will become more apparent.

I can explain more in Matrix.

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@jsign jsign force-pushed the jsign-proof-tests branch from d3dee8c to 946621d Compare November 2, 2023 16:19
@gballet gballet marked this pull request as ready for review November 3, 2023 09:44
@gballet gballet marked this pull request as draft November 3, 2023 09:44
@gballet gballet mentioned this pull request Nov 3, 2023
…sence 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>
@jsign jsign marked this pull request as ready for review November 3, 2023 16:43
@jsign jsign merged commit 6465478 into jsign-tree-reconstr-extra-checks Nov 3, 2023
5 of 6 checks passed
gballet pushed a commit that referenced this pull request Nov 6, 2023
* proof: verify proof of absence steam is sorted

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* proof: more length checks

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* more tree from proof checks

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* add checks in CreatePath

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* lints

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* fix comments

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* Proof of absence border case test (#413)

* 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>

* lint

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* add extra border case test

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* fix bug

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* further fixes

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>

* fix

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant