Txpool v2 insertion #2193

AurelienFT · 2024-09-13T12:20:13Z

REVIEWER NOTE : Review #2162 first for context

Linked Issues/PRs

Description

This PR allows you to insert Transaction in the TxPool v2 service and so perform all the necessary verifications and the insertion.

Changes in code logic from `TxPool v1`

The verifications are performed in a new order specified in Allow insertion of transactions in the TxPool v2 #2186. The goal is to avoid making the computation heavy work if the simple checks aren't valid. In this new version we also ensure that verifications are done in order by having wrapper type around each step to allow only one verification path.
The insertion is performed in a separate thread pool, the goal is to not block the pool on any verifications/insertions and to manage the ressources we allocate to these works
The insertion rules and conditions has change to the following :
- A transaction with dependencies can collide only with one other transaction
- A transaction without dependencies can collide with multiple transaction
- Rules to free up space for new transaction
  - If a transaction is colliding with another verify if deleting the colliding transaction and dependents subtree is enough otherwise refuses the tx
  - If a transaction is dependent and not enough space, don't accept transaction
  - If a transaction is executable, try to free has much space used by less profitable transactions as possible in the pool to include it
New limits on the size of the pool : max_pool_bytes_size and max_pool_gas

Changes from the base branch `txpool-v2`

All the tests has been refactored to be way more clearer to understand and easier to read. They also now all follow the naming and the GWT convention.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

xgreenx · 2024-09-25T13:18:33Z

crates/services/txpool_v2/src/collision_manager/basic.rs

-    ) -> Result<Collisions<S::StorageIndex>, Error> {
-        let mut collisions = Collisions::new();
+    ) -> Result<Option<S::StorageIndex>, Error> {
+        let mut collision: Option<S::StorageIndex> = None;


We need to document the rule that we agreed on in the code since it is not clear why we allow only one collision=)

Also, I want to highlight that we agreed that this is applicable to dependent transactions. In the case of executable transactions, we can collide with more than one transaction if it is more profitable than all collided subtrees=)

But we can handle it in a separate PR if you want=) But I think it is better to return all behavior of the BasicCollisionManager and just check the number of collided transactions in another place

Ok I will add the documentation and will change the behavior however I don't like to check the number of collisions depending on being executable or not outside of the CollisionManager maybe I can add a new separate method to the collision manager. For me it's not only a detector it should be able to resolve them and be sure that the one left are less worth

Done I documented in collision manager and check pool full. I have split the detection and resolution of collisions in two different function and reworked the whole logic. Could be a bit optimised on the collisions resolution iteration but I think it will not be our pain point for now

crates/services/txpool_v2/src/collision_manager/basic.rs

crates/services/txpool_v2/src/collision_manager/mod.rs

crates/services/txpool_v2/src/config.rs

crates/services/txpool_v2/src/heavy_async_processing.rs

xgreenx · 2024-09-25T13:43:37Z

crates/services/txpool_v2/src/pool.rs

+        let mut gas_left = self.current_gas.saturating_add(tx_gas);
+        let mut bytes_left = self.current_bytes_size.saturating_add(bytes_size);
+        let mut txs_left = self.storage.count().saturating_add(1);
+        let current_ratio = Ratio::new(tx.tip(), tx_gas);
+        let mut sorted_txs = self
+            .storage
+            .get_worst_ratio_tip_gas_subtree_roots()?
+            .into_iter();
+        while gas_left > self.config.pool_limits.max_gas
+            || bytes_left > self.config.pool_limits.max_bytes_size
+            || txs_left > self.config.pool_limits.max_txs
+        {
+            let storage_id = sorted_txs.next().ok_or(Error::NotInsertedLimitHit)?;
+            let storage_data = self.storage.get(&storage_id)?;
+            let mut dependencies = self.storage.get_dependencies(storage_id)?;
+            match dependencies.next() {
+                Some(_) => {}
+                None => {
+                    let stored_ratio = Ratio::new(
+                        storage_data.transaction.tip(),
+                        storage_data.transaction.max_gas(),
+                    );
+                    if stored_ratio >= current_ratio {
+                        return Err(Error::NotInsertedLimitHit);
+                    }
+                }
+            }
+            gas_left = gas_left.saturating_sub(storage_data.dependents_cumulative_gas);
+            bytes_left =
+                bytes_left.saturating_sub(storage_data.dependents_cumulative_bytes_size);
+            txs_left = txs_left.saturating_sub(1);
+            removed_transactions.push(storage_id);
+        }


