From efb3850bfac471814e7fbd69cfd58f2a60b6434b Mon Sep 17 00:00:00 2001 From: "Sam H. Smith" Date: Thu, 4 Jul 2024 08:44:09 +0200 Subject: [PATCH] [fix] Smarter transaction expire logic Signed-off-by: Sam H. Smith --- core/src/queue.rs | 24 +++++++++++++----------- core/src/sumeragi/main_loop.rs | 5 +++-- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/core/src/queue.rs b/core/src/queue.rs index d3ac864685b..2c791e315b9 100644 --- a/core/src/queue.rs +++ b/core/src/queue.rs @@ -106,12 +106,12 @@ impl Queue { } } - fn is_pending(&self, tx: &AcceptedTransaction, state_view: &StateView) -> bool { - !self.is_expired(tx) && !tx.is_in_blockchain(state_view) + fn is_pending(&self, tx: &AcceptedTransaction, state_view: &StateView, time_padding: Duration) -> bool { + !self.is_expired(tx, time_padding) && !tx.is_in_blockchain(state_view) } /// Checks if the transaction is waiting longer than its TTL or than the TTL from [`Config`]. - pub fn is_expired(&self, tx: &AcceptedTransaction) -> bool { + pub fn is_expired(&self, tx: &AcceptedTransaction, time_padding: Duration) -> bool { let tx_creation_time = tx.as_ref().creation_time(); let time_limit = tx.as_ref().time_to_live().map_or_else( @@ -120,7 +120,7 @@ impl Queue { ); let curr_time = self.time_source.get_unix_time(); - curr_time.saturating_sub(tx_creation_time) > time_limit + curr_time.saturating_sub(tx_creation_time) + time_padding > time_limit } /// If `true`, this transaction is regarded to have been tampered to have a future timestamp. @@ -136,7 +136,7 @@ impl Queue { state_view: &'state StateView, ) -> impl Iterator + 'state { self.accepted_txs.iter().filter_map(|tx| { - if self.is_pending(tx.value(), state_view) { + if self.is_pending(tx.value(), state_view, Duration::from_secs(0)) { return Some(tx.value().clone()); } @@ -152,7 +152,7 @@ impl Queue { ) -> Vec { self.accepted_txs .iter() - .filter(|e| self.is_pending(e.value(), state_view)) + .filter(|e| self.is_pending(e.value(), state_view, Duration::from_secs(0))) .map(|e| e.value().clone()) .choose_multiple( &mut rand::thread_rng(), @@ -160,10 +160,10 @@ impl Queue { ) } - fn check_tx(&self, tx: &AcceptedTransaction, state_view: &StateView) -> Result<(), Error> { + fn check_tx(&self, tx: &AcceptedTransaction, state_view: &StateView, time_padding: Duration) -> Result<(), Error> { if self.is_in_future(tx) { Err(Error::InFuture) - } else if self.is_expired(tx) { + } else if self.is_expired(tx, time_padding) { Err(Error::Expired) } else if tx.is_in_blockchain(state_view) { Err(Error::InBlockchain) @@ -178,7 +178,7 @@ impl Queue { /// See [`enum@Error`] pub fn push(&self, tx: AcceptedTransaction, state_view: &StateView) -> Result<(), Failure> { trace!(?tx, "Pushing to the queue"); - if let Err(err) = self.check_tx(&tx, state_view) { + if let Err(err) = self.check_tx(&tx, state_view, Duration::from_secs(0)) { return Err(Failure { tx, err }); } @@ -242,6 +242,7 @@ impl Queue { seen: &mut Vec>, state_view: &StateView, expired_transactions: &mut Vec, + time_padding: Duration, ) -> Option { loop { let hash = self.tx_hashes.pop()?; @@ -258,7 +259,7 @@ impl Queue { }; let tx = entry.get(); - if let Err(e) = self.check_tx(tx, state_view) { + if let Err(e) = self.check_tx(tx, state_view, time_padding) { let (_, tx) = entry.remove_entry(); self.decrease_per_user_tx_count(tx.as_ref().authority()); if let Error::Expired = e { @@ -299,6 +300,7 @@ impl Queue { state_view: &StateView, max_txs_in_block: NonZeroUsize, transactions: &mut Vec, + time_padding: Duration, ) { if transactions.len() >= max_txs_in_block.get() { return; @@ -308,7 +310,7 @@ impl Queue { let mut expired_transactions = Vec::new(); let txs_from_queue = core::iter::from_fn(|| { - self.pop_from_queue(&mut seen_queue, state_view, &mut expired_transactions) + self.pop_from_queue(&mut seen_queue, state_view, &mut expired_transactions, time_padding) }); let transactions_hashes: IndexSet> = diff --git a/core/src/sumeragi/main_loop.rs b/core/src/sumeragi/main_loop.rs index f8c2d8dd17b..525caec53cc 100644 --- a/core/src/sumeragi/main_loop.rs +++ b/core/src/sumeragi/main_loop.rs @@ -371,7 +371,7 @@ impl Sumeragi { fn cache_transaction(&mut self, state_block: &StateBlock<'_>) { self.transaction_cache.retain(|tx| { - !state_block.has_transaction(tx.as_ref().hash()) && !self.queue.is_expired(tx) + !state_block.has_transaction(tx.as_ref().hash()) && !self.queue.is_expired(tx, self.block_time + self.commit_time) }); } @@ -1026,7 +1026,7 @@ pub(crate) fn run( .transaction_cache // Checking if transactions are in the blockchain is costly .retain(|tx| { - let expired = sumeragi.queue.is_expired(tx); + let expired = sumeragi.queue.is_expired(tx, sumeragi.block_time + sumeragi.commit_time); if expired { debug!(?tx, "Transaction expired") } @@ -1044,6 +1044,7 @@ pub(crate) fn run( .try_into() .expect("INTERNAL BUG: transactions in block exceed usize::MAX"), &mut sumeragi.transaction_cache, + sumeragi.block_time + sumeragi.commit_time, ); let view_change_index = sumeragi.prune_view_change_proofs_and_calculate_current_index(