Txpool v2 features for futures connections with other modules #2216

AurelienFT · 2024-09-18T12:41:48Z

REVIEWER NOTE: Review #2193 first for context

Linked Issues/PRs

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

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

…r' into txpool-v2-block-producer-importer

xgreenx · 2024-10-07T15:19:10Z

crates/services/txpool_v2/src/tests/tests_e2e.rs

@@ -0,0 +1,272 @@
+//! Test module for update_sender


File identical to the old one that we had=) skipping

xgreenx · 2024-10-07T15:27:12Z

crates/services/txpool_v2/src/tests/tests_permits.rs

@@ -0,0 +1,129 @@
+use std::{


The file is identical to V1, skipping=)

xgreenx · 2024-10-07T15:29:31Z

crates/services/txpool_v2/src/tx_status_stream.rs

@@ -0,0 +1,135 @@
+use fuel_core_types::{


The file is identical to V1, skipping=)

xgreenx · 2024-10-07T15:31:36Z

crates/services/txpool_v2/src/update_sender.rs

@@ -0,0 +1,392 @@
+use std::{


Content is identical to V1, the difference that 2 types were merged int one file.

xgreenx · 2024-10-07T15:32:20Z

crates/services/txpool_v2/src/tests/utils.rs

@@ -0,0 +1,260 @@
+use std::{


Identical to the V1, skipping.

xgreenx · 2024-10-07T15:33:10Z

crates/services/txpool_v2/src/tests/tests_update_stream_state.rs

@@ -0,0 +1,87 @@
+//! Test module for validating TxUpdateStream state transitions.


Identical to V1, skipping.

…bs/fuel-core into txpool-v2-block-producer-importer

…ge different tasks.

acerone85 · 2024-10-07T19:13:50Z

crates/services/txpool_v2/src/service.rs

+    pool: Shared<TxPool>,
+    current_height: Arc<RwLock<BlockHeight>>,


is there any reason why you use the Shared type alias for pool and not for current_height?
Overall I think Arc<RwLock<_>> is more clear

crates/services/txpool_v2/src/service.rs

netrome

Some opinions on the Shared type, continuing my review...

netrome · 2024-10-07T19:56:08Z

crates/services/src/lib.rs

+// For tests
+use crate as fuel_core_services;
+#[allow(unused_imports)]
+use fuel_core_services as _;


What is the purpose of this aliasing dance? We import the crate under a type alias and then alias it to _. This feels weird, but I guess we must achieve something by doing this.

It was done to enable feature by default during testing

netrome · 2024-10-07T20:02:08Z

crates/services/txpool_v2/src/service.rs

-    heavy_async_processor: Arc<HeavyAsyncProcessor>,
-    utxo_validation: bool,
-}
+pub type Shared<T> = Arc<RwLock<T>>;


While this is a pretty nice type-alias, I don't see any point in making it public. Any other modules that would benefit from the type alias can redefine it, which would be more straight-forward to read and just as concise as importing it without the extra indirection.

Suggested change

pub type Shared<T> = Arc<RwLock<T>>;

type Shared<T> = Arc<RwLock<T>>;

Optionally if this is used in a lot of places we should move it out to another module, since the alias has nothing to do with this particular service.

I changed it to pub(crate) because it's used in the tests

netrome · 2024-10-07T20:04:12Z

crates/services/txpool_v2/src/service/verifications.rs

+    },
+    service::{
+        memory::MemoryPool,
+        Shared,


For example here I would find it more readable if we instead of importing Shared redefine the alias in this module.

will let @xgreenx give his advice on this as it's a type he added

Up to @AurelienFT )

Noted on my note will think about it in the next days to not block the merge.

…bs/fuel-core into txpool-v2-block-producer-importer

netrome

Another suggestion (not blocking merge)

netrome · 2024-10-07T20:58:05Z

crates/services/txpool_v2/src/service.rs

+                    should_continue = true;
+                } else {
+                    should_continue = false;


I know the pattern was established before this PR, but it would make more sense to me to just return here instead of declaring and assigning the variable to later return it.

Suggested change

should_continue = true;

} else {

should_continue = false;

return Ok(true);

} else {

return Ok(false);

(and the same for all other branches)

Nevermind, I see you already merged! 🎉

It's Ok you can leave comment and I will address in patch in the future. I note this and let this open in the meantime

crates/services/txpool_v2/src/pool.rs

crates/services/txpool_v2/src/service.rs

crates/services/txpool_v2/src/pool.rs

@rafal-ch

## Version v0.38.0 ### Added - [2309](#2309): Limit number of concurrent queries to the graphql service. - [2216](#2216): Add more function to the state and task of TxPoolV2 to handle the future interactions with others modules (PoA, BlockProducer, BlockImporter and P2P). - [2263](#2263): Use the Txpool v2 in the whole codebase. ### Removed - [2306](#2306): Removed hack for genesis asset contract from the code. ## What's Changed * Updates to price algorithm by @rafal-ch in #2292 * Removed hack from the code by @xgreenx in #2306 * fix(p2p): useless dials to reserved nodes which dont have a location multiaddress by @rymnc in #2308 * Txpool v2 features for futures connections with other modules by @AurelienFT in #2216 * Limit number of concurrent queries to the graphql service by @netrome in #2309 * Linking TxPoolV2 in place of V1 by @AurelienFT in #2263 **Full Changelog**: v0.37.1...v0.38.0 --------- Co-authored-by: AurelienFT <[email protected]>

AurelienFT and others added 30 commits September 5, 2024 00:20

Add skeleton txpool v2

2e07d37

Add basic interfaces and retrieve some functionnalities of the old pool.

4cbb11c

Add all machinery to make the tests

bef4aff

Add some logic that needs to be moved behind abstraction

552af83

Add all storages for transactions and clean code a bit

8d39499

Merge branch 'master' into txpool-v2

3d1a1bb

Fix compilation errors and add selection mecanism

3d65076

Start thinking about traits

19830cb

Add all abstractions in txpool to clean code

795567e

Split all logic across implementations and uncomment half of the tests

67afb9d

Add all tests

b956e2c

Fmt + toml lint + spellcheck

9ed577e

Resolves most of the TODO, remove unwrap and add test

29ec06d

Document and allow warnings on dead code/unused

9a22983

Merge branch 'master' into txpool-v2

ca38a25

fix fuel-core-type usage in ttxpool v2

fe9e995

Remove usage of txpool v2 in fuel-core

34c7e07

fmt fix

4576797

Don't allow dependencies on output and change and specify todos

c8e86ec

Rework dependency between components

16a7e8f

Simplify usage of the db trait

60ada65

update graph cumultative gas and tip

0cf8835

Docs fixes and config renaming

b328bc8

fmt

0d69dc1

start definition of the pool

29dd130

Use persistant storage instead of db and reuse a view provider

75a9180

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

4049dac

Add insertion skeleton

dd6f7c1

Fix comments in tests

87e5027

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

460beef

AurelienFT and others added 3 commits October 7, 2024 18:04

Merge branch 'master' into txpool-v2-block-producer-importer

8dfbf99

Merge remote-tracking branch 'origin/txpool-v2-block-producer-importe…

d683d93

…r' into txpool-v2-block-producer-importer

Shuffle code)

dbc9206

xgreenx reviewed Oct 7, 2024

View reviewed changes

AurelienFT and others added 5 commits October 7, 2024 18:14

fix compilation txpool

9d73f89

Merge branch 'txpool-v2-block-producer-importer' of github.com:FuelLa…

f28da55

…bs/fuel-core into txpool-v2-block-producer-importer

Implemented sync and async processor. Used both in the TxPool to mana…

4e79e4a

…ge different tasks.

Make CI happy

d3dd371

Mia

68df4bc

xgreenx previously approved these changes Oct 7, 2024

View reviewed changes

acerone85 reviewed Oct 7, 2024

View reviewed changes

fix tests old tx pool

71213d9

AurelienFT dismissed xgreenx’s stale review via 71213d9 October 7, 2024 19:28

netrome reviewed Oct 7, 2024

View reviewed changes

crates/services/txpool_v2/src/service.rs Show resolved Hide resolved

AurelienFT and others added 2 commits October 7, 2024 21:55

Fix wrong comparison max gas

0533ee1

Merge branch 'master' into txpool-v2-block-producer-importer

feda383

netrome reviewed Oct 7, 2024

View reviewed changes

AurelienFT added 2 commits October 7, 2024 22:12

Change type to pub(crate)

badb310

Merge branch 'txpool-v2-block-producer-importer' of github.com:FuelLa…

6d9c468

…bs/fuel-core into txpool-v2-block-producer-importer

acerone85 approved these changes Oct 7, 2024

View reviewed changes

xgreenx approved these changes Oct 7, 2024

View reviewed changes

AurelienFT enabled auto-merge (squash) October 7, 2024 20:41

AurelienFT merged commit fdcec1c into master Oct 7, 2024
32 checks passed

AurelienFT deleted the txpool-v2-block-producer-importer branch October 7, 2024 20:45

netrome reviewed Oct 7, 2024

View reviewed changes

rafal-ch reviewed Oct 7, 2024

View reviewed changes

crates/services/txpool_v2/src/pool.rs Show resolved Hide resolved

crates/services/txpool_v2/src/service.rs Show resolved Hide resolved

crates/services/txpool_v2/src/pool.rs Show resolved Hide resolved

AurelienFT mentioned this pull request Oct 7, 2024

Add specific tests for each module in the TxPool v2 #2196

Open

xgreenx mentioned this pull request Oct 8, 2024

Release v0.38.0 #2315

Merged

netrome mentioned this pull request Oct 9, 2024

refactor: Eager returns in txpool_v2::service::Task::run #2325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Txpool v2 features for futures connections with other modules #2216

Txpool v2 features for futures connections with other modules #2216

AurelienFT commented Sep 18, 2024 •

edited

Loading

xgreenx Oct 7, 2024

xgreenx Oct 7, 2024

xgreenx Oct 7, 2024

xgreenx Oct 7, 2024

xgreenx Oct 7, 2024

xgreenx Oct 7, 2024

acerone85 Oct 7, 2024

netrome left a comment

netrome Oct 7, 2024

AurelienFT Oct 7, 2024

xgreenx Oct 7, 2024

netrome Oct 7, 2024

netrome Oct 7, 2024

AurelienFT Oct 7, 2024

netrome Oct 7, 2024

AurelienFT Oct 7, 2024

xgreenx Oct 7, 2024

AurelienFT Oct 7, 2024

netrome left a comment

netrome Oct 7, 2024

netrome Oct 7, 2024

AurelienFT Oct 7, 2024

		@@ -0,0 +1,87 @@
		//! Test module for validating TxUpdateStream state transitions.

		pool: Shared<TxPool>,
		current_height: Arc<RwLock<BlockHeight>>,

	pub type Shared<T> = Arc<RwLock<T>>;
	type Shared<T> = Arc<RwLock<T>>;

Txpool v2 features for futures connections with other modules #2216

Txpool v2 features for futures connections with other modules #2216

Conversation

AurelienFT commented Sep 18, 2024 • edited Loading

Linked Issues/PRs

Description

Block producer

PoA

Block Importer

P2P

GraphQL API

Others changes

Checklist

Before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurelienFT commented Sep 18, 2024 •

edited

Loading