search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.27k stars 3.62k forks source link

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

Open pr0n00gler opened 11 months ago

pr0n00gler commented 11 months ago

Is there an existing issue for this?

What happened?

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

The issue is similar to https://github.com/cosmos/cosmos-sdk/issues/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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

beer-1 commented 11 months ago

Capability also have same problem when it call InitMemStore

https://github.com/cosmos/cosmos-sdk/blob/03d578b9de5136ffc658f2b0c07434373b07b7da/x/capability/keeper/keeper.go#L122

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

facundomedica commented 11 months ago

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

julienrbrt commented 11 months ago

For reference, Neutron added this fix in their fork: https://github.com/neutron-org/cosmos-sdk/pull/11/

RaulBernal commented 5 months ago

Do you think guys this issue could be directly related with mine here (https://github.com/ignite/cli/issues/4141) ?