Controlc/tx p2p #491

ControlCplusControlV · 2022-07-14T20:28:39Z

Design to closes #477 and #478 . Adds 2 new channels to the txpool designed for p2p to hook into. One channel is a mpsc channel designed to send transactions received on the txpool from the gql endpoint to p2p, so that it can broadcast them to other nodes. The other is a simple broadcast channel which is used to receive transactions from p2p which should be included in the txpool. Tests are bit rough atm and need some polishing, but making a draft PR for general design review and feedback

Voxelot · 2022-07-14T20:54:04Z

fuel-txpool/src/service.rs

    db: Option<Box<dyn TxPoolDb>>,
    import_block_events: Option<broadcast::Receiver<ImportBlockBroadcast>>,
+    broadcast_txs: Option<mpsc::Sender<TransactionBroadcast>>,


Based on our earlier call today, I think we should use P2pRequestEvent instead of the TransactionBroadcast enum for sending.

Another minor nit is that broadcast_txs isn't totally clear which direction the broadcast is going. Maybe something like network_outbox would help clarify the intention?

Suggested change

broadcast_txs: Option<mpsc::Sender<TransactionBroadcast>>,

network_outbox: Option<mpsc::Sender<P2pRequestEvent>>,

I moved this to P2pRequestEvent, I do remember talking about this, I think it got mixed up between fuel-core-interfaces/p2p/P2pRequestEvent::BroadcastNewTransaction and fuel-core-interfaces/p2p/TransactionBroadcast::NewTransaction as both have similar names, and added the rename

Voxelot · 2022-07-14T23:42:03Z

fuel-txpool/src/service.rs

