feat(addressbook): prune stale entries by last-seen time#5513
feat(addressbook): prune stale entries by last-seen time#5513martinconic wants to merge 6 commits into
Conversation
155323e to
1716cdb
Compare
|
Here, prune here runs once, at startup only, and it can lead that if we have some stable connections over bigger period of time, where we haven't updated LastSeen, it will remove those on next restart. But those are our most valuable connections. Maybe we could use One gap is left, what do to with the peer we only hear over hive gossip, but we didn't connect to? |
This is what we've converged to with @janos as a naive starting point to ship.
For now let's leave them in. The first line of defense is to get rid of garbage which is in the statestore. A peer which is seen over hive and is not connected to is still a potential peer you'd like to connect to. I agree that over time yes, probably you don't wanna keep those. I suggest to ship this first and see it works, then evolve into more well defined scenarios. |
|
Added two fixes:
|
|
|
||
| // mu serializes the read-modify-write in UpdateLastSeen against Put, so a | ||
| // concurrent Put is not rolled back by a stale copy of the entry. | ||
| mu sync.Mutex |
There was a problem hiding this comment.
i don't see why this is needed. this is not business critical information and i think it is fine with this hypothetical use case happening every now and then, if ever.
|
|
||
| // Prune addressbook entries whose overlays have not been seen recently, so | ||
| // the address book does not accumulate stale peers indefinitely. | ||
| if err := addressbook.Prune(time.Now().Add(-addressbookPruneAfter)); err != nil { |
There was a problem hiding this comment.
since you prune only on startup - there's no need to expose the method over an interface - just make sure that addressbook ctor runs the pruning internally.
| continue | ||
| } | ||
|
|
||
| // Hearing about a peer we already know is a sighting in its own right, |
There was a problem hiding this comment.
it seems Seen() now fires even when the incoming record is later rejected for being in the future (ErrTimestampInFuture) or invalid (ErrTimestampInvalid). These aren't meaningful sightings — a bogus record from a third party shouldn't refresh a legitimate peer's last-seen. The old placement after CheckTimestamp, gated on ErrTimestampStale || ErrTimestampTooSoon, was more precise.
Was moving it before CheckTimestamp intentional?
There was a problem hiding this comment.
I think it was part of the addressing of my previous comments. The address still passes some validation until we reach this point. And it could be that the new address was minted and sent not out of malicious behavior. I am fine whatever you two decide about this - have it before or after does not matter from my side, originally I think it was inside the error check after the CheckTimestamp and I wanted to avoid a situation where the errors dictate whether the peer gets marked seen yes/no. Before or after is better from my perspective, I don't really mind which one.
acud
left a comment
There was a problem hiding this comment.
minus the comment about exporting Prune (which should anyway be a method, not a pure function), this looks good from my side
…uning Resolved a conflict in pkg/addressbook/addressbook_test.go: master's #5534 added mockCorruptedStore and TestGetCorruptedNilAddress next to this branch's Seen and prune tests. The two additions are independent, so both are kept.
Checklist
Description
Implements address book pruning (#5491).
The address book recently gained a wrapping verifiedAddress{Address, Verified} struct (v2.8.0, #5477). This PR extends it with a last-seen timestamp and uses it to drop peers we have not seen for over a month, preventing the address book from accumulating stale, unreachable peers indefinitely.
Changes
AI Disclosure