452 vault client retries transaction submission for unrecoverable err…

…ors (#453)

* avoid retry when error will fail always

* default to unrecoverable error

* send shutdown signal when starting the vault fails irrecoverably

* retry transaction if unhanded pool issue

* refactor how recoverable errors and the corresponding error string is defined

* fixes

* comments and small refactor

* remove pool_unactionable from retry

* remove pool issue

* handle error when calling notify retry in vault

* format

* remove outer retry execution loop

* cargo fmt

clients/runtime/src/error.rs

@@ -90,6 +90,16 @@ pub enum Error {
 	PrometheusError(#[from] PrometheusError),
 }
 
+pub enum Recoverability {
+	Recoverable(String),
+	Unrecoverable(String),
+}
+
+enum RecoverableError {
+	InabilityToPayFees,
+	OutdatedTransaction,
+}
+
 impl Error {
 	pub fn is_module_err(&self, pallet_name: &str, error_name: &str) -> bool {
 		matches!(
@@ -121,24 +131,31 @@ impl Error {
 		}
 	}
 
-	pub fn is_invalid_transaction(&self) -> Option<String> {
+	pub fn is_invalid_transaction(&self) -> Option<Recoverability> {
 		self.map_custom_error(|custom_error| {
 			if custom_error.code() == POOL_INVALID_TX {
-				Some(custom_error.data().map(ToString::to_string).unwrap_or_default())
+				let data_string = custom_error.data().map(ToString::to_string).unwrap_or_default();
+
+				if RecoverableError::is_recoverable(&data_string) {
+					return Some(Recoverability::Recoverable(data_string))
+				}
+
+				return Some(Recoverability::Unrecoverable(data_string))
 			} else {
 				None
 			}
 		})
 	}
 
-	pub fn is_pool_too_low_priority(&self) -> Option<()> {
+	pub fn is_pool_too_low_priority(&self) -> bool {
 		self.map_custom_error(|custom_error| {
 			if custom_error.code() == POOL_TOO_LOW_PRIORITY {
 				Some(())
 			} else {
 				None
 			}
 		})
+		.is_some()
 	}
 
 	pub fn is_rpc_disconnect_error(&self) -> bool {
@@ -185,6 +202,24 @@ impl Error {
 	}
 }
 
+impl RecoverableError {
+	// The string which is used to match the error type
+	// For definitions, see: https://github.com/paritytech/substrate/blob/033d4e86cc7eff0066cd376b9375f815761d653c/primitives/runtime/src/transaction_validity.rs#L99
+
+	fn as_str(&self) -> &'static str {
+		match self {
+			RecoverableError::InabilityToPayFees => "Inability to pay some fees",
+			RecoverableError::OutdatedTransaction => "Transaction is outdated",
+		}
+	}
+
+	fn is_recoverable(error_message: &str) -> bool {
+		[RecoverableError::InabilityToPayFees, RecoverableError::OutdatedTransaction]
+			.iter()
+			.any(|error| error_message.contains(error.as_str()))
+	}
+}
+
 #[derive(Error, Debug)]
 pub enum KeyLoadingError {
 	#[error("Key not found in file")]

clients/runtime/src/lib.rs

@@ -12,7 +12,7 @@ use subxt::{
 };
 
 pub use assets::TryFromSymbol;
-pub use error::{Error, SubxtError};
+pub use error::{Error, Recoverability, SubxtError};
 pub use primitives::CurrencyInfo;
 pub use retry::{notify_retry, RetryPolicy};
 #[cfg(feature = "testing-utils")]

clients/runtime/src/rpc.rs

@@ -24,6 +24,7 @@ use primitives::Hash;
 
 use crate::{
 	conn::{new_websocket_client, new_websocket_client_with_retry},
+	error::Recoverability,
 	metadata, notify_retry,
 	types::*,
 	AccountId, Error, RetryPolicy, ShutdownSender, SpacewalkRuntime, SpacewalkSigner, SubxtError,
@@ -232,18 +233,24 @@ impl SpacewalkParachain {
 			},
 			|result| async {
 				match result.map_err(Into::<Error>::into) {
-					Ok(te) => Ok(te),
-					Err(err) =>
-						if let Some(data) = err.is_invalid_transaction() {
-							Err(RetryPolicy::Skip(Error::InvalidTransaction(data)))
-						} else if err.is_pool_too_low_priority().is_some() {
-							Err(RetryPolicy::Skip(Error::PoolTooLowPriority))
-						} else if err.is_block_hash_not_found_error() {
-							log::info!("Re-sending transaction after apparent fork");
-							Err(RetryPolicy::Skip(Error::BlockHashNotFound))
-						} else {
-							Err(RetryPolicy::Throw(err))
+					Ok(ok) => Ok(ok),
+					Err(err) => match err.is_invalid_transaction() {
+						Some(Recoverability::Recoverable(data)) =>
+							Err(RetryPolicy::Skip(Error::InvalidTransaction(data))),
+						Some(Recoverability::Unrecoverable(data)) =>
+							Err(RetryPolicy::Throw(Error::InvalidTransaction(data))),
+						None => {
+							// Handle other errors
+							if err.is_pool_too_low_priority() {
+								Err(RetryPolicy::Skip(Error::PoolTooLowPriority))
+							} else if err.is_block_hash_not_found_error() {
+								log::info!("Re-sending transaction after apparent fork");
+								Err(RetryPolicy::Skip(Error::BlockHashNotFound))
+							} else {
+								Err(RetryPolicy::Throw(err))
+							}
 						},
+					},
 				}
 			},
 		)

clients/runtime/src/tests.rs

@@ -74,7 +74,7 @@ async fn test_too_low_priority_matching() {
 	let (client, _tmp_dir) = default_provider_client(AccountKeyring::Alice).await;
 	let parachain_rpc = setup_provider(client.clone(), AccountKeyring::Alice).await;
 	let err = parachain_rpc.get_too_low_priority_error(AccountKeyring::Bob.into()).await;
-	assert!(err.is_pool_too_low_priority().is_some())
+	assert!(err.is_pool_too_low_priority())
 }
 
 #[tokio::test(flavor = "multi_thread")]

clients/vault/src/requests/execution.rs

@@ -172,7 +172,7 @@ async fn execute_open_request_async(
 					request.request_type(),
 					request.hash()
 				);
-				retry_count += 1; // increase retry count
+				break // There is also no need to retry on an unrecoverable error.
 			},
 			Err(error) => {
 				retry_count += 1; // increase retry count

clients/vault/src/requests/structs.rs

@@ -6,9 +6,9 @@ use crate::{
 };
 use primitives::{stellar::PublicKey, CurrencyId};
 use runtime::{
-	OraclePallet, RedeemPallet, ReplacePallet, SecurityPallet, SpacewalkRedeemRequest,
-	SpacewalkReplaceRequest, StellarPublicKeyRaw, StellarRelayPallet, UtilFuncs, VaultId,
-	VaultRegistryPallet, H256,
+	Error as EnrichedError, OraclePallet, Recoverability, RedeemPallet, ReplacePallet, RetryPolicy,
+	SecurityPallet, SpacewalkRedeemRequest, SpacewalkReplaceRequest, StellarPublicKeyRaw,
+	StellarRelayPallet, UtilFuncs, VaultId, VaultRegistryPallet, H256,
 };
 use sp_runtime::traits::StaticLookup;
 use std::{convert::TryInto, sync::Arc, time::Duration};
@@ -199,11 +199,26 @@ impl Request {
 				)
 			},
 			|result| async {
-				match result {
+				match result.map_err(Into::<EnrichedError>::into) {
 					Ok(ok) => Ok(ok),
-					Err(err) if err.is_rpc_disconnect_error() =>
-						Err(runtime::RetryPolicy::Throw(err)),
-					Err(err) => Err(runtime::RetryPolicy::Skip(err)),
+					Err(err) => match err.is_invalid_transaction() {
+						Some(Recoverability::Recoverable(data)) =>
+							Err(RetryPolicy::Skip(EnrichedError::InvalidTransaction(data))),
+						Some(Recoverability::Unrecoverable(data)) =>
+							Err(RetryPolicy::Throw(EnrichedError::InvalidTransaction(data))),
+						None => {
+							// Handle other errors
+							if err.is_pool_too_low_priority() {
+								Err(RetryPolicy::Skip(EnrichedError::PoolTooLowPriority))
+							} else if err.is_rpc_disconnect_error() {
+								Err(RetryPolicy::Throw(err))
+							} else if err.is_block_hash_not_found_error() {
+								Err(RetryPolicy::Skip(EnrichedError::BlockHashNotFound))
+							} else {
+								Err(RetryPolicy::Throw(err))
+							}
+						},
+					},
 				}
 			},
 		)

clients/vault/src/system.rs

@@ -281,7 +281,13 @@ impl Service<VaultServiceConfig, Error> for VaultService {
 	}
 
 	async fn start(&mut self) -> Result<(), ServiceError<Error>> {
-		self.run_service().await
+		let result = self.run_service().await;
+		if let Err(error) = result {
+			let _ = self.shutdown.send(());
+			Err(error)
+		} else {
+			result
+		}
 	}
 }