Optimize decryption key handling

[fredo]

After some discussion with @ulope, we decided to keep the keys in an in-memory cache instead of checking against the db.
The reasons are:

• lower latency (even though db queries are not that latency heavy, but it adds up)
• we typically expect that keys have a short life cycle. We don't need to keep decryption keys for a long time and we don't need to keep a lot of keys in memory at the same time. The optimization aims to reduce the validation time of receiving the same message content from different peers. An LRU cache is ideal for such a scenario.

[jannikluhn]

The code looks correct to me.

I don't think adding in memory state to the handler is the right way to go though. As far as we know, the bottleneck is computation, not db queries, so this feels like premature optimization. Even if it turns out to be db queries, I think we should look at this more holistically and not start randomly getting rid of this one query among the 10s of others we do during message processing. The downside of adding the cache is increased complexity which makes it harder to reason about the system: Before, we had all state in the DB, now we have to consider the in-memory state as well (which might or might not be in agreement).

One other issue is that the cache is not wide enough. Keys that are created locally and send to the network will pass through the validator, but not through the handler, so they won't make it into the cache, while they would make it into the db.

So all in all I would prefer using only the DB for caching for as long as it's not the bottleneck. But that's just my opinion, it's definitely a significant performance improvement, so I'm ok with this as well.

[jannikluhn]

For unreachable errors I personally prefer if err != nil { panic("msg") }. It's self documenting and if the error unexpectedly occurs, most likely we would panic later anyways (because cache would be nil), but at a random a more random line.

[fredo]

Thanks for the review and your opinion. I think you made a valid point regarding a more holistic approach to caching instead of randomly adding caches here and there and that resonates with me very much. I'm actually happy to think about a proper solution once we have the time for it.
Just a couple of additional thoughts which Ulo and I came up with.

• Synchronization: I thought about the different scenarios and I think the cache is built in a way that being out of sync will "only" fallback to the current state. And then only once until it is in sync again. If key is in cache but not in DB (for whatever reason) it will be added to the DB by the handler. If key is in DB but not in cache it will be verified once (expensive) but then added to the cache.
• We calculated a bit worst case scenarios, where you also could give your statement to. Let's assume 21 keypers and 47 keys (maximum number for 1 M gas). How often would the keys have to be verified? I would assume that in the worst case we would receive 20 messages with each 47 keys in it (maybe it's not very likely). That would result in ~1000 DB reads. But I understand that the performance gain compared to crypto verification is probably large enough. And optimizing from DB to in-memory is maybe a matter of a few ms to us. Maybe adding additional complexity is indeed over-optimization at this point but I don't know.

I would love to get some benchmarking actually. Do you think the benchmark test would give a valuable output?