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: apply default restrictions to max depth #352

Merged
merged 12 commits into from
Aug 27, 2024
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

# Unreleased

- fix(go): interpret max_depth in inner specs as 128 if left to 0 [#352](https://github.com/cosmos/ics23/pull/352).

# 0.12.0

## Rust
Expand Down
2 changes: 1 addition & 1 deletion go/fuzz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func FuzzExistenceProofCheckAgainstSpec(f *testing.F) {

seedDataMap := CheckAgainstSpecTestData(f)
for _, seed := range seedDataMap {
if seed.IsErr {
if seed.Err != "" {
// Erroneous data, skip it.
continue
}
Expand Down
8 changes: 7 additions & 1 deletion go/proof.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,13 @@ func (p *ExistenceProof) CheckAgainstSpec(spec *ProofSpec) error {
if spec.MinDepth > 0 && len(p.Path) < int(spec.MinDepth) {
return fmt.Errorf("innerOps depth too short: %d", len(p.Path))
}
if spec.MaxDepth > 0 && len(p.Path) > int(spec.MaxDepth) {

maxDepth := spec.MaxDepth
if maxDepth == 0 {
maxDepth = 128
}
Comment on lines +194 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a note to proto doc that if max_depth is zero then we treat as default 128?

https://github.com/cosmos/ics23/blob/master/proto/cosmos/ics23/v1/proofs.proto#L165-L166

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing 👍


if len(p.Path) > int(maxDepth) {
return fmt.Errorf("innerOps depth too long: %d", len(p.Path))
}

Expand Down
3 changes: 2 additions & 1 deletion go/proof_data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func CheckLeafTestData(tb testing.TB) map[string]CheckLeafTestStruct {
type CheckAgainstSpecTestStruct struct {
Proof *ExistenceProof
Spec *ProofSpec
IsErr bool
Err string
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make this error instead of string?

Copy link
Contributor

@damiannolan damiannolan Aug 19, 2024

Choose a reason for hiding this comment

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

maybe not worth doing this tbh. I see the testdata json structs are unmarshalled to this struct and we'd need to handle custom unmarshalJSON functionality to handle the built-in error interface, this is probably why bool was used to begin with

Using string is fine by me 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, I tried doing error but felt it was too much work to dig further into

}

func CheckAgainstSpecTestData(tb testing.TB) map[string]CheckAgainstSpecTestStruct {
Expand All @@ -70,6 +70,7 @@ func CheckAgainstSpecTestData(tb testing.TB) map[string]CheckAgainstSpecTestStru
if err != nil {
tb.Fatal(err)
}

return cases
}

Expand Down
7 changes: 3 additions & 4 deletions go/proof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,15 @@ func TestCheckLeaf(t *testing.T) {
}

func TestCheckAgainstSpec(t *testing.T) {
t.Skip()
cases := CheckAgainstSpecTestData(t)

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
err := tc.Proof.CheckAgainstSpec(tc.Spec)
if tc.IsErr && err == nil {
t.Fatal("Expected error, but got nil")
} else if !tc.IsErr && err != nil {
if tc.Err == "" && err != nil {
t.Fatalf("Unexpected error: %v", err)
} else if tc.Err != "" && tc.Err != err.Error() {
t.Fatalf("Expected error: %s, got %s", tc.Err, err.Error())
}
})
}
Expand Down
1 change: 1 addition & 0 deletions go/proofs.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions proto/cosmos/ics23/v1/proofs.proto
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ message ProofSpec {
LeafOp leaf_spec = 1;
InnerSpec inner_spec = 2;
// max_depth (if > 0) is the maximum number of InnerOps allowed (mainly for fixed-depth tries)
// the max_depth is interpreted as 128 if set to 0
int32 max_depth = 3;
// min_depth (if > 0) is the minimum number of InnerOps allowed (mainly for fixed-depth tries)
int32 min_depth = 4;
Expand Down
2 changes: 2 additions & 0 deletions rust/src/cosmos.ics23.v1.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// This file is @generated by prost-build.
/// *
/// ExistenceProof takes a key and a value and a set of steps to perform on it.
/// The result of peforming all these steps will provide a "root hash", which can
Expand Down Expand Up @@ -146,6 +147,7 @@ pub struct ProofSpec {
#[prost(message, optional, tag = "2")]
pub inner_spec: ::core::option::Option<InnerSpec>,
/// max_depth (if > 0) is the maximum number of InnerOps allowed (mainly for fixed-depth tries)
/// the max_depth is interpreted as 128 if set to 0
#[prost(int32, tag = "3")]
pub max_depth: i32,
/// min_depth (if > 0) is the minimum number of InnerOps allowed (mainly for fixed-depth tries)
Expand Down
Binary file modified rust/src/proto_descriptor.bin
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was added when I ran make proto-all. Hope it is correct if I update it @romac

Binary file not shown.
Loading
Loading