bitcoindevkit / bitcoindevkit/bdk
Strengthen the reference changeset vectors in persist_test_utils
- Dominant language
- Rust
- Stars
- 1.1k
- Forks
- 483
- Avg merge
- 20d 3h
- Merged PRs (30d)
- 3
Description
The reference changeset vectors in `bdk_testenv::persist_test_utils` (added in #2012) currently only exercise the "insert brand-new key" path. The cases where a persistence backend actually breaks — overwriting a key that is already stored — are not covered, so a backend using plain `INSERT` (or `INSERT OR IGNORE`) instead of `REPLACE` / `ON CONFLICT DO UPDATE` passes the helper while silently dropping updates in production.
Follow-up to #2012; the `drop(store)` fix for the reopen phase is being handled separately.
### 1. `local_chain_changesets` — no reorg vector
The two changesets use disjoint heights (`910425`/`910426`, then `910427`), so the LWW overwrite path is never hit. Add a vector that re-inserts an existing height with a *different* hash.
### 2. `tx_graph_changesets` — no upsert vector
Changeset 2 introduces a brand-new `tx2`, so `last_seen` / `first_seen` / `last_evicted` are never re-persisted with a newer value for a txid that is already stored. Add a second entry for `tx1.compute_txid()` with a later `last_seen` to changeset 2.
(`keychain_txout_changesets` already covers this — `last_revealed` for `descriptor_ids[0]` goes `1` → `2`.)
### 3. Block removal `(height, None)` is currently untestable
Worth deciding before someone tries: the `None` case cannot just be added to `local_chain_changesets`. `local_chain::ChangeSet::from_sqlite` (`crates/chain/src/rusqlite_impl.rs`) deletes the row and therefore can never return a `None` entry, so `assert_persist_changesets`' strict `load() == merge(all changesets)` equality is unsatisfiable for deletions — a correct backend would fail.
Two options:
- document that `None` is out of scope for these vectors, or
- relax the comparison to compare *applied state* rather than raw merged changesets.
### 4. Minor: incoherent timestamps in `tx_graph_changeset1`
`last_seen: 1755416650` is earlier than `first_seen: 1755416655` for the same txid; changeset 2 is internally consistent, so it looks like a transposition. Harmless for a round-trip check, but these vectors are the published reference for third-party backend authors, and a backend that validates `first_seen <= last_seen` on write would reject them.
### 5. Consider splitting the helper
`assert_persist_changesets` only checks the accumulate-then-reload contract; it never persists the *same* changeset twice. Splitting into `assert_persists` and `assert_persists_idempotent` would let a backend opt into the stronger claim instead of leaving callers to guess which one is covered — relevant for the planned semilattice `LocalChain` changeset, where idempotency is the point.
Contributor guide
Research direction
Start in bdk_testenv::persist_test_utils at local_chain_changesets, tx_graph_changesets, and assert_persist_changesets; inspect crates/chain/src/rusqlite_impl.rs to understand why None entries cannot round-trip. Add overwrite and upsert vectors, correct the incoherent timestamps, and decide whether to document deletion scope or compare applied state. Consider whether splitting the helper is needed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, sqlite
- Domain
- databases, testing
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100