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

Fix: Own our CertifiedAddrBook #555

Merged
merged 7 commits into from
May 21, 2024
Merged

Fix: Own our CertifiedAddrBook #555

merged 7 commits into from
May 21, 2024

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Apr 17, 2024

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:

  • GossipSub's PX relied on go-libp2p's Identify consuming the signed peer record and inserting it into the host's peerstore.
  • go-libp2p's Identify was updated to avoid saving the signed peer records because:
    • They were not consumed anywhere. (by go-libp2p)
    • We need to filter the addresses to avoid polluting the address book with local addresses.
  • GossipSub's PX silently broke as it depended on an up to date CertifiedAddrBook on the host, but it was effectively unused.

The proposed solution in this PR:

  • Use the emitted events from go-libp2p to maintain gossipsub's own Certified Address Book.

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

gossipsub.go Show resolved Hide resolved
gossipsub.go Outdated
@@ -522,6 +529,45 @@ func (gs *GossipSubRouter) Attach(p *PubSub) {
}
}

func (gs *GossipSubRouter) manageAddrBook() error {

Choose a reason for hiding this comment

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

could we just not return anything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it should, the error is ignored anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you return nothing and log errors?
These are just swallowed now.

Copy link
Collaborator

@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.

looks reasonable, let me know when it is ready to merge.

gossipsub.go Show resolved Hide resolved
@MarcoPolo
Copy link
Contributor Author

@vyzo this is ready

Copy link
Collaborator

@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.

Just a nit, otherwise lgtm

&event.EvtPeerConnectednessChanged{},
})
if err != nil {
log.Errorf("failed to subscribe to peer identification events: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

and return, as it may panic trying to access sub.Out in the loop

@MarcoPolo
Copy link
Contributor Author

Hmm, need to investigate why the test is failing.

@MarcoPolo MarcoPolo mentioned this pull request May 20, 2024
@MarcoPolo
Copy link
Contributor Author

Hmm, need to investigate why the test is failing.

It's the resource manager being IP aware :)

@MarcoPolo MarcoPolo merged commit dbd1c9e into master May 21, 2024
4 checks passed
@Stebalien Stebalien deleted the own-certified-addr-book branch May 23, 2024 16:47
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.

Identify protocol: SignedPeerRecords not being added to the CertifiedAddrBook
4 participants