Make binaries have protection against accidental downgrades

[ValarDragon]

Summary

A common issue node operators have is accidentally starting with their default installed binary, after cosmovisor has done an automatic upgrade. If their binary is on the wrong version, they compute the wrong app hash, and their node gets stuck. (And since we can't rollback, #10281 #10198 you have to redownload your entire state)

We should add protection against this as well.

Proposal

On node start, we should look at the state to see all the upgrades plans executed, and what upgrade plans are registered. If there is a plan that has been executed, that has not been registered, it should halt saying that this binary is a downgrade.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

Yes! This actually happened to me on Osmosis due to an invalid setup of cosmovisor.

I think the proposal makes sense. If there is support from @AmauryM or @robert-zaremba, I can tackle it.

[robert-zaremba]

ACK - we can use info from upgrade module.

[robert-zaremba]

@alexanderbez - if you have time you can go with it. We could do a short call. Otherwise during the standup we were talking about this task and @aleem1314 could handle it.

[alexanderbez]

@aleem1314 go for it! Happy to review :)

[anilcse]

@robert-zaremba I am not sure if we can access upgrade info from store before starting the app, I believe we cannot. That is the reason we are using upgrade-info.json file for the upgrade information.
cc @aleem1314

[ValarDragon]

Can we add something in BeginBlock that checks this then, or make a new hook in the server's main command for this safety check?

This would help a lot with v0.42.x node upgrades as well.

[alexanderbez]

Yeah we should really strive to do something here to prevent this execution path. ACK @ValarDragon's idea.

[aleem1314]

Can we add something in BeginBlock that checks this then, or make a new hook in the server's main command for this safety check?

Yes, we can add this check in the BeginBlock. But we don't have enough info in the upgrade module to check the binary version.
In the upgrade module, we are storing executed plans and registered upgrades if any. That is not enough to check the binary version.

[ValarDragon]

Yes, we can add this check in the BeginBlock. But we don't have enough info in the upgrade module to check the binary version.

Why do you need to look at the binary version? We should look at the state to see all the upgrades plans executed, and what upgrade plans are registered. If there is a plan that has been executed, but has not been registered, then we know we are on an old version

[anilcse]

Yes, we can add this check in the BeginBlock. But we don't have enough info in the upgrade module to check the binary version.

Why do you need to look at the binary version? We should look at the state to see all the upgrades plans executed, and what upgrade plans are registered. If there is a plan that has been executed, but has not been registered, then we know we are on an old version

I believe we are pruning the plans once they are executed.

[ValarDragon]

How does osmosisd q upgrade applied v4 work then? It returns the header of the block the upgrade was applied at.

It calls this function internally:

cosmos-sdk/x/upgrade/keeper/keeper.go

Lines 230 to 239 in e657c7f

	// GetDoneHeight returns the height at which the given upgrade was executed
	func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 {
	store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{types.DoneByte})
	bz := store.Get([]byte(name))
	if len(bz) == 0 {
	return 0
	}
	return int64(binary.BigEndian.Uint64(bz))
	}

So presumably all the upgrades can be obtained by iterating over the store.

[robert-zaremba]

We only prune skipped upgrades.