Prevent storing non-contactable ENRs #246
Conversation
|
I think this should resolve #243 |
| match self.kbuckets.write().entry(&key) { | ||
| kbucket::Entry::Present(entry, _) if entry.value().seq() < enr.seq() => { | ||
| entry.remove() | ||
| } | ||
| kbucket::Entry::Pending(mut entry, _) => { | ||
| if entry.value().seq() < enr.seq() { | ||
| entry.remove() | ||
| } | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
nit:
is there be a situation for legit storing a non contactable ENR?
If not can we (instead) panic if it exists inkbuckets` while on debug, as we do in
Line 598 in 6707bcf
something like:
#[cfg(debug_assertions)]
{
match self.kbuckets.write().entry(&key) {
kbucket::Entry::Present(entry, _) if entry.value().seq() < enr.seq() => {
panic!("ENR shouldn't have been stored");
}
kbucket::Entry::Pending(mut entry, _) => {
if entry.value().seq() < enr.seq() {
panic!("ENR shouldn't have been stored");
}
}
_ => {}
}
}There was a problem hiding this comment.
The entry here is about finding a new ENR.
So we could have an ENR that is contactable and we store it in the routing table. Then later, we find they have changed their ENR to something non-contactable and we see an updated version. Because the key in the table is the node-id, it can be that the earlier version was contactable but the new one is not. So we remove the old one here and don't store the new one.
I can't wrap this in a debug only mode, because this is a valid case.
For the sake of speed, I"ll merge this PR and if my analysis here is wrong we can make a future PR to add this in.
There was a problem hiding this comment.
Thanks for clarifying age, makes sense
ENR updates could allow for storing non-contactable ENRs.
This PR handles some edge cases where we could update old ENRs with newer ones which are no longer contactable.