Refactor file store #1684

nymius · 2024-11-13T14:05:03Z

Description

The Store::open method doesn't recovers the previous Store state saved in the file and emplaces the file pointer just after the magic bytes prefix, this later is agravated by Store::append_changeset which sets the file pointer after the last written changeset. The combination of both causes the lost of any previous changeset there may have been in the file.

Is natural to think this shouldn't be the expected behavior, as @KnowWhoami pointed out in #1517, and the Store should recover the previous changesets stored in the file store.

The approach proposed in #1632 triggered a discussion about more changes in Store, which motivated the current refactor.

Besides the fix for #1517, the following methods have been changed/renamed/repurposed in Store:

create: create file and retrieve store, if exists fail.
load: load changesets from file, aggregate them and return aggregated changeset and Store. If there are problems with decoding, fail.
dump: aggregate all changesets and return them.
load_or_create: try load or fallback to create.

Fixes #1517.
Overrides #1632.

Notes to the reviewers

Load return type is Result<(Option<C>, Store), StoreErrorWithDump> to allow methods which doesn't use WalletPersister to get the aggregated changeset right away.

Dump is kept to allow WalletPersister::initialize method to retrieve the aggregated changeset without forcing the inclusion of the additional parameters needed by store::load to the trait, which would also affect the rusqlite implementation.

Changelog notice

Added:

StoreError enum, which includes Io, Bincode and InvalidMagicBytes.
"not intended for production" notice in general README for file store.

Changed:

Store::create_new to Store::create, with new return type: Result<Self, StoreError>
Store::open to Store::load, with new return type: Result<(Self, Option<C>), StoreErrorWithDump<C>>
Store::open_or_create to Store::load_or_create, with new return type: Result<(Option<C>, Self), StoreErrorWithDump<C>>
Store::aggregate_changesets to Store::dump, with new return type: Result<Option<C>, StoreErrorWithDump<C>>
FileError to StoreError
AggregateChangesetsError to StoreErrorWithDump, which now can include all the variants of StoreError in the error field.

Deleted:

IterError deleted.

Checklists

All Submissions:

~~I've signed all my commits~~
~~I followed the contribution guidelines~~
~~I ran cargo fmt and cargo clippy before committing~~

New Features:

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

Bugfixes:

~~This pull request breaks the existing API~~
~~I've added tests to reproduce the issue which are now passing~~
~~I'm linking the issue being fixed by this PR~~

ValuedMammal

I did a first pass. Overall it looks good. I also made suggestions in this commit ValuedMammal/bdk@6312ca3

If you plan to rebase make sure commits compile on their own. For example changing a method name and updating the method call sites should be in the same commit in my opinion.

crates/wallet/tests/wallet.rs

crates/file_store/src/store.rs

nymius · 2024-12-18T20:37:02Z

@ValuedMammal thanks for the review, I'm already working in the changes.

nymius · 2024-12-19T20:56:33Z

Rebased on 03a08bb
Haven't applied changes solicited in #1684 (comment) because now I find more valuable to report the error and allow the user to recover from it instead of returning whatever aggregated changeset we could parse in a best effort approach.

ValuedMammal · 2024-12-23T15:49:11Z

In 0f104cc:

error[E0432]: unresolved import `crate::FileError`
 --> crates/file_store/src/store.rs:1:41
  |
1 | use crate::{bincode_options, EntryIter, FileError, IterError};
  |                                         ^^^^^^^^^ no `FileError` in the root

For more information about this error, try `rustc --explain E0432`.
error: could not compile `bdk_file_store` (lib) due to 1 previous error

Maybe just squash in the next commit 8897a20 but keep the commit message from 8897a20 ?

nymius · 2024-12-23T16:32:07Z

In 0f104cc:

error[E0432]: unresolved import `crate::FileError`
 --> crates/file_store/src/store.rs:1:41
  |
1 | use crate::{bincode_options, EntryIter, FileError, IterError};
  |                                         ^^^^^^^^^ no `FileError` in the root

For more information about this error, try `rustc --explain E0432`.
error: could not compile `bdk_file_store` (lib) due to 1 previous error

Maybe just squash in the next commit 8897a20 but keep the commit message from 8897a20 ?

Thanks! should be fixed now. I checked each commit with:

 git rebase --exec 'cargo build; cargo test; cargo clippy; cargo fmt' 0f104c^

