diff --git a/Cargo.lock b/Cargo.lock index 4604afaa0e..0a47a5b61e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10988,6 +10988,7 @@ dependencies = [ "domain-runtime-primitives", "domain-test-service", "frame-support", + "frame-system", "futures", "hash-db 0.16.0", "log", diff --git a/crates/sp-domains-fraud-proof/Cargo.toml b/crates/sp-domains-fraud-proof/Cargo.toml index da5e0ad63b..e201947b39 100644 --- a/crates/sp-domains-fraud-proof/Cargo.toml +++ b/crates/sp-domains-fraud-proof/Cargo.toml @@ -41,6 +41,7 @@ thiserror = { version = "1.0.48", optional = true } domain-block-builder = { version = "0.1.0", path = "../../domains/client/block-builder" } domain-block-preprocessor = { version = "0.1.0", path = "../../domains/client/block-preprocessor" } domain-test-service = { version = "0.1.0", path = "../../domains/test/service" } +frame-system = { version = "4.0.0-dev", default-features = false, git = "https://github.com/subspace/polkadot-sdk", rev = "c63a8b28a9fd26d42116b0dcef1f2a5cefb9cd1c" } futures = "0.3.29" pallet-balances = { version = "4.0.0-dev", git = "https://github.com/subspace/polkadot-sdk", rev = "c63a8b28a9fd26d42116b0dcef1f2a5cefb9cd1c" } sc-cli = { version = "0.10.0-dev", git = "https://github.com/subspace/polkadot-sdk", rev = "c63a8b28a9fd26d42116b0dcef1f2a5cefb9cd1c", default-features = false } diff --git a/crates/sp-domains-fraud-proof/src/tests.rs b/crates/sp-domains-fraud-proof/src/tests.rs index a187949acd..946d01a2d5 100644 --- a/crates/sp-domains-fraud-proof/src/tests.rs +++ b/crates/sp-domains-fraud-proof/src/tests.rs @@ -1,46 +1,18 @@ use codec::Encode; -use domain_block_preprocessor::stateless_runtime::StatelessRuntime; -use domain_runtime_primitives::{CheckTxValidityError, DomainCoreApi}; -use domain_test_service::domain::EvmDomainClient as DomainClient; -use domain_test_service::evm_domain_test_runtime::Runtime as TestRuntime; -use domain_test_service::EcdsaKeyring::{Alice, Bob, Charlie}; +use domain_runtime_primitives::DomainCoreApi; +use domain_test_service::EcdsaKeyring::{Alice, Charlie}; use domain_test_service::Sr25519Keyring::Ferdie; -use domain_test_service::{construct_extrinsic_generic, GENESIS_DOMAIN_ID}; -use sc_client_api::{HeaderBackend, ProofProvider, StorageProof}; +use domain_test_service::GENESIS_DOMAIN_ID; +use sc_client_api::HeaderBackend; use sc_service::{BasePath, Role}; -use sp_api::{BlockT, ProvideRuntimeApi}; -use sp_domains::DomainsApi; -use sp_runtime::traits::{BlakeTwo256, Header as HeaderT}; +use sp_api::{ApiExt, ProvideRuntimeApi, TransactionOutcome}; use sp_runtime::transaction_validity::{InvalidTransaction, TransactionValidityError}; -use sp_runtime::{OpaqueExtrinsic, Storage}; -use sp_trie::{read_trie_value, LayoutV1}; -use std::collections::BTreeMap; -use std::sync::Arc; -use subspace_runtime_primitives::opaque::Block; -use subspace_test_service::{produce_blocks, MockConsensusNode}; +use sp_runtime::OpaqueExtrinsic; +use subspace_test_service::MockConsensusNode; use tempfile::TempDir; -type HashFor = <::Header as HeaderT>::Hash; - -fn generate_storage_proof_for_tx_validity( - block_hash: HashFor, - client: Arc, - keys: Vec>, -) -> StorageProof { - client - .read_proof( - block_hash, - &mut keys - .iter() - .map(|k| k.as_slice()) - .collect::>() - .into_iter(), - ) - .unwrap() -} - #[tokio::test(flavor = "multi_thread")] -async fn check_tx_validity_runtime_api_should_work() { +async fn storage_change_of_the_same_runtime_instance_should_perserved_cross_runtime_calls() { let directory = TempDir::new().expect("Must be able to create temporary directory"); let mut builder = sc_cli::LoggerBuilder::new(""); @@ -57,7 +29,7 @@ async fn check_tx_validity_runtime_api_should_work() { ); // Run Alice (a evm domain authority node) - let alice = domain_test_service::DomainNodeBuilder::new( + let mut alice = domain_test_service::DomainNodeBuilder::new( tokio_handle.clone(), Alice, BasePath::new(directory.path().join("alice")), @@ -65,163 +37,355 @@ async fn check_tx_validity_runtime_api_should_work() { .build_evm_node(Role::Authority, GENESIS_DOMAIN_ID, &mut ferdie) .await; - // Run Bob (a evm domain full node) - let bob = domain_test_service::DomainNodeBuilder::new( - tokio_handle, - Bob, - BasePath::new(directory.path().join("bob")), - ) - .build_evm_node(Role::Full, GENESIS_DOMAIN_ID, &mut ferdie) - .await; + let best_hash = alice.client.as_ref().info().best_hash; + let best_number = alice.client.as_ref().info().best_number; + + // transfer most of the alice's balance + let alice_balance = alice.free_balance(Alice.to_account_id()); - // Bob is able to sync blocks. - produce_blocks!(ferdie, alice, 1, bob).await.unwrap(); + let mut alice_nonce = alice.account_nonce(); - let bob_nonce = bob.account_nonce(); - let transfer_to_charlie = construct_extrinsic_generic::( - &bob.client, + let transfer_to_charlie_with_big_tip_1 = alice.construct_extrinsic_with_tip( + alice_nonce, + (alice_balance / 3) * 2, pallet_balances::Call::transfer_allow_death { dest: Charlie.to_account_id(), - value: 8, + value: 1, }, - bob.key, - false, - bob_nonce + 1, - 1, ); + alice_nonce += 1; - produce_blocks!(ferdie, alice, 2, bob).await.unwrap(); - - let opaque_extrinsic = OpaqueExtrinsic::from_bytes(&transfer_to_charlie.encode()).unwrap(); - let extrinsic_best_hash = bob.client.as_ref().info().best_hash; - let bob_account_id = Bob.to_account_id().encode(); - let extrinsic_best_number = bob.client.as_ref().info().best_number; - - let extrinsic_era = bob - .client - .runtime_api() - .extrinsic_era(bob.client.as_ref().info().best_hash, &opaque_extrinsic) - .unwrap(); - - let test_table = vec![ - ( - Some(4), - Some(TransactionValidityError::Invalid( - InvalidTransaction::AncientBirthBlock, - )), - ), - ( - Some(2), - Some(TransactionValidityError::Invalid( - InvalidTransaction::Payment, - )), - ), - ( - Some(1), - Some(TransactionValidityError::Invalid( - InvalidTransaction::BadProof, - )), - ), - ( - Some(0), - Some(TransactionValidityError::Invalid( - InvalidTransaction::BadProof, - )), - ), - (None, None), - ]; + let transfer_to_charlie_with_big_tip_2 = alice.construct_extrinsic_with_tip( + alice_nonce, + (alice_balance / 3) * 2, + pallet_balances::Call::transfer_allow_death { + dest: Charlie.to_account_id(), + value: 1, + }, + ); - for table_data in test_table { - let mut storage_keys_for_tx_validity = bob + // A runtime instance preserve changes in between + let runtime_instance = alice.client.runtime_api(); + + // First transaction should be okay + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_to_charlie_with_big_tip_1.clone().into(), + best_number, + best_hash, + ) + .unwrap(), + Ok(()) + ); + + // Second transaction should error out with exact error encountered during check of bundle validity + // to prove that state is preserved between two calls to same runtime instance + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_to_charlie_with_big_tip_2.clone().into(), + best_number, + best_hash, + ) + .unwrap(), + Err(TransactionValidityError::Invalid( + InvalidTransaction::Payment + )) + ); + + // If second tx is executed in dedicated runtime instance it would error with `InvalidTransaction::Future` + // as the nonce at the block for this account is less than the tx's nonce. And there is no overlay + // preserved between runtime instance. + assert_eq!( + alice .client .runtime_api() - .storage_keys_for_verifying_transaction_validity( - extrinsic_best_hash, - bob_account_id.clone(), - extrinsic_best_number, - extrinsic_era, + .check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_to_charlie_with_big_tip_2.clone().into(), + best_number, + best_hash, ) - .unwrap() - .unwrap(); - - let maybe_storage_key_to_remove = table_data.0; - let original_storage_keys_for_tx_validity = storage_keys_for_tx_validity.clone(); - - if let Some(storage_key_to_remove) = maybe_storage_key_to_remove { - storage_keys_for_tx_validity = storage_keys_for_tx_validity - .drain(..) - .enumerate() - .filter(|(i, _v)| *i != storage_key_to_remove) - .map(|(_i, v)| v) - .collect(); + .unwrap(), + Err(TransactionValidityError::Invalid( + InvalidTransaction::Future + )) + ); + + let test_commit_mode = vec![true, false]; + + for commit_mode in test_commit_mode { + alice_nonce = alice.account_nonce(); + + let transfer_with_big_tip_1 = alice.construct_extrinsic_with_tip( + alice_nonce, + alice_balance / 3, + pallet_balances::Call::transfer_allow_death { + dest: Charlie.to_account_id(), + value: 1, + }, + ); + alice_nonce += 1; + + let transfer_with_big_tip_2 = alice.construct_extrinsic_with_tip( + alice_nonce, + alice_balance / 3, + pallet_balances::Call::transfer_allow_death { + dest: Charlie.to_account_id(), + value: 1, + }, + ); + + if commit_mode { + alice_nonce += 1; } - let maybe_error = table_data.1; - - let expected_out = if let Some(err) = maybe_error { - let err_with_storage_keys = CheckTxValidityError::InvalidTransaction { - error: err, - storage_keys: original_storage_keys_for_tx_validity, - }; - Err(err_with_storage_keys) - } else { - Ok(()) - }; - - let storage_proof = generate_storage_proof_for_tx_validity( - bob.client.as_ref().info().best_hash, - bob.client.clone(), - storage_keys_for_tx_validity.clone(), + let transfer_with_big_tip_3 = alice.construct_extrinsic_with_tip( + alice_nonce, + alice_balance / 3, + pallet_balances::Call::transfer_allow_death { + dest: Charlie.to_account_id(), + value: 1, + }, ); - let header = bob - .client - .as_ref() - .header(bob.client.as_ref().info().best_hash); - let state_root = header.unwrap().unwrap().state_root; + // A runtime instance can rollback changes safely. + let runtime_instance = alice.client.runtime_api(); - let wasm_bundle = ferdie - .client - .runtime_api() - .domain_runtime_code(ferdie.client.as_ref().info().best_hash, GENESIS_DOMAIN_ID) - .unwrap() - .unwrap(); - - let mut domain_stateless_runtime = - StatelessRuntime::::new(ferdie.executor.clone().into(), wasm_bundle.into()); - - let db = storage_proof.into_memory_db::(); - let mut top_storage_map = BTreeMap::new(); - for storage_key in storage_keys_for_tx_validity.iter() { - let storage_value = read_trie_value::, _>( - &db, - &state_root, - storage_key, - None, - None, + assert!(runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_with_big_tip_1.clone().into(), + best_number, + best_hash ) .unwrap() - .unwrap(); - top_storage_map.insert(storage_key.to_vec(), storage_value); - } - - let storage = Storage { - top: top_storage_map, - children_default: Default::default(), - }; - - domain_stateless_runtime.set_storage(storage); + .is_ok()); + + assert!(runtime_instance + .execute_in_transaction(|api| { + if commit_mode { + TransactionOutcome::Commit(api.check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_with_big_tip_2.clone().into(), + best_number, + best_hash, + )) + } else { + TransactionOutcome::Rollback(api.check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_with_big_tip_2.clone().into(), + best_number, + best_hash, + )) + } + }) + .is_ok()); assert_eq!( - domain_stateless_runtime - .check_transaction_validity( - &opaque_extrinsic, - bob.client.as_ref().info().best_number, - bob.client.as_ref().info().best_hash + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + &transfer_with_big_tip_3.clone().into(), + best_number, + best_hash ) .unwrap(), - expected_out - ); + if commit_mode { + Err(TransactionValidityError::Invalid( + InvalidTransaction::Payment, + )) + } else { + Ok(()) + } + ) + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn check_tx_validity_and_pre_dispatch_runtime_api_should_work() { + 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 mut alice_nonce = alice.account_nonce(); + let best_hash = alice.client.as_ref().info().best_hash; + let best_number = alice.client.as_ref().info().best_number; + let dummy_runtime_call = pallet_balances::Call::transfer_allow_death { + dest: Charlie.to_account_id(), + value: 1, + }; + + let transfer_to_charlie = + alice.construct_extrinsic_with_tip(alice_nonce, 0, dummy_runtime_call.clone()); + alice_nonce += 1; + + let transfer_to_charlie_2 = + alice.construct_extrinsic_with_tip(alice_nonce, 0, dummy_runtime_call.clone()); + + let sample_valid_bundle_extrinsics = vec![ + OpaqueExtrinsic::from_bytes(&transfer_to_charlie.encode()).unwrap(), + OpaqueExtrinsic::from_bytes(&transfer_to_charlie_2.encode()).unwrap(), + ]; + + { + let runtime_instance = alice.client.runtime_api(); + for extrinsic in sample_valid_bundle_extrinsics { + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + &extrinsic, + best_number, + best_hash, + ) + .unwrap(), + Ok(()) + ); + } + } + + let transfer_to_charlie_3_duplicate_nonce_extrinsic = + alice.construct_extrinsic_with_tip(alice_nonce, 0, dummy_runtime_call.clone()); + + // Here tx index: 2 is invalid + let sample_invalid_bundle_with_same_nonce_extrinsic = vec![ + OpaqueExtrinsic::from_bytes(&transfer_to_charlie.encode()).unwrap(), + OpaqueExtrinsic::from_bytes(&transfer_to_charlie_2.encode()).unwrap(), + OpaqueExtrinsic::from_bytes(&transfer_to_charlie_3_duplicate_nonce_extrinsic.encode()) + .unwrap(), + ]; + + { + let runtime_instance = alice.client.runtime_api(); + for (i, extrinsic) in sample_invalid_bundle_with_same_nonce_extrinsic + .iter() + .enumerate() + { + if i != 2 { + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + extrinsic, + best_number, + best_hash, + ) + .unwrap(), + Ok(()) + ); + } else { + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + extrinsic, + best_number, + best_hash, + ) + .unwrap(), + Err(TransactionValidityError::Invalid(InvalidTransaction::Stale)) + ); + } + } + } + + // Resetting the nonce + alice_nonce = alice.account_nonce(); + + // transfer most of the alice's balance + let alice_balance = alice.free_balance(Alice.to_account_id()); + + let transfer_to_charlie_with_big_tip_1 = alice.construct_extrinsic_with_tip( + alice_nonce, + (alice_balance / 3) * 2, + dummy_runtime_call.clone(), + ); + + alice_nonce += 1; + + let transfer_to_charlie_with_big_tip_2 = alice.construct_extrinsic_with_tip( + alice_nonce, + (alice_balance / 3) * 2, + dummy_runtime_call.clone(), + ); + + // In single tx context both are valid (both tx are identical except nonce), so no need to test both of them independently. + // only testing one should suffice + assert_eq!( + alice + .client + .runtime_api() + .check_transaction_and_do_pre_dispatch( + best_hash, + &OpaqueExtrinsic::from_bytes(&transfer_to_charlie_with_big_tip_1.encode()).unwrap(), + best_number, + best_hash, + ) + .unwrap(), + Ok(()) + ); + + // Tx index: 1 is invalid since low balance to pay for the tip + let transfer_to_charlie_with_big_tip = vec![ + transfer_to_charlie_with_big_tip_1.clone().into(), + transfer_to_charlie_with_big_tip_2.clone().into(), + ]; + + { + let runtime_instance = alice.client.runtime_api(); + for (i, extrinsic) in transfer_to_charlie_with_big_tip.iter().enumerate() { + if i == 1 { + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + extrinsic, + best_number, + best_hash, + ) + .unwrap(), + Err(TransactionValidityError::Invalid( + InvalidTransaction::Payment + )) + ); + } else { + assert_eq!( + runtime_instance + .check_transaction_and_do_pre_dispatch( + best_hash, + extrinsic, + best_number, + best_hash, + ) + .unwrap(), + Ok(()) + ); + } + } } } diff --git a/crates/sp-domains/src/lib.rs b/crates/sp-domains/src/lib.rs index 8d966dfd99..340bb1ac90 100644 --- a/crates/sp-domains/src/lib.rs +++ b/crates/sp-domains/src/lib.rs @@ -809,9 +809,9 @@ impl InvalidBundleType { match self { Self::UndecodableTx(_) => 1, Self::OutOfRangeTx(_) => 2, - Self::IllegalTx(_) => 3, - Self::InvalidXDM(_) => 4, - Self::InherentExtrinsic(_) => 5, + Self::InherentExtrinsic(_) => 3, + Self::IllegalTx(_) => 4, + Self::InvalidXDM(_) => 5, } } diff --git a/domains/client/block-preprocessor/src/lib.rs b/domains/client/block-preprocessor/src/lib.rs index 6d3c666806..01bb98352e 100644 --- a/domains/client/block-preprocessor/src/lib.rs +++ b/domains/client/block-preprocessor/src/lib.rs @@ -286,19 +286,6 @@ where ))); } - let is_legal_tx = self - .client - .runtime_api() - .check_transaction_validity(at, &extrinsic, domain_block_number, at)? - .is_ok(); - - if !is_legal_tx { - // TODO: Generate a fraud proof for this invalid bundle - return Ok(BundleValidity::Invalid(InvalidBundleType::IllegalTx( - index as u32, - ))); - } - // Check if this extrinsic is an inherent extrinsic. // If so, this is an invalid bundle since these extrinsics should not be included in the // bundle. Extrinsic is always decodable due to the check above. @@ -308,6 +295,20 @@ where )); } + // Using one instance of runtime_api throughout the loop in order to maintain context + // between them. + // Using `check_transaction_and_do_pre_dispatch` instead of `check_transaction_validity` + // to maintain side-effect in the storage buffer. + let is_legal_tx = runtime_api + .check_transaction_and_do_pre_dispatch(at, &extrinsic, domain_block_number, at)? + .is_ok(); + + if !is_legal_tx { + return Ok(BundleValidity::Invalid(InvalidBundleType::IllegalTx( + index as u32, + ))); + } + // TODO: the behavior is changed, as before invalid XDM will be dropped silently, // and the other extrinsic of the bundle will be continue processed, now the whole // bundle is considered as invalid and excluded from further processing. diff --git a/domains/client/block-preprocessor/src/stateless_runtime.rs b/domains/client/block-preprocessor/src/stateless_runtime.rs index 946e511dc0..9f02bbc0b3 100644 --- a/domains/client/block-preprocessor/src/stateless_runtime.rs +++ b/domains/client/block-preprocessor/src/stateless_runtime.rs @@ -1,12 +1,13 @@ use codec::{Codec, Encode}; use domain_runtime_primitives::opaque::AccountId; -use domain_runtime_primitives::{CheckTxValidityError, DomainCoreApi}; +use domain_runtime_primitives::DomainCoreApi; use sc_executor::RuntimeVersionOf; use sp_api::{ApiError, BlockT, Core, Hasher, RuntimeVersion}; use sp_core::traits::{CallContext, CodeExecutor, FetchRuntimeCode, RuntimeCode}; use sp_messenger::messages::ExtractedStateRootsFromProof; use sp_messenger::MessengerApi; use sp_runtime::traits::NumberFor; +use sp_runtime::transaction_validity::TransactionValidityError; use sp_runtime::Storage; use sp_state_machine::BasicExternalities; use std::borrow::Cow; @@ -212,13 +213,14 @@ where ) } - pub fn check_transaction_validity( + /// This is stateful runtime api call and require setting of storage keys. + pub fn check_transaction_and_do_pre_dispatch( &self, uxt: &::Extrinsic, block_number: NumberFor, block_hash: ::Hash, - ) -> Result, ApiError> { - >::check_transaction_validity( + ) -> Result, ApiError> { + >::check_transaction_and_do_pre_dispatch( self, Default::default(), uxt, diff --git a/domains/client/domain-operator/src/domain_bundle_proposer.rs b/domains/client/domain-operator/src/domain_bundle_proposer.rs index 971024a1fb..2de5f4f094 100644 --- a/domains/client/domain-operator/src/domain_bundle_proposer.rs +++ b/domains/client/domain-operator/src/domain_bundle_proposer.rs @@ -3,8 +3,8 @@ use codec::Encode; use domain_runtime_primitives::DomainCoreApi; use futures::{select, FutureExt}; use sc_client_api::{AuxStore, BlockBackend}; -use sc_transaction_pool_api::{InPoolTransaction, TransactionSource}; -use sp_api::{HeaderT, NumberFor, ProvideRuntimeApi}; +use sc_transaction_pool_api::InPoolTransaction; +use sp_api::{ApiExt, HeaderT, NumberFor, ProvideRuntimeApi}; use sp_block_builder::BlockBuilder; use sp_blockchain::HeaderBackend; use sp_domains::{ @@ -109,74 +109,81 @@ where let mut extrinsics = Vec::new(); let mut estimated_bundle_weight = Weight::default(); let mut bundle_size = 0u32; - for pending_tx in pending_iterator { - let pending_tx_data = pending_tx.data(); - - let is_within_tx_range = self - .client - .runtime_api() - .is_within_tx_range(parent_hash, pending_tx_data, &bundle_vrf_hash, &tx_range) - .map_err(|err| { - tracing::error!( - ?err, - ?pending_tx_data, - "Error occurred in locating the tx range" - ); - }) - .unwrap_or(false); - if !is_within_tx_range { - continue; - } - // Double check the transaction validity, because the tx pool are re-validate the transaction - // in pool asynchronously so there is race condition that the operator imported a domain block - // and start producing bundle immediately before the re-validation based on the latest block - // is finished, cause the bundle contains illegal tx accidentally and being considered as invalid - // bundle and slashing on the honest operator. - // - // NOTE: this check need to be kept aligned with how the illegal tx is detected in the block-preprocessor - // thus when https://github.com/subspace/subspace/issues/2184 is implemented, we also need to - // perfome the check for bundle as a whole instead of checking tx one by one. - let transaction_validity = self - .client - .runtime_api() - .validate_transaction( - parent_hash, - TransactionSource::External, - pending_tx_data.clone(), - parent_hash, - ) - .map_err(|error| { - sp_blockchain::Error::Application(Box::from(format!( - "Error getting transaction validity: {error}" - ))) - })?; - if transaction_validity.is_err() { - continue; - } + // Seperate code block to make sure that runtime api instance is dropped after validation is done. + { + // We are using one runtime api instance here to maintain storage changes in the instance's internal buffer + // between runtime calls done in this loop. + let runtime_api_instance = self.client.runtime_api(); + for pending_tx in pending_iterator { + let pending_tx_data = pending_tx.data(); - let tx_weight = self - .client - .runtime_api() - .extrinsic_weight(parent_hash, pending_tx_data) - .map_err(|error| { - sp_blockchain::Error::Application(Box::from(format!( - "Error getting extrinsic weight: {error}" - ))) - })?; - let next_estimated_bundle_weight = estimated_bundle_weight.saturating_add(tx_weight); - if next_estimated_bundle_weight.any_gt(domain_block_limit.max_block_weight) { - break; - } + let is_within_tx_range = runtime_api_instance + .is_within_tx_range(parent_hash, pending_tx_data, &bundle_vrf_hash, &tx_range) + .map_err(|err| { + tracing::error!( + ?err, + ?pending_tx_data, + "Error occurred in locating the tx range" + ); + }) + .unwrap_or(false); + if !is_within_tx_range { + continue; + } - let next_bundle_size = bundle_size + pending_tx_data.encoded_size() as u32; - if next_bundle_size > domain_block_limit.max_block_size { - break; - } + let tx_weight = runtime_api_instance + .extrinsic_weight(parent_hash, pending_tx_data) + .map_err(|error| { + sp_blockchain::Error::Application(Box::from(format!( + "Error getting extrinsic weight: {error}" + ))) + })?; + let next_estimated_bundle_weight = + estimated_bundle_weight.saturating_add(tx_weight); + if next_estimated_bundle_weight.any_gt(domain_block_limit.max_block_weight) { + break; + } - estimated_bundle_weight = next_estimated_bundle_weight; - bundle_size = next_bundle_size; - extrinsics.push(pending_tx_data.clone()); + let next_bundle_size = bundle_size + pending_tx_data.encoded_size() as u32; + if next_bundle_size > domain_block_limit.max_block_size { + break; + } + + estimated_bundle_weight = next_estimated_bundle_weight; + bundle_size = next_bundle_size; + + // Double check the transaction validity, because the tx pool are re-validate the transaction + // in pool asynchronously so there is race condition that the operator imported a domain block + // and start producing bundle immediately before the re-validation based on the latest block + // is finished, cause the bundle contains illegal tx accidentally and being considered as invalid + // bundle and slashing on the honest operator. + // + // This check is done in similar fashion to block builder's build block. + // This check needs to be the last check as otherwise if the tx won't be part of bundle due to + // some other checks, its side effect will still be part of RuntimeApiImpl's changes buffer. + let transaction_validity_result = + runtime_api_instance.execute_in_transaction(|api| { + let transaction_validity_result = api + .check_transaction_and_do_pre_dispatch( + parent_hash, + pending_tx_data, + parent_number, + parent_hash, + ); + // Only commit, if there are no errors (both ApiError and CheckTxValidityError) + if let Ok(Ok(_)) = transaction_validity_result { + sp_api::TransactionOutcome::Commit(transaction_validity_result) + } else { + sp_api::TransactionOutcome::Rollback(transaction_validity_result) + } + })?; + if transaction_validity_result.is_err() { + continue; + } + + extrinsics.push(pending_tx_data.clone()); + } } let extrinsics_root = HeaderHashingFor::::ordered_trie_root( diff --git a/domains/client/domain-operator/src/tests.rs b/domains/client/domain-operator/src/tests.rs index 9a4831809b..42bd86c90b 100644 --- a/domains/client/domain-operator/src/tests.rs +++ b/domains/client/domain-operator/src/tests.rs @@ -2777,7 +2777,7 @@ async fn test_domain_transaction_fee_and_operator_reward() { // Transaction fee (including the tip) is deducted from alice's account let alice_free_balance_changes = pre_alice_free_balance - alice.free_balance(Alice.to_account_id()); - assert!(alice_free_balance_changes >= tip as u128); + assert!(alice_free_balance_changes >= tip); // All the transaction fee is collected as operator reward assert_eq!(alice_free_balance_changes, receipt.total_rewards); diff --git a/domains/primitives/runtime/src/lib.rs b/domains/primitives/runtime/src/lib.rs index e3ce4bfa70..4d76358c20 100644 --- a/domains/primitives/runtime/src/lib.rs +++ b/domains/primitives/runtime/src/lib.rs @@ -20,9 +20,7 @@ use parity_scale_codec::{Decode, Encode}; use scale_info::TypeInfo; use sp_runtime::generic::{Era, UncheckedExtrinsic}; -use sp_runtime::traits::{ - Block as BlockT, Convert, IdentifyAccount, LookupError, NumberFor, Verify, -}; +use sp_runtime::traits::{Block as BlockT, Convert, IdentifyAccount, NumberFor, Verify}; use sp_runtime::transaction_validity::TransactionValidityError; use sp_runtime::{Digest, MultiAddress, MultiSignature}; use sp_std::vec::Vec; @@ -136,33 +134,6 @@ pub mod opaque { pub type AccountId = Vec; } -#[derive(Debug, PartialEq, Eq, Encode, Decode, TypeInfo)] -pub enum CheckTxValidityError { - /// Can not find the sender from address. - Lookup, - /// Unable to extract signer from tx - UnableToExtractSigner { error: TransactionValidityError }, - /// Transaction is invalid. - InvalidTransaction { - /// Concrete transaction validity error type. - error: TransactionValidityError, - /// Storage keys of state accessed in the validation. - storage_keys: Vec>, - }, -} - -impl From for CheckTxValidityError { - fn from(_lookup_error: LookupError) -> Self { - Self::Lookup - } -} - -#[derive(Debug, PartialEq, Eq, Encode, Decode, TypeInfo)] -pub enum VerifyTxValidityError { - /// Failed to decode the opaque account id into the runtime account type. - FailedToDecodeAccountId, -} - sp_api::decl_runtime_apis! { /// Base API that every domain runtime must implement. pub trait DomainCoreApi { @@ -195,19 +166,12 @@ sp_api::decl_runtime_apis! { /// Returns true if the extrinsic is an inherent extrinsic. fn is_inherent_extrinsic(extrinsic: &::Extrinsic) -> bool; - /// Checks the validity of extrinsic in a bundle. - fn check_transaction_validity( + /// Checks the validity of extrinsic + do pre_dispatch as well. + fn check_transaction_and_do_pre_dispatch( uxt: &::Extrinsic, block_number: NumberFor, block_hash: ::Hash - ) -> Result<(), CheckTxValidityError>; - - /// Returns the storage keys of states accessed in the API `check_transaction_validity`. - fn storage_keys_for_verifying_transaction_validity( - account_id: opaque::AccountId, - block_number: NumberFor, - tx_era: Option, - ) -> Result>, VerifyTxValidityError>; + ) -> Result<(), TransactionValidityError>; /// Returns extrinsic Era if present fn extrinsic_era( diff --git a/domains/runtime/evm/src/lib.rs b/domains/runtime/evm/src/lib.rs index b027c8e55f..7ed4eb84c8 100644 --- a/domains/runtime/evm/src/lib.rs +++ b/domains/runtime/evm/src/lib.rs @@ -13,12 +13,12 @@ use domain_runtime_primitives::opaque::Header; pub use domain_runtime_primitives::{opaque, Balance, BlockNumber, Hash, Nonce}; use domain_runtime_primitives::{MultiAccountId, TryConvertBack, SLOT_DURATION}; use fp_account::EthereumSignature; -use fp_self_contained::SelfContainedCall; +use fp_self_contained::{CheckedSignature, SelfContainedCall}; use frame_support::dispatch::{DispatchClass, GetDispatchInfo}; use frame_support::inherent::ProvideInherent; use frame_support::traits::{ ConstU16, ConstU32, ConstU64, Currency, Everything, FindAuthor, Imbalance, OnFinalize, - OnUnbalanced, PalletInfoAccess, + OnUnbalanced, }; use frame_support::weights::constants::{ BlockExecutionWeight, ExtrinsicBaseWeight, ParityDbWeight, WEIGHT_REF_TIME_PER_MILLIS, @@ -46,16 +46,17 @@ use sp_messenger::messages::{ }; use sp_runtime::generic::Era; use sp_runtime::traits::{ - BlakeTwo256, Block as BlockT, Convert, DispatchInfoOf, Dispatchable, IdentifyAccount, - IdentityLookup, PostDispatchInfoOf, UniqueSaturatedInto, Verify, + BlakeTwo256, Block as BlockT, Checkable, Convert, DispatchInfoOf, Dispatchable, + IdentifyAccount, IdentityLookup, One, PostDispatchInfoOf, SignedExtension, UniqueSaturatedInto, + ValidateUnsigned, Verify, }; use sp_runtime::transaction_validity::{ - TransactionSource, TransactionValidity, TransactionValidityError, + InvalidTransaction, TransactionSource, TransactionValidity, TransactionValidityError, }; use sp_runtime::{ create_runtime_str, generic, impl_opaque_keys, ApplyExtrinsicResult, ConsensusEngineId, Digest, }; -pub use sp_runtime::{MultiAddress, Perbill, Permill, SaturatedConversion}; +pub use sp_runtime::{MultiAddress, Perbill, Permill}; use sp_std::marker::PhantomData; use sp_std::prelude::*; #[cfg(feature = "std")] @@ -707,30 +708,12 @@ pub fn extract_signer( .collect() } -fn storage_keys_for_verifying_tx_validity_inner( - sender: AccountId, - block_number: BlockNumber, - maybe_tx_era: Option, -) -> Vec> { - let mut storage_keys = sp_std::vec![ - frame_system::BlockHash::::hashed_key_for(BlockNumber::from(0u32)), - frame_support::storage::storage_prefix(System::name().as_bytes(), b"Number").to_vec(), - frame_system::Account::::hashed_key_for(sender), - pallet_transaction_payment::NextFeeMultiplier::::hashed_key().to_vec() - ]; - - match maybe_tx_era { - None => storage_keys, - Some(era) => { - let birth_number = era - .birth(block_number.saturated_into::()) - .saturated_into::(); - storage_keys.push(frame_system::BlockHash::::hashed_key_for( - birth_number, - )); - storage_keys - } - } +fn extrinsic_era(extrinsic: &::Extrinsic) -> Option { + extrinsic + .0 + .signature + .as_ref() + .map(|(_, _, extra)| extra.4 .0) } #[cfg(feature = "runtime-benchmarks")] @@ -912,49 +895,54 @@ impl_runtime_apis! { } } - fn check_transaction_validity( + fn check_transaction_and_do_pre_dispatch( uxt: &::Extrinsic, block_number: BlockNumber, block_hash: ::Hash - ) -> Result<(), domain_runtime_primitives::CheckTxValidityError> { + ) -> Result<(), TransactionValidityError> { let lookup = frame_system::ChainContext::::default(); - let maybe_signer_info = extract_signer_inner(uxt, &lookup); - if let Some(signer_info) = maybe_signer_info { - let signer = signer_info.map_err(|tx_validity_error| { - domain_runtime_primitives::CheckTxValidityError::UnableToExtractSigner { - error: tx_validity_error - } - })?; + // Initializing block related storage required for validation + System::initialize( + &(block_number + BlockNumber::one()), + &block_hash, + &Default::default(), + ); - let tx_validity = - Executive::validate_transaction(TransactionSource::External, uxt.clone(), block_hash); + let xt = uxt.clone().check(&lookup)?; - tx_validity.map(|_| ()).map_err(|tx_validity_error| { - domain_runtime_primitives::CheckTxValidityError::InvalidTransaction { - error: tx_validity_error, - storage_keys: storage_keys_for_verifying_tx_validity_inner(signer, block_number, Self::extrinsic_era(uxt)), - } - }) - } else { - Ok(()) + let dispatch_info = xt.get_dispatch_info(); + + if dispatch_info.class == DispatchClass::Mandatory { + return Err(InvalidTransaction::BadMandatory.into()); } - } - fn storage_keys_for_verifying_transaction_validity( - who: opaque::AccountId, - block_number: BlockNumber, - maybe_tx_era: Option - ) -> Result>, domain_runtime_primitives::VerifyTxValidityError> { - let sender = AccountId::decode(&mut who.as_slice()) - .map_err(|_| domain_runtime_primitives::VerifyTxValidityError::FailedToDecodeAccountId)?; - Ok(storage_keys_for_verifying_tx_validity_inner(sender, block_number, maybe_tx_era)) + let encoded_len = uxt.encoded_size(); + + // We invoke `pre_dispatch` in addition to `validate_transaction`(even though the validation is almost same) + // as that will add the side effect of SignedExtension in the storage buffer + // which would help to maintain context across multiple transaction validity check against same + // runtime instance. + match xt.signed { + CheckedSignature::Signed(account_id, extra) => { + extra.pre_dispatch(&account_id, &xt.function, &dispatch_info, encoded_len).map(|_| ()) + }, + CheckedSignature::Unsigned => { + Runtime::pre_dispatch(&xt.function).map(|_| ())?; + SignedExtra::pre_dispatch_unsigned(&xt.function, &dispatch_info, encoded_len).map(|_| ()) + }, + CheckedSignature::SelfContained(self_contained_signing_info) => { + xt.function.pre_dispatch_self_contained(&self_contained_signing_info, &dispatch_info, encoded_len).ok_or(TransactionValidityError::Invalid( + InvalidTransaction::Call, + )).map(|_| ()) + } + } } fn extrinsic_era( extrinsic: &::Extrinsic ) -> Option { - extrinsic.0.signature.as_ref().map(|(_, _, extra)| extra.4.0) + extrinsic_era(extrinsic) } fn extrinsic_weight(ext: &::Extrinsic) -> Weight { diff --git a/domains/test/runtime/evm/src/lib.rs b/domains/test/runtime/evm/src/lib.rs index a99f10a19f..d4f6c72fa2 100644 --- a/domains/test/runtime/evm/src/lib.rs +++ b/domains/test/runtime/evm/src/lib.rs @@ -13,12 +13,12 @@ pub use domain_runtime_primitives::opaque::Header; pub use domain_runtime_primitives::{opaque, Balance, BlockNumber, Hash, Nonce}; use domain_runtime_primitives::{MultiAccountId, TryConvertBack, SLOT_DURATION}; use fp_account::EthereumSignature; -use fp_self_contained::SelfContainedCall; +use fp_self_contained::{CheckedSignature, SelfContainedCall}; use frame_support::dispatch::{DispatchClass, GetDispatchInfo}; use frame_support::inherent::ProvideInherent; use frame_support::traits::{ ConstU16, ConstU32, ConstU64, Currency, Everything, FindAuthor, Imbalance, OnFinalize, - OnUnbalanced, PalletInfoAccess, + OnUnbalanced, }; use frame_support::weights::constants::{ BlockExecutionWeight, ExtrinsicBaseWeight, ParityDbWeight, WEIGHT_REF_TIME_PER_MILLIS, @@ -46,15 +46,15 @@ use sp_messenger::messages::{ }; use sp_runtime::generic::Era; use sp_runtime::traits::{ - BlakeTwo256, Block as BlockT, Convert, DispatchInfoOf, Dispatchable, IdentifyAccount, - IdentityLookup, PostDispatchInfoOf, UniqueSaturatedInto, Verify, + BlakeTwo256, Block as BlockT, Checkable, Convert, DispatchInfoOf, Dispatchable, + IdentifyAccount, IdentityLookup, One, PostDispatchInfoOf, SignedExtension, UniqueSaturatedInto, + ValidateUnsigned, Verify, }; use sp_runtime::transaction_validity::{ - TransactionSource, TransactionValidity, TransactionValidityError, + InvalidTransaction, TransactionSource, TransactionValidity, TransactionValidityError, }; use sp_runtime::{ create_runtime_str, generic, impl_opaque_keys, ApplyExtrinsicResult, ConsensusEngineId, Digest, - SaturatedConversion, }; pub use sp_runtime::{MultiAddress, Perbill, Permill}; use sp_std::marker::PhantomData; @@ -702,30 +702,12 @@ pub fn extract_signer( .collect() } -fn storage_keys_for_verifying_tx_validity_inner( - sender: AccountId, - block_number: BlockNumber, - maybe_tx_era: Option, -) -> Vec> { - let mut storage_keys = sp_std::vec![ - frame_system::BlockHash::::hashed_key_for(BlockNumber::from(0u32)), - frame_support::storage::storage_prefix(System::name().as_bytes(), b"Number").to_vec(), - frame_system::Account::::hashed_key_for(sender), - pallet_transaction_payment::NextFeeMultiplier::::hashed_key().to_vec() - ]; - - match maybe_tx_era { - None => storage_keys, - Some(era) => { - let birth_number = era - .birth(block_number.saturated_into::()) - .saturated_into::(); - storage_keys.push(frame_system::BlockHash::::hashed_key_for( - birth_number, - )); - storage_keys - } - } +fn extrinsic_era(extrinsic: &::Extrinsic) -> Option { + extrinsic + .0 + .signature + .as_ref() + .map(|(_, _, extra)| extra.4 .0) } impl_runtime_apis! { @@ -896,49 +878,54 @@ impl_runtime_apis! { } } - fn check_transaction_validity( + fn check_transaction_and_do_pre_dispatch( uxt: &::Extrinsic, block_number: BlockNumber, block_hash: ::Hash - ) -> Result<(), domain_runtime_primitives::CheckTxValidityError> { + ) -> Result<(), TransactionValidityError> { let lookup = frame_system::ChainContext::::default(); - let maybe_signer_info = extract_signer_inner(uxt, &lookup); - if let Some(signer_info) = maybe_signer_info { - let signer = signer_info.map_err(|tx_validity_error| { - domain_runtime_primitives::CheckTxValidityError::UnableToExtractSigner { - error: tx_validity_error - } - })?; + // Initializing block related storage required for validation + System::initialize( + &(block_number + BlockNumber::one()), + &block_hash, + &Default::default(), + ); - let tx_validity = - Executive::validate_transaction(TransactionSource::External, uxt.clone(), block_hash); + let xt = uxt.clone().check(&lookup)?; - tx_validity.map(|_| ()).map_err(|tx_validity_error| { - domain_runtime_primitives::CheckTxValidityError::InvalidTransaction { - error: tx_validity_error, - storage_keys: storage_keys_for_verifying_tx_validity_inner(signer, block_number, Self::extrinsic_era(uxt)), - } - }) - } else { - Ok(()) + let dispatch_info = xt.get_dispatch_info(); + + if dispatch_info.class == DispatchClass::Mandatory { + return Err(InvalidTransaction::BadMandatory.into()); } - } - fn storage_keys_for_verifying_transaction_validity( - who: opaque::AccountId, - block_number: BlockNumber, - maybe_tx_era: Option - ) -> Result>, domain_runtime_primitives::VerifyTxValidityError> { - let sender = AccountId::decode(&mut who.as_slice()) - .map_err(|_| domain_runtime_primitives::VerifyTxValidityError::FailedToDecodeAccountId)?; - Ok(storage_keys_for_verifying_tx_validity_inner(sender, block_number, maybe_tx_era)) + let encoded_len = uxt.encoded_size(); + + // We invoke `pre_dispatch` in addition to `validate_transaction`(even though the validation is almost same) + // as that will add the side effect of SignedExtension in the storage buffer + // which would help to maintain context across multiple transaction validity check against same + // runtime instance. + match xt.signed { + CheckedSignature::Signed(account_id, extra) => { + extra.pre_dispatch(&account_id, &xt.function, &dispatch_info, encoded_len).map(|_| ()) + }, + CheckedSignature::Unsigned => { + Runtime::pre_dispatch(&xt.function).map(|_| ())?; + SignedExtra::pre_dispatch_unsigned(&xt.function, &dispatch_info, encoded_len).map(|_| ()) + }, + CheckedSignature::SelfContained(self_contained_signing_info) => { + xt.function.pre_dispatch_self_contained(&self_contained_signing_info, &dispatch_info, encoded_len).ok_or(TransactionValidityError::Invalid( + InvalidTransaction::Call, + )).map(|_| ()) + } + } } fn extrinsic_era( extrinsic: &::Extrinsic ) -> Option { - extrinsic.0.signature.as_ref().map(|(_, _, extra)| extra.4.0) + extrinsic_era(extrinsic) } fn extrinsic_weight(ext: &::Extrinsic) -> Weight { diff --git a/domains/test/service/src/domain.rs b/domains/test/service/src/domain.rs index dfd2356c2d..c453d7230e 100644 --- a/domains/test/service/src/domain.rs +++ b/domains/test/service/src/domain.rs @@ -2,7 +2,9 @@ #![warn(missing_docs)] use crate::chain_spec::create_domain_spec; -use crate::{construct_extrinsic_generic, node_config, EcdsaKeyring, UncheckedExtrinsicFor}; +use crate::{ + construct_extrinsic_generic, node_config, BalanceOf, EcdsaKeyring, UncheckedExtrinsicFor, +}; use cross_domain_message_gossip::ChainTxPoolMsg; use domain_client_operator::{BootstrapResult, Bootstrapper, OperatorStreams}; use domain_runtime_primitives::opaque::Block; @@ -341,7 +343,7 @@ where self.key, false, self.account_nonce(), - 0, + 0.into(), ); self.rpc_handlers.send_transaction(extrinsic.into()).await } @@ -352,14 +354,21 @@ where nonce: u32, function: impl Into<::RuntimeCall>, ) -> UncheckedExtrinsicFor { - construct_extrinsic_generic::(&self.client, function, self.key, false, nonce, 0) + construct_extrinsic_generic::( + &self.client, + function, + self.key, + false, + nonce, + 0.into(), + ) } /// Construct an extrinsic with the given transaction tip. pub fn construct_extrinsic_with_tip( &mut self, nonce: u32, - tip: u32, + tip: BalanceOf, function: impl Into<::RuntimeCall>, ) -> UncheckedExtrinsicFor { construct_extrinsic_generic::( diff --git a/domains/test/service/src/lib.rs b/domains/test/service/src/lib.rs index 249713bfff..72a2e4d74b 100644 --- a/domains/test/service/src/lib.rs +++ b/domains/test/service/src/lib.rs @@ -177,7 +177,7 @@ pub fn construct_extrinsic_generic( caller: EcdsaKeyring, immortal: bool, nonce: u32, - tip: u32, + tip: BalanceOf, ) -> UncheckedExtrinsicFor where Runtime: frame_system::Config + pallet_transaction_payment::Config + Send + Sync, @@ -207,7 +207,7 @@ where }), frame_system::CheckNonce::::from(nonce.into()), frame_system::CheckWeight::::new(), - pallet_transaction_payment::ChargeTransactionPayment::::from(tip.into()), + pallet_transaction_payment::ChargeTransactionPayment::::from(tip), ); let raw_payload = generic::SignedPayload::< ::RuntimeCall,