Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(bolt-sidecar): add params in err message #660

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions bolt-sidecar/src/primitives/commitment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,24 +185,24 @@ impl InclusionRequest {
preconfirmed_gas: u64,
min_inclusion_profit: u64,
max_base_fee: u128,
) -> Result<bool, PricingError> {
) -> Result<(bool, u128, u128), PricingError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to keep the signature like before? Result<bool, PricingError> and embed the values in the error instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us know if you get stuck!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
) -> Result<(bool, u128, u128), PricingError> {
) -> Result<bool, PricingError> {

// Each included tx will move the price up
// So we need to calculate the minimum priority fee for each tx
let mut local_preconfirmed_gas = preconfirmed_gas;
for tx in &self.txs {
// Calculate minimum required priority fee for this transaction
let min_priority_fee = pricing
.calculate_min_priority_fee(tx.gas_limit(), preconfirmed_gas)?
+ min_inclusion_profit;
.calculate_min_priority_fee(tx.gas_limit(), preconfirmed_gas)? +
min_inclusion_profit;

let tip = tx.effective_tip_per_gas(max_base_fee).unwrap_or_default();
if tip < min_priority_fee as u128 {
return Ok(false);
return Ok((false, tip, min_priority_fee as u128));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Ok((false, tip, min_priority_fee as u128));
return Ok(false);

}
// Increment the preconfirmed gas for the next transaction in the bundle
local_preconfirmed_gas = local_preconfirmed_gas.saturating_add(tx.gas_limit());
}
Ok(true)
Ok((true, 0, 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Ok((true, 0, 0))
Ok(true)

}

/// Returns the total gas limit of all transactions in this request.
Expand Down
25 changes: 13 additions & 12 deletions bolt-sidecar/src/state/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use std::collections::HashMap;
use thiserror::Error;
use tracing::{debug, error, trace, warn};

use crate::state::pricing;
use crate::{
builder::BlockTemplate,
common::{
Expand All @@ -19,11 +18,11 @@ use crate::{
primitives::{
signature::SignatureError, AccountState, InclusionRequest, SignedConstraints, Slot,
},
state::pricing,
telemetry::ApiMetrics,
};

use super::InclusionPricer;
use super::{account_state::AccountStateCache, fetcher::StateFetcher};
use super::{account_state::AccountStateCache, fetcher::StateFetcher, InclusionPricer};

/// Possible commitment validation errors.
///
Expand Down Expand Up @@ -61,8 +60,8 @@ pub enum ValidationError {
#[error("Max priority fee per gas is greater than max fee per gas")]
MaxPriorityFeePerGasTooHigh,
/// Max priority fee per gas is less than min priority fee.
#[error("Max priority fee per gas is less than min priority fee")]
MaxPriorityFeePerGasTooLow,
#[error("Max priority fee per gas {0} is less than min priority fee {1}")]
MaxPriorityFeePerGasTooLow(u128, u128),
/// The sender does not have enough balance to pay for the transaction.
#[error("Not enough balance to pay for value + maximum fee")]
InsufficientBalance,
Expand Down Expand Up @@ -114,7 +113,7 @@ impl ValidationError {
Self::GasLimitTooHigh => "gas_limit_too_high",
Self::TransactionSizeTooHigh => "transaction_size_too_high",
Self::MaxPriorityFeePerGasTooHigh => "max_priority_fee_per_gas_too_high",
Self::MaxPriorityFeePerGasTooLow => "max_priority_fee_per_gas_too_low",
Self::MaxPriorityFeePerGasTooLow(_, _) => "max_priority_fee_per_gas_too_low",
Self::InsufficientBalance => "insufficient_balance",
Self::Pricing(_) => "pricing",
Self::Eip4844Limit => "eip4844_limit",
Expand Down Expand Up @@ -320,14 +319,16 @@ impl<C: StateFetcher> ExecutionState<C> {
return Err(ValidationError::BaseFeeTooLow(max_basefee));
}

// Ensure max_priority_fee_per_gas is greater than or equal to the calculated min_priority_fee
if !req.validate_min_priority_fee(
// Ensure max_priority_fee_per_gas is greater than or equal to the calculated
// min_priority_fee
let (validated, tip, min_priority_fee) = req.validate_min_priority_fee(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let Err(err) = req.validate_min_priority_fee(

&self.pricing,
template_committed_gas,
self.limits.min_inclusion_profit,
max_basefee,
)? {
return Err(ValidationError::MaxPriorityFeePerGasTooLow);
)?;
if !validated {
return Err(ValidationError::MaxPriorityFeePerGasTooLow(tip, min_priority_fee));
Copy link
Contributor

@0ex-d 0ex-d Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
)?;
if !validated {
return Err(ValidationError::MaxPriorityFeePerGasTooLow(tip, min_priority_fee));
) {
return Err(err.into());

}

if target_slot < self.slot {
Expand Down Expand Up @@ -977,7 +978,7 @@ mod tests {

assert!(matches!(
state.validate_request(&mut request).await,
Err(ValidationError::MaxPriorityFeePerGasTooLow)
Err(ValidationError::MaxPriorityFeePerGasTooLow(_, _))
));

// Create a transaction with a max priority fee that is correct
Expand Down Expand Up @@ -1020,7 +1021,7 @@ mod tests {

assert!(matches!(
state.validate_request(&mut request).await,
Err(ValidationError::MaxPriorityFeePerGasTooLow)
Err(ValidationError::MaxPriorityFeePerGasTooLow(_, _))
));

// Create a transaction with a gas price that is correct
Expand Down
5 changes: 2 additions & 3 deletions bolt-sidecar/src/state/pricing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ impl InclusionPricer {
/// important reason is that we omit large outlier transactions to improve average
/// fit, which disproportionately affects the most valuable transactions.
/// """
///
pub fn calculate_min_priority_fee(
&self,
incoming_gas: u64,
Expand All @@ -84,8 +83,8 @@ impl InclusionPricer {
let after_gas = remaining_gas - incoming_gas;

// Calculate numerator and denominator for the logarithm
let fraction = (self.gas_scalar * (remaining_gas as f64) + 1.0)
/ (self.gas_scalar * (after_gas as f64) + 1.0);
let fraction = (self.gas_scalar * (remaining_gas as f64) + 1.0) /
(self.gas_scalar * (after_gas as f64) + 1.0);

// Calculate block space value in Ether
let block_space_value = self.base_multiplier * fraction.ln();
Expand Down