Problem: estimate gas in evm tx is not accurate in relayer #1209
Conversation
Signed-off-by: mmsqe <mavis@crypto.com>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1209 +/- ##
===========================================
+ Coverage 16.31% 35.82% +19.50%
===========================================
Files 79 116 +37
Lines 5816 10638 +4822
===========================================
+ Hits 949 3811 +2862
- Misses 4789 6452 +1663
- Partials 78 375 +297
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for methodName := range irelayerABI.Methods { | ||
| var methodID [4]byte | ||
| copy(methodID[:], irelayerABI.Methods[methodName].ID[:4]) | ||
| switch methodName { | ||
| case CreateClient: | ||
| relayerGasRequiredByMethod[methodID] = 200000 | ||
| case RecvPacket, Acknowledgement: | ||
| relayerGasRequiredByMethod[methodID] = 250000 | ||
| case UpdateClient, UpgradeClient: | ||
| relayerGasRequiredByMethod[methodID] = 400000 | ||
| default: | ||
| relayerGasRequiredByMethod[methodID] = 100000 | ||
| } | ||
| } |
Check failure
Code scanning / gosec
the value in the range statement should be _ unless copying a map: want: for key := range m Error
| for methodName := range irelayerABI.Methods { | ||
| var methodID [4]byte | ||
| copy(methodID[:], irelayerABI.Methods[methodName].ID[:4]) | ||
| switch methodName { | ||
| case CreateClient: | ||
| relayerGasRequiredByMethod[methodID] = 200000 | ||
| case RecvPacket, Acknowledgement: | ||
| relayerGasRequiredByMethod[methodID] = 250000 | ||
| case UpdateClient, UpgradeClient: | ||
| relayerGasRequiredByMethod[methodID] = 400000 | ||
| default: | ||
| relayerGasRequiredByMethod[methodID] = 100000 | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
WalkthroughThe recent updates focus on refining the gas estimation and execution processes within the EVM and IBC relayer components. Pull requests have introduced more accurate gas estimation, improved the websocket system, and enhanced the parallelization of data restoration. A new Solidity interface and corresponding Go bindings have been added to streamline relayer operations. Additionally, the test suite has been updated to reflect these changes with more precise assertions and updated configurations. Changes
TipsChat with CodeRabbit Bot (
|
| } | ||
| return baseCost | ||
| // base cost to prevent large input size | ||
| baseCost := uint64(inputLen) * authtypes.DefaultTxSizeCostPerByte |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion Error
mmsqe
requested review from
calvinaco 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: 9
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- nix/sources.json
Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- integration_tests/configs/ibc_rly.jsonnet (1 hunks)
- integration_tests/ibc_utils.py (2 hunks)
- integration_tests/test_ibc_rly_gas.py (1 hunks)
- nix/default.nix (1 hunks)
- scripts/.env (1 hunks)
- scripts/gen-bindings-contracts (1 hunks)
- x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go (1 hunks)
- x/cronos/events/bindings/src/RelayerFunctions.sol (1 hunks)
- x/cronos/keeper/precompiles/relayer.go (3 hunks)
- x/cronos/keeper/precompiles/utils.go (1 hunks)
Files not summarized due to errors (1)
- x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- nix/default.nix
- scripts/.env
Additional comments: 11
CHANGELOG.md (2)
15-16: The changelog entries for pull requests Problem: require gas in relayer precompile is higher than consumed #1232 and Problem: estimate gas in evm tx is not accurate in relayer #1209 are correctly categorized under "State Machine Breaking" and align with the provided summaries.
20-21: The changelog entries for pull requests Problem: websocket refactoring not used #1239 and Problem: memiavl restore not fast enough #1241 are correctly categorized under "Improvements" and align with the provided summaries.
integration_tests/configs/ibc_rly.jsonnet (1)
- 18-23: The change in the
gas_multiplierfrom1.2to1.1is noted. Ensure that this change aligns with the intended improvements in gas estimation accuracy and that it has been tested to not adversely affect the relayer's operations.integration_tests/ibc_utils.py (2)
268-270: The removal of the balance assertion on line 269 should be verified to ensure that the test still correctly validates the IBC transfer's effects on the balance. If this assertion is critical for the test's correctness, its removal might introduce a risk of missing a regression.
665-670: The simplification of the
log_gas_recordsfunction to append gas usage records only ifgas_usedis present is a good practice to ensure that only valid gas usage data is recorded.integration_tests/test_ibc_rly_gas.py (1)
- 24-31: The change in the
diffvariable significantly tightens the tolerance for the assertion check. Verify that this new level of precision is supported by the underlying system and that it does not introduce flakiness in the tests due to rounding or minor fluctuations.The use of
int()for division in line 30 may lead to loss of precision. If the intention is to compare floating-point ratios, consider using float division instead. Verify that integer division aligns with the intended precision and functionality of the test.x/cronos/events/bindings/cosmos/precompile/relayer/i_relayer_functions.abigen.go (1)
- 1-831: The auto-generated Go bindings for the Ethereum contract appear to be correctly set up and match the Solidity contract's methods. No manual changes should be made to this file, as they will be lost when the code is re-generated.
x/cronos/events/bindings/src/RelayerFunctions.sol (3)
5-35: The functions in
IRelayerFunctionsall returnbytes calldata. Verify that this is intentional and that the consuming code can handle such generic return types. Typically, Solidity functions returnbytes memoryor more specific types.23-35: The functions
updateClientAnd...returnboolwhile all other functions returnbytes calldata. Ensure that this inconsistency is intentional and aligns with the design of the interface and its usage.5-35: All functions in
IRelayerFunctionsare marked asexternal payable. Confirm that it is necessary for all these functions to accept Ether, as making functions payable without need can introduce security risks.x/cronos/keeper/precompiles/utils.go (1)
- 107-108: In
execMultipleWithHooks, the second action is only executed iferrfrom the first action isnilandlen(e.input2) > 0. However, the result of the second action is not used or returned. If the second action is intended to produce a result, this should be handled appropriately.
Signed-off-by: mmsqe <mavis@crypto.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 16-16: The changelog entry for PR Problem: estimate gas in evm tx is not accurate in relayer #1209 is correctly categorized under "State Machine Breaking" changes, indicating that it introduces changes that could affect the consensus or require a state migration.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration_tests/test_ibc_rly_gas.py (1 hunks)
- x/cronos/keeper/precompiles/relayer.go (3 hunks)
- x/cronos/keeper/precompiles/utils.go (1 hunks)
Additional comments: 6
integration_tests/test_ibc_rly_gas.py (2)
24-24: The change in the
diffvalue from 0.5 to 0.004 significantly increases the precision of the gas consumption assertions. Ensure that this level of precision is supported by the underlying gas consumption measurements and that it does not lead to flaky tests due to minor fluctuations in gas usage.30-30: The explicit casting of
e2ande1tofloatbefore performing division is a good practice to avoid integer division, which can lead to incorrect results in Python 2 or when operands are integers. This change ensures that the division operation results in a floating-point number as expected.x/cronos/keeper/precompiles/relayer.go (3)
62-129: Initialization of
irelayerABIand the mappings in theinitfunction appears to be correct and follows the new dynamic approach using ABI and method names.184-185: The check for input length in the
Runfunction is a good practice to prevent slice bounds out of range panic.214-280: The use of a switch case to handle different method executions in the
Runfunction is appropriate and aligns with standard smart contract practices.x/cronos/keeper/precompiles/utils.go (1)
- 44-44: The error message "don't support multi-signers message" has been updated as previously suggested to include the number of signers found in the error message.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (2 hunks)
- x/cronos/keeper/precompiles/relayer.go (2 hunks)
Additional comments: 5
CHANGELOG.md (2)
15-18: The changes listed under "State Machine Breaking" align with the PR objectives, specifically addressing the accurate gas estimation in EVM transactions and updates to dependencies that could affect gas estimation logic.
22-23: The improvements listed, such as the refactor of the websocket/subscription system and the parallelization of memiavl restoration, are likely to contribute to the overall system performance enhancements mentioned in the PR objectives.
x/cronos/keeper/precompiles/relayer.go (3)
63-130: Initialization of
irelayerABIand the gas requirement maps appears to be correct and aligns with the PR objectives to refine gas estimation.159-181: The
RequiredGasmethod has been updated to calculate gas based on the method ID and input arguments, and includes a check to prevent integer overflow.183-285: The
Runmethod has been refactored to handle method execution based on the method ID and input arguments, and includes a length check to prevent slice bounds out of range panic.
| var methodID [4]byte | ||
| copy(methodID[:], input[:4]) | ||
| requiredGas, ok := relayerGasRequiredByMethod[methodID] | ||
| intrinsicGas, _ := core.IntrinsicGas(input, nil, false, bc.isHomestead, bc.isIstanbul, bc.isShanghai) |
Check warning
Code scanning / gosec
Returned error is not propagated up the stack. Warning
5971e6e
Summary by CodeRabbit
New Features
IRelayerFunctionsfor advanced relayer operations.Improvements
Bug Fixes
test_ibc_rly_gas.py.Refactor
Executorstruct for enhanced execution of actions.Documentation
Chores
vendorSha256for the "rly" package to reflect new vendor source.