Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

txpool refactor: support multiple implementations via trait object #4561

Closed
wants to merge 13 commits into from
Closed
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cumulus/client/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ pub struct BuildNetworkParams<
pub net_config:
sc_network::config::FullNetworkConfiguration<Block, <Block as BlockT>::Hash, Network>,
pub client: Arc<Client>,
pub transaction_pool: Arc<sc_transaction_pool::FullPool<Block, Client>>,
pub transaction_pool: Arc<sc_transaction_pool::TransactionPoolImpl<Block, Client>>,
pub para_id: ParaId,
pub relay_chain_interface: RCInterface,
pub spawn_handle: SpawnTaskHandle,
Expand Down
6 changes: 3 additions & 3 deletions cumulus/polkadot-parachain/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use sp_blockchain::{Error as BlockChainError, HeaderBackend, HeaderMetadata};
pub type RpcExtension = jsonrpsee::RpcModule<()>;

/// Full client dependencies
pub struct FullDeps<C, P> {
pub struct FullDeps<C, P: ?Sized> {
/// The client instance to use.
pub client: Arc<C>,
/// Transaction pool instance.
Expand All @@ -57,7 +57,7 @@ where
C::Api: frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>,
C::Api: pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>,
C::Api: BlockBuilder<Block>,
P: TransactionPool + Sync + Send + 'static,
P: TransactionPool + Sync + Send + 'static + ?Sized,
B: sc_client_api::Backend<Block> + Send + Sync + 'static,
B::State: sc_client_api::backend::StateBackend<sp_runtime::traits::HashingFor<Block>>,
{
Expand Down Expand Up @@ -91,7 +91,7 @@ where
C::Api: frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>,
C::Api: pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>,
C::Api: BlockBuilder<Block>,
P: TransactionPool + Sync + Send + 'static,
P: TransactionPool + Sync + Send + 'static + ?Sized,
{
use frame_rpc_system::{System, SystemApiServer};
use pallet_transaction_payment_rpc::{TransactionPayment, TransactionPaymentApiServer};
Expand Down
33 changes: 19 additions & 14 deletions cumulus/polkadot-parachain/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub type Service<RuntimeApi> = PartialComponents<
ParachainBackend,
(),
sc_consensus::DefaultImportQueue<Block>,
sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>,
sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<RuntimeApi>>,
(ParachainBlockImport<RuntimeApi>, Option<Telemetry>, Option<TelemetryWorkerHandle>),
>;

Expand Down Expand Up @@ -154,13 +154,14 @@ where
telemetry
});

let transaction_pool = sc_transaction_pool::BasicPool::new_full(
config.transaction_pool.clone(),
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);
let transaction_pool = sc_transaction_pool::Builder::new()
.with_options(config.transaction_pool.clone())
.build(
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);

let block_import = ParachainBlockImport::new(client.clone(), backend.clone());

Expand Down Expand Up @@ -214,7 +215,7 @@ where
DenyUnsafe,
Arc<ParachainClient<RuntimeApi>>,
Arc<ParachainBackend>,
Arc<sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>>,
Arc<sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<RuntimeApi>>>,
) -> Result<jsonrpsee::RpcModule<()>, sc_service::Error>
+ 'static,
BIQ: FnOnce(
Expand All @@ -231,7 +232,7 @@ where
Option<TelemetryHandle>,
&TaskManager,
Arc<dyn RelayChainInterface>,
Arc<sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>>,
Arc<sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<RuntimeApi>>>,
Arc<SyncingService<Block>>,
KeystorePtr,
Duration,
Expand Down Expand Up @@ -464,7 +465,7 @@ fn build_parachain_rpc_extensions<RuntimeApi>(
deny_unsafe: sc_rpc::DenyUnsafe,
client: Arc<ParachainClient<RuntimeApi>>,
backend: Arc<ParachainBackend>,
pool: Arc<sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>>,
pool: Arc<sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<RuntimeApi>>>,
) -> Result<jsonrpsee::RpcModule<()>, sc_service::Error>
where
RuntimeApi: ConstructRuntimeApi<Block, ParachainClient<RuntimeApi>> + Send + Sync + 'static,
Expand All @@ -482,7 +483,7 @@ fn build_contracts_rpc_extensions(
deny_unsafe: sc_rpc::DenyUnsafe,
client: Arc<ParachainClient<FakeRuntimeApi>>,
_backend: Arc<ParachainBackend>,
pool: Arc<sc_transaction_pool::FullPool<Block, ParachainClient<FakeRuntimeApi>>>,
pool: Arc<sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<FakeRuntimeApi>>>,
) -> Result<jsonrpsee::RpcModule<()>, sc_service::Error> {
let deps = crate::rpc::FullDeps { client: client.clone(), pool: pool.clone(), deny_unsafe };

Expand Down Expand Up @@ -866,7 +867,9 @@ fn start_relay_chain_consensus(
telemetry: Option<TelemetryHandle>,
task_manager: &TaskManager,
relay_chain_interface: Arc<dyn RelayChainInterface>,
transaction_pool: Arc<sc_transaction_pool::FullPool<Block, ParachainClient<FakeRuntimeApi>>>,
transaction_pool: Arc<
sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<FakeRuntimeApi>>,
>,
_sync_oracle: Arc<SyncingService<Block>>,
_keystore: KeystorePtr,
_relay_chain_slot_duration: Duration,
Expand Down Expand Up @@ -937,7 +940,9 @@ fn start_lookahead_aura_consensus(
telemetry: Option<TelemetryHandle>,
task_manager: &TaskManager,
relay_chain_interface: Arc<dyn RelayChainInterface>,
transaction_pool: Arc<sc_transaction_pool::FullPool<Block, ParachainClient<FakeRuntimeApi>>>,
transaction_pool: Arc<
sc_transaction_pool::TransactionPoolImpl<Block, ParachainClient<FakeRuntimeApi>>,
>,
sync_oracle: Arc<SyncingService<Block>>,
keystore: KeystorePtr,
relay_chain_slot_duration: Duration,
Expand Down
2 changes: 1 addition & 1 deletion cumulus/test/service/benches/transaction_throughput.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use criterion::{criterion_group, criterion_main, BatchSize, Criterion, Throughpu
use cumulus_client_cli::get_raw_genesis_header;
use cumulus_test_runtime::{AccountId, BalancesCall, ExistentialDeposit, SudoCall};
use futures::{future, StreamExt};
use sc_transaction_pool_api::{TransactionPool as _, TransactionSource, TransactionStatus};
use sc_transaction_pool_api::{TransactionSource, TransactionStatus};
use sp_core::{crypto::Pair, sr25519};
use sp_runtime::OpaqueExtrinsic;

Expand Down
19 changes: 10 additions & 9 deletions cumulus/test/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ pub type Backend = TFullBackend<Block>;
pub type ParachainBlockImport = TParachainBlockImport<Block, Arc<Client>, Backend>;

/// Transaction pool type used by the test service
pub type TransactionPool = Arc<sc_transaction_pool::FullPool<Block, Client>>;
pub type TransactionPool = Arc<sc_transaction_pool::TransactionPoolImpl<Block, Client>>;

/// Recovery handle that fails regularly to simulate unavailable povs.
pub struct FailingRecoveryHandle {
Expand Down Expand Up @@ -178,7 +178,7 @@ pub type Service = PartialComponents<
Backend,
(),
sc_consensus::import_queue::BasicQueue<Block>,
sc_transaction_pool::FullPool<Block, Client>,
sc_transaction_pool::TransactionPoolImpl<Block, Client>,
ParachainBlockImport,
>;

Expand Down Expand Up @@ -213,13 +213,14 @@ pub fn new_partial(

let block_import = ParachainBlockImport::new(client.clone(), backend.clone());

let transaction_pool = sc_transaction_pool::BasicPool::new_full(
config.transaction_pool.clone(),
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);
let transaction_pool = sc_transaction_pool::Builder::new()
.with_options(config.transaction_pool.clone())
.build(
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);

let slot_duration = sc_consensus_aura::slot_duration(&*client)?;
let import_queue = cumulus_client_consensus_aura::import_queue::<AuthorityPair, _, _, _, _, _>(
Expand Down
17 changes: 9 additions & 8 deletions polkadot/node/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ fn new_partial<ChainSelection>(
FullBackend,
ChainSelection,
sc_consensus::DefaultImportQueue<Block>,
sc_transaction_pool::FullPool<Block, FullClient>,
sc_transaction_pool::TransactionPoolImpl<Block, FullClient>,
(
impl Fn(
polkadot_rpc::DenyUnsafe,
Expand Down Expand Up @@ -506,13 +506,14 @@ fn new_partial<ChainSelection>(
where
ChainSelection: 'static + SelectChain<Block>,
{
let transaction_pool = sc_transaction_pool::BasicPool::new_full(
config.transaction_pool.clone(),
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);
let transaction_pool = sc_transaction_pool::Builder::new()
.with_options(config.transaction_pool.clone())
.build(
config.role.is_authority().into(),
config.prometheus_registry(),
task_manager.spawn_essential_handle(),
client.clone(),
);

let grandpa_hard_forks = if config.chain_spec.is_kusama() {
grandpa_support::kusama_hard_forks()
Expand Down
4 changes: 2 additions & 2 deletions polkadot/rpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub struct BeefyDeps<AuthorityId: AuthorityIdBound> {
}

/// Full client dependencies
pub struct FullDeps<C, P, SC, B, AuthorityId: AuthorityIdBound> {
pub struct FullDeps<C, P: ?Sized, SC, B, AuthorityId: AuthorityIdBound> {
/// The client instance to use.
pub client: Arc<C>,
/// Transaction pool instance.
Expand Down Expand Up @@ -112,7 +112,7 @@ where
C::Api: pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>,
C::Api: BabeApi<Block>,
C::Api: BlockBuilder<Block>,
P: TransactionPool + Sync + Send + 'static,
P: TransactionPool + Sync + Send + 'static + ?Sized,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
P: TransactionPool + Sync + Send + 'static + ?Sized,
P: TransactionPool + 'static + ?Sized,

Nit. Already bound by Send+Sync. Probably there are other instance around.

SC: SelectChain<Block> + 'static,
B: sc_client_api::Backend<Block> + Send + Sync + 'static,
B::State: sc_client_api::StateBackend<sp_runtime::traits::HashingFor<Block>>,
Expand Down
50 changes: 50 additions & 0 deletions prdoc/pr_4561.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
title: "txpool refactor: support multiple implementations via trait object"

doc:
- audience: Node Dev
description: |
The transaction pool references spread across codebase can now be Unsized allowing to use trait object.

crates:
- name: cumulus-client-service
bump: minor
- name: polkadot-parachain-bin
bump: minor
- name: cumulus-test-service
bump: minor
- name: polkadot-service
bump: minor
- name: polkadot-rpc
bump: minor
- name: staging-node-cli
bump: minor
- name: node-rpc
bump: minor
- name: sc-basic-authorship
bump: minor
- name: sc-cli
bump: minor
- name: sc-consensus-manual-seal
bump: minor
- name: sc-mixnet
bump: minor
- name: sc-rpc
bump: minor
- name: sc-rpc-spec-v2
bump: minor
- name: sc-service
bump: minor
- name: sc-service-test
bump: minor
- name: sc-transaction-pool
bump: minor
- name: sc-transaction-pool-api
bump: minor
- name: substrate-frame-rpc-system
bump: minor
- name: minimal-template-node
bump: minor
- name: parachain-template-node
bump: minor
- name: solochain-template-node
bump: minor
18 changes: 5 additions & 13 deletions substrate/bin/node/cli/benches/transaction_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,20 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use polkadot_sdk::*;
use std::time::Duration;

use criterion::{criterion_group, criterion_main, BatchSize, Criterion, Throughput};
use futures::{future, StreamExt};
use kitchensink_runtime::{constants::currency::*, BalancesCall, SudoCall};
use node_cli::service::{create_extrinsic, fetch_nonce, FullClient, TransactionPool};
use node_primitives::AccountId;
use polkadot_sdk::*;
use sc_service::{
config::{
BlocksPruning, DatabaseSource, KeystoreConfig, NetworkConfiguration, OffchainWorkerConfig,
PruningMode, RpcBatchRequestConfig, TransactionPoolOptions,
},
BasePath, Configuration, Role,
};
use sc_transaction_pool::PoolLimit;
use sc_transaction_pool_api::{TransactionPool as _, TransactionSource, TransactionStatus};
use sc_transaction_pool_api::{TransactionSource, TransactionStatus};
use sp_core::{crypto::Pair, sr25519};
use sp_keyring::Sr25519Keyring;
use sp_runtime::OpaqueExtrinsic;
Expand All @@ -57,12 +54,7 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
impl_version: "1.0".into(),
role: Role::Authority,
tokio_handle: tokio_handle.clone(),
transaction_pool: TransactionPoolOptions {
ready: PoolLimit { count: 100_000, total_bytes: 100 * 1024 * 1024 },
future: PoolLimit { count: 100_000, total_bytes: 100 * 1024 * 1024 },
reject_future_transactions: false,
ban_time: Duration::from_secs(30 * 60),
},
transaction_pool: TransactionPoolOptions::new_for_benchmarks(),
network: network_config,
keystore: KeystoreConfig::InMemory,
database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
Expand Down Expand Up @@ -247,7 +239,7 @@ fn transaction_pool_benchmarks(c: &mut Criterion) {

runtime.block_on(future::join_all(prepare_extrinsics.into_iter().map(|tx| {
submit_tx_and_wait_for_inclusion(
&node.transaction_pool,
&*node.transaction_pool,
tx,
&node.client,
true,
Expand All @@ -259,7 +251,7 @@ fn transaction_pool_benchmarks(c: &mut Criterion) {
|extrinsics| {
runtime.block_on(future::join_all(extrinsics.into_iter().map(|tx| {
submit_tx_and_wait_for_inclusion(
&node.transaction_pool,
&*node.transaction_pool,
tx,
&node.client,
false,
Expand Down
Loading
Loading