Skip to content

Add functions to test persistence#2012

Open
110CodingP wants to merge 2 commits intobitcoindevkit:masterfrom
110CodingP:add_persist_test_utils
Open

Add functions to test persistence#2012
110CodingP wants to merge 2 commits intobitcoindevkit:masterfrom
110CodingP:add_persist_test_utils

Conversation

@110CodingP
Copy link
Copy Markdown
Contributor

@110CodingP 110CodingP commented Aug 12, 2025

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:

New Features:

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

@110CodingP 110CodingP force-pushed the add_persist_test_utils branch 3 times, most recently from 8f06dc6 to 247d478 Compare August 15, 2025 11:53
@110CodingP 110CodingP marked this pull request as ready for review August 15, 2025 13:13
@notmandatory notmandatory moved this to Needs Review in BDK Chain Aug 19, 2025
@110CodingP 110CodingP force-pushed the add_persist_test_utils branch from 247d478 to 83b54a6 Compare August 19, 2025 05:33
@ValuedMammal
Copy link
Copy Markdown
Collaborator

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

@110CodingP 110CodingP force-pushed the add_persist_test_utils branch from 83b54a6 to 51d148e Compare August 20, 2025 15:44
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Aug 21, 2025
@ValuedMammal ValuedMammal added this to the Wallet 2.2.0 milestone Aug 21, 2025
@evanlinjin
Copy link
Copy Markdown
Member

Great idea. ConceptACK

@oleonardolima oleonardolima added new feature New feature or request tests labels Aug 29, 2025
@110CodingP 110CodingP force-pushed the add_persist_test_utils branch 2 times, most recently from f2623db to b77a114 Compare September 14, 2025 18:14
@110CodingP
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Contributor Author

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

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

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
Copy link
Copy Markdown
Member

notmandatory commented Sep 24, 2025

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.

@ValuedMammal ValuedMammal removed this from the Wallet 2.2.0 milestone Oct 1, 2025
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Oct 1, 2025
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I made a couple comments and it needs a rebase and then I think it's ready to go.

Comment on lines +15 to +19
let db_tx = db.transaction()?;
tx_graph::ChangeSet::<ConfirmationBlockTime>::init_sqlite_tables(&db_tx)?;
let changeset = tx_graph::ChangeSet::<ConfirmationBlockTime>::from_sqlite(&db_tx)?;
db_tx.commit()?;
Ok(changeset)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be extracted into a function rather than being duplicated?

Comment on lines +22 to +24
let db_tx = db.transaction()?;
changeset.persist_to_sqlite(&db_tx)?;
Ok(db_tx.commit()?)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above.

Copy link
Copy Markdown
Contributor Author

@110CodingP 110CodingP Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6f09bde.

last_evicted: [(tx1.compute_txid(), 1755416660)].into(),
};

// persist and load
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ Docs should follow the rust style guidelines with first line starts with a capital letter and ends with a period.

https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Done in 70a3662.

@oleonardolima
Copy link
Copy Markdown
Collaborator

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 110CodingP force-pushed the add_persist_test_utils branch from c4b6873 to d56cea9 Compare April 1, 2026 11:41
@110CodingP 110CodingP force-pushed the add_persist_test_utils branch 2 times, most recently from 392160f to 6d1738b Compare April 1, 2026 12:19
Added the following functions to test if persistence of `bdk_chain` is
happening correctly.
  - `persist_txgraph_changeset`
  - `persist_indexer_changeset`
  - `persist_local_chain_changeset`
  - `persist_last_seen`, `persist_last_evicted`, `persist_first_seen`
  - `persist_txouts`
  - `persist_txs`
  - `persist_anchors`
  - `persist_last_revealed`
  - `persist_spk_cache`

Even though the first three tests cover every part of the `ChangeSet`s ,
the other tests are retained so as to help in unit testing.
@110CodingP 110CodingP force-pushed the add_persist_test_utils branch from 6d1738b to 6f09bde Compare April 1, 2026 12:41
@110CodingP
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request tests

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

5 participants