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

Check for placeholder nodes in proof verification #61

Merged
merged 10 commits into from
Jan 28, 2022
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
58 changes: 52 additions & 6 deletions go/proof.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,27 +210,27 @@ func (p *NonExistenceProof) Verify(spec *ProofSpec, root CommitmentRoot, key []b
return nil
}

// IsLeftMost returns true if this is the left-most path in the tree
// IsLeftMost returns true if this is the left-most path in the tree, excluding placeholder (empty child) nodes
func IsLeftMost(spec *InnerSpec, path []*InnerOp) bool {
minPrefix, maxPrefix, suffix := getPadding(spec, 0)

// ensure every step has a prefix and suffix defined to be leftmost
// ensure every step has a prefix and suffix defined to be leftmost, unless it is a placeholder node
for _, step := range path {
if !hasPadding(step, minPrefix, maxPrefix, suffix) {
if !hasPadding(step, minPrefix, maxPrefix, suffix) && !leftBranchesAreEmpty(spec, step, 0) {
return false
}
}
return true
}

// IsRightMost returns true if this is the left-most path in the tree
// IsRightMost returns true if this is the left-most path in the tree, excluding placeholder (empty child) nodes
func IsRightMost(spec *InnerSpec, path []*InnerOp) bool {
last := len(spec.ChildOrder) - 1
minPrefix, maxPrefix, suffix := getPadding(spec, int32(last))

// ensure every step has a prefix and suffix defined to be rightmost
// ensure every step has a prefix and suffix defined to be rightmost, unless it is a placeholder node
for _, step := range path {
if !hasPadding(step, minPrefix, maxPrefix, suffix) {
if !hasPadding(step, minPrefix, maxPrefix, suffix) && !rightBranchesAreEmpty(spec, step, int32(last)) {
return false
}
}
Expand Down Expand Up @@ -285,6 +285,7 @@ func isLeftStep(spec *InnerSpec, left *InnerOp, right *InnerOp) bool {
return rightidx == leftidx+1
}

// checks if an op has the expected padding
func hasPadding(op *InnerOp, minPrefix, maxPrefix, suffix int) bool {
if len(op.Prefix) < minPrefix {
return false
Expand All @@ -309,6 +310,51 @@ func getPadding(spec *InnerSpec, branch int32) (minPrefix, maxPrefix, suffix int
return
}

// leftBranchesAreEmpty returns true if the padding bytes correspond to all empty children
roysc marked this conversation as resolved.
Show resolved Hide resolved
// on the left side of this branch, ie. it's a valid placeholder on a leftmost path
func leftBranchesAreEmpty(spec *InnerSpec, op *InnerOp, branch int32) bool {
idx := getPosition(spec.ChildOrder, branch)
// count branches to left of this
leftBranches := idx
if leftBranches == 0 {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

That's an odd choice.
If there are no left branches, then all left branches are empty, right?
Why return false here?

(The true case would fall out after the loop anyway, but fine to explicitly return early. Not sure why you want to return false here: on the left side of this branch, ie. it's a valid placeholder on a leftmost path implies that no leftBranches at all would satisfy this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the index is 0, the op describes a left-branch, but the op contains the data of the siblings of that branch, right? So leftBranches is the count of branches to the left of the "described" branch, which is none. It's not really a valid input to the function, because it asks a meaningless question: "are the branches to the left of this one empty?" That's how I see it, so an error may be more appropriate, but returning false seemed better than true.

Copy link
Contributor

Choose a reason for hiding this comment

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

again, as this function is only used to check "leftmost" of a path, I would do this differently. But no need to repeat myself

}
// compare prefix with the expected number of empty branches
actualPrefix := len(op.Prefix) - leftBranches*int(spec.ChildSize)
if actualPrefix < 0 {
return false
}
for i := 0; i < leftBranches; i++ {
from := actualPrefix + i*int(spec.ChildSize)
if !bytes.Equal(spec.EmptyChild, op.Prefix[from:from+int(spec.ChildSize)]) {
return false
}
}
return true
}

// rightBranchesAreEmpty returns true if the padding bytes correspond to all empty children
// on the right side of this branch, ie. it's a valid placeholder on a rightmost path
func rightBranchesAreEmpty(spec *InnerSpec, op *InnerOp, branch int32) bool {
idx := getPosition(spec.ChildOrder, branch)
// count branches to right of this one
rightBranches := len(spec.ChildOrder) - 1 - idx
if rightBranches == 0 {
return false
}
// compare suffix with the expected number of empty branches
if len(op.Suffix) != rightBranches*int(spec.ChildSize) {
return false // sanity check
}
for i := 0; i < rightBranches; i++ {
from := i * int(spec.ChildSize)
if !bytes.Equal(spec.EmptyChild, op.Suffix[from:from+int(spec.ChildSize)]) {
return false
}
}
return true
}

// getPosition checks where the branch is in the order and returns
// the index of this branch
func getPosition(order []int32, branch int32) int {
Expand Down
112 changes: 112 additions & 0 deletions go/proof_data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,115 @@ func CheckAgainstSpecTestData(t *testing.T) map[string]CheckAgainstSpecTestStruc
}
return cases
}

var SpecWithEmptyChild = &ProofSpec{
LeafSpec: &LeafOp{
Prefix: []byte{0},
Hash: HashOp_SHA256,
PrehashValue: HashOp_SHA256,
},
InnerSpec: &InnerSpec{
ChildOrder: []int32{0, 1},
ChildSize: 32,
MinPrefixLength: 1,
MaxPrefixLength: 1,
EmptyChild: []byte("32_empty_child_placeholder_bytes"),
Hash: HashOp_SHA256,
},
}

type EmptyBranchTestStruct struct {
Op *InnerOp
Spec *ProofSpec
IsLeft bool
IsRight bool
}

func EmptyBranchTestData(t *testing.T) []EmptyBranchTestStruct {
var emptyChild = SpecWithEmptyChild.InnerSpec.EmptyChild

return []EmptyBranchTestStruct{
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: append([]byte{1}, emptyChild...),
Suffix: nil,
Hash: SpecWithEmptyChild.InnerSpec.Hash,
},
Spec: SpecWithEmptyChild,
IsLeft: true,
IsRight: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this also be isRight: true? there is no suffix

},
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: []byte{1},
Suffix: emptyChild,
Hash: SpecWithEmptyChild.InnerSpec.Hash,
},
Spec: SpecWithEmptyChild,
IsLeft: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, this is leftmost from the previous definition

IsRight: true,
},
// non-empty cases
EmptyBranchTestStruct{
roysc marked this conversation as resolved.
Show resolved Hide resolved
Op: &InnerOp{
Prefix: append([]byte{1}, make([]byte, 32)...),
Suffix: nil,
Hash: SpecWithEmptyChild.InnerSpec.Hash,
},
Spec: SpecWithEmptyChild,
IsLeft: false,
IsRight: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect: IsRight: true

},
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: []byte{1},
Suffix: make([]byte, 32),
Hash: SpecWithEmptyChild.InnerSpec.Hash,
},
Spec: SpecWithEmptyChild,
IsLeft: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect: IsLeft: true

IsRight: false,
},
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: append(append([]byte{1}, emptyChild[0:28]...), []byte("xxxx")...),
Suffix: nil,
Hash: SpecWithEmptyChild.InnerSpec.Hash,
},
Spec: SpecWithEmptyChild,
IsLeft: false,
IsRight: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect: IsRight: true

},
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: []byte{1},
Suffix: append(append([]byte(nil), emptyChild[0:28]...), []byte("xxxx")...),
Hash: SpecWithEmptyChild.InnerSpec.Hash,
},
Spec: SpecWithEmptyChild,
IsLeft: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect: IsLeft: true

