Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add libp2p service and configuration #282

Merged
merged 73 commits into from
Mar 13, 2023
Merged

Add libp2p service and configuration #282

merged 73 commits into from
Mar 13, 2023

Conversation

mycognosist
Copy link
Contributor

@mycognosist mycognosist commented Feb 28, 2023

Implements a minimal networking layer using the libp2p library, including transport establishment, networking configuration and network behaviour composition.

Relates to #278

To Do

  • Keypair configuration
    • Generate a new keypair when one doesn't exist
    • Persist the keypair to file using PEM format
    • Load the keypair from file
  • Define swarm configuration parameters
    • Set connection-related defaults
    • Decide on default QUIC port
  • Define the NetworkBehaviour of the node
    • ping
    • mDNS
  • Swarm
    • Configure and create QUIC transport
    • Listen for incoming connections
    • Dial peer addresses for replication (if provided)
    • Log events
  • Install Protocol Buffers compiler for CI workflows
  • Add Protocol Buffers compiler installation requirement note to README

Notes

aquadoggo currently uses envy to parse environment variables and overlay them onto the default configuration parameters. However, it does not currently handle nested structs. This means that it is not possible to tune libp2p configuration parameters given the current code structure (the same applies to ReplicationConfig). I will need to use the flatten attribute from serde (docs) to achieve this.

Out of Scope

I'm noting this down here for future iterations on the networking layer functionality.

  • Dummy replication (self-implemented behaviour for learning purposes)
    • Let's rather do this after the base networking functionality has been merged
  • Address book
    • Read from file
    • Add discovered peers (via mDNS, for example)
    • Write to file
  • TOML configuration file for the application
    • I'll tackle this in another PR

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@adzialocha
Copy link
Member

adzialocha commented Mar 8, 2023

@adzialocha

It fails because of a port conflict. mDNS uses port 5353. The recommended approach is to run one instance per machine (or to somehow isolate the instances on a single machine).

I see, thank you! Is this specified by libp2p? For multicast sockets it's usually an exception to re-bind to the same port via SO_REUSEADDR.

@mycognosist
Copy link
Contributor Author

@adzialocha

I see, thank you! Is this specified by libp2p? For multicast sockets it's usually an exception to re-bind to the same port via SO_REUSEADDR.

Aha! Excellent question! This helped me to fix some of my incorrect assumptions. Thank you!

Going back to your failed attempt to run two nodes on your local machine with mDNS: I was wrong. The cause of the panic was actually the QUIC port!

I dug into the libp2p code to look for the port reuse option and ended up finding it here (their mDNS docs are very sparse and do not mention this, or the port 5353). Then I tried to run two aquadoggo nodes on my local machine, one with mDNS enabled and one with it disabled. That still resulted in a panic. That's when I thought it might be the QUIC port conflict. I added a --quic-port CLI option to test my assumption and it was correct; I now have mDNS working when one node is using 2022 (QUIC default) and the other is on 2023.

Let me clean up the code and commit it.

P.s. I'm really sorry that this PR has become so big and complicated -_-

Copy link
Member

@adzialocha adzialocha left a comment

Choose a reason for hiding this comment

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

Looks great! ✨ Thank you for all the extra changes. I think it's good to go 👍🏻

Copy link
Member

@sandreae sandreae left a comment

Choose a reason for hiding this comment

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

This is a great PR and represents a huge amount of very thorough research, thanks so much for guiding and grounding this complex area of work. We have a network!!!!

Just some small comments in this review, i agreed with all the points @adzialocha already made. One extra thing is that I wonder if we need some more info in the README specifically about the network layer. This can happen in a follow-up PR and once we have the other pieces in place.

Also, maybe i'm doing something wrong, but should i be getting any chatter when running two nodes locally? Like mDNS or ping? I don't see anything in the logs when i run:

RUST_LOG=aquadoggo=debug cargo run

and then:

RUST_LOG=aquadoggo=debug cargo run -- --http-port 2021 --quic-port 2023

I'd just like to check this point and the small comments i made then I'm very happy to approve and merge.

aquadoggo/Cargo.toml Outdated Show resolved Hide resolved
aquadoggo/Cargo.toml Outdated Show resolved Hide resolved
aquadoggo/src/config.rs Show resolved Hide resolved
aquadoggo/src/network/service.rs Outdated Show resolved Hide resolved
@adzialocha
Copy link
Member

Just some small comments in this review, i agreed with all the points @adzialocha already made. One extra thing is that I wonder if we need some more info in the README specifically about the network layer. This can happen in a follow-up PR and once we have the other pieces in place.

Agreed that it will be good to write something in the README.md but maybe that's still too early? I would write something if we have a fully fledged out networking layer, but so far that's "only the beginning" 😆

@mycognosist
Copy link
Contributor Author

Agreed that it will be good to write something in the README.md but maybe that's still too early?

I'll put together a simple doc on Laub with details of what we have in the networking layer thus far. That can inform the README section at a later stage.

@mycognosist
Copy link
Contributor Author

@sandreae

Thanks so much for the review! 🖤 Loads of awesome teamwork and sharing has made this possible.

I have removed the noise feature (well spotted!) and have added a note about mDNS to the network service doc comment.

Also, maybe i'm doing something wrong, but should i be getting any chatter when running two nodes locally? Like mDNS or ping?

You need one small addition to your second invocation to specify a non-default base path. That results in a separate peer ID being created for the second node. The way you tried it before meant that both nodes were running with the same ID and therefore mDNS does not discover a new node.

RUST_LOG=aquadoggo=debug cargo run

RUST_LOG=aquadoggo=debug cargo run -- --http-port 2021 --quic-port 2023 --data-dir=$HOME/test

You should see something like this in the log output:

[2023-03-10T11:31:38Z DEBUG aquadoggo::network::service] mDNS discovered a new peer: 12D3KooWDC8Kmq9z9m4VSYZD68ioJYfxqR3DTKKfrjh7XMH9NXGZ

Note that the mDNS protocol handler will need to be updated in a future PR to act on peer discovery events. At this point, the discovery event is simply logged. We'll want to either invoke swarm.dial() for the discovered peer directly in the handler or add the peer to the address book and handle the dialing elsewhere.

@adzialocha
Copy link
Member

You need one small addition to your second invocation to specify a non-default base path. That results in a separate peer ID being created for the second node. The way you tried it before meant that both nodes were running with the same ID and therefore mDNS does not discover a new node.

That's a new pattern: Nodes have key pairs now 🤯

@sandreae sandreae self-requested a review March 13, 2023 08:48
@sandreae
Copy link
Member

Ah, thanks, yeh that did the trick! I see discovery happening now :-)

@sandreae
Copy link
Member

@mycognosist I had a look at doing the rebase again, but I'm not seeing any problem in my local git commit history, so hoping this is just something weird here in the web ui. Let's merge! You wanna do it? We normally squash.

@mycognosist mycognosist merged commit 0bc88f2 into main Mar 13, 2023
@adzialocha adzialocha deleted the libp2p branch March 14, 2023 15:56
adzialocha added a commit that referenced this pull request Mar 16, 2023
* main:
  Update breaking API calls for new `p2panda-rs` version (#293)
  Update Cargo.lock as well
  Use released p2panda-rs version 0.7.0
  Migrate CLI from `structopt` to `clap` (#289)
  Increase timeout for failing materializer test
  Introduce requeue flag (#286)
  Do transactions correctly (#285)
  Add libp2p service and configuration (#282)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants