fatxpool: handling limits and priorities improvements
#6405
Conversation
michalkucharczyk
force-pushed
the
mku-fatxpool-priorities
branch
from
d0d8ddc to
7b96ca3
Compare
michalkucharczyk
changed the title
fatxpool: improvements in handling of prioritiesfatxpool:limits and priorities improvements
michalkucharczyk
changed the title
fatxpool:limits and priorities improvementsfatxpool: limits and priorities improvements
michalkucharczyk
changed the title
fatxpool: limits and priorities improvementsfatxpool: handling limits and priorities improvements
|
/cmd prdoc --bump major --audience node_dev |
michalkucharczyk
added
R0-silent
substrate/client/transaction-pool/src/fork_aware_txpool/dropped_watcher.rs
Outdated
Show resolved
Hide resolved
substrate/client/transaction-pool/src/fork_aware_txpool/multi_view_listener.rs
Show resolved
Hide resolved
| Some(( | ||
| // These transactions are coming from retracted blocks, we should | ||
| // simply consider them external. | ||
| TimedTransactionSource::new_external(false), |
There was a problem hiding this comment.
Should probably not be changed since it was like this before, but any idea why this is External instead of InBlock?
There was a problem hiding this comment.
From what I understand it could be InBlock. But I don't understand all the consequences of changing this value, as it goes to runtime side. I'd not touch it in this PR :)
| self.future_transaction_views.entry(tx_hash).or_default().insert(block_hash); | ||
| }, | ||
| TransactionStatus::Ready | TransactionStatus::InBlock(..) => { | ||
| if let Some(mut views) = self.future_transaction_views.remove(&tx_hash) { |
There was a problem hiding this comment.
DQ: Why do we move the entire list of views from future to ready here? This is just one view reporting the transaction as ready now right? I would expect that we only remove that one block hash from the future transactions view list.
There was a problem hiding this comment.
Good question.
It is about if we want to drop transaction when the last view referencing a transaction is dropped.
And now for transaction that is future, and never was seen as ready on any fork, we want to remove it from the pool when the last view referencing transaction is gone. We basically don't want to keep future transaction too long. If the transaction is no longer in any view it was likely pushed out by some newly incoming transactions.
If the transaction was seen as ready on any fork, it means that it can be soon included into some new fork that can pop out. This is why in a DroppedWatcher I decided to treat such transaction as ready. Due to this we will keep this transaction in a mempool for a while - we will not send a dropped event to the pool.
I treat an internal mempool as a secondary, best-effort, buffer for transactions - meaning that it can hold ready transactions that are currently not referenced by any view (note that it applies to the situation when limits are hit). When the blocks are built, those txs will be resubmitted and will get to the view. The other approach that could be simple would be to simply drop everything what is not referenced by views. Maybe it is a way to go. I'll give it more consideration when all groundwork is done.
Please also note that hitting limits on transaction pool is not a typical case. I would not expect hitting limits even during spamming tests. Hitting limits may occur during some attack or maybe when block production is stopped. This should not even happen when finality is stalled (becasue FinalityTimeout events shall be triggered and listeners will be dropped - still to be tested).
I am still here experimenting a bit, so it may happen that these code will get re-iterated.
There was a problem hiding this comment.
Okay I could follow this explanation, but its kind of complicated behaviour, maybe a comment in the code would help for unsuspecting developers :D
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd fmt |
|
Command "fmt" has started 🚀 See logs here |
|
Command "fmt" has finished ✅ See logs here |
This PR provides a number of improvements around handling limits and priorities in the fork-aware transaction pool.
Notes to reviewers.
Following are the notable changes:
Better support for
UsurpedtransactionsWhen any view reports an
Usurpedtransaction (replaced by other with higher priority) it is removed from all the views (also inactive). Removal is implemented by simply submitting usurper transaction to all the views. It is also ensured that usurped tx will not sneak into theview_storein newly created view (this is whyViewStore::pending_txs_replacementswas added).TimedTransactionSourceintroduced:Every view now has an information when the transaction entered the pool. Enforce limits (now only for future txs) uses this timestamp to find worst transactions. Having common timestamp ensures coherent assessment of the transaction's importance across different views. This also could later be used to select which ready transaction shall be dropped.
DroppedWatcher: improved logic for future transactionsFor future transaction - if the last referencing view is removed, the transaction will be dropped from the pool. This prevents future unincluded and un-promoted transactions from staying in the pool for long time.
And some minor changes:
update_view_with_mempool(code duplication + minor bug fix).graph::BasePool: handling priorities for future transaction improved (previously transaction with lower prio was reported as failed),graph::listener: dedicatedlimit_enforced/usurped/droppedcalls added,related to: #5809