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

Application panics on ICA host keeper instantiation #6435

Closed
3 tasks
GAtom22 opened this issue May 30, 2024 · 1 comment · Fixed by #6436
Closed
3 tasks

Application panics on ICA host keeper instantiation #6435

GAtom22 opened this issue May 30, 2024 · 1 comment · Fixed by #6436
Labels
type: bug Something isn't working as expected

Comments

@GAtom22
Copy link
Contributor

GAtom22 commented May 30, 2024

Summary of Bug

When trying to upgrade the Evmos chain to use ibc-go v7.5.0, encountered an issue related to the changes introduced in #5785.

On starting the chain binary, the application panics with the error

panic: proto: could not resolve import "iavl/changeset.proto": not found

This panic is triggered in the newModuleQuerySafeAllowList function, when calling gogoproto.MergedRegistry(). It is related to the memIAVL dependency, specifically the memiavl/wal.proto file.

The panic happens because the file is registered using the proto.RegisterFile function which uses FileOptions{AllowUnresolvable: true}, while the gogoproto.MergedRegistry() uses the function protodesc.NewFiles(), which uses FileOptions{}.NewFiles(fd) (i.e. AllowUnresolvable: false). In the protodesc.New() function called by the protodesc.NewFiles() there's the corresponding logic

...
               if err == protoregistry.NotFound && (o.AllowUnresolvable || imp.IsWeak) {
			f = filedesc.PlaceholderFile(path)
		} else if err != nil {
			return nil, errors.New("could not resolve import %q: %v", path, err)
		}
...

In the reported case, the error is protoregistry.NotFound and the o.AllowUnresolvable == false, so the application panics.

Expected Behaviour

Should not panic

Version

v7.5.0

And any other version with the changes introduced in #5785

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner
Copy link
Contributor

For my understanding, essentially there are imports that could not be resolved by proto. By allowing these imports to be unresolved and thus replaced by a placeholder file, we avoid a panic and this is safe because the files in question are unused?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants