crypto: Add more logs when sharing keys #3274
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3274 +/- ##
==========================================
+ Coverage 83.62% 83.64% +0.01%
==========================================
Files 239 239
Lines 24705 24725 +20
==========================================
+ Hits 20660 20680 +20
Misses 4045 4045 ☔ View full report in Codecov by Sentry. |
poljar
requested changes
Apr 2, 2024
| &self, | ||
| already_shared_devices: &Vec<(ReadOnlyDevice, ShareState)>, | ||
| withthelds: &Vec<(ReadOnlyDevice, WithheldCode)>, | ||
| ) -> Vec<(String, String, String)> { |
There was a problem hiding this comment.
A Vec<(String, String, String)> is quite the handful. Can we add a type with the desired Debug implementation instead? Then The split_devices() method can return that directly.
| devices: BTreeMap<OwnedUserId, Vec<ReadOnlyDevice>>, | ||
| outbound: &OutboundGroupSession, | ||
| ) -> (Vec<ReadOnlyDevice>, Vec<(ReadOnlyDevice, ShareState)>) { | ||
| let split: (Vec<(ReadOnlyDevice, ShareState)>, Vec<(ReadOnlyDevice, ShareState)>) = devices |
There was a problem hiding this comment.
Suggested change
| let split: (Vec<(ReadOnlyDevice, ShareState)>, Vec<(ReadOnlyDevice, ShareState)>) = devices | |
| let (to_share_with, shared_with): (_, Vec<_>) = devices |
| }) | ||
| .partition(|(_, state)| matches!(state, ShareState::NotShared)); | ||
|
|
||
| (split.0.into_iter().map(|(d, _)| d).collect(), split.1) |
There was a problem hiding this comment.
Suggested change
| (split.0.into_iter().map(|(d, _)| d).collect(), split.1) | |
| (to_share_with.into_iter().map(|(d, _)| d).collect(), shared_with) |
| @@ -810,6 +806,13 @@ impl GroupSessionManager { | |||
| let unable_to_encrypt_devices = | |||
| self.encrypt_for_devices(devices, &outbound, &mut changes).await?; | |||
|
|
|||
| // This could be verbose, but usefull to debug why a room key wasn't shared. | |||
There was a problem hiding this comment.
Suggested change
| // This could be verbose, but usefull to debug why a room key wasn't shared. | |
| // This could be verbose, but useful to debug why a room key wasn't shared. |
|
This is now months old without any comment. If you want to move this along, please reopen. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have been investigating some rageshakes recently, and often we don't have enough log history to see when/if a key was correctly distributed to another device.
This is an attempt to not only log to whom a key is shared in the current round, but also to log the other devices that we already tried to distribute too.
It might be a bit verbose? Let me know what you think.
The log look like that:
Signed-off-by: