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
2 changes: 1 addition & 1 deletion bolt-sidecar/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
/// The version of the Bolt sidecar binary.
pub const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION");

/// Calculates the max_basefee `slot_diff` blocks in the future given a current basefee (in gwei).
/// Calculates the max_basefee `slot_diff` blocks in the future given a current basefee (in wei).
/// Returns None if an overflow would occur.
/// Cfr. https://github.com/flashbots/ethers-provider-flashbots-bundle/blob/7ddaf2c9d7662bef400151e0bfc89f5b13e72b4c/src/index.ts#L308
mempirate marked this conversation as resolved.
Show resolved Hide resolved
pub fn calculate_max_basefee(current: u128, block_diff: u64) -> Option<u128> {
Expand Down
22 changes: 20 additions & 2 deletions bolt-sidecar/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ pub const DEFAULT_RPC_PORT: u16 = 8000;
/// Default port for the Constraints proxy server.
pub const DEFAULT_CONSTRAINTS_PROXY_PORT: u16 = 18551;

// Default limit values
pub const DEFAULT_MAX_COMMITMENTS: usize = 128;
pub const DEFAULT_MAX_COMMITTED_GAS: u64 = 10_000_000;
pub const DEFAULT_MIN_PRIORITY_FEE: u128 = 2;

/// Command-line options for the Bolt sidecar
#[derive(Parser, Debug)]
pub struct Opts {
Expand Down Expand Up @@ -55,6 +60,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<u128>>,
/// 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 +166,19 @@ 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 in gwei
pub min_priority_fee: NonZero<u128>,
}

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"),
max_commitments_per_slot: NonZero::new(DEFAULT_MAX_COMMITMENTS)
.expect("Valid non-zero"),
max_committed_gas_per_slot: NonZero::new(DEFAULT_MAX_COMMITTED_GAS)
.expect("Valid non-zero"),
min_priority_fee: NonZero::new(DEFAULT_MIN_PRIORITY_FEE).expect("Valid non-zero"),
}
}
}
Expand Down Expand Up @@ -195,6 +209,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
37 changes: 31 additions & 6 deletions bolt-sidecar/src/primitives/commitment.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use serde::{de, Deserialize, Deserializer, Serialize};
use std::str::FromStr;

use alloy::primitives::{keccak256, Address, Signature, B256};
use alloy::{
consensus::constants::GWEI_TO_WEI,
primitives::{keccak256, Address, Signature, B256},
};

use crate::{
crypto::SignerECDSA,
Expand Down Expand Up @@ -143,12 +146,34 @@ impl InclusionRequest {
true
}

pub fn validate_priority_fee(&self) -> bool {
/// Validates the priority fee against the max fee per gas.
/// Returns true if the fee is less than or equal to the max fee per gas, false otherwise.
pub fn validate_max_priority_fee(&self) -> bool {
for tx in &self.txs {
let gas_price = tx.as_legacy().map(|tx| tx.gas_price).unwrap_or(0);
let max_priority_fee = tx.max_priority_fee_per_gas().unwrap_or(0);
if max_priority_fee + gas_price > tx.max_fee_per_gas() {
return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw this was already there though

Copy link
Collaborator

Choose a reason for hiding this comment

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

oops I removed it, yeah I see it was already present but I still am not sure about the usage of gas_price here

Copy link
Collaborator

Choose a reason for hiding this comment

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

The above code (that was already there) doesn't make sense to me, trying to understand it more

}

true
}

/// Validates the priority fee against a minimum priority fee.
/// Returns true if the fee is greater than or equal to the set min 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 tx
.max_priority_fee_per_gas()
.is_some_and(|max_priority_fee| max_priority_fee > tx.max_fee_per_gas())
{
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;
Copy link
Contributor

@thedevbirb thedevbirb Oct 1, 2024

Choose a reason for hiding this comment

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

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_price or max_priority_fee by checking the type of the transaction?
  • Can you add some comments as well with steps and motivation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, addressed, thanks

}
}
Expand Down
97 changes: 92 additions & 5 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,7 +280,7 @@ 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);
}

Expand All @@ -288,6 +291,11 @@ impl<C: StateFetcher> ExecutionState<C> {
let max_basefee = calculate_max_basefee(self.basefee, slot_diff)
.ok_or(ValidationError::MaxBaseFeeCalcOverflow)?;

// 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.get()) {
return Err(ValidationError::MaxPriorityFeePerGasTooLow);
}

debug!(%slot_diff, basefee = self.basefee, %max_basefee, "Validating basefee");

// Validate the base fee
Expand Down Expand Up @@ -523,14 +531,15 @@ mod tests {
use std::{num::NonZero, str::FromStr, time::Duration};

use alloy::{
consensus::constants::ETH_TO_WEI,
consensus::constants::{ETH_TO_WEI, LEGACY_TX_TYPE_ID},
eips::eip2718::Encodable2718,
network::EthereumWallet,
primitives::{uint, Uint},
providers::{network::TransactionBuilder, Provider, ProviderBuilder},
signers::local::PrivateKeySigner,
};
use fetcher::{StateClient, StateFetcher};
use tracing::info;

use crate::{
crypto::{bls::Signer, SignableBLS, SignerBLS},
Expand Down Expand Up @@ -785,9 +794,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 +820,82 @@ mod tests {
Ok(())
}

#[tokio::test]
async fn test_invalid_inclusion_request_legacy_tx() -> eyre::Result<()> {
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 legacy transaction with a max priority fee that is too low
let tx = default_test_transaction(*sender, None)
.transaction_type(LEGACY_TX_TYPE_ID)
.with_gas_price(10000000000); // Value is in wei = 10 gwei

info!(?tx, "Transaction");

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

// The request fails because the max base fee is also taken into account which decreases the
// final sum less than the min priority fee
assert!(matches!(
state.validate_request(&mut request).await,
Err(ValidationError::MaxPriorityFeePerGasTooLow)
));

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 +1002,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