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

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

Description

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

Notes to the reviewers

An alternative approach could be to create a struct whose methods are these tests but the current approach seems better since these tests are meant to be used as unit tests. For the same reason redundant functions to check persistence of individual fields of the ChangeSets are provided.

Changelog notice

Added
   - `miniscript` feature to testenv which depends on `bdk_chain/miniscript` .
    - functions to testenv to check `ChangeSet`s are persisted and merged `ChangeSet`s are loaded properly ,gated by the `miniscript` and `std` feature flags.
    - tests to `file_store` and `chain` based on utilities added to `testenv`.
 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.

oleonardolima · 2026-03-19T21:53:07Z

I moved this to chain-0.24.0 milestone for visibility, @110CodingP let us know when it's ready for another round of review.

110CodingP · 2026-04-01T12:51:20Z

Since we use v13 of miniscript now, I finally added a miniscript feature to testenv and made it a default feature. The test-utils are now gated by miniscript and std features of testenv.

notmandatory · 2026-04-01T15:30:12Z

This PR will be important to have if we still want to do #1980 to help test persistence implementations.

tvpeter · 2026-04-06T04:44:26Z

+fn txgraph_is_persisted() {
+    persist_txgraph_changeset::<rusqlite::Connection, _, _, _>(
+        "wallet.sqlite",
+        |path| Ok(bdk_chain::rusqlite::Connection::open(path)?),


nit: Is there a reason you are using different connection types? from rusqlite::Connection and bdk_chain::rusqlite::Connection

tvpeter · 2026-04-06T04:49:58Z

+    let temp_dir = tempfile::tempdir().expect("must create tempdir");
+    let file_path = temp_dir.path().join(file_name);
+    let mut store = create_store(&file_path).expect("store should get created");


nit: seemed like every fn creates the store, could you refactor it into a separate fn?

tvpeter

Hi @110CodingP , this is incredible work on a massive PR.

I have left some nit comments.

In addition to the comments, I think it will also be great if you could consider including test utils for handling edge cases/conflicts, especially in the persist_test_utils (if that will not make the PR too big)

nymius

cACK 6f09bde

nymius · 2026-04-14T11:24:17Z

+    let mut tx_graph_changeset1 = ChangeSet::<ConfirmationBlockTime> {
+        txs: [tx1.clone()].into(),
+        txouts: [
+            (OutPoint::new(hash!("BDK"), 0), create_txout(1300, ADDRS[1])),
+            (
+                OutPoint::new(hash!("Bitcoin_fixes_things"), 0),
+                create_txout(1400, ADDRS[1]),
+            ),
+        ]
+        .into(),
+        anchors: [(conf_anchor, tx1.compute_txid())].into(),
+        last_seen: [(tx1.compute_txid(), 1755416650)].into(),
+        first_seen: [(tx1.compute_txid(), 1755416655)].into(),
+        last_evicted: [(tx1.compute_txid(), 1755416660)].into(),
+    };


Here we are also testing the persistence of all the other changesets. Being such a high level test, Is it worth it to have the granularity we have, i.e., a test for each of the smaller changesets?

I agree they are redundant. The reason I was doing it this way was that users will then be able to use these redundant tests as unit tests in case they have separate functions to persist each of the smaller changesets. I would be fine with removing them too if that increases maintainability.

nymius · 2026-04-14T11:27:27Z

+    let tx2 = Arc::new(create_test_tx(
+        [tx1.compute_txid()],
+        [0],
+        [20_000],
+        [ADDRS[0]],
+        1,
+        0,
+    ));


https://github.com/rust-fuzz/arbitrary could help remove work here, at the same time is exposing the API for fuzzers and other tests.

nymius · 2026-04-14T11:30:59Z

+        input: input_vec,
+        output: output_vec,
+    }
+}


Again, https://github.com/rust-fuzz/arbitrary could help with this.

110CodingP · 2026-04-14T16:26:32Z

