fix: 🐛 Avoid retrying in same tasks (Moonsong-Labs#377)

* feat: ✨ Pass transaction error to `should_retry` callback, now `submit_proof` only retries if there's a timeout

* feat: ✨ Add `retry_only_if_timeout` to `RetryStrategy` builder

* feat: ✨ Use `retry_only_if_timeout` in tasks

* feat: ✨ Queue proof submission if transaction fails after retries

* fix: 🩹 Add `.into()` calls to make rust analyzer stop bothering

* refactor: ♻️ Unify utility functions into `bspStored` with options

* revert: ⏪ Revert unnecessary changes for rust analyzer

* fix: 🐛 Avoid resubmitting proof requests with the same seed which is a problem with reorgs

* fix: 🩹 Remove leftover `only: true` flag in tests

* chore: 🏗️ Add `rust-analyzer` as component in `rust-toolchain.toml` to keep it up to date with the currently used toolchain

* fix: ✅ Fix integration tests failing as consequence of latest changes

* fix: 🩹 Remove `only: true` flag leftover

Author: ffarall

Diff for: client/blockchain-service/src/commands.rs

@@ -29,11 +29,11 @@ use storage_hub_runtime::{AccountId, Balance, StorageDataUnit};
 
 use crate::{
     handler::BlockchainService,
-    transaction::{SubmittedTransaction, WatchTransactionError},
+    transaction::SubmittedTransaction,
     types::{
         ConfirmStoringRequest, Extrinsic, ExtrinsicResult, FileDeletionRequest, MinimalBlockInfo,
         RespondStorageRequest, RetryStrategy, SendExtrinsicOptions,
-        StopStoringForInsolventUserRequest, SubmitProofRequest,
+        StopStoringForInsolventUserRequest, SubmitProofRequest, WatchTransactionError,
     },
 };
 
@@ -852,7 +852,7 @@ where
                     warn!(target: LOG_TARGET, "Transaction failed: {:?}", err);
 
                     if let Some(ref should_retry) = retry_strategy.should_retry {
-                        if !should_retry().await {
+                        if !should_retry(err.clone()).await {
                             return Err(anyhow::anyhow!("Exhausted retry strategy"));
                         }
                     }

Diff for: client/blockchain-service/src/events.rs

@@ -140,12 +140,22 @@ impl From<ProcessFileDeletionRequestData> for ForestWriteLockTaskData {
     }
 }
 
+/// Data required to build a proof to submit to the runtime.
 #[derive(Debug, Clone, Encode, Decode)]
 pub struct ProcessSubmitProofRequestData {
+    /// The Provider ID of the BSP that is submitting the proof.
     pub provider_id: ProofsDealerProviderId,
+    /// The tick for which the proof is being built.
+    ///
+    /// This tick should be the tick where [`Self::seed`] was generated.
     pub tick: BlockNumber,
+    /// The seed that was used to generate the challenges for this proof.
     pub seed: RandomnessOutput,
+    /// All the Forest challenges that the proof to generate has to respond to.
+    ///
+    /// This includes the [`Self::checkpoint_challenges`].
     pub forest_challenges: Vec<H256>,
+    /// The checkpoint challenges that the proof to generate has to respond to.
     pub checkpoint_challenges: Vec<CustomChallenge>,
 }
 

Diff for: client/blockchain-service/src/transaction.rs

@@ -12,7 +12,7 @@ use tokio::sync::mpsc::Receiver;
 
 use crate::{
     commands::BlockchainServiceInterface,
-    types::{Extrinsic, ExtrinsicHash, ExtrinsicResult},
+    types::{Extrinsic, ExtrinsicHash, ExtrinsicResult, WatchTransactionError},
     BlockchainService,
 };
 
@@ -268,18 +268,3 @@ impl SubmittedTransaction {
         Ok(extrinsic_in_block)
     }
 }