IsRight: false,
},
// some cases using a spec with no empty child
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: append([]byte{1}, make([]byte, 32)...),
Suffix: nil,
Hash: TendermintSpec.InnerSpec.Hash,
},
Spec: TendermintSpec,
IsLeft: false,
IsRight: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expected: IsRight: true

},
EmptyBranchTestStruct{
Op: &InnerOp{
Prefix: []byte{1},
Suffix: make([]byte, 32),
Hash: TendermintSpec.InnerSpec.Hash,
},
Spec: TendermintSpec,
IsLeft: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Expected IsLeft: true

IsRight: false,
},
}
}
22 changes: 22 additions & 0 deletions go/proof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,25 @@ func TestCheckAgainstSpec(t *testing.T) {
})
}
}

func TestEmptyBranch(t *testing.T) {
cases := EmptyBranchTestData(t)

for _, tc := range cases {
t.Run("case", func(t *testing.T) {
if err := tc.Op.CheckAgainstSpec(tc.Spec); err != nil {
t.Errorf("Invalid InnerOp: %v", err)
}
order, err := orderFromPadding(tc.Spec.InnerSpec, tc.Op)
if err != nil {
t.Errorf("Cannot get orderFromPadding: %v", err)
}
if leftBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op, order) != tc.IsLeft {
t.Errorf("Expected leftBranchesAreEmpty to be %t but it wasn't", tc.IsLeft)
}
if rightBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op, order) != tc.IsRight {
t.Errorf("Expected rightBranchesAreEmpty to be %t but it wasn't", tc.IsRight)
}
})
}
}
11 changes: 7 additions & 4 deletions go/vectors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,26 @@ func TestBatchVectors(t *testing.T) {
// non-existence
valid := VerifyNonMembership(tc.Spec, tc.Ref.RootHash, tc.Proof, tc.Ref.Key)
if valid == tc.Invalid {
t.Fatalf("Expected proof validity: %t", !tc.Invalid)
t.Logf("name: %+v", name)
t.Logf("ref: %+v", tc.Ref)
t.Logf("spec: %+v", tc.Spec)
t.Errorf("Expected proof validity: %t", !tc.Invalid)
}
keys := [][]byte{tc.Ref.Key}
valid = BatchVerifyNonMembership(tc.Spec, tc.Ref.RootHash, tc.Proof, keys)
if valid == tc.Invalid {
t.Fatalf("Expected batch proof validity: %t", !tc.Invalid)
t.Errorf("Expected batch proof validity: %t", !tc.Invalid)
}
} else {
valid := VerifyMembership(tc.Spec, tc.Ref.RootHash, tc.Proof, tc.Ref.Key, tc.Ref.Value)
if valid == tc.Invalid {
t.Fatalf("Expected proof validity: %t", !tc.Invalid)
t.Errorf("Expected proof validity: %t", !tc.Invalid)
}
items := make(map[string][]byte)
items[string(tc.Ref.Key)] = tc.Ref.Value
valid = BatchVerifyMembership(tc.Spec, tc.Ref.RootHash, tc.Proof, items)
if valid == tc.Invalid {
t.Fatalf("Expected batch proof validity: %t", !tc.Invalid)
t.Errorf("Expected batch proof validity: %t", !tc.Invalid)
}
}
})
Expand Down
Loading