Add functions to test persistence by 110CodingP · Pull Request #2012 · bitcoindevkit/bdk

110CodingP · 2025-08-12T19:25:20Z

Description

Added a helper assert_persist_changesets to the testenv for testing custom persistence backends. Partially solves bdk_wallet#14 and might help with bdk_wallet#234.

Changelog notice

Added
   - `miniscript` feature to testenv which depends on `bdk_chain/miniscript` .
    - `assert_persist_changesets` to testenv to check `ChangeSet`s are persisted properly.
    - `tx_graph_changesets`, `keychain_txout_changesets`, `local_chain_changesets` which provide changesets
    to be used as arguments to `assert_persist_changesets`.
    - `create_test_tx`, `spk_at_index`,`parse_descriptor`, `create_txout` helpers to testenv for creating changesets for testing.
 Changed
   - default feature of testenv to also activate `miniscript` feature.

Todo

Add docs
Add tests to bdk_chain based on testenv utilities

Checklists

All Submissions:

I followed the contribution guidelines

New Features:

I've added tests for the new feature
I've added docs for the new feature

ValuedMammal · 2025-08-20T14:42:18Z

I tried re-running CI to no avail, maybe try force pushing.

evanlinjin · 2025-08-29T05:34:58Z

Great idea. ConceptACK

110CodingP · 2025-09-14T18:56:51Z

In 10c42dc I added a miniscript feature to testenv but I am not sure if this is the correct approach. Earlier I was including the miniscript feature of bdk_chain by default but that doesn't work since miniscript requires either std or no-std but cargo doesn't allow specifying the feature of a dependency of a dependency. Adding miniscript as a separate dependency also does not work since we require bdk_chain/miniscript. But the current solution is also not satisfactory since miniscript feature requires the std feature!

ValuedMammal · 2025-09-20T13:47:40Z

In 10c42dc I added a miniscript feature to testenv but I am not sure if this is the correct approach. Earlier I was including the miniscript feature of bdk_chain by default but that doesn't work since miniscript requires either std or no-std but cargo doesn't allow specifying the feature of a dependency of a dependency. Adding miniscript as a separate dependency also does not work since we require bdk_chain/miniscript. But the current solution is also not satisfactory since miniscript feature requires the std feature!

We could have the testenv default feature enable bdk_chain/default, which in turn enables miniscript/std. I'm not aware of anyone needing a miniscript/no-std feature for the testenv crate.

110CodingP · 2025-09-22T17:07:28Z

Should the miniscript feature introduced be removed? I was thinking if some users might not want bdk_chain/miniscript feature to be enabled?

110CodingP · 2025-09-24T08:24:51Z

miniscript feature still depends on std feature of testenv since ig it would be confusing otherwise like when miniscript is enabled but not std even though miniscript would activate bdk_chain/std. Made miniscript a default feature too. Sorry if I am overthinking 😅 .

notmandatory · 2025-09-24T20:07:02Z

~~Since this is currently based on the master branch should we re-assign this to the 3.0 milestone and then do a new PR to back port it to 2.2?~~ Nevermind, this is for the bdk crate so my question doesn't apply.

110CodingP · 2026-05-12T03:29:26Z

Thanks a lot @evanlinjin for the review!
I implemented them in 48912ee. The only major difference is the error handling part which is inspired from bdk_wallet#343.

Also for convenience the PR still contains changeset specific persistence tests ( persist_txgraph_changeset).

I will try to leverage arbitrary and add edge cases in later commits in this PR.

evanlinjin

Much better already.

evanlinjin · 2026-05-13T15:36:20Z


 [dev-dependencies]
 tempfile = "3"
+anyhow = { version = "1.0.102", default-features = false}


Suggested change

anyhow = { version = "1.0.102", default-features = false}

anyhow = { version = "1", default-features = false}

No need to explicitly define the minor and patch versions.

evanlinjin · 2026-05-13T15:53:31Z