If you store the total_gas_of_children and the total_size_of_children in the storage data, you don't need to do iterations here=) Plus I think removed_transactions doesn't belong to this function as well.

The iteration is to delete multiple subtrees if needed. However, the dependencies things is useless and I have simplified the function
If this function doesn't retuns the removed_transactions we will not know which transaction to delete to make some space

xgreenx · 2024-09-25T13:44:06Z

crates/services/txpool_v2/src/pool.rs

+        self.config.black_list.check_blacklisting(&tx)?;
+        Self::check_blob_does_not_exist(&tx, &latest_view)?;
+        self.storage
+            .validate_inputs(&tx, &latest_view, self.config.utxo_validation)?;
+        let collision = self
+            .collision_manager
+            .collect_colliding_transaction(&tx, &self.storage)?;
+        let dependencies = self.storage.collect_transaction_dependencies(&tx)?;


Why we can't call can_insert_transaction instead?

In this case I will need to make can_insert_transaction to return the deleted transactions and the dependencies which, IMO, makes the function prototype very strange versus a bit of code duplication, I went for a bit of code duplication

xgreenx · 2024-09-25T13:47:11Z

crates/services/txpool_v2/src/pool.rs

+        if let Some(collision) = collision {
+            if self
+                .storage
+                .is_in_dependencies_subtrees(collision, &dependencies)?
+            {
+                return Err(Error::NotInsertedCollisionIsDependency);
+            }
+        }


I don't like that this functionality moved from storage to TxPool. I think it should be part of the validate_inputs logic

Ok, I have moved it to can_store_transaction of storage which IMO is more approriate

xgreenx · 2024-09-25T14:15:49Z

crates/services/txpool_v2/src/pool.rs

+        let current_ratio = Ratio::new(tx.tip(), tx_gas);
+        let mut sorted_txs = self
+            .storage
+            .get_worst_ratio_tip_gas_subtree_roots()?


I think get_worst_ratio_tip_gas_subtree_roots shouldn't live inside of the storage. We overload the storage responsibility. It is responsibility of the TxPool to decide how to squeeze out non profitable transactions.

Plus this function is too heavy. It iterates over all nodes in the graph.

I think we should use simplest implementation and leave proper functionality for follow-up PR.

The simplest solution will be to have a sorting map in the TxPool that sorts all dependent transactions(and maybe executable as well). When TxPool is full, we just go over less profitable transactions in mapping and delete them with all their children while there is no space, and the new transaction is more profitable(taking into account the profitability of children).

The problem is that the sorting will be redo each time we delete a child of someone as the cumulative_gas/cumulative_tip of a lot of transactions will change. So I was thinking that computing it during insert could be best but I will change.
But I don't really know how this can be done outside of the storage has it's the storage that is in charge of modifying the cumulative_gas/cumulative_tip and the rest of the txpool don't know when these values are updated

Following our discussion, I changed this, now it uses a function inside the selection algorithm which seems to be the most appropriate for this. I have also fixed a bug that was there

Co-authored-by: Green Baneling <[email protected]>

