Counterparty chains don't have ICS23 0.10.0 support.

[hdevalence]

We discovered a critical issue with ICS23 support on counterparty chains that will disrupt our ability to connect to them unless we work with other ecosystem participants.

ICS23 provides a generic mechanism for non-inclusion proofs: a non-inclusion proof for key k is a pair of inclusion proofs for keys k1, k2, a check that k1 and k2 are adjacent in the tree, and a check that k1 < k < k2. The problem is that this mechanism only works on ordered trees like the IAVL tree where the key ordering corresponds to the tree structure. On a sparse merkle tree like our JMT, this isn't the case -- sparsity is achieved by using the hash H(k) of the key, so keys are randomly ordered.

To fix this, we upstreamed a prehash_key_before_comparison field into the ICS23 spec. This provides a backwards-compatible extension supporting trees like the JMT: if the field is absent, it's false, and noninclusion proofs are verified as before; if it's present, it's true, and the noninclusion proof verifier checks that H(k1) and H(k2) are adjacent and that H(k1) < H(k) < H(k2)`.

Because all proto fields are optional and proto implementations are supposed to ignore unknown fields, this extension should also have been forwards-compatible, in the sense that a client could be created on a counterparty chain with prehash_key_before_comparison = true, with the following behavior:

• if the counterparty chain is using ICS23 0.10.0 or greater, it parses the prehash_key_before_comparison field and verifies non-inclusion proofs correctly;
• if the counterparty chain is using an older ICS23 version, it ignores the prehash_key_before_comparison field, and all non-inclusion proofs fail to verify.

In the latter case, packet timeouts don't work, but because the on-chain data is correct, as soon as the counterparty chain updates their ICS23 version, they would start working without any migrations to chain state.

However, what actually happens is that the Cosmos SDK rejects unknown fields: https://github.com/cosmos/cosmos-sdk/blob/a3a049bc2577632b94ff2236c2b01a250e85b16d/x/auth/tx/decoder.go#L27

To fix this, we need to urgently coordinate with the rest of the ecosystem to move them onto versions of ibc-go that use ics23 0.10.0 or greater. This is included by default in v7.1.0, but that version is much newer than many counterparty chains, so there will be longer upgrade latency. For instance, Osmosis is on v4. Ideally, we could convince the ibc-go maintainers to release backport releases that bump the ICS23 version.

[colin-axner]

Hi, thanks for the writeup @hdevalence. For clarification, the concern with opening connections occurs when the prehash_key_before_comparison is set to true or false? In previous releases when adding new fields in a backwards compatible way, we have relied on the ability to not encode empty fields to avoid the reject unknown fields issue. The proto codec (I'm fairly certain) does this be default, but the json marshaler implemented by the SDK does not. See here, EmitDefaults needs to be set to false.There is an issue for this cosmos/cosmos-sdk#13727, but in the meantime we have gotten around the issue by simply using a json marshaler which sets EmitDefaults to false

I'm curious if there's a fix related to the don't encode empty fields behaviour that could avoid requiring counterparties to perform software upgrades?

Assuming the above context doesn't help because you are concerned with creating a light client with the prehash_key_before_comparison key set to true before the counterparty has upgraded to v0.10, then indeed you would need counterparties to upgrade. You could potentially allow counterparties to run the light clients without timeout support in the meantime and then perform an IBC software upgrade to change counterparty proof specs to set the prehash key to true. You can see documentation on this in the IBC client breaking upgrades where changing the proof specs is supported functionality:

changing the ProofSpecs: Supported, this should be changed if the proof structure needed to verify IBC proofs is changed across the upgrade. Ex: Switching from an IAVL store, to a SimpleTree Store

This approach would not be very clean though as it would try to upgrade all counterparty clients to using the new field in ics23 v0.10 at the same moment, it is also a feature that has not been used very frequently so it may take some effort UX wise

[avahowell]

My current understanding w.r.t ICS23 0.10.0 support:

If the counterparties (cosmos SDK) had accepted unknown fields as we expected, we would have the situation where:

• The counterparties accept our proof specs and can form channels
• Channels, packets, and acks, all work normally (since the inclusion proof spec works)
• Timeouts will not work, since every proof verification fails without the changes we made in v0.10.0

Therefore, to enable timeouts in that scenario, the counterparty would have to:

• Upgrade their ibc-go impl to use ICS23 >0.10.0 to get noninclusion proofs for the JMT

Alternatively, in the situation we find ourselves in, where unknown fields are rejected, we have this situation:

• The counterparties can accept our proof specs, if we elide prehash_key_before_comparison (and allow this elision on the penumbra side (!!) )
• Channels, packets, and acks work normally (with this modified proof spec)
• Timeouts will not work, due to missing proof verification modification (same as above)

To enable timouts in that scenario, the counterparty has to:

• Upgrade their ibc-go impl to use ICS23 >0.10.0 to get noninclusion proofs for the JMT (same as above)
• Issue an IBC UpgradeClient to update the proofspec of every penumbra client

Additionally, there are security questions around allowing the prehash_key_before_comparison field to vary on the penumbra side (not doing strict validation of it). I'm not sure what those would be since I haven't done the security analysis, naively it doesn't seem like soundness would be effected, but I'm not sure. My intuition is that it would be a "fail closed" situation; no noninclusion proofs would verify.

The ideal situation is where counterparties upgrade to 0.10.0 before launch, this way there is no period of time where timeouts do not work, and no client upgrades required.

[hdevalence]

Tracking issue for Osmosis: osmosis-labs/osmosis#6482

[hdevalence]

Osmosis support may have landed: osmosis-labs/osmosis#6653

[hdevalence]

Still a blocker for Osmosis: osmosis-labs/osmosis#6482 (comment)

[avahowell]

Osmosis is working as of the latest testnet release v21

[hdevalence]

Marking this as done, we have a critical mass.