]> Untitled Git - bdk/commitdiff
Merge bitcoindevkit/bdk#1684: Refactor file store
author志宇 <hello@evanlinjin.me>
Thu, 6 Mar 2025 00:57:06 +0000 (11:57 +1100)
committer志宇 <hello@evanlinjin.me>
Thu, 6 Mar 2025 00:57:22 +0000 (11:57 +1100)
54251a7c9fdf08bced132be3851207917cbc36ab docs(file_store): Show how to overwrite original file during recovery (志宇)
52f2061fd94fb4f739779949675c707bb4fd0776 refactor(store)!: change Store's method and error names (nymius)
fc76d6603fa354386de8590368ab10881cbd949e feat(store): add `Bincode` error variant to FileError enum (nymius)
39987882b90b5ddaef3b5ebfc01c5c6e2cb6c0d7 fix(store): replace `Path.exists` by `OpenOptions.create_new` (nymius)

Pull request description:

  ### 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.

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  ### Checklists

  #### All Submissions:

  * ~~I've signed all my commits~~
  * ~~I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)~~
  * ~~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~~

ACKs for top commit:
  evanlinjin:
    ACK 54251a7c9fdf08bced132be3851207917cbc36ab

Tree-SHA512: d41dee149af7d0c2eba4f0b84b360eb2aad2b5f3df2d3160de285370e637619c25156678ee84a12295c7d2410704182819f6c5c612f68f81556747ad7dd0eb17


Trivial merge