-
-#[derive(thiserror::Error, Debug)]
-pub enum WatchTransactionError {
-    #[error("Timeout waiting for transaction to be included in a block")]
-    Timeout,
-    #[error("Transaction watcher channel closed")]
-    WatcherChannelClosed,
-    #[error("Transaction failed. DispatchError: {dispatch_error}, DispatchInfo: {dispatch_info}")]
-    TransactionFailed {
-        dispatch_error: String,
-        dispatch_info: String,
-    },
-    #[error("Unexpected error: {0}")]
-    Internal(String),
-}

Diff for: client/blockchain-service/src/types.rs

@@ -228,6 +228,50 @@ pub type ExtrinsicHash = H256;
 /// Type alias for the tip.
 pub type Tip = pallet_transaction_payment::ChargeTransactionPayment<storage_hub_runtime::Runtime>;
 
+/// Options for [`send_extrinsic`](crate::BlockchainService::send_extrinsic).
+///
+/// You can safely use [`SendExtrinsicOptions::default`] to create a new instance of `SendExtrinsicOptions`.
+#[derive(Debug)]
+pub struct SendExtrinsicOptions {
+    /// Tip to add to the transaction to incentivize the collator to include the transaction in a block.
+    tip: Tip,
+    /// Optionally override the nonce to use when sending the transaction.
+    nonce: Option<u32>,
+}
+
+impl SendExtrinsicOptions {
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    pub fn with_tip(mut self, tip: u128) -> Self {
+        self.tip = Tip::from(tip);
+        self
+    }
+
+    pub fn with_nonce(mut self, nonce: Option<u32>) -> Self {
+        self.nonce = nonce;
+        self
+    }
+
+    pub fn tip(&self) -> Tip {
+        self.tip.clone()
+    }
+
+    pub fn nonce(&self) -> Option<u32> {
+        self.nonce
+    }
+}
+
+impl Default for SendExtrinsicOptions {
+    fn default() -> Self {
+        Self {
+            tip: Tip::from(0),
+            nonce: None,
+        }
+    }
+}
+
 /// A struct which defines a submit extrinsic retry strategy. This defines a simple strategy when
 /// sending and extrinsic. It will retry a maximum number of times ([Self::max_retries]).
 /// If the extrinsic is not included in a block within a certain time frame [`Self::timeout`] it is
@@ -252,10 +296,16 @@ pub struct RetryStrategy {
     /// A higher value will make tips grow faster.
     pub base_multiplier: f64,
     /// An optional check function to determine if the extrinsic should be retried.
+    ///
     /// If this is provided, the function will be called before each retry to determine if the
     /// extrinsic should be retried or the submission should be considered failed. If this is not
     /// provided, the extrinsic will be retried until [`Self::max_retries`] is reached.
-    pub should_retry: Option<Box<dyn Fn() -> Pin<Box<dyn Future<Output = bool> + Send>> + Send>>,
+    ///
+    /// Additionally, the function will receive the [`WatchTransactionError`] as an argument, to
+    /// help determine if the extrinsic should be retried or not.
+    pub should_retry: Option<
+        Box<dyn Fn(WatchTransactionError) -> Pin<Box<dyn Future<Output = bool> + Send>> + Send>,
+    >,
 }
 
 impl RetryStrategy {
@@ -270,35 +320,76 @@ impl RetryStrategy {
         }
     }
 
+    /// Set the maximum number of times to retry sending the extrinsic.
     pub fn with_max_retries(mut self, max_retries: u32) -> Self {
         self.max_retries = max_retries;
         self
     }
 
+    /// Set the timeout for the extrinsic.
+    ///
+    /// After this timeout, the extrinsic will be retried (if applicable) or fail.
     pub fn with_timeout(mut self, timeout: Duration) -> Self {
         self.timeout = timeout;
         self
     }
 
+    /// Set the maximum tip for the extrinsic.
+    ///
+    /// As the number of times the extrinsic is retried increases, the tip will increase
+    /// exponentially, up to this maximum tip.
     pub fn with_max_tip(mut self, max_tip: f64) -> Self {
         self.max_tip = max_tip;
         self
     }
 
+    /// The base multiplier for the exponential backoff.
+    ///
+    /// A higher value will make the exponential backoff more aggressive, making the tip
+    /// increase quicker.
     pub fn with_base_multiplier(mut self, base_multiplier: f64) -> Self {
         self.base_multiplier = base_multiplier;
         self
     }
 
