feat!: disable ibc upgrade proposal handler

[cmwaters]

The proposal handler to upgrade IBC also invokes the standard ScheduleUpgrade within the upgrading module. See https://github.com/cosmos/ibc-go/blob/e2201aaf1b016356bbd40fcdc17988437adce5ae/modules/core/02-client/keeper/proposal.go#L82. This effectively means that one can create a governance proposal to upgrade the chain which is something we didn't want to support. As there is no need to handle updating the IBC client yet, this temporarily disables the proposal type.

[evan-forbes]

This effectively means that one can create a governance proposal to upgrade the chain which is something we didn't want to support.

mind elaborating on the flow that we would go through? I'm good with merging this now, and then we can expand on the existing tests to ensure that we don't accidently enable this

[cmwaters]

I create a governance proposal with *types.UpgradeProposal as the Content. In there I include a types.Plan to upgrade at height 10,000. If it passes, ScheduleUpgrade is called on our standard SDK upgrade keeper. This will create the plan and at height 10,000 will result in all nodes panicking and halting (until people switch to the new version)

[cmwaters]

@evan-forbes do you want me to try write a test?

[evan-forbes]

do you want me to try write a test?

this working makes sense to me, but it would be good practice I think to write a test to ensure that this can actually happen and we're not missing something small or hidden before creating a consensus breaking change with some potetially important (albiet temporary as we could always fix with a hardfork) consequences with IBC

[evan-forbes]

I'm still missing as to why this exists in the first place tbh. Why should another chain be able to trigger an upgrade ever? I was aware of the the client upgrades, as what is described in the linked docs, and those make sense. This specific code described doesn't make sense, but is clearly there.

edit: per a sync discussion, this still isn't documented that well, but does make more sense to me now. This seems to just be a helper function to update the client state and schedule an upgrade simultaneously. Imo its simpler to just create a normal upgrade and handle everything there, including updating the client state if necessary.

[cmwaters]

I've added a test to the x/upgrade/test directory that uses the IBC Upgrade Proposal Handler. Without the modifications this test validated that in fact the proposal would pass (and assumable cause the upgrade module to invoke an upgrade at the specified height.

Just as further context, the reason why there are two governance related messages for invoking a classic sdk styled upgrade is one is a normal state machine breaking change while the ibc upgrade is for ibc specific breaking changes whereby the client state needs to be upgraded.

[codecov-commenter]

Codecov Report

Merging #2574 (7c545ed) into main (a3d2453) will decrease coverage by 0.02%.
Report is 2 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2574      +/-   ##
==========================================
- Coverage   20.64%   20.63%   -0.02%     
==========================================
  Files         132      133       +1     
  Lines       15334    15344      +10     
==========================================
  Hits         3166     3166              
- Misses      11865    11875      +10     
  Partials      303      303              

Files	Coverage Δ
app/app.go	4.52% <0.00%> (ø)
app/ibc_proposal_handler.go	0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

[renaynay]

alternatively t.Cleanup

[evan-forbes]

thanks for adding the test! really great find! 👍