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

split a blank host out of the basic host #1259

Closed
wants to merge 5 commits into from
Closed

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Dec 10, 2021

Fixes #1247.

As suggested by @Stebalien in #1257 (comment).

We probably need to work on naming here. Should we rename the basic package to full?

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

lets punt on this for now.

This was referenced Dec 14, 2021
@marten-seemann
Copy link
Contributor Author

lets punt on this for now

go-libp2p v0.17.0 is out now.

I'd really like to get this into v0.18.0. Not having duplicated code for our hosts is a prerequisite for further refactors.

@vyzo
Copy link
Contributor

vyzo commented Dec 20, 2021

yeah, lets do it; base -vs- basic is confusing. Lets also have a blank profile to replace the blankhost.

@vyzo
Copy link
Contributor

vyzo commented Dec 20, 2021

or is that not useful?

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

looking good, lets introduce the FullHost name.

@vyzo
Copy link
Contributor

vyzo commented Dec 20, 2021

I believ we want identify in the base host, the autonat dialer wont work without it.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

This actually seems pretty clean.

Although, now that i see it... it would be really nice if we just had a truly pluggable host.

p2p/host/base/base_host.go Outdated Show resolved Hide resolved
if !ok {
return errors.New("peerstore does not support signed records")
}
rec := peer.PeerRecordFromAddrInfo(peer.AddrInfo{ID: h.ID(), Addrs: h.Addrs()})
Copy link
Member

Choose a reason for hiding this comment

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

What happens if we don't do this? I'm asking because:

  1. We're going to clobber it in the Basic/Full host.
  2. The addresses are going to be wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien Can you elaborate? It would be nice if the base host supported signed records, wouldn't it?

The addresses are only going to wrong if an address factory is used.
Also, in the full host we generate a new record for ourselves, so we'll overwrite the one that the base host generated.

Copy link
Member

Choose a reason for hiding this comment

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

The addresses are only going to wrong if an address factory is used.

These records won't update to changing interface or listen addresses.

As a matter of fact, we're unlikely to even be listening on any addresses by the time we run this code. Normally, we construct the host first:

go-libp2p/config/config.go

Lines 215 to 226 in 4400328

h, err := bhost.NewHost(swrm, &bhost.HostOpts{
ConnManager: cfg.ConnManager,
AddrsFactory: cfg.AddrsFactory,
NATManager: cfg.NATManager,
EnablePing: !cfg.DisablePing,
UserAgent: cfg.UserAgent,
MultiaddrResolver: cfg.MultiaddrResolver,
EnableHolePunching: cfg.EnableHolePunching,
HolePunchingOptions: cfg.HolePunchingOptions,
EnableRelayService: cfg.EnableRelayService,
RelayServiceOpts: cfg.RelayServiceOpts,
})

Then we add the transports:

go-libp2p/config/config.go

Lines 243 to 246 in 4400328

if err := cfg.addTransports(h); err != nil {
h.Close()
return nil, err
}

Then we start listening:

go-libp2p/config/config.go

Lines 250 to 253 in 4400328

if err := h.Network().Listen(cfg.ListenAddrs...); err != nil {
h.Close()
return nil, err
}

@Stebalien
Copy link
Member

I believe we want identify in the base host, the autonat dialer wont work without it.

What does it use identify for?

@vyzo
Copy link
Contributor

vyzo commented Dec 21, 2021

actually maybe you are right, it doesnt really care.

@marten-seemann
Copy link
Contributor Author

Right, the blankhost used by autonat today doesn't have identify.

@marten-seemann marten-seemann force-pushed the base-host branch 4 times, most recently from 81af661 to 4adc5c6 Compare January 9, 2022 11:17
@marten-seemann marten-seemann changed the title split a base host out of the basic host split a blank host out of the basic host Jan 9, 2022
@marten-seemann marten-seemann mentioned this pull request Jan 18, 2022
38 tasks
@marten-seemann marten-seemann mentioned this pull request Apr 2, 2022
65 tasks
@marten-seemann marten-seemann mentioned this pull request May 20, 2022
41 tasks
@BigLep BigLep added this to the Best Effort Track milestone Aug 26, 2022
@BigLep BigLep added the P2 Medium: Good to have, but can wait until someone steps up label Aug 26, 2022
@BigLep
Copy link
Contributor

BigLep commented Aug 26, 2022

2022-08-26 conversation: this turned out to be more than we expected. Core maintainers will take this on sometime in the future.

@BigLep BigLep marked this pull request as draft September 9, 2022 16:02
@MarcoPolo
Copy link
Contributor

Turns out it was a lot more work than expected. But it seems like we may be circling back on this soon. This PR though is really outdated.

@MarcoPolo MarcoPolo closed this Jun 3, 2024
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
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

remove go-libp2p-blankhost
5 participants