Include p2p into fuel core #492

leviathanbeak · 2022-07-15T12:38:33Z

Resolves #480

I have included both things in a single PR since once I included p2p into fuel-core and added p2p field to config the cli part of the codebase was complaining.

For clarity, there is:

P2pService
NetworkOrchestrator which contains P2pService
Service which wraps the NetworkOrchestrator (I have named it Service to keep in line with other modules that had top level Service)

Creating the top level `Service`

I have went and implemented a similar pattern for the network as our other Services have done, by adding start() and stop() methods.

I have achieved that by wrapping the NetworkOrchestrator into a Service but I had to change some things:

P2P Service and Network needed to shut down

Unlike some other simpler Services, once network Service was stopped, we needed to shut down the whole p2p network.
I have achieved that by moving p2p_service from a NetworkOrchestrator field into a simple variable within run() method, so once run() ended (ie stop() was called) the p2p_service would go out of scope and get removed completely.

And if start() was called again, a new p2p_service would be instantiated - I had to keep the p2p_config within the NetworkOrchestrator for this case.

FuturesUnordered and their blocking of the executor

While testing out start() and stop() I have discovered a bug in our NetworkOrchestrator.

It's field outbound_responses which was of type

type ResponseFuture = Pin<Box<dyn Future<Output = Option<(OutboundResponse, RequestId)>>>>;
FuturesUnordered<ResponseFuture>

