storage: fix the nine incorrect queries from the postgres audit - #178
Open
bennyhodl wants to merge 12 commits into
Open
storage: fix the nine incorrect queries from the postgres audit#178bennyhodl wants to merge 12 commits into
bennyhodl wants to merge 12 commits into
Conversation
fetch_one on a DELETE with no RETURNING always returns RowNotFound, so every call errored and rolled back before deleting anything. Use execute instead, removing contract_data before contract_metadata so the foreign key never blocks the delete.
BDK's local chain maps height to hash, and a reorg replaces the hash at a height. Keying block rows by hash let both hashes accumulate at one height and the reader picked one nondeterministically, so the wallet could resurrect an orphaned block. The anchor_tx foreign key also made reorg deletes roll back the whole persist transaction; anchors carry their block in the JSONB payload and may reference blocks that left the sparse chain, so the constraint was wrong. The migration deduplicates existing rows, preferring the hash an anchor references.
BDK's merge rule for last_revealed keeps the greater index. A plain UPDATE let a stale changeset write a smaller index, and after a restart the wallet re-revealed already-used addresses. Clamp with GREATEST.
The tx_graph changeset also carries first_seen and last_evicted timestamps, and the keychain indexer carries a derived-spk cache. All three were dropped on persist while the staged changeset was cleared, so the data was permanently lost; losing last_evicted can resurrect an RBF-replaced or evicted transaction as unconfirmed after a restart. Upserts follow the BDK merge rules: first_seen only decreases, last_evicted only increases.
A changeset can carry last_seen for a txid whose row does not exist yet; the plain UPDATE affected zero rows and silently dropped the value. Insert-or-update, clamped with GREATEST since last_seen only ever increases.
update_contract did a SELECT-then-INSERT/UPDATE, which races under concurrent updates (both see missing, one dies on the unique violation), and its insert arm hardcoded is_offer_party = false and fee_rate_per_vb = 1 - corrupted rows then leaked into get_contract_offers, which filters on is_offer_party. Replace the branch with INSERT ... ON CONFLICT DO UPDATE and bind the contract's real values via a new Contract::get_fee_rate_per_vb accessor.
Both were plain INSERTs, so re-staging either (the wallet re-create path) hit a unique violation and rolled back the whole persist transaction. Descriptors and network never change for a wallet, so DO NOTHING is the correct conflict action and preserves last_revealed.
A fresh keychain row read back last_revealed = 0, which BDK interprets as index 0 already revealed, so the wallet skipped its first address. Existing rows at 0 are ambiguous and left untouched.
The address handlers dropped the persist result, so a failed persist lost the revealed derivation index invisibly and enabled address reuse after a restart. Return the error to the caller instead.
Consumers had to point sqlx-cli at the migration files deep inside the crate source to apply or revert the schema. The migrations are already compiled in via sqlx::migrate!, so expose the Migrator: MIGRATOR.run applies up and MIGRATOR.undo reverts, against any database, with no access to the source tree.
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
1 participant
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
Fixes the nine incorrect queries found in the Postgres storage audit (added here as
docs/postgres-storage-audit.md, alongside the BDK wallet improvement plan). Each audit finding is its own commit with a regression test.Changes
delete_contractnow deletes: it calledfetch_oneon aDELETEwith noRETURNING, so it always returnedRowNotFoundand rolled backblockby(wallet_name, height)to match BDK's height→hash model, dedupes reorg leftovers, and drops theanchor_tx → blockforeign key (anchors carry their block in the JSONB payload and may reference blocks no longer in the sparse chain; the FK made reorg deletes poison the persist transaction)last_revealedis clamped withGREATESTso a stale write can no longer regress the derivation index (address reuse)first_seen,last_evicted(newtxcolumns), and the keychain indexerspk_cache(new table), following BDK merge rules (first_seen only decreases, last_evicted/last_seen only increase)last_seenis upserted instead of updated, so a value arriving before its tx row exists is no longer silently droppedupdate_contractis a single atomicINSERT ... ON CONFLICT DO UPDATEinstead of a racy SELECT-then-branch, and binds realis_offer_party/fee_rate_per_vbvalues via a newContract::get_fee_rate_per_vb()accessor instead of hardcoding theminsert_descriptor/insert_networkare idempotent (ON CONFLICT DO NOTHING), so re-staging no longer poisons the persist transactionlast_revealed DEFAULT 0, which made fresh wallets skip derivation index 0let _ =Testing
last_revealed, timestamp/spk-cache roundtrip, missing-rowlast_seen, real upsert metadata, and idempotent re-stagingddklib suite with thepostgresfeature (46 passed) andddk-managerlib suite (41 passed); clippy and fmt cleanblockprimary key — on databases with a largeblocktable (staging is ~843k rows) the first boot after upgrade will spend some time in the migration