Connect P2p with TxPool #590

ControlCplusControlV · 2022-09-04T00:51:39Z

Connects P2p with TxPool which should allow transactions inserted over the txpool of one node to be broadcasted to the others. Done in collaboration with @bvrooman but this has fallen stale so I had to re-impl over master since merge conflicts and git history had grown too messy

Closes #477
Closes #478

ControlCplusControlV · 2022-09-21T17:31:38Z

Seems a lot of tests are failing now, do you know why yet @bvrooman ? I am seeing a lot of timeouts

bvrooman · 2022-09-23T02:00:33Z

It appears that the difficulty we are encountering is caused by the different configurations created by the combinations of different feature flags, specifically the "p2p" and "relayer" feature flags. These enable or disable the P2P service and Relayer service respectively. When we disable services and channels based on flags, we must also be cognizant of how the senders and receivers of each channel is affected.

I'm mapping out the channels in a draw.io doc here. This map is not complete, and it may not be completely accurate thus far.

From this map, we can observe that certain channels will have a different set of senders/receivers depending on the services enabled. When running tests with all features enabled, including P2P and Relayer, tests pass. However, we can see that without P2P, some senders are removed from the system and some signals are never passed to receivers. This causes some channels to wait indefinitely causing timeouts.

We need to make sure that tests pass for all combinations of the matrix:

P2P	Relayer
✗ Disabled	✗ Disabled
✓ Enabled	✗ Disabled
✗ Disabled	✓ Enabled
✓ Enabled	✓ Enabled

Right now, the code is written as if all services are enabled. Tests fail when P2P is disabled (e.g. no-default-features).

It is clear from reading the code and mapping out the connections that using channels to this volume introduces a lot of complexity and potential for error.

ControlCplusControlV · 2022-09-23T02:50:06Z

Will try and reconfigure so we can get all feature combos working, although realistically we just disable tx gossiping if both aren't enabled. Maybe we could do something like wrapping gossip under a meta feature?

Voxelot · 2022-09-23T04:28:54Z

FYI - I don't have permission to open that draw.io link

I think we should try to find the simplest way to enable this for now without coming up with a perfect solution to the channels issue, since we have a pending initiative to redesign our service arch and reduce our reliance on channels where possible.

bvrooman · 2022-09-23T18:09:24Z

I propose a couple of options:

Enable the incoming_tx_receiver.recv() branch of the txpool service context only when p2p is enabled (otherwise, the recv will return an error). However, macros like #[cfg(feature = "p2p")] are not valid syntax once nested inside the tokio::select! macro (see tokio::select! cfg-gated tokio-rs/tokio#3974).
Box the incoming_tx_sender to store it on the heap and leak the box to prevent RAII deletion. This will keep the sender alive, ensuring the receiver has a corresponding sender. Of course, the cost is the memory leak.
Alternatively, we can just make p2p a default feature, and we can worry about the enabling/disabling of p2p once we go through the rearchitecting of the services.

I am pushing a commit to implement the second option. This option is quick and dirty, but it will unblock the tests.

… into controlc/p2p_tx

bvrooman · 2022-09-29T19:18:39Z

fuel-tests/tests/tx_gossip.rs

+    let node_two = FuelService::new_node(node_config).await.unwrap();
+    let client_two = FuelClient::from(node_two.bound_address);
+
+    tokio::time::sleep(Duration::new(3, 0)).await;


TODO: We can replace the sleep calls with polling for peers once the functionality is enabled. This will be tracked in a separate issue here: #649.

bvrooman · 2022-09-29T19:19:33Z

fuel-tests/tests/tx_gossip.rs

+    let tx = client_one.transaction(&result.0.to_string()).await.unwrap();
+    assert!(tx.is_some());
+
+    tokio::time::sleep(Duration::new(3, 0)).await;


TODO: Same as above; we will replace sleep with polling once that is available.

xgreenx

Brief review

xgreenx · 2022-09-30T00:54:09Z

fuel-txpool/src/txpool.rs

+                        tx: tx.clone(),
+                        status: TxStatus::Submitted,
+                    });
+                    let _ = network_sender


The method insert below do the same except for this part. Can we move common logic to a separate function and pass a closure that will broadcast in the case of p2p.

Maybe rather than complicating with a closure, this method could just call the one below, iterate over the Vec<Result<Arc<Transaction>>> return type and then trigger a broadcast for each successful one.

