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.

[robert-zaremba]

(sorry wrong button)

[robert-zaremba]

I think we can use InitChainer rather than BeginBlocker.

[ethanfrey]

I'm a bit confused.

I wrote this code and tested it in the original upgrade handler in 2019. I thought this was still there.

The old binary would die if it started too late. New binary would die if it started too soon.
Or is this a different issue?

[ethanfrey]

I think we can use InitChainer rather than BeginBlocker.

InitChainer is only called once at genesis, not when restarting

[robert-zaremba]

The old binary would die if it started too late. New binary would die if it started too soon.

Currently, the first case holds, the latter doesn't. I don't know what changed. @ethanfrey what was your original solution?

[anilcse]

@robert-zaremba I think it's vice-versa. It always fails if we start new binary early. If we start old-binary at the time of upgrade also it fails. But after the upgrade if we start old binary, as there's no active plan, it tries to produce blocks and thus creating consensus failures iiuc

[ethanfrey]

You can read https://medium.com/regen-network/amazonas-upgrade-guide-b31e9e18e0be

When running this in testnets 2 years ago, I had to handle both cases, to avoid operator error.

If you find the binary used for that upgrade and the regen/cosmos-sdk code used at that time, you can find my solution (which was 0.37/0.38 compatible)