From 2ee2f62283ab6598d48dae795b9378b0e9bc036b Mon Sep 17 00:00:00 2001 From: gianfra-t <96739519+gianfra-t@users.noreply.github.com> Date: Fri, 8 Dec 2023 10:39:06 -0300 Subject: [PATCH] 452 vault client retries transaction submission for unrecoverable errors (#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 | 41 +++++++++++++++++++++++-- clients/runtime/src/lib.rs | 2 +- clients/runtime/src/rpc.rs | 29 ++++++++++------- clients/runtime/src/tests.rs | 2 +- clients/vault/src/requests/execution.rs | 2 +- clients/vault/src/requests/structs.rs | 29 ++++++++++++----- clients/vault/src/system.rs | 8 ++++- 7 files changed, 88 insertions(+), 25 deletions(-) diff --git a/clients/runtime/src/error.rs b/clients/runtime/src/error.rs index 587364297..bf3461e6a 100644 --- a/clients/runtime/src/error.rs +++ b/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,17 +131,23 @@ impl Error { } } - pub fn is_invalid_transaction(&self) -> Option { + pub fn is_invalid_transaction(&self) -> Option { 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(()) @@ -139,6 +155,7 @@ impl Error { 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")] diff --git a/clients/runtime/src/lib.rs b/clients/runtime/src/lib.rs index ef66088e3..3cf39fc36 100644 --- a/clients/runtime/src/lib.rs +++ b/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")] diff --git a/clients/runtime/src/rpc.rs b/clients/runtime/src/rpc.rs index e843e306b..9d0741740 100644 --- a/clients/runtime/src/rpc.rs +++ b/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::::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)) + } }, + }, } }, ) diff --git a/clients/runtime/src/tests.rs b/clients/runtime/src/tests.rs index 18029d4db..4ba834167 100644 --- a/clients/runtime/src/tests.rs +++ b/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")] diff --git a/clients/vault/src/requests/execution.rs b/clients/vault/src/requests/execution.rs index a8eda4095..8d0c0ab95 100644 --- a/clients/vault/src/requests/execution.rs +++ b/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 diff --git a/clients/vault/src/requests/structs.rs b/clients/vault/src/requests/structs.rs index 801b6bfce..58652bfd1 100644 --- a/clients/vault/src/requests/structs.rs +++ b/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::::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)) + } + }, + }, } }, ) diff --git a/clients/vault/src/system.rs b/clients/vault/src/system.rs index 121fcdaa6..59849155d 100644 --- a/clients/vault/src/system.rs +++ b/clients/vault/src/system.rs @@ -281,7 +281,13 @@ impl Service for VaultService { } async fn start(&mut self) -> Result<(), ServiceError> { - self.run_service().await + let result = self.run_service().await; + if let Err(error) = result { + let _ = self.shutdown.send(()); + Err(error) + } else { + result + } } }