fix(miner): Reset gas meter in prepare proposal. #1270
Conversation
WalkthroughThe changes primarily focus on improving the gas management in the Cosmos blockchain codebase. The updates include the addition of gas refunding and resetting mechanisms, reordering of code execution, and the removal of state plugin priming. These modifications enhance the efficiency and robustness of the system. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- cosmos/go.mod
- cosmos/go.sum
Files selected for processing (2)
- cosmos/miner/miner.go (3 hunks)
- cosmos/x/evm/keeper/processor.go (1 hunks)
Additional comments: 6
cosmos/x/evm/keeper/processor.go (1)
- 54-55: Good error handling here, wrapping the error provides more context which can be useful for debugging.
cosmos/miner/miner.go (5)
24-31: The import statement for
storetypespackage has been added. Ensure that this package is being used in the code and that it is a necessary import.96-98: The
PreBlockerfunction is being called withnilas the second argument. Ensure that this is the intended behavior and that the function can handle anilargument.104-110: The gas meter is being reset here. This is a critical operation and should be handled with care. Ensure that this operation does not introduce any side effects or vulnerabilities.
113-115: The
SetLatestQueryContextfunction is being called here. Ensure that the context being passed is the correct one and that it is being set at the right time in the execution flow.118-119: The
buildBlockfunction is being called here. Ensure that the context being passed is the correct one and that it is being set at the right time in the execution flow.
| // Reset GasMeter to 0. | ||
| gasMeter.RefundGas(gasMeter.GasConsumed(), "reset before evm block") | ||
| blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset before evm block") | ||
| defer gasMeter.ConsumeGas(gasMeter.GasConsumed(), "reset after evm") | ||
| defer gasMeter.RefundGas(gasMeter.GasConsumed(), "reset after evm") | ||
| defer blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset after evm") |
There was a problem hiding this comment.
The use of defer here is appropriate for ensuring the gas meters are reset after the function execution. However, it's important to note that if an error occurs and the function exits early, the deferred statements will still execute. This could potentially lead to incorrect gas meter readings if the function doesn't complete its intended operations. Consider adding error handling to prevent this scenario.
- defer gasMeter.RefundGas(gasMeter.GasConsumed(), "reset after evm")
- defer blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset after evm")
+ defer func() {
+ if err == nil {
+ gasMeter.RefundGas(gasMeter.GasConsumed(), "reset after evm")
+ blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset after evm")
+ }
+ }()Commitable suggestion (Beta)
| // Reset GasMeter to 0. | |
| gasMeter.RefundGas(gasMeter.GasConsumed(), "reset before evm block") | |
| blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset before evm block") | |
| defer gasMeter.ConsumeGas(gasMeter.GasConsumed(), "reset after evm") | |
| defer gasMeter.RefundGas(gasMeter.GasConsumed(), "reset after evm") | |
| defer blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset after evm") | |
| // Reset GasMeter to 0. | |
| gasMeter.RefundGas(gasMeter.GasConsumed(), "reset before evm block") | |
| blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset before evm block") | |
| defer func() { | |
| if err == nil { | |
| gasMeter.RefundGas(gasMeter.GasConsumed(), "reset after evm") | |
| blockGasMeter.RefundGas(blockGasMeter.GasConsumed(), "reset after evm") | |
| } | |
| }() |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- e2e/testapp/go.mod
- e2e/testapp/go.sum
Files selected for processing (1)
- cosmos/miner/miner.go (3 hunks)
Additional comments: 2
cosmos/miner/miner.go (2)
24-31: The new import "cosmossdk.io/store/types" is used for gas metering configuration. Ensure that the package is correctly imported and used.
92-97: The PreBlocker and BeginBlocker are run to get the chain into the state it'll be in when the EVM transaction actually runs. Ensure that these functions are correctly implemented and handle errors properly.
| return nil, err | ||
| } | ||
|
|
||
| ctx.GasMeter().RefundGas(ctx.GasMeter().GasConsumed(), "prepare proposal") | ||
| ctx.BlockGasMeter().RefundGas(ctx.BlockGasMeter().GasConsumed(), "prepare proposal") | ||
| ctx = ctx.WithKVGasConfig(storetypes.GasConfig{}). | ||
| WithTransientKVGasConfig(storetypes.GasConfig{}). | ||
| WithGasMeter(storetypes.NewInfiniteGasMeter()) | ||
|
|
||
| // We have to prime the state plugin. | ||
| if err = m.keeper.SetLatestQueryContext(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Trigger the geth miner to build a block. | ||
| if payloadEnvelopeBz, err = m.buildBlock(ctx); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The gas meter is refunded and reset, and the context is updated with a new gas configuration. The state plugin is primed with the updated context. The block is then built. Ensure that the gas metering and block building processes are correctly implemented and handle errors properly.
- ctx.GasMeter().RefundGas(ctx.GasMeter().GasConsumed(), "prepare proposal")
- ctx.BlockGasMeter().RefundGas(ctx.BlockGasMeter().GasConsumed(), "prepare proposal")
- ctx = ctx.WithKVGasConfig(storetypes.GasConfig{}).
- WithTransientKVGasConfig(storetypes.GasConfig{}).
- WithGasMeter(storetypes.NewInfiniteGasMeter())
+ gasConsumed := ctx.GasMeter().GasConsumed()
+ ctx.GasMeter().RefundGas(gasConsumed, "prepare proposal")
+ blockGasConsumed := ctx.BlockGasMeter().GasConsumed()
+ ctx.BlockGasMeter().RefundGas(blockGasConsumed, "prepare proposal")
+ ctx = ctx.WithKVGasConfig(storetypes.GasConfig{}).
+ WithTransientKVGasConfig(storetypes.GasConfig{}).
+ WithGasMeter(storetypes.NewInfiniteGasMeter())Commitable suggestion (Beta)
| return nil, err | |
| } | |
| ctx.GasMeter().RefundGas(ctx.GasMeter().GasConsumed(), "prepare proposal") | |
| ctx.BlockGasMeter().RefundGas(ctx.BlockGasMeter().GasConsumed(), "prepare proposal") | |
| ctx = ctx.WithKVGasConfig(storetypes.GasConfig{}). | |
| WithTransientKVGasConfig(storetypes.GasConfig{}). | |
| WithGasMeter(storetypes.NewInfiniteGasMeter()) | |
| // We have to prime the state plugin. | |
| if err = m.keeper.SetLatestQueryContext(ctx); err != nil { | |
| return nil, err | |
| } | |
| // Trigger the geth miner to build a block. | |
| if payloadEnvelopeBz, err = m.buildBlock(ctx); err != nil { | |
| return nil, err | |
| gasConsumed := ctx.GasMeter().GasConsumed() | |
| ctx.GasMeter().RefundGas(gasConsumed, "prepare proposal") | |
| blockGasConsumed := ctx.BlockGasMeter().GasConsumed() | |
| ctx.BlockGasMeter().RefundGas(blockGasConsumed, "prepare proposal") | |
| ctx = ctx.WithKVGasConfig(storetypes.GasConfig{}). | |
| WithTransientKVGasConfig(storetypes.GasConfig{}). | |
| WithGasMeter(storetypes.NewInfiniteGasMeter()) | |
| // We have to prime the state plugin. | |
| if err = m.keeper.SetLatestQueryContext(ctx); err != nil { | |
| return nil, err | |
| } | |
| // Trigger the geth miner to build a block. | |
| if payloadEnvelopeBz, err = m.buildBlock(ctx); err != nil { | |
| return nil, err | |
| } |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1270 +/- ##
==========================================
- Coverage 48.73% 48.70% -0.04%
==========================================
Files 84 84
Lines 4838 4841 +3
==========================================
Hits 2358 2358
- Misses 2306 2309 +3
Partials 174 174
|
Summary by CodeRabbit
Refactor:
miner.gofile in thecosmos/minermodule has been refactored for better gas management. This includes the addition of gas refunding and resetting, and reordering of code execution for efficiency.ProcessPayloadEnvelopefunction in theprocessor.gofile of thecosmos/x/evm/keepermodule has been updated to refund and reset gas meters after execution, ensuring accurate gas accounting.These changes aim to improve the efficiency and accuracy of gas management in the system, leading to more predictable and reliable operation.