feat: migrate bridgesync claim data into claimsync DB on startup

[joanestebanr]

🔄 Changes Summary

• Add ImportDataFromBridgesyncer which copies block, claim, set_claim and unset_claim rows from a bridgesync SQLite DB into the claimsync DB. Handles old schema versions (missing block.hash, claim.tx_hash, claim.block_timestamp, claim.type) gracefully via column detection.
• Add ImportKeyValueFromBridgesyncer which copies the single key_value row, replacing the owner field with the claimsync syncer ID.
• Both functions are idempotent (INSERT OR IGNORE) and no-ops when the source DB has no relevant data (claimDB is not created unless data exists).
• A migration guard validates that bridgesync0012 (the last migration touching the imported tables) has been applied before proceeding.
• In cmd/run.go, wire the migration via runImportFromBridgeSyncerIfNeeded at the call site, before both the bridge and claim syncers start:
  • L1: triggered when BRIDGE or L1BRIDGESYNC are active
  • L2: triggered when any of BRIDGE, L2BRIDGESYNC, L2CLAIMSYNC, AGGSENDER, AGGSENDERVALIDATOR or AGGCHAINPROOFGEN are active
• It removes the old tables from bridgesyncer

Cases:

#	Condition	Shuld Migrate	Outcome
0	Componentsdoesn't imply run Bridge syncer neither Claim sycner	❌	Skip
1	BridgeDB doesn't exist	❌	Skip
2	ClaimDB exists	❌	Skip. ClaimDB has data so no migration required
3	BridgeDB doesn't have claims tables or they are empty	❌	Skip. No data to migrate
4	BridgeDB migrations below bridgesync0012	⚠️	Abort. It's not possible to migrate it, update to an intermediate version
5	All conditions met	✅	Migration runs

⚠️ Breaking Changes

None

📋 Config Updates

None

✅ Testing

• Unit tests
• Manually upgrade v0.8.2 to this version

🐞 Issues

• Closes feat: claimsyncer: migrate data from bridgesyncer #1546

[joanestebanr]

@claude

[claude]

Claude finished @joanestebanr's task in 1m 27s —— View job

---

Code Review Complete ✓

---

🚀 What's New

This PR adds automatic migration of claim-related data from the legacy bridgesync SQLite database into the new claimsync database on startup. The migration handles schema evolution gracefully (supporting old bridgesync schemas missing optional columns like block.hash, claim.tx_hash, etc.) and is fully idempotent. A new migration (bridgesync0015) removes the now-unused claim tables from bridgesync after the data has been migrated.

🐛 Bug Fixes

None - this is a new feature to handle data migration during the bridgesync → claimsync transition.

📋 Config Updates

None - migration is triggered automatically based on which components are running.

⚠️ Breaking Changes

None - the migration is backward-compatible and handles databases at different schema versions.

---

Code Review Feedback

🟢 Strengths

1. Excellent test coverage: 83.6% coverage on new code with comprehensive test cases covering edge cases (empty tables, missing migrations, old schemas, idempotency).
2. Robust schema compatibility: The column detection logic (bridgeColumnExists) elegantly handles evolution of the bridgesync schema across versions.
3. Clear separation of concerns: InspectBridgeSyncer → BridgeSyncerStatus.Validate() → BridgeSyncerStatus.ShouldMigrate() → ImportDataFromBridgesyncer provides a clean decision pipeline.
4. Idempotency: All imports use INSERT OR IGNORE ensuring the migration can be safely re-run.
5. Good error messages: The blocking error when migration is missing is actionable ("upgrade to intermediate version first").

🟡 Suggestions

1. Security: SQL Injection via fmt.Sprintf in PRAGMA query (claimsync/storage/import_data_from_bridgesyncer.go:262)

Issue: The bridgeColumnExists function constructs a PRAGMA query using fmt.Sprintf:

rows, err := conn.QueryContext(ctx, fmt.Sprintf(`PRAGMA bridge.table_info(%s)`, tableName))

While tableName is currently passed from trusted constants in this codebase, this pattern is vulnerable if the function is ever called with user-controlled input. SQLite's PRAGMA commands don't support parameter binding.

Recommendation: Add input validation to ensure tableName contains only alphanumeric characters and underscores:

func bridgeColumnExists(ctx context.Context, conn *sql.Conn, tableName, columnName string) (bool, error) {
    // Validate table name to prevent SQL injection
    if !regexp.MustCompile(`^[a-zA-Z0-9_]+$`).MatchString(tableName) {
        return false, fmt.Errorf("invalid table name: %s", tableName)
    }
    rows, err := conn.QueryContext(ctx, fmt.Sprintf(`PRAGMA bridge.table_info(%s)`, tableName))
    // ...
}

2. Error Handling: Deferred DETACH ignores errors (claimsync/storage/import_data_from_bridgesyncer.go:188)

Issue: The deferred DETACH DATABASE call has //nolint:errcheck, which could mask issues:

defer conn.ExecContext(ctx, `DETACH DATABASE bridge`) //nolint:errcheck

Recommendation: Log the error instead of silently ignoring it:

defer func() {
    if _, err := conn.ExecContext(ctx, `DETACH DATABASE bridge`); err != nil {
        logger.Warnf("failed to detach bridge database: %v", err)
    }
}()

3. Potential Race Condition: File existence check is not atomic (claimsync/storage/import_data_from_bridgesyncer.go:83-87)

Issue: The InspectBridgeSyncer function checks file existence separately from opening the database:

if _, err := os.Stat(bridgeDBFilename); err == nil {
    status.BridgeDBExists = true
}
// ... later ...
bdb, err := db.NewSQLiteDB(bridgeDBFilename)

In theory, the file could be deleted between the Stat check and the NewSQLiteDB call, though this is unlikely in practice.

Recommendation: Accept this as a known limitation or reverse the logic - attempt to open the DB first, and if successful, set BridgeDBExists = true. However, the current approach is reasonable for a startup migration check.

4. Code Quality: Inconsistent error wrapping style

Observation: Most errors use fmt.Errorf("context: %w", err), but some use format strings without context:

• Line 296: fmt.Errorf("ImportDataFromBridgesyncer: failed to import blocks: %w", err) ✓
• Line 329: fmt.Errorf("ImportDataFromBridgesyncer: failed to import claims: %w", err) ✓
• Line 209: fmt.Errorf("ImportDataFromBridgesyncer: failed to begin transaction: %w", err) ✓

Recommendation: Already consistent - good job! This is a strength, not an issue.

5. Migration Logic: Missing migration error could be more specific (claimsync/storage/import_data_from_bridgesyncer.go:117-122)

Issue: When the gorp_migrations table doesn't exist, the query error is silently ignored:

err = conn.QueryRowContext(ctx,
    `SELECT COUNT(*) FROM gorp_migrations WHERE id = $1`, requiredBridgeMigration).Scan(&migCount)
if err == nil {
    status.MigrationOK = migCount > 0
}

If the table doesn't exist, err != nil but we don't distinguish between "table doesn't exist" (migration definitely not applied) vs. other errors.

Recommendation: Explicitly handle the "table doesn't exist" case:

err = conn.QueryRowContext(ctx,
    `SELECT COUNT(*) FROM gorp_migrations WHERE id = $1`, requiredBridgeMigration).Scan(&migCount)
if err != nil {
    // Table doesn't exist or other error - treat as migration not applied
    // but log unexpected errors
    if !strings.Contains(err.Error(), "no such table") {
        logger.Warnf("unexpected error checking migration: %v", err)
    }
    status.MigrationOK = false
} else {
    status.MigrationOK = migCount > 0
}

6. Test Quality: Missing test case for concurrent migration attempts

Observation: The tests verify idempotency (running migration twice sequentially), but don't test concurrent access patterns.

Recommendation: Consider adding a test that simulates two processes attempting migration simultaneously to verify the INSERT OR IGNORE behavior handles race conditions correctly. This is low priority since SQLite handles this well with its transaction model.

🔴 Required Changes

None - All the suggestions above are recommended improvements, not blocking issues.

---

Summary

This is a well-implemented, production-ready PR. The code demonstrates:

• Solid engineering with comprehensive test coverage
• Thoughtful handling of schema evolution
• Clear error messages and logging
• Idempotent operations suitable for production rollout

The suggestions above are minor improvements that would further harden the code, but they don't block merging. The SQL injection concern in bridgeColumnExists is the highest priority item, though it's currently unexploitable since all callers use hardcoded table names.

Recommendation: ✅ Approve with optional follow-up to address the input validation suggestion.

---

[Copilot]

Pull request overview

Adds a startup-time migration to preserve historical claim indexing data by copying claim-related tables out of the legacy bridgesync SQLite DB into the claimsync DB, and then removing the legacy tables from bridgesync via a new migration.

Changes:

• Wire a startup hook in cmd/run.go to inspect bridgesync/claimsync DBs and run the import when appropriate (L1/L2).
• Add claimsync storage helpers to inspect bridgesync DB state and import block/claim/set_claim/unset_claim plus a key_value entry.
• Add bridgesync migration bridgesync0015 to drop legacy claim tables, with a test validating tables are removed.

Reviewed changes

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

Show a summary per file

File	Description
cmd/run.go	Calls the bridgesync → claimsync import before starting syncers based on active components.
claimsync/storage/import_data_from_bridgesyncer.go	Implements DB inspection and the actual data/key_value import logic.
claimsync/storage/import_data_from_bridgesyncer_test.go	Adds unit tests for the inspection and import helpers.
bridgesync/migrations/migrations_test.go	Adds coverage verifying the new bridgesync migration drops claim tables.
bridgesync/migrations/bridgesync0015.sql	Drops legacy claim tables from bridgesync (and recreates them on Down).

[Copilot]

The doc comment lists defaults as "." for several optional columns, but the implementation defaults to empty string ('') and 0 for timestamps. Please fix the comment so it reflects the real fallback values.

Suggested change

	// - block.hash - present since bridgesync migration 0003; defaults to ".
	// - claim.tx_hash - present since bridgesync migration 0002; defaults to ".
	// - claim.block_timestamp - present since bridgesync migration 0002; defaults to 0.
	// - claim.type - present since bridgesync migration 0012; defaults to ".
	// - block.hash - present since bridgesync migration 0003; defaults to ''.
	// - claim.tx_hash - present since bridgesync migration 0002; defaults to ''.
	// - claim.block_timestamp - present since bridgesync migration 0002; defaults to 0.
	// - claim.type - present since bridgesync migration 0012; defaults to ''.

[Copilot]

PR description lists additional tests (e.g., TestImportDataFromBridgesyncer_NoTables / _EmptyTables) but they are not present in this test file. Either add the missing cases or update the PR description/testing checklist to reflect what is actually covered.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud