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(sidecar): min priority fee #246

Merged
merged 12 commits into from
Oct 2, 2024
11 changes: 11 additions & 0 deletions bolt-sidecar/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ pub struct Opts {
/// Max committed gas per slot
#[clap(long, env = "BOLT_SIDECAR_MAX_COMMITTED_GAS")]
pub(super) max_committed_gas: Option<NonZero<u64>>,
/// Min priority fee to accept for a commitment
mempirate marked this conversation as resolved.
Show resolved Hide resolved
#[clap(long, env = "BOLT_SIDECAR_MIN_PRIORITY_FEE")]
pub(super) min_priority_fee: Option<NonZero<u64>>,
Copy link
Contributor

@thedevbirb thedevbirb Sep 30, 2024

Choose a reason for hiding this comment

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

Isn't u128 a better type to express priority fees? I'm not sure tho, could you take a look?

/// Validator indexes of connected validators that the sidecar
/// should accept commitments on behalf of. Accepted values:
/// - a comma-separated list of indexes (e.g. "1,2,3,4")
Expand Down Expand Up @@ -158,13 +161,17 @@ pub struct Limits {
/// Maximum number of commitments to accept per block
pub max_commitments_per_slot: NonZero<usize>,
pub max_committed_gas_per_slot: NonZero<u64>,

/// Minimum priority fee to accept for a commitment
pub min_priority_fee: NonZero<u64>,
}

impl Default for Limits {
fn default() -> Self {
Self {
max_commitments_per_slot: NonZero::new(128).expect("Valid non-zero"),
max_committed_gas_per_slot: NonZero::new(10_000_000).expect("Valid non-zero"),
min_priority_fee: NonZero::new(2).expect("Valid non-zero"),
mempirate marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down Expand Up @@ -195,6 +202,10 @@ impl TryFrom<Opts> for Config {
config.limits.max_committed_gas_per_slot = max_committed_gas;
}

if let Some(min_priority_fee) = opts.min_priority_fee {
config.limits.min_priority_fee = min_priority_fee;
}

if let Some(commit_boost_url) = &opts.signing.commit_boost_url {
if let Ok(url) = Url::parse(commit_boost_url) {
if let Ok(socket_addrs) = url.socket_addrs(|| None) {
Expand Down
15 changes: 14 additions & 1 deletion bolt-sidecar/src/primitives/commitment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ impl InclusionRequest {
true
}

pub fn validate_priority_fee(&self) -> bool {
pub fn validate_max_priority_fee(&self) -> bool {
for tx in &self.txs {
if tx
.max_priority_fee_per_gas()
Expand All @@ -156,6 +156,19 @@ impl InclusionRequest {
true
}

pub fn validate_min_priority_fee(&self, min_priority_fee: u128) -> bool {
for tx in &self.txs {
if tx
.max_priority_fee_per_gas()
.is_some_and(|max_priority_fee| max_priority_fee < 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.

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.

{
return false;
}
}

true
}

/// Returns the total gas limit of all transactions in this request.
pub fn gas_limit(&self) -> u64 {
self.txs.iter().map(|tx| tx.gas_limit()).sum()
Expand Down
53 changes: 49 additions & 4 deletions bolt-sidecar/src/state/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ use super::fetcher::StateFetcher;

/// Possible commitment validation errors.
///
/// NOTE: unfortuntately it cannot implement `Clone` due to `BlobTransactionValidationError`
/// not implementing it
/// NOTE: `Clone` not implementable due to `BlobTransactionValidationError`
#[derive(Debug, Error)]
pub enum ValidationError {
/// The transaction fee is too low to cover the maximum base fee.
Expand Down Expand Up @@ -55,6 +54,9 @@ pub enum ValidationError {
/// Max priority fee per gas is greater than max fee per gas.
#[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,
/// 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 @@ -103,6 +105,7 @@ impl ValidationError {
ValidationError::GasLimitTooHigh => "gas_limit_too_high",
ValidationError::TransactionSizeTooHigh => "transaction_size_too_high",
ValidationError::MaxPriorityFeePerGasTooHigh => "max_priority_fee_per_gas_too_high",
ValidationError::MaxPriorityFeePerGasTooLow => "max_priority_fee_per_gas_too_low",
ValidationError::InsufficientBalance => "insufficient_balance",
ValidationError::Eip4844Limit => "eip4844_limit",
ValidationError::SlotTooLow(_) => "slot_too_low",
Expand Down Expand Up @@ -277,10 +280,15 @@ impl<C: StateFetcher> ExecutionState<C> {
}

// Ensure max_priority_fee_per_gas is less than max_fee_per_gas
if !req.validate_priority_fee() {
if !req.validate_max_priority_fee() {
return Err(ValidationError::MaxPriorityFeePerGasTooHigh);
}

// Ensure max_priority_fee_per_gas is greater than or equal to min_priority_fee
if !req.validate_min_priority_fee(self.limits.min_priority_fee.get() as u128) {
return Err(ValidationError::MaxPriorityFeePerGasTooLow);
}

// Check if the max_fee_per_gas would cover the maximum possible basefee.
let slot_diff = target_slot.saturating_sub(self.slot);

Expand Down Expand Up @@ -785,9 +793,10 @@ mod tests {
let anvil = launch_anvil();
let client = StateClient::new(anvil.endpoint_url());

let limits: Limits = Limits {
let limits = Limits {
max_commitments_per_slot: NonZero::new(10).unwrap(),
max_committed_gas_per_slot: NonZero::new(5_000_000).unwrap(),
min_priority_fee: NonZero::new(2).unwrap(),
};
let mut state = ExecutionState::new(client.clone(), limits).await?;

Expand All @@ -810,6 +819,41 @@ mod tests {
Ok(())
}

#[tokio::test]
async fn test_invalid_inclusion_request_min_priority_fee() -> eyre::Result<()> {
mempirate marked this conversation as resolved.
Show resolved Hide resolved
let _ = tracing_subscriber::fmt::try_init();

let anvil = launch_anvil();
let client = StateClient::new(anvil.endpoint_url());

let limits = Limits {
max_commitments_per_slot: NonZero::new(10).unwrap(),
max_committed_gas_per_slot: NonZero::new(5_000_000).unwrap(),
min_priority_fee: NonZero::new(10).unwrap(),
};

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

let sender = anvil.addresses().first().unwrap();
let sender_pk = anvil.keys().first().unwrap();

// initialize the state by updating the head once
let slot = client.get_head().await?;
state.update_head(None, slot).await?;

// 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(5);

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

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

Ok(())
}

#[tokio::test]
async fn test_invalidate_inclusion_request() -> eyre::Result<()> {
let _ = tracing_subscriber::fmt::try_init();
Expand Down Expand Up @@ -916,6 +960,7 @@ mod tests {
let limits: Limits = Limits {
max_commitments_per_slot: NonZero::new(10).unwrap(),
max_committed_gas_per_slot: NonZero::new(5_000_000).unwrap(),
min_priority_fee: NonZero::new(2).unwrap(),
};
let mut state = ExecutionState::new(client.clone(), limits).await?;

Expand Down
Loading