Merge bitcoindevkit#1684: Refactor file store

54251a7 docs(file_store): Show how to overwrite original file during recovery (志宇)
52f2061 refactor(store)!: change Store's method and error names (nymius)
fc76d66 feat(store): add `Bincode` error variant to FileError enum (nymius)
3998788 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 bitcoindevkit#1517, and the `Store` should recover the previous changesets stored in the file store.

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

  Besides the fix for bitcoindevkit#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 bitcoindevkit#1517.
  Overrides bitcoindevkit#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 54251a7

Tree-SHA512: d41dee149af7d0c2eba4f0b84b360eb2aad2b5f3df2d3160de285370e637619c25156678ee84a12295c7d2410704182819f6c5c612f68f81556747ad7dd0eb17

README.md

@@ -42,7 +42,7 @@ The project is split up into several crates in the `/crates` directory:
 - [`wallet`](./crates/wallet): Contains the central high level `Wallet` type that is built from the low-level mechanisms provided by the other components
 - [`chain`](./crates/chain): Tools for storing and indexing chain data
 - [`persist`](./crates/persist): Types that define data persistence of a BDK wallet
-- [`file_store`](./crates/file_store): A (experimental) persistence backend for storing chain data in a single file.
+- [`file_store`](./crates/file_store): Persistence backend for storing chain data in a single file. Intended for testing and development purposes, not for production.
 - [`esplora`](./crates/esplora): Extends the [`esplora-client`] crate with methods to fetch chain data from an esplora HTTP server in the form that [`bdk_chain`] and `Wallet` can consume.
 - [`electrum`](./crates/electrum): Extends the [`electrum-client`] crate with methods to fetch chain data from an electrum server in the form that [`bdk_chain`] and `Wallet` can consume.
 

crates/file_store/src/entry_iter.rs

@@ -1,3 +1,4 @@
+use crate::StoreError;
 use bincode::Options;
 use std::{
     fs::File,
@@ -37,7 +38,7 @@ impl<T> Iterator for EntryIter<'_, T>
 where
     T: serde::de::DeserializeOwned,
 {
-    type Item = Result<T, IterError>;
+    type Item = Result<T, StoreError>;
 
     fn next(&mut self) -> Option<Self::Item> {
         if self.finished {
@@ -63,7 +64,7 @@ where
                         }
                     }
                     self.db_file.seek(io::SeekFrom::Start(pos_before_read))?;
-                    Err(IterError::Bincode(*e))
+                    Err(StoreError::Bincode(*e))
                 }
             }
         })()
@@ -80,29 +81,3 @@ impl<T> Drop for EntryIter<'_, T> {
         }
     }
 }
-
-/// Error type for [`EntryIter`].
-#[derive(Debug)]
-pub enum IterError {
-    /// Failure to read from the file.
-    Io(io::Error),
-    /// Failure to decode data from the file.
-    Bincode(bincode::ErrorKind),
-}
-
-impl core::fmt::Display for IterError {
-    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
-        match self {
-            IterError::Io(e) => write!(f, "io error trying to read entry {}", e),
-            IterError::Bincode(e) => write!(f, "bincode error while reading entry {}", e),
-        }
-    }
-}
-
-impl From<io::Error> for IterError {
-    fn from(value: io::Error) -> Self {
-        IterError::Io(value)
-    }
-}
-
-impl std::error::Error for IterError {}

crates/file_store/src/lib.rs

@@ -13,14 +13,16 @@ pub(crate) fn bincode_options() -> impl bincode::Options {
 
 /// Error that occurs due to problems encountered with the file.
 #[derive(Debug)]
-pub enum FileError {
+pub enum StoreError {
     /// IO error, this may mean that the file is too short.
     Io(io::Error),
     /// Magic bytes do not match what is expected.
     InvalidMagicBytes { got: Vec<u8>, expected: Vec<u8> },
+    /// Failure to decode data from the file.
+    Bincode(bincode::ErrorKind),
 }
 
-impl core::fmt::Display for FileError {
+impl core::fmt::Display for StoreError {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         match self {
             Self::Io(e) => write!(f, "io error trying to read file: {}", e),
@@ -29,14 +31,15 @@ impl core::fmt::Display for FileError {
                 "file has invalid magic bytes: expected={:?} got={:?}",
                 expected, got,
             ),
+            Self::Bincode(e) => write!(f, "bincode error while reading entry {}", e),
         }
     }
 }
 
-impl From<io::Error> for FileError {
+impl From<io::Error> for StoreError {
     fn from(value: io::Error) -> Self {
         Self::Io(value)
     }
 }
 
-impl std::error::Error for FileError {}
+impl std::error::Error for StoreError {}