Merge pull request #2186 from subspace/bundle-check

Double check the domain transaction validity before putting it into bundle
autonomys · Nov 1, 2023 · c0f905f · c0f905f
2 parents cdcf885 + 668b967
commit c0f905f
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 29 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/domains/client/domain-operator/Cargo.toml b/domains/client/domain-operator/Cargo.toml
@@ -31,6 +31,7 @@ sp-keystore = { version = "0.27.0", git = "https://github.com/subspace/polkadot-
 sp-messenger = { version = "0.1.0", path = "../../primitives/messenger" }
 sp-runtime = { version = "24.0.0", git = "https://github.com/subspace/polkadot-sdk", rev = "892bf8e938c6bd2b893d3827d1093cd81baa59a1" }
 sp-state-machine = { version = "0.28.0", git = "https://github.com/subspace/polkadot-sdk", rev = "892bf8e938c6bd2b893d3827d1093cd81baa59a1" }
+sp-transaction-pool = { version = "4.0.0-dev", git = "https://github.com/subspace/polkadot-sdk", rev = "892bf8e938c6bd2b893d3827d1093cd81baa59a1" }
 sp-trie = { version = "22.0.0", git = "https://github.com/subspace/polkadot-sdk", rev = "892bf8e938c6bd2b893d3827d1093cd81baa59a1" }
 sp-weights = { version = "20.0.0", git = "https://github.com/subspace/polkadot-sdk", rev = "892bf8e938c6bd2b893d3827d1093cd81baa59a1" }
 subspace-core-primitives = { version = "0.1.0", path = "../../../crates/subspace-core-primitives" }

diff --git a/domains/client/domain-operator/src/domain_bundle_producer.rs b/domains/client/domain-operator/src/domain_bundle_producer.rs
@@ -16,6 +16,7 @@ use sp_domains::{
 use sp_keystore::KeystorePtr;
 use sp_runtime::traits::{Block as BlockT, Zero};
 use sp_runtime::RuntimeAppPublic;
+use sp_transaction_pool::runtime_api::TaggedTransactionQueue;
 use std::convert::{AsRef, Into};
 use std::marker::PhantomData;
 use std::sync::Arc;
@@ -101,7 +102,7 @@ where
     NumberFor<Block>: Into<NumberFor<CBlock>>,
     NumberFor<ParentChainBlock>: Into<NumberFor<Block>>,
     Client: HeaderBackend<Block> + BlockBackend<Block> + AuxStore + ProvideRuntimeApi<Block>,
-    Client::Api: BlockBuilder<Block> + DomainCoreApi<Block>,
+    Client::Api: BlockBuilder<Block> + DomainCoreApi<Block> + TaggedTransactionQueue<Block>,
     CClient: HeaderBackend<CBlock> + ProvideRuntimeApi<CBlock>,
     CClient::Api: DomainsApi<CBlock, Block::Header> + BundleProducerElectionApi<CBlock, Balance>,
     ParentChain: ParentChainInterface<Block, ParentChainBlock> + Clone,

diff --git a/domains/client/domain-operator/src/domain_bundle_proposer.rs b/domains/client/domain-operator/src/domain_bundle_proposer.rs
@@ -4,12 +4,13 @@ use codec::Encode;
 use domain_runtime_primitives::DomainCoreApi;
 use futures::{select, FutureExt};
 use sc_client_api::{AuxStore, BlockBackend};
-use sc_transaction_pool_api::InPoolTransaction;
+use sc_transaction_pool_api::{InPoolTransaction, TransactionSource};
 use sp_api::{HeaderT, NumberFor, ProvideRuntimeApi};
 use sp_block_builder::BlockBuilder;
 use sp_blockchain::HeaderBackend;
 use sp_domains::{BundleHeader, ExecutionReceipt, HeaderHashingFor, ProofOfElection};
 use sp_runtime::traits::{Block as BlockT, Hash as HashT, One, Zero};
+use sp_transaction_pool::runtime_api::TaggedTransactionQueue;
 use sp_weights::Weight;
 use std::marker::PhantomData;
 use std::sync::Arc;
@@ -49,7 +50,7 @@ where
     CBlock: BlockT,
     NumberFor<Block>: Into<NumberFor<CBlock>>,
     Client: HeaderBackend<Block> + BlockBackend<Block> + AuxStore + ProvideRuntimeApi<Block>,
-    Client::Api: BlockBuilder<Block> + DomainCoreApi<Block>,
+    Client::Api: BlockBuilder<Block> + DomainCoreApi<Block> + TaggedTransactionQueue<Block>,
     CClient: HeaderBackend<CBlock>,
     TransactionPool: sc_transaction_pool_api::TransactionPool<Block = Block>,
 {
@@ -114,32 +115,59 @@ where
                     );
                 })
                 .unwrap_or(false);
+            if !is_within_tx_range {
+                continue;
+            }
+
+            // Double check the transaction validity, because the tx pool are re-validate the transaction
+            // in pool asynchronously so there is race condition that the operator imported a domain block
+            // and start producing bundle immediately before the re-validation based on the latest block
+            // is finished, cause the bundle contains illegal tx accidentally and being considered as invalid
+            // bundle and slashing on the honest operator.
+            //
+            // NOTE: this check need to be kept aligned with how the illegal tx is detected in the block-preprocessor
+            // thus when https://github.com/subspace/subspace/issues/2184 is implemented, we also need to
+            // perfome the check for bundle as a whole instead of checking tx one by one.
+            let transaction_validity = self
+                .client
+                .runtime_api()
+                .validate_transaction(
+                    parent_hash,
+                    TransactionSource::External,
+                    pending_tx_data.clone(),
+                    parent_hash,
+                )
+                .map_err(|error| {
+                    sp_blockchain::Error::Application(Box::from(format!(
+                        "Error getting transaction validity: {error}"
+                    )))
+                })?;
+            if transaction_validity.is_err() {
+                continue;
+            }
 
-            if is_within_tx_range {
-                let tx_weight = self
-                    .client
-                    .runtime_api()
-                    .extrinsic_weight(parent_hash, pending_tx_data)
-                    .map_err(|error| {
-                        sp_blockchain::Error::Application(Box::from(format!(
-                            "Error getting extrinsic weight: {error}"
-                        )))
-                    })?;
-                let next_estimated_bundle_weight =
-                    estimated_bundle_weight.saturating_add(tx_weight);
-                if next_estimated_bundle_weight.any_gt(domain_block_limit.max_block_weight) {
-                    break;
-                }
-
-                let next_bundle_size = bundle_size + pending_tx_data.encoded_size() as u32;
-                if next_bundle_size > domain_block_limit.max_block_size {
-                    break;
-                }
-
-                estimated_bundle_weight = next_estimated_bundle_weight;
-                bundle_size = next_bundle_size;
-                extrinsics.push(pending_tx_data.clone());
+            let tx_weight = self
+                .client
+                .runtime_api()
+                .extrinsic_weight(parent_hash, pending_tx_data)
+                .map_err(|error| {
+                    sp_blockchain::Error::Application(Box::from(format!(
+                        "Error getting extrinsic weight: {error}"
+                    )))
+                })?;
+            let next_estimated_bundle_weight = estimated_bundle_weight.saturating_add(tx_weight);
+            if next_estimated_bundle_weight.any_gt(domain_block_limit.max_block_weight) {
+                break;
             }
+
+            let next_bundle_size = bundle_size + pending_tx_data.encoded_size() as u32;
+            if next_bundle_size > domain_block_limit.max_block_size {
+                break;
+            }
+
+            estimated_bundle_weight = next_estimated_bundle_weight;
+            bundle_size = next_bundle_size;
+            extrinsics.push(pending_tx_data.clone());
         }
 
         let extrinsics_root = HeaderHashingFor::<Block::Header>::ordered_trie_root(

diff --git a/domains/client/domain-operator/src/domain_worker_starter.rs b/domains/client/domain-operator/src/domain_worker_starter.rs
@@ -35,6 +35,7 @@ use sp_core::H256;
 use sp_domains::{BundleProducerElectionApi, DomainsApi};
 use sp_messenger::MessengerApi;
 use sp_runtime::traits::NumberFor;
+use sp_transaction_pool::runtime_api::TaggedTransactionQueue;
 use std::sync::Arc;
 use subspace_runtime_primitives::Balance;
 use tracing::{info, Instrument};
@@ -84,7 +85,8 @@ pub(super) async fn start_worker<
     Client::Api: DomainCoreApi<Block>
         + MessengerApi<Block, NumberFor<Block>>
         + BlockBuilder<Block>
-        + sp_api::ApiExt<Block>,
+        + sp_api::ApiExt<Block>
+        + TaggedTransactionQueue<Block>,
     CClient: HeaderBackend<CBlock>
         + HeaderMetadata<CBlock, Error = sp_blockchain::Error>
         + BlockBackend<CBlock>

diff --git a/domains/client/domain-operator/src/operator.rs b/domains/client/domain-operator/src/operator.rs
@@ -19,6 +19,7 @@ use sp_core::H256;
 use sp_domains::{BundleProducerElectionApi, DomainsApi};
 use sp_messenger::MessengerApi;
 use sp_runtime::traits::{Block as BlockT, NumberFor};
+use sp_transaction_pool::runtime_api::TaggedTransactionQueue;
 use std::sync::Arc;
 use subspace_runtime_primitives::Balance;
 
@@ -74,7 +75,8 @@ where
     Client::Api: DomainCoreApi<Block>
         + MessengerApi<Block, NumberFor<Block>>
         + sp_block_builder::BlockBuilder<Block>
-        + sp_api::ApiExt<Block>,
+        + sp_api::ApiExt<Block>
+        + TaggedTransactionQueue<Block>,
     CClient: HeaderBackend<CBlock>
         + HeaderMetadata<CBlock, Error = sp_blockchain::Error>
         + BlockBackend<CBlock>