Apply review suggestions

Signed-off-by: linning <linningde25@gmail.com>
autonomys · Jun 23, 2023 · af29acd · af29acd
1 parent 3e928cb
commit af29acd
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 12 deletions.
diff --git a/crates/subspace-service/src/lib.rs b/crates/subspace-service/src/lib.rs
@@ -56,7 +56,7 @@ use sc_service::error::Error as ServiceError;
 use sc_service::{Configuration, NetworkStarter, PartialComponents, SpawnTasksParams, TaskManager};
 use sc_subspace_block_relay::{build_consensus_relay, NetworkWrapper};
 use sc_telemetry::{Telemetry, TelemetryWorker};
-use sp_api::{ApiExt, ConstructRuntimeApi, Metadata, ProvideRuntimeApi, TransactionFor};
+use sp_api::{ApiExt, ConstructRuntimeApi, HeaderT, Metadata, ProvideRuntimeApi, TransactionFor};
 use sp_block_builder::BlockBuilder;
 use sp_blockchain::HeaderMetadata;
 use sp_consensus::{Error as ConsensusError, SyncOracle};
@@ -781,17 +781,21 @@ where
 
     let sync_oracle = sync_service.clone();
     let best_hash = client.info().best_hash;
+    let best_number = client.info().best_number;
     let mut imported_blocks_stream = client.import_notification_stream();
     task_manager.spawn_handle().spawn(
         "maintain-bundles-stored-in-last-k",
         None,
         Box::pin(async move {
             if !sync_oracle.is_major_syncing() {
-                bundle_validator.update_recent_stored_bundles(best_hash);
+                bundle_validator.update_recent_stored_bundles(best_hash, best_number);
             }
             while let Some(incoming_block) = imported_blocks_stream.next().await {
                 if !sync_oracle.is_major_syncing() && incoming_block.is_new_best {
-                    bundle_validator.update_recent_stored_bundles(incoming_block.hash);
+                    bundle_validator.update_recent_stored_bundles(
+                        incoming_block.hash,
+                        *incoming_block.header.number(),
+                    );
                 }
             }
         }),

diff --git a/crates/subspace-transaction-pool/src/bundle_validator.rs b/crates/subspace-transaction-pool/src/bundle_validator.rs
@@ -55,14 +55,22 @@ where
     fn successfully_submitted_bundles_at(
         &self,
         block_hash: Block::Hash,
+        block_number: NumberFor<Block>,
     ) -> sp_blockchain::Result<HashSet<Hash>> {
-        let bundle_hashes: HashSet<_> = self
+        match self
             .client
             .runtime_api()
-            .successful_bundle_hashes(block_hash)?
-            .into_iter()
-            .collect();
-        Ok(bundle_hashes)
+            .successful_bundle_hashes(block_hash)
+        {
+            Ok(bundle_hashes) => Ok(bundle_hashes.into_iter().collect()),
+            Err(err) => {
+                tracing::error!(
+                    %err,
+                    "Failed to calling runtime api bundles at block {block_hash:?}#{block_number:?}"
+                );
+                Err(err.into())
+            }
+        }
     }
 
     /// Initialize recent stored bundle from the last K block
@@ -96,7 +104,7 @@ where
             }
         }
         for (hash, number) in blocks {
-            let bundles = self.successfully_submitted_bundles_at(hash)?;
+            let bundles = self.successfully_submitted_bundles_at(hash, number)?;
             bundle_stored_in_last_k.push_front(BlockBundle::new(hash, number, bundles));
         }
         Ok(())
@@ -146,7 +154,8 @@ where
         // the chain grows.
         let needless_block = enacted.len().saturating_sub(self.confirm_depth_k);
         for enacted_block in enacted.iter().skip(needless_block) {
-            let bundles = self.successfully_submitted_bundles_at(enacted_block.hash)?;
+            let bundles =
+                self.successfully_submitted_bundles_at(enacted_block.hash, enacted_block.number)?;
             bundle_stored_in_last_k.push_front(BlockBundle::new(
                 enacted_block.hash,
                 enacted_block.number,
@@ -274,13 +283,19 @@ where
         }
     }
 
-    pub fn update_recent_stored_bundles(&mut self, new_best_hash: Block::Hash) {
+    pub fn update_recent_stored_bundles(
+        &mut self,
+        new_best_hash: Block::Hash,
+        block_number: NumberFor<Block>,
+    ) {
         if let Err(err) = self.bundle_stored_in_last_k.update_with(|bundles| {
             self.bundle_collector
                 .on_new_best_block(new_best_hash, bundles)
         }) {
             tracing::error!(
                 %err,
+                ?new_best_hash,
+                ?block_number,
                 "Failed to update recent stored bundles for bundle-validator"
             );
         }

diff --git a/test/subspace-test-service/src/lib.rs b/test/subspace-test-service/src/lib.rs
@@ -348,7 +348,7 @@ impl MockPrimaryNode {
                         maybe_block_imported = imported_blocks_stream.next() => {
                             match maybe_block_imported {
                                 Some(block) => if block.is_new_best {
-                                    bundle_validator.update_recent_stored_bundles(block.hash);
+                                    bundle_validator.update_recent_stored_bundles(block.hash, *block.header.number());
                                 }
                                 None => break,
                             }
@@ -533,6 +533,8 @@ impl MockPrimaryNode {
             &BlockId::Hash(self.client.info().best_hash),
             &[self.transaction_pool.hash_of(tx)],
         )?;
+        // `ban_time` have set to 0, explicitly wait 1ms here to ensure `clear_stale` will remove
+        // all the bans as the ban time must be passed.
         tokio::time::sleep(time::Duration::from_millis(1)).await;
         self.transaction_pool
             .pool()