refactor: refactor gossip usage of kbuckets #1480
Conversation
|
My intention was to NOT change any functionality. But I observed some things in the code that I'm questioning, so I would like to highlight them here, and we can have a discussion if they are correct or not.
|
There was a problem hiding this comment.
- IIRC there's some reason that non-connected nodes are in our routing table... maybe bc we only evict nodes if a bucket is full? so even if it disconnects but the bucket isn't full, then we keep it in, so we need to filter these out from active gossip
- I don't think this is spec defined and up to client implementation... In the past, I've attempted to integrate a RFN lookup (so we include some nodes, that might not be in our routing table, in the gossip)... idk why, but it just didn't work very well (most likely due to implementation error), though it's probably worth another effort
3.1 I think you're right that this doesn't work for state. We'll need to implement some special handling depending on network type.
3.2 IIUC fallback_find_content only triggers concurrent subsequent requests if offers are received from other peers during transmission/validation for the given content key. So if a peer spams us with random content keys, but we don't receive offers for those content keys during tx from another peer, then the process will stop. Unless, a malicious party spins up 100 nodes and offers us the same spammy content key from each node... which I guess is an attack vector....
- Probably just leftover from earlier refactoring
This is just off the top of my head though, but i'm happy to dig deeper / verify any assumptions above
portalnet/src/gossip.rs
Outdated
| // Key is base64 string of node's ENR. | ||
| let mut enrs_and_content: HashMap<String, Vec<(TContentKey, Vec<u8>)>> = HashMap::new(); | ||
| // Map from content_ids to interested ENRs | ||
| let mut interested_ends = batch_interested_enrs::<TMetric>(&content_ids, kbuckets); |
There was a problem hiding this comment.
Or I guess
| let mut interested_ends = batch_interested_enrs::<TMetric>(&content_ids, kbuckets); | |
| let mut interested_enr_batch = batch_interested_enrs::<TMetric>(&content_ids, kbuckets); |
Since interested_enrs is used below for those interested in a single ID.
There was a problem hiding this comment.
I settled for content_id_to_interested_enrs
There was a problem hiding this comment.
Yup, looks great! 👍🏻
For your questions. Agree with Nick, but to add:
2. I think it's ok (good?) that a peer is only effective at gossiping to nearby peers. Let the content gossip die out if there are no known interested peers. Recursive gossip sounds like the job of the bridge, to me. So no change necessary.
3. I stumbled on that code-block myself recently and had the same concern. I don't understand Nick's take.
portalnet/src/gossip.rs
Outdated
| let random_farther_enrs = farther_enrs | ||
| .into_iter() | ||
| .choose_multiple(&mut rand::thread_rng(), NUM_FARTHER_NODES); |
There was a problem hiding this comment.
Surely this isn't of practical importance to performance, because I wouldn't expect enrs to get too huge, but I got curious and thought I would report my findings:
- Randomly selecting from an iterator seems like it needs to iterate the whole thing, making it
O(farther_enrs.len())here, which is supported by the docs - The algorithmically fastest option I found is
partial_shuffle()on a slice, which isO(NUM_FARTHER_NODES), as makes sense
There was a problem hiding this comment.
I also think it's of little practical importance, but since we started debate, let's go deeper :)
I see 3 ways to do it:
// 1. Shuffle: clean, very readable, no cloning. BAD: O(farther_enrs.len())
farther_enrs.shuffle(&mut rand::thread_rng());
farther_enrs.truncate(NUM_FARTHER_NODES);
enrs.extend(farther_enrs);
// 2. Partial shuffle: clean, O(NUM_FARTHER_NODES), BAD: we have to clone ENRs
let (selected_farther_enrs, _other_farther_enrs) =
farther_enrs.partial_shuffle(&mut rand::thread_rng(), NUM_FARTHER_NODES);
enrs.extend_from_slice(selected_farther_enrs);
// 3. Manual: simple, O(NUM_FARTHER_NODES), no cloning. BAD: not as clean, very manual
let mut rng = rand::thread_rng();
for _ in 0..NUM_FARTHER_NODES {
let enr = farther_enrs.swap_remove(rng.gen_range(0..farther_enrs.len()));
enrs.push(enr);
}First one is the cleanest, but worse performance (question is how much it matters).
Second one has good performance, but we have to clone ENRs (again, it's only 4 of them, does it matter?)
Last one, that I ended up going with, has both best performance and no cloning. The downside is that I don't find it as "clean" as the other two, but I think it's simple and clean enough.
I don't have strong preferences to any of the them, so if you do, I'm willing to change it.
There was a problem hiding this comment.
Hah, a good bit of performance fun. Good catch on the cloning. LGTM!
portalnet/src/gossip.rs
Outdated
| let permit = match utp_controller { | ||
| Some(ref utp_controller) => match utp_controller.get_outbound_semaphore() { | ||
| Some(permit) => Some(permit), | ||
| None => continue, | ||
| None => { | ||
| debug!("Permit for gossip not acquired! Skipping gossiping to enr: {enr}"); |
There was a problem hiding this comment.
IMO a log that could spew inside a loop like this belongs in a trace!
portalnet/src/gossip.rs
Outdated
| // Key is base64 string of node's ENR. | ||
| let mut enrs_and_content: HashMap<String, Vec<(TContentKey, Vec<u8>)>> = HashMap::new(); | ||
| // Map from content_ids to interested ENRs | ||
| let mut interested_ends = batch_interested_enrs::<TMetric>(&content_ids, kbuckets); |
There was a problem hiding this comment.
Or I guess
| let mut interested_ends = batch_interested_enrs::<TMetric>(&content_ids, kbuckets); | |
| let mut interested_enr_batch = batch_interested_enrs::<TMetric>(&content_ids, kbuckets); |
Since interested_enrs is used below for those interested in a single ID.
| .filter(|node| { | ||
| XorMetric::distance(&content_id, &node.key.preimage().raw()) < node.value.data_radius() | ||
| TMetric::distance(content_id, &node.key.preimage().raw()) <= node.value.data_radius |
There was a problem hiding this comment.
I imagine we are inconsistent about <= and = for comparing to the radius. I just scanned the specs and don't see an official disambiguation. It's fine to change, because I think it's practically irrelevant. Do you think it's important? If so, maybe we should put it in the spec.
There was a problem hiding this comment.
In my mental model, radius was always inclusive. Reason being, if radius is MAX, we want all content, even if it is the exact opposite from our node_id (in practice, not likely to happen).
As I was typing this, I can see the argument for radius being non-inclusive. If radius is zero, we don't want any content, even if it is the same as our node_id. If I have to pick which of the two impossible scenarios make more sense, I would still go with the first one (you can interpret it as "I'm responsible for my node_id +/- radius", in which case, you are always "responsible" for at least your node_id because it's about you).
When deciding if we should store the content (link) we use radius as inclusive, so at least now we will be somewhat consistent.
Either way, I think it's practically irrelevant, so I don't think it we need to update the spec. We can ask others, maybe somebody has strong opinion or good reason for one way or the other.
| 3. I stumbled on that code-block myself recently and had the same concern. I don't understand Nick's take. I think it tries to cover the scenario when we are receiving multiple offers for the same content id (e.g. new data being seeded into network and we are offered the same content from multiple other nodes, as part of their gossip). There is logic to accept only one Offer request for one content key, but if that one fails, we try to get it from other peer that offered it to us. @njgheorghita is that correct? |
|
@morph-dev yup! It's basically protection for the "thundering herd" problem as you described, though maybe the actual mechanism could use a better name since the name "fallback" might seem to imply that it will go out to seek all failed content txs from the network... |
Right, there is no formal open connection. As I recall, a node is considered disconnected if it recently failed to respond to a ping or other message. Then we sporadically ping "disconnected" nodes to see if it's back online. ETA an example of node status changing to Disconnected after a request timeout: |
What was wrong?
The gossip logic holds kbuckets lock for non-trivial amount of code-lines, which makes my improvements regarding kbuckets lock somewhat hard. So I want to refactor this logic upfront.
How was it fixed?
Extracted logic that uses locks into separate functions:
interested_enrsandbatch_interested_enrs. These functions will be moved into separate structure in the next PR so they shouldn't contain gossip specific functionality (e.g. callingselect_gossip_recipients).To-Do