Revert "Gaslimit + gascap flags" #1717
Conversation
This reverts commit 952a281.
WalkthroughThe overall change reflects a comprehensive update to the gas limit configuration and handling within a blockchain-related codebase. It involves increasing gas limits, consolidating multiple gas-related settings into a single Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? 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: 5
Configuration used: CodeRabbit UI
Files selected for processing (20)
- .github/workflows/build-pr.yml (1 hunks)
- contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1 hunks)
- go/config/enclave_cli_flags.go (2 hunks)
- go/config/enclave_config.go (2 hunks)
- go/config/enclave_config_test.go (3 hunks)
- go/enclave/components/batch_executor.go (6 hunks)
- go/enclave/components/interfaces.go (1 hunks)
- go/enclave/components/rollup_compression.go (1 hunks)
- go/enclave/crosschain/message_bus_manager.go (2 hunks)
- go/enclave/enclave.go (7 hunks)
- go/enclave/evm/evm_facade.go (7 hunks)
- go/enclave/l2chain/l2_chain.go (3 hunks)
- go/enclave/nodetype/sequencer.go (2 hunks)
- go/enclave/nodetype/validator.go (1 hunks)
- integration/common/constants.go (2 hunks)
- integration/datagenerator/common.go (1 hunks)
- integration/simulation/devnetwork/node.go (2 hunks)
- integration/simulation/network/network_utils.go (2 hunks)
- integration/simulation/simulation.go (1 hunks)
- tools/hardhatdeployer/contract_deployer.go (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/build-pr.yml
- integration/simulation/devnetwork/node.go
Additional comments: 25
integration/datagenerator/common.go (1)
- 52-52: The change from
gethcommon.Big1tobig.NewInt(1)for theGasPricefield in theCreateL2TxDatafunction should not affect the behavior, as both expressions evaluate to the same value. However, ensure that this change is consistent with the usage ofbig.Intthroughout the rest of the codebase for maintainability.integration/common/constants.go (1)
- 87-87: The
GasLimitfield in theDefaultEnclaveConfigfunction is now set toparams.MaxGasLimit / 6. Verify that this new gas limit aligns with the intended transaction throughput and system capabilities.go/config/enclave_config_test.go (1)
- 44-44: The renaming of
GasBatchExecutionLimittoL2GasLimitFlagshould be reflected across all test cases and usage within the codebase. Confirm that the new name is consistently applied and that it accurately describes the flag's functionality.go/enclave/nodetype/validator.go (1)
- 136-136: The addition of the
gasLimitparameter to theCreateGenesisStatemethod in thehandleGenesisfunction should be carefully reviewed to ensure that it is being used correctly within the method and that it aligns with the intended genesis state creation logic.go/enclave/components/interfaces.go (1)
- 79-79: The addition of an extra
*big.Intparameter to theCreateGenesisStatemethod in theBatchExecutorinterface should be reviewed to ensure that it is correctly implemented and that the method's consumers are updated to pass this new parameter.go/config/enclave_cli_flags.go (1)
- 57-57: The consolidation of
GasBatchExecutionLimitandGasLocalExecutionCapFlagintoL2GasLimitFlagshould be thoroughly checked to ensure that all references to the old flags are updated and that the new flag is being used correctly throughout the codebase.contracts/deployment_scripts/testnet/layer2/001_whitelist_tokens.ts (1)
- 125-125: The increase of the
gasLimitparameter to1024_000_000within therelayMessagefunction call is substantial. Ensure that this new limit is justified by the complexity of the operations being performed and that it does not introduce any inefficiencies or security concerns.integration/simulation/network/network_utils.go (1)
- 97-97: The
GasLimithas been set to a very high value of1_000_000_000_000_000_000. Confirm that this value is appropriate for the system's requirements and does not pose any risk of inefficient gas usage or other unintended consequences.tools/hardhatdeployer/contract_deployer.go (1)
- 94-95: The
GasPriceandGasvalues in thedeployContractTxstruct have been significantly increased to2000000000and1025_000_000, respectively. Verify that these values are justified by the network conditions and the contract's complexity, and ensure they do not lead to excessive costs or other issues.go/config/enclave_config.go (1)
- 187-187: The initialization of
GasLimitusingL2GasLimitFlagappears correct. Confirm thatL2GasLimitFlagis correctly defined and used consistently across the codebase.go/enclave/evm/evm_facade.go (3)
6-6: The
mathpackage has been imported. Verify that it is used within the file, otherwise, it should be removed to keep the imports clean.43-43: The
ExecuteTransactionsfunction now initializes a gas pool (gp) withininitParams. Ensure thatgpis correctly used within the function and that its introduction does not affect the transaction execution logic.205-210: The
initParamsfunction now returns a*gethcore.GasPool. Confirm that all calls toinitParamscorrectly handle the new return value and that the gas pool is utilized appropriately.go/enclave/l2chain/l2_chain.go (2)
37-37: The removal of the
gasEstimationCapfield from theobscuroChainstruct simplifies the configuration. Confirm that this field is no longer required and that its removal does not affect any functionality that previously depended on it.37-37: The
Registryfield in theobscuroChainstruct has been moved to a separate line. This change is cosmetic and should not affect the functionality, but verify that the repositioning aligns with the codebase's struct layout conventions.go/enclave/crosschain/message_bus_manager.go (2)
89-89: The gas limit for the
GenerateMessageBusDeployTxfunction has been increased to 5,000,000. Ensure that this increase is justified by the complexity of the contract being deployed and does not lead to unnecessarily high transaction costs.222-222: The gas limit for the
CreateSyntheticTransactionsfunction has been increased to 5,000,000. Verify that this increase is necessary for the transactions being created and that it does not result in excessive gas costs.go/enclave/nodetype/sequencer.go (1)
- 141-141: The addition of
s.settings.BatchGasLimitas a parameter to theCreateGenesisStatemethod should be checked to ensure that the method correctly handles the new*big.Inttype for gas limit.go/enclave/components/batch_executor.go (2)
42-45: The removal of the
batchGasLimitfield from thebatchExecutorstruct should be carefully checked to ensure that it does not affect other parts of the code where it might have been used.311-314: The default value assignment for
gasLimitin theCreateGenesisStatemethod should be reviewed to ensure it aligns with the expected gas limit for genesis state creation.go/enclave/components/rollup_compression.go (1)
- 439-439: The initialization of a new
big.Intwith the value ofcalldataRollupHeader.GasLimitfor theCreateGenesisStatemethod call should be verified to ensure that thegasLimitis being correctly used and handled.go/enclave/enclave.go (4)
84-86: The addition of
GlobalGasCapandBaseFeefields to theenclaveImplstruct is consistent with the summary provided. Ensure that these new fields are properly initialized and used throughout the codebase.187-187: The
NewEnclavefunction now initializesbatchExecutorwithout theconfig.GasBatchExecutionLimitparameter, which aligns with the summary. Verify that this change is reflected everywhereNewBatchExecutoris called.225-225: The
BatchGasLimitwithinSequencerSettingsis now set toconfig.GasLimit. Confirm that this new assignment is correctly handled in all relevant parts of the codebase whereBatchGasLimitis used.1031-1031: The
DoEstimateGasmethod now usese.GlobalGasCapwhich is consistent with the removal ofGasLocalExecutionCapFlag. Ensure that theGlobalGasCapis properly used in all gas estimation logic.
| GlobalGasCap: 5_000_000_000, // todo (#627) - make config | ||
| BaseFee: gethcommon.Big0, |
There was a problem hiding this comment.
The GlobalGasCap and BaseFee fields are initialized with hardcoded values. Consider making these configurable through the config object or environment variables to avoid recompilation for changes.
| hi = e.GlobalGasCap | ||
| } | ||
| // Normalize the max fee per gas the call is willing to spend. | ||
| var feeCap *big.Int |
There was a problem hiding this comment.
The DoEstimateGas method has a high cognitive complexity. Consider refactoring to simplify the logic and improve readability. Additionally, address the TODO comment regarding the review of gas mechanics/tokenomics.
| @@ -35,7 +35,7 @@ type SequencerSettings struct { | |||
| MaxBatchSize uint64 | |||
| MaxRollupSize uint64 | |||
| GasPaymentAddress gethcommon.Address | |||
| BatchGasLimit uint64 | |||
| BatchGasLimit *big.Int | |||
There was a problem hiding this comment.
The BatchGasLimit field type change from uint64 to *big.Int requires careful handling of the new pointer type to avoid nil pointer dereferences. Ensure that all usages of BatchGasLimit check for nil before dereferencing.
| GasPaymentAddress gethcommon.Address | ||
| BaseFee *big.Int | ||
| GasLimit *big.Int |
There was a problem hiding this comment.
The consolidation of GasBatchExecutionLimit and GasLocalExecutionCapFlag into a single GasLimit field simplifies the configuration. Ensure that all usages of the old fields are updated to use GasLimit.
| @@ -227,7 +227,7 @@ func (s *Simulation) deployObscuroERC20s() { | |||
|
|
|||
| deployContractTx := types.DynamicFeeTx{ | |||
| Nonce: NextNonce(s.ctx, s.RPCHandles, owner), | |||
| Gas: 2_000_000, | |||
| Gas: 1025_000_000, | |||
There was a problem hiding this comment.
The Gas value for deploying Obscuro ERC20 contracts has been significantly increased to 1025_000_000. This seems excessively high and could lead to very high transaction costs. Verify that this value is correct and necessary for the deployment of these contracts.
|
closed in preference of #1721 |
Reverts #1682