feat: migrate bridgesync claim data into claimsync DB on startup

[joanestebanr]

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

Test plan

• TestImportDataFromBridgesyncer_NoTables – bridge DB with no tables: no-op, no claimDB created
• TestImportDataFromBridgesyncer_EmptyTables – tables present but empty: no-op, no claimDB created
• TestImportDataFromBridgesyncer_Success – verifies row counts per table
• TestImportDataFromBridgesyncer_Idempotent – second call does not duplicate rows
• TestImportDataFromBridgesyncer_MissingRequiredMigration – error when bridgesync0012 absent
• TestImportDataFromBridgesyncer_OldSchemaNoHash – old schema without optional columns, block.hash defaults to ''
• TestImportKeyValueFromBridgesyncer_NoTable, _EmptyTable, _Success, _Idempotent

🤖 Generated with Claude Code

[claude]

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

---

PR Review Complete ✅

---

🚀 What's New

This PR adds functionality to migrate bridgesync claim data into the claimsync DB on startup. The implementation includes:

• ImportDataFromBridgesyncer: Copies block, claim, set_claim, and unset_claim tables from bridgesync to claimsync DB
• ImportKeyValueFromBridgesyncer: Migrates key_value rows with owner field replacement
• Both functions are idempotent (INSERT OR IGNORE) and handle old schema versions gracefully via column detection
• Migration guard validates bridgesync0012 has been applied before import
• Integration in cmd/run.go for both L1 and L2 scenarios

🐛 Bug Fixes

None

📋 Config Updates

None - uses existing BridgeL1Sync.DBPath, ClaimL1Sync.DBPath, BridgeL2Sync.DBPath, ClaimL2Sync.DBPath configuration paths.

⚠️ Breaking Changes

None - this is additive functionality that runs automatically on startup when conditions are met.

---

Code Review Feedback

🟢 Strengths

1. Excellent backward compatibility handling: The column detection logic (lines 81-96 in import_data_from_bridgesyncer.go) gracefully handles different schema versions
2. Strong idempotency guarantees: INSERT OR IGNORE ensures safe re-runs
3. Comprehensive test coverage: All major edge cases covered including empty tables, missing migrations, old schemas, and idempotency
4. Smart no-op behavior: Doesn't create claimDB when no data exists (lines 50-52)
5. Migration guard: Validates bridgesync0012 before import (lines 175-186)
6. Proper resource management: Deferred cleanup of connections and transactions

🟡 Suggested Improvements

