fix: drop legacy unique indexes on activity id that block wallet-scoped rows (#137) - #142
Open
coreyphillips wants to merge 1 commit into
Open
fix: drop legacy unique indexes on activity id that block wallet-scoped rows (#137)#142coreyphillips wants to merge 1 commit into
coreyphillips wants to merge 1 commit into
Conversation
Databases created before activity data became wallet-scoped can still carry idx_onchain_id and idx_lightning_id, which make an activity id globally unique and so contradict the PRIMARY KEY (wallet_id, id) both tables now use. A transaction visible to two wallet scopes (paying your own hardware wallet) then fails to insert, and since the watcher writes its snapshot in one transaction, the whole snapshot is rejected and the hardware wallet shows an empty activity list on every poll. Drop both indexes on every init: the statements that created them are gone from this crate, but CREATE ... IF NOT EXISTS never undid them, and running an older build against a migrated database can recreate them.
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.
Closes #137
Drop the legacy idx_onchain_id and idx_lightning_id unique indexes on activity DB init, so an activity id can exist once per wallet scope as the composite primary key intends.
Databases predating wallet-scoped activity data can still carry
idx_onchain_idandidx_lightning_id, unique indexes over the activity id alone. They contradict thePRIMARY KEY (wallet_id, id)both tables now use: an on-chain id is the txid, so a transaction visible to two wallet scopes (paying your own hardware wallet) cannot be stored twice. The watcher writes its snapshot in one transaction, so the collision rejects the entire snapshot, not just the colliding row, and the hardware wallet shows an empty activity list that never recovers. The statements that created the indexes were removed from the crate by the wallet-scoping commit (cc1ff06), but nothing ever dropped them from existing databases.What changed
ActivityDB::initializenow runs a newLEGACY_INDEX_DROP_STATEMENTSstep (DROP INDEX IF EXISTS idx_onchain_id/idx_lightning_id) unconditionally, after the table migrations and before the index creation loop, insrc/modules/activity/implementation.rssrc/modules/activity/tests.rs:test_init_drops_legacy_activity_id_unique_indexes(legacy indexes hand-created on a current-schema database, then a two-scope snapshot upsert),test_legacy_schema_migration_drops_activity_id_unique_indexes(pre-wallet-scoped schema plus both indexes), andtest_activity_tables_have_no_extra_unique_indexes(drift guard)extra_unique_index_namesreadsPRAGMA index_listand returns unique indexes whose origin is notpk, so the drift guard fails on any future non-primary-key unique index overactivities,onchain_activity, orlightning_activityHow to test
cargo test --lib modules::activity: 192 passed, 0 failed.LEGACY_INDEX_DROP_STATEMENTS.iter().take(0)and reran:test_init_drops_legacy_activity_id_unique_indexesfails on the leftover index, and restoring the loop makes it pass. That test is the one that covers the reported failure.cargo test(full suite): 496 passed, 11 failed. All 11 failures aremodules::blocktanktests hittingapi.stag.blocktank.to, which is unreachable from this sandbox; they are unrelated to this change.rustfmt --checkis clean on both touched files.cargo fmt --checkreports a pre-existing diff insrc/modules/activity/backup_migration.rs, which this change does not touch (reproduced againstHEADbefore the change).cargo clippy --all-targetsproduces no errors and no new warnings.Notes
Worth knowing which of the two migration paths is actually broken, since it narrows who is affected. I verified by disabling the drop loop and rerunning:
test_init_drops_legacy_activity_id_unique_indexesfails,test_legacy_schema_migration_drops_activity_id_unique_indexespasses. The reason is thatmigrate_activity_tables_to_wallet_primary_keysrenames the old tables and drops them, and SQLite drops a table's indexes with it, so a database coming from the pre-wallet-scoped schema sheds both indexes as a side effect. The affected population is databases that already carry the composite primary key (so the rebuild is skipped) yet still hold the indexes, which is what the reported regtest database looked like. The route there is running an older build against an already-migrated database: itsCREATE UNIQUE INDEX IF NOT EXISTSrecreates the index on the new composite table. That is why the drop is unconditional on every init rather than folded into the migration, and I kept the legacy-schema test even though it passes today, as it pins the rebuild behaviour the fix would otherwise mask. On the suggested audit: history showsidx_pre_activity_metadata_id,_address,_tx_id, andidx_activity_tags_tag_activitywere all redefined under the same name with awallet_idprefix, soCREATE INDEX IF NOT EXISTSsilently keeps a stale definition wherever the old one survives. All four sit on tables that the wallet-scoping migrations rebuild, and they are non-unique, so the effect is at worst a suboptimal index, never a rejected write. Dropping and recreating them on every init would rebuild indexes on each launch, so I left them alone.