diff --git a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs index e81bd4bfa0b0..ab5be1f24e5d 100644 --- a/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs +++ b/substrate/client/rpc-spec-v2/src/chain_head/test_utils.rs @@ -34,12 +34,13 @@ use sp_runtime::{ }; use sp_version::RuntimeVersion; use std::sync::Arc; -use substrate_test_runtime::{Block, Hash, Header}; +use substrate_test_runtime::{Block, Hash, Header, H256}; pub struct ChainHeadMockClient { client: Arc, import_sinks: Mutex>>>, finality_sinks: Mutex>>>, + best_block: Mutex>, } impl ChainHeadMockClient { @@ -48,6 +49,7 @@ impl ChainHeadMockClient { client, import_sinks: Default::default(), finality_sinks: Default::default(), + best_block: Default::default(), } } @@ -86,6 +88,11 @@ impl ChainHeadMockClient { let _ = sink.unbounded_send(notification.clone()); } } + + /// Set the best block hash and number that is reported by the `info` method. + pub fn set_best_block(&self, hash: H256, number: u64) { + *self.best_block.lock() = Some((hash, number)); + } } // ChainHead calls `import_notification_stream` and `finality_notification_stream` in order to @@ -309,8 +316,10 @@ impl + Send + Sync> HeaderMetadata< } } -impl + Send + Sync> HeaderBackend +impl, Client: HeaderBackend + Send + Sync> HeaderBackend for ChainHeadMockClient +where + <::Header as HeaderT>::Number: From, { fn header( &self, @@ -320,7 +329,14 @@ impl + Send + Sync> HeaderBackend Info { - self.client.info() + let mut info = self.client.info(); + + if let Some((block_hash, block_num)) = self.best_block.lock().take() { + info.best_hash = block_hash; + info.best_number = block_num.into(); + } + + info } fn status(&self, hash: Block::Hash) -> sc_client_api::blockchain::Result { diff --git a/substrate/client/rpc-spec-v2/src/transaction/tests/mod.rs b/substrate/client/rpc-spec-v2/src/transaction/tests/mod.rs index ab0caaf906fd..e14603ddb755 100644 --- a/substrate/client/rpc-spec-v2/src/transaction/tests/mod.rs +++ b/substrate/client/rpc-spec-v2/src/transaction/tests/mod.rs @@ -22,3 +22,4 @@ mod middleware_pool; mod setup; mod transaction_broadcast_tests; +mod transaction_tests; diff --git a/substrate/client/rpc-spec-v2/src/transaction/tests/setup.rs b/substrate/client/rpc-spec-v2/src/transaction/tests/setup.rs index 04ee7b9b4c94..4a15657a7f69 100644 --- a/substrate/client/rpc-spec-v2/src/transaction/tests/setup.rs +++ b/substrate/client/rpc-spec-v2/src/transaction/tests/setup.rs @@ -19,9 +19,9 @@ use crate::{ chain_head::test_utils::ChainHeadMockClient, transaction::{ - api::TransactionBroadcastApiServer, + api::{TransactionApiServer, TransactionBroadcastApiServer}, tests::executor::{TaskExecutorBroadcast, TaskExecutorState}, - TransactionBroadcast as RpcTransactionBroadcast, + Transaction as RpcTransaction, TransactionBroadcast as RpcTransactionBroadcast, }, }; use futures::Future; @@ -92,6 +92,29 @@ pub fn setup_api( (api, pool, client_mock, tx_api, executor_recv, pool_state) } +pub fn setup_api_tx() -> ( + Arc, + Arc, + Arc>>, + RpcModule>>>, + TaskExecutorState, + MiddlewarePoolRecv, +) { + let (pool, api, _) = maintained_pool(Default::default()); + let (pool, pool_state) = MiddlewarePool::new(Arc::new(pool).clone()); + let pool = Arc::new(pool); + + let builder = TestClientBuilder::new(); + let client = Arc::new(builder.build()); + let client_mock = Arc::new(ChainHeadMockClient::new(client.clone())); + let (task_executor, executor_recv) = TaskExecutorBroadcast::new(); + + let tx_api = + RpcTransaction::new(client_mock.clone(), pool.clone(), Arc::new(task_executor)).into_rpc(); + + (api, pool, client_mock, tx_api, executor_recv, pool_state) +} + /// Get the next event from the provided middleware in at most 5 seconds. macro_rules! get_next_event { ($middleware:expr) => { @@ -102,6 +125,18 @@ macro_rules! get_next_event { }; } +/// Get the next event from the provided middleware in at most 5 seconds. +macro_rules! get_next_event_sub { + ($sub:expr) => { + tokio::time::timeout(std::time::Duration::from_secs(5), $sub.next()) + .await + .unwrap() + .unwrap() + .unwrap() + .0 + }; +} + /// Collect the next number of transaction events from the provided middleware. macro_rules! get_next_tx_events { ($middleware:expr, $num:expr) => {{ diff --git a/substrate/client/rpc-spec-v2/src/transaction/tests/transaction_tests.rs b/substrate/client/rpc-spec-v2/src/transaction/tests/transaction_tests.rs new file mode 100644 index 000000000000..c83bc948c437 --- /dev/null +++ b/substrate/client/rpc-spec-v2/src/transaction/tests/transaction_tests.rs @@ -0,0 +1,151 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use crate::{ + hex_string, + transaction::{TransactionBlock, TransactionEvent}, +}; +use assert_matches::assert_matches; +use codec::Encode; +use jsonrpsee::rpc_params; +use sc_transaction_pool_api::{ChainEvent, MaintainedTransactionPool}; +use sp_core::H256; +use std::sync::Arc; +use substrate_test_runtime_client::AccountKeyring::*; +use substrate_test_runtime_transaction_pool::uxt; + +// Test helpers. +use crate::transaction::tests::setup::{setup_api_tx, ALICE_NONCE}; + +#[tokio::test] +async fn tx_invalid_bytes() { + let (_api, _pool, _client_mock, tx_api, _exec_middleware, _pool_middleware) = setup_api_tx(); + + // This should not rely on the tx pool state. + let mut sub = tx_api + .subscribe_unbounded("transactionWatch_unstable_submitAndWatch", rpc_params![&"0xdeadbeef"]) + .await + .unwrap(); + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_matches!(event, TransactionEvent::Invalid(_)); +} + +#[tokio::test] +async fn tx_in_finalized() { + let (api, pool, client, tx_api, _exec_middleware, _pool_middleware) = setup_api_tx(); + let block_1_header = api.push_block(1, vec![], true); + client.set_best_block(block_1_header.hash(), 1); + + let uxt = uxt(Alice, ALICE_NONCE); + let xt = hex_string(&uxt.encode()); + + let mut sub = tx_api + .subscribe_unbounded("transactionWatch_unstable_submitAndWatch", rpc_params![&xt]) + .await + .unwrap(); + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!(event, TransactionEvent::Validated); + + // Import block 2 with the transaction included. + let block_2_header = api.push_block(2, vec![uxt.clone()], true); + let block_2 = block_2_header.hash(); + + // Announce block 2 to the pool. + let event = ChainEvent::NewBestBlock { hash: block_2, tree_route: None }; + pool.inner_pool.maintain(event).await; + let event = ChainEvent::Finalized { hash: block_2, tree_route: Arc::from(vec![]) }; + pool.inner_pool.maintain(event).await; + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!( + event, + TransactionEvent::BestChainBlockIncluded(Some(TransactionBlock { + hash: block_2, + index: 0 + })) + ); + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!(event, TransactionEvent::Finalized(TransactionBlock { hash: block_2, index: 0 })); +} + +#[tokio::test] +async fn tx_with_pruned_best_block() { + let (api, pool, client, tx_api, _exec_middleware, _pool_middleware) = setup_api_tx(); + let block_1_header = api.push_block(1, vec![], true); + client.set_best_block(block_1_header.hash(), 1); + + let uxt = uxt(Alice, ALICE_NONCE); + let xt = hex_string(&uxt.encode()); + + let mut sub = tx_api + .subscribe_unbounded("transactionWatch_unstable_submitAndWatch", rpc_params![&xt]) + .await + .unwrap(); + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!(event, TransactionEvent::Validated); + + // Import block 2 with the transaction included. + let block_2_header = api.push_block(2, vec![uxt.clone()], true); + let block_2 = block_2_header.hash(); + let event = ChainEvent::NewBestBlock { hash: block_2, tree_route: None }; + pool.inner_pool.maintain(event).await; + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!( + event, + TransactionEvent::BestChainBlockIncluded(Some(TransactionBlock { + hash: block_2, + index: 0 + })) + ); + + // Import block 2 again without the transaction included. + let block_2_header = api.push_block(2, vec![], true); + let block_2 = block_2_header.hash(); + let event = ChainEvent::NewBestBlock { hash: block_2, tree_route: None }; + pool.inner_pool.maintain(event).await; + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!(event, TransactionEvent::BestChainBlockIncluded(None)); + + let block_2_header = api.push_block(2, vec![uxt.clone()], true); + let block_2 = block_2_header.hash(); + let event = ChainEvent::NewBestBlock { hash: block_2, tree_route: None }; + pool.inner_pool.maintain(event).await; + + // The tx is validated again against the new block. + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!(event, TransactionEvent::Validated); + + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!( + event, + TransactionEvent::BestChainBlockIncluded(Some(TransactionBlock { + hash: block_2, + index: 0 + })) + ); + + let event = ChainEvent::Finalized { hash: block_2, tree_route: Arc::from(vec![]) }; + pool.inner_pool.maintain(event).await; + let event: TransactionEvent = get_next_event_sub!(&mut sub); + assert_eq!(event, TransactionEvent::Finalized(TransactionBlock { hash: block_2, index: 0 })); +} diff --git a/substrate/client/rpc-spec-v2/src/transaction/transaction.rs b/substrate/client/rpc-spec-v2/src/transaction/transaction.rs index d44006392dca..6a7c69b8f7d1 100644 --- a/substrate/client/rpc-spec-v2/src/transaction/transaction.rs +++ b/substrate/client/rpc-spec-v2/src/transaction/transaction.rs @@ -28,8 +28,8 @@ use crate::{ }; use codec::Decode; use futures::{StreamExt, TryFutureExt}; -use jsonrpsee::{core::async_trait, types::error::ErrorObject, PendingSubscriptionSink}; -use sc_rpc::utils::pipe_from_stream; +use jsonrpsee::{core::async_trait, PendingSubscriptionSink}; +use sc_rpc::utils::{pipe_from_stream, to_sub_message}; use sc_transaction_pool_api::{ error::IntoPoolError, BlockHash, TransactionFor, TransactionPool, TransactionSource, TransactionStatus, @@ -39,6 +39,8 @@ use sp_core::Bytes; use sp_runtime::traits::Block as BlockT; use std::sync::Arc; +pub(crate) const LOG_TARGET: &str = "rpc-spec-v2"; + /// An API for transaction RPC calls. pub struct Transaction { /// Substrate client. @@ -63,13 +65,6 @@ impl Transaction { /// some unique transactions via RPC and have them included in the pool. const TX_SOURCE: TransactionSource = TransactionSource::External; -/// Extrinsic has an invalid format. -/// -/// # Note -/// -/// This is similar to the old `author` API error code. -const BAD_FORMAT: i32 = 1001; - #[async_trait] impl TransactionApiServer> for Transaction where @@ -83,17 +78,21 @@ where let pool = self.pool.clone(); let fut = async move { - // This is the only place where the RPC server can return an error for this - // subscription. Other defects must be signaled as events to the sink. let decoded_extrinsic = match TransactionFor::::decode(&mut &xt[..]) { Ok(decoded_extrinsic) => decoded_extrinsic, Err(e) => { - let err = ErrorObject::owned( - BAD_FORMAT, - format!("Extrinsic has invalid format: {}", e), - None::<()>, + log::debug!(target: LOG_TARGET, "Extrinsic bytes cannot be decoded: {:?}", e); + + let Ok(sink) = pending.accept().await else { return }; + + // The transaction is invalid. + let msg = to_sub_message( + &sink, + &TransactionEvent::Invalid::>(TransactionError { + error: "Extrinsic bytes cannot be decoded".into(), + }), ); - let _ = pending.reject(err).await; + let _ = sink.send(msg).await; return }, }; @@ -147,7 +146,7 @@ pub fn handle_event( TransactionStatus::Usurped(_) => Some(TransactionEvent::Invalid(TransactionError { error: "Extrinsic was rendered invalid by another extrinsic".into(), })), - TransactionStatus::Dropped => Some(TransactionEvent::Invalid(TransactionError { + TransactionStatus::Dropped => Some(TransactionEvent::Dropped(TransactionDropped { error: "Extrinsic dropped from the pool due to exceeding limits".into(), })), TransactionStatus::Invalid => Some(TransactionEvent::Invalid(TransactionError {