+    /// Set a function to determine if the extrinsic should be retried.
+    ///
+    /// If this function is provided, it will be called before each retry to determine if the
+    /// extrinsic should be retried or the submission should be considered failed. If this function
+    /// is not provided, the extrinsic will be retried until [`Self::max_retries`] is reached.
+    ///
+    /// Additionally, the function will receive the [`WatchTransactionError`] as an argument, to
+    /// help determine if the extrinsic should be retried or not.
     pub fn with_should_retry(
         mut self,
-        should_retry: Option<Box<dyn Fn() -> Pin<Box<dyn Future<Output = bool> + Send>> + Send>>,
+        should_retry: Option<
+            Box<dyn Fn(WatchTransactionError) -> Pin<Box<dyn Future<Output = bool> + Send>> + Send>,
+        >,
     ) -> Self {
         self.should_retry = should_retry;
         self
     }
 
+    /// Sets [`Self::should_retry`] to retry only if the extrinsic times out.
+    ///
+    /// This means that the extrinsic will not be sent again if, for example, it
+    /// is included in a block but it fails.
+    ///
+    /// See [`WatchTransactionError`] for other possible errors.
+    pub fn retry_only_if_timeout(mut self) -> Self {
+        self.should_retry = Some(Box::new(|error| {
+            Box::pin(async move {
+                match error {
+                    WatchTransactionError::Timeout => true,
+                    _ => false,
+                }
+            })
+        }));
+        self
+    }
+
     /// Computes the tip for the given retry count.
+    ///
     /// The formula for the tip is:
     /// [`Self::max_tip`] * (([`Self::base_multiplier`] ^ (retry_count / [`Self::max_retries`]) - 1) /
     /// ([`Self::base_multiplier`] - 1)).
@@ -330,6 +421,21 @@ impl Default for RetryStrategy {
     }
 }
 
+#[derive(thiserror::Error, Debug, Clone)]
+pub enum WatchTransactionError {
+    #[error("Timeout waiting for transaction to be included in a block")]
+    Timeout,
+    #[error("Transaction watcher channel closed")]
+    WatcherChannelClosed,
+    #[error("Transaction failed. DispatchError: {dispatch_error}, DispatchInfo: {dispatch_info}")]
+    TransactionFailed {
+        dispatch_error: String,
+        dispatch_info: String,
+    },
+    #[error("Unexpected error: {0}")]
+    Internal(String),
+}
+
 /// Minimum block information needed to register what is the current best block
 /// and detect reorgs.
 #[derive(Debug, Clone, Encode, Decode, Default, Copy)]
@@ -480,47 +586,3 @@ impl Ord for ForestStorageSnapshotInfo {
         }
     }
 }
-
-/// Options for [`send_extrinsic`](crate::BlockchainService::send_extrinsic).
-///
-/// You can safely use [`SendExtrinsicOptions::default`] to create a new instance of `SendExtrinsicOptions`.
-#[derive(Debug)]
-pub struct SendExtrinsicOptions {
-    /// Tip to add to the transaction to incentivize the collator to include the transaction in a block.
-    tip: Tip,
-    /// Optionally override the nonce to use when sending the transaction.
-    nonce: Option<u32>,
-}
-
-impl SendExtrinsicOptions {
-    pub fn new() -> Self {
-        Self::default()
-    }
-
-    pub fn with_tip(mut self, tip: u128) -> Self {
-        self.tip = Tip::from(tip);
-        self
-    }
-
-    pub fn with_nonce(mut self, nonce: Option<u32>) -> Self {
-        self.nonce = nonce;
-        self
-    }
-
-    pub fn tip(&self) -> Tip {
-        self.tip.clone()
-    }
-
-    pub fn nonce(&self) -> Option<u32> {
-        self.nonce
-    }
-}
-
-impl Default for SendExtrinsicOptions {
-    fn default() -> Self {
-        Self {
-            tip: Tip::from(0),
-            nonce: None,
-        }
-    }
-}