txpool refactor: support multiple implementations via trait object #4561

michalkucharczyk · 2024-05-23T21:40:03Z

This pull request slightly refactors the transaction pool crate, laying the groundwork for a fork-aware implementation (which will be provided as separate PR). The main goal of this refactor is to allow multiple implementations of transaction-pool. This PR does not change any logic yet, it is intended to make the review of the fork-aware transaction pool PR easier.

Key changes include:

Transition to TransactionPoolImpl Trait Object:
- Replaced sc_transaction_pool::FullPool with sc_transaction_pool::TransactionPoolImpl.
- Updated references and type definitions across multiple files.
Builder Pattern:
- Introduced TransactionPoolType enum for different implementations.
- Created a Builder struct for constructing transaction pool trait object.
- Enhanced TransactionPoolOptions for parameter-based initialization.
Modularization and Refactoring:
- Moved components that will be re-used by fork-aware implementation (e.g., error handling, enactment state, metrics) to a common module.
- Created a single_state_txpool module for the existing implementation of transaction pool.
- Renamed and moved several files for clarity, adjusting import paths accordingly.
Support for ?Sized Trait:
- Updated struct definitions and methods to support the ?Sized trait for TransactionPool.
- Applied these updates across various files.

Related to: #1202

This reverts commit e451a82.

paritytech-cicd-pr · 2024-06-03T08:18:21Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6374882

davxy

I see that ?Sized bound is spread all over the place.
What about adding the bound directly to the definition of the TransactionPool and MaintainedTransactionPool traits?

davxy · 2024-06-17T10:08:14Z

substrate/client/transaction-pool/src/lib.rs


-pub(crate) const LOG_TARGET: &str = "txpool";
+//testing:


Suggested change

//testing:

davxy · 2024-06-17T10:10:02Z

substrate/client/transaction-pool/src/lib.rs


-use sp_blockchain::{HashAndNumber, TreeRoute};
+//benches:


Suggested change

//benches:

davxy · 2024-06-17T10:10:12Z

substrate/client/transaction-pool/src/lib.rs


-type BoxedReadyIterator<Hash, Data> =
-	Box<dyn ReadyTransactions<Item = Arc<graph::base_pool::Transaction<Hash, Data>>> + Send>;
+// shared types


Suggested change

// shared types

davxy · 2024-06-17T10:15:15Z

substrate/client/transaction-pool/src/builder.rs

+// You should have received a copy of the GNU General Public License
+// along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+//! Utitlity for building substrate transaction pool trait object.


Suggested change

//! Utitlity for building substrate transaction pool trait object.

//! Utility for building transaction pool trait object.

davxy · 2024-06-17T10:24:32Z

substrate/client/transaction-pool/src/builder.rs

+pub struct TransactionPoolOptions {
+	txpool_type: TransactionPoolType,
+	options: crate::graph::Options,
+}


Just wonder if is better to bundle the 'type' and 'options' in a struct named "Options" or just keep the two things separated

davxy · 2024-06-17T10:25:56Z

substrate/client/transaction-pool/src/builder.rs

+	}
+}
+
+use crate::{common::api::FullChainApi, graph::ChainApi};


Nit. Can we move these at the begin of the file?

davxy · 2024-06-17T10:38:20Z

substrate/client/transaction-pool/src/builder.rs

+	<Block as BlockT>::Hash: std::marker::Unpin,
+	Client::Api: sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block>,
+{
+	/// Creates new instance of `Builder`


Maybe is worth specifying what type of pool is built by default

davxy · 2024-06-17T10:40:17Z

substrate/client/transaction-pool/src/builder.rs

+		is_validator: IsValidator,
+		prometheus: Option<&PrometheusRegistry>,


I'd add with_prometeus and and something to set if is validator as part of the methods of the builder and removing these two params

davxy · 2024-06-17T10:43:45Z

substrate/client/transaction-pool/src/single_state_txpool/single_state_txpool.rs

Silly nit. I'd personally embed this file content into the mod.rs file of this module and rename the module to just single_state (the txpool prefix is a bit redundant as we are in the tx pool crate).
But this is a matter of taste :-)

I assume that this file content is the same as the old lib.rs file right?

davxy · 2024-06-17T10:56:15Z

substrate/client/transaction-pool/src/single_state_txpool/single_state_txpool.rs

