From 111868391c95c05e74edece693964161dc282ed7 Mon Sep 17 00:00:00 2001 From: dancoombs Date: Tue, 2 Jul 2024 16:50:09 -0500 Subject: [PATCH] fix(builder): improve replacement underpriced handling --- crates/builder/src/bundle_sender.rs | 27 +++++---- crates/builder/src/transaction_tracker.rs | 70 +++++++++++++---------- 2 files changed, 58 insertions(+), 39 deletions(-) diff --git a/crates/builder/src/bundle_sender.rs b/crates/builder/src/bundle_sender.rs index e3e77a31..fe283fca 100644 --- a/crates/builder/src/bundle_sender.rs +++ b/crates/builder/src/bundle_sender.rs @@ -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!( @@ -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), )); } @@ -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, @@ -711,9 +711,17 @@ impl SenderMachineState { 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) { @@ -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 @@ -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), } diff --git a/crates/builder/src/transaction_tracker.rs b/crates/builder/src/transaction_tracker.rs index 582ab6f6..3055afbb 100644 --- a/crates/builder/src/transaction_tracker.rs +++ b/crates/builder/src/transaction_tracker.rs @@ -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`. @@ -124,7 +128,6 @@ where builder_index: u64, nonce: U256, transactions: Vec, - has_dropped: bool, has_abandoned: bool, attempt_count: u64, } @@ -163,7 +166,6 @@ where builder_index, nonce, transactions: vec![], - has_dropped: false, has_abandoned: false, attempt_count: 0, }) @@ -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(); @@ -248,7 +249,7 @@ where T: TransactionSender, { fn get_nonce_and_required_fees(&self) -> TransactionTrackerResult<(U256, Option)> { - let gas_fees = if self.has_dropped || self.has_abandoned { + let gas_fees = if self.has_abandoned { None } else { self.transactions.last().map(|tx| { @@ -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(); @@ -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)) @@ -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 } => { @@ -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 }), }) } @@ -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 for TransactionTrackerError { @@ -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 { @@ -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); @@ -756,7 +768,7 @@ mod tests { Box::pin(async { Ok(SentTxInfo { nonce: U256::from(0), - tx_hash: H256::zero(), + tx_hash: H256::random(), }) }) });