Handle the app version when state syncing

[evan-forbes]

In:

• fix: state sync on v2.x #3808
• fix: state sync on main #3811

we hacked around the issues of properly passing the app version to the kv store while mounting it for state sync

celestia-app/app/app.go

Lines 789 to 813 in ad250dd

	// OfferSnapshot is a wrapper around the baseapp's OfferSnapshot method. It is
	// needed to mount stores for the appropriate app version.
	func (app *App) OfferSnapshot(req abci.RequestOfferSnapshot) abci.ResponseOfferSnapshot {
	if app.IsSealed() {
	// If the app is sealed, keys have already been mounted so this can
	// delegate to the baseapp's OfferSnapshot.
	return app.BaseApp.OfferSnapshot(req)
	}
	if app.upgradeHeightV2 == 0 {
	app.Logger().Debug("v2 upgrade height not set, assuming app version 2")
	app.mountKeysAndInit(v2)
	return app.BaseApp.OfferSnapshot(req)
	}
	if req.Snapshot.Height >= uint64(app.upgradeHeightV2) {
	app.Logger().Debug("snapshot height is greater than or equal to upgrade height, assuming app version 2")
	app.mountKeysAndInit(v2)
	return app.BaseApp.OfferSnapshot(req)
	}
	app.Logger().Debug("snapshot height is less than upgrade height, assuming app version 1")
	app.mountKeysAndInit(v1)
	return app.BaseApp.OfferSnapshot(req)
	}

To close this issue, we should come up with a long term solution that works for all future app versions.

related to:

• Add a test for state sync #3807

[rootulp]

Notes from meeting:

1. Try to implement this in a way that isn't breaking for v2.x.
2. Consider adding this field:

type RequestOfferSnapshot struct {
	Snapshot *Snapshot `protobuf:"bytes,1,opt,name=snapshot,proto3" json:"snapshot,omitempty"`
	AppHash  []byte    `protobuf:"bytes,2,opt,name=app_hash,json=appHash,proto3" json:"app_hash,omitempty"`
+	AppVersion uint64
}

3. If a node gets a snapshot that doesn't include AppVersion, fall back to the existing approach where we infer it based on --v2-upgrade-height
4. [optional] since we're modifying ABCI types, consider adding AppVersion to RequestInfo

[cmwaters]

If a node gets a snapshot that doesn't include AppVersion, fall back to the existing approach where we infer it based on --v2-upgrade-height

It can also potentially reject that snapshot and tendermint will search for another one but I think it's probably better that we fallback

I'm not sure if we should add it to the request like you've shown or to the Snapshot struct. I had a feeling when I looked at the plumbing that having it in the actual snapshot would be easier but would need to double check.

4. Doesn't have to be coupled with this but it would be nice to have it with the multiapp and v3 as I believe this would clean up the initialisation a lot. (Remember we will still be doing this off the v0.34.x branch so regardless v2 will probably end up having the AppVersion in the request regardless of whether we use it or not

[rootulp]

Let's de-couple 4 from this issue: #3844

[rootulp]

Notes from investigation:

• celestia-core calls list snapshots
• cosmos-sdk implements list snapshots
• We can't co-opt the existing snapshot format field to use it for app version. Note if we add a field to the snapshot type then we likely must bump https://github.com/celestiaorg/cosmos-sdk/blob/13d3e17430f313fdf4bd2e5fa9b68ae41768f907/snapshots/types/format.go#L3-L6
• We can't co-opt the existing snapshot metadata field because it's populated
• It may be possible to query the consensus params at the height of the snapshot and use that version. See state sync specs. Looks like this happens here.