ADR-40 Effect on IBC

[AdityaSripal]

Explanation of the Proposed Changes and the Proposed Fix

The SDK team is looking to make some major changes to the way stores work in IBC. There are separate but related proposed changes.

The first is to replace IAVL stores with SMT stores.
The second is to remove the Multistore altogether with each seperate store becoming a prefixed subspace of a single root store.

Here are the discussions on the SDK:
cosmos/cosmos-sdk#9331
cosmos/cosmos-sdk#8297
cosmos/cosmos-sdk#9331 (reply in thread)
cosmos/cosmos-sdk#8297 (reply in thread)

The first change should be supported natively in IBC without any changes to the protocol. This is because we simply need to change the ProofSpecs on the counterparty clients from IAVLSpec to SMTSpec.

The upgrade process would be as follows:

1. Upgrading chain passes an UpgradeProposal with a new UpgradedClientState that includes the changed ProofSpec with an upgrade height (time-based upgrades disallowed)
2. Governance passes the proposal
3. The Upgrade occurs at upgrade height and all nodes on the chain halt and perform upgrade
4. Relayers query the UpgradedClientState along with a proof from the old chain state (thus, there must be at least 1 node that does not immediately perform upgrade but instead just halts at upgrade height)
5. Relayers submit the UpgradeClientMsg to all counterparty chains, which then can upgrade their clients upon receiving the new client state with associated proof.

Note: This only requires a proposal on the upgrading chain, all counterparties can upgrade their client after upgrade height with a relayer msg.

The second proposed fix is not currently supported in IBC. The reasons why are explained here.

A fix on ibc-go that requires all counterparties to upgrade will be very costly from a time and coordination point-of-view. The current estimate is that this could be upwards of a year time for all important chains to upgrade: cosmos/cosmos-sdk#8297 (reply in thread)

There does exist a fix that does not require all counterparties to upgrade. It would work like so:

1. Upgrading chain passes an UpgradeProposal with a new UpgradedClientState that includes the changed ProofSpec to [NoOpSpec, SMTSpec]. The migration for the upgrade also moves the ibc store to the root level, effectively removing the ibc/ prefix from all ibc keys.

Same steps as above

All counterparty chains once their clients have upgraded are now verifying proofs of ibc state without the ibc/ prefix. Similarly, the upgraded chain is now storing all ibc state without the ibc/ prefix. Since both the prover and verifier are back to agreement on the key structure, all the proof verification will pass.

In the future, once all counterparties have upgraded to a version that supports better prefixing logic. The upgrading chain can do a second upgrade to add back the ibc prefix.

1. Upgrading chain passes UpgradeProposal with new ProofSpecs SMTSpec. The migration moves all ibc state from root level back to the ibc prefix.
2. Same steps as above

Now the counterparties chains will apply the ibc/ prefix as a prepend to the original key rather than as a separate key in a chained proof. The prover and verifier are in agreement on key structure, and the proofs pass.

[AdityaSripal]

Explanation of the work involved on IBC side to support Proposed Changes

So to support the second change, there will need to be a non-trivial amount of work on the IBC team.

Implementation work for the first upgrade to remove prefix:

1. Implement and test a NoOpSpec
2. Change Default ProofSpecs to NoOpSpec, SMTSpec, and change the connection VerifySelfClient logic to enforce counterparties use NoOpSpec, SMTSpec for the versions of ibc-go that is compatible with "no-multistore" SDK
3. Write Migration script to move ibc store to root level (trivial to implement, but must test)
4. Manually test the upgrade to ensure that there are no issues upgrading existing connections

Interim Technical Debt:

1. Counterparty clients will have a NoOpSpec in their proof specs that has no purpose but to ensure that proof verification ignores the ibc prefix set in the connection.
2. Upgraded chains will have all ibc state set at the root level. This is not so bad since all IBC state is further prefixed by clients/, connections/, channels/. But it is still not ideal, thus we want to ensure a path back to adding back prefix.

Removing Technical Debt once all counterparties upgrade to better ApplyPrefix Logic (~1-2 yrs later):

1. Change Default ProofSpecs to NSMTSpec, and change the connection VerifySelfClient logic to enforce counterparties use SMTSpec.
2. Write Migration script to move all ibc state back into ibc/ (slightly less trivial than previous migration, must test this)
3. Manually test the upgrade to ensure there are no issues upgrading existing connections.

The work required on this I believe will take 1 month of time on each end, and in the interim (~1-2 yrs) there is some hacky technical debt on legacy chains. However, I appreciate that this is a tradeoff between having hacky technical debt on SDK for 1-2 yrs, so this discussion is here to help us decide on a path forward that is best for both developer teams and the community at large. I've laid out the backwards-compatible solution from IBC and the work/technical debt involved to achieve it; hopefully that helps steer the discussion to the best outcome.

cc: @robert-zaremba @zmanian @colin-axner @cwgoes @marbar3778

[robert-zaremba]

Implement and test a NoOpSpec

I thought this is already implemented. So if's not, then it will require all counterparty chains to do the ibc module upgrade?

[robert-zaremba]

BTW, for the prefix removal migration. For the MultiStore removal we were planning the following migration:

• move all objects (key, values) from every multistore store to the new "general" store, by prepending the store prefix to the key - this is required to assure no collision.
• we can make an exception and don't prefix IBC objects when moving to the "general" store.

If we can't apply this migration for the IBC "sub-store" then w need to assure no collision. I think this is done by the hash function itself - you hash objects, which are not present in other stores.

However, what's more difficult is the second phase - if we have general store and we don't have ibc/ prefix there, how will we know which objects "belong" to the IBC module?

[AdityaSripal]

I thought this is already implemented. So if's not, then it will require all counterparty chains to do the ibc module upgrade?

No the counterparties don't need to upgrade. They will receive a new proof spec, but they are capable of handling any new proof spec struct without any issues.

However, what's more difficult is the second phase - if we have general store and we don't have ibc/ prefix there, how will we know which objects "belong" to the IBC module?

While we're removing the ibc prefix. Everything in ibc store is further prefixed by which sub-module the key is in. So everything in ibc-store will be prefixed by clients/, connections/ or channels/. So to do second phase we iterate through everything in these prefixes and add the additional prefix ibc/

[robert-zaremba]

ah, so with "Implement and test a NoOpSpec", you mean the proof spec (as we were discussing), not a new IBC operator, right?

[AdityaSripal]

Hmmm, I think it may still require the counterparty to change its verification logic upon receiving NoOpSpec. Which doesn't help

I have to think of if there's a way to use the current verification logic, with new proof spec parameters and get essentially a no-op proof. Idk, i'll think about it over the weekend.

[robert-zaremba]

Was the UpgradeClientMsg already tested in a live chain? In other words, was there a live network tests of updating counter-party clients?

[AdityaSripal]

This was manually tested by @colin-axner against a live testing chain and it is confirmed to work

[robert-zaremba]

Second solution for the Multistore removal for IBC.

We were discussing the second solution for Multistore removal:

• we will keep it only for the IBC module
• we will need to make a "special" case for the IBC module - not a big deal.
• all modules, except IBC, will store keys in the general store.
• we will try to reduce the interface and the functionality of Multistore

[robert-zaremba]

The RootStore should be the only point where prefixed substores need to be registered, and that should happen at initialization to efficiently encode prefixes.

So we need an additional level of coordination for the chain upgrade?