making the network middleware peer manager and connection gating optional #1040
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1040 +/- ##
==========================================
- Coverage 53.38% 53.33% -0.05%
==========================================
Files 318 318
Lines 21491 21506 +15
==========================================
- Hits 11472 11471 -1
- Misses 8450 8468 +18
+ Partials 1569 1567 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
That now makes sense to me thanks to your comment!
I left a few nits, which should not be blocking.
| // unicastMessageTimeout is the timeout used for unicast messages | ||
| // connectionGating if set to True, restricts this node to only talk to other nodes which are part of the identity list | ||
| // managePeerConnections if set to True, enables the default PeerManager which continuously updates the node's peer connections | ||
| // validators are the set of the different message validators that each inbound messages is passed through | ||
| func NewMiddleware(log zerolog.Logger, |
There was a problem hiding this comment.
Nit: do we want to log an issue for refactoring this long list of configuration options to a cfg struct with a functional options pattern, or stg similar to how internal libp2p config options work? That's probably also valid for our LibP2P Host and its long sequence of configs.
There was a problem hiding this comment.
Created this issue - https://github.com/dapperlabs/flow-go/issues/5713 to track that work
network/p2p/middleware.go
Outdated
| if m.managePeerConnections { | ||
| m.peerManager.RequestPeerUpdate() |
There was a problem hiding this comment.
Nit: that works, but there's a naked nil peerManager in the middleware that's a crash waiting to happen if it's one day accessed without checking the flag. Could we use a getter here?
network/p2p/middleware.go
Outdated
| err = m.libP2PNode.UpdateAllowList(identityList(idsMap)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update approved peer list: %w", err) | ||
| // update libp2pNode's approve lists is this middleware also does connection gating |
There was a problem hiding this comment.
| // update libp2pNode's approve lists is this middleware also does connection gating | |
| // update libp2pNode's approve lists if this middleware also does connection gating |
Currently, for the regular (staked) network we have connection gating and peer manager ON by default.
For the unstaked Access Node network, we don't want either of these since the participants of the network are not known upfront and for peer management we could directly use the libp2p's PeerDiscovery.
This change makes those two features configurable in the middleware and switches them off for the access node on the unstaked network.
Closes: https://github.com/dapperlabs/flow-go/issues/5703