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

Track the number of ratchet advances in the Olm Session, and include it in the debug output. #134

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Mar 19, 2024

In order to help understand Olm decryption errors, let's keep track of the number of DH ratchet clicks, and log them as part of the debug output.

@richvdh richvdh force-pushed the rav/ratchet_count_logging branch from 8662b17 to 5d9e3a8 Compare March 19, 2024 14:46
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 81.48148% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 84.65%. Comparing base (7a1887f) to head (5599cef).
Report is 1 commits behind head on main.

Files Patch % Lines
src/olm/session/double_ratchet.rs 81.81% 4 Missing ⚠️
src/olm/session/receiver_chain.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
- Coverage   84.72%   84.65%   -0.08%     
==========================================
  Files          32       32              
  Lines        1768     1786      +18     
==========================================
+ Hits         1498     1512      +14     
- Misses        270      274       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I left some nits and I'm going to require tests for this one.

src/olm/session/double_ratchet.rs Outdated Show resolved Hide resolved
src/olm/session/double_ratchet.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -2,6 +2,12 @@

All notable changes to this project will be documented in this file.

## UNRELEASED
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I need to inform you that vodozemac never abandoned git-cliff for changelog entries. So this belongs in a commit message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right. Presumably putting it in the subject line of the PR, and then squash-merging the PR, will do the right sort of thing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it's a conventional commit it will be picked up, since you historically complained that commit messages don't make great changelog entries you can put the conventional commit into a merge commit.

The merge commit message is anyways useless so it's a good opportunity to repurpose it for a better cause.

In the future I'd like to make it possible so you can have a special tag in merge commits which will mark the start of the changelog entry, similar to how the Signoff is treated specially in commit messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, well, I've removed this for now.

@richvdh richvdh changed the title Record the number of times the Olm ratchet is advanced Track the number of ratchet advances in the Olm Session, and include it in the debug output. Mar 20, 2024
@richvdh richvdh requested a review from poljar March 20, 2024 13:46
@richvdh
Copy link
Member Author

richvdh commented Mar 20, 2024

@poljar: have added some tests. Let me know what you think. Happy to add more if you have suggestions.

Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the tests.

@richvdh richvdh force-pushed the rav/ratchet_count_logging branch from 5599cef to ab00f4b Compare March 20, 2024 17:01
@poljar poljar merged commit 6101be8 into main Mar 21, 2024
26 checks passed
@poljar poljar deleted the rav/ratchet_count_logging branch March 21, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants