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

examples: use circuitv2 in relay example #1795

Merged
merged 8 commits into from
Oct 9, 2022

Conversation

DanielVernall
Copy link
Contributor

Hi, I've never contributed to go-libp2p before and I'm new to the project having come back to it a few times. So, I thought I would start by updating examples/docs that I find need updating as I'm learning libp2p.

I couldn't find much documentation on the circuit-relay v2 implementation, but I have pieced together a sample from the source and tests I have found. The only thing I'm not certain of is the use of WithUseTransient(), as I can't find much info on this. Am I correctly using this configuration in the circuit relay stream creation, or should I be looking to upgrade the connection in some way prior to connecting via the relay?

@marten-seemann marten-seemann changed the title Updated relay example to use circuit-v2 examples: use circuitv2 in relay example Oct 2, 2022
@marten-seemann marten-seemann self-requested a review October 2, 2022 18:19
@DanielVernall
Copy link
Contributor Author

I renamed the h1 through h3 variables to unreachableN and relay1, just to make it really clear in the example for a newcomer which would be acting as which. But I understand you may prefer h1-3 by convention as h is almost always the host name, and because in reality a relay host isn't just a relay host, it's simply another node in the network.

Also, I'm not certain of go-libp2p's naming conventions for Multiaddr and AddrInfo, as I've seen quite a few variables named in all lowercase, eg. "relayaddr" instead of mixed caps, "relayAddr", so I stuck with all lower for some variables in here, but they were quite long (not very Go like, I'm afraid).

@p-shahi p-shahi added this to the Best Effort Track milestone Oct 3, 2022
@p-shahi p-shahi linked an issue Oct 3, 2022 that may be closed by this pull request
@p-shahi p-shahi added the P2 Medium: Good to have, but can wait until someone steps up label Oct 3, 2022

// Tell the host use relays
h1, err := libp2p.New(libp2p.EnableRelay())
// Create 2 "unreachable" libp2p hosts that want to communicate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Create 2 "unreachable" libp2p hosts that want to communicate.
// Create two "unreachable" libp2p hosts that want to communicate.


err = unreachable1.Connect(context.Background(), unreachable2info)
if err == nil {
log.Printf("This actually should have failed. Did you make changes to the example...?")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.Printf("This actually should have failed. Did you make changes to the example...?")
log.Printf("This actually should have failed.")

log.Println("Yep, that worked!")

// **** MIGHT BE WORTH ADDING A COMMENT ADDRESSING WHAT WithUseTransient() is here,
// and why it is needed ******************
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do that? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really confident with transient connections, as I'm new to libp2p 😅. From what I can piece together from reading some source comments is that transient stream are those that have opened a connection but the stream is not yet fully established, as a protocol hasn't been negotiated yet.

So, my guess would be that, in the context of a circuit relay, we have established an initial connection to the relay peer (which is marked as transient) but we don't have a connection to our destination peer yet, so when we open our stream to the destination peer we indicate that we are using a transient connection, but it is isn't a problem as the relay will connect to the destination peer and our connection will be upgraded once we've negotiated our stream with the destination peer. But it is possible I'm way off there 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can piece together from reading some source comments is that transient stream are those that have opened a connection but the stream is not yet fully established, as a protocol hasn't been negotiated yet.

Yeah, confusing terminology here. Sorry for that. That would be transient in the context of the resource manager. In this context, it means something completely different.

Transient here means that it's a connection established on a relayed (as opposed to a direct) connection. Since the relay limits the amount of data that can be exchanged over the relayed connection, application controls need to explicitly opt-in into using a relayed connection. In general, they should only do that if they have low bandwidth requirements, and they're ok with the connection being killed when the relayed connection is replaced with a direct (holepunched) connection.

I hope this explanation makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that's interesting, thanks for the details. Is there anything in the docs or source I could read to explore this a bit more? For example, how an application/relay node would go about lifting the relay data limits, or upgrading the connection for devices where hole punching is not possible.

@marten-seemann
Copy link
Contributor

Looks like this PR needs to be rebased.

@marten-seemann marten-seemann added the need/author-input Needs input from the original author label Oct 8, 2022
@DanielVernall DanielVernall reopened this Oct 9, 2022
examples/go.work Outdated
@@ -0,0 +1,3 @@
go 1.18
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thank you!

@marten-seemann marten-seemann removed the need/author-input Needs input from the original author label Oct 9, 2022
@marten-seemann marten-seemann merged commit 164ccd7 into libp2p:master Oct 9, 2022
julian88110 added a commit that referenced this pull request Oct 11, 2022
examples: use circuitv2 in relay example (#1795)

* Updated relay example to use circuit-v2

* fixed incorrect import

* updated test fixture

* upgrade dependencies

* merged upstream

* Implemented suggested changes in upstream PR

* Implemented suggested changes in upstream PR

roadmap: fix header level on "Mid Q4" (#1818)

examples: connect to all peers in example mdns chat app (#1798)

* Fixed bug in example mdns chat app.

* Added continue, so that if connection to other host fails, will go back to discovering.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Medium: Good to have, but can wait until someone steps up
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

examples: update relay example to use circuit-relay-v2
3 participants