@@ -10,9 +13,11 @@ pub struct ServiceBuilder {
    sender: txpool::Sender,
    receiver: mpsc::Receiver<TxPoolMpsc>,
    config: Config,
-    broadcast: broadcast::Sender<TxStatusBroadcast>,
+    consumer: broadcast::Sender<TxStatusBroadcast>,


consumer could mean a lot of things... we could just call this tx_status_updates?

I agree. I am aiming to disambiguate some of the naming: sender, receiver, consumer, etc., are pretty vague and I find it can be confusing trying to understand which channels are used for what. I am taking a stab at that now.

bvrooman · 2022-07-14T23:58:20Z

I noticed that the test insert_from_p2p seems to be flaky when I run it locally: most of the times it passes but sometimes it fails. It looks like there's a race condition. We're expecting the same transaction to be sent twice and for an error to raise on the second submission. However, in some cases, it looks like we get to the assert and check the receiver before both submissions are received, causing the test to fail.

ControlCplusControlV · 2022-07-15T00:33:51Z

I noticed that the test insert_from_p2p seems to be flaky when I run it locally: most of the times it passes but sometimes it fails. It looks like there's a race condition. We're expecting the same transaction to be sent twice and for an error to raise on the second submission. However, in some cases, it looks like we get to the assert and check the receiver before both submissions are received, causing the test to fail.

@bvrooman Race condition is most likely because send isn't awaited in any way, so if the send doesn't process in time then insertion could possible go through (as the previous transaction hasn't been inserted yet), leading to no error and the test failing. This is one thing that needs to be fixed in the test, which ideally should just have a base way to test the sending of the transaction rather than measuring side affects

bvrooman · 2022-07-15T22:53:31Z

I have some additional thoughts:

As currently designed in this PR, the txpool service (and inner context) has a new receiver that will receive events for incoming transactions. When it receives one, it will insert it into the txpool.

This is similar to when the existing txpool receiver gets an insert event, where it then signals the service to insert the transactions into the txpool. However, the difference between inserting transactions and receiving an incoming transaction is that we must broadcast the transactions and status (submitted, squeezed out, etc.) on insertion, but not on an incoming transaction.

Because this PR currently uses the same TxPool::insert method in both cases, we had to augment the signature to accept an optional broadcaster: we pass Some(network_outbox) on insertion, but None on incoming transactions. Inside insert, we then check against the Option, to see if we need to put anything in that outbox. This is a bit of a hack we employed to get our draft working. I'd argue that this approach can be improved upon, and the insert method isn't well suited for the latter case of an incoming transaction. Rather, we can introduce a new TxPool method, something like TxPool::insert_one to handle this case. This is further evidenced by the fact that insert is designed to operate on a vector of transactions, rather than a single transaction, and we have to wrap any incoming transaction in a 1-sized vector to accommodate this interface.

When I get a chance, I will look at this refactor and push a commit here, along with the ongoing refactoring of the existing service and context channel names.

bvrooman · 2022-07-15T23:06:16Z

We also do not seem to have much consistency on our channel naming conventions. In a number of places, we use terms like "sender", "producer", and now "outbox" to refer to objects of Sender. We use terms like "receiver", "incoming", and "events", for objects of type Receiver. Personally, I find this can be fairly confusing and unintuitive to read. It would be great to consolidate on some naming convention for channels.

I am aiming to address naming consistency issues in areas localized to this PR, but I'd like to hear any thoughts/suggestions you guys might have on this.

ControlCplusControlV · 2022-07-20T13:29:03Z

I agree @bvrooman that fixing naming would be nice to add, I also think it would be good to have a single insert, as it cuts out of a lot interation logic which isn't needed for a singular tx. I think this may be best for a later PR though as we will eventually want to do batch insert from p2p, which will then require a different flow. Unless you think it will be worth having both flows built out? (ie p2p_insert_single, and p2p_insert_many)

bvrooman · 2022-07-20T18:31:28Z

I had a conversation on Monday with @Voxelot regarding the feature as well as the currently proposed code:

One key discussion point was that we do not need to perform any broadcasting on new local transactions (at least, not at this time). The agreed action item was to simplify the current implementation, and to remove the broadcasting on this event.

This means a couple of things:

The handler for this event becomes simpler, and we do not need to send anything to the network_outbox
We do not need to test for broadcasts on local insertions
The network_outbox itself is unnecessary, and it can be removed entirely from the service and context, along with the corresponding service builder methods
The existing TxPool::insert function signature can be reverted back to its original signature, and we no longer need the Option<Sender> parameter

This simplifies things in the current PR. I am putting up the commit for this shortly.

I'd just like to get @Voxelot to confirm that my understanding of this is correct.

When it comes to future flows, like supporting batch insert from p2p, I think we should address that when the time comes. We can take an iterative approach and build up. As long as we are careful not to paint ourselves into any corners right now, we should be in a good position to build out further use cases when they are required.

CC: @ControlCplusControlV

Voxelot · 2022-07-21T00:11:15Z

I think there's some confusion around the terminology of what was meant by local insert. What I thought we were discussing is that we don't need to do any additional p2p broadcast for insert_local (which is confusingly only actually used when receiving txs from non-local sources like the p2p network). We would still need to broadcast from the main TxPool::insert method since that's what the GraphQL API uses to insert txs from a locally submitted txn. If we rip out the p2p request sender/outbox entirely from the TxPool service, I don't know how other nodes would be expected to receive anything in that case if there's no mechanism at all to send messages when a user submits a tx.

ControlCplusControlV · 2022-07-21T15:18:31Z

@Voxelot yes this is in line with what I was originally thinking. Were you still thinking of using TxPool::Insert or breaking that out into a new a mspc method to use an entirely different function to insert from p2p or to insert from gql. I am locked of my slack until I am back from ETHcc (passcode issue) but happy to discuss on here and over something like TG

EDIT : Also looking at CI issues

bvrooman · 2022-07-22T01:51:50Z

I talked with @Voxelot today, and did a bit of refactoring based on our discussion. I am introducing Txpool::insert_with_broadcast that effectively wraps Txpool::insert and broadcasts a new p2p transaction if the insertion was successful. Now, we perform insert on p2p transactions, and insert_with_broadcast on local transactions (i.e., graphql).

Voxelot · 2022-07-27T20:25:55Z

fuel-core/src/service/modules.rs

@@ -56,9 +59,15 @@ pub async fn start_modules(config: &Config, database: &Database) -> Result<Modul
                .unwrap(),
        );

+    // Meant to simulate p2p's channels which hook in to communicate with txpool
+    let (network_sender, _) = mpsc::channel(100);


Without any network_receiver, this queue will fill up and may stop the txpool from working once it's reached 100 txs.

We likely need to spawn a task to drain this queue whenever txs are added until the p2p service can perform this function.

bvrooman · 2022-07-28T22:25:13Z

I am squashing & merging this branch into another dev branch to make future rebases easier. We are going to rebase with Elvis's development branch to continue development.

* added p2p stuff, tests still WIP * Restructure DummyP2PInterface and use it in tests * Added p2p stuff for real this time I think * ooops * tests go green * ooops * partial test * comment * ooops * ooops * Separate network_outbox and incoming_txs on txpool service builder * updated test * updated test * oops * Remove `network_outbox` and rename senders/receivers * Fmt * Missed refactor * Typo * insert_with_broadcast * Fmt * Add network sender to modules * Fmt imports * retrigger checks * retrigger checks * Fix flaky test * Update test name * Replace insert call with duplicate logic * Add additional test * some fixes but not a lot Co-authored-by: Brandon Vrooman <brandon.vrooman@gmail.com> Co-authored-by: Brandon Vrooman <brandon@DESKTOP-CNCCSJQ.localdomain> Co-authored-by: ControlC ControlV <controlc@ControlCs-MacBook-Pro.local>

ControlCplusControlV and others added 9 commits July 13, 2022 14:07

added p2p stuff, tests still WIP

ef7df98

Restructure DummyP2PInterface and use it in tests

41f308a

Added p2p stuff for real this time I think

98072f0

Merge remote-tracking branch 'origin/master' into controlc/tx_p2p

c71ba87

ooops

0471f4c

tests go green

74a1c3f

ooops

a005643

partial test

4ec01b1

comment

2b16508

ControlCplusControlV added enhancement New feature or request fuel-txpool labels Jul 14, 2022

ControlCplusControlV requested review from Voxelot and rakita July 14, 2022 20:28

ControlCplusControlV assigned bvrooman and ControlCplusControlV Jul 14, 2022

ControlCplusControlV marked this pull request as draft July 14, 2022 20:35

Voxelot reviewed Jul 14, 2022

View reviewed changes

ControlCplusControlV added 2 commits July 14, 2022 15:23

ooops

d14904e

ooops

f708dda

Voxelot reviewed Jul 14, 2022

View reviewed changes

bvrooman and others added 4 commits July 14, 2022 22:57

Separate network_outbox and incoming_txs on txpool service builder

22ed701

updated test

26946b7

updated test

05f6d02

oops

73c39fe

Merge branch 'master' into controlc/tx_p2p

7fad1fe

bvrooman and others added 6 commits July 21, 2022 20:10

insert_with_broadcast

3e4ba07

Fmt

07487ba

Add network sender to modules

b0e1940

Fmt imports

682bca9

retrigger checks

5f0b114

retrigger checks

33d7be9

bvrooman and others added 5 commits July 23, 2022 00:06

Fix flaky test

04d1873

Update test name

f7dc16e

Merge branch 'master' into controlc/tx_p2p

ea737fb

Replace insert call with duplicate logic

076bf38

Add additional test

2e87fa5

ControlCplusControlV removed the request for review from rakita July 26, 2022 17:31

ControlCplusControlV marked this pull request as ready for review July 26, 2022 17:31

ControlCplusControlV requested review from Voxelot and bvrooman July 26, 2022 17:31

Voxelot reviewed Jul 27, 2022

View reviewed changes

some fixes but not a lot

e7687c6

ControlCplusControlV marked this pull request as draft July 27, 2022 22:58

ControlCplusControlV mentioned this pull request Jul 28, 2022

Include p2p into fuel core #492

Merged

Merge branch 'master' into controlc/tx_p2p

fb0d431

bvrooman changed the base branch from master to bvrooman-tx_p2p_merge July 28, 2022 22:23

bvrooman marked this pull request as ready for review July 28, 2022 22:25

bvrooman merged commit cb70e24 into bvrooman-tx_p2p_merge Jul 28, 2022

bvrooman deleted the controlc/tx_p2p branch July 28, 2022 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controlc/tx p2p #491

Controlc/tx p2p #491

ControlCplusControlV commented Jul 14, 2022

Voxelot Jul 14, 2022

ControlCplusControlV Jul 14, 2022 •

edited

Loading

Voxelot Jul 14, 2022

bvrooman Jul 14, 2022

bvrooman commented Jul 14, 2022

ControlCplusControlV commented Jul 15, 2022

bvrooman commented Jul 15, 2022 •

edited

Loading

bvrooman commented Jul 15, 2022 •

edited

Loading

ControlCplusControlV commented Jul 20, 2022

bvrooman commented Jul 20, 2022 •

edited

Loading

Voxelot commented Jul 21, 2022 •

edited

Loading

ControlCplusControlV commented Jul 21, 2022 •

edited

Loading

bvrooman commented Jul 22, 2022

Voxelot Jul 27, 2022 •

edited

Loading

bvrooman commented Jul 28, 2022

	broadcast_txs: Option<mpsc::Sender<TransactionBroadcast>>,
	network_outbox: Option<mpsc::Sender<P2pRequestEvent>>,

Controlc/tx p2p #491

Controlc/tx p2p #491

Conversation

ControlCplusControlV commented Jul 14, 2022

Voxelot Jul 14, 2022

Choose a reason for hiding this comment

ControlCplusControlV Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Voxelot Jul 14, 2022

Choose a reason for hiding this comment

bvrooman Jul 14, 2022

Choose a reason for hiding this comment

bvrooman commented Jul 14, 2022

ControlCplusControlV commented Jul 15, 2022

bvrooman commented Jul 15, 2022 • edited Loading

bvrooman commented Jul 15, 2022 • edited Loading

ControlCplusControlV commented Jul 20, 2022

bvrooman commented Jul 20, 2022 • edited Loading

Voxelot commented Jul 21, 2022 • edited Loading

ControlCplusControlV commented Jul 21, 2022 • edited Loading

bvrooman commented Jul 22, 2022

Voxelot Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

bvrooman commented Jul 28, 2022

ControlCplusControlV Jul 14, 2022 •

edited

Loading

bvrooman commented Jul 15, 2022 •

edited

Loading

bvrooman commented Jul 15, 2022 •

edited

Loading

bvrooman commented Jul 20, 2022 •

edited

Loading

Voxelot commented Jul 21, 2022 •

edited

Loading

ControlCplusControlV commented Jul 21, 2022 •

edited

Loading

Voxelot Jul 27, 2022 •

edited

Loading