+}
+
+#[cfg_attr(test, derive(Debug))]
+enum RevalidationStatus<N> {


Maybe makes sense to move revalidation related stuff in the revalidation.rs file (i.e. RevalidationStatus, RevalidationStrategy, ..)

davxy · 2024-06-17T12:36:42Z

polkadot/rpc/src/lib.rs

@@ -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,


Suggested change

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

P: TransactionPool + 'static + ?Sized,

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

davxy · 2024-06-17T12:40:19Z

substrate/client/transaction-pool/src/builder.rs

+/// `FullClientTransactionPool` with the given `Client` and `Block` types.
+///
+/// This trait object abstracts away the specific implementations of the transaction pool.
+pub type TransactionPoolImpl<Block, Client> = dyn FullClientTransactionPool<Block, Client>;


Nit. I'm not sure about this name, I'm referring to the Impl suffix. But I can't suggest a better name :-)

davxy · 2024-06-17T12:40:51Z

templates/parachain/node/src/rpc.rs

@@ -42,7 +42,7 @@ where
 	C::Api: pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi<Block, Balance>,
 	C::Api: substrate_frame_rpc_system::AccountNonceApi<Block, AccountId, Nonce>,
 	C::Api: BlockBuilder<Block>,
-	P: TransactionPool + Sync + Send + 'static,
+	P: TransactionPool + Sync + Send + 'static + ?Sized,


Suggested change

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

P: TransactionPool + 'static + ?Sized,

bkchr

I think the approach with making every unsized is not really sexy. If we touch all of this stuff any way, we should directly put transaction pool everywhere as Arc<dyn TransactionPool> or whatever.

bkchr · 2024-06-17T18:32:36Z

substrate/client/transaction-pool/src/builder.rs

+pub struct Builder<Block, Client> {
+	options: TransactionPoolOptions,
+	_phantom: PhantomData<(Client, Block)>,
+}


This is not really a builder pattern. Not really sure we need this.

We need to translate options to instance. I can rename it, but we need some entry point to instantiate.

I mean sth like this:

polkadot-sdk/cumulus/polkadot-parachain/src/service.rs

Lines 157 to 164 in 62b08b0

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(),

);

bkchr · 2024-06-17T18:33:47Z

substrate/client/service/src/builder.rs

@@ -349,7 +349,7 @@ where
 }

 /// Parameters to pass into `build`.
-pub struct SpawnTasksParams<'a, TBl: BlockT, TCl, TExPool, TRpc, Backend> {
+pub struct SpawnTasksParams<'a, TBl: BlockT, TCl, TExPool: ?Sized, TRpc, Backend> {


All these ?Sized are "confusing". IMO we should just make transaction_pool: Arc<dyn TransactionPool> and thats it.

Do I understand correctly that you want to remove TransactionPool generic type parameter from all generic types?

Yes, would probably make it easier.

bkchr · 2024-06-17T18:36:49Z

substrate/client/transaction-pool/src/builder.rs

+#[derive(Debug, Clone)]
+pub enum TransactionPoolType {
+	/// Single-state transaction pool
+	SingleState,


Suggested change

SingleState,

NonForkAware,

Maybe this way? Not sure, but SingleState is weird. If you keep the name, at least be a little bit more descriptive.

bkchr · 2024-06-17T18:37:57Z

substrate/client/cli/src/params/transaction_pool_params.rs

+#[value(rename_all = "kebab-case")]
+pub enum TransactionPoolType {
+	/// Uses a legacy, single-state transaction pool.
+	SingleState,


Yeah or just call it LegacyTxPool.

michalkucharczyk added 4 commits May 23, 2024 21:41

txpool: internal crate reorg

4965180

References to transaction pool are now Unsized

e6ca18e

transaction pool type option added + txpool::builder used

3a65f84

make clippy happy: more fixes here and there

5b998cc

michalkucharczyk changed the title ~~Mku txpool reorg~~ txpool refactor: support multiple implementations via trait object May 23, 2024

michalkucharczyk requested a review from a team May 24, 2024 06:12

michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label May 24, 2024

michalkucharczyk mentioned this pull request May 24, 2024

Improve transaction handling for Parachains #1202

Closed

michalkucharczyk added 8 commits May 24, 2024 12:15

version bumps

e451a82

prdoc added

cd3717d

license added

8d98809

Cargo.lock updated

4f73653

Revert "version bumps"

11e16e6

This reverts commit e451a82.

Cargo.lock

2232372

Merge remote-tracking branch 'origin/master' into mku-txpool-reorg

aac0dec

Merge branch 'master' into mku-txpool-reorg

5c5e40a

Merge branch 'master' into mku-txpool-reorg

1a2f1ce

davxy reviewed Jun 17, 2024

View reviewed changes

davxy requested a review from a team June 17, 2024 12:45

bkchr reviewed Jun 17, 2024

View reviewed changes

michalkucharczyk closed this Jun 18, 2024

michalkucharczyk mentioned this pull request Aug 27, 2024

fatxpool: use dyn instead of TransactionPoolWrapper #5489

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txpool refactor: support multiple implementations via trait object #4561

txpool refactor: support multiple implementations via trait object #4561

michalkucharczyk commented May 23, 2024 •

edited

Loading

paritytech-cicd-pr commented Jun 3, 2024

davxy left a comment

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024

davxy Jun 17, 2024 •

edited

Loading

bkchr left a comment

bkchr Jun 17, 2024

michalkucharczyk Jun 17, 2024

michalkucharczyk Jun 19, 2024

bkchr Jun 17, 2024

michalkucharczyk Jun 17, 2024

bkchr Jun 18, 2024

bkchr Jun 17, 2024

bkchr Jun 17, 2024

	//! Utitlity for building substrate transaction pool trait object.
	//! Utility for building transaction pool trait object.

		is_validator: IsValidator,
		prometheus: Option<&PrometheusRegistry>,

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

	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(),
	);

txpool refactor: support multiple implementations via trait object #4561

txpool refactor: support multiple implementations via trait object #4561

Conversation

michalkucharczyk commented May 23, 2024 • edited Loading

paritytech-cicd-pr commented Jun 3, 2024

davxy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davxy Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkucharczyk commented May 23, 2024 •

edited

Loading

davxy Jun 17, 2024 •

edited

Loading