Network Orchestrator service proposal #402

leviathanbeak · 2022-06-10T17:22:58Z

Network Orchestrator is the service that:

holds and runs p2p service
holds multiple tokio::sync::mpsc::Sender values for propagating p2p messages to different components
holds and listens from tokio::sync::mpsc::Receiver to get messages from different fuel-core components

This initial version is (loosely) based on miro design.

Apart from general design I would need feedback on specific components

fuel-p2p/src/orchestrator.rs

vlopes11 · 2022-06-13T12:20:43Z

fuel-p2p/src/orchestrator.rs

+    pub async fn run(&mut self) {
+        loop {
+            tokio::select! {
+                p2p_event = self.p2p_service.next_event() => {


instead of nesting all these match patterns, maybe create a single level identity who will check the cases?

fuel-p2p/src/orchestrator.rs

…ator API

leviathanbeak · 2022-06-29T20:10:25Z

Most of the reasoning for these changes can be found at: https://fuellabs.slack.com/archives/C01L1HUJSL8/p1656343694067609

It could be argued that it's a bit over-engineered but on the flip side we do not .clone() any values from Arc<T> that we receive/send so it is definitely cleaner and more performant in that regard.

rakita

tbh it is hard to do indepth review without knowing libp2p lib. Reviewing the code it looked good so I am okay to approve it.
I think a few things will pop out when starting to integrate p2p with the rest of the system. In wiki It was hard to specify everything in one go here so i see there is inconsistencies as block producer broadcasts block (it is used by multiple parties) but p2p has mpsc that wants to receive it.

rakita · 2022-07-06T08:14:59Z

fuel-p2p/src/orchestrator.rs

+
+    db: Arc<dyn RelayerDb>,
+
+    outbound_responses: FuturesUnordered<ResponseFuture>,


Didn't know about this, nice :)

rakita · 2022-07-06T08:16:15Z

fuel-p2p/src/orchestrator.rs

+        tx_transaction: Sender<TransactionBroadcast>,
+        tx_block: Sender<BlockBroadcast>,
+
+        db: Arc<dyn RelayerDb>,


[nit] You can define yourown db trait in fuel-core-interface and implement it inside fuel-core. Leave if for future work

Voxelot · 2022-07-06T20:35:44Z

fuel-core-interfaces/src/p2p.rs

@@ -16,15 +17,18 @@ pub enum BlockBroadcast {
    NewBlock(FuelBlock),
 }

-pub enum P2pMpsc {
+pub enum P2PRequestEvent {


[nit] Camel-casing applies to acronyms as well (P2P -> P2p):

In UpperCamelCase, acronyms and contractions of compound words count as one word: use Uuid rather than UUID, Usize rather than USize or Stdin rather than StdIn
https://rust-lang.github.io/api-guidelines/naming.html

Suggested change

pub enum P2PRequestEvent {

pub enum P2pRequestEvent {

good point, but "P2p" feels so wrong 😬

Voxelot · 2022-07-06T20:44:29Z

fuel-p2p/src/behavior.rs

-        } else {
-            debug!("ResponseChannel for {:?} does not exist!", request_id);
-            return Err(ResponseError::ResponseChannelDoesNotExist);
+            _ => {}


Should we log anything in the event of an error?

Suggested change

_ => {}

(Err(e), _) => { debug!("unexpected error occurred while processing message response: {:?}", e); }

logged it, and added another error enum variant

…fuel-core into leviathanbeak/p2p_orchestrator

Voxelot · 2022-07-08T00:03:32Z

fuel-p2p/src/orchestrator.rs

+
+

normally cargo fmt would trim this, but don't think it works inside of select! macros.

Suggested change

yes select! support is terrible

Voxelot · 2022-07-08T00:13:49Z

fuel-p2p/src/orchestrator.rs

+
+                                        self.outbound_responses.push(
+                                            Box::pin(async move {
+                                                db.get_sealed_block(block_height).await.map(|block| (OutboundResponse::ResponseBlock(block), request_id))


It may make sense in the future to have some other service/module handle fetching blocks from the DB for the orchestrator. For instance, if there are many outstanding requests (e.g. lots of full-syncs) having a dedicated block-fetching worker on the spawn_blocking threadpool would help avoid stalling the main async runtime.

since the fetching itself is already async, shouldn't it be just fine to spawn another async task with tokio::spawn instead ?

good point, it should be the responsibility of this async-trait implementor to deal with any potential blocking issues. A regular spawn doesn't really net us anything here since that's more for launching background tasks that don't have an active poller.

Voxelot · 2022-07-08T00:26:53Z

fuel-p2p/src/gossipsub/topics.rs

+
+    /// Given a `GossipsubBroadcastRequest` retruns a `GossipTopic`
+    /// which is broadcast over the network with the serialized inner value of `GossipsubBroadcastRequest`
+    pub fn get_gossipsub_topic(&self, outgoing_request: &GossipsubBroadcastRequest) -> GossipTopic {


Do we have tests to verify all of these lookups and topic encodings are accurate? Since topic setup relies on manual string formatting and setup, it will be harder for the compiler to detect errors during refactoring etc.

good observation, added a test that checks all the options

Voxelot · 2022-07-08T00:30:17Z

fuel-p2p/src/service.rs

@@ -412,8 +416,8 @@ mod tests {
                            // verifies that we've got at least a single peer address to send message to
                            if !peer_addresses.is_empty() && !message_sent  {
                                message_sent = true;
-                                let default_tx = FuelGossipsubMessage::NewTx(Transaction::default());
-                                node_a.publish_message(selected_topic.clone(), default_tx).unwrap();
+                                let default_tx = GossipsubBroadcastRequest::NewTx(Arc::new(Transaction::default()));


We should also have tests that ensure message routing and serialization work as expected across all network event types we have. Currently, it seems like we only exercise the NewTx and RequestBlock code paths in our tests.

added tests for all Gossipsub Messages, as for Request-Response, currently we only support RequestBlock anyway

Voxelot · 2022-07-08T00:31:11Z

ready to approve once test coverage for new message types is improved.

leviathanbeak · 2022-07-08T14:13:21Z

did some additional cleaning:

removed unwrap() when initializing the Orchestrator::new()
removed some unused methods
only made public the Orchestrator and what is needed for it to start

Voxelot · 2022-07-08T18:44:40Z

fuel-p2p/src/service.rs


+    #[tokio::test]
+    #[instrument]
+    async fn gossipsub_broadcast_vote() {


I like these granular clean looking tests

Voxelot · 2022-07-08T18:45:21Z

LGTM! Good improvements with the test coverage

leviathanbeak commented Jun 10, 2022

View reviewed changes

fuel-p2p/src/orchestrator.rs Outdated Show resolved Hide resolved

leviathanbeak commented Jun 10, 2022

View reviewed changes

fuel-p2p/src/orchestrator.rs Show resolved Hide resolved

leviathanbeak commented Jun 10, 2022

View reviewed changes

fuel-p2p/src/orchestrator.rs Outdated Show resolved Hide resolved

leviathanbeak commented Jun 10, 2022

View reviewed changes

fuel-p2p/src/orchestrator.rs Outdated Show resolved Hide resolved

adlerjohn assigned leviathanbeak Jun 11, 2022

adlerjohn added the enhancement New feature or request label Jun 11, 2022

vlopes11 reviewed Jun 13, 2022

View reviewed changes

leviathanbeak added 18 commits June 24, 2022 17:47

fix Utc serialization with ts_seconds

7565f81

remove request message

c68aaf2

add orchestrator example

db6dfad

remove ts_seconds

eebf960

update p2pmessages

40b07a1

update RequestResponse and Gossipsub messages logic to match orchestr…

2862ceb

…ator API

handle gossipsub and RequestBlock messages

f8d6eaf

implement intermediate response

7eb5a7b

fix Utc serialization with ts_seconds

1976954

remove request message

2c88db2

add orchestrator example

96ad318

remove ts_seconds

e20a097

update p2pmessages

f453acc

update RequestResponse and Gossipsub messages logic to match orchestr…

c1ee6da

…ator API

handle gossipsub and RequestBlock messages

1736feb

fix imports

9fe7a9c

Merge branch 'once_again' into leviathanbeak/p2p_orchestrator

91f33cf

remove merge conflict

ad0577a

leviathanbeak force-pushed the leviathanbeak/p2p_orchestrator branch from 4e6a3bb to ad0577a Compare June 29, 2022 19:57

cleanup

7cab2c6

leviathanbeak requested review from rakita and Voxelot June 29, 2022 20:19

leviathanbeak requested a review from vlopes11 June 29, 2022 20:19

Merge branch 'master' into leviathanbeak/p2p_orchestrator

427e07c

leviathanbeak marked this pull request as ready for review July 1, 2022 19:28

rakita previously approved these changes Jul 6, 2022

View reviewed changes

Voxelot reviewed Jul 6, 2022

View reviewed changes

leviathanbeak added 3 commits July 7, 2022 16:42

rename to lower 'p' in P2p

c98fdd8

handle conversion failing

81563f0

Merge branch 'leviathanbeak/p2p_orchestrator' of github.com:FuelLabs/…

45071ee

…fuel-core into leviathanbeak/p2p_orchestrator

leviathanbeak dismissed rakita’s stale review via 45071ee July 7, 2022 17:56

Voxelot reviewed Jul 8, 2022

View reviewed changes

leviathanbeak added 8 commits July 8, 2022 12:41

add test for GossipsubTopics

5634aa1

rename to matching

fb6eb18

test all gossipsub requests

5153ae0

fix type to Consensus

15df643

propagate the error

5d9e789

cleanup space

80d6a74

export correct stuctures

f5ac575

remove unused methods from behavior and service

5cec3f0

Voxelot reviewed Jul 8, 2022

View reviewed changes

fuel-p2p/src/service.rs

#[tokio::test]

#[instrument]

async fn gossipsub_broadcast_vote() {

Copy link

Member

Voxelot Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these granular clean looking tests

Voxelot approved these changes Jul 8, 2022

View reviewed changes

Merge branch 'master' into leviathanbeak/p2p_orchestrator

2bb87dd

leviathanbeak merged commit 6ea571e into master Jul 8, 2022

leviathanbeak deleted the leviathanbeak/p2p_orchestrator branch July 8, 2022 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network Orchestrator service proposal #402

Network Orchestrator service proposal #402

leviathanbeak commented Jun 10, 2022

vlopes11 Jun 13, 2022

leviathanbeak commented Jun 29, 2022

rakita left a comment

rakita Jul 6, 2022

rakita Jul 6, 2022

Voxelot Jul 6, 2022 •

edited

Loading

leviathanbeak Jul 7, 2022

Voxelot Jul 6, 2022 •

edited

Loading

leviathanbeak Jul 7, 2022

Voxelot Jul 8, 2022

leviathanbeak Jul 8, 2022

Voxelot Jul 8, 2022 •

edited

Loading

leviathanbeak Jul 8, 2022

Voxelot Jul 8, 2022 •

edited

Loading

Voxelot Jul 8, 2022

leviathanbeak Jul 8, 2022

Voxelot Jul 8, 2022

leviathanbeak Jul 8, 2022

Voxelot commented Jul 8, 2022

leviathanbeak commented Jul 8, 2022 •

edited

Loading

Voxelot Jul 8, 2022

Voxelot commented Jul 8, 2022


		db: Arc<dyn RelayerDb>,

		outbound_responses: FuturesUnordered<ResponseFuture>,

	_ => {}
	(Err(e), _) => { debug!("unexpected error occurred while processing message response: {:?}", e); }

Network Orchestrator service proposal #402

Network Orchestrator service proposal #402

Conversation

leviathanbeak commented Jun 10, 2022

Choose a reason for hiding this comment

leviathanbeak commented Jun 29, 2022

rakita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Jul 6, 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 Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Jul 8, 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

Choose a reason for hiding this comment

Voxelot commented Jul 8, 2022

leviathanbeak commented Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Voxelot commented Jul 8, 2022

Voxelot Jul 6, 2022 •

edited

Loading

Voxelot Jul 6, 2022 •

edited

Loading

Voxelot Jul 8, 2022 •

edited

Loading

Voxelot Jul 8, 2022 •

edited

Loading

leviathanbeak commented Jul 8, 2022 •

edited

Loading