fix absent keys in proofs #401

jsign · 2023-10-15T22:38:02Z

This PR proposes a fix to #400.

It fixes two things:

Verifier fix: rebuilding the tree from a proof that has an absent key (other) case.
Prover fix: leaf nodes shouldn't open C1/C2 for keys that don't match the leaf steam.

tree.go

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

jsign · 2023-10-16T19:50:45Z

doc.go

@@ -35,6 +35,7 @@ var (
 	errInsertIntoOtherStem    = errors.New("insert splits a stem where it should not happen")
 	errUnknownNodeType        = errors.New("unknown node type detected")
 	errMissingNodeInStateless = errors.New("trying to access a node that is missing from the stateless view")
+	errIsPOAStub              = errors.New("trying to read/write a proof of absence leaf node")


I created a new error that is returned whenever we try to read/write a leaf node that is only there to prove the absence of another stem.

This helps the new tests check that those operations fail for the expected reason.

jsign · 2023-10-16T19:51:21Z

proof_test.go

@@ -485,6 +487,45 @@ func TestProofOfAbsenceOtherMultipleLeaves(t *testing.T) {
 	if len(proof.PoaStems) > 1 {
 		t.Fatalf("invalid number of proof-of-absence stems: %d", len(proof.PoaStems))
 	}
+


@gballet, pulled this extra twist in the test you did in the other PR. But did some changes that I'll describe in comments.

jsign · 2023-10-16T19:53:06Z

proof_test.go

+	if _, ok := node.children[ret3[4]].(*LeafNode); !ok {
+		t.Fatalf("expected leaf node at depth 5, got %v", node.children[ret3[4]])
+	}
+	if ln, ok := node.children[key[4]].(*LeafNode); !ok || !ln.isPOAStub {
+		t.Fatalf("expected unknown node at depth 5, got %v", node.children[key[4]])
+	}


Two changes compared to the original:

I fixed the t.Fatalf(...) ret3[X] and key[X] indices, that I later saw you also fixed in the PR. Just that I fixed it before you fixed it there, so that part should be fine here.

In L526, I do (*LeafNode) and not the original (*UnknownNode) for obvious reasons of this solution compared to the original one. I also check that this LeafNode has the isPOAStub turned on. Just to be sure since that should be true.

jsign · 2023-10-16T19:53:26Z

proof_test.go

@@ -990,4 +1031,28 @@ func TestGenerateProofWithOnlyAbsentKeys(t *testing.T) {
 	if ok, err := VerifyVerkleProof(dproof, pe.Cis, pe.Zis, pe.Yis, cfg); !ok || err != nil {
 		t.Fatalf("reconstructed proof didn't verify: %v", err)
 	}
+


I added some extra logic to the original test in #400.

jsign · 2023-10-16T19:53:53Z

proof_test.go

+	// Double-check that if we try to access any key in 40000000000000000000000000000000000000000000000000000000000000{XX}
+	// in the reconstructed tree, we get an error. This LeafNode is only supposed to prove
+	// the absence of 40100000000000000000000000000000000000000000000000000000000000{YY}, so
+	// we don't know anything about any value for slots XX.
+	for i := 0; i < common.VectorLength; i++ {
+		var key [32]byte
+		copy(key[:], presentKey)
+		key[31] = byte(i)
+		if _, err := droot.Get(key[:], nil); err != errIsPOAStub {
+			t.Fatalf("expected ErrPOALeafValue, got %v", err)
+		}
+	}


Double-check that read access to any key of the proof of absence leaf node must fail with the expected error.

jsign · 2023-10-16T20:01:17Z

tree.go

+		// Note that keys might contain keys that don't correspond to this leaf node.
+		// We should only analize the inclusion of C1/C2 for keys corresponding to this
+		// leaf node stem.
+		if equalPaths(n.stem, key) {
+			hasC1 = hasC1 || (key[31] < 128)
+			hasC2 = hasC2 || (key[31] >= 128)
+			if hasC2 {
+				break
+			}
+		}


Nothing new here, this is the original fix.

jsign · 2023-10-16T20:22:11Z

tree.go

 		if equalPaths(child.stem, stem) {
+			// We can't insert anything into a POA leaf node.
+			if child.isPOAStub {
+				return errIsPOAStub
+			}
+			n.cowChild(nChild)
 			return child.insertMultiple(stem, values)
 		}
+		n.cowChild(nChild)


Interestingly, (n *InsertNode) Insert(...) internally calls InsertStem(...) (and not (*LeafNode) Insert(...), so we short-circuit here.

Note that unfortunately I had to put n.cowChild(...) in two places, to avoid an unwanted side effect of doing that and doing return errIsPOAStub later, which would be incorrect. (i.e: unwanted side-effect).

not sure what side effect you mean, but in any case please add a comment so that we know later on why the n.cowChild ended up here.

My point is that we need to return before calling n.cowChild(..).

If we do n.cowChild(...) (saying that this children as changed), and return an error later, it would be incorrect to mark that children as changed.

Not sure if that deserves a comment. It's just something that any API that fails should be aware of (i.e: assuming it will succeed doing stuff, and fail later without reverting the effects).

jsign · 2023-10-16T20:22:51Z

tree.go

+			// We can't return the values since it's a POA leaf node, so we know nothing
+			// about its values.
+			if child.isPOAStub {
+				return nil, errIsPOAStub
+			}


Other important short-circuit. (*InternalNode) Get(..) internally calls (*InternalNode) GetStem(...) to get all the values and index later.

jsign · 2023-10-16T20:24:00Z

tree.go

+	// If this tree is a full tree (i.e: not a stateless tree), we know we have c1 and c2 values.
+	// Also, we _need_ them independently of hasC1 or hasC2 since the prover needs `Fis`.
+	if !n.isPOAStub {
+		if err := banderwagon.BatchMapToScalarField([]*Fr{&poly[2], &poly[3]}, []*Point{n.c1, n.c2}); err != nil {
+			return nil, nil, nil, fmt.Errorf("batch mapping to scalar fields: %s", err)
 		}


If this leaf node isn't a stub, we do the original thing that I removed from above. Nothing weird, since we are in a non-stateless tree.

jsign · 2023-10-16T20:24:26Z

tree.go

+	} else if hasC1 || hasC2 || n.c1 != nil || n.c2 != nil {
+		// This LeafNode is a proof of absence stub. It must be true that
+		// both c1 and c2 are nil, and that hasC1 and hasC2 are false.
+		// Let's just check that to be sure, since the code below can't use
+		// poly[2] or poly[3].
+		return nil, nil, nil, fmt.Errorf("invalid proof of absence stub")


If the LeafNode is a stub, let's be defensive and check some invariants:

We can't conclude that hasC1 is true.

Same with hasC2.

Any LeafNode with isPOAStub was constructed with nil c1 and c2. So check that.

This is important since lines below L1433 and L1439 will try to use the poly[2] and poly[3] values. Which in this case aren't calculated (because we can't!).

Maybe this check that I did is very paranoid; since indirectly those branches below have the "right" if cases. But my check is stronger: I don't care what the hasC1 or hasC2 evaluation was; those can never be ON in a stub. Period.

gballet

LGTM overall, but left a few questions and comments

gballet · 2023-10-17T09:00:54Z

tree.go

 		if equalPaths(child.stem, stem) {
+			// We can't insert anything into a POA leaf node.
+			if child.isPOAStub {
+				return errIsPOAStub
+			}
+			n.cowChild(nChild)
 			return child.insertMultiple(stem, values)
 		}
+		n.cowChild(nChild)


not sure what side effect you mean, but in any case please add a comment so that we know later on why the n.cowChild ended up here.

gballet · 2023-10-17T09:13:10Z

tree.go

+			// We can't insert anything into a POA leaf node.
+			if child.isPOAStub {
+				return errIsPOAStub
+			}
+			n.cowChild(nChild)


I don't quite like this comment, because it sounds like you can not insert in ANY type of PoA LeafNode, and the fact is that you can. I would rephrase as "you can't update leaf values of a PoA stem" or something like taht.

because it sounds like you can not insert in ANY type of PoA LeafNode, and the fact is that you can

You can't. In today's definition of isPOAStub, the only case is true is for a LeafNode that is only there to prove the absence of a stem. In this case, you can't insert any value. (i.e: you don't have C1 nor C2, thus you can't update things)

Maybe you're thinking about potential future "stub" cases where leaf nodes are partially filled and you might be able to insert things?

yes, the way you can is precisely when you are inserting a new leaf that has a common prefix with the PoA stem. So you can not insert values into it, but you can insert a LeafNode into it (by producing intermediate InternalNodes).

Right, but my comment is inside equalPaths(child.stem, stem) so I'm not talking about that case, but inserting values.

In any case, I can add insert values in the comment if that helps.

gballet · 2023-10-17T09:17:50Z

tree.go

-	if err := banderwagon.BatchMapToScalarField([]*Fr{&poly[2], &poly[3]}, []*Point{n.c1, n.c2}); err != nil {
-		return nil, nil, nil, fmt.Errorf("batch mapping to scalar fields: %s", err)
-	}


right, in 402 I catch this even at the call site in (*InternalNode) so that I don't have to come here. What I don't see, is why you need to move this code instead of just using an if like you do anyway line 1421 ?

gballet · 2023-10-17T09:18:51Z

tree.go

-	if err := banderwagon.BatchMapToScalarField([]*Fr{&poly[2], &poly[3]}, []*Point{n.c1, n.c2}); err != nil {
-		return nil, nil, nil, fmt.Errorf("batch mapping to scalar fields: %s", err)
-	}


you can still check the invariant later, but apart from that I don't quite see what that changes 🤔 ?

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

gballet

Alright, I don't quite like the added nesting in (*LeafNode).GetProofItems but we've spent enough time on this. Merging.

* test: gen proof with only absent keys Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * review feedback Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * fix absent keys in proofs (#401) * fix absent keys in proofs Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * add POA stub marker Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * defensive case Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * fixes Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * improving comment 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 mentioned this pull request Oct 15, 2023

proof: absent key proof doesn't verify #400

Merged

jsign commented Oct 15, 2023

View reviewed changes

tree.go Outdated Show resolved Hide resolved

gballet mentioned this pull request Oct 16, 2023

add stem to UnkownNode to allow for insertion in proof-of-absence leaf #402

Closed

fix absent keys in proofs

cff36e8

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

jsign force-pushed the jsign-test-case-fix branch from 68ebe55 to cff36e8 Compare October 16, 2023 14:28

jsign added 3 commits October 16, 2023 16:49

add POA stub marker

aef9219

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

defensive case

dfc77ae

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

fixes

db311ca

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

jsign commented Oct 16, 2023

View reviewed changes

jsign marked this pull request as ready for review October 16, 2023 20:24

jsign requested a review from gballet October 16, 2023 20:25

gballet reviewed Oct 17, 2023

View reviewed changes

jsign requested a review from gballet October 17, 2023 11:25

improving comment

0e4967a

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

gballet approved these changes Oct 17, 2023

View reviewed changes

gballet merged commit c4f5874 into jsign-test-case Oct 17, 2023
1 check passed

jsign deleted the jsign-test-case-fix branch October 17, 2023 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix absent keys in proofs #401

fix absent keys in proofs #401

jsign commented Oct 15, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

gballet Oct 17, 2023

jsign Oct 17, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

jsign Oct 16, 2023

gballet left a comment

gballet Oct 17, 2023

gballet Oct 17, 2023

jsign Oct 17, 2023 •

edited

Loading

gballet Oct 17, 2023

jsign Oct 17, 2023

jsign Oct 17, 2023

jsign Oct 17, 2023

gballet Oct 17, 2023

gballet Oct 17, 2023

gballet left a comment

fix absent keys in proofs #401

fix absent keys in proofs #401

Conversation

jsign commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsign Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

jsign Oct 17, 2023 •

edited

Loading