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

fatxpool: cross check (and improve) support for priority transactions #5809

Open
Tracked by #5472
michalkucharczyk opened this issue Sep 23, 2024 · 1 comment · May be fixed by #6647
Open
Tracked by #5472

fatxpool: cross check (and improve) support for priority transactions #5809

michalkucharczyk opened this issue Sep 23, 2024 · 1 comment · May be fixed by #6647
Assignees
Labels
T0-node This PR/Issue is related to the topic “node”.

Comments

@michalkucharczyk
Copy link
Contributor

Make sure that priority transactions are replacing lower prio transaction that are already in the pool.
These may require some support in ttxt.

@michalkucharczyk
Copy link
Contributor Author

Related comment:
#4639 (comment)

@michalkucharczyk michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label Sep 23, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 3, 2024
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:
1. #### [Better
support](414ec3c)
for `Usurped` transactions

When any view reports an `Usurped` transaction (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 the
`view_store` in newly created view (this is why
`ViewStore::pending_txs_replacements` was added).

1. ####
[`TimedTransactionSource`](f10590f)
introduced:

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.

1. #### `DroppedWatcher`: [improved
logic](560db28)
for future transactions
For 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:

1.
[simplified](2d0bbf8)
the flow in `update_view_with_mempool` (code duplication + minor bug
fix).
2. `graph::BasePool`: [handling
priorities](c9f2d39)
for future transaction improved (previously transaction with lower prio
was reported as failed),
3. `graph::listener`: dedicated `limit_enforced`/`usurped`/`dropped`
[calls
added](7b58a68),
4. flaky test
[fixed](e0a7bc6)
5. new tests added,

related to: #5809

---------

Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: Iulian Barbu <[email protected]>
Krayt78 pushed a commit to Krayt78/polkadot-sdk that referenced this issue Dec 18, 2024
)

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:
1. #### [Better
support](paritytech@414ec3c)
for `Usurped` transactions

When any view reports an `Usurped` transaction (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 the
`view_store` in newly created view (this is why
`ViewStore::pending_txs_replacements` was added).

1. ####
[`TimedTransactionSource`](paritytech@f10590f)
introduced:

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.

1. #### `DroppedWatcher`: [improved
logic](paritytech@560db28)
for future transactions
For 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:

1.
[simplified](paritytech@2d0bbf8)
the flow in `update_view_with_mempool` (code duplication + minor bug
fix).
2. `graph::BasePool`: [handling
priorities](paritytech@c9f2d39)
for future transaction improved (previously transaction with lower prio
was reported as failed),
3. `graph::listener`: dedicated `limit_enforced`/`usurped`/`dropped`
[calls
added](paritytech@7b58a68),
4. flaky test
[fixed](paritytech@e0a7bc6)
5. new tests added,

related to: paritytech#5809

---------

Co-authored-by: GitHub Action <[email protected]>
Co-authored-by: Iulian Barbu <[email protected]>
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 a pull request may close this issue.

1 participant