bindings/crypto-nodejs: after each receiveSyncChanges call, there is an empty KeysUploadRequest pending

[turt2live]

sample logs:

Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixClientLite] Performing sync with token s25976_489518_6_623_3309_168_5298_2809_1
Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixHttpClient] (REQ-38) GET http://localhost:8338/_matrix/client/v3/sync
Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixHttpClient] (REQ-38) qs = {"full_state":false,"timeout":30000,"since":"s25976_489518_6_623_3309_168_5298_2809_1"}
Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixHttpClient] (REQ-38 RESP-H200) {
  next_batch: 's25976_489519_6_623_3309_168_5298_2809_1',
  presence: { events: [ [Object] ] },
  device_one_time_keys_count: { signed_curve25519: 50 },
  'org.matrix.msc2732.device_unused_fallback_key_types': [],
  device_unused_fallback_key_types: []
}
Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixClientLite] Received sync. Next token: s25976_489519_6_623_3309_168_5298_2809_1
{}
Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixHttpClient] (REQ-39) POST http://localhost:8338/_matrix/client/v3/keys/upload
Wed, 29 Jun 2022 21:00:56 GMT [DEBUG] [MatrixHttpClient] (REQ-39) body = {"device_keys":null,"one_time_keys":{}}

The outgoing request should be omitted if there's nothing to actually upload.

[Hywan]

I bet it's related to matrix-sdk-crypto itself as the Node.js binding simply forwards calls and data.

[poljar]

I wouldn't bet on that.

matrix-rust-sdk/crates/matrix-sdk-crypto/src/machine.rs

Line 515 in a8601e1

if device_keys.is_none() && one_time_keys.is_empty() && fallback_keys.is_empty() {

So we can see that the keys upload request will be None if there isn't anything to upload. One of those three will be Some.

We can also see that, when we convert the request to js, we don't seem to map the fallback key field:

matrix-rust-sdk/bindings/matrix-sdk-crypto-nodejs/src/requests.rs

Line 245 in a8601e1

request!(KeysUploadRequest from RumaKeysUploadRequest maps fields device_keys, one_time_keys);

So my bet is that we never upload the fallback key because we don't pass it to the js side, the machine rightfully so tries to upload it again on the next sync.

[poljar]

Come to think of, why doesn't that macro use serde_json::to_string() directly instead of mapping things manually. This deliberately loses data, which causes this bug.

It ends up converting the map to a string in the end anyways.

[turt2live]

(exposing the fallback key would also be a great thing to have)

[poljar]

What do you mean by exposing?

[turt2live]

fallback keys from a http standpoint are only two parts: /sync and /upload. We currently have /sync as part of the bindings, but not /upload (as your comment mentioned regarding the field disappearing between translations)

[poljar]

Ok in that case, not only is it a great thing, it's a necessary thing. That's the whole bug here, we don't and won't support not uploading the fallback key.

[turt2live]

sorry, to rephrase: the intent is that the bindings will support fallback keys?

[poljar]

They have to support fallback keys, otherwise you end up uploading empty /keys/upload responses.

Yes, they will support fallback keys.

[Hywan]

Come to think of, why doesn't that macro use serde_json::to_string() directly instead of mapping things manually. This deliberately loses data, which causes this bug.

It ends up converting the map to a string in the end anyways.

That's a good question. I guess I got inspired by the crypto-FFI binding itself:

matrix-rust-sdk/bindings/matrix-sdk-crypto-ffi/src/responses.rs

Lines 131 to 142 in a8601e1

	KeysUpload(u) => {
	let body = json!({
	"device_keys": u.device_keys,
	"one_time_keys": u.one_time_keys,
	});
	Request::KeysUpload {
	request_id: r.request_id().to_string(),
	body: serde_json::to_string(&body)
	.expect("Can't serialize keys upload request"),
	}
	}

[poljar]

Come to think of, why doesn't that macro use serde_json::to_string() directly instead of mapping things manually. This deliberately loses data, which causes this bug.
It ends up converting the map to a string in the end anyways.

That's a good question. I guess I got inspired by the crypto-FFI binding itself:

matrix-rust-sdk/bindings/matrix-sdk-crypto-ffi/src/responses.rs

Lines 131 to 142 in a8601e1

	KeysUpload(u) => {
	let body = json!({
	"device_keys": u.device_keys,
	"one_time_keys": u.one_time_keys,
	});
	Request::KeysUpload {
	request_id: r.request_id().to_string(),
	body: serde_json::to_string(&body)
	.expect("Can't serialize keys upload request"),
	}
	}

That's a good catch and I remembered why we don't serialize u there. Because Serialize is not implemented for it.

Let's fix this the ugly way for now and we'll have customized requests/responses that do implement Serialize later on. This will just be a continuation on #767.

[Hywan]

That's a good catch and I remembered why we don't serialize u there. Because Serialize is not implemented for it.

Correct. Some types from matrix-sdk-crypto can implement Serialize quite easily, but for Ruma, there must be a reason why they aren't implementing Serialize and Deserialize.