fix: Don't drop messages. RouteBack cache update

[mfornet]

Increased route back cache to avoid dropping messages which
are supposed to be routed back. The problem was that when some
nodes are syncing several chunk request are asked, and request
arrive faster than responses, effectively invalidating the cache
most of the responses are dropped, since the route back hash was
dropped from the cache.

This PR also introduce cache strategy proposed here

Fix #2756

Test plan

• Reproduced scenario where several routed messages were dropped locally, and tested with new cache and with old cache. The fix was the size increase.
• Unit tests to check the new strategy is working properly.

[gitpod-io]

[bowenwang1996]

We should in general avoid negative numbers as much as possible, especially when it is not really needed.

[bowenwang1996]

Am I correct that you are using BTreeSet as a heap? If so, why bother with the negative number trick and not use BinaryHeap instead?

[mfornet]

Not exactly, because I need to insert/remove arbitrary elements from the collection.

[bowenwang1996]

if the cache is empty wouldn't this line crash the node?

[mfornet]

no, this function is only called when the cache is full so it should have at least one element. There is an assertion that cache capacity should be a positive number.

[SkidanovAlex]

With this implementation once we got to is_full, we will likely on each insert trigger the folloiwng:

1. Check if is_full, it is, so do remove_evicted, which is O(num_peers). In most cases there will be nothing to evict.
2. Call to remove_frequent, and actually remove one entry, and is also rather expensive (multiple accesses to a BTreeMap).
3. Insert.

It will be happening on each insert, so makes the cache pretty expensive compared to a regular cache that does one insert into a hashtable and one linked list insertion.

One simple-ish approach is to change remove_frequent to remove several (like 100) entries from the most offending peer. With a cache size of 1M and 40 peers we expect more than 25K messages from the offending peer, so removing 100 isn't much different from removing 1, it's still a tiny percentage of all the messages, but it removes the necessity to call to remove_evicted for 100 iterations, and also makes cost per removal in remove_frequent slightly lower, since iterating over the first 100 elements is cheaper than pulling the first 100 times.

With that, I would also always call remove_frequent from remove_evicted, for the reasons similar to above (remove_frequent is cheaper than remove_evicted, so doesn't add much to the cost, but makes it more predictable that the expensive evictions won't be happening more than once every 100 inserts)

[bowenwang1996]

this comment is no longer correct?