Add a way to fetch transactions in P2P without specifying a peer #2376

AurelienFT · 2024-10-21T12:39:53Z

Linked Issues/PRs

This is a requirement for #2361

Description

This PR adds a way to fetch transactions with p2p but without giving a specific peer and let p2p choose the one they prefer.
This will be used in #2361

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-10-21T16:32:29Z

crates/services/p2p/src/service.rs

+                        let peer = self.p2p_service.get_peer_id_with_height(&height);
+                        if self.p2p_service.send_request_msg(peer, request_msg, channel).is_err() {
+                            tracing::warn!("No peers found for block at height {:?}", block_height_range.end);
+                        }


If peer is None, you need return an error instead of printing the log.

Done. I followed the pattern from the headers should I change the headers one ?

xgreenx · 2024-10-21T16:33:00Z

crates/services/p2p/src/service.rs

+        block_height_range: Range<u32>,
+        channel: OnResponse<Option<Vec<Transactions>>>,
+    },
+    GetTransactionsFromPeer {


I think we can remove this variant

I think it's useful in case of syncing when you already received the header from someone in the current syncing process and so you can directly ask him without having to redo peer selection. Maybe there is also othhers use cases but this is the only one I see.

It seems that after your PR we don't need this variant anymore. But we can address it in your PR

Mhhh yeah let's discuss it in the other PR because IMO what I have written above still works in certain cases.

crates/services/p2p/src/service.rs

acerone85

LGTM

xgreenx · 2024-10-29T17:27:30Z

crates/services/p2p/src/service.rs

+        block_height_range: Range<u32>,
+        channel: OnResponse<Option<Vec<Transactions>>>,
+    },
+    GetTransactionsFromPeer {


It seems that after your PR we don't need this variant anymore. But we can address it in your PR

xgreenx · 2024-10-29T17:29:28Z

crates/services/p2p/src/service.rs

+                        let request_msg = RequestMessage::Transactions(block_height_range.clone());
+                        let height = BlockHeight::from(block_height_range.end.saturating_sub(1));
+                        let peer = self.p2p_service.get_peer_id_with_height(&height);
+                        if self.p2p_service.send_request_msg(peer, request_msg, channel).is_err() {


We should return error to the channel if peer is None, to allow proper handling of it by the caller.

I can change it, ok. However, I want to ask if we want the same in GetSealedHeaders (just above in the code) ? It has the same behavior and doesn't return the error for now.

Yep, good catch, let's fix it as well

Done. It complexify a bit the type in the channel because we have a two-stage error type but with unification of return type etc that we want to do in p2p maybe we could improve this in the future.

rymnc · 2024-10-30T08:35:13Z

crates/services/p2p/src/service.rs

+        self.request_sender
+            .send(TaskRequest::GetTransactions {
+                block_height_range: range,
+                channel: sender,
+            })
+            .await?;


perhaps we should continue to use the TaskRequest::GetTransactionsFromPeer and perform peer selection for a given block height instead -

fuel-core/crates/services/p2p/src/service.rs

Lines 264 to 266 in f597ac9

fn get_peer_id_with_height(&self, height: &BlockHeight) -> Option<PeerId> {

self.peer_manager().get_peer_id_with_height(height)

}

I personally think that there is still use case where it's interesting to already give a peer that we know have the information, we use this un sync service.

right, i agree ~ but this information is also valid for a peer returned by get_peer_id_with_height. perhaps we should not duplicate valid peers for a given height then.

I don't get your comment. My flow is that I ask for an header and so the peer is returned along with the header then I can directly ask to this peer about the transactions because in our network if you have the header we assume you have the transactions. It avoid running this get_peer_id_with_height function that can be costly.
Maybe I miss your point and in this case we can go over more voice discussion :)

perhaps we should continue to use the TaskRequest::GetTransactionsFromPeer and perform peer selection for a given block height instead

If we do peer selection outside of the run loop, we can face race condition were peer is disconnected already

Add a way to fetch transactions without specifying a peer in P2P

42719bf

AurelienFT changed the title ~~Add a way to fetch transactions without specifying a peer in P2P~~ Add a way to fetch transactions in P2P without specifying a peer Oct 21, 2024

Update CANGELOG.md

c80f3ac

AurelienFT marked this pull request as ready for review October 21, 2024 12:41

AurelienFT requested review from xgreenx, Dentosal and MitchTurner as code owners October 21, 2024 12:41

xgreenx reviewed Oct 21, 2024

View reviewed changes

acerone85 reviewed Oct 21, 2024

View reviewed changes

crates/services/p2p/src/service.rs Outdated Show resolved Hide resolved

Cahnge print ot err in case no peer.

130cb86

AurelienFT requested a review from xgreenx October 22, 2024 07:56

acerone85 previously approved these changes Oct 22, 2024

View reviewed changes

Merge branch 'master' into add_p2p_fetch_txs_no_peer_specified

851f9d8

AurelienFT dismissed acerone85’s stale review via 851f9d8 October 23, 2024 10:09

Dentosal previously approved these changes Oct 23, 2024

View reviewed changes

AurelienFT requested a review from acerone85 October 23, 2024 13:30

Merge branch 'master' into add_p2p_fetch_txs_no_peer_specified

be68c8a

AurelienFT dismissed Dentosal’s stale review via be68c8a October 28, 2024 12:06

AurelienFT self-assigned this Oct 28, 2024

xgreenx reviewed Oct 29, 2024

View reviewed changes

rymnc reviewed Oct 30, 2024

View reviewed changes

Add returned errors in channel when peer selection failed.

024c33f

AurelienFT requested a review from xgreenx October 30, 2024 09:43

Merge branch 'master' into add_p2p_fetch_txs_no_peer_specified

60db84f

xgreenx approved these changes Oct 30, 2024

View reviewed changes

Merge branch 'master' into add_p2p_fetch_txs_no_peer_specified

4be029e

AurelienFT requested a review from rymnc October 31, 2024 00:21

rymnc approved these changes Oct 31, 2024

View reviewed changes

AurelienFT merged commit 32b29a3 into master Oct 31, 2024
38 checks passed

AurelienFT deleted the add_p2p_fetch_txs_no_peer_specified branch October 31, 2024 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to fetch transactions in P2P without specifying a peer #2376

Add a way to fetch transactions in P2P without specifying a peer #2376

AurelienFT commented Oct 21, 2024

xgreenx Oct 21, 2024

AurelienFT Oct 22, 2024

xgreenx Oct 21, 2024

AurelienFT Oct 21, 2024

xgreenx Oct 29, 2024

AurelienFT Oct 29, 2024

acerone85 left a comment

xgreenx Oct 29, 2024

xgreenx Oct 29, 2024

AurelienFT Oct 29, 2024 •

edited

Loading

xgreenx Oct 29, 2024

AurelienFT Oct 30, 2024

rymnc Oct 30, 2024

AurelienFT Oct 30, 2024

rymnc Oct 30, 2024

AurelienFT Oct 30, 2024

xgreenx Oct 30, 2024

	fn get_peer_id_with_height(&self, height: &BlockHeight) -> Option<PeerId> {
	self.peer_manager().get_peer_id_with_height(height)
	}

Add a way to fetch transactions in P2P without specifying a peer #2376

Add a way to fetch transactions in P2P without specifying a peer #2376

Conversation

AurelienFT commented Oct 21, 2024

Linked Issues/PRs

Description

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

acerone85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurelienFT Oct 29, 2024 • 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

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 Oct 29, 2024 •

edited

Loading