Aren't we creating the dehydrated device now? How can it already have keys? Aren't devices unique per user/device ID?

This is creating/storing the dehydrated device - to your question of how it can already have keys I actually don't know - per the MSC the keys were to be uploaded over the /keys/upload endpoint, implying that they already exist: "After the dehydrated device is uploaded, the client will upload the encryption
keys using POST /keys/upload/{device_id}, where the device_id parameter is
the device ID given in the response to PUT /dehydrated_device"

#15929 changed this so that the key upload was integrated into the PUT endpoint, and this PR is further refining the key upload such that storing the device and storing the keys are part of one single database transaction, addressing @poljar's concerns around storing the dehdrated device and it's keys being atomic.

Uploading the public keys at the time of the dehydrated device creation tries to address this race: matrix-org/matrix-spec-proposals#3814 (comment).

As to how can it already have keys, well if we remember what a dehydrated device is then it becomes quite clear. A dehydrated device are the private identity and one-time keys of a device, of course they are encrypted so the server can't access them.

It becomes quite natural, that we would like to upload the public parts of those same keys to the server at the same time, once we realize this. I think that doing it in a separate POST /keys/upload/ call is a mistake and quite weird from an API point of view, which the above mentioned race condition showcases.

OK, but part of this function checks if we already have keys uploaded for this device (and bails if they're not exactly equal to the new keys). I still don't follow the order of events that would let you upload keys before the device is created.

This could be me being overzealous here - I basically made sure that all the checks that happen in the /keys/upload pathway happen in this pathway, but maybe that's not necessary and we can just store the keys without checking them?

Part of it would be straightforward (Move _set_e2e_device_keys_txn to a method (instead of an inner function) and call it from both places.)

Although it seems this entire function is just a copy of upload_keys_for_user -- can we just call that instead?

The way we use the API, they are guaranteed to be unique. Furthermore since we only upload keys once for a dehydrated device there's little chance of getting this wrong.

I think if this is the case we can probably call the store methods that set the values directly and avoid the complicated logic of reusing keys.

call the store methods that set the values directly

so the reason I didn't do this was that my understanding of what was being asked was that the storing of the keys and the storing of the device needs to all happen in the same transaction - ie in _store_dehydrated_device_txn - wouldn't calling the store methods open a separate transaction?

They would need to be refactored to take a transaction as the first parameter and current callers would call that also.

Right I think this is sorted now, sorry for the confusion.

Please note that the key JSON must be in canonical JSON form.

Alternatively, update _upload_one_time_keys_for_user and _store_dehydrated_device_txn to not do the encoding and do it internally.

I've fixed the comment, thanks for pointing that out!