Cosmos-sdk: Upgrade height judgment error

[dreamer-zq]

https://github.com/cosmos/cosmos-sdk/blob/1a5f2b78597805a2d254756f8b80e637e31760f1/x/upgrade/types/storeloader.go#L13

There are two errors in this place:

1. The CommitMultiStore has not been loaded yet, so ms.LastCommitID().Version is always equal to 0;

2. When the upgrade height is reached, the pain in the beginblocker, so the height has not yet been submitted, Should use ms.LastCommitID().Version == upgradeHeight-1

[anilCSE]

The implementation looks correct for me.

1. The app will have access to ms.LastCommitID().Version
2. The upgradeHeight refers to the height at which upgrade should happen. The upgrade should not be triggered before that height. ms.LastCommitID().Version == upgradeHeight, the logic looks fine for me.

cc @aaronc @ethanfrey

[ethanfrey]

1. The CommitMultiStore has not been loaded yet, so ms.LastCommitID().Version is always equal to 0;

At least in the standard gaia app (and all xrnd apps that tested this in public testnets), this is definitely not 0 and is loaded first.

1. When the upgrade height is reached, the pain in the beginblocker, so the height has not yet been submitted, Should use ms.LastCommitID().Version == upgradeHeight-1

This is a valid point and should be well documented. I think the current implementation is if you set an upgrade height to 54000, then block 54000 will be run with the old code and then the upgrade occurs before running 54001. This should be checked and documented in any case. Also, maybe we rename upgradeHeight to upgradeAfterBlock and then it is clear the given block number is run before the upgrade is applied (if that is the case)

[dreamer-zq]

Of course, it may be my understanding error, but I also tested it several times, and it does not load the height correctly. I think there may be a problem in this place:

func (app *BaseApp) LoadLatestVersion() error {
    err := app.storeLoader(app.cms)
    if err != nil {
        return fmt.Errorf("failed to load latest version: %w", err)
    }

    return app.init()
}

When calling app.storeLoader(app.cms), app.cms is not loaded, and then ms.LastCommitID().Version is used directly, so the version is always 0. I think that before calling ms.LastCommitID(), app.cms.LoadLatestVersion() should be called first. But in the logic of upgrading, I didn't see it. I don't know if my understanding is correct. For example: the modified code is as follows:

func (app *BaseApp) LoadLatestVersion() error {
        if err := app.cms.LoadLatestVersion() ;err != nil {
              return err
        }
    err := app.storeLoader(app.cms)
    if err != nil {
        return fmt.Errorf("failed to load latest version: %w", err)
    }

    return app.init()
}

@ethanfrey @anilCSE

[ethanfrey]

The issue is what you set app.storeLoader() to be.

The two StoreLoaders we provide as standard options both load the cms. In fact, the default is DefaultStoreLoader which is nothing more than cms.LoadLatestVersion()

The issue is likely a misconfiguration of storeLoader in your application.

[dreamer-zq]

Thanks, it is possible that the method I used is wrong or the order of the code is wrong

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[alessio]

Does this affect Launchpad as well?