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

fix absent keys in proofs #401

Merged
merged 5 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

)

const (
Expand Down
67 changes: 66 additions & 1 deletion proof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
"fmt"
"reflect"
"testing"

"github.com/crate-crypto/go-ipa/common"
)

func TestProofVerifyTwoLeaves(t *testing.T) {
Expand Down Expand Up @@ -472,7 +474,7 @@ func TestProofOfAbsenceOtherMultipleLeaves(t *testing.T) {
if err := root.Insert(key, testValue, nil); err != nil {
t.Fatalf("could not insert key: %v", err)
}
root.Commit()
rootC := root.Commit()

ret1, _ := hex.DecodeString("0303030303030303030303030303030303030303030303030303030303030300")
ret2, _ := hex.DecodeString("0303030303030303030303030303030303030303030303030303030303030301")
Expand All @@ -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))
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

deserialized, err := PreStateTreeFromProof(proof, rootC)
if err != nil {
t.Fatalf("error deserializing %v", err)
}

got, err := deserialized.Get(ret1, nil)
if err != nil {
t.Fatalf("error while trying to read missing value: %v", err)
}
if got != nil {
t.Fatalf("should have returned nil, got: %v", got)
}

// simulate the execution of a tx that creates a leaf at an address that isn't the one that is
// proven for absence, but needs to be inserted in the proof-of-absence stem.
// It differs from the poa stem here: 🠃
ret3, _ := hex.DecodeString("0303030304030303030303030303030303030303030303030303030303030300")
err = deserialized.Insert(ret3, testValue, nil)
if err != nil {
t.Fatalf("error inserting value in proof-of-asbsence stem: %v", err)
}

// check that there are splits up to depth 4
node := deserialized.(*InternalNode)
for node.depth < 4 {
child, ok := node.children[ret3[node.depth]].(*InternalNode)
if !ok {
t.Fatalf("expected Internal node at depth %d, trie = %s", node.depth, ToDot(deserialized))
}
node = child
}

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]])
}
Comment on lines +523 to +528
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

func TestProofOfAbsenceNoneMultipleStems(t *testing.T) {
Expand Down Expand Up @@ -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)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

// 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)
}
}
Comment on lines +1035 to +1046
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


// The same applies to trying to insert values in this LeafNode, this shouldn't be allowed since we don't know
// anything about C1 or C2 to do a proper updating.
for i := 0; i < common.VectorLength; i++ {
var key [32]byte
copy(key[:], presentKey)
key[31] = byte(i)
if err := droot.Insert(key[:], zeroKeyTest, nil); err != errIsPOAStub {
t.Fatalf("expected ErrPOALeafValue, got %v", err)
}
}
Comment on lines +1048 to +1057
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same with writes.

I could have made both checks in the same loop... but I think this is fine anyway.

}
65 changes: 57 additions & 8 deletions tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,11 @@ type (
c1, c2 *Point

depth byte

// IsPOAStub indicates if this LeafNode is a proof of absence
// for a steam that isn't present in the tree. This flag is only
// true in the context of a stateless tree.
isPOAStub bool
Comment on lines +187 to +190
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new marker.

}
)

Expand Down Expand Up @@ -380,10 +385,15 @@ func (n *InternalNode) InsertStem(stem []byte, values [][]byte, resolver NodeRes
// splits.
return n.InsertStem(stem, values, resolver)
case *LeafNode:
n.cowChild(nChild)
if equalPaths(child.stem, stem) {
// We can't insert any values into a POA leaf node.
if child.isPOAStub {
return errIsPOAStub
}
n.cowChild(nChild)
return child.insertMultiple(stem, values)
}
n.cowChild(nChild)

// A new branch node has to be inserted. Depending
// on the next word in both keys, a recursion into
Expand Down Expand Up @@ -439,6 +449,15 @@ func (n *InternalNode) CreatePath(path []byte, stemInfo stemInfo, comms []*Point
n.children[path[0]] = Empty{}
case extStatusAbsentOther:
// insert poa stem
newchild := &LeafNode{
commitment: comms[0],
stem: stemInfo.stem,
values: nil,
depth: n.depth + 1,
isPOAStub: true,
}
n.children[path[0]] = newchild
comms = comms[1:]
Comment on lines +452 to +460
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So now this case in the tree reconstruction is setting that isPOAStub = true.

Note that c1 and c2 will remain nil, but that should be fine since isPOAStub should be short-circuiting any kind of access to that data.

Actually, it's a good thing those things are nil. If for whatever reason tomorrow there's a new path that doesn't do the isPOAStem check, the implementation will panic trying to do stuff with nil values.

case extStatusPresent:
// insert stem
newchild := &LeafNode{
Expand Down Expand Up @@ -518,6 +537,11 @@ func (n *InternalNode) GetStem(stem []byte, resolver NodeResolverFn) ([][]byte,
return n.GetStem(stem, resolver)
case *LeafNode:
if equalPaths(child.stem, stem) {
// 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
}
Comment on lines +540 to +544
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

return child.values, nil
}
return nil, nil
Expand Down Expand Up @@ -1014,6 +1038,10 @@ func (n *InternalNode) touchCoW(index byte) {
}

func (n *LeafNode) Insert(key []byte, value []byte, _ NodeResolverFn) error {
if n.isPOAStub {
return errIsPOAStub
}
Comment on lines +1041 to +1043
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interestingly, this (n *LeafNode) Insert(...) API isn't used in the majority of cases, since most writes use the updateMultipleLeaves(...) path (to be mentioned below).

In any case to be sure, the short circuit is here since it's the right thing to do anyways.


if len(key) != StemSize+1 {
return fmt.Errorf("invalid key size: %d", len(key))
}
Expand Down Expand Up @@ -1268,6 +1296,10 @@ func (n *LeafNode) Delete(k []byte, _ NodeResolverFn) (bool, error) {
}

func (n *LeafNode) Get(k []byte, _ NodeResolverFn) ([]byte, error) {
if n.isPOAStub {
return nil, errIsPOAStub
}
Comment on lines +1299 to +1301
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same with this API. In the majority of cases, the (n *InternalNode) Get(...) will use (n *InternalNode) GetStem(...) (L542 above) which does a direct child.values access, and then a [i-th] access.

Again, did the short-circuit here again since it's also the correct thing to do.


if !equalPaths(k, n.stem) {
// If keys differ, return nil in order to
// signal that the key isn't present in the
Expand Down Expand Up @@ -1368,19 +1400,36 @@ func (n *LeafNode) GetProofItems(keys keylist, _ NodeResolverFn) (*ProofElements
if err := StemFromBytes(&poly[1], n.stem); err != nil {
return nil, nil, nil, fmt.Errorf("error serializing stem '%x': %w", n.stem, err)
}
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)
}
Comment on lines -1371 to -1373
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, so here's a vital correctness fix.

We can't always do this here. If we're calling this GetProofItems(...) in a stateless reconstructed tree, then n.c1 and n.c2 will be nil. (Remember that now we only set the isPOAStub marker; so we can't blindly assume the data is there).

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, in 402 I catch this even at the call site in (*InternalNode) so that I don't have to come here.

Personally, I don't like that solution since if someone calls (*LeafNode) GetProofItems(...) that code would do the incorrect thing. You can say: "but usually, GetProofItems(...) gets call through an InternalNode". While that's true, there's nothing that prevents the caller to have some LeafNode and call this API. (As we do in many tests even).

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 ?

It's true that I didn't need to do that, but I did so I can put in the same if the extra checks in the else case below. Also, putting this earlier wasn't needed since it's use later in the logic.

In any case, I can move it back and have two separate ifs.


// First pass: add top-level elements first
var hasC1, hasC2 bool
for _, key := range keys {
hasC1 = hasC1 || (key[31] < 128)
hasC2 = hasC2 || (key[31] >= 128)
if hasC2 {
break
// 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
}
}
Comment on lines +1407 to +1416
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nothing new here, this is the original fix.

}

// 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)
}
Comment on lines +1419 to 1424
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

} 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")
Comment on lines +1425 to +1430
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

if hasC1 {
pe.Cis = append(pe.Cis, n.commitment)
pe.Zis = append(pe.Zis, 2)
Expand Down