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

feat(bolt-sidecar): use pricing model to validate priority fee #599

Merged
merged 17 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
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
4 changes: 2 additions & 2 deletions bolt-sidecar/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ BOLT_SIDECAR_BUILDER_PRIVATE_KEY=
BOLT_SIDECAR_MAX_COMMITMENTS_PER_SLOT=128
# Max committed gas per slot
BOLT_SIDECAR_MAX_COMMITTED_GAS_PER_SLOT=10_000_000
# Min priority fee to accept for a commitment
BOLT_SIDECAR_MIN_PRIORITY_FEE=2000000000 # 2 Gwei = 2 * 10^9 wei
# Min profit per gas to accept a commitment
BOLT_SIDECAR_MIN_PROFIT=2000000000 # 2 Gwei = 2 * 10^9 wei
estensen marked this conversation as resolved.
Show resolved Hide resolved

# Chain configuration
# Chain on which the sidecar is running
Expand Down
8 changes: 4 additions & 4 deletions bolt-sidecar/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,11 @@ Options:
[env: BOLT_SIDECAR_MAX_COMMITTED_GAS=]
[default: 10000000]

--min-priority-fee <MIN_PRIORITY_FEE>
Min priority fee to accept for a commitment
--min-inclusion-profit <MIN_INCLUSION_PROFIT>
Min profit per gas to accept a commitment

[env: BOLT_SIDECAR_MIN_PRIORITY_FEE=]
[default: 1000000000]
[env: BOLT_SIDECAR_MIN_PROFIT=]
[default: 2000000000]

--chain <CHAIN>
Chain on which the sidecar is running
Expand Down
14 changes: 7 additions & 7 deletions bolt-sidecar/src/config/limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ pub const DEFAULT_MAX_COMMITMENTS: usize = 128;
/// Default max committed gas per block.
pub const DEFAULT_MAX_COMMITTED_GAS: u64 = 10_000_000;

/// Default min priority fee to accept for a commitment.
pub const DEFAULT_MIN_PRIORITY_FEE: u128 = 1_000_000_000; // 1 Gwei
/// Default min profit to accept for a commitment.
pub const DEFAULT_MIN_PROFIT: u64 = 2_000_000_000; // 2 Gwei

/// Default max account states size.
pub const DEFAULT_MAX_ACCOUNT_STATES_SIZE: u64 = 1_024;
Expand All @@ -32,13 +32,13 @@ pub struct LimitsOpts {
default_value_t = LimitsOpts::default().max_committed_gas_per_slot
)]
pub max_committed_gas_per_slot: NonZero<u64>,
/// Min priority fee to accept for a commitment
/// Min profit per gas to accept a commitment
#[clap(
long,
env = "BOLT_SIDECAR_MIN_PRIORITY_FEE",
default_value_t = LimitsOpts::default().min_priority_fee
env = "BOLT_SIDECAR_MIN_PROFIT",
default_value_t = LimitsOpts::default().min_inclusion_profit
)]
pub min_priority_fee: u128,
pub min_inclusion_profit: u64,
/// The maximum size in MiB of the [crate::state::ExecutionState] ScoreCache that holds account
/// states. Each [crate::primitives::AccountState] is 48 bytes, its score is [usize] bytes, and
/// its key is 20 bytes, so the default value of 1024 KiB = 1 MiB can hold around 15k account
Expand All @@ -58,7 +58,7 @@ impl Default for LimitsOpts {
.expect("Valid non-zero"),
max_committed_gas_per_slot: NonZero::new(DEFAULT_MAX_COMMITTED_GAS)
.expect("Valid non-zero"),
min_priority_fee: DEFAULT_MIN_PRIORITY_FEE,
min_inclusion_profit: DEFAULT_MIN_PROFIT,
max_account_states_size: NonZero::new(1_024).expect("Valid non-zero"),
}
}
Expand Down
34 changes: 29 additions & 5 deletions bolt-sidecar/src/primitives/commitment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ use alloy::{
};
use serde::{de, Deserialize, Deserializer, Serialize};

use crate::crypto::SignerECDSA;
use crate::{
crypto::SignerECDSA,
state::{pricing::PricingError, PreconfPricing},
};

