Merge pull request #2564 from subspace/fix-flaky-fp-tests

Retry produce bundle with the same slot in the domain test

Author: NingLin-P

crates/subspace-malicious-operator/src/malicious_bundle_producer.rs

@@ -140,6 +140,7 @@ where
             operator_keystore.clone(),
             // The malicious operator doesn't skip empty bundle
             false,
+            false,
         );
 
         let malicious_bundle_tamper =

crates/subspace-malicious-operator/src/malicious_domain_instance_starter.rs

@@ -148,6 +148,7 @@ where
                     domain_message_receiver,
                     provider: eth_provider,
                     skip_empty_bundle_production: true,
+                    skip_out_of_order_slot: false,
                     // Always set it to `None` to not running the normal bundle producer
                     maybe_operator_id: None,
                 };

crates/subspace-node/src/commands/run/domain.rs

@@ -483,6 +483,7 @@ where
                 domain_message_receiver,
                 provider: eth_provider,
                 skip_empty_bundle_production: true,
+                skip_out_of_order_slot: false,
                 maybe_operator_id: operator_id,
             };
 

domains/client/domain-operator/src/domain_bundle_producer.rs

@@ -7,6 +7,7 @@ use sc_client_api::{AuxStore, BlockBackend};
 use sp_api::ProvideRuntimeApi;
 use sp_block_builder::BlockBuilder;
 use sp_blockchain::HeaderBackend;
+use sp_consensus_slots::Slot;
 use sp_domains::core_api::DomainCoreApi;
 use sp_domains::{
     Bundle, BundleProducerElectionApi, DomainId, DomainsApi, OperatorId, OperatorPublicKey,
@@ -40,7 +41,13 @@ where
     keystore: KeystorePtr,
     bundle_producer_election_solver: BundleProducerElectionSolver<Block, CBlock, CClient>,
     domain_bundle_proposer: DomainBundleProposer<Block, Client, CBlock, CClient, TransactionPool>,
+    // TODO: both `skip_empty_bundle_production` and `skip_out_of_order_slot` are only used in the
+    // tests, we should introduce a trait for `DomainBundleProducer` and use a wrapper of `DomainBundleProducer`
+    // in the test, both `skip_empty_bundle_production` and `skip_out_of_order_slot` should move into the wrapper
+    // to keep the production code clean.
     skip_empty_bundle_production: bool,
+    skip_out_of_order_slot: bool,
+    last_processed_slot: Option<Slot>,
 }
 
 impl<Block, CBlock, Client, CClient, TransactionPool> Clone
@@ -59,6 +66,8 @@ where
             bundle_producer_election_solver: self.bundle_producer_election_solver.clone(),
             domain_bundle_proposer: self.domain_bundle_proposer.clone(),
             skip_empty_bundle_production: self.skip_empty_bundle_production,
+            skip_out_of_order_slot: self.skip_out_of_order_slot,
+            last_processed_slot: None,
         }
     }
 }
@@ -92,6 +101,7 @@ where
         bundle_sender: Arc<BundleSender<Block, CBlock>>,
         keystore: KeystorePtr,
         skip_empty_bundle_production: bool,
+        skip_out_of_order_slot: bool,
     ) -> Self {
         let bundle_producer_election_solver = BundleProducerElectionSolver::<Block, CBlock, _>::new(
             keystore.clone(),
@@ -106,6 +116,8 @@ where
             bundle_producer_election_solver,
             domain_bundle_proposer,
             skip_empty_bundle_production,
+            skip_out_of_order_slot,
+            last_processed_slot: None,
         }
     }
 
@@ -131,7 +143,16 @@ where
             // already processed a block higher than the local best and submitted the receipt to
             // the parent chain, we ought to catch up with the consensus block processing before
             // producing new bundle.
-            !domain_best_number.is_zero() && domain_best_number <= head_receipt_number
+            let is_operator_lagging =
+                !domain_best_number.is_zero() && domain_best_number <= head_receipt_number;
+
+            let skip_out_of_order_slot = self.skip_out_of_order_slot
+                && self
+                    .last_processed_slot
+                    .map(|last_slot| last_slot >= slot)
+                    .unwrap_or(false);
+
+            is_operator_lagging || skip_out_of_order_slot
         };
 
         if should_skip_slot {
@@ -182,6 +203,8 @@ where
                 return Ok(None);
             }
 
+            self.last_processed_slot.replace(slot);
+
             info!("🔖 Producing bundle at slot {:?}", slot_info.slot);
 
             let to_sign = bundle_header.hash();

domains/client/domain-operator/src/lib.rs

@@ -176,6 +176,7 @@ pub struct OperatorParams<
     pub domain_confirmation_depth: NumberFor<Block>,
     pub block_import: SharedBlockImport<Block>,
     pub skip_empty_bundle_production: bool,
+    pub skip_out_of_order_slot: bool,
 }
 
 pub(crate) fn load_execution_receipt_by_domain_hash<Block, CBlock, Client>(

domains/client/domain-operator/src/operator.rs

@@ -136,6 +136,7 @@ where
             params.bundle_sender,
             params.keystore.clone(),
             params.skip_empty_bundle_production,
+            params.skip_out_of_order_slot,
         );
 
         let fraud_proof_generator = FraudProofGenerator::new(

domains/client/domain-operator/src/tests.rs

@@ -2965,6 +2965,7 @@ async fn stale_and_in_future_bundle_should_be_rejected() {
             Arc::new(bundle_sender),
             alice.operator.keystore.clone(),
             false,
+            false,
         )
     };
 
@@ -3882,6 +3883,7 @@ async fn test_bad_receipt_chain() {
             Arc::new(bundle_sender),
             alice.operator.keystore.clone(),
             false,
+            false,
         )
     };
 

domains/service/src/domain.rs

@@ -231,6 +231,7 @@ where
     pub domain_message_receiver: TracingUnboundedReceiver<ChainTxPoolMsg>,
     pub provider: Provider,
     pub skip_empty_bundle_production: bool,
+    pub skip_out_of_order_slot: bool,
 }
 
 /// Builds service for a domain full node.
@@ -328,6 +329,7 @@ where
         domain_message_receiver,
         provider,
         skip_empty_bundle_production,
+        skip_out_of_order_slot,
     } = domain_params;
 
     // TODO: Do we even need block announcement on domain node?
@@ -446,6 +448,7 @@ where
             domain_confirmation_depth,
             block_import,
             skip_empty_bundle_production,
+            skip_out_of_order_slot,
         },
     )
     .await?;

domains/test/service/src/domain.rs

@@ -228,6 +228,7 @@ where
             domain_message_receiver,
             provider: DefaultProvider,
             skip_empty_bundle_production,
+            skip_out_of_order_slot: true,
             maybe_operator_id,
         };
 

test/subspace-test-service/src/lib.rs

@@ -529,8 +529,8 @@ impl MockConsensusNode {
         NewSlot,
         OpaqueBundle<NumberFor<Block>, Hash, DomainHeader, Balance>,
     ) {
+        let slot = self.produce_slot();
         for _ in 0..MAX_PRODUCE_BUNDLE_TRY {
-            let slot = self.produce_slot();
             if let Some(bundle) = self.notify_new_slot_and_wait_for_bundle(slot).await {
                 return (slot, bundle);
             }