ValuedMammal

ACK 18aa75e I tested on example_esplora

nymius · 2025-01-15T14:50:48Z

@notmandatory we have been trying to figure out with @ValuedMammal what would be the correct handling for this change.

This semver FAQ entry implies it isn't a breaking change for wallet as long as we don't change the API, but this single line is a breaking change, so I think if we don't rollback the error name to the previous one, we should wait until wallet-2.0 .
If we rollback the name change to the old one, I think we can introduce all the changes in a minor version for wallet, and then move the name change to a future update closer to the wallet 2.0 release, or drop it altogether.

What do you think we should do?

nymius · 2025-01-23T22:25:09Z

I tried to just change the name of StoreErrorWithDump and roll it back to AggreagateChangesetsError but the field error of the former cannot be changed back to iter_error without breaking the logic of the new changes.

So I decided to go with the option to use bdk_file_store as a registry dependency instead of a path dependency to avoid the force version bump of bdk_wallet when merging these changes.
It showed up to be more complex than I thought initially and there are some weird things like bdk_core depending solely on bdk_testenv and bdk_chain and not in the actual crate. I need to think more about this solution, but is the only working so far.

cc: @notmandatory @ValuedMammal

nymius · 2025-02-28T05:29:18Z

#1860 allow this to be a breaking change without bumping bdk_wallet major version. I have squashed all commits addressing comments requesting changes and rebased onto latest b26ff89

evanlinjin

I like where this is going.

evanlinjin · 2025-02-28T05:44:13Z

crates/file_store/src/store.rs

    }

-    /// Attempt to open existing [`Store`] file; create it if the file is non-existent.
+    /// Aggregate [`Store`] changesets and return them as a single changeset.


Suggested change

/// Aggregate [`Store`] changesets and return them as a single changeset.

/// Dump the aggregate of all changesets in [`Store`].

crates/file_store/src/store.rs

crates/file_store/src/lib.rs

`Path.exists` is not safe against time-of-creation, time-of-use (TOCTOU) bugs. `OpenOptions.create_new` on the other hand is atomic, so not prone to this kind of problems.

The changes in this commit were motivated due to a bug in the `StoreFile` which caused old data to be lost if the file was `open` instead of created and new data was appended. The bugfix later motivated a general name cleanup in StoreFile's methods and errors and some minor changes in their signatures. FileError was renamed to StoreError, which now includes the IterError variants, allowing the remplacement of the former form. The new StoreFile methods are: - create: create file in write only mode or fail if file exists. - load: open existing file, check integrity of content and retrieve Store. - append: add new changesets to Store. Do nothing if changeset is empty. - dump: aggregate and retrieve all stored changesets in store. - load_or_create: load if file exists, create if not, and retrieve Store.

nymius · 2025-03-05T23:50:45Z

Rebased onto 362c3dc

evanlinjin

ACK 54251a7

e125014 feat(examples): Change `example_wallet_rpc` to detect evictions. (志宇) 12b00e0 feat(wallet): Add method `apply_evicted_txs` (valued mammal) a247215 deps!: update bdk_chain to 0.22.0 (valued mammal) f882945 ci: Unpin some dependencies (valued mammal) Pull request description: This PR updates `bdk_chain` dependency to the latest version 0.22.0 ### Notes to the reviewers This PR doesn't make use of `CanonicalizationParams` for any advanced use cases - instead we rely on the default params for all tx-graph queries. fixes #202 fixes #224 ### Changelog notice - deps: bump `bdk_chain` to 0.22.0 - deps: bump `bdk_file_store` to 0.20.0 #### Added - Added `start_sync_with_revealed_spks_at` - Added `start_full_scan_at` - Added `apply_evicted_txs` #### Changed - `start_sync_with_revealed_spks` is feature gated by "std", as it uses the system time to provide the time of the sync and also the tx last-seen. - `apply_update` no longer requires std, as the timestamp is provided in the sync / full scan request - `start_full_scan` is also gated behind "std" - `start_sync_*` now includes the expected SPK history (via `list_expected_spk_txids`) #### Removed - Removed `apply_update_at` #### BREAKING - `bdk_core::TxUpdate` is non-exhaustive bitcoindevkit/bdk#1838 - For the optional file_store feature, the Load variant of the `FileStoreError` is changed to hold a new inner type bitcoindevkit/bdk#1684 #### Changes to persisted data The `tx_graph::ChangeSet` gained a new default-able field `last_evicted` to persist the fact that a tx is no longer part of the canonical chain. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing * [x] I've added docs for the new feature * [x] This pull request breaks the existng API * [x] I'm linking the issue being fixed by this PR ACKs for top commit: notmandatory: ACK e125014 Tree-SHA512: 91f77e4268aac0aaee2503a3fdca13754fc291159b9f37b0997be8c011b02b8ac16b4b6a7edbbb731005c83b37d438666fb803dbe18a6b9496b11f7e3e1ab155