use super::{deserialize_txs, serialize_txs, FullTransaction, TransactionExt};

Expand Down Expand Up @@ -173,10 +176,31 @@ impl InclusionRequest {
/// 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)
})
/// Returns an error if min priority fee cannot be calculated.
pub fn 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.

I would move the bulk logic of this function to ExecutionState, and just have this validate that the effective_tip_per_gas is greater than the result of running the pricing algo. So the function can actually remain the same as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok if I create an issue and follow up in a separate PR? Keen on getting this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

&self,
pricing: &PreconfPricing,
preconfirmed_gas: u64,
min_inclusion_profit: u64,
max_base_fee: u128,
) -> 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;

let tip = tx.effective_tip_per_gas(max_base_fee).unwrap_or_default();
if 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)
}

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

use crate::state::pricing;
use crate::{
builder::BlockTemplate,
common::{
Expand All @@ -21,6 +22,7 @@ use crate::{
telemetry::ApiMetrics,
};

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

/// Possible commitment validation errors.
Expand Down Expand Up @@ -64,6 +66,9 @@ pub enum ValidationError {
/// The sender does not have enough balance to pay for the transaction.
#[error("Not enough balance to pay for value + maximum fee")]
InsufficientBalance,
/// Pricing calculation error.
#[error("Pricing calculation error: {0}")]
Pricing(#[from] pricing::PricingError),
/// There are too many EIP-4844 transactions in the target block.
#[error("Too many EIP-4844 transactions in target block")]
Eip4844Limit,
Expand Down Expand Up @@ -111,6 +116,7 @@ impl ValidationError {
Self::MaxPriorityFeePerGasTooHigh => "max_priority_fee_per_gas_too_high",
Self::MaxPriorityFeePerGasTooLow => "max_priority_fee_per_gas_too_low",
Self::InsufficientBalance => "insufficient_balance",
Self::Pricing(_) => "pricing",
Self::Eip4844Limit => "eip4844_limit",
Self::SlotTooLow(_) => "slot_too_low",
Self::MaxCommitmentsReachedForSlot(_, _) => "max_commitments_reached_for_slot",
Expand Down Expand Up @@ -166,6 +172,8 @@ pub struct ExecutionState<C> {
client: C,
/// Other values used for validation
validation_params: ValidationParams,
/// Pricing calculator for preconfirmations.
pricing: PreconfPricing,
}

/// Other values used for validation.
Expand Down Expand Up @@ -222,6 +230,7 @@ impl<C: StateFetcher> ExecutionState<C> {
kzg_settings: EnvKzgSettings::default(),
// TODO: add a way to configure these values from CLI
validation_params: ValidationParams::new(gas_limit),
pricing: PreconfPricing::new(gas_limit),
estensen marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down Expand Up @@ -311,8 +320,13 @@ impl<C: StateFetcher> ExecutionState<C> {
return Err(ValidationError::BaseFeeTooLow(max_basefee));
}

// Ensure max_priority_fee_per_gas is greater than or equal to min_priority_fee
if !req.validate_min_priority_fee(max_basefee, self.limits.min_priority_fee) {
// Ensure max_priority_fee_per_gas is greater than or equal to the calculated min_priority_fee
if !req.validate_min_priority_fee(
&self.pricing,
template_committed_gas,
self.limits.min_inclusion_profit,
max_basefee,
)? {
return Err(ValidationError::MaxPriorityFeePerGasTooLow);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should return the value of the minimum fee in this error, so it can be propagated to any consumers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok if I create an issue and follow up in a separate PR? Keen on getting this merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

Expand Down Expand Up @@ -829,7 +843,7 @@ mod tests {
state.update_head(None, slot).await?;

// Set the sender balance to just enough to pay for 1 transaction
let balance = U256::from_str("500000000000000").unwrap(); // leave just 0.0005 ETH
let balance = U256::from_str("600000000000000").unwrap(); // leave just 0.0005 ETH
let sender_account = client.get_account_state(sender, None).await.unwrap();
let balance_to_burn = sender_account.balance - balance;

Expand All @@ -848,7 +862,8 @@ mod tests {
let tx = default_test_transaction(*sender, Some(1));
let mut request = create_signed_inclusion_request(&[tx], sender_pk, 10).await?;

assert!(state.validate_request(&mut request).await.is_ok());
let validation = state.validate_request(&mut request).await;
assert!(validation.is_ok(), "Validation failed: {validation:?}");

let message = ConstraintsMessage::build(Default::default(), request.clone());
let signature = signer.sign_commit_boost_root(message.digest())?;
Expand All @@ -861,10 +876,13 @@ mod tests {

// this should fail because the balance is insufficient as we spent
// all of it on the previous preconfirmation
assert!(matches!(
state.validate_request(&mut request).await,
Err(ValidationError::InsufficientBalance)
));
let validation_result = state.validate_request(&mut request).await;

assert!(
matches!(validation_result, Err(ValidationError::InsufficientBalance)),
"Expected InsufficientBalance error, got {:?}",
validation_result
);

Ok(())
}
Expand Down Expand Up @@ -940,7 +958,7 @@ mod tests {
let anvil = launch_anvil();
let client = StateClient::new(anvil.endpoint_url());

let limits = LimitsOpts { min_priority_fee: 2 * GWEI_TO_WEI as u128, ..Default::default() };
let limits = LimitsOpts { ..Default::default() };

let mut state = ExecutionState::new(client.clone(), limits, DEFAULT_GAS_LIMIT).await?;

Expand All @@ -953,7 +971,7 @@ mod tests {

// Create a transaction with a max priority fee that is too low
let tx = default_test_transaction(*sender, None)
.with_max_priority_fee_per_gas(GWEI_TO_WEI as u128);
.with_max_priority_fee_per_gas(GWEI_TO_WEI as u128 / 2);

let mut request = create_signed_inclusion_request(&[tx], sender_pk, 10).await?;

Expand All @@ -978,7 +996,7 @@ mod tests {
let anvil = launch_anvil();
let client = StateClient::new(anvil.endpoint_url());

let limits = LimitsOpts { min_priority_fee: 2 * GWEI_TO_WEI as u128, ..Default::default() };
let limits = LimitsOpts { ..Default::default() };

let mut state = ExecutionState::new(client.clone(), limits, DEFAULT_GAS_LIMIT).await?;

Expand All @@ -996,7 +1014,7 @@ mod tests {

// Create a transaction with a gas price that is too low
let tx = default_test_transaction(*sender, None)
.with_gas_price(max_base_fee + GWEI_TO_WEI as u128);
.with_gas_price(max_base_fee + GWEI_TO_WEI as u128 / 2);

let mut request = create_signed_inclusion_request(&[tx], sender_pk, 10).await?;

Expand All @@ -1021,7 +1039,7 @@ mod tests {
let anvil = launch_anvil();
let client = StateClient::new(anvil.endpoint_url());

let limits = LimitsOpts { min_priority_fee: 2 * GWEI_TO_WEI as u128, ..Default::default() };
let limits = LimitsOpts { ..Default::default() };

let mut state = ExecutionState::new(client.clone(), limits, DEFAULT_GAS_LIMIT).await?;

Expand Down Expand Up @@ -1156,7 +1174,7 @@ mod tests {
let anvil = launch_anvil();
let client = StateClient::new(anvil.endpoint_url());

let limits = LimitsOpts { min_priority_fee: 1000000000, ..Default::default() };
let limits = LimitsOpts { min_inclusion_profit: 1_000_000_000, ..Default::default() };
let mut state = ExecutionState::new(client.clone(), limits, DEFAULT_GAS_LIMIT).await?;

let sender = anvil.addresses().first().unwrap();
Expand Down Expand Up @@ -1279,11 +1297,13 @@ mod tests {
.with_value(uint!(11_000_U256 * Uint::from(ETH_TO_WEI)));

let mut request = create_signed_inclusion_request(&[tx1, tx2, tx3], sender_pk, 10).await?;
let validation_result = state.validate_request(&mut request).await;

assert!(matches!(
state.validate_request(&mut request).await,
Err(ValidationError::InsufficientBalance)
));
assert!(
matches!(validation_result, Err(ValidationError::InsufficientBalance)),
"Expected InsufficientBalance error, got {:?}",
validation_result
);

Ok(())
}
Expand Down
Loading