Breaking change in proofs.proto with v0.11.0.

[SpicyLemon]

The proofs.proto file in v0.11.0 is not backwards compatible to v0.10.0.

Error: Enum value "3" on enum "HashOp" changed name from "KECCAK" to "KECCAK256".
Error: buf found 1 breaking changes.

That error is from the bufbuild/buf-breaking-action@v1.1.4 github action that's run in another repo. I believe that pretty much just runs the buf breaking command.

For us, it's checking against our own set of third-party protobuf files that we previously submitted to the buf registry and which includes your proofs.proto file. When we tried to bump ics23/go from v0.10.0 to v0.11.0, we grabbed a new copy of the proofs.proto file, which caused the buf breaking check to fail.

[colin-axner]

Yea it looks like this CI was also run on the pr that made the change https://github.com/cosmos/ics23/actions/runs/7181837418/job/19557046360?pr=217, I'm unsure why the warning was ignored (or at least commented on).

The guidelines around what marks a breaking change in protobuf are notoriously a bit vague here, which is why buf sets a guideline. We are primarily concerned with over the wire changes. In this case, to the best of my knowledge, proto3 encodings won't change based on the name (as I believe they reference the field number), but proto3json does since json uses the field name in encodings. In ibc-go, I'm not sure in what case one would be proving a proof spec json encoded, or at least one would need to go out of their way to create some sequence of non sense actions to create an unreceivable packet (which would timeout). Nevertheless, it might have been nice to leave the field name as is so not to cause unnecessary trouble over a small naming difference, but what's done is done.

Thank you @SpicyLemon for being active and opening an issue. Do you have concerns with your own CI failing (ie it's a problem that needs to be resolved)? Do you have concerns of the nature of this change or were you mostly opening an issue to remark on an anomaly?

[SpicyLemon]

Primarily, I just noticed the breaking check failing in our stuff and wanted to bring it to your attention. I'm not very familiar with this library or that proto, so I don't really know how that enum gets used. It seemed reasonable to assume that some stuff out there depends on the JSON encoding, but I don't know of anything specific.

If you feel it's safe, that's good enough for me.

[damiannolan]

proto3 encodings won't change based on the name (as I believe they reference the field number)

This is correct wrt to protobuf encoding. However, yes, I agree that something that solely depended on the JSON encoding using this enum type would be effected.
From the perspective of IBC, the only scenario I could think of which would cause an issue is creation of client (where proof spec is defined within it) in JSON encoding over ICA (json - note, proto3 is default here iirc). Which as @colin-axner points out, would ultimately just fallback to a timed out packet, as it would be unreceivable by the destination host. The usecase for such an action is also unclear to me right now.

what's done is done

Agreed!
The PR which made this change does mention that this HashOp is unused to the best of their knowledge at the moment.
Sadly, we can't do much about this now. But in order to close this issue perhaps we could take an action item of making the protobuf breaking CI check a required pass in order to merge. What do you think?

[colin-axner]

The protobuf break check is now a required check for all pr's! I will close this issue as we believe the break introduced in v0.11.0 is safe given the context of IBC usage. Thank you @SpicyLemon for bringing this to our attention! 🙏