Problem: no required gas log in precompiles #1223
Conversation
WalkthroughThe changes primarily focus on enhancing the logging functionality and gas calculation in the precompiled contracts of the Cronos application. The Changes
TipsChat with CodeRabbit Bot (
|
Signed-off-by: mmsqe <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
|
|
||
| func assignMethodGas(prefix int, gas uint64) { | ||
| data := make([]byte, 4) | ||
| binary.LittleEndian.PutUint32(data, uint32(prefix)) |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion Error
mmsqe
requested review from
yihuang and
devashishdxt
and removed request for
a team
![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: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- x/cronos/keeper/precompiles/bank.go (4 hunks)
- x/cronos/keeper/precompiles/base_contract.go (2 hunks)
- x/cronos/keeper/precompiles/ica.go (4 hunks)
- x/cronos/keeper/precompiles/relayer.go (1 hunks)
Additional comments: 19
CHANGELOG.md (1)
- 10-13: The new hunk in the
CHANGELOG.mdfile provides an update on the improvements made in the project. It mentions the addition of a required gas log in precompiles, which is linked to PR #1223. This change is consistent with the summary provided for the pull request. No issues or concerns are identified in this hunk.app/app.go (1)
- 556-557: The
NewRelayerContractandNewIcaContractfunctions now include an additionalapp.Logger()parameter. Ensure that the logger is properly initialized and that it doesn't introduce any potential data races when used concurrently. Also, verify that the logger is used appropriately within these functions to log relevant information.x/cronos/keeper/precompiles/base_contract.go (5)
4-5: The new imports
github.com/cometbft/cometbft/libs/logandgithub.com/cosmos/cosmos-sdk/store/typesare introduced to support the logging functionality and to use theGasConfigtype respectively. Ensure that these packages are included in your dependencies.15-15: The
RequiredGasmethod is added to theBaseContractinterface. This method is expected to calculate the gas required for a contract based on the input. Make sure all implementations ofBaseContractimplement this method.18-24: The
baseContractstruct is enhanced with additional fields forkvGasConfig,nameByMethod,gasByMethod,emptyGasIfInputLessThanPrefix, andlogger. These fields are used to configure the gas usage and logging for the contract. Ensure that these fields are properly initialized and used in the contract's methods.27-43: The
NewBaseContractfunction now takes additional parameters to initialize the new fields in thebaseContractstruct. Ensure that all calls toNewBaseContractare updated to match the new function signature.49-67: The
RequiredGasmethod is implemented for thebaseContractstruct. This method calculates the gas required for a contract based on the input. It also logs the method name, input length, and required gas. Ensure that this method is correctly calculating the gas and logging the information.x/cronos/keeper/precompiles/relayer.go (6)
6-6: The
fmtpackage is imported but not used in the new hunks. Please verify if it's used in the rest of the file. If not, consider removing it to avoid unnecessary imports.25-32: The
assignMethodGasfunction is a new addition that helps to assign gas values to methods in a more structured way. It also maps method prefixes to their string representations. This is a good practice as it improves code readability and maintainability.34-51: The
initfunction has been updated to use the newassignMethodGasfunction. This is a good practice as it improves code readability and maintainability.60-78: The
NewRelayerContractfunction has been updated to include aloggerparameter and to initialize theBaseContractwith additional parameters. This is a good practice as it improves the functionality and configurability of the contract.80-82: The
Addressfunction is unchanged and continues to return therelayerContractAddress. This is consistent with the previous implementation.83-86: The
RequiredGasfunction has been removed from theRelayerContractstruct. This is consistent with the changes inbase_contract.gowhere theRequiredGasmethod has been added to theBaseContractstruct. This is a good practice as it reduces code duplication and improves maintainability.x/cronos/keeper/precompiles/bank.go (3)
7-7: The new hunk imports the
logpackage fromgithub.com/cometbft/cometbft/libs/log. This is likely to be used for logging purposes in the code.35-35: A new global variable
bankMethodMapof typemap[[4]byte]string{}is introduced. This map will likely be used to map method IDs to their corresponding method names.55-55: In the
init()function, thebankMethodMapis populated with method IDs as keys and method names as values. This will allow for easy retrieval of method names using their IDs.x/cronos/keeper/precompiles/ica.go (3)
9-9: The new hunk imports the
logpackage fromgithub.com/cometbft/cometbft/libs/log. This package is used to provide logging functionality to theIcaContract.38-38: The new hunk introduces a global variable
icaMethodMapof typemap[[4]byte]string{}. This map is used to store the mapping between method IDs and method names.62-62: The new hunk adds the method name to the
icaMethodMapin theinitfunction. This ensures that each method ID is associated with its corresponding method name.
x/cronos/keeper/precompiles/bank.go
Outdated
| BaseContract | ||
|
|
||
| bankKeeper types.BankKeeper | ||
| cdc codec.Codec | ||
| } | ||
|
|
||
| // NewBankContract creates the precompiled contract to manage native tokens | ||
| func NewBankContract(bankKeeper types.BankKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract { | ||
| return &BankContract{bankKeeper, cdc, kvGasConfig} | ||
| func NewBankContract(bankKeeper types.BankKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig, logger log.Logger) vm.PrecompiledContract { | ||
| return &BankContract{ | ||
| BaseContract: NewBaseContract( | ||
| bankContractAddress, | ||
| kvGasConfig, | ||
| bankMethodMap, | ||
| bankGasRequiredByMethod, | ||
| false, | ||
| logger.With("precompiles", "bank"), | ||
| ), | ||
| cdc: cdc, | ||
| bankKeeper: bankKeeper, | ||
| } |
There was a problem hiding this comment.
The BankContract struct is updated to embed the BaseContract struct. The kvGasConfig field is removed from BankContract as it is now part of BaseContract. The NewBankContract function is updated to initialize the BaseContract with additional parameters including the contract address, gas configuration, method map, gas required by method, a boolean indicating if the contract is stateless, and a logger with a specific context. This change enhances the modularity and maintainability of the code by moving common functionality to the BaseContract.
| func (bc *BankContract) Address() common.Address { | ||
| return bankContractAddress | ||
| } | ||
|
|
There was a problem hiding this comment.
The RequiredGas method is removed from the BankContract struct. This is likely because the method is now part of the BaseContract and can be accessed through it. This change reduces code duplication and enhances maintainability.
x/cronos/keeper/precompiles/ica.go
Outdated
| func NewIcaContract( | ||
| icaauthKeeper types.Icaauthkeeper, | ||
| cronosKeeper types.CronosKeeper, | ||
| cdc codec.Codec, | ||
| kvGasConfig storetypes.GasConfig, | ||
| logger log.Logger, | ||
| ) vm.PrecompiledContract { | ||
| return &IcaContract{ | ||
| BaseContract: NewBaseContract(icaContractAddress), | ||
| BaseContract: NewBaseContract( | ||
| icaContractAddress, | ||
| kvGasConfig, | ||
| icaMethodMap, | ||
| icaGasRequiredByMethod, | ||
| false, | ||
| logger.With("precompiles", "ica"), | ||
| ), | ||
| cdc: cdc, | ||
| icaauthKeeper: icaauthKeeper, | ||
| cronosKeeper: cronosKeeper, | ||
| kvGasConfig: kvGasConfig, | ||
| } | ||
| } |
There was a problem hiding this comment.
The NewIcaContract function signature has been updated to include two new parameters: kvGasConfig of type storetypes.GasConfig and logger of type log.Logger. The kvGasConfig parameter is used to configure the gas usage for key-value store operations, while the logger parameter is used for logging. The kvGasConfig field has been removed from the IcaContract struct, and the logger field has been added to the BaseContract struct. The RequiredGas function has been removed from the IcaContract and moved to the BaseContract. These changes are part of the effort to enhance the functionality and configurability of the contracts.
- func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract {
+ func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig, logger log.Logger) vm.PrecompiledContract {
return &IcaContract{
- BaseContract: NewBaseContract(icaContractAddress),
+ BaseContract: NewBaseContract(
+ icaContractAddress,
+ kvGasConfig,
+ icaMethodMap,
+ icaGasRequiredByMethod,
+ false,
+ logger.With("precompiles", "ica"),
+ ),
cdc: cdc,
icaauthKeeper: icaauthKeeper,
cronosKeeper: cronosKeeper,
- kvGasConfig: kvGasConfig,
}
}Committable suggestion (Beta)
| func NewIcaContract( | |
| icaauthKeeper types.Icaauthkeeper, | |
| cronosKeeper types.CronosKeeper, | |
| cdc codec.Codec, | |
| kvGasConfig storetypes.GasConfig, | |
| logger log.Logger, | |
| ) vm.PrecompiledContract { | |
| return &IcaContract{ | |
| BaseContract: NewBaseContract(icaContractAddress), | |
| BaseContract: NewBaseContract( | |
| icaContractAddress, | |
| kvGasConfig, | |
| icaMethodMap, | |
| icaGasRequiredByMethod, | |
| false, | |
| logger.With("precompiles", "ica"), | |
| ), | |
| cdc: cdc, | |
| icaauthKeeper: icaauthKeeper, | |
| cronosKeeper: cronosKeeper, | |
| kvGasConfig: kvGasConfig, | |
| } | |
| } | |
| func NewIcaContract( | |
| icaauthKeeper types.Icaauthkeeper, | |
| cronosKeeper types.CronosKeeper, | |
| cdc codec.Codec, | |
| kvGasConfig storetypes.GasConfig, | |
| logger log.Logger, | |
| ) vm.PrecompiledContract { | |
| return &IcaContract{ | |
| BaseContract: NewBaseContract( | |
| icaContractAddress, | |
| kvGasConfig, | |
| icaMethodMap, | |
| icaGasRequiredByMethod, | |
| false, | |
| logger.With("precompiles", "ica"), | |
| ), | |
| cdc: cdc, | |
| icaauthKeeper: icaauthKeeper, | |
| cronosKeeper: cronosKeeper, | |
| } | |
| } |
thomas-nguy
changed the title
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1223 +/- ##
===========================================
+ Coverage 36.89% 48.13% +11.23%
===========================================
Files 115 74 -41
Lines 10255 6197 -4058
===========================================
- Hits 3784 2983 -801
+ Misses 6097 2896 -3201
+ Partials 374 318 -56 |
| inputLen := len(input) | ||
| defer func() { | ||
| method := c.nameByMethod[methodID] | ||
| c.logger.Info("required", "gas", gas, "method", method, "len", inputLen) |
There was a problem hiding this comment.
what's this for, temporary debugging?
There was a problem hiding this comment.
I thought we want to record required gas for each method
There was a problem hiding this comment.
for debugging or what reason?
There was a problem hiding this comment.
yes debugging, we could collect the data on some nodes to tune the requiredgas map later on
but the data has to be the gas consumed at cosmos level during execution, not the value returned from the map
There was a problem hiding this comment.
I see, so this could be a draft PR for building a local binary, no need to merge, right?
There was a problem hiding this comment.
I wonder if we could use those data on mainnet
https://cronos.yummy.capital/txs/E2B835B672BC046028E3FEBEB1770CD8A506DB505380982AD16C39F13E5DD861
Somehow hermes in mainnet is spending more gas than that. But may be also because of hermez config?
There was a problem hiding this comment.
we should have bigger client header like more validators, and I'm not sure why we use TransientGasConfig
There was a problem hiding this comment.
For the TransientGasConfig, it inspired from evmos implementation of the gasRequired
https://github.com/evmos/evmos/blob/main/precompiles/common/precompile.go#L34
They also use the TransientGasConfig, I think that make sense since the goal for us is just to add an "extra" fee based on the length of the input, to avoid DDOS attack for very large input
But in practice, the additional fee does not make much sense since input length is not related to the cost of operation in memory
There was a problem hiding this comment.
I think evmos use KvGasConfig instead and input length does related to TxSizeCost which is the major cost in cosmos tx
There was a problem hiding this comment.
mmm in the original context, the base cost is just an extra cost to avoid DDOS attack, since the input it never written in store, it would make sense to use TransientGasConfig for the computation.
But I think what you are trying to do is to replicate the cosmos intrinsic cost? In that case KvGasConfig is used
| return | ||
| } | ||
| // base cost to prevent large input size | ||
| gas = uint64(inputLen) * c.writeCostPerByte |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion Error
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- x/cronos/keeper/precompiles/bank.go (4 hunks)
- x/cronos/keeper/precompiles/base_contract.go (2 hunks)
- x/cronos/keeper/precompiles/ica.go (4 hunks)
- x/cronos/keeper/precompiles/relayer.go (4 hunks)
Additional comments: 18
CHANGELOG.md (1)
- 18-21: The change log update is clear and concise, providing a link to the relevant pull request for more context. Ensure that the version number and date are updated appropriately when the changes are released.
app/app.go (1)
- 539-540: Ensure that all calls to
NewRelayerContractandNewIcaContractthroughout the codebase have been updated to match the new signature.- cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, app.Logger()), - cronosprecompiles.NewIcaContract(&app.ICAAuthKeeper, &app.CronosKeeper, appCodec, gasConfig, app.Logger()), + cronosprecompiles.NewRelayerContract(app.IBCKeeper, appCodec, app.Logger()), + cronosprecompiles.NewIcaContract(&app.ICAAuthKeeper, &app.CronosKeeper, appCodec, gasConfig, app.Logger()),x/cronos/keeper/precompiles/base_contract.go (4)
11-15: The
BaseContractinterface has been updated to include theRequiredGasmethod. Ensure that all implementations of this interface have been updated accordingly.17-24: The
baseContractstruct has been updated with new fields. Ensure that these fields are being used appropriately throughout the codebase.26-42: The
NewBaseContractfunction has been updated to accept new parameters. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.48-66: The
RequiredGasmethod has been added to calculate the contract gas use. Ensure that this method is being called and used correctly throughout the codebase.x/cronos/keeper/precompiles/relayer.go (4)
13-19: The import section is well organized and follows the best practices of separating standard library imports from third-party imports.
36-55: The
initfunction is correctly used to initialize therelayerMethodMapandrelayerGasRequiredByMethodmaps. It calls theassignMethodGasfunction with the correct parameters.65-84: The
NewRelayerContractfunction has been updated to accept an additionalibcKeeper *ibckeeper.Keeperparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.90-105: The
RequiredGasmethod now calls theBaseContract.RequiredGasmethod and logs additional information. This is a good practice as it enhances the debuggability of the code.x/cronos/keeper/precompiles/bank.go (4)
4-10: The new import
github.com/cometbft/cometbft/libs/logis added to support logging functionality. Ensure that the package is available and accessible.32-36: The
bankMethodMapis introduced and initialized with method IDs and names. This map will be used to map method IDs to their corresponding names.52-56: The
bankMethodMapis populated with method IDs and their corresponding names in theinitfunction. This is a good practice as it ensures that the map is populated before it is used anywhere in the code.61-92: The
NewBankContractfunction signature is modified to include aloggerparameter. The function is also updated to initializeBaseContractand setcdcandbankKeeperfields. TheRequiredGasfunction is removed, and theIsStatefulfunction is modified to returntrue. Ensure that all calls toNewBankContractthroughout the codebase have been updated to match the new signature.- func NewBankContract(bankKeeper types.BankKeeper, cdc codec.Codec) vm.PrecompiledContract { + func NewBankContract(bankKeeper types.BankKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig, logger log.Logger) vm.PrecompiledContract {x/cronos/keeper/precompiles/ica.go (4)
6-12: The new import statement for the "github.com/cometbft/cometbft/libs/log" package is added correctly.
35-39: The new global variable "icaMethodMap" is added correctly. Ensure that it is used appropriately throughout the codebase.
78-98: The "NewIcaContract" function signature is updated to include a "logger" parameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig) vm.PrecompiledContract { + func NewIcaContract(icaauthKeeper types.Icaauthkeeper, cronosKeeper types.CronosKeeper, cdc codec.Codec, kvGasConfig storetypes.GasConfig, logger log.Logger) vm.PrecompiledContract {
- 86-93: The "BaseContract" initialization within the "NewIcaContract" function is modified correctly to include the new "logger" parameter.
- BaseContract: NewBaseContract(icaContractAddress, kvGasConfig.WriteCostPerByte, icaMethodMap, icaGasRequiredByMethod, false), + BaseContract: NewBaseContract(icaContractAddress, kvGasConfig.WriteCostPerByte, icaMethodMap, icaGasRequiredByMethod, false, logger.With("precompiles", "ica")),
| return | ||
| } | ||
| // base cost to prevent large input size | ||
| gas = uint64(inputLen) * c.writeCostPerByte |
There was a problem hiding this comment.
Potential integer overflow by integer type conversion. Consider adding a check to prevent overflow.
+ if inputLen > math.MaxUint64/c.writeCostPerByte {
+ // handle the error, possibly return an error
+ }
gas = uint64(inputLen) * c.writeCostPerByteCommitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| gas = uint64(inputLen) * c.writeCostPerByte | |
| if inputLen > math.MaxUint64/c.writeCostPerByte: | |
| # handle the error, possibly return an error | |
| gas = uint64(inputLen) * c.writeCostPerByte |
| func assignMethodGas(prefix int, gas uint64) { | ||
| data := make([]byte, 4) | ||
| binary.LittleEndian.PutUint32(data, uint32(prefix)) | ||
| var id [4]byte | ||
| copy(id[:], data[:4]) | ||
| relayerMethodMap[id] = fmt.Sprintf("%d", prefix) | ||
| relayerGasRequiredByMethod[id] = gas | ||
| } |
There was a problem hiding this comment.
The assignMethodGas function is well implemented. It populates the relayerMethodMap and relayerGasRequiredByMethod maps with the correct values. However, there is a potential integer overflow issue on line 29 as pointed out by the bot in the previous comments. You should handle this potential overflow.
binary.LittleEndian.PutUint32(data, uint32(prefix))
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Refactor
baseContractstructure and its associated methods for better configurability and functionality, including a new method to calculate contract gas usage.RequiredGasfunction from theBankContract,IcaContract, andRelayerContract, simplifying the gas calculation process.New Feature