feat(wallet): migrate legacy output key-ids to current format on startup (closes #7829)

[0xPepeSilvia]

Summary

Closes #7829.

On startup, the OutputManagerService now spawns a background task that converts any spending_key / script_private_key column values stored in the legacy "managed.<branch>.<index>" or "imported.<pubkey>" format to the current TariKeyId encoding.

Design choices

• LIKE filter - WHERE spending_key LIKE 'managed.%' OR ... LIKE 'imported.%' targets only rows that need migration. No full-table scan; no BLOB columns loaded (only id, spending_key, script_private_key).
• No offset paging - converted rows no longer match the filter, so the batch always starts at offset 0 and the result set shrinks naturally until empty.
• Batch size 100 per iteration keeps each SQLite round-trip bounded.
• Non-blocking - launched via tokio::spawn before the service event loop; wallet startup is not delayed.
• No catch_unwind - nothing in the migration path should panic; errors for individual rows are logged and skipped.

Files changed

File	Change
storage/database/backend.rs	Two new trait methods: fetch_outputs_with_legacy_key_ids / update_output_key_ids
storage/sqlite_db/output_sql.rs	OutputSql::find_outputs_with_legacy_key_ids (LIKE query, no BLOBs) + OutputSql::update_key_ids
storage/sqlite_db/mod.rs	Trait impl on OutputManagerSqliteDatabase
storage/database/mod.rs	Wrapper methods on OutputManagerDatabase<T>
service.rs	migrate_legacy_output_keys free async fn + tokio::spawn call in start()
tests/.../storage.rs	test_migrate_legacy_output_key_ids - injects "managed.comms.0", verifies LIKE filter detects it, verifies conversion to TariKeyId::SpendKey, verifies row is cleared from filter, verifies clean outputs are unaffected

Test

cargo test -p minotari_wallet test_migrate_legacy_output_key_ids

Passes locally.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a background migration task to convert legacy key-id strings in the output table to the current TariKeyId format. The review feedback highlights a critical issue where the migration loop is susceptible to an infinite loop and 100% CPU usage if any row fails to convert or update. To address this, the reviewer suggests implementing keyset pagination (using last_id) across the database backend, SQL queries, and tests. Additionally, a minor recommendation is made to add a short sleep between batch iterations to prevent SQLite database lock contention.

[gemini-code-assist]

The current migration loop is susceptible to an infinite loop if any row fails to convert or update. Because the query uses a LIKE filter without any paging or offset, any row that fails to convert (and thus still matches the LIKE filter) will be repeatedly fetched at the beginning of the next batch, causing the background task to loop indefinitely and consume 100% CPU.

To prevent this, we should use keyset pagination (cursor-based paging) by tracking the last_id processed and filtering the query with id > last_id. This ensures that we always make forward progress through the table, even if some rows fail to migrate.

/// Number of output rows processed per iteration of the legacy-key migration.
const LEGACY_KEY_MIGRATION_BATCH_SIZE: i64 = 100;

/// Background task that converts any `spending_key` / `script_private_key` column values stored in the legacy
/// "managed.<branch>.<index>" or "imported.<pubkey>" format to the current `TariKeyId` encoding.
///
/// The migration runs in a loop, fetching `LEGACY_KEY_MIGRATION_BATCH_SIZE` rows per iteration. It uses keyset
/// pagination (filtering by `id > last_id`) to avoid infinite loops if some rows fail to convert or update.
///
/// Errors for individual rows are logged and skipped; the migration continues so a single bad row cannot block the
/// rest. Errors fetching a batch cause the migration to abort early with a warning.
async fn migrate_legacy_output_keys<TBackend, TWalletConnectivity, TKeyManagerInterface>(
    resources: OutputManagerResources<TBackend, TWalletConnectivity, TKeyManagerInterface>,
) where
    TBackend: OutputManagerBackend + 'static,
    TWalletConnectivity: Send,
    TKeyManagerInterface: LegacyTransactionKeyManagerInterface,
{
    use std::str::FromStr;

    let mut total_migrated: usize = 0;
    let mut last_id = 0;
    loop {
        let batch = match resources
            .db
            .fetch_outputs_with_legacy_key_ids(last_id, LEGACY_KEY_MIGRATION_BATCH_SIZE)
        {
            Ok(b) => b,
            Err(e) => {
                warn!(
                    target: LOG_TARGET,
                    "Legacy key migration: failed to fetch next batch - aborting migration early: {e}"
                );
                return;
            },
        };

        if batch.is_empty() {
            break;
        }

        for (output_id, spending_key_str, script_key_str) in &batch {
            last_id = *output_id;

[gemini-code-assist]

Update the trait method signature to accept last_id to support keyset pagination.

    /// Fetch a batch of outputs whose `spending_key` or `script_private_key` columns still hold a legacy key-id
    /// string (i.e., one that starts with "managed." or "imported." and therefore cannot be parsed by
    /// `TariKeyId::from_str`). Only the three columns required for migration are returned; no BLOB columns
    /// (rangeproof, encrypted_data, etc.) are loaded. `batch_size` rows are returned per call, starting after `last_id`.
    fn fetch_outputs_with_legacy_key_ids(
        &self,
        last_id: i32,
        batch_size: i64,
    ) -> Result<Vec<(i32, String, String)>, OutputManagerStorageError>;

[gemini-code-assist]

Update the database wrapper method to pass last_id to the backend.

    /// See `OutputManagerBackend::fetch_outputs_with_legacy_key_ids`.
    pub fn fetch_outputs_with_legacy_key_ids(
        &self,
        last_id: i32,
        batch_size: i64,
    ) -> Result<Vec<(i32, String, String)>, OutputManagerStorageError> {
        self.db.fetch_outputs_with_legacy_key_ids(last_id, batch_size)
    }

[gemini-code-assist]

Update the backend implementation to pass last_id to the SQL query.

Suggested change

	fn fetch_outputs_with_legacy_key_ids(
	&self,
	batch_size: i64,
	) -> Result<Vec<(i32, String, String)>, OutputManagerStorageError> {
	let mut conn = self.database_connection.get_pooled_connection()?;
	OutputSql::find_outputs_with_legacy_key_ids(batch_size, &mut conn)
	}
	fn fetch_outputs_with_legacy_key_ids(
	&self,
	last_id: i32,
	batch_size: i64,
	) -> Result<Vec<(i32, String, String)>, OutputManagerStorageError> {
	let mut conn = self.database_connection.get_pooled_connection()?;
	OutputSql::find_outputs_with_legacy_key_ids(last_id, batch_size, &mut conn)
	}

[gemini-code-assist]

Update the SQL query to filter by id > last_id and order by id ASC to implement keyset pagination.

    /// Return up to `batch_size` outputs whose `spending_key` or `script_private_key` still contains a legacy key-id
    /// string - i.e., one that begins with `"managed."` or `"imported."` and therefore cannot be parsed by
    /// `TariKeyId::from_str`. Only the three columns needed by the migration are fetched; no BLOB columns are loaded.
    ///
    /// Callers should keep calling with `batch_size` until an empty vec is returned, passing the last processed ID
    /// to avoid infinite loops if some rows fail to convert.
    pub fn find_outputs_with_legacy_key_ids(
        last_id: i32,
        batch_size: i64,
        conn: &mut SqliteConnection,
    ) -> Result<Vec<(i32, String, String)>, OutputManagerStorageError> {
        outputs::table
            .select((outputs::id, outputs::spending_key, outputs::script_private_key))
            .filter(outputs::id.gt(last_id))
            .filter(
                outputs::spending_key
                    .like("managed.%")
                    .or(outputs::spending_key.like("imported.%"))
                    .or(outputs::script_private_key.like("managed.%"))
                    .or(outputs::script_private_key.like("imported.%")),
            )
            .order_by(outputs::id.asc())
            .limit(batch_size)
            .load::<(i32, String, String)>(conn)
            .map_err(OutputManagerStorageError::DieselError)
    }

[gemini-code-assist]

Update the test calls to pass 0 as the last_id parameter to match the updated signature.

    // Sanity check: no legacy keys yet.
    let before = db.fetch_outputs_with_legacy_key_ids(0, 100).unwrap();
    assert!(before.is_empty(), "expected no legacy keys before injection");

    // 2. Overwrite kmo1's spending_key with the legacy comms-key string. wallet_types constants: SPEND_KEY_BRANCH =
    //    "comms", VIEW_KEY_BRANCH = "data encryption".
    let legacy_key_str = "managed.comms.0";
    {
        let mut conn = connection.get_pooled_connection().unwrap();
        diesel::update(outputs::table.filter(outputs::commitment.eq(&kmo1.commitment.to_vec())))
            .set(outputs::spending_key.eq(legacy_key_str))
            .execute(&mut conn)
            .unwrap();
    }

    // 3. Migration query must find exactly the one injected row.
    let found = db.fetch_outputs_with_legacy_key_ids(0, 100).unwrap();
    assert_eq!(found.len(), 1, "expected exactly 1 output with a legacy spending_key");
    let (output_id, found_spending, found_script) = found.into_iter().next().unwrap();
    assert_eq!(found_spending, legacy_key_str);

    // 4. Parse -> convert -> update (mirrors what `migrate_legacy_output_keys` does per row).
    let legacy_id = LegacyTariKeyId::from_str(&found_spending).expect("must parse as LegacyTariKeyId");
    let current_id = key_manager
        .convert_legacy_tari_key_id_to_current(&legacy_id)
        .expect("conversion must succeed");
    // "managed.comms.0" must convert to TariKeyId::SpendKey.
    assert_eqblock(
        current_id,
        TariKeyId::SpendKey,
        "legacy managed.comms.0 should convert to TariKeyId::SpendKey"
    );

    let new_spending = current_id.to_string();
    db.update_output_key_ids(output_id, new_spending, found_script).unwrap();

    // 5. After the update the legacy filter must return nothing.
    let after = db.fetch_outputs_with_legacy_key_ids(0, 100).unwrap();
    assert!(after.is_empty(), "expected no legacy keys after migration");

[gemini-code-assist]

Since SQLite is a single-writer database, running a tight loop of database updates in a background task can cause database lock contention (SQLITE_BUSY errors) and starve the main event loop. Adding a small sleep at the end of each batch iteration yields control and allows other database connections in the pool to acquire the write lock.

            }
        }
        // Yield to other tasks to prevent SQLite database lock contention
        tokio::time::sleep(std::time::Duration::from_millis(50)).await;
    }

[SWvheerden]

you are not capturing all legacy types here:

managed.%
imported.%
derived.managed.%
derived.imported.%

etc, you need all