fix(db): add indexes, transaction atomicity, and N+1 query elimination

[PastaPastaPasta]

Summary

• Add missing network indexes to high-traffic database tables for faster queries
• Wrap insert_token() in a database transaction for atomicity
• Optimize identity and identity_order loading with LEFT JOIN to eliminate N+1 query patterns

Test plan

• Verify app starts and loads identities/tokens without errors
• Check database query performance with existing data
• Confirm token insertion is atomic (all-or-nothing)

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling and safer parsing for data deserialization and ID fields.

• Performance

  • Faster identity and top-up loading via consolidated queries.
  • Added several network-specific database indexes (including for scheduled votes).

• Chores

  • Bumped database schema version and added migrations for new indexes.
  • Made token insertions transactional and introduced identity-token balance storage.

• Tests

  • Added comprehensive tests for token insertion, ordering, replacement, and deletion.

[coderabbitai]

📝 Walkthrough

Walkthrough

Consolidates identity/top-up loading into single LEFT JOIN queries to eliminate N+1 patterns, bumps DB version to 27 and adds network indexes, and adds transactional token insertion that upserts tokens and their identity_token_balances with improved ID/error handling and tests.

Changes

Cohort / File(s)	Summary
Identity query consolidation src/database/identities.rs	Replaced per-identity top_up queries with single LEFT JOIN selects. Iteration refactored to manual row traversal with current_id tracking, deduplication, in-place population of identity fields and aggregation of top_ups. load_identity_order now uses LEFT JOIN to detect and batch-remove dangling references.
Database version & indexes src/database/initialization.rs, src/database/scheduled_votes.rs	Bumped DB version 26→27, added migration hook to create network-specific indexes. Added idx_scheduled_votes_network, idx_token_network, and idx_identity_token_balances_network; ensured wallet and asset_lock_transaction index creation paths updated.
Token operations & balances src/database/tokens.rs	Introduced transactional token insert flow: collect identities, start transaction, INSERT/ON CONFLICT token upsert, insert identity_token_balances within same transaction (ON CONFLICT DO NOTHING), then commit. Added table init/drop helpers for identity_token_balances.
Error handling & tests src/database/tokens.rs (tests included)	Replaced prints with tracing::error, replaced unwrap/expect with map_err for ID and DataContract parsing, propagated errors from token and balance retrievals, and added unit tests for token save/load, replacement, empty loads, and deletion scenarios.

Sequence Diagram

sequenceDiagram
    participant Client
    participant LockManager as Lock Manager
    participant DB as Database
    participant Tx as Transaction
    participant Bal as IdentityBalances

    Client->>LockManager: request DB operation (insert/upsert token)
    LockManager->>DB: acquire exclusive DB lock
    alt lock acquired
        LockManager->>Tx: begin transaction
        Tx->>DB: INSERT/UPDATE token (ON CONFLICT DO UPDATE)
        DB-->>Tx: token upserted
        Tx->>Bal: for each identity -> INSERT identity_token_balances (ON CONFLICT DO NOTHING)
        Bal->>DB: insert balance rows
        DB-->>Bal: balances inserted/ignored
        Tx->>DB: COMMIT
        DB-->>Tx: committed
        Tx-->>Client: success
    else lock failure
        LockManager-->>Client: retry or return error
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 I hop through joined rows and tidy each cup,

