Skip to content

Commit

Permalink
fix(builder): handle more tx sender errors (#918)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dancoombs
dancoombs authored Nov 25, 2024
1 parent 6817ed7 commit bb2dbca
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 78 deletions.
63 changes: 52 additions & 11 deletions crates/builder/src/bundle_sender.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,14 @@ enum SendBundleAttemptResult {
NoOperationsAfterFeeFilter,
// There were no operations after the bundle was simulated
NoOperationsAfterSimulation,
// Underpriced
Underpriced,
// Replacement Underpriced
ReplacementUnderpriced,
// Condition not met
ConditionNotMet,
// Rejected
Rejected,
// Nonce too low
NonceTooLow,
}
Expand Down Expand Up @@ -296,19 +300,33 @@ where
info!("Nonce too low, starting new bundle attempt");
state.reset();
}
Ok(SendBundleAttemptResult::Underpriced) => {
info!(
"Bundle underpriced, marking as underpriced. Num fee increases {:?}",
inner.fee_increase_count
);
state.update(InnerState::Building(inner.underpriced(block_number)));
}
Ok(SendBundleAttemptResult::ReplacementUnderpriced) => {
info!("Replacement transaction underpriced, marking as underpriced. Num fee increases {:?}", inner.fee_increase_count);
// unabandon to allow fee estimation to consider any submitted transactions, wait for next trigger
state.transaction_tracker.unabandon();
state.update(InnerState::Building(
inner.replacement_underpriced(block_number),
));
state.update(InnerState::Building(inner.underpriced(block_number)));
}
Ok(SendBundleAttemptResult::ConditionNotMet) => {
info!("Condition not met, notifying proposer and starting new bundle attempt");
self.proposer.notify_condition_not_met();
state.update(InnerState::Building(inner.retry()));
}
Ok(SendBundleAttemptResult::Rejected) => {
// Bundle was rejected, try with a higher price
// May want to consider a simple retry instead of increasing fees, but this should be rare
info!(
"Bundle rejected, assuming underpriced. Num fee increases {:?}",
inner.fee_increase_count
);
state.update(InnerState::Building(inner.underpriced(block_number)));
}
Err(error) => {
error!("Bundle send error {error:?}");
self.metrics.bundle_txns_failed.increment(1);
Expand Down Expand Up @@ -418,8 +436,10 @@ where
self.metrics.soft_cancellations.increment(1);
state.reset();
}
Err(TransactionTrackerError::ReplacementUnderpriced) => {
info!("Replacement transaction underpriced during cancellation, trying again");
Err(TransactionTrackerError::Rejected)
| Err(TransactionTrackerError::Underpriced)
| Err(TransactionTrackerError::ReplacementUnderpriced) => {
info!("Transaction underpriced/rejected during cancellation, trying again. {cancel_res:?}");
if inner.fee_increase_count >= self.settings.max_cancellation_fee_increases {
// abandon the cancellation
warn!("Abandoning cancellation after max fee increases {}, starting new bundle attempt", inner.fee_increase_count);
Expand All @@ -439,7 +459,14 @@ where
info!("Nonce too low during cancellation, starting new bundle attempt");
state.reset();
}
Err(e) => {
Err(TransactionTrackerError::ConditionNotMet) => {
error!(
"Unexpected condition not met during cancellation, starting new bundle attempt"
);
self.metrics.cancellation_txns_failed.increment(1);
state.reset();
}
Err(TransactionTrackerError::Other(e)) => {
error!("Failed to cancel transaction, moving back to building state: {e:#?}");
self.metrics.cancellation_txns_failed.increment(1);
state.reset();
Expand Down Expand Up @@ -580,6 +607,11 @@ where
warn!("Bundle attempt nonce too low");
Ok(SendBundleAttemptResult::NonceTooLow)
}
Err(TransactionTrackerError::Underpriced) => {
self.metrics.bundle_txn_underpriced.increment(1);
warn!("Bundle attempt underpriced");
Ok(SendBundleAttemptResult::Underpriced)
}
Err(TransactionTrackerError::ReplacementUnderpriced) => {
self.metrics.bundle_replacement_underpriced.increment(1);
warn!("Bundle attempt replacement transaction underpriced");
Expand All @@ -590,9 +622,14 @@ where
warn!("Bundle attempt condition not met");
Ok(SendBundleAttemptResult::ConditionNotMet)
}
Err(e) => {
Err(TransactionTrackerError::Rejected) => {
self.metrics.bundle_txn_rejected.increment(1);
warn!("Bundle attempt rejected");
Ok(SendBundleAttemptResult::Rejected)
}
Err(TransactionTrackerError::Other(e)) => {
error!("Failed to send bundle with unexpected error: {e:?}");
Err(e.into())
Err(e)
}
}
}
Expand Down Expand Up @@ -882,10 +919,10 @@ impl BuildingState {
self
}

// Mark a replacement as underpriced
// Mark as underpriced
//
// The next state will wait for a trigger to reduce bundle building loops
fn replacement_underpriced(self, block_number: u64) -> Self {
fn underpriced(self, block_number: u64) -> Self {
let ui = if let Some(underpriced_info) = self.underpriced_info {
underpriced_info
} else {
Expand Down Expand Up @@ -1179,12 +1216,16 @@ struct BuilderMetric {
bundle_txns_nonce_used: Counter,
#[metric(describe = "the count of bundle transactions fee increase events.")]
bundle_txn_fee_increases: Counter,
#[metric(describe = "the count of bundle transactions underprice replaced events.")]
#[metric(describe = "the count of bundle transactions underpriced events.")]
bundle_txn_underpriced: Counter,
#[metric(describe = "the count of bundle transactions underpriced replacement events.")]
bundle_replacement_underpriced: Counter,
#[metric(describe = "the count of bundle transactions nonce too low events.")]
bundle_txn_nonce_too_low: Counter,
#[metric(describe = "the count of bundle transactions condition not met events.")]
bundle_txn_condition_not_met: Counter,
#[metric(describe = "the count of bundle transactions rejected.")]
bundle_txn_rejected: Counter,
#[metric(describe = "the count of cancellation bundle transactions sent events.")]
cancellation_txns_sent: Counter,
#[metric(describe = "the count of cancellation bundle transactions mined events.")]
Expand Down
13 changes: 5 additions & 8 deletions crates/builder/src/sender/bloxroute.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,12 @@ struct BloxrouteResponse {

impl From<jsonrpsee::core::ClientError> for TxSenderError {
fn from(value: jsonrpsee::core::ClientError) -> Self {
match &value {
jsonrpsee::core::ClientError::Call(e) => {
if super::is_replacement_underpriced(e.message()) {
TxSenderError::ReplacementUnderpriced
} else {
TxSenderError::Other(value.into())
}
if let jsonrpsee::core::ClientError::Call(e) = &value {
if let Some(e) = super::parse_known_call_execution_failed(e.message()) {
return e;
}
_ => TxSenderError::Other(value.into()),
}

TxSenderError::Other(value.into())
}
}
71 changes: 50 additions & 21 deletions crates/builder/src/sender/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ pub(crate) enum TxStatus {
/// Errors from transaction senders
#[derive(Debug, thiserror::Error)]
pub(crate) enum TxSenderError {
/// Transaction was underpriced and dropped
#[error("transaction underpriced")]
Underpriced,
/// Replacement transaction was underpriced
#[error("replacement transaction underpriced")]
ReplacementUnderpriced,
Expand All @@ -61,6 +64,12 @@ pub(crate) enum TxSenderError {
/// Conditional value not met
#[error("storage slot value condition not met")]
ConditionNotMet,
/// Transaction was rejected
///
/// This is a catch-all for when a transaction is rejected for any reason
/// that can be solved with a retry.
#[error("transaction rejected")]
Rejected,
/// Soft cancellation failed
#[error("soft cancel failed")]
SoftCancelFailed,
Expand Down Expand Up @@ -230,33 +239,53 @@ impl From<ProviderError> for TxSenderError {
match &value {
ProviderError::RPC(e) => {
if let Some(e) = e.as_error_resp() {
if is_replacement_underpriced(&e.message) {
return TxSenderError::ReplacementUnderpriced;
// geth, erigon, reth
} else if e.message.contains("nonce too low") {
return TxSenderError::NonceTooLow;
// Arbitrum conditional sender error message
// TODO push them to use a specific error code and to return the specific slot that is not met.
} else if e
.message
.to_lowercase()
.contains("storage slot value condition not met")
{
return TxSenderError::ConditionNotMet;
// Client impls use different error codes, just match on the message
if let Some(e) = parse_known_call_execution_failed(&e.message) {
e
} else {
TxSenderError::Other(value.into())
}
} else {
TxSenderError::Other(value.into())
}
TxSenderError::Other(value.into())
}
_ => TxSenderError::Other(value.into()),
}
}
}

fn is_replacement_underpriced(e: &str) -> bool {
// geth
e.to_lowercase().contains("replacement transaction underpriced") ||
// erigon
e.to_lowercase().contains("could not replace existing tx") ||
// reth
e.to_lowercase().contains("insufficient gas price to replace existing transaction")
fn parse_known_call_execution_failed(e: &str) -> Option<TxSenderError> {
match &e.to_lowercase() {
// geth
x if x.contains("transaction underpriced") => Some(TxSenderError::Underpriced),
// erigon
x if x.contains("underpriced") => Some(TxSenderError::Underpriced),
// reth
x if x.contains("transaction discarded outright due to pool size constraints") => {
Some(TxSenderError::Underpriced)
}
// geth. Reth & erigon don't have similar
x if x.contains("future transaction tries to replace pending") => {
Some(TxSenderError::Rejected)
}
// geth
x if x.contains("replacement transaction underpriced") => {
Some(TxSenderError::ReplacementUnderpriced)
}
// erigon
x if x.contains("could not replace existing tx") => {
Some(TxSenderError::ReplacementUnderpriced)
}
// reth
x if x.contains("insufficient gas price to replace existing transaction") => {
Some(TxSenderError::ReplacementUnderpriced)
}
// geth, erigon, reth
x if x.contains("nonce too low") => Some(TxSenderError::NonceTooLow),
// Arbitrum conditional sender error message
x if x.contains("storage slot value condition not met") => {
Some(TxSenderError::ConditionNotMet)
}
_ => None,
}
}
44 changes: 6 additions & 38 deletions crates/builder/src/transaction_tracker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,14 @@ pub(crate) trait TransactionTracker: Send + Sync {
pub(crate) enum TransactionTrackerError {
#[error("nonce too low")]
NonceTooLow,
#[error("transaction underpriced")]
Underpriced,
#[error("replacement transaction underpriced")]
ReplacementUnderpriced,
#[error("storage slot value condition not met")]
ConditionNotMet,
#[error("rejected")]
Rejected,
/// All other errors
#[error(transparent)]
Other(#[from] anyhow::Error),
Expand Down Expand Up @@ -507,10 +511,12 @@ impl From<TxSenderError> for TransactionTrackerError {
fn from(value: TxSenderError) -> Self {
match value {
TxSenderError::NonceTooLow => TransactionTrackerError::NonceTooLow,
TxSenderError::Underpriced => TransactionTrackerError::Underpriced,
TxSenderError::ReplacementUnderpriced => {
TransactionTrackerError::ReplacementUnderpriced
}
TxSenderError::ConditionNotMet => TransactionTrackerError::ConditionNotMet,
TxSenderError::Rejected => TransactionTrackerError::Rejected,
TxSenderError::SoftCancelFailed => {
TransactionTrackerError::Other(anyhow::anyhow!("soft cancel failed"))
}
Expand Down Expand Up @@ -728,44 +734,6 @@ mod tests {
tracker.send_transaction(tx, &exp).await.unwrap();
}

// TODO(#295): fix dropped status
// #[tokio::test]
// async fn test_wait_for_update_dropped() {
// 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) }));

// sender.expect_send_transaction().returning(move |_a, _b| {
// Box::pin(async {
// Ok(SentTxInfo {
// nonce: U256::ZERO,
// tx_hash: B256::zero(),
// })
// })
// });

// provider
// .expect_get_transaction_count()
// .returning(move |_a| Ok(U256::ZERO));

// provider.expect_get_block_number().returning(move || Ok(1));

// let tracker = create_tracker(sender, provider).await;

// let tx = Eip1559TransactionRequest::new().nonce(0);
// let exp = ExpectedStorage::default();
// let _sent_transaction = tracker.send_transaction(tx.into(), &exp).await.unwrap();
// let tracker_update = tracker.wait_for_update().await.unwrap();

// assert!(matches!(
// tracker_update,
// TrackerUpdate::LatestTxDropped { .. }
// ));
// }

#[tokio::test]
async fn test_check_for_update_nonce_used() {
let (mut sender, mut provider) = create_base_config();
Expand Down

0 comments on commit bb2dbca

Please sign in to comment.