once called next() on it, it would return None so in our loop { tokio::select!... pattern it was constantly returning None
without letting any other items to be polled, basically it was blocking the executor. I discovered this while adding tokio's sleep within a test, and it never got to finish! So I have changed this part of the code slightly by using tokio's channels instead of FuturesUnordered.

There were few other changes and clean ups.

leviathanbeak · 2022-07-15T15:16:42Z

@rakita I have just passed some of the p2p channels into other services and removed the placeholders, please review and let me know if they are correct

fuel-core/src/cli/run/p2p.rs

fuel-core/src/service/modules.rs

Voxelot · 2022-07-20T01:31:04Z

fuel-core/src/cli/run/p2p.rs

+            match args.keypair {
+                Some(path) => {
+                    let mut bytes = std::fs::read(path)?;
+                    Keypair::secp256k1_from_der(&mut bytes)?


Should we also include a cli command for randomly generating libp2p keyfiles? Otherwise it will be hard to configure a multinode deployment if the keys are generated on the fly, since we won't know the bootnode peer id in advance.

Also this might be something we want to check with @digorithm about standardizing keyfile formats to use mnemonics instead of binary serialization. If we had a standard mnemonic format it would be a lot easier for users to setup their peering keys without needing additional keygen tools or cli commands.

Yeah, the SDK and fuel-crypto have some primitives around keygen/mnemonics. Maybe we should use those here.

SDK documentation for wallets/mnemonics: https://fuellabs.github.io/fuels-rs/v0.19.0/wallets/mnemonic-wallet.html

fuel-crypto documentation for SecretKey generation: https://docs.rs/fuels/latest/fuels/signers/fuel_crypto/struct.SecretKey.html

Seems like we should migrate some of the mnemonics logic to fuel-crypto, to avoid requiring the whole SDK for key parsing.

Makes sense to me.

great, should we then create a separate ticket for migrating that logic to fuel-crypto and another for incorporating it in a new cli command ?

@Voxelot already created one for migrating the logic to fuel-crypto: FuelLabs/fuel-crypto#27

I guess we need another one for the latter as you said!

fuel-core/src/cli/run/p2p.rs

fuel-p2p/src/orchestrator.rs

fuel-core/src/service/modules.rs

fuel-p2p/src/orchestrator.rs

…beak/include_p2p_into_fuel_core

ControlCplusControlV · 2022-08-13T20:47:47Z

Sorry for re-running clippy showing the rust 1.63 errors, I needed a sanity check against my upstream branch since I wasn't sure if clippy changes had been moved to this branch yet. Once you can finish merge conflicts @leviathanbeak I can re-review and then we can get this moved as git history is starting to get very long

…beak/include_p2p_into_fuel_core

fuel-core/src/cli/run.rs

fuel-core/src/cli/run/p2p.rs

ControlCplusControlV

LGTM, nothing block on my end, just some questions.

Voxelot · 2022-08-17T00:46:17Z

I'm starting to get concerned with the weight of all the extra dependencies being linked to fuel-core. Can we put the service under a feature flag, so that libp2p is not compiled or included when --no-default-features is used when building fuel-core? We should also do this for the relayer.

Since the SDK uses fuel-core in it's test harness, it would be much more efficient if contract testing didn't load in big deps like ethers, libp2p and rocksdb.

Voxelot · 2022-08-17T00:49:55Z

fuel-core/src/cli/run/p2p.rs

+                    let mut bytes = std::fs::read(path)?;
+                    Keypair::secp256k1_from_der(&mut bytes)?


This should use the new fuel-crypto mnemonics to load/generate the secret key.

I switched to using fuel-crypto for both loading a mnemonic and generating a random key,

Since libp2p's Keypair struct was needed for p2pConfig I have added a function to convert from fuel-crypto's SecretKey into libp2p's SecretKey -> Keypair

leviathanbeak · 2022-08-18T19:58:14Z

ok, now the last thing is to added the service into optional features

leviathanbeak · 2022-08-22T11:19:49Z

added p2p optional feature to fuel-core

fuel-core/Cargo.toml

ControlCplusControlV

leviathanbeak added 6 commits July 15, 2022 09:08

add network service

56763da

reuse config method

f049814

add local_keypair to p2p_config

7e5ecbf

add network service to fuel-core

5f47f0c

add p2p args

e731966

move channels out of service initialization

1d1f0fd

leviathanbeak requested review from rakita and Voxelot July 15, 2022 12:47

leviathanbeak self-assigned this Jul 15, 2022

leviathanbeak added fuel-core fuel-p2p labels Jul 15, 2022

add p2p channels to other modules/services

f9f1d17

Voxelot reviewed Jul 20, 2022

View reviewed changes

fuel-core/src/cli/run/p2p.rs Outdated Show resolved Hide resolved

Voxelot reviewed Jul 20, 2022

View reviewed changes

fuel-core/src/service/modules.rs Outdated Show resolved Hide resolved

Voxelot reviewed Jul 20, 2022

View reviewed changes

fuel-core/src/cli/run/p2p.rs Outdated Show resolved Hide resolved

Voxelot reviewed Jul 20, 2022

View reviewed changes

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

ControlCplusControlV reviewed Jul 28, 2022

View reviewed changes

fuel-core/src/service/modules.rs Show resolved Hide resolved

ControlCplusControlV reviewed Jul 28, 2022

View reviewed changes

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

ControlCplusControlV and others added 5 commits July 28, 2022 15:37

Merge branch 'master' into leviathanbeak/include_p2p_into_fuel_core

c4d2963

fmt is hard in the online editor

49677f9

ooops

9cbc347

Sorry for ruining commit history

f29f0b5

oops

d88d81a

bvrooman mentioned this pull request Jul 28, 2022

Controlc/tx p2p #491

Merged

Voxelot removed the request for review from rakita August 2, 2022 00:40

leviathanbeak added 3 commits August 2, 2022 17:41

pass bootstrap nodes in a vector instead

fe7d5fb

remove box from db trait object

156e2d6

update peering port

b8d08da

leviathanbeak added 2 commits August 11, 2022 13:53

add some comments

df7aeee

Merge branch 'master' of github.com:FuelLabs/fuel-core into leviathan…

25e9f83

…beak/include_p2p_into_fuel_core

leviathanbeak mentioned this pull request Aug 11, 2022

Unify names of our mpsc channels #556

Closed

leviathanbeak added 2 commits August 15, 2022 15:59

Merge branch 'master' of github.com:FuelLabs/fuel-core into leviathan…

b6cc049

…beak/include_p2p_into_fuel_core

clean up

8d817bd

leviathanbeak requested review from Voxelot, bvrooman and ControlCplusControlV August 16, 2022 14:29

Merge branch 'master' into leviathanbeak/include_p2p_into_fuel_core

bb940b6

ControlCplusControlV reviewed Aug 17, 2022

View reviewed changes

fuel-core/src/cli/run.rs Show resolved Hide resolved

ControlCplusControlV reviewed Aug 17, 2022

View reviewed changes

fuel-core/src/cli/run/p2p.rs Outdated Show resolved Hide resolved

ControlCplusControlV previously approved these changes Aug 17, 2022

View reviewed changes

Voxelot reviewed Aug 17, 2022

View reviewed changes

leviathanbeak added 2 commits August 17, 2022 17:37

clean extra space

9d8e16c

set default values within clap

418ed65

leviathanbeak dismissed ControlCplusControlV’s stale review via 418ed65 August 17, 2022 16:01

use fuel-crypto for key generation

53581e5

leviathanbeak added 2 commits August 22, 2022 13:09

add p2p optional feature

06e0daf

Merge branch 'master' into leviathanbeak/include_p2p_into_fuel_core

3fe6d82

Voxelot reviewed Aug 23, 2022

View reviewed changes

fuel-core/Cargo.toml Outdated Show resolved Hide resolved

Voxelot added 2 commits August 23, 2022 07:11

Update fuel-core/Cargo.toml

cae141c

Merge branch 'master' into leviathanbeak/include_p2p_into_fuel_core

ac83774

Voxelot approved these changes Aug 23, 2022

View reviewed changes

ControlCplusControlV approved these changes Aug 23, 2022

View reviewed changes

leviathanbeak merged commit e50553a into master Aug 24, 2022

leviathanbeak deleted the leviathanbeak/include_p2p_into_fuel_core branch August 24, 2022 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include p2p into fuel core #492

Include p2p into fuel core #492

leviathanbeak commented Jul 15, 2022

leviathanbeak commented Jul 15, 2022

Voxelot Jul 20, 2022 •

edited

Loading

digorithm Aug 2, 2022

Voxelot Aug 2, 2022

digorithm Aug 2, 2022

leviathanbeak Aug 2, 2022

digorithm Aug 2, 2022 •

edited

Loading

ControlCplusControlV commented Aug 13, 2022

ControlCplusControlV left a comment •

edited

Loading

Voxelot commented Aug 17, 2022 •

edited

Loading

Voxelot Aug 17, 2022

leviathanbeak Aug 18, 2022 •

edited

Loading

leviathanbeak commented Aug 18, 2022

leviathanbeak commented Aug 22, 2022

ControlCplusControlV left a comment

		let mut bytes = std::fs::read(path)?;
		Keypair::secp256k1_from_der(&mut bytes)?

Include p2p into fuel core #492

Include p2p into fuel core #492

Conversation

leviathanbeak commented Jul 15, 2022

Creating the top level Service

P2P Service and Network needed to shut down

FuturesUnordered and their blocking of the executor

leviathanbeak commented Jul 15, 2022

Voxelot Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

digorithm Aug 2, 2022

Choose a reason for hiding this comment

Voxelot Aug 2, 2022

Choose a reason for hiding this comment

digorithm Aug 2, 2022

Choose a reason for hiding this comment

leviathanbeak Aug 2, 2022

Choose a reason for hiding this comment

digorithm Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

ControlCplusControlV commented Aug 13, 2022

ControlCplusControlV left a comment • edited Loading

Choose a reason for hiding this comment

Voxelot commented Aug 17, 2022 • edited Loading

Voxelot Aug 17, 2022

Choose a reason for hiding this comment

leviathanbeak Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

leviathanbeak commented Aug 18, 2022

leviathanbeak commented Aug 22, 2022

ControlCplusControlV left a comment

Choose a reason for hiding this comment

Creating the top level `Service`

Voxelot Jul 20, 2022 •

edited

Loading

digorithm Aug 2, 2022 •

edited

Loading

ControlCplusControlV left a comment •

edited

Loading

Voxelot commented Aug 17, 2022 •

edited

Loading

leviathanbeak Aug 18, 2022 •

edited

Loading