Fix bug in periodic peer pinging #579
Conversation
dht.go
Outdated
| maxLastSuccessfulOutboundThreshold time.Duration | ||
| // maxLastSuccessfulOutboundThreshold is the max threshold/upper limit on the time duration | ||
| // between the current time and the last time a peer was useful to us. | ||
| maxLastSuccessfulOutboundThreshold float64 |
There was a problem hiding this comment.
why make this a float? it's only used here to compare with a time duration?
There was a problem hiding this comment.
Because it's calculated using a formula that translates to a float64. Take a look at:
Line 291 in 82c2333
| if queryPeer { | ||
| dht.routingTable.UpdateLastSuccessfulOutboundQuery(p, time.Now()) | ||
| if newlyAdded && queryPeer { | ||
| dht.routingTable.UpdateLastUsefulAt(p, time.Now()) |
There was a problem hiding this comment.
If this is a new peer, don't set this to now in addPeer regardless?
There was a problem hiding this comment.
@aarshkshah1992 not blocking a merge on this as it shouldn't really matter. We can fix this later.
There was a problem hiding this comment.
@Stebalien The idea is that if we discover and add a peer because of an inbound/outbound query, we do NOT want to evict the peer for one ''cycle".
Note that the time will NOT be "now" for peers we connect to without querying them .
There was a problem hiding this comment.
Talking this over with @aschmahmann, I think I was right. In addPeer (in the routing table), if queryPeer is true, we'll set "last useful" to now when the peer wasn't already in our routing table. Then, when the peer wasn't already in our routing table, we'll mark them as useful again.
There was a problem hiding this comment.
@Stebalien I see what you mean. I am removing this particular check from here and adding this commit so when we first query a peer after connecting to it(that's two calls to addPeer, once on connection and once upon a successful query), we give it one and ONLY one usefulness bump.
Need to set the maxThreshold correctly.