feat(sidecar): add max_committed_gas_per_slot to sidecar Limits #150
Conversation
|
Warning Rate limit exceeded@namn-grg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes enhance the Bolt sidecar's execution state management by introducing new gas commitment limits. The Changes
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
Nice work! Can you please add some tests? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- bolt-sidecar/bin/sidecar.rs (1 hunks)
- bolt-sidecar/src/builder/template.rs (1 hunks)
- bolt-sidecar/src/config/mod.rs (3 hunks)
- bolt-sidecar/src/state/execution.rs (13 hunks)
Additional comments not posted (10)
bolt-sidecar/bin/sidecar.rs (1)
31-35: Ensure correct usage of the new parameter.The
ExecutionStateis now initialized with an additional parameterconfig.limits.max_committed_gas_per_slot. Ensure that this parameter is correctly used in theExecutionStateimplementation.Verification successful
Verified: The new parameter
max_committed_gas_per_slotis correctly used in theExecutionStateimplementation.
- The
ExecutionStatestruct includes themax_committed_gas_per_slotfield.- The
newmethod inExecutionStatecorrectly initializes themax_committed_gas_per_slotfield with the provided parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new parameter in the `ExecutionState` implementation. # Test: Search for the implementation of `ExecutionState::new`. Expect: The new parameter is used correctly. ast-grep --lang rust --pattern $'impl ExecutionState { $$$ fn new($_, $_, $_) { $$$ } $$$ }'Length of output: 105
Script:
#!/bin/bash # Description: Search for the `ExecutionState` struct definition to understand its parameters. ast-grep --lang rust --pattern 'struct ExecutionState { $$$ }' # Description: Search for the `new` method implementation of `ExecutionState`. ast-grep --lang rust --pattern 'impl ExecutionState { fn new($_, $_, $_) { $$$ } }'Length of output: 160
Script:
#!/bin/bash # Description: Search for the `ExecutionState` keyword in the codebase to locate its struct definition and method implementations. rg 'ExecutionState'Length of output: 1418
Script:
#!/bin/bash # Description: Extract the `ExecutionState` struct definition from `bolt-sidecar/src/state/execution.rs`. rg -A 20 'pub struct ExecutionState' bolt-sidecar/src/state/execution.rs # Description: Extract the `new` method implementation from `bolt-sidecar/src/state/execution.rs`. rg -A 30 'impl<C: StateFetcher> ExecutionState<C> {' bolt-sidecar/src/state/execution.rsLength of output: 1205
Script:
#!/bin/bash # Description: Extract the complete `ExecutionState` struct definition from `bolt-sidecar/src/state/execution.rs`. rg -A 40 'pub struct ExecutionState' bolt-sidecar/src/state/execution.rs # Description: Extract the `new` method implementation from `bolt-sidecar/src/state/execution.rs` with corrected regex. rg -A 40 'impl<C: StateFetcher> ExecutionState<C> \{' bolt-sidecar/src/state/execution.rsLength of output: 3031
bolt-sidecar/src/config/mod.rs (4)
143-143: Ensure correct usage of the new field.The
max_committed_gas_per_slotfield is added to theLimitsstruct. Ensure that this field is correctly used in the application logic.
50-52: Ensure correct usage of the new field.The
max_committed_gasfield is added to theOptsstruct. Ensure that this field is correctly used in the configuration process.Verification successful
Ensure correct usage of the new field.
The
max_committed_gasfield is added to theOptsstruct and is used in various parts of the codebase. The instances found insidecar.rs,execution.rs, andmod.rssuggest that the field is integrated into the configuration process and state management. Ensure that these usages are correct and consistent with the intended functionality.
bolt-sidecar/bin/sidecar.rs: Line 78bolt-sidecar/src/state/execution.rs: Lines 50, 52, 54, 56, 58, 60, 62bolt-sidecar/src/config/mod.rs: Lines 52, 54, 56, 58, 60Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new field `max_committed_gas` in the configuration process. # Test: Search for the usage of `max_committed_gas`. Expect: The field is used correctly in the configuration process. rg --type rust 'max_committed_gas'Length of output: 1168
150-150: Verify the appropriateness of the default value.The default value for
max_committed_gas_per_slotis set to 10,000,000. Ensure that this value is appropriate for the application's requirements.
177-180: Ensure correct integration of the new option.The
max_committed_gasoption is integrated into theTryFrom<Opts>implementation. Ensure that this option is correctly used in the configuration process.Verification successful
Integration of
max_committed_gasis correct.The
max_committed_gasoption is correctly integrated into theTryFrom<Opts>implementation and is used appropriately in the configuration process.
mod.rs: Defined in theLimitsstruct and set within theTryFrom<Opts>implementation.execution.rs: Utilized in theExecutionStateinitialization and validation logic.sidecar.rs: Passed to theExecutionStateduring initialization.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the `max_committed_gas` option in the `TryFrom<Opts>` implementation. # Test: Search for the usage of `max_committed_gas` in the `TryFrom<Opts>` implementation. Expect: The option is used correctly in the configuration process. rg --type rust 'max_committed_gas'Length of output: 1168
Script:
#!/bin/bash # Description: Gather detailed context for the usage of `max_committed_gas` in the relevant files. # Display lines around the occurrences of `max_committed_gas` to understand its context and usage. rg --type rust 'max_committed_gas' -A 5 -B 5Length of output: 7921
bolt-sidecar/src/builder/template.rs (1)
114-124: Ensure correct implementation and usage of the new method.The
committed_gasmethod calculates and returns the total committed gas for the block template. Ensure that this method is correctly implemented and verify its usage.Verification successful
The
committed_gasmethod is correctly implemented and used.The method is used in
bolt-sidecar/src/state/execution.rsto enforce the constraint on the maximum committed gas per slot, ensuring the block template adheres to the allowed gas limits.
bolt-sidecar/src/state/execution.rs: Usage ofcommitted_gasto check and enforce gas limits.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new method `committed_gas` in the `BlockTemplate` implementation. # Test: Search for the usage of `committed_gas`. Expect: The method is used correctly in the `BlockTemplate` implementation. rg --type rust 'committed_gas'Length of output: 1243
Script:
#!/bin/bash # Description: Extract and review the context of the `committed_gas` method usage in `bolt-sidecar/src/state/execution.rs`. # Extract 10 lines before and after the usage of `committed_gas` for context. rg --type rust -A 10 -B 10 'template.committed_gas\(\)' bolt-sidecar/src/state/execution.rsLength of output: 1050
bolt-sidecar/src/state/execution.rs (4)
210-226: LGTM! Verify the logic and error handling.The addition of checks for
max_committed_gas_per_slotin thevalidate_commitment_requestfunction looks good.Ensure that the logic and error handling are correctly implemented.
Verification successful
LGTM! Verify the logic and error handling.
The addition of checks for
max_committed_gas_per_slotin thevalidate_commitment_requestfunction looks good. The logic and error handling are correctly implemented, as evidenced by the comprehensive test cases and error handling in the actual usage.
- Test cases in
execution.rscover various validation errors.- Error handling in
sidecar.rsincludes proper logging and response handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic and error handling for `max_committed_gas_per_slot` in the `validate_commitment_request` function. # Test: Search for the usage of the `validate_commitment_request` function. Expect: Proper logic and error handling. rg --type rust 'validate_commitment_request'Length of output: 1290
Script:
#!/bin/bash # Extract context around the usage of `validate_commitment_request` function to review logic and error handling. # Extract 10 lines of context around each match rg --type rust 'validate_commitment_request' -A 10 -B 10Length of output: 17325
119-120: LGTM! Verify the initialization and usage of the new field.The addition of
max_committed_gas_per_slotto theExecutionStatestruct looks good.Ensure that the new field is correctly initialized and used in the codebase.
Verification successful
Initialization and usage of the
max_committed_gas_per_slotfield are correct.
- The field is initialized with a valid non-zero value in
config/mod.rs.- It is used appropriately for validation in
execution.rs.- It is passed as part of the configuration in
sidecar.rs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of the new `max_committed_gas_per_slot` field in the codebase. # Test: Search for the initialization and usage of the new field. Expect: Proper initialization and usage of the new field. rg --type rust 'max_committed_gas_per_slot'Length of output: 993
Script:
#!/bin/bash # Description: Verify the initialization and usage of the new `max_committed_gas_per_slot` field in the codebase. # Check initialization in config/mod.rs rg --type rust 'max_committed_gas_per_slot' -A 5 bolt-sidecar/src/config/mod.rs # Check usage in execution.rs rg --type rust 'max_committed_gas_per_slot' -A 5 bolt-sidecar/src/state/execution.rs # Check usage in sidecar.rs rg --type rust 'max_committed_gas_per_slot' -A 5 bolt-sidecar/bin/sidecar.rsLength of output: 2341
63-65: LGTM! Verify the usage of the newValidationErrorvariant.The addition of
MaxCommittedGasReachedForSlotto theValidationErrorenum looks good and the error message format is consistent with the existing ones.Ensure that the new error variant is correctly used and handled in the codebase.
Line range hint
153-168:
LGTM! Verify the parameter passing and initialization logic.The modification to the
newfunction to acceptmax_committed_gas_per_slotas a parameter looks good.Ensure that the parameter passing and initialization logic are correctly implemented.
Verification successful
LGTM! The parameter passing and initialization logic for
max_committed_gas_per_slotare correctly implemented.
- The
newfunction inExecutionStatecorrectly acceptsmax_committed_gas_per_slotas a parameter.- The initialization logic within the function properly assigns this parameter to the corresponding field in the
Selfstruct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the parameter passing and initialization logic for `max_committed_gas_per_slot` in the `new` function. # Test: Search for the usage of the `new` function. Expect: Proper parameter passing and initialization logic. rg --type rust 'ExecutionState::new'Length of output: 1107
Script:
#!/bin/bash # Description: Extract the implementation of the `new` function in `ExecutionState` to verify parameter passing and initialization logic. # Fetch the implementation of the `new` function in `ExecutionState`. ast-grep --lang rust --pattern 'impl ExecutionState { pub async fn new($_, $_, $_) -> Result<Self, TransportError> { $$$ } }' --debugLength of output: 361
Script:
#!/bin/bash # Description: Extract the implementation of the `new` function in `ExecutionState` to verify parameter passing and initialization logic. # Fetch the implementation of the `new` function in `ExecutionState`. ast-grep --lang rust --pattern 'impl ExecutionState { pub async fn new($_, $_, $_) -> Result<Self, TransportError> { $$$ } }' --debug-queryLength of output: 346
Script:
#!/bin/bash # Description: Extract the implementation of the `new` function in `ExecutionState` to verify parameter passing and initialization logic. # Fetch the implementation of the `new` function in `ExecutionState`. rg 'impl ExecutionState' -A 50 --type rustLength of output: 44
Script:
#!/bin/bash # Description: Extract the implementation of the `new` function in `ExecutionState` to verify parameter passing and initialization logic. # Fetch the implementation of the `new` function in `ExecutionState`. rg 'pub async fn new' -A 50 --type rustLength of output: 9023
bolt-sidecar/src/state/execution.rs
Outdated
| // Check if the committed gas exceeds the maximum | ||
| if template.committed_gas().to::<u64>() >= max_committed_gas_per_slot { | ||
| return Err(ValidationError::MaxCommittedGasReachedForSlot( | ||
| self.slot, | ||
| max_committed_gas_per_slot, | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Here I think you should check if
template.committed_gas() + tx.gas_limit >= max_committed_gas_per_slot
otherwise with the previous request I can ask for huge gas limit and overcome the max value.
Does it make sense?
There was a problem hiding this comment.
Yes good point! Then I think we should also do the tx.gas_limit >= max_committed_gas_per_slot check in case no template is there right? updated
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- bolt-sidecar/bin/sidecar.rs (1 hunks)
- bolt-sidecar/src/builder/template.rs (1 hunks)
- bolt-sidecar/src/state/execution.rs (17 hunks)
Additional comments not posted (6)
bolt-sidecar/bin/sidecar.rs (1)
31-31: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
ExecutionState::newmatch the new signature.Verification successful
All instances of
ExecutionState::newmatch the new signature.The function calls to
ExecutionState::neware consistent with the new signature across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ExecutionState::new` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'ExecutionState::new'Length of output: 5228
bolt-sidecar/src/builder/template.rs (1)
114-124: LGTM! Thecommitted_gasmethod is well-implemented.The method correctly calculates the total committed gas in the block template.
bolt-sidecar/src/state/execution.rs (4)
64-66: New validation error type added.The
MaxCommittedGasReachedForSlotvariant is correctly added to theValidationErrorenum.
120-121: New field added toExecutionState.The
max_committed_gas_per_slotfield is correctly added to theExecutionStatestruct.
Line range hint
151-165:
Constructor updated to acceptLimitsstruct.The constructor is correctly updated to accept the
Limitsstruct and initialize the new field.
Line range hint
207-233:
Validation logic updated to check committed gas.The validation logic is correctly updated to check the committed gas against the new limit.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- bolt-sidecar/src/state/execution.rs (16 hunks)
Additional comments not posted (5)
bolt-sidecar/src/state/execution.rs (5)
64-66: Addition of new validation error type looks good.The new
MaxCommittedGasReachedForSloterror type is correctly defined.
120-121: Addition of new fieldmax_committed_gas_per_slotlooks good.The new field
max_committed_gas_per_slotis correctly defined.
Line range hint
151-165:
Modification of constructor methodnewlooks good.The constructor method
newhas been correctly updated to accept aLimitsstruct.
217-235: Addition of checks for maximum committed gas looks good.The new checks for maximum committed gas have been correctly added to the
validate_commitment_requestmethod.
Line range hint
725-926:
Addition of new testtest_invalid_inclusion_request_with_excess_gaslooks good.The new test
test_invalid_inclusion_request_with_excess_gasis correctly defined.
bolt-sidecar/src/state/execution.rs
Outdated
| chain_id: u64, | ||
| /// The maximum number of commitments per slot. | ||
| max_commitments_per_slot: NonZero<usize>, | ||
| /// The maximum committed gas per slot. | ||
| max_committed_gas_per_slot: NonZero<u64>, | ||
| /// The KZG settings for validating blobs. |
There was a problem hiding this comment.
Is it better to make them into a single field - limits?
Co-authored-by: Lorenzo <lorenzo.feroleto.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- bolt-sidecar/src/state/execution.rs (16 hunks)
Additional comments not posted (4)
bolt-sidecar/src/state/execution.rs (4)
64-66: Addition of new validation error variant looks good.The new variant
MaxCommittedGasReachedForSlothas been correctly added to theValidationErrorenum.
120-121: Addition of new fieldmax_committed_gas_per_slotlooks good.The new field
max_committed_gas_per_slothas been correctly added to theExecutionStatestruct.
Line range hint
151-165:
Updates to the constructor methodnewlook good.The constructor method
newhas been correctly updated to accept aLimitsstruct and initialize the new fieldmax_committed_gas_per_slot.
602-605: New test cases look good.The new test cases correctly validate the behavior of the system when the committed gas exceeds the defined limit.
Also applies to: 724-754, 871-925
bolt-sidecar/src/state/execution.rs
Outdated
| chain_id: u64, | ||
| /// The maximum number of commitments per slot. | ||
| max_commitments_per_slot: NonZero<usize>, | ||
| /// The maximum committed gas per slot. | ||
| max_committed_gas_per_slot: NonZero<u64>, | ||
| /// The KZG settings for validating blobs. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (3)
- bolt-sidecar/bin/sidecar.rs (1 hunks)
- bolt-sidecar/src/config/mod.rs (3 hunks)
- bolt-sidecar/src/state/execution.rs (16 hunks)
Additional comments not posted (8)
bolt-sidecar/bin/sidecar.rs (1)
31-31: Initialization ofExecutionStatewithconfig.limitslooks good.The change improves the flexibility and maintainability of the initialization process.
bolt-sidecar/src/config/mod.rs (3)
50-52: Addition ofmax_committed_gastoOptsstruct looks good.The new field aligns with the PR objectives and is correctly integrated.
143-150: Addition ofmax_committed_gas_per_slottoLimitsstruct and update todefaultimplementation look good.The new field aligns with the PR objectives and is correctly integrated.
177-179: Modification toTryFrom<Opts>forConfiglooks good.The change ensures that the new gas limit is integrated into the configuration process.
bolt-sidecar/src/state/execution.rs (4)
64-66: Addition ofMaxCommittedGasReachedForSlottoValidationErrorenum looks good.The new variant aligns with the PR objectives and is correctly integrated.
118-119: Update toExecutionStatestruct to includelimitsfield looks good.The change improves the management of limits related to transaction commitments and gas usage.
Line range hint
149-162:
Update tonewmethod ofExecutionStateto acceptLimitsstruct looks good.The change improves the initialization process by passing the entire
limitsobject.
206-226: Updates tovalidate_commitment_requestlook good.The method has been correctly updated to include checks for the new
max_committed_gas_per_slotlimit. Ensure that redundant checks are avoided.
fixes #144
Summary by CodeRabbit
New Features
Bug Fixes
Documentation