From 52c49b3cb3e688fb57e64399ee55b79bb772521d Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 21 Jun 2023 07:58:47 +0800 Subject: [PATCH 1/8] Add domain_number field to ExecutionReceipt Previously we use the ExecutionReceipt::primary_number as the domain_number as every primary block will drive a domain block, but this will be changed and some primary block won't drive a domain block hence the block number not longer match each other, thus we need a domain_number field in ExecutionReceipt for our usage. Signed-off-by: linning --- crates/pallet-domains/src/tests.rs | 1 + crates/sp-domains/src/lib.rs | 3 +++ domains/client/domain-executor/src/aux_schema.rs | 1 + domains/client/domain-executor/src/domain_block_processor.rs | 1 + 4 files changed, 6 insertions(+) diff --git a/crates/pallet-domains/src/tests.rs b/crates/pallet-domains/src/tests.rs index 4789c37c1b..c76bf232b6 100644 --- a/crates/pallet-domains/src/tests.rs +++ b/crates/pallet-domains/src/tests.rs @@ -115,6 +115,7 @@ fn create_dummy_receipt( ExecutionReceipt { primary_number, primary_hash, + domain_number: primary_number, domain_hash: H256::random(), trace: if primary_number == 0 { Vec::new() diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 32448faeda..24a07438ec 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -390,6 +390,8 @@ pub struct ExecutionReceipt { pub primary_number: Number, /// Hash of the origin primary block this receipt corresponds to. pub primary_hash: Hash, + /// Domain block number. + pub domain_number: Number, /// Hash of the domain block this receipt points to. pub domain_hash: DomainHash, /// List of storage roots collected during the domain block execution. @@ -419,6 +421,7 @@ impl ExecutionReceipt Date: Wed, 21 Jun 2023 08:14:11 +0800 Subject: [PATCH 2/8] Skip building domain block if there are no bundle contains in the primary block Signed-off-by: linning --- crates/sp-domains/src/lib.rs | 2 +- domains/client/block-preprocessor/src/lib.rs | 17 +++--- .../domain-executor/src/bundle_processor.rs | 49 ++++++++++------ .../src/domain_block_processor.rs | 57 ++++++++++--------- 4 files changed, 74 insertions(+), 51 deletions(-) diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 24a07438ec..7c9992cef4 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -407,7 +407,7 @@ impl ExecutionReceipt ExecutionReceipt { +impl ExecutionReceipt { #[cfg(any(feature = "std", feature = "runtime-benchmarks"))] pub fn dummy( primary_number: Number, diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index 0754a43447..ca47b2047d 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -273,18 +273,17 @@ where ) -> sp_blockchain::Result>> { // `domain_hash` is unused in `preprocess_primary_block` when using stateless runtime api. let domain_hash = Default::default(); - Ok(self - .preprocess_primary_block(primary_hash, domain_hash)? - .into_iter() - .map(|ext| ext.encode()) - .collect()) + match self.preprocess_primary_block(primary_hash, domain_hash)? { + Some(extrinsics) => Ok(extrinsics.into_iter().map(|ext| ext.encode()).collect()), + None => Ok(Vec::new()), + } } pub fn preprocess_primary_block( &self, primary_hash: PBlock::Hash, domain_hash: Block::Hash, - ) -> sp_blockchain::Result> { + ) -> sp_blockchain::Result>> { let (primary_extrinsics, shuffling_seed, maybe_new_runtime) = prepare_domain_block_elements::( self.domain_id, @@ -297,6 +296,10 @@ where .runtime_api() .extract_successful_bundles(primary_hash, primary_extrinsics)?; + if bundles.is_empty() && maybe_new_runtime.is_none() { + return Ok(None); + } + let extrinsics = compile_own_domain_bundles::(bundles); let extrinsics_in_bundle = deduplicate_and_shuffle_extrinsics( @@ -330,7 +333,7 @@ where extrinsics.push(set_code_extrinsic); } - Ok(extrinsics) + Ok(Some(extrinsics)) } fn filter_invalid_xdm_extrinsics( diff --git a/domains/client/domain-executor/src/bundle_processor.rs b/domains/client/domain-executor/src/bundle_processor.rs index e789f9cc7e..0e065cdd18 100644 --- a/domains/client/domain-executor/src/bundle_processor.rs +++ b/domains/client/domain-executor/src/bundle_processor.rs @@ -11,7 +11,7 @@ use sp_core::traits::CodeExecutor; use sp_domains::{DomainId, ExecutorApi}; use sp_keystore::KeystorePtr; use sp_messenger::MessengerApi; -use sp_runtime::traits::{Block as BlockT, HashFor, One}; +use sp_runtime::traits::{Block as BlockT, HashFor}; use sp_runtime::Digest; use std::sync::Arc; @@ -148,9 +148,12 @@ where let mut domain_parent = initial_parent; for primary_info in primary_imports { - domain_parent = self + if let Some(next_domain_parent) = self .process_bundles_at((primary_info.hash, primary_info.number), domain_parent) - .await?; + .await? + { + domain_parent = next_domain_parent; + } } } @@ -161,7 +164,7 @@ where &self, primary_info: (PBlock::Hash, NumberFor), parent_info: (Block::Hash, NumberFor), - ) -> sp_blockchain::Result<(Block::Hash, NumberFor)> { + ) -> sp_blockchain::Result)>> { let (primary_hash, primary_number) = primary_info; let (parent_hash, parent_number) = parent_info; @@ -170,9 +173,31 @@ where on top of parent block #{parent_number},{parent_hash}" ); - let extrinsics = self + // TODO: Retrieve using consensus chain runtime API + let head_receipt_number = parent_number; + // let head_receipt_number = self + // .primary_chain_client + // .runtime_api() + // .head_receipt_number(primary_hash, self.domain_id)? + // .into(); + + let extrinsics = match self .domain_block_preprocessor - .preprocess_primary_block(primary_hash, parent_hash)?; + .preprocess_primary_block(primary_hash, parent_hash)? + { + Some(exts) => exts, + None => { + tracing::debug!( + "No domain bundle contains in primary block {primary_info:?}, skip building domain block" + ); + self.domain_block_processor.on_domain_block_processed( + primary_hash, + None, + head_receipt_number, + )?; + return Ok(None); + } + }; let domain_block_result = self .domain_block_processor @@ -184,14 +209,6 @@ where ) .await?; - // TODO: Retrieve using consensus chain runtime API - let head_receipt_number = domain_block_result.header_number - One::one(); - // let head_receipt_number = self - // .primary_chain_client - // .runtime_api() - // .head_receipt_number(primary_hash, self.domain_id)? - // .into(); - assert!( domain_block_result.header_number > head_receipt_number, "Consensus chain number must larger than execution chain number by at least 1" @@ -204,13 +221,13 @@ where self.domain_block_processor.on_domain_block_processed( primary_hash, - domain_block_result, + Some(domain_block_result), head_receipt_number, )?; self.domain_receipts_checker .check_state_transition(primary_hash)?; - Ok(built_block_info) + Ok(Some(built_block_info)) } } diff --git a/domains/client/domain-executor/src/domain_block_processor.rs b/domains/client/domain-executor/src/domain_block_processor.rs index 8e8b89856f..fd176f3a66 100644 --- a/domains/client/domain-executor/src/domain_block_processor.rs +++ b/domains/client/domain-executor/src/domain_block_processor.rs @@ -365,36 +365,39 @@ where pub(crate) fn on_domain_block_processed( &self, primary_hash: PBlock::Hash, - domain_block_result: DomainBlockResult, + domain_block_result: Option>, head_receipt_number: NumberFor, ) -> sp_blockchain::Result<()> { - let DomainBlockResult { - header_hash, - header_number: _, - execution_receipt, - } = domain_block_result; - - crate::aux_schema::write_execution_receipt::<_, Block, PBlock>( - &*self.client, - head_receipt_number, - &execution_receipt, - )?; - - crate::aux_schema::track_domain_hash_to_primary_hash( - &*self.client, - header_hash, - primary_hash, - )?; - - // Notify the imported domain block when the receipt processing is done. - let domain_import_notification = DomainBlockImportNotification { - domain_block_hash: header_hash, - primary_block_hash: primary_hash, + let domain_hash = match domain_block_result { + Some(DomainBlockResult { + header_hash, + header_number: _, + execution_receipt, + }) => { + crate::aux_schema::write_execution_receipt::<_, Block, PBlock>( + &*self.client, + head_receipt_number, + &execution_receipt, + )?; + + // Notify the imported domain block when the receipt processing is done. + let domain_import_notification = DomainBlockImportNotification { + domain_block_hash: header_hash, + primary_block_hash: primary_hash, + }; + self.import_notification_sinks.lock().retain(|sink| { + sink.unbounded_send(domain_import_notification.clone()) + .is_ok() + }); + + header_hash + } + None => { + // TODO: No new domain block produced, thus this primary block should map to the same + // domain block as its parent block + unimplemented!() + } }; - self.import_notification_sinks.lock().retain(|sink| { - sink.unbounded_send(domain_import_notification.clone()) - .is_ok() - }); Ok(()) } From 9a09c212bb0db8e37fff9efa85fde5e594bb1701 Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 21 Jun 2023 08:19:23 +0800 Subject: [PATCH 3/8] Introduce auxiliary storage for primary/domain block hash to handle chain fork properly Signed-off-by: linning --- .../client/domain-executor/src/aux_schema.rs | 98 +++++++++++++++++-- .../src/domain_block_processor.rs | 74 ++++++++++---- 2 files changed, 146 insertions(+), 26 deletions(-) diff --git a/domains/client/domain-executor/src/aux_schema.rs b/domains/client/domain-executor/src/aux_schema.rs index 1d26f9fac8..6c9ca226a1 100644 --- a/domains/client/domain-executor/src/aux_schema.rs +++ b/domains/client/domain-executor/src/aux_schema.rs @@ -25,8 +25,26 @@ const BAD_RECEIPT_MISMATCH_INFO: &[u8] = b"bad_receipt_mismatch_info"; const BAD_RECEIPT_NUMBERS: &[u8] = b"bad_receipt_numbers"; /// domain_block_hash => primary_block_hash +/// +/// Updated only when there is a new domain block produced const PRIMARY_HASH: &[u8] = b"primary_hash"; +/// domain_block_hash => latest_primary_block_hash +/// +/// Updated whenever primary block is processed, the `latest_primary_block_hash` is the block +/// hash of the processed primary block, and the `domain_block_hash` is the best hash of domain +/// branch driving from the processed primary block if there is no new domain block produced +/// the same `domain_block_hash` will be inserted with a different `latest_primary_block_hash` +const LATEST_PRIMARY_HASH: &[u8] = b"latest_primary_hash"; + +/// primary_block_hash => best_domain_block_hash +/// +/// Updated whenever primary block is processed, the `best_domain_block_hash` is the best hash +/// of domain branch driving from the processed primary block, if there is no new domain block +/// produced the `best_domain_block_hash` will be the same as the processed primary block's +/// parent block +const BEST_DOMAIN_HASH: &[u8] = b"best_domain_hash"; + /// Prune the execution receipts when they reach this number. const PRUNING_DEPTH: BlockNumber = 1000; @@ -66,6 +84,7 @@ where { let block_number = execution_receipt.primary_number; let primary_hash = execution_receipt.primary_hash; + let domain_hash = execution_receipt.domain_hash; let block_number_key = (EXECUTION_RECEIPT_BLOCK_NUMBER, block_number).encode(); let mut hashes_at_block_number = @@ -109,6 +128,11 @@ where execution_receipt_key(primary_hash).as_slice(), execution_receipt.encode().as_slice(), ), + // TODO: prune the stale mappings. + ( + (PRIMARY_HASH, domain_hash).encode().as_slice(), + primary_hash.encode().as_slice(), + ), ( block_number_key.as_slice(), hashes_at_block_number.encode().as_slice(), @@ -125,6 +149,26 @@ where ) } +/// Load the execution receipt for given domain block hash. +pub(super) fn load_execution_receipt_by_domain_hash( + backend: &Backend, + domain_hash: Hash, +) -> ClientResult>> +where + Backend: AuxStore, + Hash: Encode + Decode, + Number: Decode, + PHash: Encode + Decode, +{ + match primary_hash_for::(backend, domain_hash)? { + Some(primary_block_hash) => load_decode( + backend, + execution_receipt_key(primary_block_hash).as_slice(), + ), + None => Ok(None), + } +} + /// Load the execution receipt for given primary block hash. pub(super) fn load_execution_receipt( backend: &Backend, @@ -142,27 +186,65 @@ where ) } -pub(super) fn track_domain_hash_to_primary_hash( +pub(super) fn track_domain_hash_and_primary_hash( backend: &Backend, - domain_hash: Hash, - primary_hash: PHash, + best_domain_hash: Hash, + latest_primary_hash: PHash, ) -> ClientResult<()> where Backend: AuxStore, - Hash: Encode, + Hash: Clone + Encode, PHash: Encode, { // TODO: prune the stale mappings. backend.insert_aux( - &[( - (PRIMARY_HASH, domain_hash).encode().as_slice(), - primary_hash.encode().as_slice(), - )], + &[ + ( + (LATEST_PRIMARY_HASH, best_domain_hash.clone()) + .encode() + .as_slice(), + latest_primary_hash.encode().as_slice(), + ), + ( + (BEST_DOMAIN_HASH, latest_primary_hash).encode().as_slice(), + best_domain_hash.encode().as_slice(), + ), + ], vec![], ) } +pub(super) fn best_domain_hash_for( + backend: &Backend, + primary_hash: &PHash, +) -> ClientResult> +where + Backend: AuxStore, + Hash: Decode, + PHash: Encode, +{ + load_decode( + backend, + (BEST_DOMAIN_HASH, primary_hash).encode().as_slice(), + ) +} + +pub(super) fn latest_primary_hash_for( + backend: &Backend, + domain_hash: &Hash, +) -> ClientResult> +where + Backend: AuxStore, + Hash: Encode, + PHash: Decode, +{ + load_decode( + backend, + (LATEST_PRIMARY_HASH, domain_hash).encode().as_slice(), + ) +} + pub(super) fn primary_hash_for( backend: &Backend, domain_hash: Hash, diff --git a/domains/client/domain-executor/src/domain_block_processor.rs b/domains/client/domain-executor/src/domain_block_processor.rs index fd176f3a66..15c3d207b5 100644 --- a/domains/client/domain-executor/src/domain_block_processor.rs +++ b/domains/client/domain-executor/src/domain_block_processor.rs @@ -132,11 +132,13 @@ where let best_number = self.client.info().best_number; let primary_hash_for_best_domain_hash = - crate::aux_schema::primary_hash_for(&*self.backend, best_hash)?.ok_or_else(|| { - sp_blockchain::Error::Backend(format!( - "Primary hash for domain hash #{best_number},{best_hash} not found" - )) - })?; + crate::aux_schema::latest_primary_hash_for(&*self.client, &best_hash)?.ok_or_else( + || { + sp_blockchain::Error::Backend(format!( + "Primary hash for domain hash #{best_number},{best_hash} not found" + )) + }, + )?; let primary_from = primary_hash_for_best_domain_hash; let primary_to = primary_hash; @@ -179,19 +181,28 @@ where ); } (false, false) => { - let common_block_number = route.common_block().number.into(); - let parent_header = self - .client - .header(self.client.hash(common_block_number)?.ok_or_else(|| { - sp_blockchain::Error::Backend(format!( - "Header for #{common_block_number} not found" - )) - })?)? - .ok_or_else(|| { + let (common_block_number, common_block_hash) = + (route.common_block().number, route.common_block().hash); + + // Get the domain block that driving from the common primary block and use it as + // the initial domain parent block + let domain_block_hash: Block::Hash = crate::aux_schema::best_domain_hash_for( + &*self.client, + &common_block_hash, + )? + .ok_or_else( + || { sp_blockchain::Error::Backend(format!( - "Header for #{common_block_number} not found" + "Domain hash driving from primary block #{common_block_number},{common_block_hash} not found" )) - })?; + }, + )?; + let parent_header = self.client.header(domain_block_hash)?.ok_or_else(|| { + sp_blockchain::Error::Backend(format!( + "Domain block header for #{:?} not found", + domain_block_hash + )) + })?; Ok(Some(PendingPrimaryBlocks { initial_parent: (parent_header.hash(), *parent_header.number()), @@ -393,12 +404,39 @@ where header_hash } None => { - // TODO: No new domain block produced, thus this primary block should map to the same + // No new domain block produced, thus this primary block should map to the same // domain block as its parent block - unimplemented!() + let primary_header = + self.primary_chain_client + .header(primary_hash)? + .ok_or_else(|| { + sp_blockchain::Error::Backend(format!( + "Primary block header for #{primary_hash:?} not found" + )) + })?; + if !primary_header.number().is_one() { + crate::aux_schema::best_domain_hash_for( + &*self.client, + primary_header.parent_hash(), + )? + .ok_or_else(|| { + sp_blockchain::Error::Backend(format!( + "Domain hash for #{:?} not found", + primary_header.parent_hash() + )) + })? + } else { + self.client.info().genesis_hash + } } }; + crate::aux_schema::track_domain_hash_and_primary_hash( + &*self.client, + domain_hash, + primary_hash, + )?; + Ok(()) } } From 2f3c921223df3f89ceb310c4325088d111a8230f Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 21 Jun 2023 08:26:24 +0800 Subject: [PATCH 4/8] Reset the domain best block according to the new best primary block The domain best block is reset by re-importing the domain block with ForkChoiceStrategy::Custom(true), no sure if this is the best way to do so. Signed-off-by: linning --- .../domain-executor/src/bundle_processor.rs | 31 +++++++++++++++-- .../src/domain_block_processor.rs | 33 +++++++++++-------- .../domain-executor/src/domain_worker.rs | 13 ++++---- .../src/domain_worker_starter.rs | 3 +- .../client/domain-executor/src/executor.rs | 2 +- domains/client/domain-executor/src/lib.rs | 2 ++ domains/client/domain-executor/src/tests.rs | 2 +- domains/client/domain-executor/src/utils.rs | 2 ++ 8 files changed, 62 insertions(+), 26 deletions(-) diff --git a/domains/client/domain-executor/src/bundle_processor.rs b/domains/client/domain-executor/src/bundle_processor.rs index 0e065cdd18..1f479acc4b 100644 --- a/domains/client/domain-executor/src/bundle_processor.rs +++ b/domains/client/domain-executor/src/bundle_processor.rs @@ -4,9 +4,10 @@ use domain_block_preprocessor::runtime_api_full::RuntimeApiFull; use domain_block_preprocessor::DomainBlockPreprocessor; use domain_runtime_primitives::{DomainCoreApi, InherentExtrinsicApi}; use sc_client_api::{AuxStore, BlockBackend, Finalizer, StateBackendFor}; -use sc_consensus::BlockImport; +use sc_consensus::{BlockImport, BlockImportParams, ForkChoiceStrategy, StateAction}; use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_blockchain::{HeaderBackend, HeaderMetadata}; +use sp_consensus::BlockOrigin; use sp_core::traits::CodeExecutor; use sp_domains::{DomainId, ExecutorApi}; use sp_keystore::KeystorePtr; @@ -124,9 +125,9 @@ where // TODO: Handle the returned error properly, ref to https://github.com/subspace/subspace/pull/695#discussion_r926721185 pub(crate) async fn process_bundles( self, - primary_info: (PBlock::Hash, NumberFor), + primary_info: (PBlock::Hash, NumberFor, bool), ) -> sp_blockchain::Result<()> { - let (primary_hash, primary_number) = primary_info; + let (primary_hash, primary_number, is_new_best) = primary_info; tracing::debug!("Processing imported primary block #{primary_number},{primary_hash}"); @@ -155,6 +156,30 @@ where domain_parent = next_domain_parent; } } + + // The domain branch driving from the best primary branch should also be the best domain branch even + // if it is no the longest domain branch. Thus re-import the tip of the best domain branch to make it + // the new best block if it isn't. + // + // Note: this may cause the best domain fork switch to a shorter fork or in some case the best domain + // block become the ancestor block of the current best block. + let domain_tip = domain_parent.0; + if is_new_best && self.client.info().best_hash != domain_tip { + let header = self.client.header(domain_tip)?.ok_or_else(|| { + sp_blockchain::Error::Backend(format!("Header for #{:?} not found", domain_tip)) + })?; + let block_import_params = { + let mut import_block = BlockImportParams::new(BlockOrigin::Own, header); + import_block.import_existing = true; + import_block.fork_choice = Some(ForkChoiceStrategy::Custom(true)); + import_block.state_action = StateAction::Skip; + import_block + }; + self.domain_block_processor + .import_domain_block(block_import_params) + .await?; + assert_eq!(domain_tip, self.client.info().best_hash); + } } Ok(()) diff --git a/domains/client/domain-executor/src/domain_block_processor.rs b/domains/client/domain-executor/src/domain_block_processor.rs index 15c3d207b5..1c559d07ba 100644 --- a/domains/client/domain-executor/src/domain_block_processor.rs +++ b/domains/client/domain-executor/src/domain_block_processor.rs @@ -219,20 +219,11 @@ where extrinsics: Vec, digests: Digest, ) -> Result, sp_blockchain::Error> { - let primary_number = to_number_primitive(primary_number); - - if to_number_primitive(parent_number) + 1 != primary_number { - return Err(sp_blockchain::Error::Application(Box::from(format!( - "Wrong domain parent block #{parent_number},{parent_hash} for \ - primary block #{primary_number},{primary_hash}, the number of new \ - domain block must match the number of corresponding primary block." - )))); - } - // Although the domain block intuitively ought to use the same fork choice // from the corresponding primary block, it's fine to forcibly always use - // the longest chain for simplicity as we manually build all the domain - // branches by literally following the primary chain branches anyway. + // the longest chain for simplicity as we do not know whether or not the + // domain block is the new best block here, and we will manually reset the + // new best domain block to the correct one after processing the primary blocks let fork_choice = ForkChoiceStrategy::LongestChain; let (header_hash, header_number, state_root) = self @@ -287,7 +278,7 @@ where ); let execution_receipt = ExecutionReceipt { - primary_number: primary_number.into(), + primary_number, primary_hash, domain_number: to_number_primitive(header_number).into(), domain_hash: header_hash, @@ -340,6 +331,20 @@ where import_block.fork_choice = Some(fork_choice); import_block }; + self.import_domain_block(block_import_params).await?; + + Ok((header_hash, header_number, state_root)) + } + + pub(crate) async fn import_domain_block( + &self, + block_import_params: BlockImportParams>, + ) -> Result<(), sp_blockchain::Error> { + let (header_number, header_hash, parent_hash) = ( + *block_import_params.header.number(), + block_import_params.header.hash(), + *block_import_params.header.parent_hash(), + ); let import_result = (&*self.block_import) .import_block(block_import_params) @@ -370,7 +375,7 @@ where } } - Ok((header_hash, header_number, state_root)) + Ok(()) } pub(crate) fn on_domain_block_processed( diff --git a/domains/client/domain-executor/src/domain_worker.rs b/domains/client/domain-executor/src/domain_worker.rs index 94d09151ee..7ed2be6ceb 100644 --- a/domains/client/domain-executor/src/domain_worker.rs +++ b/domains/client/domain-executor/src/domain_worker.rs @@ -66,7 +66,7 @@ pub(crate) async fn handle_block_import_notifications< primary_chain_client: &PClient, best_domain_number: NumberFor, processor: ProcessorFn, - mut leaves: Vec<(PBlock::Hash, NumberFor)>, + mut leaves: Vec<(PBlock::Hash, NumberFor, bool)>, mut blocks_importing: BlocksImporting, mut blocks_imported: BlocksImported, primary_block_import_throttling_buffer_size: u32, @@ -79,7 +79,7 @@ pub(crate) async fn handle_block_import_notifications< + BlockchainEvents, PClient::Api: ExecutorApi, ProcessorFn: Fn( - (PBlock::Hash, NumberFor), + (PBlock::Hash, NumberFor, bool), ) -> Pin> + Send>> + Send + Sync @@ -92,11 +92,11 @@ pub(crate) async fn handle_block_import_notifications< let best_domain_number = to_number_primitive(best_domain_number); // Notify about active leaves on startup before starting the loop - for (hash, number) in std::mem::take(&mut leaves) { + for (hash, number, is_new_best) in std::mem::take(&mut leaves) { let _ = active_leaves.insert(hash, number); // Skip the blocks that have been processed by the execution chain. if number > best_domain_number.into() { - if let Err(error) = processor((hash, number)).await { + if let Err(error) = processor((hash, number, is_new_best)).await { tracing::error!(?error, "Failed to process primary block on startup"); // Bring down the service as bundles processor is an essential task. // TODO: more graceful shutdown. @@ -167,6 +167,7 @@ pub(crate) async fn handle_block_import_notifications< hash: header.hash(), parent_hash: *header.parent_hash(), number: *header.number(), + is_new_best: block_imported.is_new_best, }; let _ = block_info_sender.feed(Some(block_info)).await; } @@ -235,7 +236,7 @@ async fn block_imported( where PBlock: BlockT, ProcessorFn: Fn( - (PBlock::Hash, NumberFor), + (PBlock::Hash, NumberFor, bool), ) -> Pin> + Send>> + Send + Sync, @@ -252,7 +253,7 @@ where debug_assert_eq!(block_info.number.saturating_sub(One::one()), number); } - processor((block_info.hash, block_info.number)).await?; + processor((block_info.hash, block_info.number, block_info.is_new_best)).await?; Ok(()) } diff --git a/domains/client/domain-executor/src/domain_worker_starter.rs b/domains/client/domain-executor/src/domain_worker_starter.rs index c6ab3aa2c6..ef0ea098c0 100644 --- a/domains/client/domain-executor/src/domain_worker_starter.rs +++ b/domains/client/domain-executor/src/domain_worker_starter.rs @@ -141,7 +141,8 @@ pub(super) async fn start_worker< hash, parent_hash: _, number, - }| (hash, number), + is_new_best, + }| (hash, number, is_new_best), ) .collect(), Box::pin(block_importing_notification_stream), diff --git a/domains/client/domain-executor/src/executor.rs b/domains/client/domain-executor/src/executor.rs index f39e836fcd..241b3071c0 100644 --- a/domains/client/domain-executor/src/executor.rs +++ b/domains/client/domain-executor/src/executor.rs @@ -231,7 +231,7 @@ where // TODO: Remove this whole method, `self.bundle_processor` as a property and fix // `set_new_code_should_work` test to do an actual runtime upgrade #[doc(hidden)] - pub async fn process_bundles(self, primary_info: (PBlock::Hash, NumberFor)) { + pub async fn process_bundles(self, primary_info: (PBlock::Hash, NumberFor, bool)) { if let Err(err) = self.bundle_processor.process_bundles(primary_info).await { tracing::error!(?primary_info, ?err, "Error at processing bundles."); } diff --git a/domains/client/domain-executor/src/lib.rs b/domains/client/domain-executor/src/lib.rs index a506dcfdb5..bb2e038926 100644 --- a/domains/client/domain-executor/src/lib.rs +++ b/domains/client/domain-executor/src/lib.rs @@ -207,6 +207,7 @@ where hash, parent_hash, number, + is_new_best: false, }) }) .collect::>(); @@ -218,6 +219,7 @@ where hash: best_block.hash(), parent_hash: *best_block.parent_hash(), number: *best_block.number(), + is_new_best: true, }); /// The maximum number of active leaves we forward to the [`Overseer`] on startup. diff --git a/domains/client/domain-executor/src/tests.rs b/domains/client/domain-executor/src/tests.rs index eff5a5f096..1503a093f6 100644 --- a/domains/client/domain-executor/src/tests.rs +++ b/domains/client/domain-executor/src/tests.rs @@ -725,7 +725,7 @@ async fn set_new_code_should_work() { alice .executor .clone() - .process_bundles((primary_hash, primary_number)) + .process_bundles((primary_hash, primary_number, true)) .await; let best_hash = alice.client.info().best_hash; diff --git a/domains/client/domain-executor/src/utils.rs b/domains/client/domain-executor/src/utils.rs index 184236b755..d45fd8d93f 100644 --- a/domains/client/domain-executor/src/utils.rs +++ b/domains/client/domain-executor/src/utils.rs @@ -26,6 +26,8 @@ where pub parent_hash: Block::Hash, /// block's number. pub number: NumberFor, + /// Is this the new best block. + pub is_new_best: bool, } /// Converts a generic block number to a concrete primitive block number. From ff5023b33bf174b6f60bf112778442d0a1e54b20 Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 21 Jun 2023 08:50:43 +0800 Subject: [PATCH 5/8] Clarify the usage of primary_number and domain_number in the executor client When loading receipt we should use the domain_number while finalize block we should use the primary_number, this commit also include other misc changes on the executor client, one notable change is the genesis receipt, it is inevitable because we need bundle to produce domain block and bundle must contains a receipt thus to the #1 domain block we need produce a bundle that contains the genesis receipt. Signed-off-by: linning --- crates/sp-domains/src/lib.rs | 11 +++++++ .../src/domain_block_processor.rs | 11 +++++-- .../src/domain_bundle_producer.rs | 16 +++------ .../src/domain_bundle_proposer.rs | 33 ++++++++++--------- 4 files changed, 43 insertions(+), 28 deletions(-) diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 7c9992cef4..77111eff2c 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -427,6 +427,17 @@ impl ExecutionReceipt ExecutionReceipt { + ExecutionReceipt { + primary_number: Zero::zero(), + primary_hash: primary_genesis_hash, + domain_number: Zero::zero(), + domain_hash: Default::default(), + trace: Default::default(), + trace_root: Default::default(), + } + } } /// List of [`OpaqueBundle`]. diff --git a/domains/client/domain-executor/src/domain_block_processor.rs b/domains/client/domain-executor/src/domain_block_processor.rs index 1c559d07ba..0141bc2dc4 100644 --- a/domains/client/domain-executor/src/domain_block_processor.rs +++ b/domains/client/domain-executor/src/domain_block_processor.rs @@ -235,8 +235,9 @@ where on top of parent block #{parent_number},{parent_hash}" ); - if let Some(to_finalize_block_number) = - header_number.checked_sub(&self.domain_confirmation_depth) + if let Some(to_finalize_block_number) = primary_number + .into() + .checked_sub(&self.domain_confirmation_depth) { if to_finalize_block_number > self.client.info().finalized_number { let to_finalize_block_hash = @@ -548,6 +549,12 @@ where let mut bad_receipts_to_write = vec![]; for execution_receipt in receipts.iter() { + // TODO: check genesis receipt and fix https://github.com/subspace/subspace/issues/1301 to + // produce fraud proof accordingly + if execution_receipt.domain_number.is_zero() { + continue; + } + let primary_block_hash = execution_receipt.primary_hash; let local_receipt = crate::aux_schema::load_execution_receipt::< diff --git a/domains/client/domain-executor/src/domain_bundle_producer.rs b/domains/client/domain-executor/src/domain_bundle_producer.rs index 25e985344e..eb4144b369 100644 --- a/domains/client/domain-executor/src/domain_bundle_producer.rs +++ b/domains/client/domain-executor/src/domain_bundle_producer.rs @@ -159,23 +159,17 @@ where }; let should_skip_slot = { - let primary_block_number = primary_info.1; // TODO: Retrieve using consensus chain runtime API let head_receipt_number = domain_best_number.saturating_sub(One::one()); // let head_receipt_number = self // .parent_chain // .head_receipt_number(self.parent_chain.best_hash())?; - // Receipt for block #0 does not exist, simply skip slot here to bypasss this case and - // make the code cleaner - primary_block_number.is_zero() - // Executor hasn't able to finish the processing of domain block #1. - || domain_best_number.is_zero() - // Executor is lagging behind the receipt chain on its parent chain as another executor - // already processed a block higher than the local best and submitted the receipt to - // the parent chain, we ought to catch up with the primary block processing before - // producing new bundle. - || domain_best_number <= head_receipt_number + // Executor is lagging behind the receipt chain on its parent chain as another executor + // already processed a block higher than the local best and submitted the receipt to + // the parent chain, we ought to catch up with the primary block processing before + // producing new bundle. + !domain_best_number.is_zero() && domain_best_number <= head_receipt_number }; if should_skip_slot { diff --git a/domains/client/domain-executor/src/domain_bundle_proposer.rs b/domains/client/domain-executor/src/domain_bundle_proposer.rs index cf321b5f4a..aeaefee758 100644 --- a/domains/client/domain-executor/src/domain_bundle_proposer.rs +++ b/domains/client/domain-executor/src/domain_bundle_proposer.rs @@ -10,8 +10,8 @@ use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_block_builder::BlockBuilder; use sp_blockchain::HeaderBackend; use sp_consensus_slots::Slot; -use sp_domains::{BundleHeader, BundleSolution}; -use sp_runtime::traits::{BlakeTwo256, Block as BlockT, Hash as HashT, One, Saturating}; +use sp_domains::{BundleHeader, BundleSolution, ExecutionReceipt}; +use sp_runtime::traits::{BlakeTwo256, Block as BlockT, Hash as HashT, One, Saturating, Zero}; use std::marker::PhantomData; use std::sync::Arc; use std::time; @@ -168,16 +168,16 @@ where "Collecting receipts at {parent_chain_block_hash:?}" ); - let load_receipt = |primary_block_hash, block_number| { - crate::aux_schema::load_execution_receipt::< + let load_receipt = |domain_hash, block_number| { + crate::aux_schema::load_execution_receipt_by_domain_hash::< _, Block::Hash, NumberFor, PBlock::Hash, - >(&*self.client, primary_block_hash)? + >(&*self.client, domain_hash)? .ok_or_else(|| { sp_blockchain::Error::Backend(format!( - "Receipt of primary block #{block_number},{primary_block_hash} not found" + "Receipt of domain block #{block_number},{domain_hash} not found" )) }) }; @@ -201,15 +201,18 @@ where let receipt_number = (head_receipt_number + One::one()).min(available_best_receipt_number); - let primary_block_hash = self - .primary_chain_client - .hash(receipt_number.into())? - .ok_or_else(|| { - sp_blockchain::Error::Backend(format!( - "Primary block hash for #{receipt_number:?} not found" - )) - })?; + if receipt_number.is_zero() { + return Ok(ExecutionReceipt::genesis( + self.primary_chain_client.info().genesis_hash, + )); + } + + let domain_hash = self.client.hash(receipt_number)?.ok_or_else(|| { + sp_blockchain::Error::Backend(format!( + "Domain block hash for #{receipt_number:?} not found" + )) + })?; - load_receipt(primary_block_hash, receipt_number) + load_receipt(domain_hash, receipt_number) } } From 0929f4447251fdd49975526d369eefdfac87698f Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 21 Jun 2023 09:08:29 +0800 Subject: [PATCH 6/8] Add test test_domain_block_production This commit also come with some misc change to the test Signed-off-by: linning --- domains/client/domain-executor/src/tests.rs | 214 +++++++++++++++----- test/subspace-test-service/src/lib.rs | 17 ++ 2 files changed, 176 insertions(+), 55 deletions(-) diff --git a/domains/client/domain-executor/src/tests.rs b/domains/client/domain-executor/src/tests.rs index 1503a093f6..1bbe09a6df 100644 --- a/domains/client/domain-executor/src/tests.rs +++ b/domains/client/domain-executor/src/tests.rs @@ -35,10 +35,107 @@ fn number_of(primary_node: &MockPrimaryNode, block_hash: Hash) -> u32 { } #[substrate_test_utils::test(flavor = "multi_thread")] -#[ignore] +async fn test_domain_block_production() { + 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 = MockPrimaryNode::run_mock_primary_node( + 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, &mut ferdie) + .await; + + for i in 0..50 { + let (tx, slot) = if i % 2 == 0 { + // Produce bundle and include it in the primary block hence produce a domain block + let (slot, _) = ferdie.produce_slot_and_wait_for_bundle_submission().await; + // `None` means collect tx from the tx pool + (None, slot) + } else { + // No bundle hence not produce domain block + (Some(vec![]), ferdie.produce_slot()) + }; + ferdie + .produce_block_with_slot_at(slot, ferdie.client.info().best_hash, tx) + .await + .unwrap(); + } + // Domain block only produced when there is bundle contains in the primary block + assert_eq!(ferdie.client.info().best_number, 50); + assert_eq!(alice.client.info().best_number, 25); + + let primary_block_hash = ferdie.client.info().best_hash; + let domain_block_number = alice.client.info().best_number; + let domain_block_hash = alice.client.info().best_hash; + + // Fork A + // Produce 5 more primary blocks and producing domain block for every primary block + produce_blocks!(ferdie, alice, 3).await.unwrap(); + + // Record the block hashes for later use + let mut fork_b_parent_hash = ferdie.client.info().best_hash; + let fork_a_block_hash_3 = alice.client.info().best_hash; + + produce_blocks!(ferdie, alice, 2).await.unwrap(); + assert_eq!(alice.client.info().best_number, domain_block_number + 5); + + // Fork B + // Produce 6 more primary blocks but only producing 3 domain blocks because there are + // more primary block on fork B, it will become the best fork of the domain chain + for _ in 0..3 { + let slot = ferdie.produce_slot(); + fork_b_parent_hash = ferdie + .produce_block_with_slot_at(slot, fork_b_parent_hash, Some(vec![])) + .await + .unwrap(); + } + assert_eq!(alice.client.info().best_number, domain_block_number + 3); + assert_eq!(alice.client.info().best_hash, fork_a_block_hash_3); + + // Fork C + // Produce 10 more primary blocks and do not produce any domain block but because there are + // more primary block on fork C, it will become the best fork of the domain chain + let mut fork_c_parent_hash = primary_block_hash; + for _ in 0..10 { + let slot = ferdie.produce_slot(); + fork_c_parent_hash = ferdie + .produce_block_with_slot_at(slot, fork_c_parent_hash, Some(vec![])) + .await + .unwrap(); + } + // The best block fall back to an ancestor block + assert_eq!(alice.client.info().best_number, domain_block_number); + assert_eq!(alice.client.info().best_hash, domain_block_hash); + + // Simply producing more block on fork C + ferdie.clear_tx_pool().unwrap(); + produce_blocks!(ferdie, alice, 10).await.unwrap(); + assert_eq!(alice.client.info().best_number, domain_block_number + 10); +} + +#[substrate_test_utils::test(flavor = "multi_thread")] async fn collected_receipts_should_be_on_the_same_branch_with_current_best_block() { let directory = TempDir::new().expect("Must be able to create temporary directory"); - let _ = sc_cli::LoggerBuilder::new("runtime=debug").init(); + + let mut builder = sc_cli::LoggerBuilder::new(""); + builder.with_colors(false); + let _ = builder.init(); + let tokio_handle = tokio::runtime::Handle::current(); // Start Ferdie @@ -65,6 +162,14 @@ async fn collected_receipts_should_be_on_the_same_branch_with_current_best_block let best_primary_number = primary_node.client.info().best_number; assert_eq!(best_primary_number, 3); + assert_eq!(alice.client.info().best_number, 3); + let domain_block_2_hash = *alice + .client + .header(alice.client.info().best_hash) + .unwrap() + .unwrap() + .parent_hash(); + let best_header = primary_node .client .header(best_primary_hash) @@ -153,7 +258,11 @@ async fn collected_receipts_should_be_on_the_same_branch_with_current_best_block let new_best_hash = primary_node.client.info().best_hash; let new_best_number = primary_node.client.info().best_number; assert_eq!(new_best_number, 4); - assert_eq!(alice.client.info().best_number, 4); + + // The domain best block should be reverted to #2 because the primary block #3b and #4 do + // not contains any bundles + assert_eq!(alice.client.info().best_number, 2); + assert_eq!(alice.client.info().best_hash, domain_block_2_hash); // Hash of block number #3 is updated to the second fork block 3b. assert_eq!( @@ -354,6 +463,9 @@ async fn test_apply_extrinsic_proof_creation_and_verification_should_work() { test_invalid_state_transition_proof_creation_and_verification(1).await } +// TODO: the test is ignored due to the invalid receipt can not pass the state root check +// in the runtime now, thus can't construct `finalize_block` fraud proof, find a way to +// bypass the check #[substrate_test_utils::test(flavor = "multi_thread")] #[ignore] async fn test_finalize_block_proof_creation_and_verification_should_work() { @@ -420,13 +532,6 @@ async fn test_invalid_state_transition_proof_creation_and_verification( .await .unwrap(); - // Produce one more block but using the same slot to avoid producing new bundle, this step is intend - // to increase the `ProofOfElection::block_number` of the next bundle such that we can skip the runtime - // `state_root` check for the modified `trace` when testing the `finalize_block` proof. - produce_block_with!(ferdie.produce_block_with_slot(slot), alice) - .await - .unwrap(); - // Get a bundle from the txn pool and modify the receipt of the target bundle to an invalid one let (slot, bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await; let original_submit_bundle_tx = bundle_to_tx(bundle.clone().unwrap()); @@ -788,61 +893,62 @@ async fn pallet_domains_unsigned_extrinsics_should_work() { produce_blocks!(ferdie, alice, 1).await.unwrap(); // Get a bundle from alice's tx pool and used as bundle template. - let (_, bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await; + let (slot, bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await; let _bundle_template = bundle.unwrap(); - let _alice_key = Sr25519Keyring::Alice; + let _alice_key = alice.key; // Drop alice in order to control the execution chain by submitting the receipts manually later. drop(alice); - // Wait for 5 blocks to make sure the execution receipts of block 2,3,4,5 are - // able to be written to the database. - produce_blocks!(ferdie, bob, 5).await.unwrap(); + produce_block_with!(ferdie.produce_block_with_slot(slot), bob) + .await + .unwrap(); // let ferdie_client = ferdie.client.clone(); // let create_submit_bundle = |primary_number: BlockNumber| { - // let primary_hash = ferdie_client.hash(primary_number).unwrap().unwrap(); - // let execution_receipt = - // crate::aux_schema::load_execution_receipt(&*bob.backend, primary_hash) - // .expect("Failed to load execution receipt from the local aux_db") - // .unwrap_or_else(|| { - // panic!( - // "The requested execution receipt for block {primary_number} does not exist" - // ) - // }); - - // let mut opaque_bundle = bundle_template.clone(); - // opaque_bundle.sealed_header.header.primary_number = primary_number; - // opaque_bundle.sealed_header.header.primary_hash = primary_hash; - // opaque_bundle.sealed_header.signature = alice_key - // .pair() - // .sign(opaque_bundle.sealed_header.pre_hash().as_ref()) - // .into(); - // opaque_bundle.receipt = execution_receipt; - - // subspace_test_runtime::UncheckedExtrinsic::new_unsigned( - // pallet_domains::Call::submit_bundle { opaque_bundle }.into(), - // ) - // .into() + // let primary_hash = ferdie_client.hash(primary_number).unwrap().unwrap(); + // let execution_receipt = + // crate::aux_schema::load_execution_receipt(&*bob.backend, primary_hash) + // .expect("Failed to load execution receipt from the local aux_db") + // .unwrap_or_else(|| { + // panic!( + // "The requested execution receipt for block {primary_number} does not exist" + // ) + // }); + + // let mut opaque_bundle = bundle_template.clone(); + // opaque_bundle.sealed_header.header.primary_number = primary_number; + // opaque_bundle.sealed_header.header.primary_hash = primary_hash; + // opaque_bundle.sealed_header.signature = alice_key + // .pair() + // .sign(opaque_bundle.sealed_header.pre_hash().as_ref()) + // .into(); + // opaque_bundle.receipt = execution_receipt; + + // subspace_test_runtime::UncheckedExtrinsic::new_unsigned( + // pallet_domains::Call::submit_bundle { opaque_bundle }.into(), + // ) + // .into() // }; // TODO: Unlock once `head_receipt_number` API is usable. // let ferdie_client = ferdie.client.clone(); // let head_receipt_number = || { - // let best_hash = ferdie_client.info().best_hash; - // ferdie_client - // .runtime_api() - // .head_receipt_number(best_hash, DomainId::new(3u32)) - // .expect("Failed to get head receipt number") + // let best_hash = ferdie_client.info().best_hash; + // ferdie_client + // .runtime_api() + // .head_receipt_number(best_hash, DomainId::SYSTEM) + // .expect("Failed to get head receipt number") // }; // ferdie - // .submit_transaction(create_submit_bundle(1)) - // .await - // .unwrap(); + // .submit_transaction(create_submit_bundle(1)) + // .await + // .unwrap(); + // produce_blocks!(ferdie, bob, 1).await.unwrap(); // ferdie - // .submit_transaction(create_submit_bundle(2)) - // .await - // .unwrap(); + // .submit_transaction(create_submit_bundle(2)) + // .await + // .unwrap(); // produce_blocks!(ferdie, bob, 1).await.unwrap(); // assert_eq!(head_receipt_number(), 2); } @@ -972,12 +1078,10 @@ async fn existing_bundle_can_be_resubmitted_to_new_fork() { .produce_block_with_slot_at(slot, parent_hash, Some(vec![])) .await .unwrap(); - produce_block_with!( - ferdie.produce_block_with_slot_at(slot + 1, parent_hash, Some(vec![])), - alice - ) - .await - .unwrap(); + ferdie + .produce_block_with_slot_at(slot + 1, parent_hash, Some(vec![])) + .await + .unwrap(); // Manually remove the retracted block's `submit_bundle_tx` from tx pool ferdie.prune_tx_from_pool(&submit_bundle_tx).await.unwrap(); diff --git a/test/subspace-test-service/src/lib.rs b/test/subspace-test-service/src/lib.rs index cf8fb544cb..c467cd4839 100644 --- a/test/subspace-test-service/src/lib.rs +++ b/test/subspace-test-service/src/lib.rs @@ -527,6 +527,23 @@ impl MockPrimaryNode { .await } + /// Remove all tx from the tx pool + pub fn clear_tx_pool(&self) -> Result<(), Box> { + let txs: Vec<_> = self + .transaction_pool + .ready() + .map(|t| self.transaction_pool.hash_of(&t.data)) + .collect(); + self.transaction_pool + .pool() + .prune_known(&BlockId::Hash(self.client.info().best_hash), txs.as_slice())?; + self.transaction_pool + .pool() + .validated_pool() + .clear_stale(&BlockId::Number(self.client.info().best_number))?; + Ok(()) + } + /// Remove a ready transaction from transaction pool. pub async fn prune_tx_from_pool(&self, tx: &OpaqueExtrinsic) -> Result<(), Box> { self.transaction_pool.pool().prune_known( From 33f7b7c32b0b6462a4e212c4bacbb795d75a4bea Mon Sep 17 00:00:00 2001 From: linning Date: Mon, 3 Jul 2023 19:54:18 +0800 Subject: [PATCH 7/8] Apply review suggestions Signed-off-by: linning --- .../client/domain-executor/src/aux_schema.rs | 22 ++++++++++++------- .../domain-executor/src/bundle_processor.rs | 6 ++--- .../src/domain_block_processor.rs | 10 ++++----- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/domains/client/domain-executor/src/aux_schema.rs b/domains/client/domain-executor/src/aux_schema.rs index 6c9ca226a1..f900ec7163 100644 --- a/domains/client/domain-executor/src/aux_schema.rs +++ b/domains/client/domain-executor/src/aux_schema.rs @@ -31,18 +31,24 @@ const PRIMARY_HASH: &[u8] = b"primary_hash"; /// domain_block_hash => latest_primary_block_hash /// -/// Updated whenever primary block is processed, the `latest_primary_block_hash` is the block -/// hash of the processed primary block, and the `domain_block_hash` is the best hash of domain -/// branch driving from the processed primary block if there is no new domain block produced -/// the same `domain_block_hash` will be inserted with a different `latest_primary_block_hash` +/// It's important to note that a primary block could possibly contain no bundles for a specific domain, +/// leading to the situation where multiple primary blocks could correspond to the same domain block. +/// +/// PrimaryBlock10 --> DomainBlock5 +/// PrimaryBlock11 --> DomainBlock5 +/// PrimaryBlock12 --> DomainBlock5 +/// +/// This mapping is designed to track the most recent primary block that derives the domain block +/// identified by `domain_block_hash`, e.g., Hash(DomainBlock5) => Hash(PrimaryBlock12). const LATEST_PRIMARY_HASH: &[u8] = b"latest_primary_hash"; /// primary_block_hash => best_domain_block_hash /// -/// Updated whenever primary block is processed, the `best_domain_block_hash` is the best hash -/// of domain branch driving from the processed primary block, if there is no new domain block -/// produced the `best_domain_block_hash` will be the same as the processed primary block's -/// parent block +/// This mapping tracks the mapping of a primary block and the corresponding domain block derived +/// until this primary block: +/// - Hash(PrimaryBlock10) => Hash(DomainBlock5) +/// - Hash(PrimaryBlock11) => Hash(DomainBlock5) +/// - Hash(PrimaryBlock12) => Hash(DomainBlock5) const BEST_DOMAIN_HASH: &[u8] = b"best_domain_hash"; /// Prune the execution receipts when they reach this number. diff --git a/domains/client/domain-executor/src/bundle_processor.rs b/domains/client/domain-executor/src/bundle_processor.rs index 1f479acc4b..281090077d 100644 --- a/domains/client/domain-executor/src/bundle_processor.rs +++ b/domains/client/domain-executor/src/bundle_processor.rs @@ -213,9 +213,9 @@ where Some(exts) => exts, None => { tracing::debug!( - "No domain bundle contains in primary block {primary_info:?}, skip building domain block" + "No bundles and runtime upgrade for this domain in primary block {primary_info:?}, skip building domain block" ); - self.domain_block_processor.on_domain_block_processed( + self.domain_block_processor.on_primary_block_processed( primary_hash, None, head_receipt_number, @@ -244,7 +244,7 @@ where domain_block_result.header_number, ); - self.domain_block_processor.on_domain_block_processed( + self.domain_block_processor.on_primary_block_processed( primary_hash, Some(domain_block_result), head_receipt_number, diff --git a/domains/client/domain-executor/src/domain_block_processor.rs b/domains/client/domain-executor/src/domain_block_processor.rs index 0141bc2dc4..d65721bd97 100644 --- a/domains/client/domain-executor/src/domain_block_processor.rs +++ b/domains/client/domain-executor/src/domain_block_processor.rs @@ -184,7 +184,7 @@ where let (common_block_number, common_block_hash) = (route.common_block().number, route.common_block().hash); - // Get the domain block that driving from the common primary block and use it as + // Get the domain block that is derived from the common primary block and use it as // the initial domain parent block let domain_block_hash: Block::Hash = crate::aux_schema::best_domain_hash_for( &*self.client, @@ -193,7 +193,7 @@ where .ok_or_else( || { sp_blockchain::Error::Backend(format!( - "Domain hash driving from primary block #{common_block_number},{common_block_hash} not found" + "Hash of domain block derived from primary block #{common_block_number},{common_block_hash} not found" )) }, )?; @@ -379,7 +379,7 @@ where Ok(()) } - pub(crate) fn on_domain_block_processed( + pub(crate) fn on_primary_block_processed( &self, primary_hash: PBlock::Hash, domain_block_result: Option>, @@ -549,8 +549,8 @@ where let mut bad_receipts_to_write = vec![]; for execution_receipt in receipts.iter() { - // TODO: check genesis receipt and fix https://github.com/subspace/subspace/issues/1301 to - // produce fraud proof accordingly + // Skip check for genesis receipt as it is generated on the domain instantiation by + // the consensus chain. if execution_receipt.domain_number.is_zero() { continue; } From 36fc46621f64e8f57c081decb61b7a0d31dd946d Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 5 Jul 2023 09:08:56 +0800 Subject: [PATCH 8/8] Apply review suggestions Signed-off-by: linning --- .../domain-executor/src/domain_block_processor.rs | 9 ++++++--- domains/client/domain-executor/src/tests.rs | 2 +- test/subspace-test-service/src/lib.rs | 10 +++++++--- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/domains/client/domain-executor/src/domain_block_processor.rs b/domains/client/domain-executor/src/domain_block_processor.rs index d65721bd97..19e17ea204 100644 --- a/domains/client/domain-executor/src/domain_block_processor.rs +++ b/domains/client/domain-executor/src/domain_block_processor.rs @@ -221,9 +221,12 @@ where ) -> Result, sp_blockchain::Error> { // Although the domain block intuitively ought to use the same fork choice // from the corresponding primary block, it's fine to forcibly always use - // the longest chain for simplicity as we do not know whether or not the - // domain block is the new best block here, and we will manually reset the - // new best domain block to the correct one after processing the primary blocks + // the longest chain for simplicity as we manually build the domain branches + // by following the primary chain branches. Due to the possibility of domain + // branch transitioning to a lower fork caused by the change that a primary block + // can possibility produce no domain block, it's important to note that now we + // need to ensure the domain block built from the latest primary block is the + // new best domain block after processing each imported primary block. let fork_choice = ForkChoiceStrategy::LongestChain; let (header_hash, header_number, state_root) = self diff --git a/domains/client/domain-executor/src/tests.rs b/domains/client/domain-executor/src/tests.rs index 1bbe09a6df..28fac98fb0 100644 --- a/domains/client/domain-executor/src/tests.rs +++ b/domains/client/domain-executor/src/tests.rs @@ -123,7 +123,7 @@ async fn test_domain_block_production() { assert_eq!(alice.client.info().best_hash, domain_block_hash); // Simply producing more block on fork C - ferdie.clear_tx_pool().unwrap(); + ferdie.clear_tx_pool().await.unwrap(); produce_blocks!(ferdie, alice, 10).await.unwrap(); assert_eq!(alice.client.info().best_number, domain_block_number + 10); } diff --git a/test/subspace-test-service/src/lib.rs b/test/subspace-test-service/src/lib.rs index c467cd4839..51cb8fab09 100644 --- a/test/subspace-test-service/src/lib.rs +++ b/test/subspace-test-service/src/lib.rs @@ -528,19 +528,23 @@ impl MockPrimaryNode { } /// Remove all tx from the tx pool - pub fn clear_tx_pool(&self) -> Result<(), Box> { + pub async fn clear_tx_pool(&self) -> Result<(), Box> { let txs: Vec<_> = self .transaction_pool .ready() .map(|t| self.transaction_pool.hash_of(&t.data)) .collect(); + let best_block_id = BlockId::Hash(self.client.info().best_hash); self.transaction_pool .pool() - .prune_known(&BlockId::Hash(self.client.info().best_hash), txs.as_slice())?; + .prune_known(&best_block_id, txs.as_slice())?; + // `ban_time` have set to 0, explicitly wait 1ms here to ensure `clear_stale` will remove + // all the bans as the ban time must be passed. + tokio::time::sleep(time::Duration::from_millis(1)).await; self.transaction_pool .pool() .validated_pool() - .clear_stale(&BlockId::Number(self.client.info().best_number))?; + .clear_stale(&best_block_id)?; Ok(()) }