Merge pull request #2243 from subspace/fix-invalid-bundle

Ensure the domain chain follow exactly the consensus chain's fork choice

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

@@ -280,15 +280,11 @@ where
             bundles,
         } = preprocess_result;
 
-        // Although the domain block intuitively ought to use the same fork choice
-        // from the corresponding consensus block, it's fine to forcibly always use
-        // the longest chain for simplicity as we manually build the domain branches
-        // by following the consensus chain branches. Due to the possibility of domain
-        // branch transitioning to a lower fork caused by the change that a consensus block
-        // can possibility produce no domain block, it's important to note that now we
-        // need to ensure the consensus block built from the latest consensus block is the
-        // new best domain block after processing each imported consensus block.
-        let fork_choice = ForkChoiceStrategy::LongestChain;
+        // The fork choice of the domain chain should follow exactly as the consensus chain,
+        // so always use `Custom(false)` here to not set the domain block as new best block
+        // immediately, and if the domain block is indeed derive from the best consensus fork,
+        // it will be reset as the best domain block, see `BundleProcessor::process_bundles`.
+        let fork_choice = ForkChoiceStrategy::Custom(false);
         let inherent_data = get_inherent_data::<_, _, Block>(
             self.consensus_client.clone(),
             consensus_block_hash,

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

@@ -207,6 +207,14 @@ where
             .consensus_client
             .runtime_api()
             .head_receipt_number(consensus_chain_block_hash, self.domain_id)?;
+
+        // TODO: the `receipt_number` may not be the best domain block number if there
+        // is fraud proof submitted and bad ERs pruned, thus the ER may not the one that
+        // derive from the latest domain block, which may cause the lagging operator able
+        // to submit invalid bundle accidentally.
+        //
+        // We need to resolve `https://github.com/subspace/subspace/issues/1673` to fix it
+        // completely.
         let receipt_number = (head_receipt_number + One::one()).min(header_number);
 
         tracing::trace!(

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

@@ -7,7 +7,7 @@ use domain_test_service::EcdsaKeyring::{Alice, Bob};
 use domain_test_service::Sr25519Keyring::{self, Ferdie};
 use domain_test_service::GENESIS_DOMAIN_ID;
 use futures::StreamExt;
-use sc_client_api::{Backend, BlockBackend, HeaderBackend};
+use sc_client_api::{Backend, BlockBackend, BlockchainEvents, HeaderBackend};
 use sc_consensus::SharedBlockImport;
 use sc_service::{BasePath, Role};
 use sc_transaction_pool_api::error::Error as TxPoolError;
@@ -92,6 +92,65 @@ async fn test_domain_instance_bootstrapper() {
         .expect("3 consensus blocks produced successfully");
 }
 
+#[tokio::test(flavor = "multi_thread")]
+async fn test_domain_chain_fork_choice() {
+    let directory = TempDir::new().expect("Must be able to create temporary directory");
+
+    let mut builder = sc_cli::LoggerBuilder::new("");
+    builder.with_colors(false);
+    let _ = builder.init();
+
+    let tokio_handle = tokio::runtime::Handle::current();
+
+    // Start Ferdie
+    let mut ferdie = MockConsensusNode::run(
+        tokio_handle.clone(),
+        Ferdie,
+        BasePath::new(directory.path().join("ferdie")),
+    );
+
+    // Run Alice (a evm domain authority node)
+    let alice = domain_test_service::DomainNodeBuilder::new(
+        tokio_handle.clone(),
+        Alice,
+        BasePath::new(directory.path().join("alice")),
+    )
+    .build_evm_node(Role::Authority, GENESIS_DOMAIN_ID, &mut ferdie)
+    .await;
+
+    produce_blocks!(ferdie, alice, 3).await.unwrap();
+    assert_eq!(alice.client.info().best_number, 3);
+
+    let domain_hash_3 = alice.client.info().best_hash;
+    let common_consensus_hash = ferdie.client.info().best_hash;
+
+    // Fork A produce a consenus block that contains no bundle
+    let slot = ferdie.produce_slot();
+    let fork_a_block_hash = ferdie
+        .produce_block_with_slot_at(slot, common_consensus_hash, Some(vec![]))
+        .await
+        .unwrap();
+
+    // Fork B produce a consenus block that contains bundles thus derive a domain block
+    let mut alice_import_notification_stream = alice.client.every_import_notification_stream();
+    let (slot, bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await;
+    assert!(bundle.is_some());
+    ferdie
+        .produce_block_with_slot_at(slot, common_consensus_hash, None)
+        .await
+        .unwrap();
+
+    // Because the new domain block is in a stale fork thus we need to use `every_import_notification_stream`
+    let new_domain_block = alice_import_notification_stream.next().await.unwrap();
+    assert_eq!(*new_domain_block.header.number(), 4);
+
+    // The consensus fork A is still the best fork, because fork A don't derive any new
+    // domain block thus the best domain block is still #3
+    assert_eq!(ferdie.client.info().best_hash, fork_a_block_hash);
+    assert_eq!(alice.client.info().best_number, 3);
+    assert_eq!(alice.client.info().best_hash, domain_hash_3);
+}
+
 #[tokio::test(flavor = "multi_thread")]
 async fn test_domain_block_production() {
     let directory = TempDir::new().expect("Must be able to create temporary directory");