@110CodingP How about we focus this PR on getting the API shape correct and introduce something like arbitrary in a separate PR?

evanlinjin · 2026-05-13T16:01:36Z

+pub enum PersistErr<C: Debug> {
+    /// ChangeSet Mismatch
+    ChangeSetMismatch {
+        /// the resulting changeset
+        got: Box<C>,
+        /// the expected changeset
+        expected: Box<C>,
+    },
+    /// Errors thrown by underlying persistence backend.
+    Persister(Box<dyn Err + 'static + Send + Sync>),
+}


This is overengineered. We don't need to programmatically inspect different failure types. These helpers are for tests - failure just needs to panic!/assert!.

assert_persist_changesets should not need a return type and just panic on failure.

The reason I changed to this approach is that
1.) We have more control on the error message in this case.
2.) I was initially thinking that throwing a panic! would help users identify the line number in the tests that caused the panic but then we can't expect the users to go into the library code right?
3.) Users can downcast the error and handle it accordingly?

Thanks for presenting the arguments, I have changed my stance.

Yes, however we can also just return a String as the error type. Currently, the error variants are missing context that are useful to have. We are reusing the ChangeSetMismatch for different test errors where it would be more useful to have context-specific error messages.

You are right about this - Result is better.

YAGNI. I don't see a usecase for this?

My updated recommendation

Return Result<(), String> or anyhow::Result<()> where the error string gives context of the error (check my other review comments for details).

110CodingP · 2026-05-17T14:27:17Z

@110CodingP How about we focus this PR on getting the API shape correct and introduce something like arbitrary in a separate PR?

Yup, agreed!

evanlinjin · 2026-05-29T14:49:45Z

