Refactor/improve performance sqlite db #5472

multisme · 2025-07-31T17:27:21Z

Public API changes documented in changelogs (optional)

Signed-off-by: multi [email protected]

Fixes #3697

poljar

Not quite there yet I'm afraid.

crates/matrix-sdk-sqlite/src/crypto_store.rs

crates/matrix-sdk-sqlite/CHANGELOG.md

codspeed-hq · 2025-08-02T09:48:30Z

CodSpeed Performance Report

Merging #5472 will not alter performance

_{Comparing multisme:refactor/improve_performance_sqlite_db (eca9e89) with main (b6433de)}

Summary

✅ 37 untouched benchmarks

codecov · 2025-08-02T10:26:37Z

Codecov Report

❌ Patch coverage is 64.47368% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.61%. Comparing base (b6433de) to head (eca9e89).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-sqlite/src/crypto_store.rs	38.46%	6 Missing and 2 partials ⚠️
crates/matrix-sdk-sqlite/src/state_store.rs	38.46%	6 Missing and 2 partials ⚠️
crates/matrix-sdk-sqlite/src/event_cache_store.rs	22.22%	6 Missing and 1 partial ⚠️
crates/matrix-sdk-sqlite/src/utils.rs	55.55%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5472      +/-   ##
==========================================
- Coverage   88.63%   88.61%   -0.02%     
==========================================
  Files         341      341              
  Lines       95138    95191      +53     
  Branches    95138    95191      +53     
==========================================
+ Hits        84322    84354      +32     
- Misses       6628     6650      +22     
+ Partials     4188     4187       -1

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

multisme · 2025-08-02T11:31:44Z

Hey I'm unsure about the get_or_create_store_cipher could you point me in the right direction?

poljar

Hey I'm unsure about the get_or_create_store_cipher could you point me in the right direction?

What about get_or_create_store_cipher() looks reasonable except that the secrets are copied and not zeroized.

You need to either not copy them or you need to zeroize them using the zeroize crate.

crates/matrix-sdk-sqlite/src/utils.rs

crates/matrix-sdk-sqlite/src/crypto_store.rs

crates/matrix-sdk-sqlite/src/lib.rs

crates/matrix-sdk-sqlite/src/state_store.rs

crates/matrix-sdk-sqlite/src/utils.rs

…f encrypted store

…method

…nstead of a only a key to encrypt a store

…f an insecure export function

This reverts commit 3a8f5f1.

This reverts commit 009ee3a.

multisme · 2025-08-08T11:26:36Z

@poljar Hey I think I managed the implementation, could you provide me with feedback please?

…te_db

…g the opening with a key

…te_db

poljar

Sorry for the late reply, I was unavailable the past two weeks. This seems almost good to go.

Though we have a tricky problem with hidden copies of the secret key material.

poljar · 2025-08-26T11:34:07Z

crates/matrix-sdk-sqlite/CHANGELOG.md

@@ -6,6 +6,14 @@ All notable changes to this project will be documented in this file.

 ## [Unreleased] - ReleaseDate

+### Features
+- Implement a new constructrot that allow to open SqliteCryptoStore with a Key


Suggested change

- Implement a new constructrot that allow to open SqliteCryptoStore with a Key

- Implement a new constructor that allows to open the SqliteCryptoStore with a cryptographic key.

poljar · 2025-08-26T11:59:17Z

crates/matrix-sdk-sqlite/src/lib.rs

+    #[zeroize]
+    Key([u8; 32]),
+    #[zeroize]
+    PassPhrase(String),


The enum variants need to be documented.

poljar · 2025-08-26T11:59:38Z

crates/matrix-sdk-sqlite/src/lib.rs

