[Bug]: upgrade module increments GasMeter non-deterministically across validators

[pr0n00gler]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

The upgrade module's BeginBlocker function increments gasMeter non-deterministially across validators.

The issue is similar to #15015

On a first block after a start a validator's k.DowngradeVerified() equals to false, so we fall down into a condition, where k.DowngradeVerified() becomes equal to true and
lastAppliedPlan is being read from the storage.

In further blocks k.DowngradeVerified() will always be equal to true and lastAppliedPlan will never be read again.

The issue happens if a validator has been restarted on a working chain.
When a validator is restarted, it tries to read lastAppliedPlan since k.DowngradeVerified() is not set yet.

So long as SetGasMeter is always called as part of the AnteHandler chain (which is the current behavior of the default AnteHandler chain here), then this is not an issue as the GasMeter will be reset between BeginBlock and the first DeliverTx call.

However, if the application uses a custom AnteHandler chain that omits this behavior, the GasUsed within each transaction will vary, as the GasMeter will be the initial meter set during BeginBlock here which increments gas differently if the upgrade module's k.DowngradeVerified() has not yet been set.

As a solution, should we consider moving ctx initialization from L83 to L25 and also reset a usual GasMeter?
Like this:

ctx = ctx.WithBlockGasMeter(storetypes.NewInfiniteGasMeter()).WithGasMeter(storetypes.NewInfiniteGasMeter())

Cosmos SDK Version

0.47.x, 0.50.x

[alexanderbez]

However, if the application uses a custom AnteHandler chain that omits this behavior,

May I ask why such an ante-handler chain omits this? Most, if not all, chains have their own unique ante-handler chain, which is encouraged. Why omit the basic decorator?

That being said, moving the context override uptop seems like it'll work, but I want to be extra cautious of any material impact it might have on subsequent API calls.

[pr0n00gler]

Skip's Block SDK mistakenly avoids calling AnteHandlers for their tx (we are already working with them to fix the bug in their SDK) and we ran into described issue in the Cosmos SDK.

I don't think there is a reason to avoid calling NewSetUpContextDecorator(), but since AnteHandlers can be customised, don't think it's good to have even a possibility of a consensus failure.
And in general don't think it's good to have different gas consumptions on different validators in the same BeginBlocker.

If idea of the fix is good, we can make a PR with the fix implemented.

[alexanderbez]

I do not see any major objections, given sufficient test coverage 👍

[beer-1]

Capability also have same problem when it call InitMemStore

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

Line 122 in 03d578b

noGasCtx := ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter())

it only change block gas meter to infinity gas meter, but it also need to change gas meter to infinity gas meter.

[facundomedica]

@pr0n00gler do you want to open a PR? Or should I work on it?

[julienrbrt]

For reference, Neutron added this fix in their fork: neutron-org#11

[RaulBernal]

Do you think guys this issue could be directly related with mine here (ignite/cli#4141) ?