Gathering top‑ups so queries skip the loop,
I wrap tokens in transactions snug and neat,
Indexes planted so lookups are fleet,
A little rabbit’s work — database treats! 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(db): add indexes, transaction atomicity, and N+1 query elimination' directly and specifically summarizes the three main objectives of this pull request: adding database indexes, ensuring transaction atomicity in token insertion, and eliminating N+1 query patterns in identity loading.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr/database-optimizations

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/database/identities.rs`:
- Around line 219-263: The DB 'status' column is not being applied after
deserializing with QualifiedIdentity::from_bytes in
get_local_qualified_identities_in_wallets and get_identity_by_id, causing stale
status values; fix by reading the status column from the query result and
explicitly assigning it to identity.status right after creating the identity
with QualifiedIdentity::from_bytes (same approach used in
get_local_qualified_identities), making sure to reference the same row.get(...)
index or column name you used for other fields so the DB value overrides the
deserialized value.

🧹 Nitpick comments (2)

src/database/tokens.rs (2)

115-120: Drop wallets guard before starting the transaction to reduce lock contention.

The wallets read guard (Line 116) is held for the entire function body, including the database transaction (Lines 120–155). Since identities is fully collected on Line 117, the guard is no longer needed and can be dropped early.

♻️ Proposed fix

         // Collect identities before acquiring the connection lock for the transaction
         let wallets = app_context.wallets.read_or_recover();
         let identities = self.get_local_qualified_identities(app_context, &wallets)?;
+        drop(wallets);

         let mut conn = self.conn.lock_or_recover();
         let tx = conn.transaction()?;

---

266-268: Misuse of InvalidParameterName for data parsing errors.

rusqlite::Error::InvalidParameterName is semantically intended for SQL parameter binding issues, not domain-level data deserialization failures. Consider using rusqlite::Error::ToSqlConversionFailure(Box::new(e)) (as used elsewhere in this file, e.g., Line 95) for consistency and semantic correctness.

Also applies to: 344-349, 416-433

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/database/identities.rs (1)

321-341: ⚠️ Potential issue | 🟡 Minor

get_local_voting_identities does not read status from the DB column.

Unlike the refactored functions above which explicitly set identity.status = IdentityStatus::from_u8(status.unwrap_or(2)) from the DB, this function relies solely on what's deserialized from the data blob. If the DB status column is the source of truth, this will return stale status values.

The same concern applies to get_local_user_identities (Lines 347-369).

🤖 Fix all issues with AI agents

In `@src/database/tokens.rs`:
- Around line 141-152: The loop in insert_token that runs tx.execute on the
identity_token_balances table currently uses "ON CONFLICT(token_id, identity_id,
network) DO UPDATE SET balance = excluded.balance", which will overwrite
existing balances with 0; change that conflict clause in the tx.execute call
inside the for identity in &identities loop to "DO NOTHING" so balances are only
inserted when missing and existing non-zero balances are preserved.

🧹 Nitpick comments (3)

src/database/tokens.rs (2)

265-267: InvalidParameterName is semantically wrong for data-parsing errors.

rusqlite::Error::InvalidParameterName is intended for SQL named-parameter issues (e.g., :foo), not for identifier deserialization failures. This pattern is repeated at Lines 343-348 and Lines 415-432 as well. It will produce confusing error messages for callers/logs.

Consider using a custom error type or rusqlite::Error::ToSqlConversionFailure(Box::new(...)) (as done on Line 252) for consistency, even though neither is a perfect semantic fit.

---

614-752: Good test coverage for order and cleanup operations.

The tests properly cover save/load round-trips, replacement semantics, empty state, and devnet cleanup. Consider adding a test for insert_token transactional atomicity (e.g., verify the DB is clean after a simulated mid-transaction failure) since that's a stated PR objective.

src/database/identities.rs (1)

209-266: Significant code duplication with get_local_qualified_identities.

This function is nearly identical to get_local_qualified_identities (Lines 150-207), differing only in the AND i.wallet_index IS NOT NULL WHERE clause. Consider extracting the shared logic into a private helper that accepts an optional filter predicate or SQL fragment, reducing ~50 lines of duplication.

[Copilot]

Pull request overview

This PR improves local database performance and correctness by adding missing network indexes, making token insertion atomic, and eliminating N+1 query patterns when loading identities/top-ups and identity ordering.

Changes:

• Add network indexes to several high-traffic tables and bump DB schema version to 27 with a migration path.
• Wrap insert_token() operations in a single SQLite transaction.
• Replace per-identity top-up queries and per-row existence checks with LEFT JOIN-based loading to remove N+1 query behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/database/tokens.rs	Adds network indexes, makes token insertion transactional, improves parsing error handling, and adds token-order/deletion tests.
src/database/scheduled_votes.rs	Adds a network index for scheduled votes queries.
src/database/initialization.rs	Bumps schema version and adds a migration to create the new network indexes.
src/database/identities.rs	Eliminates N+1 queries via LEFT JOIN loading for identities/top-ups and identity order validation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lklimek]

Code Review Audit — Database Optimizations

Overall the PR is well-implemented. The N+1 → LEFT JOIN refactoring is correct, the transaction wrapping in insert_token adds proper atomicity, error handling improvements eliminate panics, and the new indexes are consistent between migration and fresh-install paths.

Issues not addressable via inline comments (pre-existing, outside diff):

CRITICAL — Guaranteed deadlock in insert_remote_identity_if_not_exists (identities.rs:133-144)
This function acquires self.conn.lock() at line 133, then calls self.execute() at line 140 which tries to lock the same Mutex again. Rust's std::sync::Mutex is not reentrant — this is a guaranteed deadlock. Currently marked #[allow(dead_code)], but if activated it will freeze the application. Fix: use the already-held conn reference directly (conn.execute(...)) instead of self.execute(...).

MEDIUM — unreachable!() on database data (scheduled_votes.rs:162)
The executed column read as integer hits unreachable!() for any value other than 0 or 1. A corrupted or manually edited database would panic the app. Consider replacing with a default fallback or proper error.

MEDIUM — QualifiedIdentity::from_bytes().expect() panic risk (identities.rs:184, 243, 297, 333, 361)
All identity loading functions call from_bytes() which uses .expect() internally. Corrupted blob = app crash on startup. Consider returning Result and skipping corrupted entries with a warning log.

[lklimek]

MEDIUM — Latent deadlock risk: wallets read lock held across conn lock

The wallets.read() lock (line 115) is held through the entire transaction (until line 153). This establishes a lock ordering of wallets.read() -> conn.lock(). If any future code path acquires these in reverse order (conn.lock() -> wallets.write()), it creates a deadlock.

Currently safe (no reverse ordering exists in the codebase), but fragile. The identities vector is already fully collected at line 116, so the wallets lock is no longer needed.

Suggested fix:

let identities = {
    let wallets = app_context.wallets.read().unwrap();
    self.get_local_qualified_identities(app_context, &wallets)?
};
// wallets lock released here, before conn.lock()
let mut conn = self.conn.lock().unwrap();

[lklimek]

CRITICAL — Guaranteed deadlock in insert_remote_identity_if_not_exists (identities.rs:133-144)

Function not used, I removed it in #559

MEDIUM — unreachable!() on database data (scheduled_votes.rs:162)
MEDIUM — QualifiedIdentity::from_bytes().expect() panic risk (identities.rs:184, 243, 297, 333, 361)

I created a separate issue, #560, to track these

[lklimek]

Please address that src/database/tokens.rs:118 and we'll be good to merge.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/database/tokens.rs (1)

568-581: ⚠️ Potential issue | 🟡 Minor

Silent error swallowing on parse failure — corrupted rows disappear without a trace.

If Identifier::from_vec fails for a stored row, it's silently skipped (lines 572-580). This contrasts with the rest of the file where parse failures are now propagated via map_err. At minimum, log a warning so data corruption doesn't go unnoticed.

Suggested fix

         while let Some(row) = rows.next()? {
             let token_id_bytes: Vec<u8> = row.get(0)?;
             let identity_id_bytes: Vec<u8> = row.get(1)?;
-            // Convert from raw bytes to an Identifier
-            if let Ok(token_id) = Identifier::from_vec(token_id_bytes) {
-                if let Ok(identity_id) = Identifier::from_vec(identity_id_bytes) {
-                    result.push((token_id, identity_id));
-                } else {
-                    // If for some reason it fails to parse, skip it or handle error
-                }
-            } else {
-                // If for some reason it fails to parse, skip it or handle error
-            }
+            let token_id = Identifier::from_vec(token_id_bytes).map_err(|e| {
+                rusqlite::Error::ToSqlConversionFailure(
+                    format!("Failed to parse token_id in token_order: {}", e).into(),
+                )
+            })?;
+            let identity_id = Identifier::from_vec(identity_id_bytes).map_err(|e| {
+                rusqlite::Error::ToSqlConversionFailure(
+                    format!("Failed to parse identity_id in token_order: {}", e).into(),
+                )
+            })?;
+            result.push((token_id, identity_id));
         }

🧹 Nitpick comments (2)

src/database/tokens.rs (2)

532-551: Inconsistent transaction API: unchecked_transaction() here vs transaction() in insert_token.

save_token_order uses conn.unchecked_transaction() (line 534) while insert_token uses conn.transaction() (line 121). unchecked_transaction skips the check that no transaction is already active, which could mask nested-transaction bugs. Since conn is obtained from a mutex and no outer transaction should exist, prefer transaction() for consistency and safety — just change the binding to let mut conn.

Suggested fix

     pub fn save_token_order(&self, all_ids: Vec<(Identifier, Identifier)>) -> rusqlite::Result<()> {
-        let conn = self.conn.lock().unwrap();
-        let tx = conn.unchecked_transaction()?;
+        let mut conn = self.conn.lock().unwrap();
+        let tx = conn.transaction()?;

---

266-268: InvalidParameterName is semantically wrong for data-parsing errors.

rusqlite::Error::InvalidParameterName is intended for SQL named-parameter lookup failures (e.g., :foo not found). Using it to wrap deserialization/parsing errors (here and at lines 344-349, 416-436) works at runtime but produces misleading error variants if callers pattern-match on the error kind.

Consider defining a custom error type or using rusqlite::Error::ToSqlConversionFailure(Box::new(...)) (already used elsewhere in this file, e.g., line 253) for consistency.