Skip to content

Commit

Permalink
Merge pull request #2207 from subspace/er-error-log
Browse files Browse the repository at this point in the history 
Reorder the ER verification steps and downgrade log level for some common/expected errors in pallet-domains
  • Loading branch information
@NingLin-P
NingLin-P authored Nov 8, 2023
2 parents 56c4a8b + c56ac40 commit 0905475
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 115 deletions.
189 changes: 79 additions & 110 deletions crates/pallet-domains/src/block_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,17 @@ pub(crate) fn verify_execution_receipt<T: Config>(
..
} = execution_receipt;

// Checking if the incoming ER is expected regarding to its `domain_block_number` or freshness
if let ReceiptType::Rejected(rejected_receipt_type) =
execution_receipt_type::<T>(domain_id, execution_receipt)
{
return Err(rejected_receipt_type.into());
}

// The genesis receipt is generated and added to the block tree by the runtime upon domain
// instantiation thus it is unchallengeable, we can safely skip other checks as long as we
// can ensure it is always be the same.
if domain_block_number.is_zero() {
// The genesis receipt is generated and added to the block tree by the runtime upon domain
// instantiation, thus it is unchallengeable and must always be the same.
ensure!(
does_receipt_exists::<T>(
domain_id,
Expand All @@ -169,59 +177,63 @@ pub(crate) fn verify_execution_receipt<T: Config>(
),
Error::BadGenesisReceipt
);
} else {
let bundles_extrinsics_roots: Vec<_> =
inboxed_bundles.iter().map(|b| b.extrinsics_root).collect();
let execution_inbox =
ExecutionInbox::<T>::get((domain_id, domain_block_number, consensus_block_number));
let expected_extrinsics_roots: Vec<_> =
execution_inbox.iter().map(|b| b.extrinsics_root).collect();
ensure!(
!bundles_extrinsics_roots.is_empty()
&& bundles_extrinsics_roots == expected_extrinsics_roots,
Error::InvalidExtrinsicsRoots
);

let mut trace = Vec::with_capacity(execution_trace.len());
for root in execution_trace {
trace.push(
root.encode()
.try_into()
.map_err(|_| Error::InvalidTraceRoot)?,
);
}
let expected_execution_trace_root: sp_core::H256 =
MerkleTree::from_leaves(trace.as_slice())
.root()
.ok_or(Error::InvalidTraceRoot)?
.into();
ensure!(
expected_execution_trace_root == *execution_trace_root,
Error::InvalidTraceRoot
);
return Ok(());
}

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);
}
// Check if the ER is derived from the correct consensus block in the current chain
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
}
};
ensure!(
*consensus_block_hash == excepted_consensus_block_hash,
Error::BuiltOnUnknownConsensusBlock
);

// Check if the ER is derived from the expected inboxed bundles of the consensus block
let bundles_extrinsics_roots: Vec<_> =
inboxed_bundles.iter().map(|b| b.extrinsics_root).collect();
let execution_inbox =
ExecutionInbox::<T>::get((domain_id, domain_block_number, consensus_block_number));
let expected_extrinsics_roots: Vec<_> =
execution_inbox.iter().map(|b| b.extrinsics_root).collect();
ensure!(
!bundles_extrinsics_roots.is_empty()
&& bundles_extrinsics_roots == expected_extrinsics_roots,
Error::InvalidExtrinsicsRoots
);

// Check if the `execution_trace_root` is well-format
let mut trace = Vec::with_capacity(execution_trace.len());
for root in execution_trace {
trace.push(
root.encode()
.try_into()
.map_err(|_| Error::InvalidTraceRoot)?,
);
}
let expected_execution_trace_root: sp_core::H256 = MerkleTree::from_leaves(trace.as_slice())
.root()
.ok_or(Error::InvalidTraceRoot)?
.into();
ensure!(
expected_execution_trace_root == *execution_trace_root,
Error::InvalidTraceRoot
);

