refactor: migrate database layer from sqlx to rusqlite #42
Conversation
Summary of ChangesHello @martinserts, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the application's database layer, transitioning from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that migrates the database layer from the asynchronous sqlx library to the synchronous rusqlite library, using r2d2 for connection pooling. This is a good move to simplify the database logic. However, the migration has introduced a critical issue: several parts of the asynchronous codebase now make blocking database calls directly on the async runtime's worker threads. This can lead to severe performance degradation and potential deadlocks. My review focuses on identifying these blocking calls and suggesting the use of tokio::task::spawn_blocking to correctly handle them. I've also noted a potentially destructive behavior in the new database initialization logic.
src/transactions/displayed_transaction_processor/resolver/database.rs
Outdated
Show resolved
Hide resolved
stringhandler
left a comment
stringhandler
left a comment
There was a problem hiding this comment.
untested review. Mostly looks fine, except for the one that retries a DbError over and over.
I have noted possible swallowing of errors on .next().transpose() but I am not entirely sure if this would result in a swallowing of error, it just looks like it might. If you have tested this, feel free to ignore
| println!("An intermittent error occurred during the scan cycle: {}", err_msg); | ||
| sleep(self.scan_interval).await; | ||
| }, | ||
| ScanError::DbError(err_msg) => { |
There was a problem hiding this comment.
Should this not be an error that is returned, not retried?
There was a problem hiding this comment.
Should this not be an error that is returned, not retried?
@stringhandler You are right - fixed 010f69e
| )?; | ||
|
|
||
| let rows = stmt.query(named_params! { ":name": friendly_name })?; | ||
| let row = from_rows::<AccountRow>(rows).next().transpose()?; |
There was a problem hiding this comment.
I think this might swallow an error
There was a problem hiding this comment.
I think this might swallow an error
@stringhandler I think this is alright, and does not swallow the error.
from_rows::<AccountRow>(rows).next() - Option<Result<AccountRow, Error>>
from_rows::<AccountRow>(rows).next().transpose() - Result<Option<AccountRow>, Error>
| &self, | ||
| password: &str, | ||
| ) -> Result<(RistrettoSecretKey, CompressedKey<RistrettoPublicKey>), anyhow::Error> { | ||
| ) -> WalletDbResult<(RistrettoSecretKey, CompressedKey<RistrettoPublicKey>)> { |
There was a problem hiding this comment.
I think this method is better suited to return an anyhow::Error because of the many errors that can be returned here. But this is a subjective opinion, it can remain as is
There was a problem hiding this comment.
I think this method is better suited to return an anyhow::Error because of the many errors that can be returned here. But this is a subjective opinion, it can remain as is
@stringhandler
I added another error enum DecryptionFailed(String), and simplified decrypt_keys() so that it may error out only with this error - 312d4b3
If we move to anyhow error here, it will be a chain reaction, and almost all methods will have to be turned from WalletDbResult into anyhow, and we will lose way to distinguish errors.
| claimed_sender_address as "claimed_sender_address: _", | ||
| balance_credit, | ||
| balance_debit, | ||
| REPLACE(effective_date, ' ', 'T') as effective_date, |
There was a problem hiding this comment.
This seems like it should have been migrated properly
There was a problem hiding this comment.
This seems like it should have been migrated properly
@stringhandler No, it did not work without this replace.
The thing is, that sqlite does not have a date/datetime field. It just stores plain strings, and dates were/are stored as YYYY-MM-DD HH24:MI:SS (with a space).
sqlx was doing some magic in its macros - effective_date as "effective_date: NaiveDateTime"
Now we are using serde_rusqlite, which will deserialize NativeDateTime. And it requires it to be in ISO format.
| "PRAGMA journal_mode = WAL; | ||
| PRAGMA synchronous = NORMAL; | ||
| PRAGMA foreign_keys = ON; | ||
| PRAGMA busy_timeout = 5000;", |
There was a problem hiding this comment.
These options need comments justifying their choice
There was a problem hiding this comment.
These options need comments justifying their choice
Great tip - added comments - 4ad1a9a
| "Database migration failed: {}. Please, remove database {:?} manually", | ||
| e, &path | ||
| ); | ||
| Err(WalletDbError::Unexpected( |
There was a problem hiding this comment.
This error message is misleading. It should rather be "Unexpected error occured. It is possibly a migration from sqlx that has failed. Consider removing database file and try again"
There was a problem hiding this comment.
This error message is misleading. It should rather be "Unexpected error occured. It is possibly a migration from sqlx that has failed. Consider removing database file and try again"
@stringhandler You are right, updated the message - d9a026f
| .fetch_optional(&mut *conn) | ||
| .await?; | ||
| let rows = stmt.query(named_params! { ":output_hash": output_hash })?; | ||
| let result: Option<OutputInfoRow> = from_rows(rows).next().transpose()?; |
There was a problem hiding this comment.
possible swallowing of error. Looks like you used optional in other places
There was a problem hiding this comment.
The same as above.
from_rows(rows).next() - Option<Result<OutputInfoRow, Error>>
from_rows(rows).next().transpose() - Result<Option<OutputInfoRow>, Error>
| )?; | ||
|
|
||
| let rows = stmt.query(named_params! { ":account_id": account_id })?; | ||
| let row = from_rows::<ScannedTipBlockRow>(rows).next().transpose()?; |
There was a problem hiding this comment.
possible swallowing of error
There was a problem hiding this comment.
The same as above.
from_rows::<ScannedTipBlockRow>(rows).next() - Option<Result<ScannedTipBlockRow, Error>>
from_rows::<ScannedTipBlockRow>(rows).next().transpose() - Result<Option<ScannedTipBlockRow>, Error>
3 participants
Description
Migrates DB layer from
sqlxtorusqlite.It uses
serde_rusqlitefor auto wrapping/unwrapping for DB types.rusqlite_migrationis used for migrations.Closes tari-project/tari#7643
Motivation and Context
We are planning to use
minotari-clias a library.sqlxwas making it hard.How Has This Been Tested?
{ "total": 690, "available": 690, "locked": 0, "unconfirmed": 0, "total_credits": 1058954880, "total_debits": 1058954190, "max_height": 156148, "max_date": "2025-12-11 09:27:41" }{ "error": "Not enough funds. Available: 690 µT, required: 1620 µT" }{ "utxos": [ { "version": "V0", "value": 690, "commitment_mask_key_id": { "Encrypted": { "encrypted": [ 28, 117, 129, 236, 32, 79, 33, 102, 89, 46, 254, 141, 111, 246, 112, 63, 216, 164, 86, 210, 229, 117, 14, 142, 173, 153, 120, 160, 241, 137, 219, 26, 48, 203, 210, 122, 137, 53, 36, 50, 190, 46, 54, 237, 42, 126, 207, 186, 120, 61, 110, 219, 98, 123, 91, 0, 138, 63, 149, 68, 163, 83, 175, 200, 9, 230, 246, 69, 121, 34, 144, 243 ], "key": { "inner": "view_key" } } }, "features": { "version": "V0", "output_type": 0, "maturity": 0, "coinbase_extra": "", "sidechain_feature": null, "range_proof_type": "bullet_proof_plus" }, "script": "7e66a39c2f732cdf850f813113fadb168b66efa6b037fa8da46adb6a67b3f3d119", "covenant": "", "input_data": "", "script_key_id": { "Derived": { "key": { "inner": "encrypted.1c7581ec204f2166592efe8d6ff6703fd8a456d2e5750e8ead9978a0f189db1a30cbd27a89352432be2e36ed2a7ecfba783d6edb627b5b008a3f9544a353afc809e6f645792290f3.view_key" } } }, "sender_offset_public_key": "0c5d53464985c23f5fc44847cf41ad407c2de1e13106e01d71126e54266e1078", "metadata_signature": { "ephemeral_commitment": "3a237c51d72352f1ec72d5d4e3ea9922b40085c6fd17682aa3a6cc29a3570749", "ephemeral_pubkey": "10af1f2bac2e479822ebb170215d56eb3ea595d5e1c1adfc45774202395f121c", "u_a": "1eff4fa81f84e406e5ee215510e52f03601e4bcbf49dfc0297382c52d1a3ce07", "u_x": "d7005ed1749ebf9aa10d5700d05afa0fc6582926d071109994c121501a233a06", "u_y": "0158ad9c61af0173c26fe445e898e4e087b2757530ec50517aa42e69dd8dfd05" }, "script_lock_height": 0, "encrypted_data": { "data": "3bb0d5f5a8de9b1dc345410c43d3996352c60ef6c06c4eaa71fbf037930b6a95286d04dc5ac5cdebd141e645639504b77452864a66e4d009c32d7d389546e1cc79acef8c41ba2e56612e8b9c7d7235bdea092c9c1ce737e6d94e32a806908f5db9a39f8171ffcf3a4e70cfef4773522823b10151b00bedb3889088516762637ead17535f5453c393cb13c9f3ae351c1ec71e0d7723b608874c425c152bf0f882a5e5fcf65fbb2f932767492676c87824b9f6e06eafa9e29da42ece46a2ca05b0fb688abbcc89d04ad923366a4c4e0c400d55" }, "minimum_value_promise": 0, "range_proof": "018153f8a01dd9640bc7f7f16d8985b5f1180048a047d6581503f2200eec7d60073c3106750e169d7f8044b03633bb6f69e2cf308dcfa0c6ad4517147f68bd197c9c505161e628397f1da8d266d301ca756226180a488173984c5077b3a5469865ba2ffd563b91bf3057b352531052bfc504661d490aaff6dc574831f83c71f6435734790d8b01e544c2f3f813edaa7d6db83d043f0600ee9cfa2982d3c29bc50e5a0cf3db44aa2e2993fbdd903ff986ac3f8aa7c9000ea7c118dccc16c980440e6cafdd5c4421bda6fcb2012645828f1a53b0a56c4273b4d3b5b07352d73e6900f4f6cd3d2b7d62fbafd9d720141cea3187c1f27cac43fe25f7d7bfd3b65fb94df2f8478439c9d7a47a4ec1c584823ba101f1cf9b00cb10b2e2cb8b6f5990654206c42207533f111ca79add574a500999a85267afde89cc45946aeea704494f7ce41a2c249bf979bfd08c6a42db374ed1aaee0af7d6c2f7361dad9f4c10b852652e98ca5f59491dda56ab2c2027873ae441d088f06bb1cbf437351ce3ab5e8245eacc041f74dd09592a1c2fee9ca1543bbcc3f35a178c186c21740090ea4555078640629f4650ba7c9fce520f4fdab503a000d822b351ff98593d47ec86af9e2200a012ae4e1d761b1acfb43d7a4763c328383c44eb4d8102b344374cf96c584f068c75b30bb355fd3f5067f80e5163ff91b8b1d693b51f84377dee5c118e572e78b39c86e7969a60f37d19a0aef7a34406781fc82bd65c6a0129b41af421b66fd8691085f2b6b06841b786351799221b72ed4171cab932c7e5501689a8773b12", "payment_id": { "inner": { "TransactionInfo": { "recipient_address": "12BS6NPHQWS7Xg66dDj5o7gC1t3dSvwZAqgxXsuEwwgZWUVY5LYDeWCRpbm1bTjdbvpyKJr3S1tDry6NXMfkLDZacJR", "sender_one_sided": true, "amount": 84964000, "fee": 660, "tx_type": "PaymentToOther", "sent_output_hashes": [ [ 117, 249, 186, 3, 48, 25, 229, 250, 188, 218, 97, 255, 44, 177, 102, 200, 119, 234, 174, 92, 41, 89, 46, 80, 223, 13, 60, 97, 61, 231, 102, 31 ] ], "payment_id": [ 67, 104, 97, 110, 103, 101 ] } } }, "output_hash": [ 30, 185, 128, 62, 225, 227, 196, 221, 32, 253, 89, 17, 77, 201, 153, 172, 152, 101, 177, 40, 47, 95, 16, 230, 42, 181, 251, 55, 242, 119, 90, 202 ], "commitment": "506e50bf93de6ee2570fd6f181c1605b2128cc9044a3e5bbe50a29d020cb8a32" } ], "requires_change_output": true, "total_value": 690, "fee_without_change": 355, "fee_with_change": 620 }What process can a PR reviewer use to test or verify this change?
Breaking Changes