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

[Bug]: Panic in proto decoding (v0.52) #22574

Closed
1 task done
chatton opened this issue Nov 20, 2024 · 10 comments · Fixed by #22852
Closed
1 task done

[Bug]: Panic in proto decoding (v0.52) #22574

chatton opened this issue Nov 20, 2024 · 10 comments · Fixed by #22852
Assignees
Labels
C: Proto Proto definition and proto release C:x/tx T:Bug

Comments

@chatton
Copy link
Contributor

chatton commented Nov 20, 2024

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

While upgrading ibc-go to v0.52.0, we ran into an issue in which some tests were failing when broadcasting a message. They were failing with the following error

 builder.go:103: test panicked: unable to decode tx: ibc.core.channel.v1.UpgradeFields: {TagNum: 1, WireType:"varint"}: unknown protobuf field: tx parse error [/home/cianhatton/go/pkg/mod/cosmossdk.io/x/[email protected]/decode/decode.go:97]

We were able to fix the issue by moving all of the contents of this file into this file.

link to list of replace statements being used.

Cosmos SDK Version

v0.0.0-20241108061010-a0458127fccf

How to reproduce?

Checkout this commit 0eb8205cd77c2b9d9bce7130726eb04238bafb0e in the ibc-go repo and run make test

@chatton chatton added the T:Bug label Nov 20, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Cosmos-SDK Nov 20, 2024
@julienrbrt julienrbrt added C: Proto Proto definition and proto release C:x/tx labels Nov 20, 2024
@damiannolan
Copy link
Member

We stepped through this and found that there is no FieldDescriptors found for the UpgradeFields protobuf message when landing here in x/tx/decode - https://github.com/cosmos/cosmos-sdk/blob/main/x/tx/decode/unknown.go#L40

@damiannolan
Copy link
Member

Will add screenshots for clarity.

  1. On commit 0eb8205cd77c2b9d9bce7130726eb04238bafb0e as mentioned in the issue we see FieldDescriptor == nil.

Image

  1. When moving the contents of the proto files as @chatton mentions we see it fulfilled correctly.

Image

@tac0turtle
Copy link
Member

i was debugging this as well on ibc but never completed it. Is the generated file from the upgrade imported into a module anywhere?

@tac0turtle tac0turtle self-assigned this Nov 21, 2024
@damiannolan
Copy link
Member

damiannolan commented Nov 21, 2024

Yeah, the generated upgrade.pb.go has its types used in 04-channel/types alongside everything else. e.g. here -> https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/types/upgrade.go#L15

Were you thinking that maybe the generated pb.go wasn't getting included by go pkg loader?

@julienrbrt julienrbrt assigned kocubinski and unassigned tac0turtle Dec 5, 2024
@kocubinski
Copy link
Member

I don't think this can be expected to work unless the protoregistry in google.golang.org/protobuf has the ibc proto descriptors. Right now they seem to only be in the gogo registry.

Image

In the above frame, the descriptor is indeed not loaded (see below) I'm not sure how the workaround you mentioned is working given this, but I'd expect other errors.

Image

@kocubinski
Copy link
Member

kocubinski commented Dec 12, 2024

This patched version of x/tx may work a6d28e3

Using this version of x/tx in ibc-go at the commit referenced above make test progressed past the above error to others which seem unrelated to tx decoding.

Can you try it out?

@damiannolan
Copy link
Member

damiannolan commented Dec 12, 2024

Thanks @kocubinski, I can try this out later today. I will report back and let you know here. Cheers

@damiannolan
Copy link
Member

Fix worked upgrading to that commit of x/tx.

@damiannolan
Copy link
Member

Thanks for getting this sorted! ❤

@kocubinski
Copy link
Member

after #22852 is merged we'll have an x/tx tagged alpha.3 if you want to use that

@github-project-automation github-project-automation bot moved this from 📋 Backlog to 🥳 Done in Cosmos-SDK Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Proto Proto definition and proto release C:x/tx T:Bug
Projects
Status: 🥳 Done
Development

Successfully merging a pull request may close this issue.

5 participants