Proof of absence and presence for the same path with nil proof of presence value #414

jsign · 2023-11-02T16:38:02Z

This is a proposed fix to #413.

At the end of the day, it's almost aligned with the ultra-original Python implementation, which probably did this for the same reason I found #413 problematic.

Signed-off-by: Ignacio Hagopian <[email protected]>

proof_ipa.go

tree.go

tree_test.go

proof_ipa.go

proof_test.go

gballet

LGTM. It's true that it makes things a bit simpler.

proof_test.go

tree.go

proof_test.go

proof_ipa.go

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign · 2023-11-03T14:43:37Z

proof_ipa.go

@@ -398,8 +398,9 @@ func PreStateTreeFromProof(proof *Proof, rootC *Point) (VerkleNode, error) { //
 			stems = append(stems, k[:31])
 		}
 	}
-	stemIndex := 0


We don't need this anymore, things got simplified.

jsign · 2023-11-03T14:43:56Z

proof_ipa.go

+	if len(stems) != len(proof.ExtStatus) {
+		return nil, fmt.Errorf("invalid number of stems and extension statuses: %d != %d", len(stems), len(proof.ExtStatus))
+	}


As a preliminary check, now check if there's a 1:1 match between stems and extension statuses.

jsign · 2023-11-03T14:44:14Z

proof_ipa.go

+	// We build a cache of paths that have a presence extension status.
+	pathsWithExtPresent := map[string]struct{}{}
+	i := 0
 	for _, es := range proof.ExtStatus {
-		depth := es >> 3
-		path := stems[stemIndex][:depth]
+		if es&3 == extStatusPresent {
+			pathsWithExtPresent[string(stems[i][:es>>3])] = struct{}{}
+		}
+		i++
+	}


Nothing new here compared to last review.

jsign · 2023-11-03T14:45:11Z

proof_ipa.go

+			for j := range proof.Keys { // TODO: DoS risk, use map or binary search.
+				if bytes.HasPrefix(proof.Keys[j], stems[i]) && proof.PreValues[j] != nil {
+					return nil, fmt.Errorf("proof of absence (other) stem %x has a value", si.stem)
 				}
 			}


First, we check that all keys for this stem have nil values.
This must be the case since all are absent. If that's not true, wrong proof.

jsign · 2023-11-03T14:45:40Z

proof_ipa.go

+			// For this absent path, we must first check if this path contains a proof of presence.
+			// If that is the case, we don't have to do anything since the corresponding leaf will be
+			// constructed by that extension status (already processed or to be processed).
+			// In other case, we should get the stem from the list of proof of absence stems.
+			if _, ok := pathsWithExtPresent[string(path)]; ok {
+				continue
+			}


Second, if we know there is a proof of presence for this path; we don't do anything since that proof of presence will create this path.

jsign · 2023-11-03T14:46:24Z

proof_ipa.go

+			si.stem = poas[0]
+			poas = poas[1:]


Forth, we're the first (or only) proof of absence (without proof of presences) for this path. Create it.

jsign · 2023-11-03T14:47:36Z

proof_ipa.go

 			si.values = map[byte][]byte{}
-			for i, k := range proof.Keys { // TODO: DoS risk, use map or binary search.
-				if bytes.Equal(k[:len(path)], stemPath) && proof.PreValues[i] != nil {
-					si.values[k[31]] = proof.PreValues[i]
+			si.stem = stems[i]
+			for j, k := range proof.Keys { // TODO: DoS risk, use map or binary search.
+				if bytes.Equal(k[:31], si.stem) {
+					si.values[k[31]] = proof.PreValues[j]
 					si.has_c1 = si.has_c1 || (k[31] < 128)
 					si.has_c2 = si.has_c2 || (k[31] >= 128)
-					// This key has values, its stem is the one that
-					// is present.
-					if si.stem == nil {
-						si.stem = k[:31]
-						continue
-					}
-					// Any other key with values must have the same
-					// same previously detected stem. If that isn't the case,
-					// the proof is invalid.
-					if !bytes.Equal(si.stem, k[:31]) {
-						return nil, fmt.Errorf("multiple keys with values found for stem %x", k[:31])
-					}
 				}
 			}
-			// For a proof of presence, we must always have detected a stem.
-			// If that isn't the case, the proof is invalid.
-			if si.stem == nil {
-				return nil, fmt.Errorf("no stem found for path %x", path)
-			}


All this got quite simplified since:

si.stem is stem[i] directly due to the current 1:1 correspondence

Look for keys, and simply add them into stemInfo (we don't care if their values are nil or not-nil. just add the values and mark c1/c2 appropriately).

jsign · 2023-11-03T14:48:01Z

proof_ipa.go

-
-		// Skip over all the stems that share the same path
-		// to the extension tree. This happens e.g. if two
-		// stems have the same path, but one is a proof of
-		// absence and the other one is present.
-		stemIndex++
-		for ; stemIndex < len(stems); stemIndex++ {
-			if !bytes.Equal(stems[stemIndex][:depth], path) {
-				break
-			}
-		}


Thanks to the 1:1 correspondence, all this is unnecessary since we don't have to "infer" the current stem by "tracking" some separate index from keys.

jsign · 2023-11-03T14:48:35Z

tree.go

+			// If this is the first extension status added for this path,
+			// add the proof of absence stem (only once). If later we detect a proof of
+			// presence, we'll clear the list since that proof of presence
+			// will be enough to provide the stem.
 			if len(esses) == 0 {
-				esses = append(esses, extStatusAbsentOther|(n.depth<<3))
 				poass = append(poass, n.stem)
 			}


What comment says.

jsign · 2023-11-03T14:49:12Z

tree.go

+			// Add an extension status absent other for this stem.
+			// Note we keep a cache to avoid adding the same stem twice (or more) if
+			// there're multiple keys with the same stem.
+			if _, ok := addedAbsentStems[string(key[:StemSize])]; !ok {
+				esses = append(esses, extStatusAbsentOther|(n.depth<<3))
+				addedAbsentStems[string(key[:StemSize])] = struct{}{}
+			}


We use the addedAbsentStems to avoid adding duplicate extension statuses for the same stem. This can happen since this external loop is iterating keys, and not "grouped stems".

gballet

Thanks for tackling this, it was long overdue. Before we merge, have you ensured that it could sync kaustinen and verify every proof?

jsign · 2023-11-03T16:35:53Z

Thanks for tackling this, it was long overdue. Before we merge, have you ensured that it could sync kaustinen and verify every proof?

I'll merge all back to #412, and I'll test Kaustinen with that branch before merging to main.

* proof: add proof of absence border case tests Signed-off-by: Ignacio Hagopian <[email protected]> * 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 <[email protected]> * more improvements and tests Signed-off-by: Ignacio Hagopian <[email protected]> * fix & extra test case Signed-off-by: Ignacio Hagopian <[email protected]> * make steams and extStatuses be 1:1 Signed-off-by: Ignacio Hagopian <[email protected]> * cleanup Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

* proof: verify proof of absence steam is sorted Signed-off-by: Ignacio Hagopian <[email protected]> * proof: more length checks Signed-off-by: Ignacio Hagopian <[email protected]> * more tree from proof checks Signed-off-by: Ignacio Hagopian <[email protected]> * add checks in CreatePath Signed-off-by: Ignacio Hagopian <[email protected]> * lints Signed-off-by: Ignacio Hagopian <[email protected]> * fix comments Signed-off-by: Ignacio Hagopian <[email protected]> * Proof of absence border case test (#413) * proof: add proof of absence border case tests Signed-off-by: Ignacio Hagopian <[email protected]> * 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 <[email protected]> * more improvements and tests Signed-off-by: Ignacio Hagopian <[email protected]> * fix & extra test case Signed-off-by: Ignacio Hagopian <[email protected]> * make steams and extStatuses be 1:1 Signed-off-by: Ignacio Hagopian <[email protected]> * cleanup Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]> * lint Signed-off-by: Ignacio Hagopian <[email protected]> * add extra border case test Signed-off-by: Ignacio Hagopian <[email protected]> * fix bug Signed-off-by: Ignacio Hagopian <[email protected]> * further fixes Signed-off-by: Ignacio Hagopian <[email protected]> * fix Signed-off-by: Ignacio Hagopian <[email protected]> * cleanup Signed-off-by: Ignacio Hagopian <[email protected]> --------- Signed-off-by: Ignacio Hagopian <[email protected]>

jsign added 2 commits November 2, 2023 13:33

keep the extension statuses of absent stems

f227a0d

Signed-off-by: Ignacio Hagopian <[email protected]>

more improvements and tests

de282d8

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign commented Nov 2, 2023

View reviewed changes

gballet approved these changes Nov 3, 2023

View reviewed changes

proof_test.go Outdated Show resolved Hide resolved

proof_test.go Outdated Show resolved Hide resolved

tree.go Show resolved Hide resolved

jsign force-pushed the jsign-proof-tests-fix branch from 6c426c6 to 33c0992 Compare November 3, 2023 12:58

jsign commented Nov 3, 2023

View reviewed changes

proof_test.go Show resolved Hide resolved

jsign force-pushed the jsign-proof-tests-fix branch from 33c0992 to a58c6fc Compare November 3, 2023 13:06

jsign commented Nov 3, 2023

View reviewed changes

proof_ipa.go Show resolved Hide resolved

fix & extra test case

fa7eeeb

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign force-pushed the jsign-proof-tests-fix branch from a58c6fc to fa7eeeb Compare November 3, 2023 13:19

jsign added 2 commits November 3, 2023 11:41

make steams and extStatuses be 1:1

0e58068

Signed-off-by: Ignacio Hagopian <[email protected]>

cleanup

c393e10

Signed-off-by: Ignacio Hagopian <[email protected]>

jsign commented Nov 3, 2023

View reviewed changes

jsign requested a review from gballet November 3, 2023 14:49

jsign marked this pull request as ready for review November 3, 2023 14:49

gballet approved these changes Nov 3, 2023

View reviewed changes

jsign merged commit bd5b319 into jsign-proof-tests Nov 3, 2023
1 check passed

jsign mentioned this pull request Nov 3, 2023

Fix proof generation and verification code #412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of absence and presence for the same path with nil proof of presence value #414

Proof of absence and presence for the same path with nil proof of presence value #414

jsign commented Nov 2, 2023 •

edited

Loading

gballet left a comment

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

jsign Nov 3, 2023

gballet left a comment

jsign commented Nov 3, 2023 •

edited

Loading

Proof of absence and presence for the same path with nil proof of presence value #414

Proof of absence and presence for the same path with nil proof of presence value #414

Conversation

jsign commented Nov 2, 2023 • edited Loading

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

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

jsign commented Nov 3, 2023 • edited Loading

jsign commented Nov 2, 2023 •

edited

Loading

jsign commented Nov 3, 2023 •

edited

Loading