fix(db): clean orphaned FK rows and add startup consistency checks#816
Merged
Conversation
Users whose wallets were deleted while system SQLite had FK enforcement OFF retained orphaned child rows in wallet_transactions (and potentially shielded tables). The v33 rename_network_dash_to_mainnet UPDATE triggers FK re-validation under bundled SQLite (SQLITE_DEFAULT_FOREIGN_KEYS=1), causing "FOREIGN KEY constraint failed". Add clean_orphaned_fk_rows() step that safely removes orphans before the rename, handling tables that may not yet exist. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStartup now runs non-fatal DB consistency checks before reading schema version; v33 migration defers foreign keys, deletes orphaned child rows, NULLs dangling identity refs, and renames network values. A test injects orphaned rows into a v27-like DB and verifies migration, cleanup, and schema updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Startup
participant Init as DBInitialization
participant DB as SQLite
participant Migration as Migration_v33
Startup->>Init: begin initialization
Init->>DB: open connection
Init->>DB: run_consistency_checks() (PRAGMA quick_check, PRAGMA foreign_key_check)
Init->>DB: db_schema_version()
alt schema requires v33 migration
Init->>Migration: start v33 migration
Migration->>DB: BEGIN TRANSACTION
Migration->>DB: PRAGMA defer_foreign_keys = ON
Migration->>DB: clean_orphaned_fk_rows(tx)
Migration->>DB: add_core_wallet_name_column(tx) / rename_network_dash_to_mainnet(tx)
Migration->>DB: perform remaining migration steps
Migration->>DB: COMMIT
end
Init->>Startup: initialization complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Run PRAGMA quick_check and PRAGMA foreign_key_check before migrations on every startup (skipped for first-time setup). Logs warnings for any b-tree corruption or FK violations but never blocks initialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lklimek
changed the title
Collaborator
Review GateCommit:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses migration failures caused by orphaned foreign-key rows left behind when SQLite FK enforcement was previously disabled, and adds non-fatal DB health checks at startup to surface corruption/FK issues earlier.
Changes:
- Add a v33 migration step to delete orphaned child rows before running the
network='dash' → 'mainnet'updates that re-validate FKs. - Run
PRAGMA quick_checkandPRAGMA foreign_key_checkon startup (logging warnings, never blocking startup). - Add a regression test that reproduces orphaned-FK migration failures and verifies cleanup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nsaction Add wallet_addresses to FK orphan deletion (seed_hash → wallet). For asset_lock_transaction, apply the intended ON DELETE SET NULL behavior: nullify identity_id and identity_id_potentially_in_creation where the referenced identity no longer exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/initialization.rs`:
- Around line 1048-1074: The code currently swallows iterator/row-decoding
errors by using filter_map(|r| r.ok()) on the result of stmt.query_map; change
this to collect the iterator into a Result<Vec<_>, _> (e.g.
rows.collect::<Result<Vec<_>, _>>()) or explicitly iterate over rows,
returning/logging the first Err so iteration/decoding failures are surfaced;
update the block that constructs violations (currently named violations) to
handle Err from rows collection and treat it as a query failure (log with
tracing::warn including the error) instead of pretending the check succeeded.
- Around line 950-956: The wallet_children list in initialize-orphan-cleanup
misses the asset_lock_transaction table so orphaned asset-lock rows are not
deleted; add ("asset_lock_transaction", "wallet_seed_hash") to the
wallet_children array in initialization.rs (the same place where
("shielded_notes", "wallet_seed_hash") etc. are defined) and update the
test_v33_migration_with_orphaned_fk_rows test to either add a scenario with
orphaned asset_lock_transaction rows or assert that PRAGMA foreign_key_check
returns no rows after running rename_network_dash_to_mainnet()/the v33 migration
to ensure no dangling FKs remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4007ac58-1964-4078-849d-a96c660c5dce
📒 Files selected for processing (1)
src/database/initialization.rs
…lationships Extend clean_orphaned_fk_rows to cover every FK constraint in the schema: - wallet children: wallet_addresses, wallet_transactions, platform_address_balances, shielded_notes, shielded_wallet_meta, asset_lock_transaction, identity - identity children: top_up, scheduled_votes, identity_order, identity_token_balances, token_order - identity SET NULL: asset_lock_transaction.identity_id columns - token/contract/contested_name cascades Add table_exists helper and per-table logging of cleaned rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move clean_orphaned_fk_rows to the top of the v33 migration step, before any ALTER TABLE or CREATE TABLE operations that might trigger FK re-validation on orphaned rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/database/initialization.rs (1)
1134-1144:⚠️ Potential issue | 🟡 MinorDo not drop
foreign_key_checkrow errors.
filter_map(|r| r.ok())can turn a partial read into “no violations”, so this warning path can underreport FK problems or log a false clean result.🔎 Preserve iteration failures
}) { - Ok(rows) => { - let violations: Vec<_> = rows.filter_map(|r| r.ok()).collect(); - if violations.is_empty() { - tracing::debug!("Database foreign_key_check passed — no violations"); - } else { - tracing::warn!( - "Database foreign_key_check found {} violation(s):", - violations.len() - ); - for (table, rowid, parent, fk_idx) in &violations { - tracing::warn!( - " FK violation: {table} rowid={rowid} -> {parent} (fk_index={fk_idx})" - ); - } - } - } + Ok(rows) => match rows.collect::<Result<Vec<_>, _>>() { + Ok(violations) => { + if violations.is_empty() { + tracing::debug!("Database foreign_key_check passed — no violations"); + } else { + tracing::warn!( + "Database foreign_key_check found {} violation(s):", + violations.len() + ); + for (table, rowid, parent, fk_idx) in &violations { + tracing::warn!( + " FK violation: {table} rowid={rowid} -> {parent} (fk_index={fk_idx})" + ); + } + } + } + Err(e) => { + tracing::warn!("Database foreign_key_check iteration failed: {e}"); + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/initialization.rs` around lines 1134 - 1144, The code is discarding row iteration errors by using filter_map(|r| r.ok()) on the result of stmt.query_map; change the logic around stmt.query_map(...) so you collect/propagate iteration errors instead of silently dropping them—e.g., collect the iterator into a Result<Vec<_>, _> (or iterate and push Result items into a Vec<Result<...>>) and then if any Err is present log/return the error and treat it as a violation; update the handling around the violations variable and the branch that checks violations.is_empty() to account for and report these iteration errors rather than ignoring them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/initialization.rs`:
- Around line 69-70: The rename_network_dash_to_mainnet migration must run with
FK checks deferred so parent updates don't fail; modify the code that calls
rename_network_dash_to_mainnet (the block around clean_orphaned_fk_rows and
rename_network_dash_to_mainnet) to disable or defer foreign key enforcement for
the duration of the rename: inside the same transaction (tx) issue the
appropriate PRAGMA to turn off foreign keys (or enable deferred checks) before
calling rename_network_dash_to_mainnet(tx) and restore the original setting
afterwards, ensuring the PRAGMA calls bracket the rename and run in the same
connection so children in tables like contestant and token (and parent tables
contested_name and contract) aren’t temporarily orphaned.
---
Duplicate comments:
In `@src/database/initialization.rs`:
- Around line 1134-1144: The code is discarding row iteration errors by using
filter_map(|r| r.ok()) on the result of stmt.query_map; change the logic around
stmt.query_map(...) so you collect/propagate iteration errors instead of
silently dropping them—e.g., collect the iterator into a Result<Vec<_>, _> (or
iterate and push Result items into a Vec<Result<...>>) and then if any Err is
present log/return the error and treat it as a violation; update the handling
around the violations variable and the branch that checks violations.is_empty()
to account for and report these iteration errors rather than ignoring them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d57bea4-6429-4b95-8e77-e9b01d285e91
📒 Files selected for processing (1)
src/database/initialization.rs
… cleanup ensure_wallet_columns_exist() ran ALTER TABLE wallet_addresses before the migration system, triggering FK re-validation on orphaned rows before clean_orphaned_fk_rows had a chance to run. This was the actual cause of the user-reported migration failure. Move add_wallet_balance_columns (v16) and add_address_total_received_column (v17) into the v33 migration arm, after orphan cleanup. Both are idempotent (check column existence first) so safe to re-run. Remove the pre-migration ensure_wallet_columns_exist() call from initialize(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The v16/v17 migration steps already add these columns sequentially before v33 runs. The idempotent re-add in v33 was unnecessary — any database reaching v33 has already passed through v16 and v17. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation The v0.9.0 release created scheduled_votes without a network column. The v6 migration (update_scheduled_votes_table) assumed it existed, causing migration failure for v0.9.0 users upgrading to v1.0. Fix: check if scheduled_votes_old has a network column before copying data. If missing, default to 'dash' (the only network at v0.9.0). Add test_migration_from_v090_to_current that creates the exact v0.9.0 schema at DB version 5, populates realistic data, and migrates all the way to current version — verifying data survives with correct network rename. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ging CMT-1: Add PRAGMA defer_foreign_keys = ON before rename_network_dash_to_mainnet. Tables contestant and token have composite FKs that include network — updating parent tables first would temporarily break child FK references. CMT-2: PRAGMA quick_check can return multiple rows. Replace query_row with prepare + query_map to capture all issues, with bounded logging. CMT-3: Replace filter_map(|r| r.ok()) with explicit error handling in foreign_key_check. Row decode errors are now logged instead of silently dropped. Both checks cap output at 20 issues to avoid log spam. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/scheduled_votes.rs`:
- Around line 97-100: The migration SQL that copies rows from
scheduled_votes_old inserts a hardcoded network value 'dash', which causes
migrated rows to be skipped by current network filters like get_scheduled_votes;
update the INSERT SELECT in scheduled_votes.rs (the statement that INSERTs INTO
scheduled_votes ... SELECT ... 'dash' FROM scheduled_votes_old) to use 'mainnet'
as the fallback network instead of 'dash' so legacy rows are recorded under the
current default network.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 863fcb88-cbe3-47f5-996d-e8e7a6d6c0fa
📒 Files selected for processing (2)
src/database/initialization.rssrc/database/scheduled_votes.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/database/initialization.rs
Add per-step debug logging to the v33 migration arm, per-table logging to rename_network_dash_to_mainnet (with error-level on failure), and version context to try_perform_migration error messages. This helps pinpoint exactly which statement causes FK constraint failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/database/initialization.rs`:
- Around line 34-36: The startup path removed the repair fallback for missing
wallet columns leaving ensure_wallet_columns_exist (in src/database/wallet.rs)
orphaned; restore it by invoking ensure_wallet_columns_exist() from the
initialization flow (call it after run_consistency_checks() and before querying
db_schema_version() so missing wallet columns are repaired before skipping
migrations), propagate/handle its Result (log and return Err on failure) and
ensure the ensure_wallet_columns_exist implementation remains idempotent;
alternatively, if you prefer folding it into migrations, move the same
idempotent repair logic into the migration step that applies at v33, but do not
leave ensure_wallet_columns_exist unreferenced without documentation.
- Around line 963-1074: In clean_orphaned_fk_rows(), replace vulnerable "WHERE
{fk_col} NOT IN (SELECT id FROM identity)" and similar "NOT IN (SELECT id FROM
token)" / composite "(data_contract_id, network) NOT IN (SELECT contract_id,
network FROM contract)" checks with NOT EXISTS forms to avoid NULL-subquery
issues; specifically update the DELETE statements for the identity children
(tables from identity_fk_delete: top_up, scheduled_votes, identity_order,
identity_token_balances, token_order) to use "WHERE NOT EXISTS (SELECT 1 FROM
identity WHERE identity.id = {fk_col})", update the token-related deletes on
identity_token_balances and token_order to use "WHERE NOT EXISTS (SELECT 1 FROM
token WHERE token.id = token_id)", and replace the contract-token delete to use
"WHERE NOT EXISTS (SELECT 1 FROM contract WHERE contract.contract_id =
token.data_contract_id AND contract.network = token.network)" (use the actual
column names used in that DELETE), leaving wallet.seed_hash and contested_name
queries unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 338816e5-613c-414f-b9b8-e6080d1f95ff
📒 Files selected for processing (1)
src/database/initialization.rs
…rror Introduce a structured MigrationError type that carries table name, operation details, and the underlying rusqlite::Error. This replaces the previous String-based error path in try_perform_migration and gives exact context when a migration step fails. Also adds automatic PRAGMA foreign_key_check diagnostics when a SQLITE_CONSTRAINT_FOREIGNKEY error is detected during migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CMT-2: Replace InvalidParameterName with InvalidQuery for the "schema version too new" error — semantically correct for a misuse condition rather than a parameter naming issue. CMT-3: Fix log_fk_violations to handle row decode errors (logged, capped at 3) and cap violation output at 50 entries. Early-return with explicit error messages on prepare/execute failures instead of silently dropping them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Tested by the user. |
shumkov
added a commit
that referenced
this pull request
Apr 12, 2026
…orm-wallet2 Merge feat/platform-wallet2 into the backport branch and fix 16 compile errors + 1 test failure from API divergence: - PlatformAddress import (backported #814 vs pw2 module structure) - FeatureGate import (missing after merge) - AppState field access: replace mainnet/testnet/devnet/local _app_context fields with network_contexts iteration (#814 lazy contexts vs pw2 restructure) - MigrationError conversions: chain .migration_err() on v34-v39 migration steps (pw2 error API vs #816 FK cleanup) - PlatformAddressBalances pattern: add missing `network` field - test_v33_migration_with_orphaned_fk_rows: adjust assertion — pw2 migration 37 drops/recreates wallet_transactions with per-account attribution, so v33 FK cleanup survivors are gone. Test verifies cleanup of other FK tables (wallet_addresses, etc.) which survive. 382 tests pass, 72 kittest pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
FOREIGN KEY constraint failederror during database migration v32→v33wallet_transactions,shielded_notes,shielded_wallet_meta) have FK towallet(seed_hash)withON DELETE CASCADE. When wallets were deleted while system SQLite had FK enforcement off, orphaned child rows remained. Bundled SQLite enables FK enforcement by default, so therename_network_dash_to_mainnet()UPDATE re-validates FKs on orphaned rows and fails.clean_orphaned_fk_rows()step in v33 migration that guards table existence (handles pre-shielded DBs) and deletes orphans before the network rename. All affected data is fully recoverable from network via resync.Add non-fatal database consistency checks on startup
PRAGMA quick_check(fast b-tree structure verification) andPRAGMA foreign_key_check(FK violation enumeration) before migrations.User story
Imagine you are a Dash Evo Tool user upgrading from an older version. Previously, when you deleted a wallet, some leftover transaction records stayed behind in the database (invisible to you, but still there). Now when the app tries to upgrade your database, it chokes on these orphaned records. With this fix, the upgrade silently cleans them up and completes normally — you never notice anything. Additionally, the app now checks database health on every startup and logs any issues it finds, making future problems easier to diagnose.
Test plan
test_v33_migration_fresh_install— passestest_v33_migration_from_v27— passestest_v33_migration_with_orphaned_fk_rows— new test: creates orphans with FK off, runs migration, verifies orphans removed + valid data survives withnetwork=mainnet🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
Tests