From 3bb829569cdb3a733d64bc6e49b84bf058571c6d Mon Sep 17 00:00:00 2001 From: linning Date: Sat, 18 May 2024 00:08:34 +0800 Subject: [PATCH] Add equivocated bundle check Signed-off-by: linning --- crates/pallet-domains/src/lib.rs | 76 ++++++-- crates/sp-domains/src/lib.rs | 8 + domains/client/domain-operator/src/tests.rs | 189 ++++++++++++++------ 3 files changed, 206 insertions(+), 67 deletions(-) diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index 3b4ef8288c..81b2fc3f1e 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/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 = + 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 = + StorageMap<_, Identity, OperatorId, BTreeSet, 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::::append(domain_id, bundle_hash); + OperatorBundleSlot::::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::::drain() { + // NOTE: `OperatorBundleSlot` use `BTreeSet` so `last` will return the maximum value in the set + if let Some(highest_slot) = slot_set.last() { + OperatorHighestSlot::::insert(operator_id, highest_slot); + } + } + let _ = SuccessfulFraudProofs::::clear(u32::MAX, None); Weight::zero() @@ -1576,6 +1602,7 @@ 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 @@ -1583,7 +1610,7 @@ mod pallet { .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 Pallet { .map(|operator| (operator.signing_key, operator.current_total_stake)) } - fn check_bundle_duplication(opaque_bundle: &OpaqueBundleOf) -> 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::::contains_key(bundle_header_hash), - BundleError::DuplicatedBundle - ); - Ok(()) - } - fn check_extrinsics_root(opaque_bundle: &OpaqueBundleOf) -> Result<(), BundleError> { let expected_extrinsics_root = ::Hashing::ordered_trie_root( opaque_bundle @@ -1795,6 +1810,7 @@ impl Pallet { 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::::get(operator_id).ok_or(BundleError::InvalidOperatorId)?; @@ -1811,7 +1827,18 @@ impl Pallet { 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::::get(operator_id).contains(&slot_number), + BundleError::EquivocatedBundle, + ); let domain_config = DomainRegistry::::get(domain_id) .ok_or(BundleError::InvalidDomainId)? @@ -2248,6 +2275,27 @@ impl Pallet { let storage_fund_acc = storage_fund_account::(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::::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::::get(operator_id) + .last() + .unwrap_or(&OperatorHighestSlot::::get(operator_id)) + } + } } impl sp_domains::DomainOwner for Pallet { diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 8dcc6656af..d0e7ea6d54 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -472,6 +472,10 @@ impl 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 { 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, } diff --git a/domains/client/domain-operator/src/tests.rs b/domains/client/domain-operator/src/tests.rs index 5be3743bec..435cdbbf85 100644 --- a/domains/client/domain-operator/src/tests.rs +++ b/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); +}