From 288ea3ec380bfa0dc78b8aeac68e54799accce4c Mon Sep 17 00:00:00 2001 From: linning Date: Fri, 24 May 2024 05:43:15 +0800 Subject: [PATCH] Proper handling of bad invalid bundle ER that point to non-exist extrinsic Signed-off-by: linning --- .../src/verification.rs | 41 ++- crates/sp-domains/src/lib.rs | 9 + .../client/domain-operator/src/fraud_proof.rs | 235 ++++++++++-------- domains/client/domain-operator/src/tests.rs | 116 ++++++++- 4 files changed, 278 insertions(+), 123 deletions(-) diff --git a/crates/sp-domains-fraud-proof/src/verification.rs b/crates/sp-domains-fraud-proof/src/verification.rs index ff982507e1..2234b4b463 100644 --- a/crates/sp-domains-fraud-proof/src/verification.rs +++ b/crates/sp-domains-fraud-proof/src/verification.rs @@ -535,20 +535,42 @@ where } = invalid_bundles_fraud_proof; let (bundle_index, is_true_invalid_fraud_proof) = (*bundle_index, *is_true_invalid_fraud_proof); - let invalid_bundle_entry = check_expected_bundle_entry::( + let targeted_invalid_bundle_entry = check_expected_bundle_entry::( &bad_receipt, bundle_index, invalid_bundle_type.clone(), is_true_invalid_fraud_proof, )?; + let bundle_extrinsic_root = targeted_invalid_bundle_entry.extrinsics_root; + + // Verify the bundle proof so in following we can use the bundle dirctly + match proof_data { + InvalidBundlesProofData::Bundle(bundle_with_proof) + | InvalidBundlesProofData::BundleAndExecution { + bundle_with_proof, .. + } => { + if bundle_with_proof.bundle_index != bundle_index { + return Err(VerificationError::UnexpectedInvalidBundleProofData); + } + bundle_with_proof.verify::(domain_id, &state_root)?; + } + InvalidBundlesProofData::Extrinsic(_) => {} + } + + // Fast path to check if the fraud proof is targetting a bad receipt that claim a non-exist extrinsic + // is invalid + if let Some(invalid_extrinsic_index) = targeted_invalid_bundle_entry.invalid_extrinsic_index() { + if let InvalidBundlesProofData::Bundle(bundle_with_proof) = proof_data { + if bundle_with_proof.bundle.extrinsics.len() as u32 <= invalid_extrinsic_index { + return Ok(()); + } + } + } match &invalid_bundle_type { InvalidBundleType::OutOfRangeTx(extrinsic_index) => { let bundle = match proof_data { - InvalidBundlesProofData::Bundle(bundle_with_proof) - if bundle_with_proof.bundle_index == bundle_index => - { - bundle_with_proof.verify::(domain_id, &state_root)?; + InvalidBundlesProofData::Bundle(bundle_with_proof) => { bundle_with_proof.bundle.clone() } _ => return Err(VerificationError::UnexpectedInvalidBundleProofData), @@ -590,7 +612,7 @@ where }; get_extrinsic_from_proof::( *extrinsic_index, - invalid_bundle_entry.extrinsics_root, + bundle_extrinsic_root, extrinsic_storage_proof, )? }; @@ -614,10 +636,7 @@ where InvalidBundlesProofData::BundleAndExecution { bundle_with_proof, execution_proof, - } if bundle_with_proof.bundle_index == bundle_index => { - bundle_with_proof.verify::(domain_id, &state_root)?; - (bundle_with_proof.bundle.clone(), execution_proof.clone()) - } + } => (bundle_with_proof.bundle.clone(), execution_proof.clone()), _ => return Err(VerificationError::UnexpectedInvalidBundleProofData), }; @@ -660,7 +679,7 @@ where }; get_extrinsic_from_proof::( *extrinsic_index, - invalid_bundle_entry.extrinsics_root, + bundle_extrinsic_root, extrinsic_storage_proof, )? }; diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index cf4c541f9b..b7038d4740 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -1125,6 +1125,15 @@ impl InboxedBundle { matches!(self.bundle, BundleValidity::Invalid(_)) } + pub fn invalid_extrinsic_index(&self) -> Option { + match &self.bundle { + BundleValidity::Invalid(invalid_bundle_type) => { + Some(invalid_bundle_type.extrinsic_index()) + } + BundleValidity::Valid(_) => None, + } + } + #[cfg(any(feature = "std", feature = "runtime-benchmarks"))] pub fn dummy(extrinsics_root: Hash) -> Self where diff --git a/domains/client/domain-operator/src/fraud_proof.rs b/domains/client/domain-operator/src/fraud_proof.rs index 74f5c7cb53..a50167435a 100644 --- a/domains/client/domain-operator/src/fraud_proof.rs +++ b/domains/client/domain-operator/src/fraud_proof.rs @@ -553,125 +553,140 @@ where }; let extrinsic_index = invalid_type.extrinsic_index(); - let encoded_extrinsics: Vec<_> = bundle.extrinsics.iter().map(Encode::encode).collect(); - let proof_data = match invalid_type { - InvalidBundleType::IllegalTx(expected_extrinsic_index) => { - if expected_extrinsic_index as usize >= bundle.extrinsics.len() { - return Err(FraudProofError::OutOfBoundsExtrinsicIndex { - index: expected_extrinsic_index as usize, - max: bundle.extrinsics.len(), - }); - } + let proof_data = if bundle.extrinsics.len() as u32 <= extrinsic_index { + // The bad receipt claims a non-exist extrinsic is invalid, in this case, generate a + // `bundle_with_proof` as proof data is enough + let bundle_with_proof = OpaqueBundleWithProof::generate( + &self.storage_key_provider, + self.consensus_client.as_ref(), + domain_id, + consensus_block_hash, + bundle, + bundle_index, + )?; + InvalidBundlesProofData::Bundle(bundle_with_proof) + } else { + match invalid_type { + InvalidBundleType::IllegalTx(expected_extrinsic_index) => { + if expected_extrinsic_index as usize >= bundle.extrinsics.len() { + return Err(FraudProofError::OutOfBoundsExtrinsicIndex { + index: expected_extrinsic_index as usize, + max: bundle.extrinsics.len(), + }); + } - let domain_block_parent_hash = *self - .client - .header(local_receipt.domain_block_hash)? - .ok_or_else(|| { - FraudProofError::Blockchain(sp_blockchain::Error::MissingHeader(format!( - "{:?}", - local_receipt.domain_block_hash - ))) - })? - .parent_hash(); - - let domain_block_parent_number = self - .client - .block_number_from_id(&BlockId::Hash(domain_block_parent_hash))? - .ok_or_else(|| { - FraudProofError::Blockchain(sp_blockchain::Error::Backend(format!( - "unable to get block number for domain block:{:?}", - domain_block_parent_hash - ))) - })?; + let domain_block_parent_hash = *self + .client + .header(local_receipt.domain_block_hash)? + .ok_or_else(|| { + FraudProofError::Blockchain(sp_blockchain::Error::MissingHeader( + format!("{:?}", local_receipt.domain_block_hash), + )) + })? + .parent_hash(); + + let domain_block_parent_number = self + .client + .block_number_from_id(&BlockId::Hash(domain_block_parent_hash))? + .ok_or_else(|| { + FraudProofError::Blockchain(sp_blockchain::Error::Backend(format!( + "unable to get block number for domain block:{:?}", + domain_block_parent_hash + ))) + })?; + + let mut runtime_api_instance = self.client.runtime_api(); + runtime_api_instance.record_proof(); + let proof_recorder = runtime_api_instance + .proof_recorder() + .expect("we enabled proof recording just above; qed"); + + let mut block_extrinsics = vec![]; + for (extrinsic_index, extrinsic) in bundle + .extrinsics + .iter() + .enumerate() + .take((expected_extrinsic_index + 1) as usize) + { + let encoded_extrinsic = extrinsic.encode(); + block_extrinsics.push( + ::Extrinsic::decode(&mut encoded_extrinsic.as_slice()) + .map_err(|decoding_error| { + FraudProofError::UnableToDecodeOpaqueBundleExtrinsic { + extrinsic_index, + decoding_error, + } + })?, + ); + } - let mut runtime_api_instance = self.client.runtime_api(); - runtime_api_instance.record_proof(); - let proof_recorder = runtime_api_instance - .proof_recorder() - .expect("we enabled proof recording just above; qed"); - - let mut block_extrinsics = vec![]; - for (extrinsic_index, extrinsic) in bundle - .extrinsics - .iter() - .enumerate() - .take((expected_extrinsic_index + 1) as usize) - { - let encoded_extrinsic = extrinsic.encode(); - block_extrinsics.push( - ::Extrinsic::decode(&mut encoded_extrinsic.as_slice()) - .map_err(|decoding_error| { - FraudProofError::UnableToDecodeOpaqueBundleExtrinsic { - extrinsic_index, - decoding_error, - } - })?, - ); - } + let validation_response = runtime_api_instance + .check_extrinsics_and_do_pre_dispatch( + domain_block_parent_hash, + block_extrinsics, + domain_block_parent_number, + domain_block_parent_hash, + )?; + + // If the proof is true invalid then validation response should not be Ok. + // If the proof is false invalid then validation response should not be Err. + // OR + // If it is true invalid and expected extrinsic index does not match + if (is_true_invalid == validation_response.is_ok()) + || (is_true_invalid + && validation_response + .as_ref() + .is_err_and(|e| e.extrinsic_index != expected_extrinsic_index)) + { + return Err(FraudProofError::InvalidIllegalTxFraudProofExtrinsicIndex { + index: expected_extrinsic_index as usize, + is_true_invalid, + extrinsics_validity_response: validation_response, + }); + } - let validation_response = runtime_api_instance - .check_extrinsics_and_do_pre_dispatch( - domain_block_parent_hash, - block_extrinsics, - domain_block_parent_number, - domain_block_parent_hash, + let execution_proof = proof_recorder.drain_storage_proof(); + + let bundle_with_proof = OpaqueBundleWithProof::generate( + &self.storage_key_provider, + self.consensus_client.as_ref(), + domain_id, + consensus_block_hash, + bundle, + bundle_index, )?; - // If the proof is true invalid then validation response should not be Ok. - // If the proof is false invalid then validation response should not be Err. - // OR - // If it is true invalid and expected extrinsic index does not match - if (is_true_invalid == validation_response.is_ok()) - || (is_true_invalid - && validation_response - .as_ref() - .is_err_and(|e| e.extrinsic_index != expected_extrinsic_index)) - { - return Err(FraudProofError::InvalidIllegalTxFraudProofExtrinsicIndex { - index: expected_extrinsic_index as usize, - is_true_invalid, - extrinsics_validity_response: validation_response, - }); + InvalidBundlesProofData::BundleAndExecution { + bundle_with_proof, + execution_proof, + } } + InvalidBundleType::OutOfRangeTx(_) => { + let bundle_with_proof = OpaqueBundleWithProof::generate( + &self.storage_key_provider, + self.consensus_client.as_ref(), + domain_id, + consensus_block_hash, + bundle, + bundle_index, + )?; - let execution_proof = proof_recorder.drain_storage_proof(); - - let bundle_with_proof = OpaqueBundleWithProof::generate( - &self.storage_key_provider, - self.consensus_client.as_ref(), - domain_id, - consensus_block_hash, - bundle, - bundle_index, - )?; - - InvalidBundlesProofData::BundleAndExecution { - bundle_with_proof, - execution_proof, + InvalidBundlesProofData::Bundle(bundle_with_proof) } - } - InvalidBundleType::OutOfRangeTx(_) => { - let bundle_with_proof = OpaqueBundleWithProof::generate( - &self.storage_key_provider, - self.consensus_client.as_ref(), - domain_id, - consensus_block_hash, - bundle, - bundle_index, - )?; - - InvalidBundlesProofData::Bundle(bundle_with_proof) - } - InvalidBundleType::UndecodableTx(_) | InvalidBundleType::InherentExtrinsic(_) => { - let extrinsic_proof = StorageProofProvider::< - LayoutV1>, - >::generate_enumerated_proof_of_inclusion( - encoded_extrinsics.as_slice(), - extrinsic_index, - ) - .ok_or(FraudProofError::FailToGenerateProofOfInclusion)?; + InvalidBundleType::UndecodableTx(_) | InvalidBundleType::InherentExtrinsic(_) => { + let encoded_extrinsics: Vec<_> = + bundle.extrinsics.iter().map(Encode::encode).collect(); + + let extrinsic_proof = StorageProofProvider::< + LayoutV1>, + >::generate_enumerated_proof_of_inclusion( + encoded_extrinsics.as_slice(), + extrinsic_index, + ) + .ok_or(FraudProofError::FailToGenerateProofOfInclusion)?; - InvalidBundlesProofData::Extrinsic(extrinsic_proof) + InvalidBundlesProofData::Extrinsic(extrinsic_proof) + } } }; diff --git a/domains/client/domain-operator/src/tests.rs b/domains/client/domain-operator/src/tests.rs index 504f300931..3ca35438ea 100644 --- a/domains/client/domain-operator/src/tests.rs +++ b/domains/client/domain-operator/src/tests.rs @@ -34,8 +34,8 @@ use sp_domains::{ }; use sp_domains_fraud_proof::fraud_proof::{ ApplyExtrinsicMismatch, ExecutionPhase, FinalizeBlockMismatch, FraudProofVariant, - InvalidBlockFeesProof, InvalidDomainBlockHashProof, InvalidExtrinsicsRootProof, - InvalidTransfersProof, + InvalidBlockFeesProof, InvalidBundlesProofData, InvalidDomainBlockHashProof, + InvalidExtrinsicsRootProof, InvalidTransfersProof, }; use sp_domains_fraud_proof::InvalidTransactionCode; use sp_messenger::messages::{CrossDomainMessage, FeeModel, InitiateChannelParams, Proof}; @@ -1954,6 +1954,118 @@ async fn test_false_invalid_bundles_illegal_extrinsic_proof_creation_and_verific assert!(!ferdie.does_receipt_exist(bad_receipt_hash).unwrap()); } +#[tokio::test(flavor = "multi_thread")] +async fn test_false_invalid_bundles_non_exist_extrinsic_proof_creation_and_verification() { + let directory = TempDir::new().expect("Must be able to create temporary directory"); + + let mut builder = sc_cli::LoggerBuilder::new(""); + builder.with_colors(false); + let _ = builder.init(); + + let tokio_handle = tokio::runtime::Handle::current(); + + // Start Ferdie + let mut ferdie = MockConsensusNode::run( + tokio_handle.clone(), + Ferdie, + BasePath::new(directory.path().join("ferdie")), + ); + + // Run Alice (a evm domain authority node) + let mut alice = domain_test_service::DomainNodeBuilder::new( + tokio_handle.clone(), + Alice, + BasePath::new(directory.path().join("alice")), + ) + .build_evm_node(Role::Authority, GENESIS_DOMAIN_ID, &mut ferdie) + .await; + + let bundle_to_tx = |opaque_bundle| { + subspace_test_runtime::UncheckedExtrinsic::new_unsigned( + pallet_domains::Call::submit_bundle { opaque_bundle }.into(), + ) + .into() + }; + + produce_blocks!(ferdie, alice, 5).await.unwrap(); + + alice + .construct_and_send_extrinsic(pallet_balances::Call::transfer_allow_death { + dest: Bob.to_account_id(), + value: 1, + }) + .await + .expect("Failed to send extrinsic"); + + // Produce a bundle that contains the previously sent extrinsic and record that bundle for later use + let (slot, target_bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await; + assert_eq!(target_bundle.extrinsics.len(), 1); + let extrinsics: Vec> = target_bundle + .extrinsics + .clone() + .into_iter() + .map(|ext| ext.encode()) + .collect(); + let bundle_extrinsic_root = + BlakeTwo256::ordered_trie_root(extrinsics.clone(), StateVersion::V1); + produce_block_with!(ferdie.produce_block_with_slot(slot), alice) + .await + .unwrap(); + + // produce another bundle that marks the previous valid extrinsic as invalid. + let (slot, mut opaque_bundle) = ferdie.produce_slot_and_wait_for_bundle_submission().await; + + let (bad_receipt_hash, bad_submit_bundle_tx) = { + let bad_receipt = &mut opaque_bundle.sealed_header.header.receipt; + // bad receipt marks a non-exist extrinsic as invalid + bad_receipt.inboxed_bundles = vec![InboxedBundle::invalid( + InvalidBundleType::InherentExtrinsic(u32::MAX), + bundle_extrinsic_root, + )]; + + opaque_bundle.sealed_header.signature = Sr25519Keyring::Alice + .pair() + .sign(opaque_bundle.sealed_header.pre_hash().as_ref()) + .into(); + ( + opaque_bundle.receipt().hash::(), + bundle_to_tx(opaque_bundle), + ) + }; + + // Wait for the fraud proof that target the bad ER + let wait_for_fraud_proof_fut = ferdie.wait_for_fraud_proof(move |fp| { + if let FraudProofVariant::InvalidBundles(proof) = &fp.proof { + if let InvalidBundlesProofData::Bundle(_) = proof.proof_data { + assert_eq!(fp.targeted_bad_receipt_hash(), bad_receipt_hash); + return true; + } + } + false + }); + + // Produce a consensus block that contains the `bad_submit_bundle_tx` and the bad receipt should + // be added to the consensus chain block tree + produce_block_with!( + ferdie.produce_block_with_slot_at( + slot, + ferdie.client.info().best_hash, + Some(vec![bad_submit_bundle_tx]) + ), + alice + ) + .await + .unwrap(); + assert!(ferdie.does_receipt_exist(bad_receipt_hash).unwrap()); + + let _ = wait_for_fraud_proof_fut.await; + + // Produce a consensus block that contains the fraud proof, the fraud proof wil be verified + // and executed, thus pruned the bad receipt from the block tree + ferdie.produce_blocks(1).await.unwrap(); + assert!(!ferdie.does_receipt_exist(bad_receipt_hash).unwrap()); +} + #[tokio::test(flavor = "multi_thread")] async fn test_invalid_block_fees_proof_creation() { let directory = TempDir::new().expect("Must be able to create temporary directory");