Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

batching of m.room_key to-device messages is suboptimal #24680

Closed
richvdh opened this issue Feb 27, 2023 · 5 comments
Closed

batching of m.room_key to-device messages is suboptimal #24680

richvdh opened this issue Feb 27, 2023 · 5 comments
Labels
A-E2EE O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Team: Crypto Z-Legacy-Crypto Issues affecting the legacy crypto stack

Comments

@richvdh
Copy link
Member

richvdh commented Feb 27, 2023

When we send out m.room_key messages to share a new encryption session, we attempt to batch those messages into groups of 20 per call to /sendToDevice. However the batching logic is distinctly suboptimal and we often end up creating much smaller batches.

I'm also not really sure why 20 was chosen as the target.

This leads to lots of requests to the /sendToDevice endpoint and general slowness in sending out room keys, potentially causing unable-to-decrypt errors (element-hq/element-meta#245) and causing the recipient to start sending out m.room_key_request messages, exacerbating the problem.

@richvdh richvdh added T-Defect S-Minor Impairs non-critical functionality or suitable workarounds exist O-Occasional Affects or can be seen by some users regularly or most users rarely Team: Crypto A-E2EE labels Feb 27, 2023
@richvdh
Copy link
Member Author

richvdh commented Feb 27, 2023

Likely we will fix this as part of #21972 rather than spending time fixing it in an implementation that will be thrown away.

@BillCarsonFr
Copy link
Member

Rust SDK limit is set to 250

Added a task in regression test list to later implement.

@richvdh
Copy link
Member Author

richvdh commented Feb 27, 2024

Rust SDK limit is set to 250

... and, more importantly, its batching logic looks sensible

@kegsay
Copy link
Contributor

kegsay commented Mar 15, 2024

Pending complement-crypto test to check that we batch in as large a chunk as possible i.e 250.

@kegsay
Copy link
Contributor

kegsay commented May 17, 2024

This was done.

@kegsay kegsay closed this as completed May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-E2EE O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Team: Crypto Z-Legacy-Crypto Issues affecting the legacy crypto stack
Projects
None yet
Development

No branches or pull requests

3 participants