Improve syncing and fix deadlock (#1510)

* Improve syncing * Fix potential deadlock * format * Update changelog and version * Remove more self.reads() * addresses() on AccountDetails * Logs --------- Co-authored-by: Thibault Martinez <[email protected]>
iotaledger · Oct 26, 2023 · eddac26 · eddac26
1 parent 5ac5153
commit eddac26
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 37 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/bindings/nodejs/CHANGELOG.md b/bindings/nodejs/CHANGELOG.md
@@ -19,6 +19,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security -->
 
+## 1.1.2 - 2023-10-26
+
+### Fixed
+
+- Slow syncing with many claimable outputs;
+- Potential deadlock in syncing;
+
 ## 1.1.1 - 2023-10-11
 
 ### Added

diff --git a/bindings/nodejs/package.json b/bindings/nodejs/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@iota/sdk",
-    "version": "1.1.1",
+    "version": "1.1.2",
     "description": "Node.js binding to the IOTA SDK library",
     "main": "out/index.js",
     "types": "out/index.d.ts",

diff --git a/sdk/CHANGELOG.md b/sdk/CHANGELOG.md
@@ -19,7 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security -->
 
-## 1.1.2 - 2023-MM-DD
+## 1.1.2 - 2023-10-26
 
 ### Added
 
@@ -32,6 +32,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Account::claim_outputs()` if an input has less amount than min storage deposit;
 - URLs aren't truncated after the hostname anymore;
 - Ledger nano potentially failing to identify the correct remainder output;
+- Slow syncing with many claimable outputs;
+- Potential deadlock in syncing;
 
 ## 1.1.1 - 2023-10-11
 

diff --git a/sdk/Cargo.toml b/sdk/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "iota-sdk"
-version = "1.1.1"
+version = "1.1.2"
 authors = ["IOTA Stiftung"]
 edition = "2021"
 description = "The IOTA SDK provides developers with a seamless experience to develop on IOTA by providing account abstractions and clients to interact with node APIs."

diff --git a/sdk/src/wallet/account/mod.rs b/sdk/src/wallet/account/mod.rs
@@ -294,9 +294,7 @@ impl AccountInner {
     /// Returns all addresses of the account
     pub async fn addresses(&self) -> Result<Vec<AccountAddress>> {
         let account_details = self.details().await;
-        let mut all_addresses = account_details.public_addresses().clone();
-        all_addresses.extend(account_details.internal_addresses().clone());
-        Ok(all_addresses.to_vec())
+        Ok(account_details.addresses())
     }
 
     /// Returns all public addresses of the account
@@ -445,6 +443,15 @@ impl AccountInner {
     }
 }
 
+impl AccountDetails {
+    /// Returns all addresses of the account
+    pub(crate) fn addresses(&self) -> Vec<AccountAddress> {
+        let mut all_addresses = self.public_addresses().clone();
+        all_addresses.extend(self.internal_addresses().clone());
+        all_addresses.to_vec()
+    }
+}
+
 pub(crate) fn build_transaction_from_payload_and_inputs(
     tx_id: TransactionId,
     tx_payload: TransactionPayload,

diff --git a/sdk/src/wallet/account/operations/balance.rs b/sdk/src/wallet/account/operations/balance.rs
@@ -63,12 +63,17 @@ where
     ) -> Result<Balance> {
         let network_id = self.client().get_network_id().await?;
         let rent_structure = self.client().get_rent_structure().await?;
+        let local_time = self.client().get_time_checked().await?;
         let mut balance = Balance::default();
         let mut total_rent_amount = 0;
         let mut total_native_tokens = NativeTokensBuilder::default();
 
         #[cfg(feature = "participation")]
-        let voting_output = self.get_voting_output().await?;
+        let voting_output = account_details.get_voting_output()?;
+
+        let account_addresses = account_details.addresses();
+
+        let claimable_outputs = account_details.claimable_outputs(OutputsToClaim::All, local_time)?;
 
         for address_with_unspent_outputs in addresses_with_unspent_outputs {
             #[cfg(feature = "participation")]
@@ -163,10 +168,7 @@ where
                                 // if we have multiple unlock conditions for basic or nft outputs, then we might can't
                                 // spend the balance at the moment or in the future
 
-                                let account_addresses = self.addresses().await?;
-                                let local_time = self.client().get_time_checked().await?;
-                                let is_claimable =
-                                    self.claimable_outputs(OutputsToClaim::All).await?.contains(output_id);
+                                let is_claimable = claimable_outputs.contains(output_id);
 
                                 // For outputs that are expired or have a timelock unlock condition, but no expiration
                                 // unlock condition and we then can unlock them, then

diff --git a/sdk/src/wallet/account/operations/output_claiming.rs b/sdk/src/wallet/account/operations/output_claiming.rs
@@ -16,7 +16,7 @@ use crate::{
         },
     },
     wallet::account::{
-        operations::helpers::time::can_output_be_unlocked_now, types::Transaction, Account, OutputData,
+        operations::helpers::time::can_output_be_unlocked_now, types::Transaction, Account, AccountDetails, OutputData,
         TransactionOptions,
     },
 };
@@ -32,31 +32,29 @@ pub enum OutputsToClaim {
     All,
 }
 
-impl<S: 'static + SecretManage> Account<S>
-where
-    crate::wallet::Error: From<S::Error>,
-{
+impl AccountDetails {
     /// Get basic and nft outputs that have
     /// [`ExpirationUnlockCondition`](crate::types::block::output::unlock_condition::ExpirationUnlockCondition),
     /// [`StorageDepositReturnUnlockCondition`] or
     /// [`TimelockUnlockCondition`](crate::types::block::output::unlock_condition::TimelockUnlockCondition) and can be
     /// unlocked now and also get basic outputs with only an [`AddressUnlockCondition`] unlock condition, for
     /// additional inputs
-    pub async fn claimable_outputs(&self, outputs_to_claim: OutputsToClaim) -> crate::wallet::Result<Vec<OutputId>> {
-        log::debug!("[OUTPUT_CLAIMING] claimable_outputs");
-        let account_details = self.details().await;
-
-        let local_time = self.client().get_time_checked().await?;
+    pub(crate) fn claimable_outputs(
+        &self,
+        outputs_to_claim: OutputsToClaim,
+        time: u32,
+    ) -> crate::wallet::Result<Vec<OutputId>> {
+        log::debug!("[AccountDetails] claimable_outputs");
 
         // Get outputs for the claim
         let mut output_ids_to_claim: HashSet<OutputId> = HashSet::new();
-        for (output_id, output_data) in account_details
+        for (output_id, output_data) in self
             .unspent_outputs
             .iter()
             .filter(|(_, o)| o.output.is_basic() || o.output.is_nft())
         {
             // Don't use outputs that are locked for other transactions
-            if !account_details.locked_outputs.contains(output_id) && account_details.outputs.contains_key(output_id) {
+            if !self.locked_outputs.contains(output_id) && self.outputs.contains_key(output_id) {
                 if let Some(unlock_conditions) = output_data.output.unlock_conditions() {
                     // If there is a single [UnlockCondition], then it's an
                     // [AddressUnlockCondition] and we own it already without
@@ -65,11 +63,11 @@ where
                         && can_output_be_unlocked_now(
                             // We use the addresses with unspent outputs, because other addresses of the
                             // account without unspent outputs can't be related to this output
-                            &account_details.addresses_with_unspent_outputs,
+                            &self.addresses_with_unspent_outputs,
                             // outputs controlled by an alias or nft are currently not considered
                             &[],
                             output_data,
-                            local_time,
+                            time,
                             // Not relevant without alias addresses
                             None,
                         )?
@@ -78,7 +76,7 @@ where
                             OutputsToClaim::MicroTransactions => {
                                 if let Some(sdr) = unlock_conditions.storage_deposit_return() {
                                     // If expired, it's not a micro transaction anymore
-                                    if unlock_conditions.is_expired(local_time) {
+                                    if unlock_conditions.is_expired(time) {
                                         continue;
                                     }
                                     // Only micro transaction if not the same
@@ -99,7 +97,7 @@ where
                             }
                             OutputsToClaim::Amount => {
                                 let mut claimable_amount = output_data.output.amount();
-                                if !unlock_conditions.is_expired(local_time) {
+                                if !unlock_conditions.is_expired(time) {
                                     claimable_amount -= unlock_conditions
                                         .storage_deposit_return()
                                         .map(|s| s.amount())
@@ -123,6 +121,25 @@ where
         );
         Ok(output_ids_to_claim.into_iter().collect())
     }
+}
+
+impl<S: 'static + SecretManage> Account<S>
+where
+    crate::wallet::Error: From<S::Error>,
+{
+    /// Get basic and nft outputs that have
+    /// [`ExpirationUnlockCondition`](crate::types::block::output::unlock_condition::ExpirationUnlockCondition),
+    /// [`StorageDepositReturnUnlockCondition`] or
+    /// [`TimelockUnlockCondition`](crate::types::block::output::unlock_condition::TimelockUnlockCondition) and can be
+    /// unlocked now and also get basic outputs with only an [`AddressUnlockCondition`] unlock condition, for
+    /// additional inputs
+    pub async fn claimable_outputs(&self, outputs_to_claim: OutputsToClaim) -> crate::wallet::Result<Vec<OutputId>> {
+        let account_details = self.details().await;
+
+        let local_time = self.client().get_time_checked().await?;
+
+        account_details.claimable_outputs(outputs_to_claim, local_time)
+    }
 
     /// Get basic outputs that have only one unlock condition which is [AddressUnlockCondition], so they can be used as
     /// additional inputs

diff --git a/sdk/src/wallet/account/operations/participation/mod.rs b/sdk/src/wallet/account/operations/participation/mod.rs
@@ -26,7 +26,7 @@ use crate::{
         block::output::{unlock_condition::UnlockCondition, Output, OutputId},
     },
     wallet::{
-        account::{Account, OutputData},
+        account::{Account, AccountDetails, OutputData},
         task, Result,
     },
 };
@@ -231,14 +231,7 @@ where
     ///
     /// If multiple outputs with this tag exist, the one with the largest amount will be returned.
     pub async fn get_voting_output(&self) -> Result<Option<OutputData>> {
-        log::debug!("[get_voting_output]");
-        Ok(self
-            .unspent_outputs(None)
-            .await?
-            .iter()
-            .filter(|output_data| is_valid_participation_output(&output_data.output))
-            .max_by_key(|output_data| output_data.output.amount())
-            .cloned())
+        self.details().await.get_voting_output()
     }
 
     /// Gets client for an event.
@@ -303,6 +296,21 @@ where
     }
 }
 
+impl AccountDetails {
+    /// Returns the voting output ("PARTICIPATION" tag).
+    ///
+    /// If multiple outputs with this tag exist, the one with the largest amount will be returned.
+    pub(crate) fn get_voting_output(&self) -> Result<Option<OutputData>> {
+        log::debug!("[get_voting_output]");
+        Ok(self
+            .unspent_outputs
+            .values()
+            .filter(|output_data| is_valid_participation_output(&output_data.output))
+            .max_by_key(|output_data| output_data.output.amount())
+            .cloned())
+    }
+}
+
 fn is_valid_participation_output(output: &Output) -> bool {
     // Only basic outputs can be participation outputs.
     if let Output::Basic(basic_output) = &output {