+/// check if merged `ChangeSet` is returned.
+pub fn assert_persist_changesets<CS, Store, CreateStore, Initialize, Persist>(
+    create_store: CreateStore,
+    initialize: Initialize,


Suggested change

initialize: Initialize,

load: Load,

This makes it easier to reason about. As "loading from db" is the main operation we care about.

initialize(&mut store) reads like we are setting up the store, but then it returns a changeset which is confusing.

evanlinjin · 2026-05-29T14:53:10Z

+    let init_changeset = initialize(&mut store)?;
+
+    if init_changeset != CS::default() {
+        return Err(PersistErr::ChangeSetMismatch {
+            expected: Box::new(CS::default()),
+            got: Box::new(init_changeset),
+        });
+    }


The returned error does not explicitly convey what we are testing here. This is proof that PersistErr is not earning it's keep.

The returned error should state what was being tested. In this case - a newly created store should load an empty changeset.

evanlinjin · 2026-05-29T14:56:29Z

+    let mut merged_changeset = CS::default();
+
+    for changeset in changesets {
+        persist(&mut store, changeset)?;


It will be helpful to know which changeset index we failed on alongside the db error.

evanlinjin · 2026-05-29T15:09:45Z

+pub enum PersistErr<C: Debug> {
+    /// ChangeSet Mismatch
+    ChangeSetMismatch {
+        /// the resulting changeset
+        got: Box<C>,
+        /// the expected changeset
+        expected: Box<C>,
+    },
+    /// Errors thrown by underlying persistence backend.
+    Persister(Box<dyn Err + 'static + Send + Sync>),
+}


Thanks for presenting the arguments, I have changed my stance.

Yes, however we can also just return a String as the error type. Currently, the error variants are missing context that are useful to have. We are reusing the ChangeSetMismatch for different test errors where it would be more useful to have context-specific error messages.

You are right about this - Result is better.

YAGNI. I don't see a usecase for this?

My updated recommendation

Return Result<(), String> or anyhow::Result<()> where the error string gives context of the error (check my other review comments for details).

evanlinjin · 2026-05-29T15:28:57Z

+/// the persisted one. We then create another such dummy `ChangeSet`, persist it and load it to
+/// check if merged `ChangeSet` is returned.
+pub fn assert_persist_changesets<CS, Store, CreateStore, Initialize, Persist>(
+    create_store: CreateStore,


Suggested change

create_store: CreateStore,

init: Init,

CreateStore conveys "make a brand new store". However, this closure's job is actually "give me a db handle", or in other words "Open or create".

Another note is that this test does not actually test a reopen. I propose having three sections to this test:

Load cs from a newly created db. The cs should be empty.

Interweave cs persists and loads. Ensure persists succeeds and loaded changesets are expected.

Reopen the db, load cs from db. Ensure cs is expected.

evanlinjin · 2026-05-29T15:32:29Z

+/// We create a dummy `ChangeSet`, persist it and check if loaded `ChangeSet` matches
+/// the persisted one. We then create another such dummy `ChangeSet`, persist it and load it to
+/// check if merged `ChangeSet` is returned.


This does not look correct. changesets are provided to this method, not created by this method.

We should mention what exactly is being tested. I.e. load from newly created db, interleaved persists and loads of changesets, load after reopen should succeed.

We should also explain the responsibilities of Init, Load and Persist.

Sorry I forgot to update the docs. Done in de27ded .

evanlinjin

We are close!

Compilation issues without miniscript feature.
Incomplete docs.
Various nits.

evanlinjin · 2026-07-24T03:23:27Z

+
+        let init_changeset = load(&mut store).map_err(|err| format!("Encountered an error from the persister while loading from the new store.\nGot:\n{}",err))?;
+
+        if init_changeset != C::default() {


Nit

Suggested change

if init_changeset != C::default() {

if !init_changeset.is_empty() {

evanlinjin · 2026-07-24T04:20:32Z

+    Persist: Fn(&mut Store, &C) -> Result<(), Box<dyn Err>>,
+{
+    let mut merged_changeset = C::default();
+    {


Suggested change

{

Unnecessary block.

~~Ok, so do you want me to use drop instead for dropping the store?~~ Got it, we don't need to drop the store.

noahjoeris

LGTM 🚀

Agree with Evan's last comments.

110CodingP · 2026-08-01T13:02:49Z

Ready for next review!

tvpeter

Weldone @110CodingP,

I have left some minor comments.

evanlinjin

This review was generated by Claude Code and posted from my account. I haven’t hand-verified every claim — treat it as machine-generated input, not a maintainer sign-off. Findings already raised elsewhere (@tvpeter’s drop(store) suggestion, the OP_CHECKLOCKTIMEVERIFY typo, and “Won’t this always return true?” on utils.rs:119) are omitted.

Tested against cb1cec09 in three configs (-p bdk_file_store, -p bdk_chain --features rusqlite, --workspace --no-default-features --features bdk_chain/hashbrown). All 6 new tests pass.

Opened #2284 for the test-vector gaps so they don’t block this PR.

`file_store` dev-dep now downloads bitcoind + esplora binaries

bdk_testenv = { path = "../testenv", features = ["serde"] } keeps defaults, which pull in download → bitcoind/esplora binaries. cargo test -p bdk_file_store now compiles and downloads those; previously it only needed tempfile. Offline/sandboxed runs break.

bdk_testenv = { path = "../testenv", default-features = false, features = ["std", "serde", "miniscript"] }

(Separate from @noahjoeris’ crate-choice suggestion.)

Wrong crate’s feature in `#[cfg]`

#[cfg(feature = "miniscript")] checks bdk_chain’s feature, but guards a symbol gated on bdk_testenv’s. They only align today by accident. A consumer enabling bdk_chain/miniscript with bdk_testenv defaults off gets an unresolved import instead of a skipped test.

`zip` silently truncates in `create_test_tx`

Four independent iterators are zipped. Mismatched lengths quietly produce fewer inputs/outputs (e.g. 2 txids + 1 vout → 1-input tx) with no warning. Failures then appear in unrelated tests. A debug_assert_eq! on lengths would catch this at the source.

Reference vectors only exercise inserts (tracked in #2284)

Not blocking, but third-party backends should know the gaps:

local_chain_changesets — disjoint heights only; no reorg (same height, different hash). Plain INSERT still passes.
tx_graph_changesets — changeset 2 adds a new tx, so existing txids never get updated last_seen/first_seen. Upsert path (INSERT OR IGNORE) is untested. (keychain_txout already covers this.)
Block removal (height, None) can’t be added: from_sqlite deletes the row and never returns None, so strict load() == merge(...) fails for correct backends. Document as out of scope, or compare applied state.
persist_test_utils.rs:144 — last_seen < first_seen for the same txid (looks like a swap). Harmless for round-trips, but backends that validate the ordering will reject it.
Idempotency untested. Same changeset is never persisted twice. Split into assert_persists / assert_persists_idempotent for the stronger claim.

evanlinjin

ACK 0e7f694

`assert_persist_changesets` persists each changeset in turn, checks that loading returns their aggregate, then reopens the store and checks again. It panics on any mismatch or backend error. Also add `utils` helpers that build non-empty `tx_graph`, `keychain_txout` and `local_chain` changesets, and a `miniscript` feature (on by default) that the keychain changeset needs. Co-Authored-By: 志宇 <hello@evanlinjin.me> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: 志宇 <hello@evanlinjin.me> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…esets` Co-Authored-By: 志宇 <hello@evanlinjin.me> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

evanlinjin

ACK c537631

github-project-automation Bot added this to BDK Chain Aug 12, 2025

LagginTimes assigned 110CodingP Aug 13, 2025

110CodingP force-pushed the add_persist_test_utils branch 3 times, most recently from 8f06dc6 to 247d478 Compare August 15, 2025 11:53

110CodingP marked this pull request as ready for review August 15, 2025 13:13

notmandatory mentioned this pull request Aug 18, 2025

refactor!: Implement generics for CheckPoint, LocalChain, and spk_client types #1582

Merged

3 tasks done

notmandatory moved this to Needs Review in BDK Chain Aug 19, 2025

110CodingP force-pushed the add_persist_test_utils branch from 247d478 to 83b54a6 Compare August 19, 2025 05:33

110CodingP force-pushed the add_persist_test_utils branch from 83b54a6 to 51d148e Compare August 20, 2025 15:44

ValuedMammal added this to BDK Wallet Aug 21, 2025

ValuedMammal moved this to Needs Review in BDK Wallet Aug 21, 2025

ValuedMammal added this to the Wallet 2.2.0 milestone Aug 21, 2025

oleonardolima added new feature New feature or request tests labels Aug 29, 2025

oleonardolima reviewed Sep 12, 2025

View reviewed changes

Comment thread crates/testenv/src/persist_test_utils.rs Outdated

Comment thread crates/testenv/src/persist_test_utils.rs Outdated

Comment thread crates/testenv/src/persist_test_utils.rs Outdated

Comment thread crates/testenv/src/persist_test_utils.rs Outdated

110CodingP force-pushed the add_persist_test_utils branch 2 times, most recently from f2623db to b77a114 Compare September 14, 2025 18:14

110CodingP force-pushed the add_persist_test_utils branch 2 times, most recently from 1eee95b to fd9429c Compare September 24, 2025 08:22

ValuedMammal reviewed Sep 24, 2025

View reviewed changes

Comment thread crates/testenv/Cargo.toml

ValuedMammal removed this from BDK Wallet Sep 25, 2025

ValuedMammal removed this from the Wallet 2.2.0 milestone Oct 1, 2025

110CodingP force-pushed the add_persist_test_utils branch from 0d0ac5b to 48912ee Compare May 12, 2026 03:01

110CodingP requested a review from evanlinjin May 12, 2026 18:24

evanlinjin mentioned this pull request May 13, 2026

feat!: Add #[non_exhaustive] to ChangeSet structs #1980

Closed

3 of 8 tasks

evanlinjin requested changes May 13, 2026

View reviewed changes

110CodingP force-pushed the add_persist_test_utils branch 2 times, most recently from b0b2cb2 to 75fa86d Compare May 17, 2026 17:09

110CodingP requested a review from evanlinjin May 25, 2026 04:38

evanlinjin requested changes May 29, 2026

View reviewed changes

110CodingP force-pushed the add_persist_test_utils branch from 75fa86d to de27ded Compare June 21, 2026 16:21

evanlinjin self-requested a review July 14, 2026 14:24

evanlinjin requested changes Jul 24, 2026

View reviewed changes

noahjoeris reviewed Jul 25, 2026

View reviewed changes

Comment thread crates/file_store/Cargo.toml Outdated

110CodingP force-pushed the add_persist_test_utils branch from 7058eba to cb1cec0 Compare August 1, 2026 08:17

tvpeter reviewed Sep 5, 2026

View reviewed changes

Comment thread crates/testenv/src/persist_test_utils.rs

Comment thread crates/testenv/src/utils.rs Outdated

Comment thread crates/testenv/src/utils.rs Outdated

evanlinjin mentioned this pull request Sep 10, 2026

Strengthen the reference changeset vectors in persist_test_utils #2284

Open

evanlinjin reviewed Sep 10, 2026

View reviewed changes

evanlinjin force-pushed the add_persist_test_utils branch from cb1cec0 to 0e7f694 Compare October 16, 2026 11:04

evanlinjin requested a review from a team as a code owner October 16, 2026 11:04

evanlinjin approved these changes Oct 16, 2026

View reviewed changes

110CodingP and others added 3 commits October 19, 2026 01:21

test(chain): test rusqlite persistence with assert_persist_changesets

206f9d0

Co-Authored-By: 志宇 <hello@evanlinjin.me> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test(file_store): test Store persistence with `assert_persist_chang…

c537631

…esets` Co-Authored-By: 志宇 <hello@evanlinjin.me> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

evanlinjin force-pushed the add_persist_test_utils branch from 93be1ea to c537631 Compare October 18, 2026 14:24

evanlinjin approved these changes Oct 18, 2026

View reviewed changes

evanlinjin requested a review from notmandatory October 18, 2026 14:35

	anyhow = { version = "1.0.102", default-features = false}
	anyhow = { version = "1", default-features = false}


		let init_changeset = load(&mut store).map_err(\|err\| format!("Encountered an error from the persister while loading from the new store.\nGot:\n{}",err))?;

		if init_changeset != C::default() {

	if init_changeset != C::default() {
	if !init_changeset.is_empty() {

Conversation

110CodingP commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog notice

Todo

Checklists

All Submissions:

New Features:

Uh oh!

ValuedMammal commented Aug 20, 2025

Uh oh!

evanlinjin commented Aug 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

110CodingP commented Sep 14, 2025

Uh oh!

ValuedMammal commented Sep 20, 2025

Uh oh!

110CodingP commented Sep 22, 2025

Uh oh!

110CodingP commented Sep 24, 2025

Uh oh!

Uh oh!

notmandatory commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

110CodingP commented May 12, 2026

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented May 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

My updated recommendation

Uh oh!

110CodingP commented May 17, 2026

Uh oh!

evanlinjin May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

My updated recommendation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

110CodingP commented Aug 12, 2025 •

edited

Loading

notmandatory commented Sep 24, 2025 •

edited

Loading

evanlinjin May 29, 2026 •

edited

Loading

110CodingP Jul 30, 2026 •

edited

Loading

evanlinjin left a comment •

edited

Loading

`file_store` dev-dep now downloads bitcoind + esplora binaries

Wrong crate’s feature in `#[cfg]`

`zip` silently truncates in `create_test_tx`