From f609b5f92b11308546966a504044085922ec7315 Mon Sep 17 00:00:00 2001 From: linning Date: Mon, 28 Aug 2023 06:36:16 +0800 Subject: [PATCH 1/5] Add consensus_block_info digest to the domain block header This degist contains the consensus block hash that derive the domain block, it is used in later commits to load ER. This commit also revise the sp-domain-digests to replace the deprecated primary block with consensus block and remove the consensus block number from the digest since it is useless Signed-off-by: linning --- crates/subspace-fraud-proof/src/tests.rs | 6 +++--- .../domain-operator/src/bundle_processor.rs | 9 +++++++-- domains/client/domain-operator/src/tests.rs | 2 +- domains/primitives/digests/src/lib.rs | 15 +++++++-------- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/crates/subspace-fraud-proof/src/tests.rs b/crates/subspace-fraud-proof/src/tests.rs index f85f92ca18..e9b40fd2de 100644 --- a/crates/subspace-fraud-proof/src/tests.rs +++ b/crates/subspace-fraud-proof/src/tests.rs @@ -181,7 +181,7 @@ async fn execution_proof_creation_and_verification_should_work() { let create_block_builder = || { let primary_hash = ferdie.client.hash(*header.number()).unwrap().unwrap(); let digest = Digest { - logs: vec![DigestItem::primary_block_info(( + logs: vec![DigestItem::consensus_block_info(( *header.number(), primary_hash, ))], @@ -218,7 +218,7 @@ async fn execution_proof_creation_and_verification_should_work() { Default::default(), parent_header.hash(), Digest { - logs: vec![DigestItem::primary_block_info(( + logs: vec![DigestItem::consensus_block_info(( *header.number(), primary_hash, ))], @@ -487,7 +487,7 @@ async fn invalid_execution_proof_should_not_work() { *parent_header.number(), RecordProof::No, Digest { - logs: vec![DigestItem::primary_block_info(( + logs: vec![DigestItem::consensus_block_info(( *header.number(), header.hash(), ))], diff --git a/domains/client/domain-operator/src/bundle_processor.rs b/domains/client/domain-operator/src/bundle_processor.rs index fd2501841d..f831382757 100644 --- a/domains/client/domain-operator/src/bundle_processor.rs +++ b/domains/client/domain-operator/src/bundle_processor.rs @@ -11,11 +11,12 @@ use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_blockchain::{HeaderBackend, HeaderMetadata}; use sp_consensus::BlockOrigin; use sp_core::traits::CodeExecutor; +use sp_domain_digests::AsPredigest; use sp_domains::{DomainId, DomainsApi, InvalidReceipt, ReceiptValidity}; use sp_keystore::KeystorePtr; use sp_messenger::MessengerApi; use sp_runtime::traits::{Block as BlockT, HashFor, Zero}; -use sp_runtime::Digest; +use sp_runtime::{Digest, DigestItem}; use std::sync::Arc; type DomainReceiptsChecker = ReceiptsChecker< @@ -291,6 +292,10 @@ where return Ok(None); }; + let digest = Digest { + logs: vec![DigestItem::consensus_block_info(consensus_block_hash)], + }; + let domain_block_result = self .domain_block_processor .process_domain_block( @@ -299,7 +304,7 @@ where extrinsics, invalid_bundles, extrinsics_roots, - Digest::default(), + digest, ) .await?; diff --git a/domains/client/domain-operator/src/tests.rs b/domains/client/domain-operator/src/tests.rs index a4bbfce93f..9af56a7a01 100644 --- a/domains/client/domain-operator/src/tests.rs +++ b/domains/client/domain-operator/src/tests.rs @@ -959,7 +959,7 @@ async fn fraud_proof_verification_in_tx_pool_should_work() { let digest = { Digest { - logs: vec![DigestItem::primary_block_info(( + logs: vec![DigestItem::consensus_block_info(( bad_receipt_number, ferdie.client.hash(bad_receipt_number).unwrap().unwrap(), ))], diff --git a/domains/primitives/digests/src/lib.rs b/domains/primitives/digests/src/lib.rs index 037c28cd1a..482cb37631 100644 --- a/domains/primitives/digests/src/lib.rs +++ b/domains/primitives/digests/src/lib.rs @@ -7,20 +7,19 @@ const DOMAIN_REGISTRY_ENGINE_ID: ConsensusEngineId = *b"RGTR"; /// Trait to provide simpler abstractions to create predigests for runtime. pub trait AsPredigest { - /// Return a pair of (primary_block_number, primary_block_hash). - fn as_primary_block_info(&self) -> Option<(Number, Hash)>; + /// Return `consensus_block_hash` + fn as_consensus_block_info(&self) -> Option; - /// Creates a new digest of primary block info for system domain. - fn primary_block_info(info: (Number, Hash)) -> Self; + /// Creates a new digest of the consensus block that derive the domain block. + fn consensus_block_info(consensus_block_hash: Hash) -> Self; } impl AsPredigest for DigestItem { - /// Return a pair of (primary_block_number, primary_block_hash). - fn as_primary_block_info(&self) -> Option<(Number, Hash)> { + fn as_consensus_block_info(&self) -> Option { self.pre_runtime_try_to(&DOMAIN_REGISTRY_ENGINE_ID) } - fn primary_block_info(info: (Number, Hash)) -> Self { - DigestItem::PreRuntime(DOMAIN_REGISTRY_ENGINE_ID, info.encode()) + fn consensus_block_info(consensus_block_hash: Hash) -> Self { + DigestItem::PreRuntime(DOMAIN_REGISTRY_ENGINE_ID, consensus_block_hash.encode()) } } From 279ac09caa6b0e675cef6603eb1d3b0a288c3a5c Mon Sep 17 00:00:00 2001 From: linning Date: Mon, 28 Aug 2023 06:40:57 +0800 Subject: [PATCH 2/5] Use the consensus_block_info digest of the domain block header to load ER This commit change to the way to load ER from using the domain_hash => consensus_hash mapping to using the consensus hash of the consensus_block_info digest Signed-off-by: linning --- .../src/domain_block_processor.rs | 3 +- .../src/domain_bundle_producer.rs | 6 +- .../src/domain_bundle_proposer.rs | 10 ++-- domains/client/domain-operator/src/lib.rs | 57 +++++++------------ 4 files changed, 31 insertions(+), 45 deletions(-) diff --git a/domains/client/domain-operator/src/domain_block_processor.rs b/domains/client/domain-operator/src/domain_block_processor.rs index 9ddab279a1..ef8a3615f8 100644 --- a/domains/client/domain-operator/src/domain_block_processor.rs +++ b/domains/client/domain-operator/src/domain_block_processor.rs @@ -341,9 +341,8 @@ where *genesis_header.state_root(), ) } else { - crate::load_execution_receipt_by_domain_hash::( + crate::load_execution_receipt_by_domain_hash::( &*self.client, - &self.consensus_client, parent_hash, parent_number, )? diff --git a/domains/client/domain-operator/src/domain_bundle_producer.rs b/domains/client/domain-operator/src/domain_bundle_producer.rs index 85c1e53e1c..943a8ca7fb 100644 --- a/domains/client/domain-operator/src/domain_bundle_producer.rs +++ b/domains/client/domain-operator/src/domain_bundle_producer.rs @@ -149,12 +149,12 @@ where global_randomness, } = slot_info; - let best_receipt_is_written = !crate::aux_schema::consensus_block_hash_for::< + let best_receipt_is_written = crate::aux_schema::latest_consensus_block_hash_for::< _, _, CBlock::Hash, - >(&*self.client, self.client.info().best_hash)? - .is_empty(); + >(&*self.client, &self.client.info().best_hash)? + .is_some(); // TODO: remove once the receipt generation can be done before the domain block is // committed to the database, in other words, only when the receipt of block N+1 has diff --git a/domains/client/domain-operator/src/domain_bundle_proposer.rs b/domains/client/domain-operator/src/domain_bundle_proposer.rs index b16284c635..dae12241de 100644 --- a/domains/client/domain-operator/src/domain_bundle_proposer.rs +++ b/domains/client/domain-operator/src/domain_bundle_proposer.rs @@ -188,12 +188,12 @@ where "Collecting receipts at {parent_chain_block_hash:?}" ); - let header_block_receipt_is_written = !crate::aux_schema::consensus_block_hash_for::< + let header_block_receipt_is_written = crate::aux_schema::latest_consensus_block_hash_for::< _, _, CBlock::Hash, - >(&*self.client, header_hash)? - .is_empty(); + >(&*self.client, &header_hash)? + .is_some(); // TODO: remove once the receipt generation can be done before the domain block is // committed to the database, in other words, only when the receipt of block N+1 has @@ -223,15 +223,15 @@ where )); } + // Get the domain block hash corresponding to `receipt_number` in the domain canonical chain let domain_hash = self.client.hash(receipt_number)?.ok_or_else(|| { sp_blockchain::Error::Backend(format!( "Domain block hash for #{receipt_number:?} not found" )) })?; - crate::load_execution_receipt_by_domain_hash::( + crate::load_execution_receipt_by_domain_hash::( &*self.client, - &self.consensus_client, domain_hash, receipt_number, ) diff --git a/domains/client/domain-operator/src/lib.rs b/domains/client/domain-operator/src/lib.rs index f66ec5eae5..dc29585a84 100644 --- a/domains/client/domain-operator/src/lib.rs +++ b/domains/client/domain-operator/src/lib.rs @@ -90,11 +90,13 @@ use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; use sp_consensus::{SelectChain, SyncOracle}; use sp_consensus_slots::Slot; +use sp_domain_digests::AsPredigest; use sp_domains::{Bundle, DomainId, ExecutionReceipt}; use sp_keystore::KeystorePtr; use sp_runtime::traits::{ Block as BlockT, HashFor, Header as HeaderT, NumberFor, One, Saturating, Zero, }; +use sp_runtime::DigestItem; use std::marker::PhantomData; use std::sync::Arc; use subspace_core_primitives::Randomness; @@ -240,56 +242,41 @@ where Ok(leaves.into_iter().rev().take(MAX_ACTIVE_LEAVES).collect()) } -pub(crate) fn load_execution_receipt_by_domain_hash( +pub(crate) fn load_execution_receipt_by_domain_hash( domain_client: &Client, - consensus_client: &Arc, domain_hash: Block::Hash, domain_number: NumberFor, ) -> Result, sp_blockchain::Error> where Block: BlockT, CBlock: BlockT, - Client: AuxStore, - CClient: HeaderBackend, + Client: AuxStore + HeaderBackend, { - let not_found_error = || { + let domain_header = domain_client.header(domain_hash)?.ok_or_else(|| { sp_blockchain::Error::Backend(format!( - "Receipt for domain block {domain_hash}#{domain_number} not found" + "Header for domain block {domain_hash}#{domain_number} not found" )) - }; + })?; - // Get all the consensus blocks that mapped to `domain_hash` - let consensus_block_hashes = crate::aux_schema::consensus_block_hash_for::<_, _, CBlock::Hash>( - domain_client, - domain_hash, - )?; - - // Get the consensus block that is in the current canonical consensus chain - let consensus_block_hash = match consensus_block_hashes.len() { - 0 => return Err(not_found_error()), - 1 => consensus_block_hashes[0], - _ => { - let mut canonical_consensus_hash = None; - for hash in consensus_block_hashes { - // Check if `hash` is in the canonical chain - let block_number = consensus_client.number(hash)?.ok_or_else(not_found_error)?; - let canonical_block_hash = consensus_client - .hash(block_number)? - .ok_or_else(not_found_error)?; - - if canonical_block_hash == hash { - canonical_consensus_hash.replace(hash); - break; - } - } - canonical_consensus_hash.ok_or_else(not_found_error)? - } - }; + let consensus_block_hash = domain_header + .digest() + .convert_first(DigestItem::as_consensus_block_info) + .ok_or_else(|| { + sp_blockchain::Error::Application(Box::from( + "Domain block header {domain_hash}#{domain_number} missing the consensus \ + block info predigest, this should not happen", + )) + })?; // Get receipt by consensus block hash crate::aux_schema::load_execution_receipt::<_, Block, CBlock>( domain_client, consensus_block_hash, )? - .ok_or_else(not_found_error) + .ok_or_else(|| { + sp_blockchain::Error::Backend(format!( + "Receipt for consensus block {consensus_block_hash} and domain block \ + {domain_hash}#{domain_number} not found" + )) + }) } From ae80fa1e4aacec3a214f465e4f05cee548ff8e6c Mon Sep 17 00:00:00 2001 From: linning Date: Mon, 28 Aug 2023 06:43:13 +0800 Subject: [PATCH 3/5] Remove the domain_block_hash => Vec mapping from the operator aux storage Signed-off-by: linning --- .../client/domain-operator/src/aux_schema.rs | 36 ------------------- domains/client/domain-operator/src/tests.rs | 23 ++++++------ 2 files changed, 12 insertions(+), 47 deletions(-) diff --git a/domains/client/domain-operator/src/aux_schema.rs b/domains/client/domain-operator/src/aux_schema.rs index c744a866f8..c2ca1b2c88 100644 --- a/domains/client/domain-operator/src/aux_schema.rs +++ b/domains/client/domain-operator/src/aux_schema.rs @@ -24,14 +24,6 @@ const BAD_RECEIPT_MISMATCH_INFO: &[u8] = b"bad_receipt_mismatch_info"; /// NOTE: Unbounded but the size is not expected to be large. const BAD_RECEIPT_NUMBERS: &[u8] = b"bad_receipt_numbers"; -/// domain_block_hash => Vec -/// -/// Updated only when there is a new domain block produced -/// -/// NOTE: different consensus blocks may derive the exact same domain block, thus one domain block may -/// mapping to multiple consensus block. -const CONSENSUS_HASH: &[u8] = b"consensus_block_hash"; - /// domain_block_hash => latest_consensus_block_hash /// /// It's important to note that a consensus block could possibly contain no bundles for a specific domain, @@ -93,7 +85,6 @@ where { let block_number = execution_receipt.consensus_block_number; let consensus_hash = execution_receipt.consensus_block_hash; - let domain_hash = execution_receipt.domain_block_hash; let block_number_key = (EXECUTION_RECEIPT_BLOCK_NUMBER, block_number).encode(); let mut hashes_at_block_number = @@ -131,24 +122,12 @@ where } } - let consensus_hashes = { - let mut hashes = - consensus_block_hash_for::(backend, domain_hash)?; - hashes.push(consensus_hash); - hashes - }; - backend.insert_aux( &[ ( execution_receipt_key(consensus_hash).as_slice(), execution_receipt.encode().as_slice(), ), - // TODO: prune the stale mappings. - ( - (CONSENSUS_HASH, domain_hash).encode().as_slice(), - consensus_hashes.encode().as_slice(), - ), ( block_number_key.as_slice(), hashes_at_block_number.encode().as_slice(), @@ -242,21 +221,6 @@ where ) } -pub(super) fn consensus_block_hash_for( - backend: &Backend, - domain_hash: Hash, -) -> ClientResult> -where - Backend: AuxStore, - Hash: Encode, - PHash: Decode, -{ - Ok( - load_decode(backend, (CONSENSUS_HASH, domain_hash).encode().as_slice())? - .unwrap_or_default(), - ) -} - // TODO: Unlock once domain test infra is workable again. #[allow(dead_code)] pub(super) fn target_receipt_is_pruned( diff --git a/domains/client/domain-operator/src/tests.rs b/domains/client/domain-operator/src/tests.rs index 9af56a7a01..533254c4bc 100644 --- a/domains/client/domain-operator/src/tests.rs +++ b/domains/client/domain-operator/src/tests.rs @@ -1882,17 +1882,18 @@ async fn test_multiple_consensus_blocks_derive_same_domain_block() { .await .unwrap(); - // The same domain block mapped to 2 different consensus blocks - let consensus_best_hashes = crate::aux_schema::consensus_block_hash_for::< - _, - _, - ::Hash, - >(&*alice.client, alice.client.info().best_hash) - .unwrap(); - assert_eq!( - consensus_best_hashes, - vec![consensus_block_hash_fork_a, consensus_block_hash_fork_b] - ); + // TODO: revise this test + // // The same domain block mapped to 2 different consensus blocks + // let consensus_best_hashes = crate::aux_schema::consensus_block_hash_for::< + // _, + // _, + // ::Hash, + // >(&*alice.client, alice.client.info().best_hash) + // .unwrap(); + // assert_eq!( + // consensus_best_hashes, + // vec![consensus_block_hash_fork_a, consensus_block_hash_fork_b] + // ); assert_ne!(consensus_block_hash_fork_a, consensus_block_hash_fork_b); // Produce one more block at fork A to make it the canonical chain and the operator From a55aaa013d18f0cb26132558e61767848407beae Mon Sep 17 00:00:00 2001 From: linning Date: Mon, 28 Aug 2023 06:44:11 +0800 Subject: [PATCH 4/5] Revise domain test test_multiple_consensus_blocks_derive_similar_domain_block Signed-off-by: linning --- domains/client/domain-operator/src/tests.rs | 68 +++++++++++++++++---- 1 file changed, 55 insertions(+), 13 deletions(-) diff --git a/domains/client/domain-operator/src/tests.rs b/domains/client/domain-operator/src/tests.rs index 533254c4bc..8e44787d22 100644 --- a/domains/client/domain-operator/src/tests.rs +++ b/domains/client/domain-operator/src/tests.rs @@ -1819,7 +1819,7 @@ async fn test_restart_domain_operator() { } #[substrate_test_utils::test(flavor = "multi_thread")] -async fn test_multiple_consensus_blocks_derive_same_domain_block() { +async fn test_multiple_consensus_blocks_derive_similar_domain_block() { let directory = TempDir::new().expect("Must be able to create temporary directory"); let mut builder = sc_cli::LoggerBuilder::new(""); @@ -1882,20 +1882,62 @@ async fn test_multiple_consensus_blocks_derive_same_domain_block() { .await .unwrap(); - // TODO: revise this test - // // The same domain block mapped to 2 different consensus blocks - // let consensus_best_hashes = crate::aux_schema::consensus_block_hash_for::< - // _, - // _, - // ::Hash, - // >(&*alice.client, alice.client.info().best_hash) - // .unwrap(); - // assert_eq!( - // consensus_best_hashes, - // vec![consensus_block_hash_fork_a, consensus_block_hash_fork_b] - // ); assert_ne!(consensus_block_hash_fork_a, consensus_block_hash_fork_b); + // Both consensus block from fork A and B should derive a corresponding domain block + let domain_block_hash_fork_a = + crate::aux_schema::best_domain_hash_for(&*alice.client, &consensus_block_hash_fork_a) + .unwrap() + .unwrap(); + let domain_block_hash_fork_b = + crate::aux_schema::best_domain_hash_for(&*alice.client, &consensus_block_hash_fork_b) + .unwrap() + .unwrap(); + assert_ne!(domain_block_hash_fork_a, domain_block_hash_fork_b); + + // The domain block header should contains digest that point to the consensus block, which + // devrive the domain block + let get_header = |hash| alice.client.header(hash).unwrap().unwrap(); + let get_digest_consensus_block_hash = |header: &Header| -> ::Hash { + header + .digest() + .convert_first(DigestItem::as_consensus_block_info) + .unwrap() + }; + let domain_block_header_fork_a = get_header(domain_block_hash_fork_a); + let domain_block_header_fork_b = get_header(domain_block_hash_fork_b); + let digest_consensus_block_hash_fork_a = + get_digest_consensus_block_hash(&domain_block_header_fork_a); + assert_eq!( + digest_consensus_block_hash_fork_a, + consensus_block_hash_fork_a + ); + let digest_consensus_block_hash_fork_b = + get_digest_consensus_block_hash(&domain_block_header_fork_b); + assert_eq!( + digest_consensus_block_hash_fork_b, + consensus_block_hash_fork_b + ); + + // The domain block headers should have the same `parent_hash` and `extrinsics_root` + // but different digest and `state_root` (because digest is stored on chain) + assert_eq!( + domain_block_header_fork_a.parent_hash(), + domain_block_header_fork_b.parent_hash() + ); + assert_eq!( + domain_block_header_fork_a.extrinsics_root(), + domain_block_header_fork_b.extrinsics_root() + ); + assert_ne!( + domain_block_header_fork_a.digest(), + domain_block_header_fork_b.digest() + ); + assert_ne!( + domain_block_header_fork_a.state_root(), + domain_block_header_fork_b.state_root() + ); + // Produce one more block at fork A to make it the canonical chain and the operator // should submit the ER of fork A let (slot, bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await; From 8719fa036aaa4f3674de3430f03b11af106ffbac Mon Sep 17 00:00:00 2001 From: linning Date: Mon, 28 Aug 2023 18:44:19 +0800 Subject: [PATCH 5/5] Apply review suggestion Signed-off-by: linning --- domains/client/domain-operator/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/domains/client/domain-operator/src/lib.rs b/domains/client/domain-operator/src/lib.rs index dc29585a84..42032858f6 100644 --- a/domains/client/domain-operator/src/lib.rs +++ b/domains/client/domain-operator/src/lib.rs @@ -263,8 +263,7 @@ where .convert_first(DigestItem::as_consensus_block_info) .ok_or_else(|| { sp_blockchain::Error::Application(Box::from( - "Domain block header {domain_hash}#{domain_number} missing the consensus \ - block info predigest, this should not happen", + "Domain block header {domain_hash}#{domain_number} must have consensus block info predigest" )) })?;