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

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented May 23, 2024

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:

  1. Transition to TransactionPoolImpl Trait Object:

    • Replaced sc_transaction_pool::FullPool with sc_transaction_pool::TransactionPoolImpl.
    • Updated references and type definitions across multiple files.
  2. Builder Pattern:

    • Introduced TransactionPoolType enum for different implementations.
    • Created a Builder struct for constructing transaction pool trait object.
    • Enhanced TransactionPoolOptions for parameter-based initialization.
  3. 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.
  4. 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

@michalkucharczyk michalkucharczyk changed the title Mku txpool reorg txpool refactor: support multiple implementations via trait object May 23, 2024
@michalkucharczyk michalkucharczyk requested a review from a team May 24, 2024 06:12
@michalkucharczyk michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label May 24, 2024
@paritytech-cicd-pr
Copy link

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

Copy link
Member

@davxy davxy left a comment

Choose a reason for hiding this comment

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

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?


pub(crate) const LOG_TARGET: &str = "txpool";
//testing:
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
//testing:


use sp_blockchain::{HashAndNumber, TreeRoute};
//benches:
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
//benches:


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

// 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.
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
//! Utitlity for building substrate transaction pool trait object.
//! Utility for building transaction pool trait object.

Comment on lines +40 to +43
pub struct TransactionPoolOptions {
txpool_type: TransactionPoolType,
options: crate::graph::Options,
}
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

use crate::{common::api::FullChainApi, graph::ChainApi};
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Comment on lines +203 to +204
is_validator: IsValidator,
prometheus: Option<&PrometheusRegistry>,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :-)

Copy link
Member

Choose a reason for hiding this comment

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

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

}

#[cfg_attr(test, derive(Debug))]
enum RevalidationStatus<N> {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

/// `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>;
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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,
Copy link
Member

@davxy davxy Jun 17, 2024

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,

@davxy davxy requested a review from a team June 17, 2024 12:45
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +168 to +171
pub struct Builder<Block, Client> {
options: TransactionPoolOptions,
_phantom: PhantomData<(Client, Block)>,
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean sth like this:

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

@@ -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> {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, would probably make it easier.

#[derive(Debug, Clone)]
pub enum TransactionPoolType {
/// Single-state transaction pool
SingleState,
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
SingleState,
NonForkAware,

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

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

Choose a reason for hiding this comment

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

Yeah or just call it LegacyTxPool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants