Fix: Own our CertifiedAddrBook #555
Conversation
| @@ -522,6 +529,45 @@ func (gs *GossipSubRouter) Attach(p *PubSub) { | |||
| } | |||
| } | |||
|
|
|||
| func (gs *GossipSubRouter) manageAddrBook() error { | |||
There was a problem hiding this comment.
could we just not return anything?
There was a problem hiding this comment.
it should, the error is ignored anyway.
| case event.EvtPeerIdentificationCompleted: | ||
| if ev.SignedPeerRecord != nil { | ||
| cab, ok := peerstore.GetCertifiedAddrBook(gs.cab) | ||
| if ok { | ||
| ttl := peerstore.RecentlyConnectedAddrTTL | ||
| if gs.p.host.Network().Connectedness(ev.Peer) == network.Connected { |
There was a problem hiding this comment.
This API will also be confusing for users :(
There was a problem hiding this comment.
I think for all events we need to do host.Network().Connectedness(peer) checks
There was a problem hiding this comment.
Maybe this event should have a Connectedness method?
There was a problem hiding this comment.
Open question (for me): Can we be sure that a ConnectednessChange == Connected event will always happen before a EvtPeerIdentificationCompleted event?
There was a problem hiding this comment.
I think we can improve this API, but that shouldn't block this fix. Let's do this for now.
This is blocked until the next go-libp2p release that includes libp2p/go-libp2p#2759, but please still review.
Fixes GossipSub's Peer Exchange (PX) after go-libp2p's change to stop consuming signed peer records into its Certified Address book.
I'll briefly summarize the problem and this solution here, but for more context please follow the links below.
The problem:
The proposed solution in this PR:
A slightly more long term solution would be for go-libp2p to support services libp2p/go-libp2p#1993 that can provide resources that can be shared amongst other services/protocols. Imagine if two services needed a certified address book, then it would make sense to have a separate service that could provide an up to date address book.
For now potentially duplicating data if multiple services require a certified addr book seems like an okay solution.
Related though, is anyone aware of other protocols that depend on the host's certified address book?
Fixes libp2p/go-libp2p#2754
Related to: libp2p/go-libp2p#2355