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

op-node: Add persistence of peer scores in peer store #5732

Closed
wants to merge 1 commit into from

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented May 18, 2023

Description

Introduces the concept of ExtendedPeerstore which provides a type-safe API for storing additional peer data - initially peer scores from various scoring sources.

Currently it doesn't actually use the information in the store but does update the store periodically with the latest gossip score for each peer. The extended peer store is also passed into the method that creates the connection gater to prove the plumbing all fits together but the gater hasn't been updated to actually use it.

Also simplifies the setup initialisation of the p2p layer by removing the connection gater and manager constructors from the config. These are both created as part of the config.Host call anyway so the config has control over them.

Currently scores are stored as meta entries for peers. This is simple but overly naive. The underlying datastore.Batching has been passed into the constructor to ensure that it is available when being wired up as an optimised implementation will likely use that directly. We will have to implement some form of GC if we move out of using meta (but that will give us flexibility on how long to retain scoring information which is likely to be required anyway).

Also pretty sure the API to get scores isn't the most useful thing. For disconnection we'll want to be able to get a total score value out rather than individual components, but not sure the peer store should be involved in applying weightings to calculate that score.

Tests

Please describe any tests you've added. If you've added no tests, or left important behavior untested, please explain why not.

Invariants

For changes to critical code paths, please list and describe the invariants or key security properties of your new or changed code.

Additional context

Add any other context about the problem you're solving.

Metadata

@changeset-bot
Copy link

changeset-bot bot commented May 18, 2023

⚠️ No Changeset found

Latest commit: 5b3ba8b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented May 18, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 5b3ba8b
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/64669a01b7ee2c0008f5535f

@ajsutton ajsutton force-pushed the aj/extended-peer-store branch 2 times, most recently from e038e1c to 3403457 Compare May 18, 2023 04:07
@ajsutton
Copy link
Contributor Author

Split out the simplification of init code to #5733

Comment on lines +468 to 470
// TODO: This will need to be modified to use sys.Mocknet.AddPeerWithPeerstore so we can make the peerstore
// an extended peerstore.
h, err := sys.Mocknet.GenPeer()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an unfortunate discovery. Switching to specifying the peerstore ourselves will require duplicating a bit of code that GenPeer previously did. Alternatively we could introduce the ExtendedPeerStore wrapper later rather than it being the actual host Peerstore but it feels a lot nicer to have everything go through the wrapper.

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 updated the libp2p PR to so that GenPeer can take a PeerStore and/or ConnectionManager so we can duplicate code in the short term but hopefully migrate to those new APIs in a future go-libp2p version.

@mergify
Copy link
Contributor

mergify bot commented May 18, 2023

Hey @ajsutton! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label May 18, 2023
Doesn't actually use those stores to disconnect peers but updates the store with gossip scores.
@mergify mergify bot removed the conflict label May 18, 2023
@mergify
Copy link
Contributor

mergify bot commented May 22, 2023

Hey @ajsutton! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label May 22, 2023
@ajsutton
Copy link
Contributor Author

Done in separate PRs.

@ajsutton ajsutton closed this May 23, 2023
@mergify mergify bot removed the conflict label May 23, 2023
@ajsutton ajsutton deleted the aj/extended-peer-store branch October 23, 2024 22:33
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.

1 participant