Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion crates/pallet-domains/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ fn submit_fraud_proof_should_work() {
domain_id,
bad_receipt_hash: Hash::random(),
parent_number: 99,
parent_hash: block_hashes[98],
primary_parent_hash: block_hashes[98],
pre_state_root: H256::random(),
post_state_root: H256::random(),
proof: StorageProof::empty(),
Expand Down
7 changes: 4 additions & 3 deletions crates/pallet-receipts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,11 +359,12 @@ impl<T: Config> Pallet<T> {
FraudProofError::ExecutionReceiptInFuture
);

let parent_hash = T::Hash::decode(&mut fraud_proof.parent_hash.encode().as_slice())
.map_err(|_| FraudProofError::WrongHashType)?;
let primary_parent_hash =
T::Hash::decode(&mut fraud_proof.primary_parent_hash.encode().as_slice())
Copy link
Member

Choose a reason for hiding this comment

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

Another lesson learned is that encoding/decoding erases types and can easily cause problems. We should consider adding to/from bounds and ensure that types are compatible statically even if we can't name them explicitly due to use of generics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I'm not super happy about adding the from/to bounds due to several reasons, I agree on the benefits gained from the compiler. We had a TODO, but haven't fixed it and applied the rule everythere.

.map_err(|_| FraudProofError::WrongHashType)?;
let parent_number: T::BlockNumber = fraud_proof.parent_number.into();
ensure!(
Self::primary_hash(fraud_proof.domain_id, parent_number) == Some(parent_hash),
Self::primary_hash(fraud_proof.domain_id, parent_number) == Some(primary_parent_hash),
FraudProofError::UnknownBlock
);

Expand Down
18 changes: 10 additions & 8 deletions crates/sp-domains/src/fraud_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@ use sp_trie::StorageProof;
use subspace_core_primitives::BlockNumber;
use subspace_runtime_primitives::AccountId;

/// A phase of a block's execution.
/// A phase of a block's execution, carrying necessary information needed for verifying the
/// invalid state transition proof.
#[derive(Debug, Decode, Encode, TypeInfo, PartialEq, Eq, Clone)]
pub enum ExecutionPhase {
/// Executes the `initialize_block` hook.
InitializeBlock,
InitializeBlock { domain_parent_hash: H256 },
/// Executes some extrinsic.
ApplyExtrinsic(u32),
/// Executes the `finalize_block` hook.
Expand All @@ -26,7 +27,7 @@ impl ExecutionPhase {
match self {
// TODO: Replace `DomainCoreApi_initialize_block_with_post_state_root` with `Core_initalize_block`
// Should be a same issue with https://github.com/paritytech/substrate/pull/10922#issuecomment-1068997467
Self::InitializeBlock => "DomainCoreApi_initialize_block_with_post_state_root",
Self::InitializeBlock { .. } => "DomainCoreApi_initialize_block_with_post_state_root",
Self::ApplyExtrinsic(_) => "BlockBuilder_apply_extrinsic",
Self::FinalizeBlock { .. } => "BlockBuilder_finalize_block",
}
Expand All @@ -39,7 +40,7 @@ impl ExecutionPhase {
/// result of execution reported in [`FraudProof`] is expected or not.
pub fn verifying_method(&self) -> &'static str {
match self {
Self::InitializeBlock => "DomainCoreApi_initialize_block_with_post_state_root",
Self::InitializeBlock { .. } => "DomainCoreApi_initialize_block_with_post_state_root",
Self::ApplyExtrinsic(_) => "DomainCoreApi_apply_extrinsic_with_post_state_root",
Self::FinalizeBlock { .. } => "BlockBuilder_finalize_block",
}
Expand All @@ -51,7 +52,7 @@ impl ExecutionPhase {
execution_result: Vec<u8>,
) -> Result<Header::Hash, VerificationError> {
match self {
Self::InitializeBlock | Self::ApplyExtrinsic(_) => {
Self::InitializeBlock { .. } | Self::ApplyExtrinsic(_) => {
let encoded_storage_root = Vec::<u8>::decode(&mut execution_result.as_slice())
.map_err(VerificationError::InitializeBlockOrApplyExtrinsicDecode)?;
Header::Hash::decode(&mut encoded_storage_root.as_slice())
Expand Down Expand Up @@ -158,10 +159,11 @@ pub struct InvalidStateTransitionProof {
pub bad_receipt_hash: H256,
/// Parent number.
pub parent_number: BlockNumber,
/// Parent hash of the block at which the invalid execution occurred.
/// Hash of the primary block corresponding to `parent_number`.
///
/// Runtime code for this block's execution is retrieved on top of the parent block.
pub parent_hash: H256,
/// Runtime code for the execution of the domain block that is being challenged
/// is retrieved on top of the primary parent block from the primary chain.
pub primary_parent_hash: H256,
/// State root before the fraudulent transaction.
pub pre_state_root: H256,
/// State root after the fraudulent transaction.
Expand Down
25 changes: 13 additions & 12 deletions crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,22 +259,23 @@ where
) -> Result<(), VerificationError> {
let InvalidStateTransitionProof {
domain_id,
bad_receipt_hash,
parent_number,
parent_hash,
bad_receipt_hash,
pre_state_root,
execution_phase,
..
} = invalid_state_transition_proof;

let pre_state_root_onchain = match execution_phase {
ExecutionPhase::InitializeBlock => self.client.runtime_api().state_root(
self.client.info().best_hash,
*domain_id,
NumberFor::<Block>::from(*parent_number),
Block::Hash::decode(&mut parent_hash.encode().as_slice())
.expect("Block Hash must be H256; qed"),
)?,
ExecutionPhase::InitializeBlock { domain_parent_hash } => {
self.client.runtime_api().state_root(
self.client.info().best_hash,
*domain_id,
NumberFor::<Block>::from(*parent_number),
Block::Hash::decode(&mut domain_parent_hash.encode().as_slice())
.expect("Block Hash must be H256; qed"),
)?
}
ExecutionPhase::ApplyExtrinsic(trace_index_of_pre_state_root)
| ExecutionPhase::FinalizeBlock {
total_extrinsics: trace_index_of_pre_state_root,
Expand Down Expand Up @@ -319,7 +320,7 @@ where
)?;

let post_state_root_onchain = match execution_phase {
ExecutionPhase::InitializeBlock => trace
ExecutionPhase::InitializeBlock { .. } => trace
.get(0)
.ok_or(VerificationError::PostStateRootNotFound)?,
ExecutionPhase::ApplyExtrinsic(trace_index_of_post_state_root)
Expand Down Expand Up @@ -396,15 +397,15 @@ where

let InvalidStateTransitionProof {
domain_id,
parent_hash,
primary_parent_hash,
pre_state_root,
post_state_root,
proof,
execution_phase,
..
} = invalid_state_transition_proof;

let at = PBlock::Hash::decode(&mut parent_hash.encode().as_slice())
let at = PBlock::Hash::decode(&mut primary_parent_hash.encode().as_slice())
.expect("Block Hash must be H256; qed");
let system_wasm_bundle = self
.client
Expand Down
16 changes: 9 additions & 7 deletions crates/subspace-fraud-proof/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,9 @@ async fn execution_proof_creation_and_verification_should_work() {
parent_header.hash(),
Default::default(),
);
let execution_phase = ExecutionPhase::InitializeBlock;
let execution_phase = ExecutionPhase::InitializeBlock {
domain_parent_hash: parent_header.hash(),
};
let initialize_block_call_data = new_header.encode();

let prover = ExecutionProver::new(
Expand Down Expand Up @@ -208,7 +210,7 @@ async fn execution_proof_creation_and_verification_should_work() {
domain_id: TEST_DOMAIN_ID,
bad_receipt_hash: Hash::random(),
parent_number: parent_number_alice,
parent_hash: parent_hash_alice,
primary_parent_hash: parent_hash_alice,
pre_state_root: *parent_header.state_root(),
post_state_root: intermediate_roots[0].into(),
proof: storage_proof,
Expand Down Expand Up @@ -266,7 +268,7 @@ async fn execution_proof_creation_and_verification_should_work() {
domain_id: TEST_DOMAIN_ID,
bad_receipt_hash: Hash::random(),
parent_number: parent_number_alice,
parent_hash: parent_hash_alice,
primary_parent_hash: parent_hash_alice,
pre_state_root: intermediate_roots[target_extrinsic_index].into(),
post_state_root: intermediate_roots[target_extrinsic_index + 1].into(),
proof: storage_proof,
Expand Down Expand Up @@ -320,7 +322,7 @@ async fn execution_proof_creation_and_verification_should_work() {
domain_id: TEST_DOMAIN_ID,
bad_receipt_hash: Hash::random(),
parent_number: parent_number_alice,
parent_hash: parent_hash_alice,
primary_parent_hash: parent_hash_alice,
pre_state_root: intermediate_roots.last().unwrap().into(),
post_state_root: post_execution_root,
proof: storage_proof,
Expand Down Expand Up @@ -498,7 +500,7 @@ async fn invalid_execution_proof_should_not_work() {
domain_id: TEST_DOMAIN_ID,
bad_receipt_hash: Hash::random(),
parent_number: parent_number_alice,
parent_hash: parent_hash_alice,
primary_parent_hash: parent_hash_alice,
pre_state_root: post_delta_root0,
post_state_root: post_delta_root1,
proof: proof1,
Expand All @@ -512,7 +514,7 @@ async fn invalid_execution_proof_should_not_work() {
domain_id: TEST_DOMAIN_ID,
bad_receipt_hash: Hash::random(),
parent_number: parent_number_alice,
parent_hash: parent_hash_alice,
primary_parent_hash: parent_hash_alice,
pre_state_root: post_delta_root0,
post_state_root: post_delta_root1,
proof: proof0.clone(),
Expand All @@ -526,7 +528,7 @@ async fn invalid_execution_proof_should_not_work() {
domain_id: TEST_DOMAIN_ID,
bad_receipt_hash: Hash::random(),
parent_number: parent_number_alice,
parent_hash: parent_hash_alice,
primary_parent_hash: parent_hash_alice,
pre_state_root: post_delta_root0,
post_state_root: post_delta_root1,
proof: proof0,
Expand Down
7 changes: 5 additions & 2 deletions domains/client/domain-executor/src/core_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub struct Executor<Block, SBlock, PBlock, Client, SClient, PClient, Transaction
spawner: Box<dyn SpawnNamed + Send + Sync>,
transaction_pool: Arc<TransactionPool>,
backend: Arc<Backend>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, Backend, E>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, PClient, Backend, E>,
_phantom_data: PhantomData<(SBlock, SClient)>,
}

Expand Down Expand Up @@ -133,6 +133,7 @@ where

let fraud_proof_generator = FraudProofGenerator::new(
params.client.clone(),
params.primary_chain_client.clone(),
params.spawner.clone(),
params.backend.clone(),
params.code_executor,
Expand Down Expand Up @@ -183,7 +184,9 @@ where
})
}

pub fn fraud_proof_generator(&self) -> FraudProofGenerator<Block, PBlock, Client, Backend, E> {
pub fn fraud_proof_generator(
&self,
) -> FraudProofGenerator<Block, PBlock, Client, PClient, Backend, E> {
self.fraud_proof_generator.clone()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub struct CoreGossipMessageValidator<
PBlock,
Client,
SClient,
PClient,
TransactionPool,
Backend,
E,
Expand All @@ -34,17 +35,19 @@ pub struct CoreGossipMessageValidator<
parent_chain: ParentChain,
client: Arc<Client>,
transaction_pool: Arc<TransactionPool>,
gossip_message_validator: GossipMessageValidator<Block, PBlock, Client, Backend, E>,
_phantom_data: PhantomData<(SBlock, SClient)>,
gossip_message_validator: GossipMessageValidator<Block, PBlock, Client, PClient, Backend, E>,
_phantom_data: PhantomData<(SBlock, SClient, PClient)>,
}

impl<Block, SBlock, PBlock, Client, SClient, TransactionPool, Backend, E, ParentChain> Clone
impl<Block, SBlock, PBlock, Client, SClient, PClient, TransactionPool, Backend, E, ParentChain>
Clone
for CoreGossipMessageValidator<
Block,
SBlock,
PBlock,
Client,
SClient,
PClient,
TransactionPool,
Backend,
E,
Expand All @@ -67,13 +70,14 @@ where
}
}

impl<Block, SBlock, PBlock, Client, SClient, TransactionPool, Backend, E, ParentChain>
impl<Block, SBlock, PBlock, Client, SClient, PClient, TransactionPool, Backend, E, ParentChain>
CoreGossipMessageValidator<
Block,
SBlock,
PBlock,
Client,
SClient,
PClient,
TransactionPool,
Backend,
E,
Expand All @@ -93,6 +97,7 @@ where
+ sp_api::ApiExt<Block, StateBackend = StateBackendFor<Backend, Block>>,
SClient: HeaderBackend<SBlock> + ProvideRuntimeApi<SBlock> + 'static,
SClient::Api: SystemDomainApi<SBlock, NumberFor<PBlock>, PBlock::Hash>,
PClient: HeaderBackend<PBlock> + 'static,
Backend: sc_client_api::Backend<Block> + 'static,
TransactionFor<Backend, Block>: sp_trie::HashDBT<HashFor<Block>, sp_trie::DBValue>,
TransactionPool: sc_transaction_pool_api::TransactionPool<Block = Block> + 'static,
Expand All @@ -104,7 +109,7 @@ where
client: Arc<Client>,
spawner: Box<dyn SpawnNamed + Send + Sync>,
transaction_pool: Arc<TransactionPool>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, Backend, E>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, PClient, Backend, E>,
) -> Self {
let gossip_message_validator =
GossipMessageValidator::new(client.clone(), spawner, fraud_proof_generator);
Expand All @@ -118,14 +123,15 @@ where
}
}

impl<Block, SBlock, PBlock, Client, SClient, TransactionPool, Backend, E, ParentChain>
impl<Block, SBlock, PBlock, Client, SClient, PClient, TransactionPool, Backend, E, ParentChain>
GossipMessageHandler<PBlock, Block>
for CoreGossipMessageValidator<
Block,
SBlock,
PBlock,
Client,
SClient,
PClient,
TransactionPool,
Backend,
E,
Expand All @@ -145,6 +151,7 @@ where
+ sp_api::ApiExt<Block, StateBackend = StateBackendFor<Backend, Block>>,
SClient: HeaderBackend<SBlock> + ProvideRuntimeApi<SBlock> + 'static,
SClient::Api: SystemDomainApi<SBlock, NumberFor<PBlock>, PBlock::Hash>,
PClient: HeaderBackend<PBlock> + 'static,
Backend: sc_client_api::Backend<Block> + 'static,
TransactionFor<Backend, Block>: sp_trie::HashDBT<HashFor<Block>, sp_trie::DBValue>,
TransactionPool: sc_transaction_pool_api::TransactionPool<Block = Block> + 'static,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ where
primary_chain_client: Arc<PClient>,
primary_network_sync_oracle: Arc<dyn SyncOracle + Send + Sync>,
backend: Arc<Backend>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, Backend, E>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, PClient, Backend, E>,
}

impl<Block, PBlock, Client, PClient, Backend, E> Clone
Expand Down Expand Up @@ -105,7 +105,7 @@ where
primary_chain_client: Arc<PClient>,
primary_network_sync_oracle: Arc<dyn SyncOracle + Send + Sync>,
backend: Arc<Backend>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, Backend, E>,
fraud_proof_generator: FraudProofGenerator<Block, PBlock, Client, PClient, Backend, E>,
) -> Self {
Self {
domain_id,
Expand Down
Loading