feat: clamp to consensus max gas #51
Conversation
aljo242
requested review from
davidterpay,
nivasan1 and
Eric-Warehime
as code owners
WalkthroughThe changes involve a significant reconfiguration of the application's module structure and functionality to align with EIP-1559. This includes removing the crisis module, introducing a Changes
Assessment against linked issues
Related issues
Poem
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 your favorite social media? 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: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/feemarket/types/params.pb.go
Files selected for processing (21)
- proto/feemarket/feemarket/v1/params.proto (1 hunks)
- tests/app/app.go (6 hunks)
- tests/app/config.go (6 hunks)
- tests/app/export.go (1 hunks)
- tests/app/feemarketd/cmd/root.go (3 hunks)
- tests/app/post.go (2 hunks)
- testutils/keeper/keeper.go (6 hunks)
- x/feemarket/ante/suite/suite.go (5 hunks)
- x/feemarket/fuzz/aimd_eip1559_test.go (2 hunks)
- x/feemarket/fuzz/eip1559_test.go (2 hunks)
- x/feemarket/keeper/feemarket_test.go (8 hunks)
- x/feemarket/keeper/msg_server_test.go (1 hunks)
- x/feemarket/post/expected_keeper.go (2 hunks)
- x/feemarket/post/fee.go (2 hunks)
- x/feemarket/post/fee_test.go (4 hunks)
- x/feemarket/post/feegrant_test.go (1 hunks)
- x/feemarket/post/mocks/mock_consensus_keeper.go (1 hunks)
- x/feemarket/post/mocks/mock_feemarket_keeper.go (2 hunks)
- x/feemarket/types/state.go (1 hunks)
- x/feemarket/types/state_fuzz_test.go (2 hunks)
- x/feemarket/types/state_test.go (6 hunks)
Files skipped from review due to trivial changes (1)
- tests/app/export.go
Additional comments: 46
x/feemarket/post/mocks/mock_consensus_keeper.go (1)
- 13-57: The mock implementation for
ConsensusKeeperand its constructorNewConsensusKeeperare correctly set up to simulate interface behavior and assert expectations post-test.tests/app/post.go (1)
- 31-47: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [11-44]
The addition of the
ConsensusParamsKeeperfield to thePostHandlerOptionsstruct and the corresponding validation and usage in theNewPostHandlerfunction are correctly implemented.x/feemarket/keeper/msg_server_test.go (1)
- 50-51: The
state.Updatefunction is called withparams.MaxBlockUtilizationfor both parameters. Verify if this is intended, as it may be a logic error if the function expects different values for current and maximum block utilization.x/feemarket/types/state_fuzz_test.go (2)
36-36: The
state.Updatefunction is called withparams.MaxBlockUtilizationfor both parameters in theFuzzDefaultFeeMarketfunction. Verify if this is intended, as it may be a logic error if the function expects different values for current and maximum block utilization.85-85: The
state.Updatefunction is called withparams.MaxBlockUtilizationfor both parameters in theFuzzAIMDFeeMarketfunction. Verify if this is intended, as it may be a logic error if the function expects different values for current and maximum block utilization.x/feemarket/post/expected_keeper.go (1)
- 47-56: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [4-56]
The new import
tmprotoand theConsensusKeeperinterface with itsGetmethod are correctly defined and follow Go's interface naming and method signature conventions.proto/feemarket/feemarket/v1/params.proto (1)
- 69-73: The updated comments for
TargetBlockUtilizationandMaxBlockUtilizationin theparams.protofile provide clarity by specifying that these values are measured in units of gas, aligning with EIP-1559 specifications.x/feemarket/fuzz/eip1559_test.go (2)
27-27: The
state.Updatefunction is called withparams.MaxBlockUtilizationfor both parameters in theTestLearningRatefunction. Verify if this is intended, as it may be a logic error if the function expects different values for current and maximum block utilization.53-53: The
state.Updatefunction is called withparams.MaxBlockUtilizationfor both parameters in theTestBaseFeefunction. Verify if this is intended, as it may be a logic error if the function expects different values for current and maximum block utilization.x/feemarket/post/mocks/mock_feemarket_keeper.go (2)
44-66: The mock functions
GetParamsandSetParamsfor theFeeMarketKeeperare correctly set up to simulate the behavior of getting and setting fee market parameters.92-103: The mock function
SetParamsfor theFeeMarketKeeperis correctly set up to simulate the behavior of setting fee market parameters.testutils/keeper/keeper.go (4)
7-8: The addition of
consensustypesimport is necessary for the newConsensusKeeperfield in theTestKeepersstruct.15-15: The addition of
consensuskeeperimport is necessary for the newConsensusKeeperfield in theTestKeepersstruct.28-28: The new
ConsensusKeeperfield has been added to theTestKeepersstruct. Ensure that all references toTestKeepersthroughout the codebase are updated to handle this new field.60-60: The
Consensusfunction is introduced to initialize the consensus parameters module. Ensure that this function is used wherever consensus parameters need to be initialized in tests.x/feemarket/fuzz/aimd_eip1559_test.go (2)
- 38-38: The
state.Updatefunction call has been modified to passparams.MaxBlockUtilizationinstead ofparams. Ensure that theUpdatefunction's signature has been updated accordingly and that this change is consistent across all test cases.Verification successful
The verification of the
state.Updatefunction call inaimd_eip1559_test.gohas been successful:
- The
state.Updatefunction signature has been confirmed to accept twouint64arguments, which is consistent with the change to passparams.MaxBlockUtilization.- The usage of
state.Updateacross various test files has been checked, and it consistently usesparams.MaxBlockUtilizationas the second argument.* 83-83: The `state.Update` function call has been modified to pass `params.MaxBlockUtilization` instead of `params`. Ensure that the `Update` function's signature has been updated accordingly and that this change is consistent across all test cases.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the state.Update function signature to confirm the change. rg --type go "func \(.*State\) Update" # Search for all occurrences of state.Update to ensure consistency. rg --type go "state.Update"Length of output: 7169
Verification successful
The verification process has confirmed that:
- The
state.Updatefunction's signature has been updated to accept twouint64arguments.- The change to pass
params.MaxBlockUtilizationinstead ofparamsis consistent across all test cases wherestate.Updateis called.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the state.Update function signature to confirm the change. rg --type go "func \(.*State\) Update" # Search for all occurrences of state.Update to ensure consistency. rg --type go "state.Update"Length of output: 7169
x/feemarket/types/state.go (1)
- 28-30: The
Updatemethod now takes an additionalmaxGasparameter. Ensure that all calls to this method are updated to pass the correctmaxGasvalue and that the method's logic correctly handles the new parameter.Verification successful
The verification of the
state.Updatemethod calls across the codebase shows that the second parameter being passed is consistent with the intendedmaxGasvalue as per the PR objectives and the AI-generated summary. The calls in both test and non-test files are usingparams.MaxBlockUtilizationor other appropriateuint64values as themaxGasparameter.
- The
Updatemethod's calls have been correctly updated to pass themaxGasvalue, and the method's logic is consistent with the new parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to the State.Update method to ensure they pass the correct maxGas value. rg --type go "state.Update"Length of output: 6862
x/feemarket/post/fee.go (3)
- 23-27: The
FeeMarketDeductDecoratorstruct has been updated to include aconsensusParamsKeeperfield. Ensure that all instances of this struct are updated to provide the new field.Verification successful
The shell script output shows that the
FeeMarketDeductDecoratorstruct is being correctly instantiated with the newconsensusParamsKeeperfield in the filesfeegrant_test.go,suite.go, andpost.go. The constructor functionNewFeeMarketDeductDecoratoris also updated to accept theConsensusKeeperas a parameter, which is consistent with the changes described.Based on the provided output, it appears that the integration of the
consensusParamsKeeperfield into theFeeMarketDeductDecoratorstruct and its related functions has been handled correctly in the instances found.
- The
FeeMarketDeductDecoratorstruct has been correctly updated to include theconsensusParamsKeeperfield, and all instances of this struct are updated to provide the new field.* 30-36: The `NewFeeMarketDeductDecorator` function has been updated to accept a `ConsensusKeeper` as a parameter. Ensure that all calls to this function are updated to pass the new parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all instances of FeeMarketDeductDecorator to ensure they provide the new consensusParamsKeeper field. rg --type go "FeeMarketDeductDecorator"Length of output: 1095
Verification successful
The verification process has confirmed that all calls to
NewFeeMarketDeductDecoratoracross the codebase have been updated to include the newConsensusKeeperparameter as required.
* 104-119: The `PostHandle` method now retrieves `consensusParams` and uses it to update `maxGas` before updating the state. Ensure that the retrieval and usage of `consensusParams` are correct and that error handling is adequate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to NewFeeMarketDeductDecorator to ensure they pass the new ConsensusKeeper parameter. rg --type go "NewFeeMarketDeductDecorator"Length of output: 541
Script:
#!/bin/bash # Check the parameters passed to NewFeeMarketDeductDecorator in the specified files cat x/feemarket/ante/suite/suite.go cat tests/app/post.goLength of output: 9360
x/feemarket/post/feegrant_test.go (1)
- 132-132: The addition of
s.ConsensusKeeperas an argument toFeeMarketDeductDecoratoraligns with the PR's objective to integrate max block gas limits from EIP-1559 into the consensus parameters. Ensure that theConsensusKeeperis properly initialized and used within theFeeMarketDeductDecorator.x/feemarket/ante/suite/suite.go (3)
38-44: The addition of
ConsensusKeeperandMockConsensusParamsKeeperto theTestSuitestruct is necessary for the integration of consensus parameters into the test suite. Ensure that these keepers are properly initialized and used in the test setup.81-84: The initialization of
ConsensusKeeperandMockConsensusParamsKeeperin theSetupTestSuitefunction is consistent with the PR's objectives. Verify that the initialization logic is correct and that the keepers are being used as intended in the tests.97-102: The conditional assignment of
consensusKeeperin theSetupHandlersfunction based on themockparameter is a good practice for test modularity. Confirm that the mock behavior is consistent with the expected test scenarios.tests/app/feemarketd/cmd/root.go (2)
33-33: The introduction of
flagCrisisDummyas a placeholder for the crisis module flag is a security measure to avoid importing the crisis module. Confirm that this change does not affect the functionality where the crisis module's flags are used.181-182: The modification of the
addModuleInitFlagsfunction to include theflagCrisisDummyis consistent with the removal of the crisis module. Ensure that this dummy flag is handled appropriately in the application's flag parsing logic.tests/app/config.go (2)
9-14: The removal of crisis-related modules and the addition of consensus-related modules in the imports section should be verified to ensure that it aligns with the intended changes and does not introduce any unintended side effects.
57-62: The change in the
genesisModuleOrderarray reflects the removal of the crisis module and the addition of the consensus module. Ensure that this new order maintains the correct initialization sequence and does not disrupt any dependencies between modules.x/feemarket/post/fee_test.go (3)
149-166: The new test case "error getting consensus params - should fail" has been added to simulate an error when retrieving consensus parameters. Verify that the error handling is implemented correctly and that the test case accurately reflects a realistic error scenario.
169-187: The test case "using consensus params for invalid gas limit (exceeds max gas)" is designed to test the behavior when the gas limit exceeds the max gas set in consensus parameters. Ensure that the test case is set up correctly and that the expected error is the appropriate one for this scenario.
190-208: The test case "using consensus params for max gas override - valid)" tests a scenario where the gas limit is within the max gas set in consensus parameters. Confirm that the test case is correctly testing a valid transaction and that the expected outcome is success without errors.
tests/app/app.go (5)
- 34-39: The removal of the crisis module should be verified to ensure that no references to it remain elsewhere in the codebase that could lead to compilation errors or runtime panics.
Verification successful
- The references to the crisis module found in
tests/app/feemarketd/cmd/root.goare for a dummy flag and do not indicate active use of the crisis module. This suggests that the removal of the crisis module fromapp.gois consistent and should not cause issues related to these references.* 94-99: The removal of the crisis module from `ModuleBasics` should be verified to ensure that it does not affect the application's functionality, especially in terms of invariants checking which is typically a role of the crisis module.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the crisis module. rg --type go 'crisis'Length of output: 306
- 129-134: The removal of the
CrisisKeeperfield from theTestAppstruct should be verified to ensure that there are no orphaned references or logic that depend on the crisis keeper.Verification successful
The search for
CrisisKeeperin the Go files of the codebase did not yield any results, indicating that there are no orphaned references or logic remaining after its removal from theTestAppstruct.* 206-211: The changes in the dependency injection setup should be verified to ensure that the removal of the crisis module's keeper is handled correctly and does not cause any issues with the application's initialization.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any orphaned references or logic that depend on the CrisisKeeper. rg --type go 'CrisisKeeper'Length of output: 27
Verification successful
To verify the changes in the dependency injection setup, a broader search for the initialization function of the
TestAppis needed. Let's try searching for justTestAppwithin Go files to locate the relevant initialization code.
To ensure that the removal of the crisis module's keeper is correctly reflected in the initialization of
TestApp, the contents of thetests/app/app.gofile need to be examined, particularly the constructor or initialization function.
The review of the
tests/app/app.gofile confirms that theCrisisKeeperhas been removed from theTestAppstruct and its constructor functionNew. Additionally, theConsensusParamsKeeperis present, which is in line with the PR's objectives to integrate the max block gas limits from EIP-1559 into the consensus parameters. No issues related to the removal of the crisis module's keeper are evident from the provided context.* 270-274: The addition of `ConsensusParamsKeeper` to `PostHandlerOptions` should be verified to ensure that it is correctly implemented and that the rest of the application is updated to use this new field where necessary.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any issues with the application's initialization due to the removal of the crisis module's keeper. rg --type go 'NewTestApp'Length of output: 25
Script:
#!/bin/bash # Broaden the search to find the initialization function for TestApp rg --type go 'TestApp'Length of output: 1441
Script:
#!/bin/bash # View the contents of the tests/app/app.go file to examine the initialization function cat tests/app/app.goLength of output: 13394
x/feemarket/keeper/feemarket_test.go (2)
- 77-77: The change in the
state.Updatefunction call to useparams.MaxBlockUtilizationinstead ofparamsshould be verified to ensure that the new argument is correct and that the logic of the tests still holds.Verification successful
The verification of the
state.Updatefunction call inx/feemarket/keeper/feemarket_test.goconfirms that the change to passparams.MaxBlockUtilizationas the second argument is correct, as theUpdatefunction is defined to accept twouint64arguments.* 101-101: Ensure that the changes to the `state.Update` function calls across various test cases are correct and reflect the intended logic for updating the fee market based on block utilization parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition of the Update function to ensure the new argument is correct. rg --type go 'func \(.*State\) Update'Length of output: 306
Also applies to: 124-124, 151-151, 177-177, 201-201, 228-228, 255-255
x/feemarket/types/state_test.go (7)
20-22: The change to pass
params.MaxBlockUtilizationinstead ofparamstostate.Updateis consistent with the PR's objective to align with EIP-1559's max block gas limits. Ensure that theUpdatefunction's signature has been modified to accept a singleuint64value for max block utilization, and that this change is reflected across all test cases.29-35: The test case "can add several txs to window" has been updated to use
params.MaxBlockUtilization. This change should be verified to ensure that theUpdatefunction is being called with the correct parameters and that the test logic reflects the intended behavior.42-43: The test case "errors when it exceeds max block utilization" correctly tests the boundary condition where the block utilization exceeds the maximum allowed. This is a good test for ensuring that the
Updatefunction enforces the max block gas limit as per EIP-1559.50-62: The test case "can update with several blocks in default eip-1559" has been updated to use
params.MaxBlockUtilization. It's important to verify that the state's window is updated correctly and that the test reflects the expected behavior after the update.69-89: The test case "can update with several blocks in default aimd eip-1559" has been updated to use
params.MaxBlockUtilization. This change should be verified to ensure that theUpdatefunction is being called with the correct parameters and that the test logic reflects the intended behavior for the AIMD (Additive Increase/Multiplicative Decrease) model.97-118: The test case "correctly wraps around with aimd eip-1559" has been updated to use
params.MaxBlockUtilization. This change should be verified to ensure that the state's window wraps around correctly and that the test reflects the expected behavior after the update.406-408: The test case "recovers from overflow with large max block utilization ratio" has been updated to use
params.MaxBlockUtilization. This is a critical test to ensure that the system can handle extreme values without causing an overflow, which is important for maintaining system stability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- tests/app/app.go (5 hunks)
- x/feemarket/ante/suite/suite.go (4 hunks)
- x/feemarket/post/fee.go (1 hunks)
- x/feemarket/post/fee_test.go (2 hunks)
- x/feemarket/types/state.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/app/app.go
Additional comments: 7
x/feemarket/types/state.go (1)
- 28-34: The
Updatemethod correctly checks if the block utilization exceeds the maximum allowed gas and returns an error if this condition is met. This change aligns with the PR objectives to integrate max block gas limits from EIP-1559 into the consensus parameters.x/feemarket/post/fee.go (1)
- 102-117: The logic to adjust
maxGasbased on the lesser ofconsensusMaxGasandparams.MaxBlockUtilizationis sound and correctly implemented. The error handling is also improved by wrapping the error fromstate.Updatewithsdkerrors.ErrTxTooLarge, providing more context in case of failure.x/feemarket/ante/suite/suite.go (2)
76-84: The
SetupTestSuitefunction has been correctly updated to set the consensus parameters in the context, which is necessary for testing with the new consensus parameters introduced by the PR.210-218: The
RunTestCasefunction now resets the consensus parameters after each test case, ensuring that each test starts with a clean state. This is a good practice for test isolation and reliability.x/feemarket/post/fee_test.go (3)
7-9: The addition of new imports for
tmprotoandtestkeeperis noted. Ensure that these packages are used in the test cases and that they are necessary for the changes related to EIP-1559.150-176: The test case "using consensus params for invalid gas limit (exceeds max gas)" correctly sets up a scenario where the gas limit exceeds the max gas set in the consensus parameters. This should indeed result in an
ErrTxTooLargeerror. Ensure that theMaxGasvalue used in the test reflects realistic scenarios based on EIP-1559 specifications.Verification successful
The output from the executed script provides several instances where
MaxGasis referenced in the codebase. Here's a breakdown of the findings:
- In
x/feemarket/post/fee.go,MaxGasis retrieved from the consensus parameters and compared withparams.MaxBlockUtilization. IfMaxGasfrom the consensus parameters is less thanparams.MaxBlockUtilizationand is not negative, it is used as themaxGasfor the block.- The test case in
x/feemarket/post/fee_test.gosetsMaxGasto 10, which is intended to test the scenario where the transaction gas limit exceeds the maximum gas allowed by the block parameters.- Another test case in
x/feemarket/post/fee_test.gosetsMaxGastogasLimit - 1to test the max gas override functionality.- In
testutils/keeper/keeper.go, a defaultMaxGasvalue of 1,000,000 is set in theConsensusParams.- In
x/feemarket/ante/suite/suite.go,MaxGasis set to 1,000,000,000 in the consensus parameters for the test suite context.The
MaxGasvalue of 10 in the test case is likely chosen to be deliberately low to ensure that the test triggers theErrTxTooLargeerror when the transaction gas limit exceeds this value. This is a valid test case to confirm that the system correctly handles transactions that exceed the block gas limit.Given the context, the test case seems to be correctly designed to test the behavior when the gas limit exceeds the max gas set in the consensus parameters, aligning with the objectives of the PR to integrate max block gas limits from EIP-1559 into the consensus parameters and to test their impact on block processing and transaction inclusion.
Based on the information provided, the review comment appears to be correct, and the test case is appropriately testing the intended behavior.
* 178-195: The test case "using consensus params for max gas override - valid" appears to be testing the scenario where the gas limit is just below the max gas, which should pass. This is a valid test case to ensure that transactions close to the gas limit are handled correctly. However, it's important to verify that the `gasLimit - 1` is a deliberate choice and aligns with the intended testing strategy.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the MaxGas value aligns with realistic EIP-1559 scenarios. rg --type go 'MaxGas' --context 5Length of output: 4135
Verification successful
The search for the expression
gasLimit - 1in Go files only returned the instance within the test case "using consensus params for max gas override - valid)" in thex/feemarket/post/fee_test.gofile. This indicates that the choice ofgasLimit - 1is specific to this test case and is not a pattern used elsewhere in the codebase.Given this context, it seems that the
gasLimit - 1is indeed a deliberate choice for this particular test case, likely intended to test the behavior of the system when the gas used is just below the maximum gas limit allowed. This would be a valid scenario to test to ensure that transactions are processed correctly when they are close to the gas limit.
- The use of
gasLimit - 1is specific to this test case.- It is a deliberate choice to test the system's behavior with a gas amount just below the max gas limit.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the gasLimit - 1 is a deliberate choice for testing. rg --type go 'gasLimit - 1' --context 5Length of output: 933
|
Closing as this is outdated and breaking |
Closes BLO-590
Adds the
consensusparamskeeper to thefeeposthandler. Wheneverconsensus.MaxGas<feeMarket.MaxGastheconsensusvalue is used. This effectively clamps our max gas to this value whenever we exceed it, but still allows the values to differ ifconsensus.MaxGas>feeMarket.MaxGas.Also removes the
crisismodule from the app. This module isnt needed for testing and contains vulnerabilitiesSummary by CodeRabbit
New Features
Bug Fixes
Refactor
ConsensusKeeperfor managing consensus parameters.Tests
Documentation
Chores