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

fork-aware transaction pool added #4639

Open
wants to merge 314 commits into
base: master
Choose a base branch
from

Conversation

michalkucharczyk
Copy link
Contributor

@michalkucharczyk michalkucharczyk commented May 29, 2024

Fork-Aware Transaction Pool Implementation

This PR introduces a fork-aware transaction pool (fatxpool) enhancing transaction management by maintaining the valid state of txpool for different forks.

High-level overview

The high level overview was added to sc_transaction_pool::fork_aware_txpool module. Use:

cargo  doc  --document-private-items` to build the doc.
Quick overview (documentation excerpt)

View

For every fork, a view is created. The view is a persisted state of the transaction pool computed and updated at the tip of the fork. The view is built around the existing ValidatedPool structure.

A view is created on every new best block notification. To create a view, one of the existing views is chosen and cloned.

When the chain progresses, the view is kept in the cache (retracted_views) to allow building blocks upon intermediary blocks in the fork.

The views are deleted on finalization: views lower than the finalized block are removed.

The views are updated with the transactions from the mempool—all transactions are sent to the newly created views.
A maintain process is also executed for the newly created views—basically resubmitting and pruning transactions from the appropriate tree route.

View store

View store is the helper structure that acts as a container for all the views. It provides some convenient methods.

Submitting transactions

Every transaction is submitted to every view at the tips of the forks. Retracted views are not updated.
Every transaction also goes into the mempool.

Internal mempool

Shortly, the main purpose of an internal mempool is to prevent a transaction from being lost. That could happen when a transaction is invalid on one fork and could be valid on another. It also allows the txpool to accept transactions when no blocks have been reported yet.

The mempool removes its transactions when they get finalized. Transactions are also periodically verified on every finalized event and removed from the mempool if no longer valid.

Events

Transaction events from multiple views are merged and filtered to avoid duplicated events.
Ready / Future / Inblock events are originated in the Views and are de-duplicated and forwarded to external listeners.
Finalized events are originated in fork-aware-txpool logic.
Invalid events requires special care and can be originated in both view and fork-aware-txpool logic.

Light maintain

Sometime transaction pool does not have enough time to prepare fully maintained view with all retracted transactions being revalidated. To avoid providing empty ready transaction set to block builder (what would result in empty block) the light maintain was implemented. It simply removes the imported transactions from ready iterator.

Revalidation

Revalidation is performed for every view. The revalidation process is started after a trigger is executed. The revalidation work is terminated just after a new best block / finalized event is notified to the transaction pool.
The revalidation result is applied to the newly created view which is built upon the revalidated view.

Additionally, parts of the mempool are also revalidated to make sure that no transactions are stuck in the mempool.

Logs

The most important log allowing to understand the state of the txpool is:

              maintain: txs:(0, 92) views:[2;[(327, 76, 0), (326, 68, 0)]] event:Finalized { hash: 0x8...f, tree_route: [] }  took:3.463522ms
                             ^   ^         ^     ^   ^  ^      ^   ^  ^        ^                                                   ^
unwatched txs in mempool ────┘   │         │     │   │  │      │   │  │        │                                                   │
   watched txs in mempool ───────┘         │     │   │  │      │   │  │        │                                                   │
                     views  ───────────────┘     │   │  │      │   │  │        │                                                   │
                      1st view block # ──────────┘   │  │      │   │  │        │                                                   │
                           number of ready tx ───────┘  │      │   │  │        │                                                   │
                                numer of future tx ─────┘      │   │  │        │                                                   │
                                        2nd view block # ──────┘   │  │        │                                                   │
                                      number of ready tx ──────────┘  │        │                                                   │
                                           number of future tx ───────┘        │                                                   │
                                                                 event ────────┘                                                   │
                                                                       duration  ──────────────────────────────────────────────────┘

It is logged after the maintenance is done.

The debug level enables per-transaction logging, allowing to keep track of all transaction-related actions that happened in txpool.

Integration notes

For teams having a custom node, the new txpool need to be instantiated, typically in service.rs file, here is example:

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

To enable new transaction pool the following --pool-type=fork-aware. There shall be information printed in the log:

2024-09-20 21:28:17.528  INFO main txpool: [Parachain]  creating ForkAware txpool.

For debugging the following debugs shall be enabled:

      "-lbasic-authorship=debug",
      "-ltxpool=debug",

note: trace for txpool enables per-transaction logging.

Future work

The current implementation seems to be stable, however further improvements are required.
Here is the umbrella issue for future work:

Partially fixes: #1202

Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

just a few typos and wording suggestions

@michalkucharczyk
Copy link
Contributor Author

michalkucharczyk commented Sep 19, 2024

Across the codebase a following pattern is repeated over and over:

struct SomeData<P> {
  p: Arc<P>
}

fn use_some_data<P>(s: SomeData<P>) 
where
  P: TransactionPool
{ ... }

e.g. here:

pub struct BuildNetworkParams<'a, Block, Net, TxPool, IQ, Client>
where
Block: BlockT,
Net: NetworkBackend<Block, <Block as BlockT>::Hash>,
{
/// The service configuration.
pub config: &'a Configuration,
/// Full network configuration.
pub net_config: FullNetworkConfiguration<Block, <Block as BlockT>::Hash, Net>,
/// A shared client returned by `new_full_parts`.
pub client: Arc<Client>,
/// A shared transaction pool.
pub transaction_pool: Arc<TxPool>,
/// A handle for spawning tasks.
pub spawn_handle: SpawnTaskHandle,
/// An import queue.
pub import_queue: IQ,
/// A block announce validator builder.
pub block_announce_validator_builder: Option<
Box<dyn FnOnce(Arc<Client>) -> Box<dyn BlockAnnounceValidator<Block> + Send> + Send>,
>,
/// Syncing strategy to use in syncing engine.
pub syncing_strategy: Box<dyn SyncingStrategy<Block>>,
/// User specified block relay params. If not specified, the default
/// block request handler will be used.
pub block_relay: Option<BlockRelayParams<Block, Net>>,
/// Metrics.
pub metrics: NotificationMetrics,
}

and
pub fn build_network<Block, Net, TxPool, IQ, Client>(
params: BuildNetworkParams<Block, Net, TxPool, IQ, Client>,
) -> Result<
(
Arc<dyn sc_network::service::traits::NetworkService>,
TracingUnboundedSender<sc_rpc::system::Request<Block>>,
sc_network_transactions::TransactionsHandlerController<<Block as BlockT>::Hash>,
NetworkStarter,
Arc<SyncingService<Block>>,
),
Error,
>
where
Block: BlockT,
Client: ProvideRuntimeApi<Block>
+ HeaderMetadata<Block, Error = sp_blockchain::Error>
+ Chain<Block>
+ BlockBackend<Block>
+ BlockIdTo<Block, Error = sp_blockchain::Error>
+ ProofProvider<Block>
+ HeaderBackend<Block>
+ BlockchainEvents<Block>
+ 'static,
TxPool: TransactionPool<Block = Block, Hash = <Block as BlockT>::Hash> + 'static,
IQ: ImportQueue<Block> + 'static,
Net: NetworkBackend<Block, <Block as BlockT>::Hash>,
{

Introducion of dynamic dispatching for transaction pool may require touching of all these statements, depending on choosen approach.

I tried three approaches with using transaction pool trait object.

(1) Relaxing Sized constraint in generic params.

Basically type used to refer to the transaction pool implementation is

dyn FullClientTransactionPool<Block, Client>

The generic parameter in

struct<T> { t:Arc<T> }

is by default constrained by Sized, to relax it explicit ?Sized bound needs to be added:

struct<T:?Sized> { t:Arc<T> }

This was done initially and rejected.

Please note that this approach is backward compatible and does not require to change type of field referencing transaction pool instance (Arc<P> type).

(2) Using wrapper

The type is used to reference the tranaction pool instance is:

pub struct TransactionPoolWrapper<Block, Client>(
	pub Box<dyn FullClientTransactionPool<Block, Client>>,
)

This approach does not require any changes in generic parameters or constrains used across code base.

(3) Using Arc

The type used to reference the tranaction pool instance is:

Arc<dyn FullClientTransactionPool<Block, Client>>

As we don't want to have Arc<Arc<P>> objects, which is needless and maybe confusing, and also introduces unneccesary redirection, we need to change the type of the transaction pool in beforementioned structures:

struct SomeData<P> {
  p: Arc<P>
}

fn use_some_data<P>(s: SomeData<P>) 
where
  P: TransactionPool
{ ... }

nees to be converted into:

struct SomeData<P> {
  p: P
}

fn use_some_data<P>(s: SomeData<P>) 
where
  P: TransactionPool + 'static + Clone
{ ... }

Additionally, as dynamic upcast conversion is not yet suported in stable rust, we need to add methods that allows to do that. We need a method of converting Arc<dyn FullClientTransactionPool> into Arc<dyn LocalTransactionPool> or Arc<dyn TransactionPool>. This was done by adding LocalTransactioPool::as_local_transaction_pool_arc(&self:Arc<Self>) and TransactioPool::as_transaction_pool_arc(&self:Arc<Self>) to the appropriate traits.

(4) Getting rid of pool generic parameter

There is one more option:
We could migrate from:

struct SomeData<P> {
  p: Arc<P>
}

to

struct SomeData<Block,Client> {
  p: Arc<dyn FullClientTransactionPool<Block,Client>>
}

but the problem is that TransactionPool related traits are either generic or have associated types. Which means we need to extend those structurs what make this a quite heavy refactor of many module.
I think this work is definitely out of scope of implementation of fork-aware transaction pool.

(3) seems more elegant for me, but requires many changes (also rebasing becomes more annoying as many files are touched).
(2) is lees elegant but also least invasive.

@skunert
Copy link
Contributor

skunert commented Sep 19, 2024

IMO we should pick the most pragmatic solution that gets this merged. That seems to be (2) in my book. Wrapper might not be beautiful, but changes are minimal and the PR is quite big already, we should not add another refactoring on top. All the other proposed options are still possible later.

@bkchr
Copy link
Member

bkchr commented Sep 19, 2024

Getting rid of pool generic parameter

I really don't really see the need for the generic parameter here. Just makes it more complicated and without the generic parameter the code should be easier. I would favor this.

But if you say 2 is faster for now, pick this one and then later do 4.

@michalkucharczyk
Copy link
Contributor Author

But if you say 2 is faster for now, pick this one and then later do 4.

Filed here: #5489

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve transaction handling for Parachains
7 participants