nymius mentioned this pull request Nov 13, 2024

Recover file store previous state at open #1632

Closed

8 tasks

notmandatory assigned nymius Nov 15, 2024

notmandatory added the module-database label Nov 15, 2024

notmandatory modified the milestone: 1.0.0-beta Nov 15, 2024

ValuedMammal reviewed Dec 17, 2024

View reviewed changes

crates/wallet/tests/wallet.rs Outdated Show resolved Hide resolved

crates/file_store/src/store.rs Outdated Show resolved Hide resolved

crates/file_store/src/store.rs Outdated Show resolved Hide resolved

crates/file_store/src/store.rs Outdated Show resolved Hide resolved

nymius force-pushed the bugfix/i-1517-store-append-after-open-causes-overwrite branch from 1251b82 to 5a7fe42 Compare December 19, 2024 20:45

nymius force-pushed the bugfix/i-1517-store-append-after-open-causes-overwrite branch from 5a7fe42 to 3b695ce Compare December 23, 2024 16:29

ValuedMammal approved these changes Jan 8, 2025

View reviewed changes

nymius added the api A breaking API change label Jan 15, 2025

nymius force-pushed the bugfix/i-1517-store-append-after-open-causes-overwrite branch from eed9e20 to 7d5f5dd Compare February 28, 2025 05:03

nymius requested review from ValuedMammal and evanlinjin February 28, 2025 05:29

evanlinjin requested changes Feb 28, 2025

View reviewed changes

nymius force-pushed the bugfix/i-1517-store-append-after-open-causes-overwrite branch from 7d5f5dd to ea151dd Compare March 3, 2025 05:05

nymius requested a review from evanlinjin March 3, 2025 05:24

nymius added 3 commits March 6, 2025 10:49

fix(store): replace Path.exists by OpenOptions.create_new

3998788

`Path.exists` is not safe against time-of-creation, time-of-use (TOCTOU) bugs. `OpenOptions.create_new` on the other hand is atomic, so not prone to this kind of problems.

feat(store): add Bincode error variant to FileError enum

fc76d66

nymius force-pushed the bugfix/i-1517-store-append-after-open-causes-overwrite branch from ea151dd to 52f2061 Compare March 5, 2025 23:50

docs(file_store): Show how to overwrite original file during recovery

54251a7

evanlinjin approved these changes Mar 6, 2025

View reviewed changes

evanlinjin merged commit 739b54f into bitcoindevkit:master Mar 6, 2025
23 checks passed

github-project-automation bot moved this from Needs Review to Done in BDK Wallet Mar 6, 2025

ValuedMammal mentioned this pull request Apr 3, 2025

Release wallet-1.2.0 #1923

Closed

41 tasks

ValuedMammal mentioned this pull request Apr 14, 2025

deps!: update bdk_chain to 0.22.0 bitcoindevkit/bdk_wallet#208

Merged

6 tasks

	/// Aggregate [`Store`] changesets and return them as a single changeset.
	/// Dump the aggregate of all changesets in [`Store`].

Refactor file store #1684

Refactor file store #1684

Uh oh!

Conversation

nymius commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Added:

Changed:

Deleted:

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nymius commented Dec 18, 2024

Uh oh!

nymius commented Dec 19, 2024

Uh oh!

ValuedMammal commented Dec 23, 2024

Uh oh!

nymius commented Dec 23, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

nymius commented Jan 15, 2025

Uh oh!

nymius commented Jan 23, 2025

Uh oh!

nymius commented Feb 28, 2025

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nymius commented Mar 5, 2025

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nymius commented Nov 13, 2024 •

edited

Loading