look up cross-signing keys from the DB in bulk

[uhoreg]

to improve performance of looking up 500 users' keys.

Also improve some docstrings.

[erikjohnston]

Thanks! It'd be good to also add a cache here, as e.g. if a room enables encryption then we'll get N users asking for the keys of the other N users, so adding a cache should help a lot. (c.f. @cachedList)

[uhoreg]

It'd be good to also add a cache here, as e.g. if a room enables encryption then we'll get N users asking for the keys of the other N users, so adding a cache should help a lot. (c.f. @cachedList)

Ah, I forgot to do this.

So the difficulty here is that the from_user_id will be different for each user, which would make the result different, so we can't just cache the whole thing. I think we'd have to split the first query out into a separate function, cache that, and then add in the signatures afterwards.

I think that it'll end up being a big change, so I'll take it out of review until it's done.

[uhoreg]

I've split out the first query into a separate function, and set it up to take a list of key types. However, I'm running into some issues adding the caching.

1. I did the split in the function at https://github.com/matrix-org/synapse/pull/6486/files#diff-9f5f1ff9d5f3d4e7194db99c80985c81R340 which takes a txn parameter. Is it possible to make @cachedList ignore the txn parameter, or do I need to apply @cachedList to the non-txn version (which means the two queries will be in separate transactions)?
2. If I allow it to take a list of key types to fetch, then that seems like it could cause the cache to explode, as we could have a cache entry for each user, for each possible subset of key types. This would also make cache invalidation hard. In practice, we currently only search for both master and self-signing keys with that function, but it seems a bit dangerous to rely on that always being true. Are there any suggestions for how to deal with this?

[erikjohnston]

1. I did the split in the function at https://github.com/matrix-org/synapse/pull/6486/files#diff-9f5f1ff9d5f3d4e7194db99c80985c81R340 which takes a txn parameter. Is it possible to make @cachedList ignore the txn parameter, or do I need to apply @cachedList to the non-txn version (which means the two queries will be in separate transactions)?

Yeah, I'm afraid they will need to be in separate transactions (and you can then cache both transactions). I don't think that'll cause too much overhead as its just one extra query per request, rather than an extra query per queried user?

2. If I allow it to take a list of key types to fetch, then that seems like it could cause the cache to explode, as we could have a cache entry for each user, for each possible subset of key types. This would also make cache invalidation hard. In practice, we currently only search for both master and self-signing keys with that function, but it seems a bit dangerous to rely on that always being true. Are there any suggestions for how to deal with this?

I think including the key type is fine, the default cache sizes aren't that big so it won't use that much extra memory by default. You can then use invalidate_many to invalidate all keys for a user (or the standard invalidate to invalidatejust a particular user/key). c.f._get_linearized_receipts_for_room` for an example

The other option is to have the query return all keys for a user, filter the keys after the cache, and then invalidate all keys for a user if one of them changes. It's not perfect, but probably good enough.

[uhoreg]

Tests were working fine until I merged in develop, and then get_e2e_cross_signing_keys_bulk is now infinite-looping. (e.g. f1ea148 should work fine). From my debugging (i.e. print statements), it seems to be hanging somewhere between the return statement in _get_bare_e2e_cross_signing_keys_bulk_txn and the value arriving into _get_bare_e2e_cross_signing_keys_bulk. i.e., it seems to be hanging somewhere in runInteraction. Has something changed there recently?

Anyways, aside from that, should be ready for review now.

[erikjohnston]

Looks like it might be exploding due to a leaked log context, somewhere

[uhoreg]

Tests pass now. Should be good now. There is a bit of ugliness, though, in that the @cachedList seems to add None values to the dict, which need to be skipped over by various things. So there's a question of whether we add code to skip over None values, or whether we make another iteration of the dict in get_e2e_cross_signing_keys_bulk to drop the None values. I've implemented the first option for now, but switching should be easy. Aside from that minor thing, I think this should be ready.

[erikjohnston]

Suggested change

	def _get_bare_e2e_cross_signing_keys_bulk(self, user_ids: list) -> dict:
	def _get_bare_e2e_cross_signing_keys_bulk(self, user_ids: List[str]) -> Dict[str, Dict[str, dict]]:

(You might need to import from typing)

[erikjohnston]

Suggested change

txn (twisted.enterprise.adbapi.Connection): db connection

[erikjohnston]

Why ooi?

[uhoreg]

Right, I was going to add a comment explaining it. It's because _get_bare_e2e_cross_signing_keys_bulk has type annotations, so getargspec doesn't work on it, and it suggests to use getfullargspec instead.

[erikjohnston]

aaaaaah, cool