@tvpeter and @nymius thank you for the reviews! I will restructure the PR to address the concerns. Also looking at mammal's restructuring of the wallet tests.
Will also try adding tests for the edge cases, the ones I can think of right now are the following:

persisting duplicate changesets should have no effect.
persisting empty changeset.

evanlinjin

Concept ACK

Thanks for working on this. Having some sort of tooling to ensure persistence implementations is not only helpful, but also crucial if we ever want to do #1980.

Approach NACK

There is quite a lot of boilerplate happening here and lots of repeated code making this approach hard to read and hard to maintain. All the test methods essentially all do the same thing, but with different changeset types. As mentioned by @nymius here, we should really abstract this.

Proposal

We can represent all that we want to test with a single generic helper. Then just have predefined sets of changesets (C).

pub fn assert_persists<C, S, CREATE, LOAD, PERSIST>(
    create: CREATE,
    load: LOAD,
    persist: PERSIST,
    changesets: &[C],
) where
      C: Default + Merge + PartialEq + Clone + core::fmt::Debug,
      CREATE: Fn(&Path) -> anyhow::Result<S>,
      LOAD: Fn(&mut S) -> anyhow::Result<C>,
      PERSIST: Fn(&mut S, &C) -> anyhow::Result<()>,
  {
      // Persist one at a time, reload after each, asserting merged accumulation.
      let dir = tempfile::tempdir().expect("tempdir");
      let mut store = create(dir.path()).expect("interleaved: create store");
      assert_eq!(
          load(&mut store).expect("interleaved: load empty"),
          C::default(),
      );

      let mut acc = C::default();
      for cs in changesets {
          persist(&mut store, cs).expect("interleaved: persist");
          acc.merge(cs.clone());
          assert_eq!(
              load(&mut store).expect("interleaved: reload"),
              acc,
          );
      }

      // Persist all into a fresh store, reload once.
      let dir = tempfile::tempdir().expect("tempdir");
      let mut store = create(dir.path()).expect("batched: create store");
      for cs in changesets {
          persist(&mut store, cs).expect("batched: persist");
      }
      assert_eq!(
          load(&mut store).expect("batched: reload"),
          acc,
      );
  }

Predefined test changesets via builders:

pub fn tx_graph_fixture() -> [tx_graph::ChangeSet<ConfirmationBlockTime>; 2] { … }
pub fn keychain_txout_fixture() -> [keychain_txout::ChangeSet; 2] { … }
pub fn local_chain_fixture() -> [local_chain::ChangeSet; 2] { … }

For fuzzing, proptest, we can even create methods that generates changeset sets on the fly.

codecov · 2026-05-11T18:24:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.47%. Comparing base (249413e) to head (48912ee).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2012      +/-   ##
==========================================
+ Coverage   76.63%   79.47%   +2.83%     
==========================================
  Files          29       29              
  Lines        5705     5705              
  Branches      257      257              
==========================================
+ Hits         4372     4534     +162     
+ Misses       1263     1103     -160     
+ Partials       70       68       -2