// Check if the ER is extending an existing parent ER
if let Some(parent_block_number) = domain_block_number.checked_sub(&One::one()) {
let parent_block_exist = does_receipt_exists::<T>(
domain_id,
Expand All @@ -231,26 +243,7 @@ pub(crate) fn verify_execution_receipt<T: Config>(
ensure!(parent_block_exist, Error::UnknownParentBlockReceipt);
}

match execution_receipt_type::<T>(domain_id, execution_receipt) {
ReceiptType::Rejected(RejectedReceiptType::InFuture) => {
log::error!(
target: "runtime::domains",
"Unexpected in future receipt {execution_receipt:?}, which should result in \
`UnknownParentBlockReceipt` error as it parent receipt is missing"
);
Err(Error::InFutureReceipt)
}
ReceiptType::Rejected(RejectedReceiptType::Pruned) => {
log::error!(
target: "runtime::domains",
"Unexpected pruned receipt {execution_receipt:?}, which should result in \
`InvalidExtrinsicsRoots` error as its `ExecutionInbox` is pruned at the same time"
);
Err(Error::PrunedReceipt)
}
ReceiptType::Rejected(rejected_receipt_type) => Err(rejected_receipt_type.into()),
ReceiptType::Accepted(_) => Ok(()),
}
Ok(())
}

/// Details of the confirmed domain block such as operators, rewards they would receive.
Expand Down Expand Up @@ -440,9 +433,11 @@ mod tests {
domain_id,
&genesis_receipt
));
// Submitting an invalid genesis ER will result in `NewBranchReceipt` because the operator
// need to submit fraud proof to pruned a ER first before submitting an ER at the same height
assert_err!(
verify_execution_receipt::<Test>(domain_id, &invalid_genesis_receipt),
Error::BadGenesisReceipt
Error::NewBranchReceipt
);
});
}
Expand Down Expand Up @@ -539,7 +534,7 @@ mod tests {
}

