Peer Diversity for Routing Table and Querying #88
Conversation
There was a problem hiding this comment.
Lots of comments, let me know what you think. The comments are spread between both PRs, so you may want to take a look through both before doing a response review (I've gone back and corrected myself a few times as I've been going through the review).
Thought: Would it make sense to have the filter live in the DHT? It seems like the only thing that the routing table actually needs access to is something that implements the interface TryAdd(peer.ID) bool and Remove(peer.ID). That would mean we could touch the code in this repo less so we have fewer of these two repo PRs.
| bls: cidranger.NewPCTrieRanger(), | ||
| legacyCidrs: legacyCidrs, | ||
| logKey: appName, | ||
| cplFnc: cplFnc, |
There was a problem hiding this comment.
It seems bizarre that the "default" cpl implementation needs to be passed in externally like func (p peer.ID) int {return kb.CommonPrefixLen(dht.selfKey, kb.ConvertPeerID(p))} instead of just being included in the default object. What was the idea here?
There was a problem hiding this comment.
The reason is to not have a dependency on the kbucket package.
I'll fix this if we move this to the DHT package.
There was a problem hiding this comment.
Ah, so trying to avoid circular dependencies? This will likely cause problems even in the DHT package (RT still needs Filter and Filter still needs RT). However, if the RT takes an interface this goes away.
There was a problem hiding this comment.
As discussed offline, let's punt this for now and we can pay this tech debt later if required.
| // WhitelistIPNetwork will always allow IP addresses from networks with the given CIDR. | ||
| // This will always override the blacklist. | ||
| func (f *Filter) WhitelistIPNetwork(cidr string) error { |
There was a problem hiding this comment.
It's possible that we'd want to whitelist more than just domain ranges (e.g. peers I've been well connected to for more than a month are exempted whitelisted no matter their IP address). If we were passing an interface instead of a struct into NewRoutingTable I could just make a new filter with additional whitelist criteria, but if we're only going to have one implmentation then it should be more flexible (e.g. AddWhitelist(func(p peer.ID) bool).
There was a problem hiding this comment.
@aschmahmann I don't see the point of having a Filter interface for now as we really only have one implementation. Let's just wait till this evolves to create it ?
Have added a WhiteListPeerIds function for now and a test to go with it. We can even have an interface just for the WhiteListing stuff and inject it into the Filter later on if this functionality evolves more.
| } | ||
| group := PeerGroupInfo{Id: p, Cpl: cpl, IPGroupKey: key} | ||
|
|
||
| if rs, _ := f.wls.ContainingNetworks(ip); len(rs) != 0 { |
There was a problem hiding this comment.
What are the example use cases for whitelists? I'm wondering if peers that are whitelisted should count as 0 across the board instead of just getting an approval. For example, peer A is part of groups 1 + 2 and is whitelisted, while peer B is part of group 1 only. If we add B and then A they will both make it into the table, but if we add A and then B it's possible B might not make it.
There was a problem hiding this comment.
One example use case right now is whitelisting all the private IP address (I'm still figuring out how that'd work). Once we figure out the long lived peers logic, we might want to whitelist their peer Ids as well.
Also, I agree with you. I don't think whitelisted peers should take up slots/tickets. I've changed the code accordingly and added a test for it. Let me know what you think.
| // add it to the diversity filter for now. | ||
| // if we aren't able to find a place for the peer in the table, | ||
| // we will simply remove it from the Filter later. |
There was a problem hiding this comment.
nit: This is a little awkward, especially since we can fix this by checking the diversity filter after we check if there's room instead of the other way around. This doesn't seem like it's necessarily a big deal though.
There was a problem hiding this comment.
I agree that this is a bit awkward. I think I just didn't feel like calling filter.Add at multiple places here.
|
@aschmahmann I believe I've replied to all your comments on this PR. Please take a look and let me know what you think. |
Implementation of the filter for Routing Table Diversity.