diff --git a/go/CHANGELOG.md b/go/CHANGELOG.md index ea470aa..a0beb44 100644 --- a/go/CHANGELOG.md +++ b/go/CHANGELOG.md @@ -4,6 +4,7 @@ - deps: bump golang to v1.22 ([#363](https://github.com/cosmos/ics23/pull/363)). - fix: guarantee that `spec.InnerSpec.MaxPrefixLength` < `spec.InnerSpec.MinPrefixLength` + `spec.InnerSpec.ChildSize` ([#369](https://github.com/cosmos/ics23/pull/369)) +- fix: return error instead of panic in `getPosition` which results in error returns from `IsLeftMost`, `IsRightMost`, `IsLeftNeighbor`, `leftBranchesAreEmpty`, `rightBranchesAreEmpty`, and `getPadding` - refactor: support for `BatchProof` and `CompressedBatchProof` is being dropped. * The API's `BatchVerifyMembership`, `BatchVerifyNonMembership`, and `CombineProofs` have been removed. ([#390](https://github.com/cosmos/ics23/pull/390)) * The API's `IsCompressed`, `Compress`, and `Decompress` have been removed. ([#392](https://github.com/cosmos/ics23/pull/392)) diff --git a/go/proof.go b/go/proof.go index c801e8f..202b0a7 100644 --- a/go/proof.go +++ b/go/proof.go @@ -246,15 +246,30 @@ func (p *NonExistenceProof) Verify(spec *ProofSpec, root CommitmentRoot, key []b switch { case leftKey == nil: - if !IsLeftMost(spec.InnerSpec, p.Right.Path) { + isLeftMost, err := IsLeftMost(spec.InnerSpec, p.Right.Path) + if err != nil { + return err + } + + if !isLeftMost { return errors.New("left proof missing, right proof must be left-most") } case rightKey == nil: - if !IsRightMost(spec.InnerSpec, p.Left.Path) { + isRightMost, err := IsRightMost(spec.InnerSpec, p.Left.Path) + if err != nil { + return err + } + + if !isRightMost { return errors.New("right proof missing, left proof must be right-most") } default: - if !IsLeftNeighbor(spec.InnerSpec, p.Left.Path, p.Right.Path) { + isLeftNeighbor, err := IsLeftNeighbor(spec.InnerSpec, p.Left.Path, p.Right.Path) + if err != nil { + return err + } + + if !isLeftNeighbor { return errors.New("right proof missing, left proof must be right-most") } } @@ -263,30 +278,50 @@ func (p *NonExistenceProof) Verify(spec *ProofSpec, root CommitmentRoot, key []b } // 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) +func IsLeftMost(spec *InnerSpec, path []*InnerOp) (bool, error) { + minPrefix, maxPrefix, suffix, err := getPadding(spec, 0) + if err != nil { + return false, err + } // 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) && !leftBranchesAreEmpty(spec, step) { - return false + if !hasPadding(step, minPrefix, maxPrefix, suffix) { + leftBranchesAreEmpty, err := leftBranchesAreEmpty(spec, step) + if err != nil { + return false, err + } + + if !leftBranchesAreEmpty { + return false, nil + } } } - return true + return true, nil } // IsRightMost returns true if this is the right-most path in the tree, excluding placeholder (empty child) nodes -func IsRightMost(spec *InnerSpec, path []*InnerOp) bool { +func IsRightMost(spec *InnerSpec, path []*InnerOp) (bool, error) { last := len(spec.ChildOrder) - 1 - minPrefix, maxPrefix, suffix := getPadding(spec, int32(last)) + minPrefix, maxPrefix, suffix, err := getPadding(spec, int32(last)) + if err != nil { + return false, err + } // 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) && !rightBranchesAreEmpty(spec, step) { - return false + if !hasPadding(step, minPrefix, maxPrefix, suffix) { + rightBranchesAreEmpty, err := rightBranchesAreEmpty(spec, step) + if err != nil { + return false, err + } + + if !rightBranchesAreEmpty { + return false, nil + } } } - return true + return true, nil } // IsLeftNeighbor returns true if `right` is the next possible path right of `left` @@ -295,7 +330,7 @@ func IsRightMost(spec *InnerSpec, path []*InnerOp) bool { // Validate that LPath[len-1] is the left neighbor of RPath[len-1] // For step in LPath[0..len-1], validate step is right-most node // For step in RPath[0..len-1], validate step is left-most node -func IsLeftNeighbor(spec *InnerSpec, left []*InnerOp, right []*InnerOp) bool { +func IsLeftNeighbor(spec *InnerSpec, left []*InnerOp, right []*InnerOp) (bool, error) { // count common tail (from end, near root) left, topleft := left[:len(left)-1], left[len(left)-1] right, topright := right[:len(right)-1], right[len(right)-1] @@ -307,18 +342,27 @@ func IsLeftNeighbor(spec *InnerSpec, left []*InnerOp, right []*InnerOp) bool { // now topleft and topright are the first divergent nodes // make sure they are left and right of each other if !isLeftStep(spec, topleft, topright) { - return false + return false, nil } // left and right are remaining children below the split, // ensure left child is the rightmost path, and visa versa - if !IsRightMost(spec, left) { - return false + isRightMost, err := IsRightMost(spec, left) + if err != nil { + return false, err } - if !IsLeftMost(spec, right) { - return false + if !isRightMost { + return false, nil } - return true + + isLeftMost, err := IsLeftMost(spec, right) + if err != nil { + return false, err + } + if !isLeftMost { + return false, nil + } + return true, nil } // isLeftStep assumes left and right have common parents @@ -349,8 +393,11 @@ func hasPadding(op *InnerOp, minPrefix, maxPrefix, suffix int) bool { } // getPadding determines prefix and suffix with the given spec and position in the tree -func getPadding(spec *InnerSpec, branch int32) (minPrefix, maxPrefix, suffix int) { - idx := getPosition(spec.ChildOrder, branch) +func getPadding(spec *InnerSpec, branch int32) (minPrefix, maxPrefix, suffix int, err error) { + idx, err := getPosition(spec.ChildOrder, branch) + if err != nil { + return 0, 0, 0, err + } // count how many children are in the prefix prefix := idx * int(spec.ChildSize) @@ -359,74 +406,83 @@ func getPadding(spec *InnerSpec, branch int32) (minPrefix, maxPrefix, suffix int // count how many children are in the suffix suffix = (len(spec.ChildOrder) - 1 - idx) * int(spec.ChildSize) - return minPrefix, maxPrefix, suffix + return minPrefix, maxPrefix, suffix, nil } // leftBranchesAreEmpty returns true if the padding bytes correspond to all empty siblings // on the left side of a branch, ie. it's a valid placeholder on a leftmost path -func leftBranchesAreEmpty(spec *InnerSpec, op *InnerOp) bool { +func leftBranchesAreEmpty(spec *InnerSpec, op *InnerOp) (bool, error) { idx, err := orderFromPadding(spec, op) if err != nil { - return false + return false, err } // count branches to left of this leftBranches := int(idx) if leftBranches == 0 { - return false + return false, nil } // compare prefix with the expected number of empty branches actualPrefix := len(op.Prefix) - leftBranches*int(spec.ChildSize) if actualPrefix < 0 { - return false + return false, nil } for i := 0; i < leftBranches; i++ { - idx := getPosition(spec.ChildOrder, int32(i)) + idx, err := getPosition(spec.ChildOrder, int32(i)) + if err != nil { + return false, err + } + from := actualPrefix + idx*int(spec.ChildSize) if !bytes.Equal(spec.EmptyChild, op.Prefix[from:from+int(spec.ChildSize)]) { - return false + return false, nil } } - return true + return true, nil } // rightBranchesAreEmpty returns true if the padding bytes correspond to all empty siblings // on the right side of a branch, ie. it's a valid placeholder on a rightmost path -func rightBranchesAreEmpty(spec *InnerSpec, op *InnerOp) bool { +func rightBranchesAreEmpty(spec *InnerSpec, op *InnerOp) (bool, error) { idx, err := orderFromPadding(spec, op) if err != nil { - return false + return false, err } // count branches to right of this one rightBranches := len(spec.ChildOrder) - 1 - int(idx) if rightBranches == 0 { - return false + return false, nil } // compare suffix with the expected number of empty branches if len(op.Suffix) != rightBranches*int(spec.ChildSize) { - return false // sanity check + return false, nil // sanity check } for i := 0; i < rightBranches; i++ { - idx := getPosition(spec.ChildOrder, int32(i)) + idx, err := getPosition(spec.ChildOrder, int32(i)) + if err != nil { + return false, err + } + from := idx * int(spec.ChildSize) if !bytes.Equal(spec.EmptyChild, op.Suffix[from:from+int(spec.ChildSize)]) { - return false + return false, nil } } - return true + return true, nil } // getPosition checks where the branch is in the order and returns // the index of this branch -func getPosition(order []int32, branch int32) int { +func getPosition(order []int32, branch int32) (int, error) { if branch < 0 || int(branch) >= len(order) { - panic(fmt.Errorf("invalid branch: %d", branch)) + return -1, fmt.Errorf("invalid branch: %d", branch) } for i, item := range order { if branch == item { - return i + return i, nil } } - panic(fmt.Errorf("branch %d not found in order %v", branch, order)) + + return -1, fmt.Errorf("branch %d not found in order %v", branch, order) } // This will look at the proof and determine which order it is... @@ -434,7 +490,10 @@ func getPosition(order []int32, branch int32) int { func orderFromPadding(spec *InnerSpec, inner *InnerOp) (int32, error) { maxbranch := int32(len(spec.ChildOrder)) for branch := int32(0); branch < maxbranch; branch++ { - minp, maxp, suffix := getPadding(spec, branch) + minp, maxp, suffix, err := getPadding(spec, branch) + if err != nil { + return 0, err + } if hasPadding(inner, minp, maxp, suffix) { return branch, nil } diff --git a/go/proof_test.go b/go/proof_test.go index 2c27169..0c0e3d9 100644 --- a/go/proof_test.go +++ b/go/proof_test.go @@ -2,6 +2,7 @@ package ics23 import ( "bytes" + "fmt" "testing" ) @@ -66,12 +67,86 @@ func TestEmptyBranch(t *testing.T) { if err := tc.Op.CheckAgainstSpec(tc.Spec, 1); err != nil { t.Errorf("Invalid InnerOp: %v", err) } - if leftBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op) != tc.IsLeft { + leftBranchesAreEmpty, err := leftBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op) + if err != nil { + t.Errorf("Error: %v", err) + } + if leftBranchesAreEmpty != tc.IsLeft { t.Errorf("Expected leftBranchesAreEmpty to be %t but it wasn't", tc.IsLeft) } - if rightBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op) != tc.IsRight { + rightBranchesAreEmpty, err := rightBranchesAreEmpty(tc.Spec.InnerSpec, tc.Op) + if err != nil { + t.Errorf("Error: %v", err) + } + if rightBranchesAreEmpty != tc.IsRight { t.Errorf("Expected rightBranchesAreEmpty to be %t but it wasn't", tc.IsRight) } }) } } + +func TestGetPosition(t *testing.T) { + cases := []struct { + name string + order []int32 + branch int32 + expPos int + expError error + }{ + { + name: "success: first branch", + order: IavlSpec.InnerSpec.ChildOrder, + branch: 0, + expPos: 0, + expError: nil, + }, + { + name: "success: second branch", + order: IavlSpec.InnerSpec.ChildOrder, + branch: 1, + expPos: 1, + expError: nil, + }, + { + name: "failure: negative branch value", + order: IavlSpec.InnerSpec.ChildOrder, + branch: -1, + expPos: -1, + expError: fmt.Errorf("invalid branch: %d", int32(-1)), + }, + { + name: "failure: branch is greater than length of child order", + order: IavlSpec.InnerSpec.ChildOrder, + branch: 3, + expPos: -1, + expError: fmt.Errorf("invalid branch: %d", int32(3)), + }, + { + name: "failure: branch not found in child order", + order: []int32{0, 2}, + branch: 1, + expPos: -1, + expError: fmt.Errorf("branch %d not found in order %v", 1, []int32{0, 2}), + }, + } + + for _, tc := range cases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + pos, err := getPosition(tc.order, tc.branch) + if tc.expError == nil && err != nil { + t.Fatal(err) + } + + if tc.expError != nil && (err.Error() != tc.expError.Error() || pos != -1) { + t.Fatalf("expected: %v, got: %v", tc.expError, err) + } + + if tc.expPos != pos { + t.Fatalf("expected position: %d, got: %d", tc.expPos, pos) + } + }) + + } +}