-
Notifications
You must be signed in to change notification settings - Fork 259
fix: don't limit peer exchange to dns-discovered peers #6941
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
Conversation
Jenkins BuildsClick to see older builds (22)
|
77960c0
to
3f20eb2
Compare
1c1561d
to
df48dbf
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6941 +/- ##
===========================================
- Coverage 59.19% 59.18% -0.02%
===========================================
Files 822 822
Lines 121865 121854 -11
===========================================
- Hits 72141 72119 -22
- Misses 42270 42290 +20
+ Partials 7454 7445 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
df48dbf
to
cbbbec4
Compare
// If not, the peer selection process in go-waku will filter them out anyway | ||
w.dnsAddressCacheLock.RLock() | ||
var peers peer.IDSlice | ||
for _, record := range w.dnsAddressCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if we can keep this logic of adding peers discovered via DNS to peerStore and connect to them.
This would provide better/more peers to choose from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chaitanyaprem, wouldn't we have DNS-discovered peers already in the peerStore
? Or we're not always connecting to them, only use for discovery?
So you want like no matter if we are connected to them or not, we should keep the peer exchange running with them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chaitanyaprem, wouldn't we have DNS-discovered peers already in the
peerStore
? Or we're not always connecting to them, only use for discovery?
Since they were being connected to here, was not sure if they are being added to peerStore somewhere else. If they are being added to peerStore then this can be removed.
So you want like no matter if we are connected to them or not, we should keep the peer exchange running with them?
This would be better imo as they would be bootnodes the chance of getting high quality peers is high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, indeed. I was sure they we were connecting to them elsewhere, but couldn't find it now.
Thank you, @chaitanyaprem, I will make sure we still connect to them 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we do connect to the DNS-discovered nodes, here:
status-go/messaging/waku/gowaku.go
Lines 536 to 543 in cbbbec4
func (w *Waku) discoverAndConnectPeers() { | |
for _, addrString := range w.cfg.WakuNodes { | |
err := handlePeerAddress(addrString, w) | |
if err != nil { | |
w.logger.Warn("failed to handle peer address", zap.String("addr", addrString), zap.Error(err)) | |
} | |
} | |
} |
status-go/messaging/waku/gowaku.go
Lines 563 to 567 in cbbbec4
func handlePeerAddress(addr string, handler peerAddressHandler) error { | |
if strings.HasPrefix(addr, "enrtree://") { | |
handler.discoverAndConnect(addr) | |
return nil | |
} |
And we duplicate the enrtree://
in the fleet configuration, for both static connection and Discv5 connection:
Lines 79 to 85 in f176cba
FleetStatusProd: { | |
ClusterID: 16, | |
WakuNodes: []string{ | |
"enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im", | |
}, | |
DiscV5BootstrapNodes: []string{ | |
"enrtree://AMOJVZX4V6EXP7NTJPMAYJYST2QP6AJXYW76IU6VGJS7UVSNDYZG4@boot.prod.status.nodes.status.im", |
So. All should be good. We have the fleet enrtree://
and always connect to all retrieved nodes. So they will not only appear in dnsAddressCache
, but also in the peer cache itself. So we will always run peer exchange with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from the comment.
Closes #6915
Attempted to implement a test here: #6943