From 9c2c64f8c89e9a31acad9fd0d243292b7b2efab1 Mon Sep 17 00:00:00 2001 From: Greg Fitzgerald Date: Wed, 10 Jun 2020 17:00:13 -0600 Subject: [PATCH] More reliable way to detect expired transactions (#10482) When the root slot is beyond the last valid slot, we can say with certainty that the blockhash is expired. Unfortunately, we still can't say the transaction didn't land. It may have landed a long time ago and the validator has since purged its transaction status. --- tokens/src/commands.rs | 15 +++++----- tokens/src/db.rs | 59 ++++++++++++++------------------------- tokens/src/thin_client.rs | 26 +++++++++-------- 3 files changed, 43 insertions(+), 57 deletions(-) diff --git a/tokens/src/commands.rs b/tokens/src/commands.rs index 025bce8963..3cc4c975f2 100644 --- a/tokens/src/commands.rs +++ b/tokens/src/commands.rs @@ -160,7 +160,7 @@ fn distribute_tokens( let fee_payer_pubkey = args.fee_payer.pubkey(); let message = Message::new_with_payer(&instructions, Some(&fee_payer_pubkey)); match client.send_message(message, &signers) { - Ok((transaction, _last_valid_slot)) => { + Ok((transaction, last_valid_slot)) => { db::set_transaction_info( db, &allocation.recipient.parse().unwrap(), @@ -168,6 +168,7 @@ fn distribute_tokens( &transaction, Some(&new_stake_account_address), false, + last_valid_slot, )?; } Err(e) => { @@ -332,20 +333,20 @@ fn update_finalized_transactions( if info.finalized_date.is_some() { None } else { - Some(&info.transaction) + Some((&info.transaction, info.last_valid_slot)) } }) .collect(); let unconfirmed_signatures: Vec<_> = unconfirmed_transactions .iter() - .map(|tx| tx.signatures[0]) + .map(|(tx, _slot)| tx.signatures[0]) .filter(|sig| *sig != Signature::default()) // Filter out dry-run signatures .collect(); let transaction_statuses = client.get_signature_statuses(&unconfirmed_signatures)?; - let recent_blockhashes = client.get_recent_blockhashes()?; + let root_slot = client.get_slot()?; let mut confirmations = None; - for (transaction, opt_transaction_status) in unconfirmed_transactions + for ((transaction, last_valid_slot), opt_transaction_status) in unconfirmed_transactions .into_iter() .zip(transaction_statuses.into_iter()) { @@ -353,8 +354,8 @@ fn update_finalized_transactions( db, &transaction.signatures[0], opt_transaction_status, - &transaction.message.recent_blockhash, - &recent_blockhashes, + last_valid_slot, + root_slot, ) { Ok(Some(confs)) => { confirmations = Some(cmp::min(confs, confirmations.unwrap_or(usize::MAX))); diff --git a/tokens/src/db.rs b/tokens/src/db.rs index c7829488fb..d792bb3463 100644 --- a/tokens/src/db.rs +++ b/tokens/src/db.rs @@ -1,7 +1,7 @@ use chrono::prelude::*; use pickledb::{error::Error, PickleDb, PickleDbDumpPolicy}; use serde::{Deserialize, Serialize}; -use solana_sdk::{hash::Hash, pubkey::Pubkey, signature::Signature, transaction::Transaction}; +use solana_sdk::{clock::Slot, pubkey::Pubkey, signature::Signature, transaction::Transaction}; use solana_transaction_status::TransactionStatus; use std::{cmp::Ordering, fs, io, path::Path}; @@ -12,6 +12,7 @@ pub struct TransactionInfo { pub new_stake_account_address: Option, pub finalized_date: Option>, pub transaction: Transaction, + pub last_valid_slot: Slot, } #[derive(Serialize, Deserialize, Debug, Default, PartialEq)] @@ -35,6 +36,7 @@ impl Default for TransactionInfo { new_stake_account_address: None, finalized_date: None, transaction, + last_valid_slot: 0, } } } @@ -103,6 +105,7 @@ pub fn set_transaction_info( transaction: &Transaction, new_stake_account_address: Option<&Pubkey>, finalized: bool, + last_valid_slot: Slot, ) -> Result<(), Error> { let finalized_date = if finalized { Some(Utc::now()) } else { None }; let transaction_info = TransactionInfo { @@ -111,6 +114,7 @@ pub fn set_transaction_info( new_stake_account_address: new_stake_account_address.cloned(), finalized_date, transaction: transaction.clone(), + last_valid_slot, }; let signature = transaction.signatures[0]; db.set(&signature.to_string(), &transaction_info)?; @@ -119,20 +123,22 @@ pub fn set_transaction_info( // Set the finalized bit in the database if the transaction is rooted. // Remove the TransactionInfo from the database if the transaction failed. -// Return the number of confirmations on the transaction or None if finalized. +// Return the number of confirmations on the transaction or None if either +// finalized or discarded. pub fn update_finalized_transaction( db: &mut PickleDb, signature: &Signature, opt_transaction_status: Option, - blockhash: &Hash, - recent_blockhashes: &[Hash], + last_valid_slot: Slot, + root_slot: Slot, ) -> Result, Error> { if opt_transaction_status.is_none() { - if !recent_blockhashes.contains(blockhash) { + if root_slot > last_valid_slot { eprintln!( - "Signature not found {} and blockhash not found, likely expired", + "Signature not found {} and blockhash expired. Transaction either dropped or the validator purged the transaction status.", signature ); + // Don't discard the transaction, because we are not certain the // blockhash is expired. Instead, return None to signal that // we don't need to wait for confirmations. @@ -161,7 +167,7 @@ pub fn update_finalized_transaction( return Ok(None); } - // Transaction is rooted. Set finalized in the database. + // Transaction is rooted. Set the finalized date in the database. let mut transaction_info = db.get::(&signature.to_string()).unwrap(); transaction_info.finalized_date = Some(Utc::now()); db.set(&signature.to_string(), &transaction_info)?; @@ -230,12 +236,10 @@ mod tests { let mut db = PickleDb::new_yaml(NamedTempFile::new().unwrap(), PickleDbDumpPolicy::NeverDump); let signature = Signature::default(); - let blockhash = Hash::default(); let transaction_info = TransactionInfo::default(); db.set(&signature.to_string(), &transaction_info).unwrap(); assert!(matches!( - update_finalized_transaction(&mut db, &signature, None, &blockhash, &[blockhash]) - .unwrap(), + update_finalized_transaction(&mut db, &signature, None, 0, 0).unwrap(), Some(0) )); @@ -247,7 +251,7 @@ mod tests { // Same as before, but now with an expired blockhash assert_eq!( - update_finalized_transaction(&mut db, &signature, None, &blockhash, &[]).unwrap(), + update_finalized_transaction(&mut db, &signature, None, 0, 1).unwrap(), None ); @@ -264,7 +268,6 @@ mod tests { let mut db = PickleDb::new_yaml(NamedTempFile::new().unwrap(), PickleDbDumpPolicy::NeverDump); let signature = Signature::default(); - let blockhash = Hash::default(); let transaction_info = TransactionInfo::default(); db.set(&signature.to_string(), &transaction_info).unwrap(); let transaction_status = TransactionStatus { @@ -274,14 +277,8 @@ mod tests { err: None, }; assert_eq!( - update_finalized_transaction( - &mut db, - &signature, - Some(transaction_status), - &blockhash, - &[blockhash] - ) - .unwrap(), + update_finalized_transaction(&mut db, &signature, Some(transaction_status), 0, 0) + .unwrap(), Some(1) ); @@ -298,7 +295,6 @@ mod tests { let mut db = PickleDb::new_yaml(NamedTempFile::new().unwrap(), PickleDbDumpPolicy::NeverDump); let signature = Signature::default(); - let blockhash = Hash::default(); let transaction_info = TransactionInfo::default(); db.set(&signature.to_string(), &transaction_info).unwrap(); let status = Err(TransactionError::AccountNotFound); @@ -309,14 +305,8 @@ mod tests { err: None, }; assert_eq!( - update_finalized_transaction( - &mut db, - &signature, - Some(transaction_status), - &blockhash, - &[blockhash] - ) - .unwrap(), + update_finalized_transaction(&mut db, &signature, Some(transaction_status), 0, 0) + .unwrap(), None ); @@ -330,7 +320,6 @@ mod tests { let mut db = PickleDb::new_yaml(NamedTempFile::new().unwrap(), PickleDbDumpPolicy::NeverDump); let signature = Signature::default(); - let blockhash = Hash::default(); let transaction_info = TransactionInfo::default(); db.set(&signature.to_string(), &transaction_info).unwrap(); let transaction_status = TransactionStatus { @@ -340,14 +329,8 @@ mod tests { err: None, }; assert_eq!( - update_finalized_transaction( - &mut db, - &signature, - Some(transaction_status), - &blockhash, - &[blockhash] - ) - .unwrap(), + update_finalized_transaction(&mut db, &signature, Some(transaction_status), 0, 0) + .unwrap(), None ); diff --git a/tokens/src/thin_client.rs b/tokens/src/thin_client.rs index ed26eda841..769c549b59 100644 --- a/tokens/src/thin_client.rs +++ b/tokens/src/thin_client.rs @@ -12,10 +12,6 @@ use solana_sdk::{ signature::{Signature, Signer}, signers::Signers, system_instruction, - sysvar::{ - recent_blockhashes::{self, RecentBlockhashes}, - Sysvar, - }, transaction::Transaction, transport::{Result, TransportError}, }; @@ -29,6 +25,7 @@ pub trait Client { ) -> Result>>; fn get_balance1(&self, pubkey: &Pubkey) -> Result; fn get_fees1(&self) -> Result<(Hash, FeeCalculator, Slot)>; + fn get_slot1(&self) -> Result; fn get_account1(&self, pubkey: &Pubkey) -> Result>; } @@ -64,6 +61,11 @@ impl Client for RpcClient { Ok(result.value) } + fn get_slot1(&self) -> Result { + self.get_slot() + .map_err(|e| TransportError::Custom(e.to_string())) + } + fn get_account1(&self, pubkey: &Pubkey) -> Result> { self.get_account(pubkey) .map(Some) @@ -103,6 +105,10 @@ impl Client for BankClient { self.get_recent_blockhash_with_commitment(CommitmentConfig::default()) } + fn get_slot1(&self) -> Result { + self.get_slot() + } + fn get_account1(&self, pubkey: &Pubkey) -> Result> { self.get_account(pubkey) } @@ -170,6 +176,10 @@ impl ThinClient { self.client.get_fees1() } + pub fn get_slot(&self) -> Result { + self.client.get_slot1() + } + pub fn get_balance(&self, pubkey: &Pubkey) -> Result { self.client.get_balance1(pubkey) } @@ -177,12 +187,4 @@ impl ThinClient { pub fn get_account(&self, pubkey: &Pubkey) -> Result> { self.client.get_account1(pubkey) } - - pub fn get_recent_blockhashes(&self) -> Result> { - let opt_blockhashes_account = self.get_account(&recent_blockhashes::id())?; - let blockhashes_account = opt_blockhashes_account.unwrap(); - let recent_blockhashes = RecentBlockhashes::from_account(&blockhashes_account).unwrap(); - let hashes = recent_blockhashes.iter().map(|x| x.blockhash).collect(); - Ok(hashes) - } }