From 7f001ccb6ce2f65103aaec9f5395d242524134ec Mon Sep 17 00:00:00 2001 From: linning Date: Tue, 7 Nov 2023 23:16:13 +0800 Subject: [PATCH 1/3] Reorder the ER verification steps Pure refactoring, the validity of ER is not change, while an invalid ER will still be considered as invalid, the verification will return another error. This is a short circuit for some general and expected error like Stale Receipt Signed-off-by: linning --- crates/pallet-domains/src/block_tree.rs | 189 ++++++++++-------------- 1 file changed, 79 insertions(+), 110 deletions(-) diff --git a/crates/pallet-domains/src/block_tree.rs b/crates/pallet-domains/src/block_tree.rs index 60507e188c..0169212ba8 100644 --- a/crates/pallet-domains/src/block_tree.rs +++ b/crates/pallet-domains/src/block_tree.rs @@ -158,9 +158,17 @@ pub(crate) fn verify_execution_receipt( .. } = execution_receipt; + // Checking if the incoming ER is expected regarding to its `domain_block_number`/freshness + if let ReceiptType::Rejected(rejected_receipt_type) = + execution_receipt_type::(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::( domain_id, @@ -169,59 +177,63 @@ pub(crate) fn verify_execution_receipt( ), Error::BadGenesisReceipt ); - } else { - let bundles_extrinsics_roots: Vec<_> = - inboxed_bundles.iter().map(|b| b.extrinsics_root).collect(); - let execution_inbox = - ExecutionInbox::::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::::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); - } + // Check if the ER is derived from the correct consensus block in the current chain + 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 + } + }; + 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::::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::( domain_id, @@ -231,26 +243,7 @@ pub(crate) fn verify_execution_receipt( ensure!(parent_block_exist, Error::UnknownParentBlockReceipt); } - match execution_receipt_type::(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. @@ -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::(domain_id, &invalid_genesis_receipt), - Error::BadGenesisReceipt + Error::NewBranchReceipt ); }); } @@ -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::::get(domain_id, 1).is_none()); @@ -551,7 +546,7 @@ mod tests { ); assert_err!( verify_execution_receipt::(domain_id, &pruned_receipt), - Error::InvalidExtrinsicsRoots + Error::PrunedReceipt ); assert!(ConsensusBlockHash::::get( domain_id, @@ -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::::get(domain_id); - let current_head_receipt = - get_block_tree_node_at::(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::::insert( @@ -799,26 +789,13 @@ 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 + 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!( @@ -827,7 +804,7 @@ 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::(domain_id, &unknown_consensus_block_receipt), @@ -835,7 +812,7 @@ mod tests { ); // 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::(domain_id, &unknown_parent_receipt), @@ -852,18 +829,10 @@ 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::::get(domain_id); - - // Get the head receipt - let current_head_receipt = - get_block_tree_node_at::(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::(domain_id, &invalid_receipt), @@ -871,7 +840,7 @@ mod tests { ); // 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::(domain_id, &invalid_receipt), @@ -879,7 +848,7 @@ mod tests { ); // 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::(domain_id, &invalid_receipt), @@ -887,7 +856,7 @@ mod tests { ); // 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::(domain_id, &invalid_receipt), From 3fcbfdab97a5f3b30f392023d5473266c2a92835 Mon Sep 17 00:00:00 2001 From: linning Date: Tue, 7 Nov 2023 23:39:55 +0800 Subject: [PATCH 2/3] Downgrade log level for validate_bundle/validate_fraud_proof Using warn! instead of error! since the error is expected when the malicious operator is involved, and using debug! for some common block tree errors that were due to networking delay or chain re-org to avoid the noise. Signed-off-by: linning --- crates/pallet-domains/src/lib.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index 1193b75a3f..fe16a549e1 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/crates/pallet-domains/src/lib.rs @@ -1277,10 +1277,24 @@ 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::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 { @@ -1299,7 +1313,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(), ); From c56ac401118545081fdef1c5b2c7722823749838 Mon Sep 17 00:00:00 2001 From: linning Date: Wed, 8 Nov 2023 22:57:49 +0800 Subject: [PATCH 3/3] Downgrade log level for the InFutureReceipt error in pallet-domains Signed-off-by: linning --- crates/pallet-domains/src/block_tree.rs | 2 +- crates/pallet-domains/src/lib.rs | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/pallet-domains/src/block_tree.rs b/crates/pallet-domains/src/block_tree.rs index 0169212ba8..5719902209 100644 --- a/crates/pallet-domains/src/block_tree.rs +++ b/crates/pallet-domains/src/block_tree.rs @@ -158,7 +158,7 @@ pub(crate) fn verify_execution_receipt( .. } = execution_receipt; - // Checking if the incoming ER is expected regarding to its `domain_block_number`/freshness + // 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::(domain_id, execution_receipt) { diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index fe16a549e1..5e32b5a3e7 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/crates/pallet-domains/src/lib.rs @@ -1280,7 +1280,8 @@ mod pallet { 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::StaleReceipt) + BundleError::Receipt(BlockTreeError::InFutureReceipt) + | BundleError::Receipt(BlockTreeError::StaleReceipt) | BundleError::Receipt(BlockTreeError::NewBranchReceipt) | BundleError::Receipt(BlockTreeError::BuiltOnUnknownConsensusBlock) => { log::debug!(