feat(sidecar): min priority fee #246
Conversation
bolt-sidecar/src/config/mod.rs
Outdated
| /// Min priority fee to accept for a commitment | ||
| #[clap(long, env = "BOLT_SIDECAR_MIN_PRIORITY_FEE")] | ||
| pub(super) min_priority_fee: Option<NonZero<u64>>, |
There was a problem hiding this comment.
Isn't u128 a better type to express priority fees? I'm not sure tho, could you take a look?
| .max_priority_fee_per_gas() | ||
| .is_some_and(|max_priority_fee| max_priority_fee < min_priority_fee) |
There was a problem hiding this comment.
What if in all the txs the max_priority_fee_per_gas is None? That is the case for legacy transactions and should be handled carefully by looking at the gas_price instead.
| if max_priority_fee + gas_price > tx.max_fee_per_gas() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
btw this was already there though
| let gas_price = tx.as_legacy().map(|tx| tx.gas_price).unwrap_or(0); | ||
| let gas_price = if max_base_fee >= gas_price { | ||
| 0 | ||
| } else { | ||
| (gas_price - max_base_fee) / GWEI_TO_WEI as u128 | ||
| }; | ||
| let max_priority_fee = tx.max_priority_fee_per_gas().unwrap_or(0); | ||
|
|
||
| if gas_price + max_priority_fee < min_priority_fee { | ||
| return false; |
There was a problem hiding this comment.
I don't have clear some points:
- How do you handle type 1 / EIP-2930 transactions?
- Why are you dividing the gas price for
GWEI_TO_WEI? - Can't you just check if to use
gas_priceormax_priority_feeby checking the type of the transaction? - Can you add some comments as well with steps and motivation?
There was a problem hiding this comment.
good point, addressed, thanks
|
@coderabbitai review |
WalkthroughThe pull request introduces several modifications across multiple files in the Bolt sidecar project. Key changes include updates to documentation for the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- bolt-sidecar/src/common.rs (1 hunks)
- bolt-sidecar/src/config/mod.rs (4 hunks)
- bolt-sidecar/src/primitives/commitment.rs (1 hunks)
- bolt-sidecar/src/primitives/mod.rs (3 hunks)
- bolt-sidecar/src/state/execution.rs (10 hunks)
- bolt-sidecar/src/test_util.rs (2 hunks)
🔇 Additional comments (17)
bolt-sidecar/src/primitives/commitment.rs (1)
146-157: LGTM: Improved method name and logic.The renaming and logic update correctly validate the max priority fee against the max fee per gas, aligning with EIP-1559.
bolt-sidecar/src/test_util.rs (3)
115-115: Good clarification.Comment explains the value unit. Helpful for understanding.
192-192: Efficient iteration.New order reduces cloning operations. Improves performance for large collections.
Line range hint
1-282: LGTM. Minor improvements made.Changes enhance clarity and efficiency. No issues found.
bolt-sidecar/src/config/mod.rs (6)
31-34: LGTM! Constants added as suggested.The new constants are well-defined and exposed. The
DEFAULT_MIN_PRIORITY_FEEis set to 1 Gwei as recommended.
63-65: LGTM!min_priority_feefield added correctly.The new field uses
u128as suggested and is properly defined as an optionalNonZerovalue.
169-171: LGTM!min_priority_feefield added toLimitsstruct.The field is correctly defined as a
NonZero<u128>with an accurate comment.
177-181: LGTM! Default implementation updated correctly.The
min_priority_feeis properly initialized with the default constant.
212-215: LGTM!TryFrom<Opts>implementation updated correctly.The
min_priority_feeoption is properly handled and assigned to the config.
Line range hint
1-285: Overall, the changes successfully implement the minimum priority fee configuration.The implementation aligns well with the PR objectives and addresses the linked issue #228. The code quality is good, and past review suggestions have been incorporated effectively.
bolt-sidecar/src/primitives/mod.rs (2)
464-465: LGTM. Simplified implementation.The change removes unnecessary references, improving code clarity.
486-487: LGTM. Consistent with DelegationMessage.The change aligns with the
DelegationMessageimplementation, maintaining consistency.bolt-sidecar/src/state/execution.rs (5)
57-59: Add new validation error for minimum priority feeThe new
ValidationErrorvariantMaxPriorityFeePerGasTooLowis properly declared with a clear error message. This enhances the error handling for transactions that do not meet the minimum priority fee requirement.
108-108: Include new error variant into_tag_strmethodGood job adding
MaxPriorityFeePerGasTooLowto theto_tag_strmethod. This ensures the error is correctly tagged for metrics and logging purposes.
301-305: Validate minimum priority fee invalidate_requestThe addition of the minimum priority fee check in
validate_requestcorrectly ensures thatmax_priority_fee_per_gasmeets or exceeds the configured minimum. The logic is sound and aligns with the intended behavior.
830-870: Add test for minimum priority fee validationGreat work on adding the
test_invalid_inclusion_request_min_priority_feetest. It effectively checks scenarios where the priority fee is too low and confirms that transactions with sufficient priority fee are accepted.
872-917: Include test for legacy transactionsExcellent job addressing previous feedback by adding the
test_invalid_inclusion_request_min_priority_fee_legacytest. This ensures that legacy transactions are also validated against the minimum priority fee, enhancing test coverage.
| /// Validates the priority fee against a minimum priority fee. | ||
| /// Returns `true` if the "effective priority fee" is greater than or equal to the set minimum | ||
| /// priority fee, `false` otherwise. | ||
| pub fn validate_min_priority_fee(&self, max_base_fee: u128, min_priority_fee: u128) -> bool { | ||
| for tx in &self.txs { | ||
| // If this returns None, the fee is lower than the basefee | ||
| let Some(tip) = tx.effective_tip_per_gas(max_base_fee) else { | ||
| return false; | ||
| }; | ||
|
|
||
| // Check if the effective gas tip is more than the minimum priority fee | ||
| if tip < min_priority_fee { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // If all transactions meet the minimum priority fee, return true | ||
| true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: New method implements minimum priority fee validation.
The method correctly validates the effective priority fee against a minimum threshold, addressing the PR objective.
Consider using all() for a more concise implementation:
pub fn validate_min_priority_fee(&self, max_base_fee: u128, min_priority_fee: u128) -> bool {
- for tx in &self.txs {
- // If this returns None, the fee is lower than the basefee
- let Some(tip) = tx.effective_tip_per_gas(max_base_fee) else {
- return false;
- };
-
- // Check if the effective gas tip is more than the minimum priority fee
- if tip < min_priority_fee {
- return false;
- }
- }
-
- // If all transactions meet the minimum priority fee, return true
- true
+ self.txs.iter().all(|tx| {
+ tx.effective_tip_per_gas(max_base_fee)
+ .map_or(false, |tip| tip >= min_priority_fee)
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Validates the priority fee against a minimum priority fee. | |
| /// Returns `true` if the "effective priority fee" is greater than or equal to the set minimum | |
| /// priority fee, `false` otherwise. | |
| pub fn validate_min_priority_fee(&self, max_base_fee: u128, min_priority_fee: u128) -> bool { | |
| for tx in &self.txs { | |
| // If this returns None, the fee is lower than the basefee | |
| let Some(tip) = tx.effective_tip_per_gas(max_base_fee) else { | |
| return false; | |
| }; | |
| // Check if the effective gas tip is more than the minimum priority fee | |
| if tip < min_priority_fee { | |
| return false; | |
| } | |
| } | |
| // If all transactions meet the minimum priority fee, return true | |
| true | |
| } | |
| /// Validates the priority fee against a minimum priority fee. | |
| /// Returns `true` if the "effective priority fee" is greater than or equal to the set minimum | |
| /// priority fee, `false` otherwise. | |
| pub fn validate_min_priority_fee(&self, max_base_fee: u128, min_priority_fee: u128) -> bool { | |
| self.txs.iter().all(|tx| { | |
| tx.effective_tip_per_gas(max_base_fee) | |
| .map_or(false, |tip| tip >= min_priority_fee) | |
| }) | |
| } |
There was a problem hiding this comment.
@merklefruit, glad you liked it!
(_/)
(•‿•)
/︶\
| pub fn effective_tip_per_gas(&self, base_fee: u128) -> Option<u128> { | ||
| let max_fee_per_gas = self.max_fee_per_gas(); | ||
|
|
||
| if max_fee_per_gas < base_fee { | ||
| return None; | ||
| } | ||
|
|
||
| // Calculate the difference between max_fee_per_gas and base_fee | ||
| let fee = max_fee_per_gas - base_fee; | ||
|
|
||
| // Compare the fee with max_priority_fee_per_gas (or gas price for non-EIP1559 transactions) | ||
| if let Some(priority_fee) = self.max_priority_fee_per_gas() { | ||
| Some(fee.min(priority_fee)) | ||
| } else { | ||
| Some(fee) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM. Consider adding a comment for clarity.
The implementation looks correct. For better readability, consider adding a brief comment explaining the fee calculation.
// Calculate the difference between max_fee_per_gas and base_fee
let fee = max_fee_per_gas - base_fee;
+
+// Effective tip is the minimum of fee and max_priority_fee_per_gas (if available)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn effective_tip_per_gas(&self, base_fee: u128) -> Option<u128> { | |
| let max_fee_per_gas = self.max_fee_per_gas(); | |
| if max_fee_per_gas < base_fee { | |
| return None; | |
| } | |
| // Calculate the difference between max_fee_per_gas and base_fee | |
| let fee = max_fee_per_gas - base_fee; | |
| // Compare the fee with max_priority_fee_per_gas (or gas price for non-EIP1559 transactions) | |
| if let Some(priority_fee) = self.max_priority_fee_per_gas() { | |
| Some(fee.min(priority_fee)) | |
| } else { | |
| Some(fee) | |
| } | |
| } | |
| pub fn effective_tip_per_gas(&self, base_fee: u128) -> Option<u128> { | |
| let max_fee_per_gas = self.max_fee_per_gas(); | |
| if max_fee_per_gas < base_fee { | |
| return None; | |
| } | |
| // Calculate the difference between max_fee_per_gas and base_fee | |
| let fee = max_fee_per_gas - base_fee; | |
| // Effective tip is the minimum of fee and max_priority_fee_per_gas (if available) | |
| // Compare the fee with max_priority_fee_per_gas (or gas price for non-EIP1559 transactions) | |
| if let Some(priority_fee) = self.max_priority_fee_per_gas() { | |
| Some(fee.min(priority_fee)) | |
| } else { | |
| Some(fee) | |
| } | |
| } |
fixes #228
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
calculate_max_basefeefunction regarding input units.Refactor