@@ -43,13 +44,21 @@ pub use self::state_store::{SqliteStateStore, DATABASE_NAME as STATE_STORE_DATAB
 #[cfg(test)]
 matrix_sdk_test_utils::init_tracing_for_tests!();

+#[derive(Clone, Debug, Eq, PartialEq, Zeroize)]


I think we should derive ZeroizeOnDrop here so we don't need the manual zeroize() calls.

poljar · 2025-08-26T12:47:23Z

crates/matrix-sdk-sqlite/src/lib.rs

+#[derive(Clone, Debug, Eq, PartialEq, Zeroize)]
+pub enum Secret {
+    #[zeroize]
+    Key([u8; 32]),


This should be inside a Box or inside a Zeroizing struct so we avoid unintended copies due to moves being a memcpy call in Rust land. More info here: https://benma.github.io/2020/10/16/rust-zeroize-move.html.

In short, every time you pass the Secret to a function a memcpy call will copy the contents of the array.

If a Box is used, the pointer is instead copied.

poljar · 2025-08-26T12:50:24Z

crates/matrix-sdk-sqlite/src/lib.rs

+    #[zeroize]
+    Key([u8; 32]),
+    #[zeroize]
+    PassPhrase(String),


Same here, this will produce unintended copies, Zeroizing<String> would make sense.

multisme requested a review from a team as a code owner July 31, 2025 17:27

multisme requested review from stefanceriu and removed request for a team July 31, 2025 17:27

poljar requested review from poljar and removed request for stefanceriu August 1, 2025 07:52

poljar requested changes Aug 1, 2025

View reviewed changes

crates/matrix-sdk-sqlite/src/crypto_store.rs Show resolved Hide resolved

crates/matrix-sdk-sqlite/CHANGELOG.md Outdated Show resolved Hide resolved

poljar requested changes Aug 4, 2025

View reviewed changes

multisme added 20 commits August 7, 2025 17:33

Add the key logic to the SqliteStoreConfig struct

91438c5

Replace the passphrase logic with a key logic in the implementation o…

8b072ec

…f encrypted store

Remove all passphrase mention

a99f0c9

Updated changelog

102b4cf

Updated the store encryption to use a enum Secret instead of passphrase

65a9bc0

Format files

f517587

Correct some tests

fd32c5a

More reformarting of files

4b7800b

Refactorize SqliteStoreConfig::key and SqliteStoreConfig::passphrase …

22e29ac

…method

Revert get_or_create_store_cypher to use

5af4d49

Comment the use

59c40c4

Update changelog to represent changes

734cd94

remove typo

c838f2a

correct comment on the crypto store file

de0e85f

correct comment on the state store file

f2afd18

Update some expect text to make sure they reflect the use of secret i…

a308973

…nstead of a only a key to encrypt a store

Uncomment the config directives and allows test to run faster by usin…

1def8d6

…f an insecure export function

Use of lifetime in order to not clone/copy the data

88aaffc

Update matrix-sdk with new lifetimes

4d7dd6c

Temporary comment insecure function

587331d

multisme added 5 commits August 7, 2025 17:33

Refactorize tests config to correspond with the new api

1625c16

Revert "Update matrix-sdk with new lifetimes"

b157860

This reverts commit 3a8f5f1.

Revert "Use of lifetime in order to not clone/copy the data"

063ab19

This reverts commit 009ee3a.

implement zeroizing of secrets after use

98a6e36

Correct wrong borrow

9718b79

multisme force-pushed the refactor/improve_performance_sqlite_db branch from 065ec6e to 9718b79 Compare August 7, 2025 20:33

multisme added 9 commits August 11, 2025 15:56

Merge branch 'matrix-org:main' into refactor/improve_performance_sqli…

d0d68b4

…te_db

Remove conditional logic for running tests

69e8ee0

reformat

601893b

Update changelog

f7a24ed

Merge branch 'matrix-org:main' into refactor/improve_performance_sqli…

7c8b3c5

…te_db

Remove some superfluous change

ff5ccef

Reimplement previous tests for the store and on top of the one testin…

d930a36

…g the opening with a key

Replace missing line

d8dfda6

Merge branch 'matrix-org:main' into refactor/improve_performance_sqli…

eca9e89

…te_db

multisme requested a review from poljar August 21, 2025 15:22

poljar requested changes Aug 26, 2025

View reviewed changes

	- Implement a new constructrot that allow to open SqliteCryptoStore with a Key
	- Implement a new constructor that allows to open the SqliteCryptoStore with a cryptographic key.

Refactor/improve performance sqlite db #5472

Are you sure you want to change the base?

Refactor/improve performance sqlite db #5472

Conversation

multisme commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codspeed-hq bot commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #5472 will not alter performance

Summary

Uh oh!

codecov bot commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

multisme commented Aug 2, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

multisme commented Aug 8, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

poljar Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

poljar Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

poljar Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

poljar Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

poljar Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

multisme commented Jul 31, 2025 •

edited

Loading

codspeed-hq bot commented Aug 2, 2025 •

edited

Loading

codecov bot commented Aug 2, 2025 •

edited

Loading