Maybe rather than complicating with a closure, this method could just call the one below, iterate over the Vec<Result<Arc<Transaction>>> return type and then trigger a broadcast for each successful one.

We originally proposed this but we changed it in favour of a more "performant" approach, at the cost of the extra code. But I think this approach is the simplest and cleanest, and I prefer the cleaner approach to the performant approach in this case.

The draw back is:

Double the number of iterations

But the benefits are:

No extra method or code duplication

Easier to read and reason about code

I'll opt for that instead and push a commit for that change.

We should probably atleast leave a note in the future though in case in future txpool optimizations we need areas to improve

Draft PR based on this branch: #670

xgreenx · 2022-09-30T01:09:08Z

fuel-txpool/src/service.rs

+                        match new_transaction.unwrap() {
+                            TransactionBroadcast::NewTransaction ( tx ) => {
+                                let txs = vec!(Arc::new(tx));
+                                TxPool::insert(txpool, db.as_ref().as_ref(), tx_status_sender, txs).await


You must also use insert_with_broadcast if p2p is enabled.

Also, it means that this case is not tested. Do we plan to test in this PR or in follow-up?

This shouldn't be done unless we change the current gossip logic to only broadcast to immediate peers (depth 1). Even with depth of 1, we risk bouncing broadcasts back and forth in an infinite loop. Right now, any gossiped messages will be propagated to the whole network. We have other P2P tasks to investigate how to prevent nodes from re-gossiping invalid data.

Yes, it may not be clear from a cold read, but this branch is executed when new transactions are received from gossip. In this case, we do not want to echo the same transaction back to the network with a broadcast.

xgreenx · 2022-09-30T01:17:37Z

fuel-txpool/src/service.rs

+    pub incoming_tx_receiver: broadcast::Receiver<TransactionBroadcast>,
+    #[cfg(feature = "p2p")]
+    pub network_sender: mpsc::Sender<P2pRequestEvent>,


Maybe we can already introduce the traits we discussed on the call in this PR?

You can represent P2P as Gossiper and TransactionProvider traits. It will aggregate incoming_tx_receiver and network_sender into one field, making the code cleaner. It seems easy to do because TXPool already knows nothing about P2P(based on the imports in Cargo.toml).

I'd prefer to move forward with the current PR as it's been in progress for a long time and we should checkpoint this progress, and then we can do channel / trait conversion in subsequent PRs.

xgreenx · 2022-09-30T01:30:59Z

fuel-txpool/src/txpool.rs

+        }
+        for (ret, tx) in res.iter().zip(txs.into_iter()) {
+            match ret {
+                Ok(removed) => {


In the current code, we check that transactions were in the transactions pool, but we don't check that transactions have been committed in the block.

With the introduction of P2P gossiping seems we need to do this check. Overwise, we will gossip about already committed transactions)

I think txpool cleanup should be deferred to the block importer/sync task. Without that in place, it doesn't make as much sense to implement this yet.

fuel-txpool/src/service.rs

* cleaned up feature flags * fmt * fix leak: * fmt again

Voxelot

LGTM!

The service setup w/ channels looks painful, but luckily we have a plan to address this soon.

The PR in draft can come after this one.

ControlCplusControlV added 4 commits September 3, 2022 15:59

txpool reimpled

2dc5c35

Tests go green

3ab7525

clippy

3cc1ae2

final touches?

40e0384

ControlCplusControlV requested review from Voxelot, bvrooman and leviathanbeak September 4, 2022 00:51

ControlCplusControlV self-assigned this Sep 4, 2022

ControlCplusControlV added 2 commits September 3, 2022 19:20

clippy fix

beca66d

clippy maybe?

68e21fa

ControlCplusControlV marked this pull request as draft September 4, 2022 05:11

ControlCplusControlV and others added 10 commits September 6, 2022 13:08

Merge branch 'master' into controlc/p2p_tx

9b56bd8

Merge branch 'master' into controlc/p2p_tx

d866c11

updated fmt

716a3fa

ci passes?

e248f8c

Merge branch 'master' into controlc/p2p_tx

d8edc4f

Merge branch 'master' into controlc/p2p_tx

bf32dad

Merge branch 'master' into controlc/p2p_tx

cd62488

Error when tx_receiver channel sender is dropped

3c3f514

Updated fmt

e9b8cc4

Spelling

06d0e6b

bvrooman self-assigned this Sep 22, 2022

Update modules channel wiring

5a2e7a6

Merge branch 'master' into controlc/p2p_tx

afaa697

Keep alive for incoming_tx_sender

386e073

bvrooman added 9 commits September 29, 2022 13:42

Remove readme changes

d4e2fac

Minor cleanup

df2fe70

Minor cleanup

fe89f69

Add all gossip topics to default p2p config

cb7fcda

Fix

f63683a

Merge branch 'master' into controlc/p2p_tx

9b79b61

Clean up txpool cargo

4a2346c

Merge branch 'controlc/p2p_tx' of https://github.com/FuelLabs/fuel-core…

e1da109

… into controlc/p2p_tx

Remove debug statements

be1a152

bvrooman reviewed Sep 29, 2022

View reviewed changes

ControlCplusControlV marked this pull request as ready for review September 29, 2022 22:31

ControlCplusControlV requested review from leviathanbeak, Voxelot, bvrooman and a team September 29, 2022 22:31

ControlCplusControlV added the fuel-txpool label Sep 29, 2022

xgreenx reviewed Sep 30, 2022

View reviewed changes

Merge branch 'master' into controlc/p2p_tx

b9a7812

Voxelot reviewed Sep 30, 2022

View reviewed changes

fuel-txpool/src/service.rs Outdated Show resolved Hide resolved

Merge branch 'master' into controlc/p2p_tx

427c4d8

ControlCplusControlV mentioned this pull request Oct 2, 2022

chore: Update txpool README.md #654

Closed

Remove Feature Flags from Tx Gossiping (#664)

c37a8c6

* cleaned up feature flags * fmt * fix leak: * fmt again

ControlCplusControlV requested review from xgreenx and Voxelot October 2, 2022 21:52

bvrooman mentioned this pull request Oct 3, 2022

refactor: Broadcast tx insert to p2p #670

Merged

Voxelot approved these changes Oct 4, 2022

View reviewed changes

ControlCplusControlV merged commit c6ea9dd into master Oct 4, 2022

ControlCplusControlV deleted the controlc/p2p_tx branch October 4, 2022 03:26

leviathanbeak mentioned this pull request Oct 5, 2022

update libp2p to v0.48 #661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect P2p with TxPool #590

Connect P2p with TxPool #590

ControlCplusControlV commented Sep 4, 2022

ControlCplusControlV commented Sep 21, 2022 •

edited

Loading

bvrooman commented Sep 23, 2022 •

edited

Loading

ControlCplusControlV commented Sep 23, 2022

Voxelot commented Sep 23, 2022

bvrooman commented Sep 23, 2022 •

edited

Loading

bvrooman Sep 29, 2022

bvrooman Sep 29, 2022 •

edited

Loading

xgreenx left a comment

xgreenx Sep 30, 2022

Voxelot Sep 30, 2022

bvrooman Sep 30, 2022 •

edited

Loading

ControlCplusControlV Sep 30, 2022

bvrooman Oct 3, 2022

xgreenx Sep 30, 2022

Voxelot Sep 30, 2022 •

edited

Loading

Voxelot Sep 30, 2022

bvrooman Sep 30, 2022

xgreenx Sep 30, 2022

Voxelot Sep 30, 2022 •

edited

Loading

xgreenx Sep 30, 2022

Voxelot Sep 30, 2022 •

edited

Loading

Voxelot left a comment

Connect P2p with TxPool #590

Connect P2p with TxPool #590

Conversation

ControlCplusControlV commented Sep 4, 2022

ControlCplusControlV commented Sep 21, 2022 • edited Loading

bvrooman commented Sep 23, 2022 • edited Loading

ControlCplusControlV commented Sep 23, 2022

Voxelot commented Sep 23, 2022

bvrooman commented Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

bvrooman Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvrooman Sep 30, 2022 • edited Loading

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

Voxelot Sep 30, 2022 • edited Loading

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

Voxelot Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Voxelot left a comment

Choose a reason for hiding this comment

ControlCplusControlV commented Sep 21, 2022 •

edited

Loading

bvrooman commented Sep 23, 2022 •

edited

Loading

bvrooman commented Sep 23, 2022 •

edited

Loading

bvrooman Sep 29, 2022 •

edited

Loading

bvrooman Sep 30, 2022 •

edited

Loading

Voxelot Sep 30, 2022 •

edited

Loading

Voxelot Sep 30, 2022 •

edited

Loading

Voxelot Sep 30, 2022 •

edited

Loading