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.

Sign up for GitHub

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

Check for placeholder nodes in proof verification #61

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f8d2d4
Check for placeholder nodes in proof verification
roysc Dec 10, 2021
c96dcaf
Rust: handle placeholder nodes
roysc Dec 15, 2021
d91ed00
revise unit tests
roysc Dec 16, 2021
d2555d7
revise functions
roysc Dec 16, 2021
ac9a7b8
rust revise fn
roysc Dec 16, 2021
a1f90d0
rust revise units
roysc Dec 16, 2021
8450bb8
Fix the branchesAreEmpty checks in Go
ethanfrey Jan 13, 2022
8a4b4d9
revise: fn behavior, proof test data
roysc Jan 13, 2022
05b8a53
rust revision
roysc Jan 14, 2022
548b3f0
cleanup
roysc Jan 17, 2022
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
48 changes: 42 additions & 6 deletions go/proof.go
View file
Open in desktop
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 @@ -309,6 +309,42 @@ 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)
// compare the prefix bytes with the appropriate number of empty children
leftChildren := len(spec.ChildOrder) - 1 - idx
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the failing test cases, I think this math is wrong.

Assume there are 2 children, those would have position 0, and 1.

For position 0, I would get 2-1-0 = 1 left children (but should be 0)
For position 1, I would get 2-1-1 = 0 left children. (but should be 1)

I think leftChildren := idx would be correct

Copy link
Contributor Author

@roysc roysc Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, this makes more sense. I was thinking about this backwards semantically, such that branch meant, "the index over here" rather than, "the index corresponding to sibling data with this shape"

actualPrefix := len(op.Prefix) - leftChildren*int(spec.ChildSize)
if actualPrefix < 0 {
return false
}
for i := 0; i < leftChildren; 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)
// compare the suffix bytes with the appropriate number of empty children
if len(op.Suffix) != idx*int(spec.ChildSize) {
roysc marked this conversation as resolved.
Show resolved Hide resolved
return false
}
for i := 0; i < idx; 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
View file
Open in desktop
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,
},
}
}
18 changes: 18 additions & 0 deletions go/proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,21 @@ 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)
}
if leftBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op, 0) != tc.IsLeft {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, you are calling leftBranchesAreEmpty not IsLeftMost, so maybe my comments are wrong above.

However if there are no left branch, I would assume that leftBranchesAreEmpty would be true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is an error of naming and intended meaning. The field should probably have been IsLeftPlaceholder, since I meant for this to only be true for ops that contain an empty subtree on the left side.

It didn't make sense to me to call leftBranchesAreEmpty on the InnerOp describing a left branch, since by definition its left branch is non empty. So, I mistakenly used the branch index to instead get the padding for the branch on the opposite side (which of course works only when there are 2 children. I made it worse by hard coding the branch to make it work).

Even with your fix though, I would argue that these functions should not pass on Ops that describe a non-empty branch on the corresponding side. They only pass trivially now because the padding is empty, which I'd consider a bug.
So, I propose renaming the field to IsLeftPlaceholder, and changing the behavior so the functions fail when there is no padding to check for the corresponding side so e.g. leftBranchesAreEmpty(..., branch=0) is always false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you will be the largest user if this, I can accept it. I am still not convinced this is the best or most maintainable way to write it.

I would like the function to say "isLeftmost" regardless of empty children to the left and replace the current padding check with just this function. Rather than check if it is actually the leftmost, and then check if it is not leftmost, but all (non-zero) left siblings are empty.

I think doing this logic in one piece would avoid confusion later. But I won't block this pr more.

t.Errorf("Expected leftBranchesAreEmpty to be %t but it wasn't", tc.IsLeft)
}
if rightBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op, 1) != tc.IsRight {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I started to write a patch and see another error... you hardcode 0 and 1 as positions. Let's use the real position it has from padding.

t.Errorf("Expected rightBranchesAreEmpty to be %t but it wasn't", tc.IsRight)
}
})
}
}
Loading