Add equivocated bundle check

Signed-off-by: linning <[email protected]>

crates/pallet-domains/src/lib.rs

@@ -464,6 +464,17 @@ mod pallet {
         OptionQuery,
     >;
 
+    /// The highest slot of the bundle submitted by an operator
+    #[pallet::storage]
+    pub(super) type OperatorHighestSlot<T: Config> =
+        StorageMap<_, Identity, OperatorId, u64, ValueQuery>;
+
+    /// The set of slot of the bundle submitted by an operator in the current block, cleared at the
+    /// next block initialization
+    #[pallet::storage]
+    pub(super) type OperatorBundleSlot<T: Config> =
+        StorageMap<_, Identity, OperatorId, BTreeSet<u64>, ValueQuery>;
+
     /// Temporary hold of all the operators who decided to switch to another domain.
     /// Once epoch is complete, these operators are added to new domains under next_operators.
     #[pallet::storage]
@@ -686,6 +697,11 @@ mod pallet {
         UnableToCalculateBundleLimit,
         /// Bundle weight exceeds the max bundle weight limit
         BundleTooHeavy,
+        /// The bundle slot is smaller then the highest slot from previous slot
+        /// thus potential equivocated bundle
+        SlotSmallerThanPreviousBlockBundle,
+        /// Equivocated bundle in current block
+        EquivocatedBundle,
     }
 
     #[derive(TypeInfo, Encode, Decode, PalletError, Debug, PartialEq)]
@@ -931,6 +947,7 @@ mod pallet {
             let extrinsics_root = opaque_bundle.extrinsics_root();
             let operator_id = opaque_bundle.operator_id();
             let bundle_size = opaque_bundle.size();
+            let slot_number = opaque_bundle.slot_number();
             let receipt = opaque_bundle.into_receipt();
             #[cfg_attr(feature = "runtime-benchmarks", allow(unused_variables))]
             let receipt_block_number = receipt.domain_block_number;
@@ -1060,6 +1077,8 @@ mod pallet {
 
             SuccessfulBundles::<T>::append(domain_id, bundle_hash);
 
+            OperatorBundleSlot::<T>::mutate(operator_id, |slot_set| slot_set.insert(slot_number));
+
             Self::deposit_event(Event::BundleStored {
                 domain_id,
                 bundle_hash,
@@ -1494,6 +1513,13 @@ mod pallet {
                 T::DomainBundleSubmitted::domain_bundle_submitted(domain_id);
             }
 
+            for (operator_id, slot_set) in OperatorBundleSlot::<T>::drain() {
+                // NOTE: `OperatorBundleSlot` use `BTreeSet` so `last` will return the maximum value in the set
+                if let Some(highest_slot) = slot_set.last() {
+                    OperatorHighestSlot::<T>::insert(operator_id, highest_slot);
+                }
+            }
+
             let _ = SuccessfulFraudProofs::<T>::clear(u32::MAX, None);
 
             Weight::zero()
@@ -1576,14 +1602,15 @@ mod pallet {
                         return InvalidTransactionCode::BundleStorageFeePayment.into();
                     }
 
+                    let tag = (opaque_bundle.operator_id(), opaque_bundle.slot_number());
                     ValidTransaction::with_tag_prefix("SubspaceSubmitBundle")
                         // Bundle have a bit higher priority than normal extrinsic but must less than
                         // fraud proof
                         .priority(1)
                         .longevity(T::ConfirmationDepthK::get().try_into().unwrap_or_else(|_| {
                             panic!("Block number always fits in TransactionLongevity; qed")
                         }))
-                        .and_provides(opaque_bundle.hash())
+                        .and_provides(tag)
                         .propagate(true)
                         .build()
                 }
@@ -1695,18 +1722,6 @@ impl<T: Config> Pallet<T> {
             .map(|operator| (operator.signing_key, operator.current_total_stake))
     }
 
-    fn check_bundle_duplication(opaque_bundle: &OpaqueBundleOf<T>) -> Result<(), BundleError> {
-        // NOTE: it is important to use the hash that not incliude the signature, otherwise
-        // the malicious operator may update its `signing_key` (this may support in the future)
-        // and sign an existing bundle thus creating a duplicated bundle and pass the check.
-        let bundle_header_hash = opaque_bundle.sealed_header.pre_hash();
-        ensure!(
-            !InboxedBundleAuthor::<T>::contains_key(bundle_header_hash),
-            BundleError::DuplicatedBundle
-        );
-        Ok(())
-    }
-
     fn check_extrinsics_root(opaque_bundle: &OpaqueBundleOf<T>) -> Result<(), BundleError> {
         let expected_extrinsics_root = <T::DomainHeader as Header>::Hashing::ordered_trie_root(
             opaque_bundle
@@ -1795,6 +1810,7 @@ impl<T: Config> Pallet<T> {
         let domain_id = opaque_bundle.domain_id();
         let operator_id = opaque_bundle.operator_id();
         let sealed_header = &opaque_bundle.sealed_header;
+        let slot_number = opaque_bundle.slot_number();
 
         let operator = Operators::<T>::get(operator_id).ok_or(BundleError::InvalidOperatorId)?;
 
@@ -1811,7 +1827,18 @@ impl<T: Config> Pallet<T> {
             return Err(BundleError::BadBundleSignature);
         }
 
-        Self::check_bundle_duplication(opaque_bundle)?;
+        // Ensure this is not equivocated bundle that reuse `ProofOfElection` from the previous block
+        ensure!(
+            slot_number
+                > Self::operator_highest_slot_from_previous_block(operator_id, pre_dispatch),
+            BundleError::SlotSmallerThanPreviousBlockBundle,
+        );
+
+        // Ensure there is not equivocated/duplicated bundle in the same block
+        ensure!(
+            !OperatorBundleSlot::<T>::get(operator_id).contains(&slot_number),
+            BundleError::EquivocatedBundle,
+        );
 
         let domain_config = DomainRegistry::<T>::get(domain_id)
             .ok_or(BundleError::InvalidDomainId)?
@@ -2248,6 +2275,27 @@ impl<T: Config> Pallet<T> {
         let storage_fund_acc = storage_fund_account::<T>(operator_id);
         T::Currency::reducible_balance(&storage_fund_acc, Preservation::Preserve, Fortitude::Polite)
     }
+
+    // Get the highest slot of the bundle submitted by a given operator from the previous block
+    //
+    // Return 0 if the operator not submit any bundle before
+    pub fn operator_highest_slot_from_previous_block(
+        operator_id: OperatorId,
+        pre_dispatch: bool,
+    ) -> u64 {
+        if pre_dispatch {
+            OperatorHighestSlot::<T>::get(operator_id)
+        } else {
+            // The `OperatorBundleSlot` is lazily move to `OperatorHighestSlot` in the `on_initialize` hook
+            // so when validating tx in the pool we should check `OperatorBundleSlot` first (which is from the
+            // parent block) then `OperatorHighestSlot`
+            //
+            // NOTE: `OperatorBundleSlot` use `BTreeSet` so `last` will return the maximum value in the set
+            *OperatorBundleSlot::<T>::get(operator_id)
+                .last()
+                .unwrap_or(&OperatorHighestSlot::<T>::get(operator_id))
+        }
+    }
 }
 
 impl<T: Config> sp_domains::DomainOwner<T::AccountId> for Pallet<T> {

crates/sp-domains/src/lib.rs

@@ -472,6 +472,10 @@ impl<Extrinsic: Encode, Number: Encode, Hash: Encode, DomainHeader: HeaderT, Bal
     pub fn estimated_weight(&self) -> Weight {
         self.sealed_header.header.estimated_bundle_weight
     }
+
+    pub fn slot_number(&self) -> u64 {
+        self.sealed_header.header.proof_of_election.slot_number
+    }
 }
 
 /// Bundle with opaque extrinsics.
@@ -713,6 +717,10 @@ pub struct ProofOfElection<CHash> {
     pub vrf_signature: VrfSignature,
     /// Operator index in the OperatorRegistry.
     pub operator_id: OperatorId,
+    /// TODO: this field is only used in the bundle equivocation FP which is removed,
+    /// also this field is problematic see https://github.com/subspace/subspace/issues/2737
+    /// so remove this field before next network
+    ///
     /// Consensus block hash at which proof of election was derived.
     pub consensus_block_hash: CHash,
 }

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

@@ -49,6 +49,7 @@ use sp_runtime::transaction_validity::InvalidTransaction;
 use sp_runtime::OpaqueExtrinsic;
 use sp_state_machine::backend::AsTrieBackend;
 use sp_subspace_mmr::ConsensusChainMmrLeafProof;
+use sp_weights::Weight;
 use std::collections::BTreeMap;
 use std::sync::Arc;
 use subspace_core_primitives::PotOutput;
@@ -2861,59 +2862,6 @@ async fn pallet_domains_unsigned_extrinsics_should_work() {
     // assert_eq!(head_receipt_number(), 2);
 }
 
-#[tokio::test(flavor = "multi_thread")]
-async fn duplicated_bundle_should_be_rejected() {
-    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, 1).await.unwrap();
-
-    let (slot, opaque_bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await;
-    let submit_bundle_tx: OpaqueExtrinsic =
-        subspace_test_runtime::UncheckedExtrinsic::new_unsigned(
-            pallet_domains::Call::submit_bundle { opaque_bundle }.into(),
-        )
-        .into();
-
-    // Wait for one block to ensure the bundle is stored onchain.
-    produce_block_with!(ferdie.produce_block_with_slot(slot), alice)
-        .await
-        .unwrap();
-
-    // Bundle is rejected because it is duplicated.
-    match ferdie
-        .submit_transaction(submit_bundle_tx.clone())
-        .await
-        .unwrap_err()
-    {
-        sc_transaction_pool::error::Error::Pool(TxPoolError::InvalidTransaction(invalid_tx)) => {
-            assert_eq!(invalid_tx, InvalidTransactionCode::Bundle.into())
-        }
-        e => panic!("Unexpected error: {e}"),
-    }
-}
-
 #[tokio::test(flavor = "multi_thread")]
 async fn stale_and_in_future_bundle_should_be_rejected() {
     let directory = TempDir::new().expect("Must be able to create temporary directory");
@@ -4446,3 +4394,138 @@ async fn test_verify_mmr_proof_stateless() {
         produce_blocks!(ferdie, alice, 1).await.unwrap();
     }
 }
+
+#[tokio::test(flavor = "multi_thread")]
+async fn test_equivocated_bundle_check() {
+    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 with Alice Key since that is the sudo key
+    let mut ferdie = MockConsensusNode::run_with_finalization_depth(
+        tokio_handle.clone(),
+        Ferdie,
+        BasePath::new(directory.path().join("ferdie")),
+        // finalization depth
+        Some(10),
+    );
+
+    // Run Alice (an evm domain)
+    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();
+
+    let bundle_to_tx = |opaque_bundle| -> OpaqueExtrinsic {
+        subspace_test_runtime::UncheckedExtrinsic::new_unsigned(
+            pallet_domains::Call::submit_bundle { opaque_bundle }.into(),
+        )
+        .into()
+    };
+
+    // Get a bundle from the txn pool
+    let (_, opaque_bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await;
+    let proof_of_election = opaque_bundle.sealed_header.header.proof_of_election.clone();
+
+    // Construct an equivocated bundle that with the same slot but different content
+    let submit_equivocated_bundle_tx = {
+        let mut equivocated_bundle = opaque_bundle.clone();
+        equivocated_bundle
+            .sealed_header
+            .header
+            .estimated_bundle_weight = Weight::from_all(123);
+        equivocated_bundle.sealed_header.signature = Sr25519Keyring::Alice
+            .pair()
+            .sign(equivocated_bundle.sealed_header.pre_hash().as_ref())
+            .into();
+        bundle_to_tx(equivocated_bundle)
+    };
+
+    // Submit equivocated bundle to tx pool will failed because the equivocated bundle has
+    // the same `provides` tag as the original bundle
+    match ferdie
+        .submit_transaction(submit_equivocated_bundle_tx.clone())
+        .await
+        .unwrap_err()
+    {
+        sc_transaction_pool::error::Error::Pool(TxPoolError::TooLowPriority { .. }) => {}
+        e => panic!("Unexpected error: {e}"),
+    }
+
+    // Produce consensus block with equivocated bundle which will fail to include in block,
+    // in production the whole block will be discard by the honest farmer
+    ferdie
+        .produce_block_with_extrinsics(vec![
+            bundle_to_tx(opaque_bundle.clone()),
+            submit_equivocated_bundle_tx,
+        ])
+        .await
+        .unwrap();
+    let block_hash = ferdie.client.info().best_hash;
+    let block_body = ferdie.client.block_body(block_hash).unwrap().unwrap();
+    let bundles = ferdie
+        .client
+        .runtime_api()
+        .extract_successful_bundles(block_hash, GENESIS_DOMAIN_ID, block_body)
+        .unwrap();
+    assert_eq!(bundles, vec![opaque_bundle]);
+
+    // Produce consensus block with duplicated bundle which will fail to include in block,
+    // in production the whole block will be discard by the honest farmer
+    let (_, opaque_bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await;
+    ferdie
+        .produce_block_with_extrinsics(vec![
+            bundle_to_tx(opaque_bundle.clone()),
+            bundle_to_tx(opaque_bundle.clone()),
+        ])
+        .await
+        .unwrap();
+    let block_hash = ferdie.client.info().best_hash;
+    let block_body = ferdie.client.block_body(block_hash).unwrap().unwrap();
+    let bundles = ferdie
+        .client
+        .runtime_api()
+        .extract_successful_bundles(block_hash, GENESIS_DOMAIN_ID, block_body)
+        .unwrap();
+    assert_eq!(bundles, vec![opaque_bundle]);
+
+    // Construct an equivocated bundle that reuse an old `proof_of_election`
+    let (_, mut opaque_bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await;
+    let submit_equivocated_bundle_tx = {
+        opaque_bundle.sealed_header.header.proof_of_election = proof_of_election;
+        opaque_bundle.sealed_header.signature = Sr25519Keyring::Alice
+            .pair()
+            .sign(opaque_bundle.sealed_header.pre_hash().as_ref())
+            .into();
+        bundle_to_tx(opaque_bundle)
+    };
+    // It will fail to submit to the tx pool
+    match ferdie
+        .submit_transaction(submit_equivocated_bundle_tx.clone())
+        .await
+        .unwrap_err()
+    {
+        sc_transaction_pool::error::Error::Pool(TxPoolError::InvalidTransaction(invalid_tx)) => {
+            assert_eq!(invalid_tx, InvalidTransactionCode::Bundle.into())
+        }
+        e => panic!("Unexpected error: {e}"),
+    }
+    // Also fail to include in block
+    let pre_ferdie_best_number = ferdie.client.info().best_number;
+    let pre_alice_best_number = alice.client.info().best_number;
+    ferdie
+        .produce_block_with_extrinsics(vec![submit_equivocated_bundle_tx])
+        .await
+        .unwrap();
+    assert_eq!(ferdie.client.info().best_number, pre_ferdie_best_number + 1);
+    assert_eq!(alice.client.info().best_number, pre_alice_best_number);
+}