From 9e1dc4fa765b88d4f36f5a5ac7b8a09c52dbacd9 Mon Sep 17 00:00:00 2001 From: linning Date: Sat, 19 Aug 2023 05:38:16 +0800 Subject: [PATCH 1/2] Manually track consensus block hash for ER verification The consensus block hash is only tracked if the block contains bundle and it is pruned when the corresponding ER is pruned. This commit also adjust the test correspondingly. Signed-off-by: linning --- crates/pallet-domains/src/block_tree.rs | 107 +++++++++++++++++------- crates/pallet-domains/src/lib.rs | 19 ++++- 2 files changed, 96 insertions(+), 30 deletions(-) diff --git a/crates/pallet-domains/src/block_tree.rs b/crates/pallet-domains/src/block_tree.rs index ddfff41393..5cb67e4d73 100644 --- a/crates/pallet-domains/src/block_tree.rs +++ b/crates/pallet-domains/src/block_tree.rs @@ -2,8 +2,8 @@ use crate::pallet::StateRoots; use crate::{ - BalanceOf, BlockTree, Config, DomainBlocks, ExecutionInbox, ExecutionReceiptOf, - HeadReceiptNumber, InboxedBundle, + BalanceOf, BlockTree, Config, ConsensusBlockHash, DomainBlocks, ExecutionInbox, + ExecutionReceiptOf, HeadReceiptNumber, InboxedBundle, }; use codec::{Decode, Encode}; use frame_support::{ensure, PalletError}; @@ -11,7 +11,7 @@ use scale_info::TypeInfo; use sp_core::Get; use sp_domains::merkle_tree::MerkleTree; use sp_domains::{DomainId, ExecutionReceipt, OperatorId}; -use sp_runtime::traits::{CheckedSub, One, Saturating, Zero}; +use sp_runtime::traits::{BlockNumberProvider, CheckedSub, One, Saturating, Zero}; use sp_std::cmp::Ordering; use sp_std::vec::Vec; @@ -29,6 +29,7 @@ pub enum Error { MultipleERsAfterChallengePeriod, MissingDomainBlock, InvalidTraceRoot, + UnavailableConsensusBlockHash, } #[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq)] @@ -166,14 +167,28 @@ pub(crate) fn verify_execution_receipt( expected_execution_trace_root == *execution_trace_root, Error::InvalidTraceRoot ); - } - let excepted_consensus_block_hash = - frame_system::Pallet::::block_hash(consensus_block_number); - ensure!( - *consensus_block_hash == excepted_consensus_block_hash, - Error::BuiltOnUnknownConsensusBlock - ); + let excepted_consensus_block_hash = + match ConsensusBlockHash::::get(domain_id, consensus_block_number) { + Some(hash) => hash, + // The `initialize_block` of non-system pallets is skipped in the `validate_transaction`, + // thus the hash of best block, which is recorded in the this pallet's `on_initialize` hook, + // is unavailable at this point. + None => { + let parent_block_number = + frame_system::Pallet::::current_block_number() - One::one(); + if *consensus_block_number == parent_block_number { + frame_system::Pallet::::parent_hash() + } else { + return Err(Error::UnavailableConsensusBlockHash); + } + } + }; + ensure!( + *consensus_block_hash == excepted_consensus_block_hash, + Error::BuiltOnUnknownConsensusBlock + ); + } if let Some(parent_block_number) = domain_block_number.checked_sub(&One::one()) { let parent_block_exist = BlockTree::::get(domain_id, parent_block_number) @@ -267,6 +282,11 @@ pub(crate) fn process_execution_receipt( } let _ = ExecutionInbox::::clear_prefix((domain_id, to_prune), u32::MAX, None); + ConsensusBlockHash::::remove( + domain_id, + domain_block.execution_receipt.consensus_block_number, + ); + pruned_domain_block_info = Some(PrunedDomainBlockInfo { domain_block_number: to_prune, operator_ids: domain_block.operator_ids, @@ -358,6 +378,7 @@ mod tests { use frame_system::Pallet as System; use sp_core::{Pair, H256, U256}; use sp_domains::{BundleDigest, OperatorPair, RuntimeType}; + use sp_runtime::traits::BlockNumberProvider; use subspace_runtime_primitives::SSC; fn run_to_block(block_number: T::BlockNumber, parent_hash: T::Hash) { @@ -422,12 +443,13 @@ mod tests { let head_node = get_block_tree_node_at::(domain_id, head_receipt_number).unwrap(); let mut receipt = head_node.execution_receipt; + assert_eq!( + receipt.consensus_block_number, + frame_system::Pallet::::current_block_number() + ); for block_number in (head_receipt_number + 1)..=to { - // Run to `block_number` - run_to_block::( - block_number, - frame_system::Pallet::::block_hash(block_number - 1), - ); + // Finilize parent block and initialize block at `block_number` + run_to_block::(block_number, receipt.consensus_block_hash); // Submit a bundle with the receipt of the last block let bundle_extrinsics_root = H256::random(); @@ -448,7 +470,7 @@ mod tests { get_block_tree_node_at::(domain_id, head_receipt_number).unwrap(); receipt = create_dummy_receipt( block_number, - frame_system::Pallet::::block_hash(block_number), + H256::random(), parent_block_tree_node.execution_receipt.hash(), vec![bundle_extrinsics_root], ); @@ -512,14 +534,29 @@ mod tests { // The genesis node of the block tree let genesis_node = get_block_tree_node_at::(domain_id, 0).unwrap(); let mut receipt = genesis_node.execution_receipt; + assert_eq!( + receipt.consensus_block_number, + frame_system::Pallet::::current_block_number() + ); let mut receipt_of_block_1 = None; let mut bundle_header_hash_of_block_1 = None; for block_number in 1..=(block_tree_pruning_depth + 3) { - // Run to `block_number` - run_to_block::( - block_number, - frame_system::Pallet::::block_hash(block_number - 1), - ); + // Finilize parent block and initialize block at `block_number` + run_to_block::(block_number, receipt.consensus_block_hash); + + if block_number != 1 { + // `ConsensusBlockHash` should be set to `Some` since last consensus block contains bundle + assert_eq!( + ConsensusBlockHash::::get(domain_id, block_number - 1), + Some(frame_system::Pallet::::block_hash(block_number - 1)) + ); + // ER point to last consensus block should have `NewHead` type + assert_eq!( + execution_receipt_type::(domain_id, &receipt), + ReceiptType::Accepted(AcceptedReceiptType::NewHead) + ); + assert_ok!(verify_execution_receipt::(domain_id, &receipt)); + } // Submit a bundle with the receipt of the last block let bundle_extrinsics_root = H256::random(); @@ -563,15 +600,10 @@ mod tests { // in the next bundle receipt = create_dummy_receipt( block_number, - frame_system::Pallet::::block_hash(block_number), + H256::random(), *parent_domain_block_receipt, vec![bundle_extrinsics_root], ); - assert_eq!( - execution_receipt_type::(domain_id, &receipt), - ReceiptType::Accepted(AcceptedReceiptType::NewHead) - ); - assert_ok!(verify_execution_receipt::(domain_id, &receipt)); // Record receipt of block #1 for later use if block_number == 1 { @@ -595,6 +627,11 @@ mod tests { verify_execution_receipt::(domain_id, &pruned_receipt), Error::InvalidExtrinsicsRoots ); + assert!(ConsensusBlockHash::::get( + domain_id, + pruned_receipt.consensus_block_number + ) + .is_none()); }); } @@ -765,8 +802,7 @@ mod tests { .unwrap() .execution_receipt; - // In future receipt will result in `UnknownParentBlockReceipt` error as its parent - // receipt is missing from the block tree + // Construct a future receipt let mut future_receipt = current_head_receipt.clone(); future_receipt.domain_block_number = head_receipt_number + 2; future_receipt.consensus_block_number = head_receipt_number + 2; @@ -790,6 +826,19 @@ mod tests { execution_receipt_type::(domain_id, &future_receipt), ReceiptType::Rejected(RejectedReceiptType::InFuture) ); + + // Return `UnavailableConsensusBlockHash` error since ER point to a future consensus block + assert_err!( + verify_execution_receipt::(domain_id, &future_receipt), + Error::UnavailableConsensusBlockHash + ); + ConsensusBlockHash::::insert( + domain_id, + future_receipt.consensus_block_number, + future_receipt.consensus_block_hash, + ); + + // Return `UnknownParentBlockReceipt` error as its parent receipt is missing from the block tree assert_err!( verify_execution_receipt::(domain_id, &future_receipt), Error::UnknownParentBlockReceipt diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index 8b283a9644..3eec0ecd11 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/crates/pallet-domains/src/lib.rs @@ -468,6 +468,17 @@ mod pallet { OptionQuery, >; + /// The consensus block hash used to verify ER, only store the consensus block hash for a domain + /// if that consensus block contains bundle of the domain, the hash will be pruned when the ER + /// that point to the consensus block is pruned. + /// + /// TODO: this storage is unbounded in some cases, see https://github.com/subspace/subspace/issues/1673 + /// for more details, this will be fixed once https://github.com/subspace/subspace/issues/1731 is implemented. + #[pallet::storage] + #[pallet::getter(fn consensus_hash)] + pub type ConsensusBlockHash = + StorageDoubleMap<_, Identity, DomainId, Identity, T::BlockNumber, T::Hash, OptionQuery>; + /// A set of `BundleDigest` from all bundles that successfully submitted to the consensus block, /// these bundles will be used to construct the domain block and `ExecutionInbox` is used to: /// @@ -1058,7 +1069,13 @@ mod pallet { do_upgrade_runtimes::(block_number); - let _ = SuccessfulBundles::::clear(u32::MAX, None); + // Store the hash of the parent consensus block for domain that have bundles submitted + // in that consensus block + let parent_number = block_number - One::one(); + let parent_hash = frame_system::Pallet::::block_hash(parent_number); + for (domain_id, _) in SuccessfulBundles::::drain() { + ConsensusBlockHash::::insert(domain_id, parent_number, parent_hash); + } Weight::zero() } From 5e43c9e44054f9da5064a6813b0a3b22214f71a6 Mon Sep 17 00:00:00 2001 From: linning Date: Sat, 19 Aug 2023 05:39:31 +0800 Subject: [PATCH 2/2] Remove the needless assumption between BlockTreePruningDepth and BlockHashCount Signed-off-by: linning --- crates/pallet-domains/src/lib.rs | 9 +-------- crates/subspace-runtime/src/lib.rs | 5 ----- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index 3eec0ecd11..506d44d8dc 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/crates/pallet-domains/src/lib.rs @@ -213,14 +213,7 @@ mod pallet { /// A variation of the Identifier used for holding the funds used for staking and domains. type HoldIdentifier: HoldIdentifier; - /// The block tree pruning depth, its value should <= `BlockHashCount` because we - /// need the consensus block hash to verify execution receipt, which is used to - /// construct the node of the block tree. - /// - /// TODO: `BlockTreePruningDepth` <= `BlockHashCount` is not enough to guarantee the consensus block - /// hash must exists while verifying receipt because the domain block is not mapping to the consensus - /// block one by one, we need to either store the consensus block hash in runtime manually or store - /// the consensus block hash in the client side and use host function to get them in runtime. + /// The block tree pruning depth. #[pallet::constant] type BlockTreePruningDepth: Get; diff --git a/crates/subspace-runtime/src/lib.rs b/crates/subspace-runtime/src/lib.rs index b1c9c6dffc..60cc64d081 100644 --- a/crates/subspace-runtime/src/lib.rs +++ b/crates/subspace-runtime/src/lib.rs @@ -73,7 +73,6 @@ use sp_std::prelude::*; #[cfg(feature = "std")] use sp_version::NativeVersion; use sp_version::RuntimeVersion; -use static_assertions::const_assert; use subspace_core_primitives::crypto::Scalar; use subspace_core_primitives::objects::BlockObjectMapping; use subspace_core_primitives::{ @@ -491,10 +490,6 @@ parameter_types! { pub const MaxPendingStakingOperation: u32 = 100; } -// `BlockTreePruningDepth` should <= `BlockHashCount` because we need the consensus block hash to verify -// execution receipt, which is used to construct the node of the block tree. -const_assert!(BlockTreePruningDepth::get() <= BlockHashCount::get()); - impl pallet_domains::Config for Runtime { type RuntimeEvent = RuntimeEvent; type DomainNumber = DomainNumber;