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

Explicit peers #5333

Merged
merged 5 commits into from
Mar 7, 2024
Merged

Explicit peers #5333

merged 5 commits into from
Mar 7, 2024

Conversation

mempirate
Copy link
Contributor

@mempirate mempirate commented Feb 29, 2024

Issue Addressed

Closes #5268

Proposed Changes

This PR implements explicit peering agreements (https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md#explicit-peering-agreements). It was decided not to add a separate --explicit-peers flag, but instead instantiate trusted peers as explicit. This means that when a trusted peer is defined, it will have the following properties:

  • exempt from peer scoring
  • automatic reconnects
  • unconditional forwarding of all messages

@CLAassistant
Copy link

CLAassistant commented Feb 29, 2024

CLA assistant check
All committers have signed the CLA.

@jimmygchen jimmygchen added ready-for-review The code is ready for review Networking labels Mar 1, 2024
@AgeManning
Copy link
Member

AgeManning commented Mar 4, 2024

@mempirate - Thanks for the PR.

You've caught us at a time where we are actively going through and removing old/deprecated and non-critical CLI flags, as we've had a few complaints of there being too many.

How do you feel about just using the trusted-peers CLI flag. i.e all trusted peers become explicit peers on the gossipsub level. I think this what should have been implemented when we added the trusted-peers flag.

@mempirate
Copy link
Contributor Author

mempirate commented Mar 5, 2024

@AgeManning I think that also makes sense as an approach! Agreed that there's a lot of overlapping functionality.

Done in following commit.

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Nice thanks for the addition!

Also, in the future, we can always change the base of the PR, no need to make a new one :)

(I know this, because I accidentally do it all the time 😅 )

@AgeManning AgeManning added ready-for-merge This PR is ready to merge. v5.1.0 Q2 2024 and removed ready-for-review The code is ready for review labels Mar 7, 2024
@pawanjay176
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Mar 7, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request conflicts with at least one pull request ahead in queue.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@pawanjay176
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Mar 7, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request #5333 has been manually updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@paulhauner
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Mar 7, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 641f6be

@mergify mergify bot merged commit 641f6be into sigp:unstable Mar 7, 2024
29 checks passed
@paulhauner paulhauner mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking ready-for-merge This PR is ready to merge. v5.1.0 Q2 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants