From bb2dbcac24810efa2ebfbd8893eb8d804084b2f8 Mon Sep 17 00:00:00 2001 From: Dan Coombs Date: Mon, 25 Nov 2024 11:28:16 -0600 Subject: [PATCH] fix(builder): handle more tx sender errors (#918) --- crates/builder/src/bundle_sender.rs | 63 ++++++++++++++++---- crates/builder/src/sender/bloxroute.rs | 13 ++--- crates/builder/src/sender/mod.rs | 71 ++++++++++++++++------- crates/builder/src/transaction_tracker.rs | 44 ++------------ 4 files changed, 113 insertions(+), 78 deletions(-) diff --git a/crates/builder/src/bundle_sender.rs b/crates/builder/src/bundle_sender.rs index ce4700411..2e589d14e 100644 --- a/crates/builder/src/bundle_sender.rs +++ b/crates/builder/src/bundle_sender.rs @@ -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, } @@ -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); @@ -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); @@ -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(); @@ -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"); @@ -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) } } } @@ -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 { @@ -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.")] diff --git a/crates/builder/src/sender/bloxroute.rs b/crates/builder/src/sender/bloxroute.rs index 271a2f890..4e9e7ac37 100644 --- a/crates/builder/src/sender/bloxroute.rs +++ b/crates/builder/src/sender/bloxroute.rs @@ -156,15 +156,12 @@ struct BloxrouteResponse { impl From 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()) } } diff --git a/crates/builder/src/sender/mod.rs b/crates/builder/src/sender/mod.rs index 41c53328b..a2bb432e0 100644 --- a/crates/builder/src/sender/mod.rs +++ b/crates/builder/src/sender/mod.rs @@ -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, @@ -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, @@ -230,33 +239,53 @@ impl From 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 { + 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, + } } diff --git a/crates/builder/src/transaction_tracker.rs b/crates/builder/src/transaction_tracker.rs index 012023750..65430ba07 100644 --- a/crates/builder/src/transaction_tracker.rs +++ b/crates/builder/src/transaction_tracker.rs @@ -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), @@ -507,10 +511,12 @@ impl From 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")) } @@ -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();