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 Peering Agreement implementation #8098

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

thedevbirb
Copy link
Contributor

PR Description

Implements Gossipsub v1.1 "Explicit Peering Agreement" by introducing a new CLI flag --direct-peers ("direct" is used instead of "explicit" to follow jvm-libp2p convention).
As such, documentation on https://github.com/Consensys/doc.teku should be updated as well to reflect this change (I can do it if you wish!).

Implementation

From a discovering configuration perspective, direct peers are treated as static peers, as we want to connect with them immediately on startup and persist the connection with them without being kicked out by the reputation system.
In order to do that, the list of static peers is extended with the direct peers provided in the CLI option.

Lastly, a function that determines whether a peer is direct or not is passed downstream to jvm-libp2p to enable the direct peer functionality on gossipsub.

Fixed Issue(s)

Closes #8004.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2024

CLA assistant check
All committers have signed the CLA.

@thedevbirb thedevbirb force-pushed the feat-direct-peers branch 2 times, most recently from c266d77 to 9e82e55 Compare March 20, 2024 09:31
Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. :) Added one comment about the DirectPeerManager initialization. Also will tag @Nashatyrev for his opinion.

@@ -163,5 +167,26 @@ public Builder opportunisticGraftThreshold(final Double opportunisticGraftThresh
this.opportunisticGraftThreshold = opportunisticGraftThreshold;
return this;
}

public Builder directPeers(final List<String> directPeers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think is not as simple because the String will be the whole multiaddress but we only care about the multihash. I think below should suffice and also simplifies a bit with lambda:

public Builder directPeers(final List<String> directPeers) {                  
  checkNotNull(directPeers);                                                  
  final List<NodeId> directPeerNodeIds =                                      
      directPeers.stream()                                                     
          .map(MultiaddrPeerAddress::fromAddress)                              
          .map(MultiaddrPeerAddress::getId)                                    
          .toList();                                                          
  final DirectPeerManager directPeerManager = directPeerNodeIds::contains;    
  peerScoringConfigBuilder.directPeerManager(Optional.of(directPeerManager)); 
  return this;                                                                
}                                                                                                                                                         

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have it like below in the Config class:

public Builder directPeers(final DirectPeerManager directPeers)

It would allow more flexible (dynamic peer set) and typesafe (String is pretty ambiguous) programmatic configuration

@Option(
names = {"--p2p-direct-peers"},
paramLabel = "<PEER_ADDRESSES>",
description = "Direct peers",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good place to add description similar to what will be in the docs, can mention they will be treated as explicit peers, not get downscored, etc

Copy link
Contributor

@Nashatyrev Nashatyrev left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor suggestion

@StefanBratanov it probably make sense to add a couple of libp2p unit tests to check if the isDirect is handled correctly

@@ -163,5 +167,26 @@ public Builder opportunisticGraftThreshold(final Double opportunisticGraftThresh
this.opportunisticGraftThreshold = opportunisticGraftThreshold;
return this;
}

public Builder directPeers(final List<String> directPeers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have it like below in the Config class:

public Builder directPeers(final DirectPeerManager directPeers)

It would allow more flexible (dynamic peer set) and typesafe (String is pretty ambiguous) programmatic configuration

@thedevbirb
Copy link
Contributor Author

Hey everyone, thanks for the detailed review and feedback! I hope I've addressed all your suggestions now:

  • downstream propagation of direct peers has been refactored by converting List<String> into List<NodeId> as soon as possible, and then converting it into DirectPeerManager
  • better description of "static" and "direct" peers has been added to the respective CLI options

I also added one little test to make sure the CLI option and the configurations do the right thing indeed.
You can look at the single updates in the related commits.

Thanks!

@thedevbirb thedevbirb marked this pull request as ready for review March 21, 2024 10:22
Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM to me as well. Feel free to raise a docs PR as well and we will approve it.

@StefanBratanov StefanBratanov enabled auto-merge (squash) March 21, 2024 13:42
@StefanBratanov StefanBratanov merged commit 60c38a9 into Consensys:master Mar 21, 2024
15 of 16 checks passed
@thedevbirb thedevbirb deleted the feat-direct-peers branch March 21, 2024 14:06
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.

Explicit Peering Agreement implementation
4 participants