Flag	Coverage Δ
rust	`79.47% <ø> (+2.83%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Added the following functions to test if persistence of `bdk_chain` is happening correctly. - `persist_txgraph_changeset` - `persist_indexer_changeset` - `persist_local_chain_changeset` which leverage a more general `persist_changeset`. In order to not panic and instead return errors raised by the persistence backend being tested, introduced `PersistErr`. To ensure downcasting and to allow usage of anyhow, `PersisterErr::Persister` requires an error of with 'static + Send + Sync.

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:29:05Z

There is a whole bunch of functions in this file that is only called once. Let's just inline it - this will improve readability and reduce lines of code.

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

+#[test]
+fn txgraph_is_persisted() -> anyhow::Result<()> {
+    let temp_dir = tempfile::tempdir().unwrap();
+    Ok(persist_txgraph_changeset::<rusqlite::Connection, _, _, _>(


Nit: The explicit turbofishing is not really needed for any of these functions.

Suggested change

Ok(persist_txgraph_changeset::<rusqlite::Connection, _, _, _>(

Ok(persist_txgraph_changeset(

evanlinjin · 2026-05-13T15:34:27Z

+        Ok(persist_txgraph_changeset::<
+            Store<tx_graph::ChangeSet<ConfirmationBlockTime>>,
+            _,
+            _,
+            _,
+        >(


Suggested change

Ok(persist_txgraph_changeset::<

Store<tx_graph::ChangeSet<ConfirmationBlockTime>>,

_,

_,

_,

>(

Ok(persist_txgraph_changeset(

Turbofishing not necessary.

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:39:56Z

+/// 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.
+pub fn persist_changeset<CS, Store, CreateStore, Initialize, Persist>(


persist_changeset reads like "perform persistence" but this is a persistence-assertion-helper.

Suggested change

pub fn persist_changeset<CS, Store, CreateStore, Initialize, Persist>(

pub fn assert_persist_changesets<CS, Store, CreateStore, Initialize, Persist>(

evanlinjin · 2026-05-13T15:43:50Z

+
+/// Get two [`keychain_txout::ChangeSet`](keychain_txout::ChangeSet)s.
+#[cfg(feature = "miniscript")]
+fn keychain_txout_changesets() -> [keychain_txout::ChangeSet; 2] {


Make these methods that return fixtures of changesets pub. Then we can remove the persist_xxx_changeset methods. Much less code to maintain.

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.

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

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

oleonardolima added this to the Chain 0.24.0 milestone Mar 19, 2026

110CodingP force-pushed the add_persist_test_utils branch from c4b6873 to d56cea9 Compare April 1, 2026 11:41

110CodingP requested review from evanlinjin, luisschwab, nymius and tvpeter as code owners April 1, 2026 11:41

110CodingP force-pushed the add_persist_test_utils branch 3 times, most recently from 6d1738b to 6f09bde Compare April 1, 2026 12:41

tvpeter reviewed Apr 6, 2026

View reviewed changes

EliteCoder18 mentioned this pull request Apr 8, 2026

Improve CI persistence and MSRV testing, automate issue triage bitcoindevkit/bdk_wallet#410

Open

nymius reviewed Apr 14, 2026

View reviewed changes

evanlinjin moved this from Needs Review to In Progress in BDK Chain May 9, 2026

evanlinjin requested changes May 9, 2026

View reviewed changes

110CodingP force-pushed the add_persist_test_utils branch from 6f09bde to 0d0ac5b Compare May 11, 2026 18:18

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

8 tasks

evanlinjin requested changes May 13, 2026

View reviewed changes

	Ok(persist_txgraph_changeset::<rusqlite::Connection, _, _, _>(
	Ok(persist_txgraph_changeset(

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

	pub fn persist_changeset<CS, Store, CreateStore, Initialize, Persist>(
	pub fn assert_persist_changesets<CS, Store, CreateStore, Initialize, Persist>(

Conversation

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

Uh oh!

Description

Notes to the reviewers

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!

oleonardolima commented Mar 19, 2026

Uh oh!

110CodingP commented Apr 1, 2026

Uh oh!

notmandatory commented Apr 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

Uh oh!

nymius left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

110CodingP commented Apr 14, 2026

Uh oh!

evanlinjin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Concept ACK

Approach NACK

Proposal

Uh oh!

codecov Bot commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

110CodingP commented May 12, 2026

Uh oh!

evanlinjin left a comment

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!

Choose a reason for hiding this comment

Uh oh!

110CodingP commented Aug 12, 2025 •

edited

Loading

notmandatory commented Sep 24, 2025 •

edited

Loading

evanlinjin left a comment •

edited

Loading

codecov Bot commented May 11, 2026 •

edited

Loading