Skip to content

Commit

Permalink
fix(builder): improve replacement underpriced handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@dancoombs
dancoombs committed Jul 8, 2024
1 parent f7aa694 commit 1118683
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 39 deletions.
27 changes: 17 additions & 10 deletions crates/builder/src/bundle_sender.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ where
state.update(InnerState::Cancelling(inner.to_cancelling()));
} else {
info!("No operations available, but last replacement underpriced, starting over and waiting for next trigger. Round: {}. Since block {}. Current block {}", underpriced_info.rounds, underpriced_info.since_block, block_number);
state.update_and_reset(InnerState::Building(inner.underpriced_round()));
state.update_and_abandon(InnerState::Building(inner.underpriced_round()));
}
} else if inner.fee_increase_count > 0 {
warn!(
Expand All @@ -285,7 +285,8 @@ where
}
Ok(SendBundleAttemptResult::ReplacementUnderpriced) => {
info!("Replacement transaction underpriced, marking as underpriced. Num fee increases {:?}", inner.fee_increase_count);
state.update(InnerState::Building(
// unabandon to allow fee estimation to consider any submitted transactions, wait for next trigger
state.update_and_unabandon(InnerState::Building(
inner.replacement_underpriced(block_number),
));
}
Expand Down Expand Up @@ -338,17 +339,16 @@ where
state.complete(send_bundle_result);
}
TrackerUpdate::LatestTxDropped { nonce } => {
// try again, don't wait for trigger, re-estimate fees
info!("Latest transaction dropped, starting new bundle attempt");
self.emit(BuilderEvent::latest_transaction_dropped(
self.builder_index,
nonce.low_u64(),
));
self.metrics.increment_bundle_txns_dropped();
state.reset();
// try again, increasing fees
state.update(InnerState::Building(inner.to_building()));
}
TrackerUpdate::NonceUsedForOtherTx { nonce } => {
// try again, don't wait for trigger, re-estimate fees
info!("Nonce used externally, starting new bundle attempt");
self.emit(BuilderEvent::nonce_used_for_other_transaction(
self.builder_index,
Expand Down Expand Up @@ -711,9 +711,17 @@ impl<T: TransactionTracker, TRIG: Trigger> SenderMachineState<T, TRIG> {
self.inner = InnerState::Building(building_state);
}

fn update_and_reset(&mut self, inner: InnerState) {
fn update_and_abandon(&mut self, inner: InnerState) {
self.update(inner);
self.requires_reset = true;
self.transaction_tracker.abandon();
}

// update the state and unabandoned the transaction tracker
// this will cause any "abandoned" transactions to be considered during the next
// fee estimation.
fn update_and_unabandon(&mut self, next_state: InnerState) {
self.transaction_tracker.unabandon();
self.inner = next_state;
}

fn abandon(&mut self) {
Expand Down Expand Up @@ -825,8 +833,7 @@ impl BuildingState {

// Mark a replacement as underpriced
//
// The next state will NOT wait for a trigger. Use this when fees should be increased and a new bundler
// should be attempted immediately.
// The next state will wait for a trigger to reduce bundle building loops
fn replacement_underpriced(self, block_number: u64) -> Self {
let ui = if let Some(underpriced_info) = self.underpriced_info {
underpriced_info
Expand All @@ -838,7 +845,7 @@ impl BuildingState {
};

BuildingState {
wait_for_trigger: false,
wait_for_trigger: true,
fee_increase_count: self.fee_increase_count + 1,
underpriced_info: Some(ui),
}
Expand Down
70 changes: 41 additions & 29 deletions crates/builder/src/transaction_tracker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,12 @@ pub(crate) trait TransactionTracker: Send + Sync + 'static {
/// Resets the tracker to its initial state
async fn reset(&mut self);

/// Abandons the current transaction
/// Abandons the current transaction.
/// The current transaction will still be tracked, but will no longer be considered during fee estimation
fn abandon(&mut self);

/// Un-abandons the current transaction
fn unabandon(&mut self);
}

/// Errors that can occur while using a `TransactionTracker`.
Expand Down Expand Up @@ -124,7 +128,6 @@ where
builder_index: u64,
nonce: U256,
transactions: Vec<PendingTransaction>,
has_dropped: bool,
has_abandoned: bool,
attempt_count: u64,
}
Expand Down Expand Up @@ -163,7 +166,6 @@ where
builder_index,
nonce,
transactions: vec![],
has_dropped: false,
has_abandoned: false,
attempt_count: 0,
})
Expand All @@ -172,7 +174,6 @@ where
fn set_nonce_and_clear_state(&mut self, nonce: U256) {
self.nonce = nonce;
self.transactions.clear();
self.has_dropped = false;
self.attempt_count = 0;
self.has_abandoned = false;
self.update_metrics();
Expand Down Expand Up @@ -248,7 +249,7 @@ where
T: TransactionSender,
{
fn get_nonce_and_required_fees(&self) -> TransactionTrackerResult<(U256, Option<GasFees>)> {
let gas_fees = if self.has_dropped || self.has_abandoned {
let gas_fees = if self.has_abandoned {
None
} else {
self.transactions.last().map(|tx| {
Expand Down Expand Up @@ -285,21 +286,29 @@ where
gas_fees,
attempt_number: self.attempt_count,
});
self.has_dropped = false;
self.has_abandoned = false;
self.attempt_count += 1;
self.update_metrics();
Ok(sent_tx.tx_hash)
}
Err(e) if matches!(e, TxSenderError::ReplacementUnderpriced) => {
// Only can get into this state if there is an unknown pending transaction causing replacement
// underpriced errors, or if the last transaction was abandoned.
info!("Replacement underpriced: nonce: {:?}", self.nonce);
// still store this as a pending transaction so that we can continue to increase fees.
self.transactions.push(PendingTransaction {
tx_hash: H256::zero(),
gas_fees,
attempt_number: self.attempt_count,
});
self.has_dropped = false;

// Store this transaction as pending if last is empty or if it has higher gas fees than last
// so that we can continue to increase fees.
if self.transactions.last().map_or(true, |t| {
gas_fees.max_fee_per_gas > t.gas_fees.max_fee_per_gas
&& gas_fees.max_priority_fee_per_gas > t.gas_fees.max_priority_fee_per_gas
}) {
self.transactions.push(PendingTransaction {
tx_hash: H256::zero(),
gas_fees,
attempt_number: self.attempt_count,
});
};

self.has_abandoned = false;
self.attempt_count += 1;
self.update_metrics();
Expand Down Expand Up @@ -354,7 +363,6 @@ where
attempt_number: self.attempt_count,
});

self.has_dropped = false;
self.attempt_count += 1;
self.update_metrics();
Ok(Some(cancel_info.tx_hash))
Expand Down Expand Up @@ -396,22 +404,23 @@ where
self.set_nonce_and_clear_state(external_nonce);
return Ok(Some(out));
}
// The nonce has not changed. Check to see if the latest transaction has
// dropped.
if self.has_dropped {
// has_dropped being true means that no new transactions have been
// added since the last time we checked, hence no update.
return Ok(None);
}

let Some(&last_tx) = self.transactions.last() else {
// If there are no pending transactions, there's no update either.
return Ok(None);
};

if last_tx.tx_hash == H256::zero() {
// If the last transaction was a replacement that failed to send, we
// don't need to check for updates.
return Ok(None);
}

let status = self
.sender
.get_transaction_status(last_tx.tx_hash)
.await
.context("tracker should check for dropped transactions")?;
.context("tracker should check for transaction status")?;
Ok(match status {
TxStatus::Pending => None,
TxStatus::Mined { block_number } => {
Expand All @@ -429,10 +438,7 @@ where
gas_price,
})
}
TxStatus::Dropped => {
self.has_dropped = true;
Some(TrackerUpdate::LatestTxDropped { nonce: self.nonce })
}
TxStatus::Dropped => Some(TrackerUpdate::LatestTxDropped { nonce: self.nonce }),
})
}

Expand All @@ -446,6 +452,10 @@ where
self.attempt_count = 0;
// remember the transaction in case we need to cancel it
}

fn unabandon(&mut self) {
self.has_abandoned = false;
}
}

impl From<TxSenderError> for TransactionTrackerError {
Expand Down Expand Up @@ -567,13 +577,13 @@ mod tests {
}

#[tokio::test]
async fn test_nonce_and_fees_dropped() {
async fn test_nonce_and_fees_abandoned() {
let (mut sender, mut provider) = create_base_config();
sender.expect_address().return_const(Address::zero());

sender
.expect_get_transaction_status()
.returning(move |_a| Box::pin(async { Ok(TxStatus::Dropped) }));
.returning(move |_a| Box::pin(async { Ok(TxStatus::Pending) }));

sender.expect_send_transaction().returning(move |_a, _b| {
Box::pin(async {
Expand All @@ -600,6 +610,8 @@ mod tests {
let _sent = tracker.send_transaction(tx.into(), &exp).await;
let _tracker_update = tracker.check_for_update().await.unwrap();

tracker.abandon();

let nonce_and_fees = tracker.get_nonce_and_required_fees().unwrap();

assert_eq!((U256::from(0), None), nonce_and_fees);
Expand Down Expand Up @@ -756,7 +768,7 @@ mod tests {
Box::pin(async {
Ok(SentTxInfo {
nonce: U256::from(0),
tx_hash: H256::zero(),
tx_hash: H256::random(),
})
})
});
Expand Down

0 comments on commit 1118683

Please sign in to comment.