bugfix: make router load addresses from peerdb#2941
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2941 +/- ##
===========================================
+ Coverage 57.76% 71.61% +13.84%
===========================================
Files 2111 21 -2090
Lines 174239 1913 -172326
===========================================
- Hits 100652 1370 -99282
+ Misses 64631 445 -64186
+ Partials 8956 98 -8858
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| } | ||
| for addr := range peerDB.All() { | ||
| if err := peerManager.AddAddrs(utils.Slice(addr)); err != nil { | ||
| logger.Error("peerDB: bad address", "addr", addr.String(), "err", err) |
There was a problem hiding this comment.
How often do the addresses change? Is it possible that these addresses are outdated after you restart?
There was a problem hiding this comment.
they might be outdated. We don't know how long the downtime was. We also don't know if the peers are down. We make no assumptions here. The amount of stored addresses is small. It is not problem to just dial them all.
There was a problem hiding this comment.
Yeah, the peer can be outdated but usually wouldn't with just a restart. Even if there are outdated peers, those will get evicted from the DB and be replaced by valid peers right?
There was a problem hiding this comment.
yes, we only insert entries to db if we successfully connect to a peer and evict entries of peers that we haven't seen the longest
| for _, conn := range r.peerManager.Conns().All() { | ||
| conns := r.peerManager.Conns() | ||
| if conns.Len() > 0 { | ||
| ctrl.Updated() |
There was a problem hiding this comment.
nit: should we call this only if any address changed?
There was a problem hiding this comment.
this Watch was added only for the test purposes. It is fine to call Updated even of nothing changed. The point is to call it rarely enough to not waste cpu cycles. Here we call update ~10s by default, which is fine.
| key := makeKey(rng) | ||
| info := makeInfo(key) | ||
| options := makeRouterOptions() | ||
| options.PeerStoreInterval = utils.Some(time.Second) |
There was a problem hiding this comment.
nit: Could we set it to even a bit lower to make the test run faster?
There was a problem hiding this comment.
we could, currently 1s is nowhere close the latency of many other tests, so it shouldn't hurt.
| } | ||
| for addr := range peerDB.All() { | ||
| if err := peerManager.AddAddrs(utils.Slice(addr)); err != nil { | ||
| logger.Error("peerDB: bad address", "addr", addr.String(), "err", err) |
There was a problem hiding this comment.
If we detect bad address, shall we remove them from peer db?
There was a problem hiding this comment.
nah, peer db is a cyclic buffer in a sense, it will clean itself
Even though router was correctly storing the addresses of connected peers to db, it was not loading them back on startup. Therefore router was behaving as if peerdb did not exist in the first place. I have added a test for the fix. (cherry picked from commit e82324c)
|
Successfully created backport PR for |
3 participants
Even though router was correctly storing the addresses of connected peers to db, it was not loading them back on startup. Therefore router was behaving as if peerdb did not exist in the first place. I have added a test for the fix.