bitcoindevkit / bitcoindevkit/bdk

Strengthen the reference changeset vectors in persist_test_utils

Open
#2,284 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.