REVIEWER NOTE: Review #2193 first for context ## Linked Issues/PRs Closes #2187 ## Description (2k of additions is the tests from TxPool v1 which has been instantiated again) This PR adds all the logic for the service and shared state of the TxPool to be able to be used by all the others modules and to make all the auxiliary mechanisms working. The PR changes the way the `SharedState` interact with the pool and the service. Now the `SharedState` only sends commands to the service that will answer him whenever he is time to do depending on the priority of the request (managed through a tokio select biaised). This makes all methods to return async even `find` which I'm not a big fan of but didn't found any better solution. **EDIT: I have made some tests especially tries to run `execute_suite` tests and it seems that blocking can cause "double blocking forever" and so I'm up for an alternative** Here is a list of all interactions made by the different modules and their new name/behavior/reason : ### Block producer | Existing | New | | - | - | | select_transactions(max_gas) | select_transactions(max_gas) | ### PoA | Existing | New | | - | - | | total_consumable_gas() | Nothing (unused) | | transaction_status_events() | Replaced by new_txs_notifier [reason](https://github.com/FuelLabs/fuel-core/blob/d5ba082264e1e3db54c6c81f6036eb06b9d3044c/crates/services/consensus_module/poa/src/service.rs#L590) | | pending_number() | Not used anymore after transaction_status_events() change | | remove_txs() | broadcast_txs_skipped_reason() | ### Block Importer | Existing | New | | - | - | | Stream with blockEvents | Stream with block events | ### P2P | Existing | New | | - | - | | gossiped_tx_stream | tx_from_p2p_stream | | new_tx_gossip_subscription | new_peers_subscribed_stream | | broadcast_transaction | broadcast_transaction | | get_tx_ids| get_tx_ids | | get_txs | get_txs | | notify_gossip_transaction_validity | notify_gossip_transaction_validity | ### GraphQL API | Existing | New | | - | - | | transaction | transaction | | tx_update_subscribe | tx_update_subscribe | ### Others changes - The PR also changes the error type to help separate subdomains of errors and try to be more clear about the moment errors can happens. - The TTL management of the transactions is also in place as the less priority task for the service. - A new thread pool has been added to manage P2P sync that could take a long time (same behavior as txpool v1) - The transactions are now stored as Arc because they need to be shareable with others modules (API, P2P) - All the tests from the previous pool has been instantiated and pass. - The TxPool don't have anymore unused authorization and match the correct linting requirements. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here --------- Co-authored-by: Green Baneling <[email protected]>

REVIEWER NOTE: Review FuelLabs/fuel-core#2193 first for context ## Linked Issues/PRs Closes FuelLabs/fuel-core#2187 ## Description (2k of additions is the tests from TxPool v1 which has been instantiated again) This PR adds all the logic for the service and shared state of the TxPool to be able to be used by all the others modules and to make all the auxiliary mechanisms working. The PR changes the way the `SharedState` interact with the pool and the service. Now the `SharedState` only sends commands to the service that will answer him whenever he is time to do depending on the priority of the request (managed through a tokio select biaised). This makes all methods to return async even `find` which I'm not a big fan of but didn't found any better solution. **EDIT: I have made some tests especially tries to run `execute_suite` tests and it seems that blocking can cause "double blocking forever" and so I'm up for an alternative** Here is a list of all interactions made by the different modules and their new name/behavior/reason : ### Block producer | Existing | New | | - | - | | select_transactions(max_gas) | select_transactions(max_gas) | ### PoA | Existing | New | | - | - | | total_consumable_gas() | Nothing (unused) | | transaction_status_events() | Replaced by new_txs_notifier [reason](https://github.com/FuelLabs/fuel-core/blob/d5ba082264e1e3db54c6c81f6036eb06b9d3044c/crates/services/consensus_module/poa/src/service.rs#L590) | | pending_number() | Not used anymore after transaction_status_events() change | | remove_txs() | broadcast_txs_skipped_reason() | ### Block Importer | Existing | New | | - | - | | Stream with blockEvents | Stream with block events | ### P2P | Existing | New | | - | - | | gossiped_tx_stream | tx_from_p2p_stream | | new_tx_gossip_subscription | new_peers_subscribed_stream | | broadcast_transaction | broadcast_transaction | | get_tx_ids| get_tx_ids | | get_txs | get_txs | | notify_gossip_transaction_validity | notify_gossip_transaction_validity | ### GraphQL API | Existing | New | | - | - | | transaction | transaction | | tx_update_subscribe | tx_update_subscribe | ### Others changes - The PR also changes the error type to help separate subdomains of errors and try to be more clear about the moment errors can happens. - The TTL management of the transactions is also in place as the less priority task for the service. - A new thread pool has been added to manage P2P sync that could take a long time (same behavior as txpool v1) - The transactions are now stored as Arc because they need to be shareable with others modules (API, P2P) - All the tests from the previous pool has been instantiated and pass. - The TxPool don't have anymore unused authorization and match the correct linting requirements. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here --------- Co-authored-by: Green Baneling <[email protected]>

AurelienFT added 5 commits September 12, 2024 14:56

start definition of the pool

29dd130

Merge branch 'txpool-v2' into txpool-v2-api

4049dac

Add insertion skeleton

dd6f7c1

Add all the verifications in order. Still some todo and compil errors

460beef

Fix compil errors

75fb598

AurelienFT changed the title ~~Txpool v2 api~~ Txpool v2 insertion Sep 13, 2024

AurelienFT mentioned this pull request Sep 13, 2024

TxPool v2 General architecture #2162

Merged

5 tasks

AurelienFT added 8 commits September 16, 2024 10:12

Merge branch 'txpool-v2' into txpool-v2-api

054f648

Finish verification on all inputs

a7666fc

fix verifications to make no copy

4426134

Reorganize insertion and opti some code. Start rework of tests.

86a7c86

Update all the tests

96e0cc9

Add heavy async work for insert

cb635e8

Merge branch 'txpool-v2' into txpool-v2-api

e379a0c

Clean up of the code

e7dc924

AurelienFT marked this pull request as ready for review September 18, 2024 09:14

AurelienFT requested review from Dentosal, MitchTurner and xgreenx as code owners September 18, 2024 09:14

Update changelog

c12ee38

AurelienFT requested a review from a team September 18, 2024 09:15

AurelienFT self-assigned this Sep 18, 2024

AurelienFT and others added 2 commits September 18, 2024 14:16

fix clippy

4f87fbd

Merge branch 'txpool-v2' into txpool-v2-api

3f11941

AurelienFT requested a review from netrome September 19, 2024 09:57

xgreenx mentioned this pull request Sep 19, 2024

TxPool test suite must cover success cases and failure cases #762

Closed

Merge branch 'txpool-v2' into txpool-v2-api

697cf4c

AurelienFT mentioned this pull request Sep 19, 2024

Txpool v2 features for futures connections with other modules #2216

Merged

5 tasks

AurelienFT and others added 2 commits September 20, 2024 10:07

Merge branch 'txpool-v2' into txpool-v2-api

fdd5f8a

Merge branch 'txpool-v2' into txpool-v2-api

72add0f

AurelienFT added 2 commits September 23, 2024 16:47

Change insertion checks to be simplified

cf9e9eb

Simplified insert

1c59fc7

AurelienFT marked this pull request as ready for review September 23, 2024 14:53

AurelienFT and others added 4 commits September 23, 2024 16:58

Fix collision system to allow multiple collision to the same tx

e859b60

Fix some node that wasn't removed.

1f8aaf8

format

c7b3a29

fix max size algo and add tests

0ebfc5c

xgreenx reviewed Sep 25, 2024

View reviewed changes

AurelienFT and others added 5 commits September 26, 2024 00:14

Update crates/services/txpool_v2/src/config.rs

82c9dd9

Co-authored-by: Green Baneling <[email protected]>

Update crates/services/txpool_v2/src/config.rs

70a45e9

Co-authored-by: Green Baneling <[email protected]>

Change collision manager and add more info in error

677a130

Update pool check size

26871c8

Move logic from pool to storage

76b2af4

AurelienFT requested a review from xgreenx September 25, 2024 23:50

AurelienFT and others added 9 commits September 26, 2024 01:51

fmt and clippy

13e6990

remove unused stream predicate verif

6249e5d

Merge branch 'txpool-v2' into txpool-v2-api

d6dea78

Change way to get less worth it transactions

83c56ae

Fix clippy

de44984

Merge branch 'txpool-v2' into txpool-v2-api

2edff72

Merge branch 'txpool-v2' into txpool-v2-api

029196c

simplify an unused function

4bb55ba

Merge branch 'txpool-v2' into txpool-v2-api

0570070

AurelienFT merged commit c128c75 into txpool-v2 Sep 27, 2024

AurelienFT deleted the txpool-v2-api branch September 27, 2024 16:04

Txpool v2 insertion #2193

Txpool v2 insertion #2193

Uh oh!

Conversation

AurelienFT commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked Issues/PRs

Description

Changes in code logic from TxPool v1

Changes from the base branch txpool-v2

Checklist

Before requesting review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AurelienFT Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AurelienFT Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AurelienFT commented Sep 13, 2024 •

edited

Loading

Changes in code logic from `TxPool v1`

Changes from the base branch `txpool-v2`

AurelienFT Sep 25, 2024 •

edited

Loading

AurelienFT Sep 25, 2024 •

edited

Loading