1. SQL Injection via table name in PRAGMA (claimsync/storage/import_data_from_bridgesyncer.go:212)

   • Issue: fmt.Sprintf(\PRAGMA bridge.table_info(%s)`, tableName)is vulnerable to SQL injection iftableName` is ever user-controlled
   • Severity: Low (currently only called with hardcoded values)
   • Fix: Use parameterized queries or whitelist validation

   // Validate tableName is in allowed list
   allowedTables := map[string]bool{"block": true, "claim": true, "set_claim": true, "unset_claim": true}
   if !allowedTables[tableName] {
       return false, fmt.Errorf("invalid table name: %s", tableName)
   }

2. File path injection in ATTACH DATABASE (claimsync/storage/import_data_from_bridgesyncer.go:75)

   • Issue: fmt.Sprintf(\ATTACH DATABASE 'file:%s' AS bridge`, bridgeDBFilename)` could be vulnerable if filename contains SQL special characters
   • Severity: Low (paths come from config)
   • Recommendation: Add validation or use SQLite's URI escaping

   // Validate that bridgeDBFilename doesn't contain SQL special characters
   if strings.ContainsAny(bridgeDBFilename, "'\";\x00") {
       return fmt.Errorf("invalid characters in database path")
   }

3. Error handling for RowsAffected (claimsync/storage/import_data_from_bridgesyncer.go:246, 281, 294, 307)

   • Issue: n, _ := result.RowsAffected() silently ignores errors
   • Recommendation: Log if RowsAffected returns an error for observability

   n, err := result.RowsAffected()
   if err != nil {
       logger.Warnf("failed to get rows affected count: %v", err)
       n = 0
   }

4. Transaction rollback error handling (claimsync/storage/import_data_from_bridgesyncer.go:102)

   • Issue: defer tx.Rollback() //nolint:errcheck - could mask issues
   • Recommendation: Consider logging rollback errors in defer

   defer func() {
       if err := tx.Rollback(); err != nil && !errors.Is(err, sql.ErrTxDone) {
           logger.Warnf("failed to rollback transaction: %v", err)
       }
   }()

5. Context not passed to ImportKeyValueFromBridgesyncer (claimsync/storage/import_data_from_bridgesyncer.go:321, cmd/run.go:1004)

   • Issue: ImportKeyValueFromBridgesyncer creates its own context.Background() instead of accepting ctx parameter
   • Impact: Cannot be cancelled during shutdown
   • Recommendation: Add context parameter to match ImportDataFromBridgesyncer signature

   func ImportKeyValueFromBridgesyncer(ctx context.Context, bridgeDBFilename string, claimDBFilename string, owner string) error

6. Missing logger parameter in ImportKeyValueFromBridgesyncer (claimsync/storage/import_data_from_bridgesyncer.go:320)

   • Issue: Creates its own logger instead of accepting one like ImportDataFromBridgesyncer does
   • Recommendation: Add logger parameter for consistency

7. Test uses non-atomic file operation (claimsync/storage/import_data_from_bridgesyncer_test.go:161, 221, 242)

   • Issue: os.Stat followed by os.IsNotExist check is not atomic
   • Recommendation: This is fine for tests, but document the race condition if it matters

🟢 Optional / Style

1. Comment clarity (claimsync/storage/import_data_from_bridgesyncer.go:16-17)

   • The comment says "must all exist" but it's checking if all are present. Consider clarifying: "are the bridgesync tables that must exist for import to proceed"

2. Error message formatting (throughout)

   • Consistently excellent error wrapping with context

3. Test organization (import_data_from_bridgesyncer_test.go)

   • Tests are well-organized with helper functions
   • Consider adding a test for concurrent import calls (though INSERT OR IGNORE should handle this)

✅ Security Analysis

• SQL Injection Risk: Low - all user-controlled data uses parameterized queries except PRAGMA and ATTACH (see suggestions above)
• Path Traversal Risk: Low - paths come from configuration, not user input
• Resource Exhaustion: Well handled - connections and transactions properly cleaned up
• Race Conditions: Safe - idempotent operations with proper transaction isolation

✅ Integration Review (cmd/run.go)

Lines 141-142 and 156-160 properly trigger import before syncers start:

• L1: Runs when BRIDGE or L1BRIDGESYNC components active
• L2: Runs when AGGSENDER, AGGSENDERVALIDATOR, AGGCHAINPROOFGEN, BRIDGE, L2CLAIMSYNC, or L2BRIDGESYNC active
• Fatal errors on failure (appropriate for startup migration)
• Passes correct syncer IDs (L1ClaimSyncer / L2ClaimSyncer)

✅ Test Coverage Verification

All test cases from PR description are present:

• ✅ TestImportDataFromBridgesyncer_NoTables (line 134)
• ✅ TestImportDataFromBridgesyncer_EmptyTables (line 148)
• ✅ TestImportDataFromBridgesyncer_Success (line 165)
• ✅ TestImportDataFromBridgesyncer_Idempotent (line 187)
• ✅ TestImportDataFromBridgesyncer_MissingRequiredMigration (line 301)
• ✅ TestImportDataFromBridgesyncer_OldSchemaNoHash (line 320)
• ✅ TestImportKeyValueFromBridgesyncer_NoTable (line 207)
• ✅ TestImportKeyValueFromBridgesyncer_EmptyTable (line 225)
• ✅ TestImportKeyValueFromBridgesyncer_Success (line 245)
• ✅ TestImportKeyValueFromBridgesyncer_Idempotent (line 278)

---

Summary

This is a well-designed and thoroughly tested migration implementation. The code demonstrates strong engineering practices with proper error handling, resource cleanup, and backward compatibility. The suggested improvements are minor and mostly relate to defensive hardening rather than functional issues.

Recommendation: ✅ Approve with minor suggestions

The 🟡 suggested improvements are worth addressing but don't block merging, as the current implementation is safe given the context (config-sourced paths, controlled execution environment).

---

[sonarqubecloud]

Quality Gate failed

Failed conditions
4 Security Hotspots

See analysis details on SonarQube Cloud