From b1fd9404a307775642fed8ba9780757f045d7d09 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 26 Jul 2023 13:38:01 +0800 Subject: [PATCH 1/7] Remove useless DomainExtrinsicsBuilder --- .../src/domain_extrinsics_builder.rs | 99 ------------------- .../src/invalid_state_transition_proof.rs | 70 ++----------- .../src/invalid_transaction_proof.rs | 51 ++-------- crates/subspace-fraud-proof/src/lib.rs | 1 - crates/subspace-fraud-proof/src/tests.rs | 16 --- crates/subspace-service/src/lib.rs | 16 --- domains/client/block-preprocessor/src/lib.rs | 14 --- .../domain-operator/src/bundle_processor.rs | 2 +- test/subspace-test-service/src/lib.rs | 6 -- 9 files changed, 18 insertions(+), 257 deletions(-) delete mode 100644 crates/subspace-fraud-proof/src/domain_extrinsics_builder.rs diff --git a/crates/subspace-fraud-proof/src/domain_extrinsics_builder.rs b/crates/subspace-fraud-proof/src/domain_extrinsics_builder.rs deleted file mode 100644 index 582e275595..0000000000 --- a/crates/subspace-fraud-proof/src/domain_extrinsics_builder.rs +++ /dev/null @@ -1,99 +0,0 @@ -//! This module defines a trait for building the domain extrinsics from the original -//! primary block and provides the implementation for both system domain and core domain. - -use domain_block_preprocessor::runtime_api_light::RuntimeApiLight; -use domain_block_preprocessor::DomainBlockPreprocessor; -use domain_runtime_primitives::opaque::Block; -use sc_client_api::{BlockBackend, HeaderBackend}; -use sp_api::ProvideRuntimeApi; -use sp_core::traits::CodeExecutor; -use sp_core::H256; -use sp_domains::{DomainId, DomainsApi}; -use sp_runtime::traits::Block as BlockT; -use std::marker::PhantomData; -use std::sync::Arc; - -/// Trait to build the extrinsics of domain block derived from the original primary block. -pub trait BuildDomainExtrinsics { - /// Returns the final list of encoded domain-specific extrinsics. - fn build_domain_extrinsics( - &self, - domain_id: DomainId, - consensus_block_hash: CBlock::Hash, - domain_runtime: Vec, - ) -> sp_blockchain::Result>>; -} - -/// Utility to build the system domain extrinsics. -pub struct DomainExtrinsicsBuilder { - consensus_client: Arc, - executor: Arc, - _phantom: PhantomData, -} - -impl Clone for DomainExtrinsicsBuilder { - fn clone(&self) -> Self { - Self { - consensus_client: self.consensus_client.clone(), - executor: self.executor.clone(), - _phantom: self._phantom, - } - } -} - -impl DomainExtrinsicsBuilder -where - CBlock: BlockT, - CBlock::Hash: From, - PClient: HeaderBackend - + BlockBackend - + ProvideRuntimeApi - + Send - + Sync - + 'static, - PClient::Api: - DomainsApi, - Executor: CodeExecutor, -{ - /// Constructs a new instance of [`DomainExtrinsicsBuilder`]. - pub fn new(consensus_client: Arc, executor: Arc) -> Self { - Self { - consensus_client, - executor, - _phantom: Default::default(), - } - } -} - -impl BuildDomainExtrinsics - for DomainExtrinsicsBuilder -where - CBlock: BlockT, - CBlock::Hash: From, - PClient: HeaderBackend - + BlockBackend - + ProvideRuntimeApi - + Send - + Sync - + 'static, - PClient::Api: - DomainsApi, - Executor: CodeExecutor, -{ - fn build_domain_extrinsics( - &self, - domain_id: DomainId, - consensus_block_hash: ::Hash, - domain_runtime: Vec, - ) -> sp_blockchain::Result>> { - let domain_runtime_api_light = - RuntimeApiLight::new(self.executor.clone(), domain_runtime.into()); - let domain_extrinsics = DomainBlockPreprocessor::::new( - domain_id, - self.consensus_client.clone(), - domain_runtime_api_light, - ) - .preprocess_consensus_block_for_verifier(consensus_block_hash)?; - Ok(domain_extrinsics) - } -} diff --git a/crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs b/crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs index a272ed655e..d193b5f355 100644 --- a/crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs +++ b/crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs @@ -5,7 +5,6 @@ //! block execution, block execution hooks (`initialize_block` and `finalize_block`) and any //! specific extrinsic execution are supported. -use crate::domain_extrinsics_builder::BuildDomainExtrinsics; use crate::verifier_api::VerifierApi; use codec::{Codec, Decode, Encode}; use domain_runtime_primitives::opaque::Block; @@ -181,55 +180,31 @@ where } /// Invalid state transition proof verifier. -pub struct InvalidStateTransitionProofVerifier< - CBlock, - CClient, - Exec, - Hash, - VerifierClient, - DomainExtrinsicsBuilder, -> { +pub struct InvalidStateTransitionProofVerifier { consensus_client: Arc, executor: Exec, verifier_client: VerifierClient, - domain_extrinsics_builder: DomainExtrinsicsBuilder, _phantom: PhantomData<(CBlock, Hash)>, } -impl Clone - for InvalidStateTransitionProofVerifier< - CBlock, - CClient, - Exec, - Hash, - VerifierClient, - DomainExtrinsicsBuilder, - > +impl Clone + for InvalidStateTransitionProofVerifier where Exec: Clone, VerifierClient: Clone, - DomainExtrinsicsBuilder: Clone, { fn clone(&self) -> Self { Self { consensus_client: self.consensus_client.clone(), executor: self.executor.clone(), verifier_client: self.verifier_client.clone(), - domain_extrinsics_builder: self.domain_extrinsics_builder.clone(), _phantom: self._phantom, } } } -impl - InvalidStateTransitionProofVerifier< - CBlock, - CClient, - Exec, - Hash, - VerifierClient, - DomainExtrinsicsBuilder, - > +impl + InvalidStateTransitionProofVerifier where CBlock: BlockT, H256: Into, @@ -238,20 +213,17 @@ where Exec: CodeExecutor + Clone + 'static, Hash: Encode + Decode, VerifierClient: VerifierApi, - DomainExtrinsicsBuilder: BuildDomainExtrinsics, { /// Constructs a new instance of [`InvalidStateTransitionProofVerifier`]. pub fn new( consensus_client: Arc, executor: Exec, verifier_client: VerifierClient, - domain_extrinsics_builder: DomainExtrinsicsBuilder, ) -> Self { Self { consensus_client, executor, verifier_client, - domain_extrinsics_builder, _phantom: PhantomData::<(CBlock, Hash)>, } } @@ -308,22 +280,9 @@ where ); new_header.encode() } - ExecutionPhase::ApplyExtrinsic(extrinsic_index) => { - let consensus_block_hash = self - .verifier_client - .primary_hash(*domain_id, parent_number + 1)?; - let domain_extrinsics = self - .domain_extrinsics_builder - .build_domain_extrinsics( - *domain_id, - consensus_block_hash.into(), - domain_runtime_code.wasm_bundle.to_vec(), - ) - .map_err(|_| VerificationError::FailedToBuildDomainExtrinsics)?; - domain_extrinsics - .into_iter() - .nth(*extrinsic_index as usize) - .ok_or(VerificationError::DomainExtrinsicNotFound(*extrinsic_index))? + ExecutionPhase::ApplyExtrinsic(_extrinsic_index) => { + // TODO: Provide the tx Merkle proof and get data from there + Vec::new() } ExecutionPhase::FinalizeBlock { .. } => Vec::new(), }; @@ -363,16 +322,8 @@ pub trait VerifyInvalidStateTransitionProof { ) -> Result<(), VerificationError>; } -impl - VerifyInvalidStateTransitionProof - for InvalidStateTransitionProofVerifier< - CBlock, - C, - Exec, - Hash, - VerifierClient, - DomainExtrinsicsBuilder, - > +impl VerifyInvalidStateTransitionProof + for InvalidStateTransitionProofVerifier where CBlock: BlockT, H256: Into, @@ -381,7 +332,6 @@ where Exec: CodeExecutor + Clone + 'static, Hash: Encode + Decode, VerifierClient: VerifierApi, - DomainExtrinsicsBuilder: BuildDomainExtrinsics, { fn verify_invalid_state_transition_proof( &self, diff --git a/crates/subspace-fraud-proof/src/invalid_transaction_proof.rs b/crates/subspace-fraud-proof/src/invalid_transaction_proof.rs index 059fbe6648..06f47978af 100644 --- a/crates/subspace-fraud-proof/src/invalid_transaction_proof.rs +++ b/crates/subspace-fraud-proof/src/invalid_transaction_proof.rs @@ -1,6 +1,5 @@ //! Invalid transaction proof. -use crate::domain_extrinsics_builder::BuildDomainExtrinsics; use crate::domain_runtime_code::retrieve_domain_runtime_code; use crate::verifier_api::VerifierApi; use codec::{Decode, Encode}; @@ -23,40 +22,23 @@ use std::marker::PhantomData; use std::sync::Arc; /// Invalid transaction proof verifier. -pub struct InvalidTransactionProofVerifier< - CBlock, - CClient, - Hash, - Exec, - VerifierClient, - DomainExtrinsicsBuilder, -> { +pub struct InvalidTransactionProofVerifier { consensus_client: Arc, executor: Arc, verifier_client: VerifierClient, - domain_extrinsics_builder: DomainExtrinsicsBuilder, _phantom: PhantomData<(CBlock, Hash)>, } -impl Clone - for InvalidTransactionProofVerifier< - CBlock, - CClient, - Hash, - Exec, - VerifierClient, - DomainExtrinsicsBuilder, - > +impl Clone + for InvalidTransactionProofVerifier where VerifierClient: Clone, - DomainExtrinsicsBuilder: Clone, { fn clone(&self) -> Self { Self { consensus_client: self.consensus_client.clone(), executor: self.executor.clone(), verifier_client: self.verifier_client.clone(), - domain_extrinsics_builder: self.domain_extrinsics_builder.clone(), _phantom: self._phantom, } } @@ -116,15 +98,8 @@ where Ok(runtime_api_light) } -impl - InvalidTransactionProofVerifier< - CBlock, - CClient, - Hash, - Exec, - VerifierClient, - DomainExtrinsicsBuilder, - > +impl + InvalidTransactionProofVerifier where CBlock: BlockT, Hash: Encode + Decode, @@ -132,7 +107,6 @@ where CClient: HeaderBackend + ProvideRuntimeApi + Send + Sync, CClient::Api: DomainsApi, VerifierClient: VerifierApi, - DomainExtrinsicsBuilder: BuildDomainExtrinsics, Exec: CodeExecutor + 'static, { /// Constructs a new instance of [`InvalidTransactionProofVerifier`]. @@ -140,13 +114,11 @@ where consensus_client: Arc, executor: Arc, verifier_client: VerifierClient, - domain_extrinsics_builder: DomainExtrinsicsBuilder, ) -> Self { Self { consensus_client, executor, verifier_client, - domain_extrinsics_builder, _phantom: Default::default(), } } @@ -242,16 +214,8 @@ pub trait VerifyInvalidTransactionProof { ) -> Result<(), VerificationError>; } -impl - VerifyInvalidTransactionProof - for InvalidTransactionProofVerifier< - CBlock, - Client, - Hash, - Exec, - VerifierClient, - DomainExtrinsicsBuilder, - > +impl VerifyInvalidTransactionProof + for InvalidTransactionProofVerifier where CBlock: BlockT, Hash: Encode + Decode, @@ -259,7 +223,6 @@ where Client: HeaderBackend + ProvideRuntimeApi + Send + Sync, Client::Api: DomainsApi, VerifierClient: VerifierApi, - DomainExtrinsicsBuilder: BuildDomainExtrinsics, Exec: CodeExecutor + 'static, { fn verify_invalid_transaction_proof( diff --git a/crates/subspace-fraud-proof/src/lib.rs b/crates/subspace-fraud-proof/src/lib.rs index 740355effe..41245aea53 100644 --- a/crates/subspace-fraud-proof/src/lib.rs +++ b/crates/subspace-fraud-proof/src/lib.rs @@ -2,7 +2,6 @@ #![warn(missing_docs)] -pub mod domain_extrinsics_builder; mod domain_runtime_code; pub mod invalid_state_transition_proof; pub mod invalid_transaction_proof; diff --git a/crates/subspace-fraud-proof/src/tests.rs b/crates/subspace-fraud-proof/src/tests.rs index 6e8cee5698..7165664d23 100644 --- a/crates/subspace-fraud-proof/src/tests.rs +++ b/crates/subspace-fraud-proof/src/tests.rs @@ -1,4 +1,3 @@ -use crate::domain_extrinsics_builder::DomainExtrinsicsBuilder; use crate::invalid_state_transition_proof::{ExecutionProver, InvalidStateTransitionProofVerifier}; use crate::invalid_transaction_proof::InvalidTransactionProofVerifier; use crate::verifier_api::VerifierApi; @@ -257,21 +256,16 @@ async fn execution_proof_creation_and_verification_should_work() { .unwrap(); assert_eq!(post_execution_root, intermediate_roots[0].into()); - let domain_extrinsics_builder = - DomainExtrinsicsBuilder::new(ferdie.client.clone(), Arc::new(ferdie.executor.clone())); - let invalid_state_transition_proof_verifier = InvalidStateTransitionProofVerifier::new( ferdie.client.clone(), ferdie.executor.clone(), TestVerifierClient::new(ferdie.client.clone(), alice.client.clone()), - DomainExtrinsicsBuilder::new(ferdie.client.clone(), Arc::new(ferdie.executor.clone())), ); let invalid_transaction_proof_verifier = InvalidTransactionProofVerifier::new( ferdie.client.clone(), Arc::new(ferdie.executor.clone()), TestVerifierClient::new(ferdie.client.clone(), alice.client.clone()), - domain_extrinsics_builder, ); let proof_verifier = ProofVerifier::::new( @@ -553,17 +547,12 @@ async fn invalid_execution_proof_should_not_work() { ferdie.client.clone(), ferdie.executor.clone(), TestVerifierClient::new(ferdie.client.clone(), alice.client.clone()), - DomainExtrinsicsBuilder::new(ferdie.client.clone(), Arc::new(ferdie.executor.clone())), ); - let domain_extrinsics_builder = - DomainExtrinsicsBuilder::new(ferdie.client.clone(), Arc::new(ferdie.executor.clone())); - let invalid_transaction_proof_verifier = InvalidTransactionProofVerifier::new( ferdie.client.clone(), Arc::new(ferdie.executor.clone()), TestVerifierClient::new(ferdie.client.clone(), alice.client.clone()), - domain_extrinsics_builder, ); let proof_verifier = ProofVerifier::::new( @@ -709,21 +698,16 @@ async fn invalid_execution_proof_should_not_work() { // let good_invalid_transaction_proof = extract_fraud_proof_from_tx_pool(); -// let domain_extrinsics_builder = -// DomainExtrinsicsBuilder::new(ferdie.client.clone(), Arc::new(ferdie.executor.clone())); - // let invalid_state_transition_proof_verifier = InvalidStateTransitionProofVerifier::new( // ferdie.client.clone(), // ferdie.executor.clone(), // TestVerifierClient::new(ferdie.client.clone(), alice.client.clone()), -// domain_extrinsics_builder.clone(), // ); // let invalid_transaction_proof_verifier = InvalidTransactionProofVerifier::new( // ferdie.client.clone(), // Arc::new(ferdie.executor.clone()), // TestVerifierClient::new(ferdie.client.clone(), alice.client.clone()), -// domain_extrinsics_builder, // ); // let proof_verifier = ProofVerifier::::new( diff --git a/crates/subspace-service/src/lib.rs b/crates/subspace-service/src/lib.rs index 67c1475ae5..9eaf0ff560 100644 --- a/crates/subspace-service/src/lib.rs +++ b/crates/subspace-service/src/lib.rs @@ -76,7 +76,6 @@ use sp_transaction_pool::runtime_api::TaggedTransactionQueue; use std::marker::PhantomData; use std::sync::{Arc, Mutex}; use subspace_core_primitives::crypto::kzg::{embedded_kzg_settings, Kzg}; -use subspace_fraud_proof::domain_extrinsics_builder::DomainExtrinsicsBuilder; use subspace_fraud_proof::verifier_api::VerifierClient; use subspace_networking::libp2p::multiaddr::Protocol; use subspace_networking::libp2p::Multiaddr; @@ -138,11 +137,6 @@ pub type InvalidTransactionProofVerifier = Hash, NativeElseWasmExecutor, VerifierClient, Block>, - DomainExtrinsicsBuilder< - Block, - FullClient, - NativeElseWasmExecutor, - >, >; pub type InvalidStateTransitionProofVerifier = @@ -152,11 +146,6 @@ pub type InvalidStateTransitionProofVerifier = NativeElseWasmExecutor, Hash, VerifierClient, Block>, - DomainExtrinsicsBuilder< - Block, - FullClient, - NativeElseWasmExecutor, - >, >; pub type FraudProofVerifier = subspace_fraud_proof::ProofVerifier< @@ -336,21 +325,16 @@ where let bundle_validator = BundleValidator::new(client.clone()); - let domain_extrinsics_builder = - DomainExtrinsicsBuilder::new(client.clone(), Arc::new(executor.clone())); - let invalid_transaction_proof_verifier = InvalidTransactionProofVerifier::new( client.clone(), Arc::new(executor.clone()), VerifierClient::new(client.clone()), - domain_extrinsics_builder.clone(), ); let invalid_state_transition_proof_verifier = InvalidStateTransitionProofVerifier::new( client.clone(), executor, VerifierClient::new(client.clone()), - domain_extrinsics_builder, ); let proof_verifier = subspace_fraud_proof::ProofVerifier::new( diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index b6911bdfe9..22dc22593d 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -273,20 +273,6 @@ where } } - pub fn preprocess_consensus_block_for_verifier( - &self, - consensus_block_hash: CBlock::Hash, - ) -> sp_blockchain::Result>> { - // `domain_hash` is unused in `preprocess_primary_block` when using stateless runtime api. - let domain_hash = Default::default(); - match self.preprocess_consensus_block(consensus_block_hash, domain_hash)? { - Some(PreprocessResult { extrinsics, .. }) => { - Ok(extrinsics.into_iter().map(|ext| ext.encode()).collect()) - } - None => Ok(Vec::new()), - } - } - pub fn preprocess_consensus_block( &self, consensus_block_hash: CBlock::Hash, diff --git a/domains/client/domain-operator/src/bundle_processor.rs b/domains/client/domain-operator/src/bundle_processor.rs index 689efd25b0..1f398c123e 100644 --- a/domains/client/domain-operator/src/bundle_processor.rs +++ b/domains/client/domain-operator/src/bundle_processor.rs @@ -41,7 +41,7 @@ where keystore: KeystorePtr, domain_receipts_checker: DomainReceiptsChecker, domain_block_preprocessor: - DomainBlockPreprocessor>, + DomainBlockPreprocessor>, domain_block_processor: DomainBlockProcessor, } diff --git a/test/subspace-test-service/src/lib.rs b/test/subspace-test-service/src/lib.rs index 2cbc9ff68d..d607e59726 100644 --- a/test/subspace-test-service/src/lib.rs +++ b/test/subspace-test-service/src/lib.rs @@ -69,7 +69,6 @@ use std::marker::PhantomData; use std::sync::Arc; use std::time; use subspace_core_primitives::{Randomness, Solution}; -use subspace_fraud_proof::domain_extrinsics_builder::DomainExtrinsicsBuilder; use subspace_fraud_proof::invalid_state_transition_proof::InvalidStateTransitionProofVerifier; use subspace_fraud_proof::invalid_transaction_proof::InvalidTransactionProofVerifier; use subspace_fraud_proof::verifier_api::VerifierClient; @@ -283,21 +282,16 @@ impl MockConsensusNode { let mut bundle_validator = BundleValidator::new(client.clone()); - let domain_extrinsics_builder = - DomainExtrinsicsBuilder::new(client.clone(), Arc::new(executor.clone())); - let invalid_transaction_proof_verifier = InvalidTransactionProofVerifier::new( client.clone(), Arc::new(executor.clone()), VerifierClient::new(client.clone()), - domain_extrinsics_builder.clone(), ); let invalid_state_transition_proof_verifier = InvalidStateTransitionProofVerifier::new( client.clone(), executor.clone(), VerifierClient::new(client.clone()), - domain_extrinsics_builder, ); let proof_verifier = subspace_fraud_proof::ProofVerifier::new( From 22afad3abc932314afe6734ca6a00c32b979f402 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 26 Jul 2023 14:29:50 +0800 Subject: [PATCH 2/7] Refactor TransctionSelector Rename `should_select_tx()` to `is_within_tx_range()` and handle the case of unsigned transactions within `is_within_tx_range()`. --- .../domain-operator/src/bundle_processor.rs | 2 +- .../src/domain_bundle_proposer.rs | 19 ++++++---- .../client/domain-operator/src/sortition.rs | 36 ++++++++++--------- 3 files changed, 33 insertions(+), 24 deletions(-) diff --git a/domains/client/domain-operator/src/bundle_processor.rs b/domains/client/domain-operator/src/bundle_processor.rs index 1f398c123e..689efd25b0 100644 --- a/domains/client/domain-operator/src/bundle_processor.rs +++ b/domains/client/domain-operator/src/bundle_processor.rs @@ -41,7 +41,7 @@ where keystore: KeystorePtr, domain_receipts_checker: DomainReceiptsChecker, domain_block_preprocessor: - DomainBlockPreprocessor>, + DomainBlockPreprocessor>, domain_block_processor: DomainBlockProcessor, } diff --git a/domains/client/domain-operator/src/domain_bundle_proposer.rs b/domains/client/domain-operator/src/domain_bundle_proposer.rs index 7f73a557c5..d99899a0a5 100644 --- a/domains/client/domain-operator/src/domain_bundle_proposer.rs +++ b/domains/client/domain-operator/src/domain_bundle_proposer.rs @@ -1,5 +1,5 @@ use crate::parent_chain::ParentChainInterface; -use crate::sortition::{TransactionSelectError, TransactionSelector}; +use crate::sortition::TransactionSelector; use crate::ExecutionReceiptFor; use codec::Encode; use domain_runtime_primitives::DomainCoreApi; @@ -107,13 +107,18 @@ where let mut bundle_size = 0u32; for pending_tx in pending_iterator { let pending_tx_data = pending_tx.data().clone(); + let should_select_this_tx = tx_selector - .should_select_tx(parent_hash, pending_tx_data.clone()) - .unwrap_or_else(|err| { - // Accept unsigned transactions like cross domain. - tracing::trace!("propose bundle: sortition select failed: {err:?}"); - matches!(err, TransactionSelectError::TxSignerNotFound) - }); + .is_within_tx_range(parent_hash, pending_tx_data.clone()) + .map_err(|err| { + tracing::error!( + ?err, + ?pending_tx_data, + "Error occurred in locating the tx range" + ); + }) + .unwrap_or(false); + if should_select_this_tx { let tx_weight = self .client diff --git a/domains/client/domain-operator/src/sortition.rs b/domains/client/domain-operator/src/sortition.rs index 2e75d9bea8..d2f1f15735 100644 --- a/domains/client/domain-operator/src/sortition.rs +++ b/domains/client/domain-operator/src/sortition.rs @@ -39,15 +39,15 @@ where /// Checks if the transaction should be selected based on the /// sortition scheme - pub(crate) fn should_select_tx( + pub(crate) fn is_within_tx_range( &self, at: Block::Hash, tx: Block::Extrinsic, ) -> Result { - // Extract the signer Id hash - let api = self.client.runtime_api(); - let ret = api.extract_signer(at, vec![tx])?; - let signer_id_hash = ret + let maybe_signer_id_hash = self + .client + .runtime_api() + .extract_signer(at, vec![tx])? .into_iter() .next() .and_then(|(maybe_signer, _)| { @@ -55,15 +55,19 @@ where let bytes = signer.encode(); U256::from_be_bytes(blake2b_256_hash(&bytes)) }) - }) - .ok_or(TransactionSelectError::TxSignerNotFound)?; - - // Check if the signer Id hash is within the tx range - Ok(signer_in_tx_range( - &self.bundle_vrf_hash, - &signer_id_hash, - &self.tx_range, - )) + }); + + if let Some(signer_id_hash) = maybe_signer_id_hash { + // Check if the signer Id hash is within the tx range + Ok(signer_in_tx_range( + &self.bundle_vrf_hash, + &signer_id_hash, + &self.tx_range, + )) + } else { + // Unsigned transactions are always in the range. + Ok(true) + } } } @@ -73,8 +77,8 @@ pub enum TransactionSelectError { #[error(transparent)] RuntimeApi(#[from] sp_api::ApiError), - #[error("Transaction signer not found")] - TxSignerNotFound, + #[error(transparent)] + Blockchain(#[from] sp_blockchain::Error), } /// Checks if the signer Id hash is within the tx range From d14017804856958f5e966c0db47824dc53f01537 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 26 Jul 2023 22:11:42 +0800 Subject: [PATCH 3/7] Introduce `is_with_tx_range` in DomainCoreApi This commit is a pure refactoring to make the tx range check reusable in domain-block-preprocessor later. The sortition module in domain-operator is now not useful and deleted. --- Cargo.lock | 4 + crates/sp-domains/Cargo.toml | 3 + crates/sp-domains/src/lib.rs | 10 +- crates/sp-domains/src/tests.rs | 209 ++++++++++++ .../src/domain_bundle_producer.rs | 9 +- .../src/domain_bundle_proposer.rs | 17 +- domains/client/domain-operator/src/lib.rs | 1 - .../client/domain-operator/src/sortition.rs | 301 ------------------ domains/primitives/runtime/Cargo.toml | 2 + domains/primitives/runtime/src/lib.rs | 7 + domains/runtime/evm/Cargo.toml | 2 + domains/runtime/evm/src/lib.rs | 61 +++- domains/test/runtime/evm/Cargo.toml | 2 + domains/test/runtime/evm/src/lib.rs | 59 +++- 14 files changed, 335 insertions(+), 352 deletions(-) create mode 100644 crates/sp-domains/src/tests.rs delete mode 100644 domains/client/domain-operator/src/sortition.rs diff --git a/Cargo.lock b/Cargo.lock index b433ef7ae8..9b1ef37716 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2341,6 +2341,7 @@ dependencies = [ "sp-runtime", "sp-std", "sp-weights", + "subspace-core-primitives", "subspace-runtime-primitives", ] @@ -2836,6 +2837,7 @@ dependencies = [ "sp-std", "sp-transaction-pool", "sp-version", + "subspace-core-primitives", "subspace-runtime-primitives", "substrate-wasm-builder", ] @@ -2888,6 +2890,7 @@ dependencies = [ "sp-std", "sp-transaction-pool", "sp-version", + "subspace-core-primitives", "subspace-runtime-primitives", "substrate-wasm-builder", ] @@ -9857,6 +9860,7 @@ name = "sp-domains" version = "0.1.0" dependencies = [ "blake2", + "num-traits", "parity-scale-codec", "rs_merkle", "scale-info", diff --git a/crates/sp-domains/Cargo.toml b/crates/sp-domains/Cargo.toml index a0380d8846..c9337daefc 100644 --- a/crates/sp-domains/Cargo.toml +++ b/crates/sp-domains/Cargo.toml @@ -34,6 +34,9 @@ subspace-core-primitives = { version = "0.1.0", default-features = false, path = subspace-runtime-primitives = { version = "0.1.0", default-features = false, path = "../subspace-runtime-primitives" } thiserror = { version = "1.0.38", optional = true } +[dev-dependencies] +num-traits = "0.2.15" + [features] default = ["std"] std = [ diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 069039b3aa..2075ee8631 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -20,6 +20,8 @@ pub mod bundle_producer_election; pub mod fraud_proof; pub mod merkle_tree; +#[cfg(test)] +mod tests; pub mod transaction; use bundle_producer_election::{BundleProducerElectionParams, VrfProofError}; @@ -40,7 +42,7 @@ use sp_runtime_interface::{pass_by, runtime_interface}; use sp_std::vec::Vec; use sp_weights::Weight; use subspace_core_primitives::crypto::blake2b_256_hash; -use subspace_core_primitives::{Blake2b256Hash, Randomness, U256}; +use subspace_core_primitives::{bidirectional_distance, Blake2b256Hash, Randomness, U256}; use subspace_runtime_primitives::{Balance, Moment}; /// Key type for Operator. @@ -569,6 +571,12 @@ pub struct DomainBlockLimit { pub max_block_weight: Weight, } +/// Checks if the signer Id hash is within the tx range +pub fn signer_in_tx_range(bundle_vrf_hash: &U256, signer_id_hash: &U256, tx_range: &U256) -> bool { + let distance_from_vrf_hash = bidirectional_distance(bundle_vrf_hash, signer_id_hash); + distance_from_vrf_hash <= (*tx_range / 2) +} + sp_api::decl_runtime_apis! { /// API necessary for domains pallet. pub trait DomainsApi { diff --git a/crates/sp-domains/src/tests.rs b/crates/sp-domains/src/tests.rs new file mode 100644 index 0000000000..5a087d890a --- /dev/null +++ b/crates/sp-domains/src/tests.rs @@ -0,0 +1,209 @@ +use crate::signer_in_tx_range; +use num_traits::ops::wrapping::{WrappingAdd, WrappingSub}; +use subspace_core_primitives::U256; + +#[test] +fn test_tx_range() { + let tx_range = U256::MAX / 4; + let bundle_vrf_hash = U256::MAX / 2; + + let signer_id_hash = bundle_vrf_hash + U256::from(10_u64); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::from(10_u64); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash + U256::MAX / 8; + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::MAX / 8; + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash + U256::MAX / 8 + U256::from(1_u64); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::MAX / 8 - U256::from(1_u64); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash + U256::MAX / 4; + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::MAX / 4; + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); +} + +#[test] +fn test_tx_range_wrap_under_flow() { + let tx_range = U256::MAX / 4; + let bundle_vrf_hash = U256::from(100_u64); + + let signer_id_hash = bundle_vrf_hash + U256::from(1000_u64); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash.wrapping_sub(&U256::from(1000_u64)); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash + U256::MAX / 8; + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let v = U256::MAX / 8; + let signer_id_hash = bundle_vrf_hash.wrapping_sub(&v); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash + U256::MAX / 8 + U256::from(1_u64); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let v = U256::MAX / 8 + U256::from(1_u64); + let signer_id_hash = bundle_vrf_hash.wrapping_sub(&v); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash + U256::MAX / 4; + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let v = U256::MAX / 4; + let signer_id_hash = bundle_vrf_hash.wrapping_sub(&v); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); +} + +#[test] +fn test_tx_range_wrap_over_flow() { + let tx_range = U256::MAX / 4; + let v = U256::MAX; + let bundle_vrf_hash = v.wrapping_sub(&U256::from(100_u64)); + + let signer_id_hash = bundle_vrf_hash.wrapping_add(&U256::from(1000_u64)); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::from(1000_u64); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let v = U256::MAX / 8; + let signer_id_hash = bundle_vrf_hash.wrapping_add(&v); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::MAX / 8; + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let v = U256::MAX / 8 + U256::from(1_u64); + let signer_id_hash = bundle_vrf_hash.wrapping_add(&v); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::MAX / 8 - U256::from(1_u64); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let v = U256::MAX / 4; + let signer_id_hash = bundle_vrf_hash.wrapping_add(&v); + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); + + let signer_id_hash = bundle_vrf_hash - U256::MAX / 4; + assert!(!signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); +} + +#[test] +fn test_tx_range_max() { + let tx_range = U256::MAX; + let bundle_vrf_hash = U256::MAX / 2; + + let signer_id_hash = bundle_vrf_hash + U256::from(10_u64); + assert!(signer_in_tx_range( + &bundle_vrf_hash, + &signer_id_hash, + &tx_range + )); +} diff --git a/domains/client/domain-operator/src/domain_bundle_producer.rs b/domains/client/domain-operator/src/domain_bundle_producer.rs index 5ce1401a59..10afc5df09 100644 --- a/domains/client/domain-operator/src/domain_bundle_producer.rs +++ b/domains/client/domain-operator/src/domain_bundle_producer.rs @@ -1,7 +1,6 @@ use crate::bundle_producer_election_solver::BundleProducerElectionSolver; use crate::domain_bundle_proposer::DomainBundleProposer; use crate::parent_chain::ParentChainInterface; -use crate::sortition::TransactionSelector; use crate::utils::OperatorSlotInfo; use crate::BundleSender; use codec::Decode; @@ -19,7 +18,6 @@ use sp_runtime::traits::{Block as BlockT, One, Saturating, Zero}; use sp_runtime::RuntimeAppPublic; use std::marker::PhantomData; use std::sync::Arc; -use subspace_core_primitives::U256; use subspace_runtime_primitives::Balance; type OpaqueBundle = sp_domains::OpaqueBundle< @@ -208,18 +206,13 @@ where "Error getting tx range: {error}" ))) })?; - let tx_selector = TransactionSelector::new( - U256::from_be_bytes(proof_of_election.vrf_hash()), - tx_range, - self.client.clone(), - ); let (bundle_header, extrinsics) = self .domain_bundle_proposer .propose_bundle_at( proof_of_election, consensus_block_info, self.parent_chain.clone(), - tx_selector, + tx_range, ) .await?; diff --git a/domains/client/domain-operator/src/domain_bundle_proposer.rs b/domains/client/domain-operator/src/domain_bundle_proposer.rs index d99899a0a5..bd505f765d 100644 --- a/domains/client/domain-operator/src/domain_bundle_proposer.rs +++ b/domains/client/domain-operator/src/domain_bundle_proposer.rs @@ -1,5 +1,4 @@ use crate::parent_chain::ParentChainInterface; -use crate::sortition::TransactionSelector; use crate::ExecutionReceiptFor; use codec::Encode; use domain_runtime_primitives::DomainCoreApi; @@ -15,6 +14,7 @@ use sp_weights::Weight; use std::marker::PhantomData; use std::sync::Arc; use std::time; +use subspace_core_primitives::U256; use subspace_runtime_primitives::Balance; pub(super) struct DomainBundleProposer { @@ -77,7 +77,7 @@ where proof_of_election: ProofOfElection, consensus_block_info: HashAndNumber, parent_chain: ParentChain, - tx_selector: TransactionSelector, + tx_range: U256, ) -> sp_blockchain::Result> where ParentChainBlock: BlockT, @@ -101,15 +101,18 @@ where } }; + let bundle_vrf_hash = U256::from_be_bytes(proof_of_election.vrf_hash()); let domain_block_limit = parent_chain.domain_block_limit(parent_chain.best_hash())?; let mut extrinsics = Vec::new(); let mut estimated_bundle_weight = Weight::default(); let mut bundle_size = 0u32; for pending_tx in pending_iterator { - let pending_tx_data = pending_tx.data().clone(); + let pending_tx_data = pending_tx.data(); - let should_select_this_tx = tx_selector - .is_within_tx_range(parent_hash, pending_tx_data.clone()) + let should_select_this_tx = self + .client + .runtime_api() + .is_within_tx_range(parent_hash, pending_tx_data, &bundle_vrf_hash, &tx_range) .map_err(|err| { tracing::error!( ?err, @@ -123,7 +126,7 @@ where let tx_weight = self .client .runtime_api() - .extrinsic_weight(parent_hash, &pending_tx_data) + .extrinsic_weight(parent_hash, pending_tx_data) .map_err(|error| { sp_blockchain::Error::Application(Box::from(format!( "Error getting extrinsic weight: {error}" @@ -142,7 +145,7 @@ where estimated_bundle_weight = next_estimated_bundle_weight; bundle_size = next_bundle_size; - extrinsics.push(pending_tx_data); + extrinsics.push(pending_tx_data.clone()); } } diff --git a/domains/client/domain-operator/src/lib.rs b/domains/client/domain-operator/src/lib.rs index 573da9d4f2..e957b97f9b 100644 --- a/domains/client/domain-operator/src/lib.rs +++ b/domains/client/domain-operator/src/lib.rs @@ -73,7 +73,6 @@ mod domain_worker_starter; mod fraud_proof; mod operator; mod parent_chain; -mod sortition; #[cfg(test)] mod tests; mod utils; diff --git a/domains/client/domain-operator/src/sortition.rs b/domains/client/domain-operator/src/sortition.rs deleted file mode 100644 index d2f1f15735..0000000000 --- a/domains/client/domain-operator/src/sortition.rs +++ /dev/null @@ -1,301 +0,0 @@ -//! Domain transaction sortition related logic. - -use codec::Encode; -use domain_runtime_primitives::DomainCoreApi; -use sp_api::ProvideRuntimeApi; -use sp_runtime::traits::Block as BlockT; -use std::sync::Arc; -use subspace_core_primitives::crypto::blake2b_256_hash; -use subspace_core_primitives::{bidirectional_distance, U256}; - -/// Transaction sortition for inclusion in a proposed bundle. -pub(crate) struct TransactionSelector { - /// VRF signature hash from the proof of election. - pub bundle_vrf_hash: U256, - - /// Current tx range. - pub tx_range: U256, - - /// Runtime API. - pub client: Arc, - - _p: std::marker::PhantomData, -} - -impl TransactionSelector -where - Block: BlockT, - Client: ProvideRuntimeApi, - Client::Api: DomainCoreApi, -{ - pub(crate) fn new(bundle_vrf_hash: U256, tx_range: U256, client: Arc) -> Self { - Self { - bundle_vrf_hash, - tx_range, - client, - _p: Default::default(), - } - } - - /// Checks if the transaction should be selected based on the - /// sortition scheme - pub(crate) fn is_within_tx_range( - &self, - at: Block::Hash, - tx: Block::Extrinsic, - ) -> Result { - let maybe_signer_id_hash = self - .client - .runtime_api() - .extract_signer(at, vec![tx])? - .into_iter() - .next() - .and_then(|(maybe_signer, _)| { - maybe_signer.map(|signer| { - let bytes = signer.encode(); - U256::from_be_bytes(blake2b_256_hash(&bytes)) - }) - }); - - if let Some(signer_id_hash) = maybe_signer_id_hash { - // Check if the signer Id hash is within the tx range - Ok(signer_in_tx_range( - &self.bundle_vrf_hash, - &signer_id_hash, - &self.tx_range, - )) - } else { - // Unsigned transactions are always in the range. - Ok(true) - } - } -} - -/// Error type for transaction selection. -#[derive(Debug, thiserror::Error)] -pub enum TransactionSelectError { - #[error(transparent)] - RuntimeApi(#[from] sp_api::ApiError), - - #[error(transparent)] - Blockchain(#[from] sp_blockchain::Error), -} - -/// Checks if the signer Id hash is within the tx range -fn signer_in_tx_range(bundle_vrf_hash: &U256, signer_id_hash: &U256, tx_range: &U256) -> bool { - let distance_from_vrf_hash = bidirectional_distance(bundle_vrf_hash, signer_id_hash); - distance_from_vrf_hash <= (*tx_range / 2) -} - -#[cfg(test)] -mod tests { - use super::signer_in_tx_range; - use num_traits::ops::wrapping::{WrappingAdd, WrappingSub}; - use subspace_core_primitives::U256; - - #[test] - fn test_tx_range() { - let tx_range = U256::MAX / 4; - let bundle_vrf_hash = U256::MAX / 2; - - let signer_id_hash = bundle_vrf_hash + U256::from(10_u64); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::from(10_u64); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash + U256::MAX / 8; - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::MAX / 8; - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash + U256::MAX / 8 + U256::from(1_u64); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::MAX / 8 - U256::from(1_u64); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash + U256::MAX / 4; - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::MAX / 4; - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - } - - #[test] - fn test_tx_range_wrap_under_flow() { - let tx_range = U256::MAX / 4; - let bundle_vrf_hash = U256::from(100_u64); - - let signer_id_hash = bundle_vrf_hash + U256::from(1000_u64); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash.wrapping_sub(&U256::from(1000_u64)); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash + U256::MAX / 8; - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let v = U256::MAX / 8; - let signer_id_hash = bundle_vrf_hash.wrapping_sub(&v); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash + U256::MAX / 8 + U256::from(1_u64); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let v = U256::MAX / 8 + U256::from(1_u64); - let signer_id_hash = bundle_vrf_hash.wrapping_sub(&v); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash + U256::MAX / 4; - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let v = U256::MAX / 4; - let signer_id_hash = bundle_vrf_hash.wrapping_sub(&v); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - } - - #[test] - fn test_tx_range_wrap_over_flow() { - let tx_range = U256::MAX / 4; - let v = U256::MAX; - let bundle_vrf_hash = v.wrapping_sub(&U256::from(100_u64)); - - let signer_id_hash = bundle_vrf_hash.wrapping_add(&U256::from(1000_u64)); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::from(1000_u64); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let v = U256::MAX / 8; - let signer_id_hash = bundle_vrf_hash.wrapping_add(&v); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::MAX / 8; - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let v = U256::MAX / 8 + U256::from(1_u64); - let signer_id_hash = bundle_vrf_hash.wrapping_add(&v); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::MAX / 8 - U256::from(1_u64); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let v = U256::MAX / 4; - let signer_id_hash = bundle_vrf_hash.wrapping_add(&v); - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - - let signer_id_hash = bundle_vrf_hash - U256::MAX / 4; - assert!(!signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - } - - #[test] - fn test_tx_range_max() { - let tx_range = U256::MAX; - let bundle_vrf_hash = U256::MAX / 2; - - let signer_id_hash = bundle_vrf_hash + U256::from(10_u64); - assert!(signer_in_tx_range( - &bundle_vrf_hash, - &signer_id_hash, - &tx_range - )); - } -} diff --git a/domains/primitives/runtime/Cargo.toml b/domains/primitives/runtime/Cargo.toml index 8deccbd569..b9512a651b 100644 --- a/domains/primitives/runtime/Cargo.toml +++ b/domains/primitives/runtime/Cargo.toml @@ -18,6 +18,7 @@ sp-api = { version = "4.0.0-dev", default-features = false, git = "https://githu sp-core = { version = "21.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } sp-runtime = { version = "24.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } sp-std = { version = "8.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } +subspace-core-primitives = { version = "0.1.0", path = "../../../crates/subspace-core-primitives", default-features = false } subspace-runtime-primitives = { version = "0.1.0", path = "../../../crates/subspace-runtime-primitives", default-features = false } sp-weights = { version = "20.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } @@ -30,5 +31,6 @@ std = [ "sp-core/std", "sp-runtime/std", "sp-std/std", + "subspace-core-primitives/std", "subspace-runtime-primitives/std", ] diff --git a/domains/primitives/runtime/src/lib.rs b/domains/primitives/runtime/src/lib.rs index f62a40f1c1..9a1dea2399 100644 --- a/domains/primitives/runtime/src/lib.rs +++ b/domains/primitives/runtime/src/lib.rs @@ -25,6 +25,7 @@ use sp_runtime::transaction_validity::TransactionValidityError; use sp_runtime::{MultiAddress, MultiSignature}; use sp_std::vec::Vec; use sp_weights::Weight; +use subspace_core_primitives::U256; use subspace_runtime_primitives::Moment; /// Alias to 512-bit hash when used in the context of a transaction signature on the chain. @@ -163,6 +164,12 @@ sp_api::decl_runtime_apis! { extrinsics: Vec<::Extrinsic>, ) -> Vec<(Option, ::Extrinsic)>; + fn is_within_tx_range( + extrinsic: &::Extrinsic, + bundle_vrf_hash: &U256, + tx_range: &U256, + ) -> bool; + /// Returns the intermediate storage roots in an encoded form. fn intermediate_roots() -> Vec<[u8; 32]>; diff --git a/domains/runtime/evm/Cargo.toml b/domains/runtime/evm/Cargo.toml index 53479b2880..c2f69ccf29 100644 --- a/domains/runtime/evm/Cargo.toml +++ b/domains/runtime/evm/Cargo.toml @@ -60,6 +60,7 @@ sp-session = { version = "4.0.0-dev", default-features = false, git = "https://g sp-std = { version = "8.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } sp-transaction-pool = { version = "4.0.0-dev", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } sp-version = { version = "22.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } +subspace-core-primitives = { version = "0.1.0", path = "../../../crates/subspace-core-primitives", default-features = false } subspace-runtime-primitives = { version = "0.1.0", path = "../../../crates/subspace-runtime-primitives", default-features = false } [build-dependencies] @@ -110,6 +111,7 @@ std = [ "sp-std/std", "sp-transaction-pool/std", "sp-version/std", + "subspace-core-primitives/std", "subspace-runtime-primitives/std", "substrate-wasm-builder", ] diff --git a/domains/runtime/evm/src/lib.rs b/domains/runtime/evm/src/lib.rs index 8dd988e86e..b888153bf4 100644 --- a/domains/runtime/evm/src/lib.rs +++ b/domains/runtime/evm/src/lib.rs @@ -600,27 +600,36 @@ fn extract_xdm_proof_state_roots( } } -pub fn extract_signers( - extrinsics: Vec, +fn extract_signer_inner( + ext: &UncheckedExtrinsic, lookup: &Lookup, -) -> Vec<(Option, UncheckedExtrinsic)> +) -> Option where Lookup: sp_runtime::traits::Lookup, { + if ext.0.function.is_self_contained() { + ext.0 + .function + .check_self_contained() + .and_then(|signed_info| signed_info.ok()) + .map(|account| account.encode()) + } else { + ext.0 + .signature + .as_ref() + .and_then(|(signed, _, _)| lookup.lookup(*signed).ok().map(|account| account.encode())) + } +} + +pub fn extract_signer( + extrinsics: Vec, +) -> Vec<(Option, UncheckedExtrinsic)> { + let lookup = frame_system::ChainContext::::default(); + let mut signer_extrinsics = Vec::with_capacity(extrinsics.len()); - for ext in extrinsics { - let maybe_signer = if ext.0.function.is_self_contained() { - ext.0 - .function - .check_self_contained() - .and_then(|signed_info| signed_info.ok()) - .map(|account| account.encode()) - } else { - ext.0.signature.as_ref().and_then(|(signed, _, _)| { - lookup.lookup(*signed).ok().map(|account| account.encode()) - }) - }; - signer_extrinsics.push((maybe_signer, ext)); + for extrinsic in extrinsics { + let maybe_signer = extract_signer_inner(&extrinsic, &lookup); + signer_extrinsics.push((maybe_signer, extrinsic)); } signer_extrinsics @@ -735,8 +744,26 @@ impl_runtime_apis! { fn extract_signer( extrinsics: Vec<::Extrinsic>, ) -> Vec<(Option, ::Extrinsic)> { + extract_signer(extrinsics) + } + + fn is_within_tx_range( + extrinsic: &::Extrinsic, + bundle_vrf_hash: &subspace_core_primitives::U256, + tx_range: &subspace_core_primitives::U256 + ) -> bool { + use subspace_core_primitives::U256; + use subspace_core_primitives::crypto::blake2b_256_hash; + let lookup = frame_system::ChainContext::::default(); - extract_signers(extrinsics, &lookup) + if let Some(signer) = extract_signer_inner(extrinsic, &lookup) { + // Check if the signer Id hash is within the tx range + let signer_id_hash = U256::from_be_bytes(blake2b_256_hash(&signer.encode())); + sp_domains::signer_in_tx_range(bundle_vrf_hash, &signer_id_hash, tx_range) + } else { + // Unsigned transactions are always in the range. + true + } } fn intermediate_roots() -> Vec<[u8; 32]> { diff --git a/domains/test/runtime/evm/Cargo.toml b/domains/test/runtime/evm/Cargo.toml index 3e587f1f44..7fa7eaf825 100644 --- a/domains/test/runtime/evm/Cargo.toml +++ b/domains/test/runtime/evm/Cargo.toml @@ -61,6 +61,7 @@ sp-session = { version = "4.0.0-dev", default-features = false, git = "https://g sp-std = { version = "8.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } sp-transaction-pool = { version = "4.0.0-dev", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } sp-version = { version = "22.0.0", default-features = false, git = "https://github.com/subspace/substrate", rev = "55c157cff49b638a59d81a9f971f0f9a66829c71" } +subspace-core-primitives = { version = "0.1.0", path = "../../../../crates/subspace-core-primitives", default-features = false } subspace-runtime-primitives = { version = "0.1.0", path = "../../../../crates/subspace-runtime-primitives", default-features = false } [build-dependencies] @@ -112,6 +113,7 @@ std = [ "sp-std/std", "sp-transaction-pool/std", "sp-version/std", + "subspace-core-primitives/std", "subspace-runtime-primitives/std", "substrate-wasm-builder", ] diff --git a/domains/test/runtime/evm/src/lib.rs b/domains/test/runtime/evm/src/lib.rs index ac0a929ee5..3429d5bc4c 100644 --- a/domains/test/runtime/evm/src/lib.rs +++ b/domains/test/runtime/evm/src/lib.rs @@ -601,27 +601,36 @@ fn extract_xdm_proof_state_roots( } } -pub fn extract_signers( - extrinsics: Vec, +fn extract_signer_inner( + ext: &UncheckedExtrinsic, lookup: &Lookup, -) -> Vec<(Option, UncheckedExtrinsic)> +) -> Option where Lookup: sp_runtime::traits::Lookup, { + if ext.0.function.is_self_contained() { + ext.0 + .function + .check_self_contained() + .and_then(|signed_info| signed_info.ok()) + .map(|account| account.encode()) + } else { + ext.0 + .signature + .as_ref() + .and_then(|(signed, _, _)| lookup.lookup(*signed).ok().map(|account| account.encode())) + } +} + +pub fn extract_signer( + extrinsics: Vec, +) -> Vec<(Option, UncheckedExtrinsic)> { + let lookup = frame_system::ChainContext::::default(); + let mut signer_extrinsics = Vec::with_capacity(extrinsics.len()); - for ext in extrinsics { - let maybe_signer = if ext.0.function.is_self_contained() { - ext.0 - .function - .check_self_contained() - .and_then(|signed_info| signed_info.ok()) - .map(|account| account.encode()) - } else { - ext.0.signature.as_ref().and_then(|(signed, _, _)| { - lookup.lookup(*signed).ok().map(|account| account.encode()) - }) - }; - signer_extrinsics.push((maybe_signer, ext)); + for extrinsic in extrinsics { + let maybe_signer = extract_signer_inner(&extrinsic, &lookup); + signer_extrinsics.push((maybe_signer, extrinsic)); } signer_extrinsics @@ -736,8 +745,24 @@ impl_runtime_apis! { fn extract_signer( extrinsics: Vec<::Extrinsic>, ) -> Vec<(Option, ::Extrinsic)> { + extract_signer(extrinsics) + } + + fn is_within_tx_range( + extrinsic: &::Extrinsic, + bundle_vrf_hash: &subspace_core_primitives::U256, + tx_range: &subspace_core_primitives::U256 + ) -> bool { + use subspace_core_primitives::U256; + use subspace_core_primitives::crypto::blake2b_256_hash; + let lookup = frame_system::ChainContext::::default(); - extract_signers(extrinsics, &lookup) + if let Some(signer) = extract_signer_inner(extrinsic, &lookup) { + let signer_id_hash = U256::from_be_bytes(blake2b_256_hash(&signer.encode())); + sp_domains::signer_in_tx_range(bundle_vrf_hash, &signer_id_hash, tx_range) + } else { + true + } } fn intermediate_roots() -> Vec<[u8; 32]> { From 15a14e3b6bce37311f9cc4128a307245e4367f67 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 26 Jul 2023 13:29:18 +0800 Subject: [PATCH 4/7] Refactor `compile_own_domain_bundles` and start filtering out invalid bundles - Make compile_own_domain_bundles() as a method of DomainBlockPreprocessor - Remove invalid bundles in the process of compiling bundles to extrinsics --- domains/client/block-preprocessor/src/lib.rs | 109 ++++++++++++------ .../domain-operator/src/bundle_processor.rs | 3 +- 2 files changed, 76 insertions(+), 36 deletions(-) diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index 22dc22593d..7eb79d502b 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -21,6 +21,7 @@ use crate::runtime_api::{SetCodeConstructor, SignerExtractor, StateRootExtractor use crate::xdm_verifier::verify_xdm_with_consensus_client; use codec::{Decode, Encode}; use domain_runtime_primitives::opaque::AccountId; +use domain_runtime_primitives::DomainCoreApi; use rand::seq::SliceRandom; use rand::SeedableRng; use rand_chacha::ChaCha8Rng; @@ -28,14 +29,17 @@ use runtime_api::InherentExtrinsicConstructor; use sc_client_api::BlockBackend; use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; -use sp_domains::{DomainId, DomainsApi, DomainsDigestItem, ExtrinsicsRoot, OpaqueBundles}; +use sp_domains::{ + DomainId, DomainsApi, DomainsDigestItem, ExtrinsicsRoot, OpaqueBundle, OpaqueBundles, +}; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use std::borrow::Cow; use std::collections::{BTreeMap, VecDeque}; use std::fmt::Debug; use std::marker::PhantomData; use std::sync::Arc; -use subspace_core_primitives::Randomness; +use subspace_core_primitives::crypto::blake2b_256_hash; +use subspace_core_primitives::{Randomness, U256}; use subspace_runtime_primitives::Balance; type MaybeNewRuntime = Option>; @@ -103,34 +107,6 @@ where Ok((extrinsics, shuffling_seed, maybe_new_runtime)) } -fn compile_own_domain_bundles( - bundles: OpaqueBundles, Block::Hash, Balance>, -) -> Vec -where - Block: BlockT, - CBlock: BlockT, -{ - bundles - .into_iter() - .flat_map(|bundle| { - bundle.extrinsics.into_iter().filter_map(|opaque_extrinsic| { - match <::Extrinsic>::decode( - &mut opaque_extrinsic.encode().as_slice(), - ) { - Ok(uxt) => Some(uxt), - Err(e) => { - tracing::error!( - error = ?e, - "Failed to decode the opaque extrisic in bundle, this should not happen" - ); - None - } - } - }) - }) - .collect::>() -} - fn deduplicate_and_shuffle_extrinsics( parent_hash: Block::Hash, signer_extractor: &SE, @@ -222,19 +198,21 @@ pub struct PreprocessResult { pub extrinsics_roots: Vec, } -pub struct DomainBlockPreprocessor { +pub struct DomainBlockPreprocessor { domain_id: DomainId, + client: Arc, consensus_client: Arc, runtime_api: RuntimeApi, _phantom_data: PhantomData<(Block, CBlock)>, } -impl Clone - for DomainBlockPreprocessor +impl Clone + for DomainBlockPreprocessor { fn clone(&self) -> Self { Self { domain_id: self.domain_id, + client: self.client.clone(), consensus_client: self.consensus_client.clone(), runtime_api: self.runtime_api.clone(), _phantom_data: self._phantom_data, @@ -242,7 +220,8 @@ impl Clone } } -impl DomainBlockPreprocessor +impl + DomainBlockPreprocessor where Block: BlockT, CBlock: BlockT, @@ -252,6 +231,8 @@ where + StateRootExtractor + SetCodeConstructor + InherentExtrinsicConstructor, + Client: ProvideRuntimeApi + 'static, + Client::Api: DomainCoreApi, CClient: HeaderBackend + BlockBackend + ProvideRuntimeApi @@ -262,11 +243,13 @@ where { pub fn new( domain_id: DomainId, + client: Arc, consensus_client: Arc, runtime_api: RuntimeApi, ) -> Self { Self { domain_id, + client, consensus_client, runtime_api, _phantom_data: Default::default(), @@ -299,7 +282,7 @@ where .map(|bundle| bundle.extrinsics_root()) .collect(); - let extrinsics = compile_own_domain_bundles::(bundles); + let extrinsics = self.compile_own_domain_bundles(bundles, domain_hash); let extrinsics_in_bundle = deduplicate_and_shuffle_extrinsics( domain_hash, @@ -337,6 +320,62 @@ where })) } + fn is_valid_bundle( + &self, + bundle: &OpaqueBundle< + NumberFor, + CBlock::Hash, + NumberFor, + Block::Hash, + Balance, + >, + at: Block::Hash, + ) -> bool { + for opaque_extrinsic in &bundle.extrinsics { + let Ok(extrinsic) = + <::Extrinsic>::decode(&mut opaque_extrinsic.encode().as_slice()) + else { + return false; + }; + + // check tx range + + // check tx validity + } + + true + } + + fn compile_own_domain_bundles( + &self, + bundles: OpaqueBundles, Block::Hash, Balance>, + at: Block::Hash, + ) -> Vec { + bundles + .into_iter() + .filter(|bundle| self.is_valid_bundle(bundle, at)) + .flat_map(|bundle| { + bundle + .extrinsics + .into_iter() + .filter_map(|opaque_extrinsic| { + match <::Extrinsic>::decode( + &mut opaque_extrinsic.encode().as_slice(), + ) { + Ok(uxt) => Some(uxt), + Err(e) => { + tracing::error!( + error = ?e, + "Failed to decode the opaque extrisic in bundle, this should not happen" + ); + None + } + } + }) + }) + .collect::>() + } + fn filter_invalid_xdm_extrinsics( &self, at: Block::Hash, diff --git a/domains/client/domain-operator/src/bundle_processor.rs b/domains/client/domain-operator/src/bundle_processor.rs index 689efd25b0..72bbf2eb76 100644 --- a/domains/client/domain-operator/src/bundle_processor.rs +++ b/domains/client/domain-operator/src/bundle_processor.rs @@ -41,7 +41,7 @@ where keystore: KeystorePtr, domain_receipts_checker: DomainReceiptsChecker, domain_block_preprocessor: - DomainBlockPreprocessor>, + DomainBlockPreprocessor>, domain_block_processor: DomainBlockProcessor, } @@ -109,6 +109,7 @@ where ) -> Self { let domain_block_preprocessor = DomainBlockPreprocessor::new( domain_id, + client.clone(), consensus_client.clone(), RuntimeApiFull::new(client.clone()), ); From 15abca76991da48eef5385ad69fd43b1ccdec023 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 26 Jul 2023 19:40:15 +0800 Subject: [PATCH 5/7] Mark the bundle as invalid if any tx in it is out of range or illegal --- domains/client/block-preprocessor/src/lib.rs | 50 ++++++++++++++++---- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index 7eb79d502b..9ba4f9bb70 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -38,7 +38,6 @@ use std::collections::{BTreeMap, VecDeque}; use std::fmt::Debug; use std::marker::PhantomData; use std::sync::Arc; -use subspace_core_primitives::crypto::blake2b_256_hash; use subspace_core_primitives::{Randomness, U256}; use subspace_runtime_primitives::Balance; @@ -282,7 +281,8 @@ where .map(|bundle| bundle.extrinsics_root()) .collect(); - let extrinsics = self.compile_own_domain_bundles(bundles, domain_hash); + let extrinsics = + self.compile_own_domain_bundles(bundles, consensus_block_hash, domain_hash); let extrinsics_in_bundle = deduplicate_and_shuffle_extrinsics( domain_hash, @@ -329,31 +329,65 @@ where Block::Hash, Balance, >, + consensus_block_hash: CBlock::Hash, at: Block::Hash, - ) -> bool { + ) -> sp_blockchain::Result { + let bundle_vrf_hash = + U256::from_be_bytes(bundle.sealed_header.header.proof_of_election.vrf_hash()); + let tx_range = self + .consensus_client + .runtime_api() + .domain_tx_range(consensus_block_hash, self.domain_id) + .map_err(|error| { + sp_blockchain::Error::Application(Box::from(format!( + "Error getting tx range: {error}" + ))) + })?; + for opaque_extrinsic in &bundle.extrinsics { let Ok(extrinsic) = <::Extrinsic>::decode(&mut opaque_extrinsic.encode().as_slice()) else { - return false; + return Ok(false); }; - // check tx range + let is_within_tx_range = self.client.runtime_api().is_within_tx_range( + at, + &extrinsic, + &bundle_vrf_hash, + &tx_range, + )?; + + if !is_within_tx_range { + return Ok(false); + } - // check tx validity + let is_legal_tx = self + .client + .runtime_api() + .check_transaction_validity(at, extrinsic, at)? + .is_ok(); + + if !is_legal_tx { + return Ok(false); + } } - true + Ok(true) } fn compile_own_domain_bundles( &self, bundles: OpaqueBundles, Block::Hash, Balance>, + consensus_block_hash: CBlock::Hash, at: Block::Hash, ) -> Vec { bundles .into_iter() - .filter(|bundle| self.is_valid_bundle(bundle, at)) + .filter(|bundle| { + self.is_valid_bundle(bundle, consensus_block_hash, at) + .unwrap_or(false) + }) .flat_map(|bundle| { bundle .extrinsics From 335751b55a60a6a668b4bdb7d0bf2e7521b656c9 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Wed, 26 Jul 2023 20:44:09 +0800 Subject: [PATCH 6/7] Tiny refactorings No logical changes --- domains/client/block-preprocessor/src/lib.rs | 80 +++++++++---------- .../src/domain_bundle_proposer.rs | 4 +- domains/runtime/evm/src/lib.rs | 14 ++-- domains/test/runtime/evm/src/lib.rs | 14 ++-- 4 files changed, 54 insertions(+), 58 deletions(-) diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index 9ba4f9bb70..c49a4fdc0c 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -282,7 +282,7 @@ where .collect(); let extrinsics = - self.compile_own_domain_bundles(bundles, consensus_block_hash, domain_hash); + self.compile_bundles_to_extrinsics(bundles, consensus_block_hash, domain_hash); let extrinsics_in_bundle = deduplicate_and_shuffle_extrinsics( domain_hash, @@ -320,6 +320,34 @@ where })) } + /// Filter out the invalid bundles first and then convert the remaining valid ones to + /// a list of extrinsics. + fn compile_bundles_to_extrinsics( + &self, + bundles: OpaqueBundles, Block::Hash, Balance>, + consensus_block_hash: CBlock::Hash, + at: Block::Hash, + ) -> Vec { + bundles + .into_iter() + .filter(|bundle| { + self.is_valid_bundle(bundle, consensus_block_hash, at) + .map_err(|err| { + tracing::error!(?err, "Error occurred in checking bundle validity"); + }) + .unwrap_or(false) + }) + .flat_map(|valid_bundle| { + valid_bundle.extrinsics.into_iter().map(|opaque_extrinsic| { + <::Extrinsic>::decode( + &mut opaque_extrinsic.encode().as_slice(), + ) + .expect("Must succeed as it was decoded successfully before; qed") + }) + }) + .collect::>() + } + fn is_valid_bundle( &self, bundle: &OpaqueBundle< @@ -331,23 +359,23 @@ where >, consensus_block_hash: CBlock::Hash, at: Block::Hash, - ) -> sp_blockchain::Result { + ) -> Result { let bundle_vrf_hash = U256::from_be_bytes(bundle.sealed_header.header.proof_of_election.vrf_hash()); let tx_range = self .consensus_client .runtime_api() - .domain_tx_range(consensus_block_hash, self.domain_id) - .map_err(|error| { - sp_blockchain::Error::Application(Box::from(format!( - "Error getting tx range: {error}" - ))) - })?; + .domain_tx_range(consensus_block_hash, self.domain_id)?; for opaque_extrinsic in &bundle.extrinsics { let Ok(extrinsic) = <::Extrinsic>::decode(&mut opaque_extrinsic.encode().as_slice()) else { + tracing::error!( + ?opaque_extrinsic, + "Undecodable extrinsic in bundle({})", + bundle.hash() + ); return Ok(false); }; @@ -359,6 +387,7 @@ where )?; if !is_within_tx_range { + // TODO: Generate a fraud proof for this invalid bundle return Ok(false); } @@ -369,6 +398,7 @@ where .is_ok(); if !is_legal_tx { + // TODO: Generate a fraud proof for this invalid bundle return Ok(false); } } @@ -376,40 +406,6 @@ where Ok(true) } - fn compile_own_domain_bundles( - &self, - bundles: OpaqueBundles, Block::Hash, Balance>, - consensus_block_hash: CBlock::Hash, - at: Block::Hash, - ) -> Vec { - bundles - .into_iter() - .filter(|bundle| { - self.is_valid_bundle(bundle, consensus_block_hash, at) - .unwrap_or(false) - }) - .flat_map(|bundle| { - bundle - .extrinsics - .into_iter() - .filter_map(|opaque_extrinsic| { - match <::Extrinsic>::decode( - &mut opaque_extrinsic.encode().as_slice(), - ) { - Ok(uxt) => Some(uxt), - Err(e) => { - tracing::error!( - error = ?e, - "Failed to decode the opaque extrisic in bundle, this should not happen" - ); - None - } - } - }) - }) - .collect::>() - } - fn filter_invalid_xdm_extrinsics( &self, at: Block::Hash, diff --git a/domains/client/domain-operator/src/domain_bundle_proposer.rs b/domains/client/domain-operator/src/domain_bundle_proposer.rs index bd505f765d..1c97aaeec4 100644 --- a/domains/client/domain-operator/src/domain_bundle_proposer.rs +++ b/domains/client/domain-operator/src/domain_bundle_proposer.rs @@ -109,7 +109,7 @@ where for pending_tx in pending_iterator { let pending_tx_data = pending_tx.data(); - let should_select_this_tx = self + let is_within_tx_range = self .client .runtime_api() .is_within_tx_range(parent_hash, pending_tx_data, &bundle_vrf_hash, &tx_range) @@ -122,7 +122,7 @@ where }) .unwrap_or(false); - if should_select_this_tx { + if is_within_tx_range { let tx_weight = self .client .runtime_api() diff --git a/domains/runtime/evm/src/lib.rs b/domains/runtime/evm/src/lib.rs index b888153bf4..a4c180ad66 100644 --- a/domains/runtime/evm/src/lib.rs +++ b/domains/runtime/evm/src/lib.rs @@ -626,13 +626,13 @@ pub fn extract_signer( ) -> Vec<(Option, UncheckedExtrinsic)> { let lookup = frame_system::ChainContext::::default(); - let mut signer_extrinsics = Vec::with_capacity(extrinsics.len()); - for extrinsic in extrinsics { - let maybe_signer = extract_signer_inner(&extrinsic, &lookup); - signer_extrinsics.push((maybe_signer, extrinsic)); - } - - signer_extrinsics + extrinsics + .into_iter() + .map(|extrinsic| { + let maybe_signer = extract_signer_inner(&extrinsic, &lookup); + (maybe_signer, extrinsic) + }) + .collect() } impl_runtime_apis! { diff --git a/domains/test/runtime/evm/src/lib.rs b/domains/test/runtime/evm/src/lib.rs index 3429d5bc4c..671b579c15 100644 --- a/domains/test/runtime/evm/src/lib.rs +++ b/domains/test/runtime/evm/src/lib.rs @@ -627,13 +627,13 @@ pub fn extract_signer( ) -> Vec<(Option, UncheckedExtrinsic)> { let lookup = frame_system::ChainContext::::default(); - let mut signer_extrinsics = Vec::with_capacity(extrinsics.len()); - for extrinsic in extrinsics { - let maybe_signer = extract_signer_inner(&extrinsic, &lookup); - signer_extrinsics.push((maybe_signer, extrinsic)); - } - - signer_extrinsics + extrinsics + .into_iter() + .map(|extrinsic| { + let maybe_signer = extract_signer_inner(&extrinsic, &lookup); + (maybe_signer, extrinsic) + }) + .collect() } impl_runtime_apis! { From 8db568d45e4f5e58d5e3eb6b7a6f365e1a197242 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Thu, 27 Jul 2023 21:08:22 +0800 Subject: [PATCH 7/7] Apply review suggestion --- domains/client/block-preprocessor/src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index c49a4fdc0c..f87f75016c 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -281,8 +281,12 @@ where .map(|bundle| bundle.extrinsics_root()) .collect(); - let extrinsics = - self.compile_bundles_to_extrinsics(bundles, consensus_block_hash, domain_hash); + let tx_range = self + .consensus_client + .runtime_api() + .domain_tx_range(consensus_block_hash, self.domain_id)?; + + let extrinsics = self.compile_bundles_to_extrinsics(bundles, tx_range, domain_hash); let extrinsics_in_bundle = deduplicate_and_shuffle_extrinsics( domain_hash, @@ -325,13 +329,13 @@ where fn compile_bundles_to_extrinsics( &self, bundles: OpaqueBundles, Block::Hash, Balance>, - consensus_block_hash: CBlock::Hash, + tx_range: U256, at: Block::Hash, ) -> Vec { bundles .into_iter() .filter(|bundle| { - self.is_valid_bundle(bundle, consensus_block_hash, at) + self.is_valid_bundle(bundle, &tx_range, at) .map_err(|err| { tracing::error!(?err, "Error occurred in checking bundle validity"); }) @@ -357,15 +361,11 @@ where Block::Hash, Balance, >, - consensus_block_hash: CBlock::Hash, + tx_range: &U256, at: Block::Hash, ) -> Result { let bundle_vrf_hash = U256::from_be_bytes(bundle.sealed_header.header.proof_of_election.vrf_hash()); - let tx_range = self - .consensus_client - .runtime_api() - .domain_tx_range(consensus_block_hash, self.domain_id)?; for opaque_extrinsic in &bundle.extrinsics { let Ok(extrinsic) = @@ -383,7 +383,7 @@ where at, &extrinsic, &bundle_vrf_hash, - &tx_range, + tx_range, )?; if !is_within_tx_range {