Sim2h gossip improvements #1884
Conversation
…g send" This reverts commit 5990bee.
lucksus
requested review from
willemolding,
thedavidmeister,
maackle and
zippy
There was a problem hiding this comment.
This all looks good, but It seems to me that one thing we are missing is cleaning up this data when an agent disconnects in the remove_agent() function in cache, or at least we should be marking that an agent isn't available and should be filtered out of the possible agents to get things from.
Co-Authored-By: Eric Harris-Braun <eric@harris-braun.com>
Co-Authored-By: Eric Harris-Braun <eric@harris-braun.com>
Co-Authored-By: Eric Harris-Braun <eric@harris-braun.com>
| &self, | ||
| agent_id: &AgentId, | ||
| entry_hash: &EntryHash, | ||
| aspects: &Vec<AspectHash>, |
There was a problem hiding this comment.
This should also be a HashSet, and then we can use set operations to answer the question this function asks
| for aspect in aspects { | ||
| if !missing_aspects_for_entry.contains(aspect) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
FYI we could use HashSet::difference() for this, but I'm not sure if the speed gain is worth it
crates/sim2h/src/lib.rs
Outdated
| agent_pool | ||
| .iter() | ||
| // We ignore all agents that are missing all of the same aspects as well since | ||
| // they can't help us. | ||
| .filter(|a| !space_lock.agent_is_missing_all_aspects(*a, entry_hash, aspects)) | ||
| .collect() | ||
| }; |
There was a problem hiding this comment.
This could get slow as we scale up the number of agents, since we are iterating over all agents and calculating which are missing aspects. Since we ultimately only want one agent that meets the criteria, a faster way would be to generate a random permutation of vec indices so we can iterate over the agents randomly, and return the first one that doesn't have missing aspects.
Can use: https://docs.rs/rand/0.6.5/rand/seq/trait.SliceRandom.html#tymethod.shuffle
- Shuffle the agent pool (O(1)) before conducting search so we don't have to filter the entire agent pool - Avoid an extra clone + iteration of the agent pool by including the agent ID sameness check at the same time as doing the missing aspects check
…ust into sim2h-improvements
PR summary
EntryNotFoundLocallyerror on that node)testing/benchmarking notes
If a large number of nodes enter into a network at the same time (as tested in our stress-tests), the assumption that was underlying the current gossip-mock code in sim2h doesn't apply: if a node is missing an entry aspect, sim2h assumes all other nodes to have it and picks one randomly. But if 50 nodes come on at the same time, all of them is missing entry aspects (namely agent_id and chain headers of the other 49 nodes). But only one of those other nodes actually has that data - drawing one from the whole pool with uniform distribution leads to a gossip failure in 98% of cases.
The changes in this branch might reduce the network load in our stress-tests substantially.
followups
( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )
changelog
documentation