// The receipt of the block 1 is pruned at the last iteration, verify it will result in
// `InvalidExtrinsicsRoots` error as `ExecutionInbox` of block 1 is pruned
// `PrunedReceipt` error
let pruned_receipt = receipt_of_block_1.unwrap();
let pruned_bundle = bundle_header_hash_of_block_1.unwrap();
assert!(BlockTree::<Test>::get(domain_id, 1).is_none());
Expand All @@ -551,7 +546,7 @@ mod tests {
);
assert_err!(
verify_execution_receipt::<Test>(domain_id, &pruned_receipt),
Error::InvalidExtrinsicsRoots
Error::PrunedReceipt
);
assert!(ConsensusBlockHash::<Test>::get(
domain_id,
Expand Down Expand Up @@ -767,16 +762,11 @@ mod tests {
let mut ext = new_test_ext_with_extensions();
ext.execute_with(|| {
let domain_id = register_genesis_domain(creator, vec![operator_id]);
extend_block_tree_from_zero(domain_id, operator_id, 3);

let next_receipt = extend_block_tree_from_zero(domain_id, operator_id, 3);
let head_receipt_number = HeadReceiptNumber::<Test>::get(domain_id);
let current_head_receipt =
get_block_tree_node_at::<Test>(domain_id, head_receipt_number)
.unwrap()
.execution_receipt;

// Construct a future receipt
let mut future_receipt = current_head_receipt.clone();
let mut future_receipt = next_receipt.clone();
future_receipt.domain_block_number = head_receipt_number + 2;
future_receipt.consensus_block_number = head_receipt_number as u64 + 2;
ExecutionInbox::<Test>::insert(
Expand All @@ -799,26 +789,13 @@ 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
Error::InFutureReceipt
);

// Receipt with unknown extrinsics roots
let mut unknown_extrinsics_roots_receipt = current_head_receipt.clone();
let mut unknown_extrinsics_roots_receipt = next_receipt.clone();
unknown_extrinsics_roots_receipt.inboxed_bundles =
vec![InboxedBundle::valid(H256::random(), H256::random())];
assert_err!(
Expand All @@ -827,15 +804,15 @@ mod tests {
);

// Receipt with unknown consensus block hash
let mut unknown_consensus_block_receipt = current_head_receipt.clone();
let mut unknown_consensus_block_receipt = next_receipt.clone();
unknown_consensus_block_receipt.consensus_block_hash = H256::random();
assert_err!(
verify_execution_receipt::<Test>(domain_id, &unknown_consensus_block_receipt),
Error::BuiltOnUnknownConsensusBlock
);

// Receipt with unknown parent receipt
let mut unknown_parent_receipt = current_head_receipt;
let mut unknown_parent_receipt = next_receipt;
unknown_parent_receipt.parent_domain_block_receipt_hash = H256::random();
assert_err!(
verify_execution_receipt::<Test>(domain_id, &unknown_parent_receipt),
Expand All @@ -852,42 +829,34 @@ mod tests {
let mut ext = new_test_ext_with_extensions();
ext.execute_with(|| {
let domain_id = register_genesis_domain(creator, vec![operator_id1, operator_id2]);
extend_block_tree_from_zero(domain_id, operator_id1, 3);

let head_receipt_number = HeadReceiptNumber::<Test>::get(domain_id);

// Get the head receipt
let current_head_receipt =
get_block_tree_node_at::<Test>(domain_id, head_receipt_number)
.unwrap()
.execution_receipt;
let next_receipt = extend_block_tree_from_zero(domain_id, operator_id1, 3);

// Receipt with wrong value of `execution_trace_root`
let mut invalid_receipt = current_head_receipt.clone();
let mut invalid_receipt = next_receipt.clone();
invalid_receipt.execution_trace_root = H256::random();
assert_err!(
verify_execution_receipt::<Test>(domain_id, &invalid_receipt),
Error::InvalidTraceRoot
);

// Receipt with wrong value of trace
let mut invalid_receipt = current_head_receipt.clone();
let mut invalid_receipt = next_receipt.clone();
invalid_receipt.execution_trace[0] = H256::random();
assert_err!(
verify_execution_receipt::<Test>(domain_id, &invalid_receipt),
Error::InvalidTraceRoot
);

// Receipt with addtional trace
let mut invalid_receipt = current_head_receipt.clone();
let mut invalid_receipt = next_receipt.clone();
invalid_receipt.execution_trace.push(H256::random());
assert_err!(
verify_execution_receipt::<Test>(domain_id, &invalid_receipt),
Error::InvalidTraceRoot
);

// Receipt with missing trace
let mut invalid_receipt = current_head_receipt;
let mut invalid_receipt = next_receipt;
invalid_receipt.execution_trace.pop();
assert_err!(
verify_execution_receipt::<Test>(domain_id, &invalid_receipt),
Expand Down
25 changes: 20 additions & 5 deletions crates/pallet-domains/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1279,10 +1279,25 @@ mod pallet {
match call {
Call::submit_bundle { opaque_bundle } => {
if let Err(e) = Self::validate_bundle(opaque_bundle) {
log::error!(
target: "runtime::domains",
"Bad bundle {:?}, error: {e:?}", opaque_bundle.domain_id(),
);
match e {
// These errors are common due to networking delay or chain re-org,
// using a lower log level to avoid the noise.
BundleError::Receipt(BlockTreeError::InFutureReceipt)
| BundleError::Receipt(BlockTreeError::StaleReceipt)
| BundleError::Receipt(BlockTreeError::NewBranchReceipt)
| BundleError::Receipt(BlockTreeError::BuiltOnUnknownConsensusBlock) => {
log::debug!(
target: "runtime::domains",
"Bad bundle {:?}, error: {e:?}", opaque_bundle.domain_id(),
);
}
_ => {
log::warn!(
target: "runtime::domains",
"Bad bundle {:?}, error: {e:?}", opaque_bundle.domain_id(),
);
}
}
if let BundleError::Receipt(_) = e {
return InvalidTransactionCode::ExecutionReceipt.into();
} else {
Expand All @@ -1301,7 +1316,7 @@ mod pallet {
}
Call::submit_fraud_proof { fraud_proof } => {
if let Err(e) = Self::validate_fraud_proof(fraud_proof) {
log::error!(
log::warn!(
target: "runtime::domains",
"Bad fraud proof {:?}, error: {e:?}", fraud_proof.domain_id(),
);
Expand Down

0 comments on commit 0905475

Please sign in to comment.