Explicit Peering Agreement implementation #13773
Conversation
|
|
cmd/flags.go
Outdated
| } | ||
| // DirectPeers specifies a set of peers to connect to for "Explicit Peering Agreement" | ||
| // Reference: https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md#explicit-peering-agreements | ||
| DirectPeers = &cli.StringSliceFlag{ |
There was a problem hiding this comment.
this flag is not necessary. The -peer flag means the same thing here as we only specify trusted peers there(those not penalized/disconnected with). Making them a direct peer of our mesh is a logical next step.
There was a problem hiding this comment.
I see! I added another flag because maybe you wanted to distinguish between "static" and "explicit". For instance, Teku keeps this separation (Consensys/teku#8004 (comment)), while Lighthouse doesn't.
If you want to proceed with making the -peer flag configuring direct peers, I'll update my PR accordingly soon!
There was a problem hiding this comment.
In prysm, this peer is assumed to be trusted:
https://github.com/prysmaticlabs/prysm/pull/13773/files#diff-e231d920accff9e68d1581fc082513cda53631f2a312c9005448288b29dd08a3R97
So its fine to make them direct too for us. There isn't much benefit to differentiating them with different flags.
There was a problem hiding this comment.
I've made some changes in order to configure static peers as direct in the pubsub options. I hope it is good now. Thank you!
thedevbirb
force-pushed
the
feat_explicit_peers
branch
2 times, most recently
from
64b5511
to
dbf69ed
Compare
| if err != nil { | ||
| log.WithError(err).Error("Could not convert static peers raw ENRs into multiaddresses") | ||
| return | ||
| } |
There was a problem hiding this comment.
If 0 valid addrs are returned from PeersFromStringAddrs, we should log an error/warning and exit early
There was a problem hiding this comment.
I've just added a warning log! Thank you
thedevbirb
force-pushed
the
feat_explicit_peers
branch
from
dbf69ed
to
9fc5d31
Compare
|
You have lint failures: |
beacon-chain/p2p/pubsub.go
Outdated
| return psOpts | ||
| } | ||
|
|
||
| // addDirectPeersOption parses the static peers from the service configuration and adds | ||
| // the direct peers option to the pubsub options. | ||
| func (s *Service) addDirectPeersOption(psOpts []pubsub.Option) { |
There was a problem hiding this comment.
You will need to return the opts, as it becomes a different slice below.
There was a problem hiding this comment.
Sorry for that! I've written very few lines of Go code so far :). I thought psOpts was passed by reference. Now I've enforced it and the underlying data should be modified.
Tried to run golangci-lint locally and seems good now
thedevbirb
force-pushed
the
feat_explicit_peers
branch
2 times, most recently
from
9c59203
to
a22b2fd
Compare
beacon-chain/p2p/pubsub.go
Outdated
| return | ||
| } | ||
|
|
||
| *psOpts = append(*psOpts, pubsub.WithDirectPeers(directAddrInfos)) |
There was a problem hiding this comment.
This isn't the standard way its usually done in go. To make this more readable can you return psOpts in the method ? so that it is clear what is the argument to the method and what is its output.
There was a problem hiding this comment.
I'm sorry again! I think I've simplified the logic a bit by moving the parsing of raw ENRs into a separate function. I hope it is idiomatic Go code now. Thanks for the patience
There was a problem hiding this comment.
No worries, it looks good to me now
thedevbirb
force-pushed
the
feat_explicit_peers
branch
from
a22b2fd
to
908046b
Compare
What type of PR is this?
What does this PR do? Why is it needed?
This PR enables Gossipsub v1.1 "Explicit Peering Agreement as discussed in #13635
Which issues(s) does this PR fix?
Closes #13635
Other notes for review
Explicit peers (called "direct" in the codebase, to follow
go-libp2p-pubsubconvention) are considered also "static" in order to connect with them at startup and persist the connection with them. Then, the peers specified in the CLI option are just passed downstream to pubsub configuration.