-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
There was a problem hiding this 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.
8e6432a
to
0d66cc3
Compare
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. |
yeah, lets do it; base -vs- basic is confusing. Lets also have a blank profile to replace the blankhost. |
or is that not useful? |
There was a problem hiding this 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.
I believ we want identify in the base host, the autonat dialer wont work without it. |
There was a problem hiding this 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
if !ok { | ||
return errors.New("peerstore does not support signed records") | ||
} | ||
rec := peer.PeerRecordFromAddrInfo(peer.AddrInfo{ID: h.ID(), Addrs: h.Addrs()}) |
There was a problem hiding this comment.
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:
- We're going to clobber it in the Basic/Full host.
- The addresses are going to be wrong here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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:
Lines 243 to 246 in 4400328
if err := cfg.addTransports(h); err != nil { | |
h.Close() | |
return nil, err | |
} |
Then we start listening:
Lines 250 to 253 in 4400328
if err := h.Network().Listen(cfg.ListenAddrs...); err != nil { | |
h.Close() | |
return nil, err | |
} |
What does it use identify for? |
actually maybe you are right, it doesnt really care. |
Right, the blankhost used by autonat today doesn't have identify. |
81af661
to
4adc5c6
Compare
8536f68
to
fe624a6
Compare
fe624a6
to
ef58d56
Compare
ef58d56
to
ccd3f41
Compare
2022-08-26 conversation: this turned out to be more than we expected. Core maintainers will take this on sometime in the future. |
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. |
Fixes #1247.
As suggested by @Stebalien in #1257 (comment).
We probably need to work on naming here. Should we rename the
basic
package tofull
?