Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Manually track consensus block hash for ER verification #1847

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Manually track consensus block hash for ER verification #1847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e1dc4f
Manually track consensus block hash for ER verification
NingLin-P Aug 18, 2023
5e43c9e
Remove the needless assumption between BlockTreePruningDepth and Bloc…
NingLin-P Aug 18, 2023
8c4d475
Merge branch 'main' into consensus-hash
NingLin-P Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 78 additions & 29 deletions crates/pallet-domains/src/block_tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@

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};
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;

Expand All @@ -29,6 +29,7 @@ pub enum Error {
MultipleERsAfterChallengePeriod,
MissingDomainBlock,
InvalidTraceRoot,
UnavailableConsensusBlockHash,
}

#[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -166,14 +167,28 @@ pub(crate) fn verify_execution_receipt<T: Config>(
expected_execution_trace_root == *execution_trace_root,
Error::InvalidTraceRoot
);
}

let excepted_consensus_block_hash =
frame_system::Pallet::<T>::block_hash(consensus_block_number);
ensure!(
*consensus_block_hash == excepted_consensus_block_hash,
Error::BuiltOnUnknownConsensusBlock
);
let excepted_consensus_block_hash =
match ConsensusBlockHash::<T>::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::<T>::current_block_number() - One::one();
if *consensus_block_number == parent_block_number {
frame_system::Pallet::<T>::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::<T>::get(domain_id, parent_block_number)
Expand Down Expand Up @@ -267,6 +282,11 @@ pub(crate) fn process_execution_receipt<T: Config>(
}
let _ = ExecutionInbox::<T>::clear_prefix((domain_id, to_prune), u32::MAX, None);

ConsensusBlockHash::<T>::remove(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not prune it when we see the first ER instead of waiting until challenge period is over since we do not require this for the same ERs which comes from other operators ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the ER that is still in the challenge period can be pruned by fraud proof and resubmitted by other operators thus we need the consensus hash for the verification.

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,
Expand Down Expand Up @@ -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<T: Config>(block_number: T::BlockNumber, parent_hash: T::Hash) {
Expand Down Expand Up @@ -422,12 +443,13 @@ mod tests {

let head_node = get_block_tree_node_at::<Test>(domain_id, head_receipt_number).unwrap();
let mut receipt = head_node.execution_receipt;
assert_eq!(
receipt.consensus_block_number,
frame_system::Pallet::<Test>::current_block_number()
);
for block_number in (head_receipt_number + 1)..=to {
// Run to `block_number`
run_to_block::<Test>(
block_number,
frame_system::Pallet::<Test>::block_hash(block_number - 1),
);
// Finilize parent block and initialize block at `block_number`
run_to_block::<Test>(block_number, receipt.consensus_block_hash);

// Submit a bundle with the receipt of the last block
let bundle_extrinsics_root = H256::random();
Expand All @@ -448,7 +470,7 @@ mod tests {
get_block_tree_node_at::<Test>(domain_id, head_receipt_number).unwrap();
receipt = create_dummy_receipt(
block_number,
frame_system::Pallet::<Test>::block_hash(block_number),
H256::random(),
parent_block_tree_node.execution_receipt.hash(),
vec![bundle_extrinsics_root],
);
Expand Down Expand Up @@ -512,14 +534,29 @@ mod tests {
// The genesis node of the block tree
let genesis_node = get_block_tree_node_at::<Test>(domain_id, 0).unwrap();
let mut receipt = genesis_node.execution_receipt;
assert_eq!(
receipt.consensus_block_number,
frame_system::Pallet::<Test>::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::<Test>(
block_number,
frame_system::Pallet::<Test>::block_hash(block_number - 1),
);
// Finilize parent block and initialize block at `block_number`
run_to_block::<Test>(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::<Test>::get(domain_id, block_number - 1),
Some(frame_system::Pallet::<Test>::block_hash(block_number - 1))
);
// ER point to last consensus block should have `NewHead` type
assert_eq!(
execution_receipt_type::<Test>(domain_id, &receipt),
ReceiptType::Accepted(AcceptedReceiptType::NewHead)
);
assert_ok!(verify_execution_receipt::<Test>(domain_id, &receipt));
}

// Submit a bundle with the receipt of the last block
let bundle_extrinsics_root = H256::random();
Expand Down Expand Up @@ -563,15 +600,10 @@ mod tests {
// in the next bundle
receipt = create_dummy_receipt(
block_number,
frame_system::Pallet::<Test>::block_hash(block_number),
H256::random(),
*parent_domain_block_receipt,
vec![bundle_extrinsics_root],
);
assert_eq!(
execution_receipt_type::<Test>(domain_id, &receipt),
ReceiptType::Accepted(AcceptedReceiptType::NewHead)
);
assert_ok!(verify_execution_receipt::<Test>(domain_id, &receipt));

// Record receipt of block #1 for later use
if block_number == 1 {
Expand All @@ -595,6 +627,11 @@ mod tests {
verify_execution_receipt::<Test>(domain_id, &pruned_receipt),
Error::InvalidExtrinsicsRoots
);
assert!(ConsensusBlockHash::<Test>::get(
domain_id,
pruned_receipt.consensus_block_number
)
.is_none());
});
}

Expand Down Expand Up @@ -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;
Expand All @@ -790,6 +826,19 @@ mod tests {
execution_receipt_type::<Test>(domain_id, &future_receipt),
ReceiptType::Rejected(RejectedReceiptType::InFuture)
);

// Return `UnavailableConsensusBlockHash` error since ER point to a future consensus block
assert_err!(
verify_execution_receipt::<Test>(domain_id, &future_receipt),
Error::UnavailableConsensusBlockHash
);
ConsensusBlockHash::<Test>::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::<Test>(domain_id, &future_receipt),
Error::UnknownParentBlockReceipt
Expand Down
28 changes: 19 additions & 9 deletions crates/pallet-domains/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,14 +213,7 @@ mod pallet {
/// A variation of the Identifier used for holding the funds used for staking and domains.
type HoldIdentifier: HoldIdentifier<Self>;

/// 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<Self::DomainNumber>;

Expand Down Expand Up @@ -468,6 +461,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<T: Config> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a another optimization that can done here using reference counting for all the domains rather than storing the same hash for each domain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have considered this but it seems more complected because each domain may submit bundle in different consensus blocks so the consensus hash needed to store is different from domain to domain, and when should a consensus hash be pruned is also different from domain to domain, the complexity come when we need to track these difference.

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:
///
Expand Down Expand Up @@ -1067,7 +1071,13 @@ mod pallet {

do_upgrade_runtimes::<T>(block_number);

let _ = SuccessfulBundles::<T>::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::<T>::block_hash(parent_number);
for (domain_id, _) in SuccessfulBundles::<T>::drain() {
ConsensusBlockHash::<T>::insert(domain_id, parent_number, parent_hash);
}

Weight::zero()
}
Expand Down
5 changes: 0 additions & 5 deletions crates/subspace-runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I noticed earlier that BlockHashCount was unused so removing the assertion make sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the last use of static_assertions, dependency should have been removed as well


impl pallet_domains::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type DomainNumber = DomainNumber;
Expand Down
Loading