crypto: misc refactorings #3000
Conversation
bnjbvr
force-pushed
the
commit-transaction-after-generating-otk
branch
from
111d6da
to
cdd9524
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3000 +/- ##
==========================================
- Coverage 83.50% 83.47% -0.03%
==========================================
Files 222 222
Lines 23214 23213 -1
==========================================
- Hits 19384 19378 -6
- Misses 3830 3835 +5 ☔ View full report in Codecov by Sentry. |
bnjbvr
force-pushed
the
commit-transaction-after-generating-otk
branch
2 times, most recently
from
f9cfd74
to
c1d8d4f
Compare
richvdh
requested review from
Hywan and
richvdh
and removed request for
Hywan
No. The server rejects uploads of a different one-time-key with the same ID as an existing one.
The fact that the server is catching the error is actually saving you from UTDs. The real problem with uploading OTKs before they are persisted to the db is this scenario:
|
| self.inner | ||
| .store | ||
| .with_transaction(|mut transaction| async { | ||
| let account = transaction.account().await.map_err(OlmError::Store)?; | ||
| account.update_key_counts( | ||
| sync_changes.one_time_keys_counts, | ||
| sync_changes.unused_fallback_keys, | ||
| ); | ||
| Ok((transaction, ())) | ||
| }) | ||
| .await?; |
There was a problem hiding this comment.
I'm afraid I'm failing to follow how this fixes the problem.
Is there not still a possibility that committing the transaction can fail, leaving us in the same situation as before? (As I understand it, that situation is that the Account has been updated, but not persisted to the database.)
There was a problem hiding this comment.
Hmm that's correct that this transaction can fail, if the account cannot be fetched. The rest of the code cannot fail.
There was a problem hiding this comment.
One solution would be to fetch the account before running the transaction maybe (if possible, because for now, the account is fetched by using the transaction).
There was a problem hiding this comment.
Hmm that's correct that this transaction can fail, if the account cannot be fetched. The rest of the code cannot fail
You mean to say that it's impossible that committing the transaction, after the account is updated, can fail? With the greatest of respect, I don't believe you 😛 .
There was a problem hiding this comment.
We haven't quite identified the cause of the problem yet, so we're still navigating a bit randomly here, and two days ago we thought this could be the source, but it's probably not.
But the only way to modify an Account is to get a transaction, that gives us a write-access scope to the account. There's no other way to upsert Accounts in theory; in practice, that needs to be double-checked because I'm not sure the API to write Account to the database is private/hidden/limited.
bnjbvr
force-pushed
the
commit-transaction-after-generating-otk
branch
from
c1d8d4f
to
dd48b53
Compare
bnjbvr
changed the title
|
For what it's worth, I'm retargeting this PR to only include the drive-by refactorings I've added, and I've removed the commit that was attempting to fix, because it's pretty certain that it's not a fix. As long as we don't have a clear repro for this issue, it seems inefficient to try to fix it. |
To make it easier to see it's a test function when looking up callers/callees.
…e_one_time_keys_if_needed`
bnjbvr
force-pushed
the
commit-transaction-after-generating-otk
branch
from
dd48b53
to
329157a
Compare
This started as a partial attempt to fix #1415, but the commit that was supposed to fix this has been removed from this PR, as it wasn't deemed useful: only refactorings remain, each explicited as an individual commit.
For history, the previous text for that PR stands below:
We're using application-level transactions to make sure that the account is properly synchronized in the cache vs in the database.
Before this commit, the transaction would be committed only when all the operations in it succeeded. This was based on the assumption that most encryption requests could be replayed, by re-sending them to the server. Unfortunately, this assumption doesn't hold for when generating one-time keys: it could be that one time-keys would be generated by the client, then the application-level transaction would fail, resulting in the client "forgetting" about the one time keys it uploaded. The server rejects reuploads of existing one-time keys, so that would end up wedging a device, causing unable-to-decrypt events, without a proper way out.
Here, we propose to save the account just after one-time keys have been generated, in a separate transaction.
A partial attempt to address #1415.
cc @kegsay @richvdh @BillCarsonFr