Appending changesets to `Store` after opening causes overwriting

[KnowWhoami]

Describe the bug

• let's say a Store instance initially contains changeset1 stored in its file.
  

  bdk/crates/file_store/src/store.rs

  Line 14 in d99b3ef

  pub struct Store<C>

• Then after opening the store instance using Store::open and appending a new changeset (changeset2) using append_changeset, the store will only contain changeset2, not changeset1 + changeset2.

bdk/crates/file_store/src/store.rs

Line 73 in d99b3ef

pub fn open<P>(magic: &[u8], file_path: P) -> Result<Self, FileError>

bdk/crates/file_store/src/store.rs

Line 162 in d99b3ef

pub fn append_changeset(&mut self, changeset: &C) -> Result<(), io::Error> {

To Reproduce

  #[test]
   fn test() {
       let temp_dir = tempfile::tempdir().unwrap();
       let file_path = temp_dir.path().join("db_file");
       let changeset1 = BTreeSet::from(["hello".to_string(), "world".to_string()]);
       let changeset2 = BTreeSet::from(["got the error".to_string()]);

       {
           // create new  db
           let mut db = Store::<TestChangeSet>::create_new(&TEST_MAGIC_BYTES, &file_path)
               .expect("must create");

           // append first changeset to db
           db.append_changeset(&changeset1).expect("must succeed");
       }

       {
           // open db
           let mut db = Store::<TestChangeSet>::open_or_create_new(&TEST_MAGIC_BYTES, &file_path)
               .expect("failed to load db");

           // now append the second changeset
           db.append_changeset(&changeset2).expect("must succeed");

           // Retrieve stored changesets from the database
           let stored_changesets = db
               .aggregate_changesets()
               .expect("must succeed")
               .expect("must succeed");

           // expected changeset must  be changeset2 + changeset1
           let mut expected_changeset = changeset2.clone();
           expected_changeset.extend(changeset1);

           // Assert that stored_changesets matches changeset2 but not expected_changeset
           assert_eq!(stored_changesets, changeset2);
           assert_ne!(stored_changesets, expected_changeset);
       }

       // Reason:
       //
       // On loading the pre-existing store instance,
       // it contains all the pre-appended changesets but the file pointer is positioned back to 12
       // i.e till the size of TEST_MAGIC_BYTES array.
       // thus on appending any new changeset to db -> it overwrites those past changesets
       // thus only contain the latest changeset.

       // Open the store again to verify file pointer position at the end of the file
       let mut db =
           Store::<TestChangeSet>::open(&TEST_MAGIC_BYTES, &file_path).expect("failed to load db");

       // get the current position of file pointer just after loading store
       let current_pointer = db.db_file.stream_position().expect("must suceed");

       assert_eq!(current_pointer, 12);

       // end pointer for the loaded db
       let expected_pointer = db.db_file.seek(SeekFrom::End(0)).expect("must succeed");

       // both doesn't match
       assert_ne!(current_pointer, expected_pointer);
   }

Expected behavior

• On opening the store -> its file pointer must be set to end.

Build environment

• BDK tag/commit: d99b3ef
• OS+version: Fedora 37
• Rust/Cargo version: rustc 1.81.0-nightly (f21554f7f 2024-06-08)
• Rust/Cargo target: nightly-2024-06-09-x86_64-unknown-linux-gnu

[storopoli]

@evanlinjin could this happen with the changes in #1514?

[evanlinjin]

Thanks for writing up this ticket.

The intended usecase of bdk_file_store is with BDK structures. With BDK, we never read after writing.

Maybe we can structure things to be more fool-proof. Need to think more on this.

[ValuedMammal]

Thanks @KnowWhoami. Maybe Store::open should always seek the file cursor to EOF? If this is truly a bug then I'm surprised all the tests and examples work how we expect using the file store

[KnowWhoami]

If this is truly a bug then I'm surprised all the tests and examples work how we expect using the file store

• while that's because in tests which considers opening the pre-existing Store -> we often run aggregate_changesets api that internally calls the iter_changesets api's which iterates over the stored changesets in order to collect all the stored changesets but it also changes the file pointer of Store and thus moves it to the end of file.

bdk/crates/file_store/src/store.rs

Line 135 in d99b3ef

/// **WARNING**: This method changes the write position of the underlying file. The next

bdk/crates/file_store/src/store.rs

Line 238 in d99b3ef

fn open_or_create_new() {

• Thus , it works fine if we append any new changesets after then.

[KnowWhoami]

The intended usecase of bdk_file_store is with BDK structures. With BDK, we never read after writing.
Maybe we can structure things to be more fool-proof. Need to think more on this.

• Yeah that's true, BDK wallet Api's don't care much about internal working of database , they just need to read the db to get required changesets and append's new one if found any. That's it.

• But since bdk_file_store is mainly developed for the downstream users so that they can directly used it in their BDK based wallets.
  So this bug should be taken into consideration.

[ValuedMammal]

I think it would help if we combine opening and loading into one read method that opens the file and returns the aggregate changeset, thereby ensuring the next write goes at the end of the file

[nymius]

I would like to work on this, but I've got some questions:

• There cannot be a situation where a user doesn't want to aggregate changesets?
• For any reason it's not desirable to store multiple changesets without aggregating them, like in the following succession?

1st changeset: open-append-close
2nd changeset: open-append-close
3rd changeset: open-append-close

I'm thinking in implementing something like the read method @ValuedMammal mentions above, but just using iter_changeset to verify the correctness of the stored changesets, without modifying them.
That would place the file pointer at the real end of the file, and check any existing changeset for write errors, leaving to the user the decision of aggregating changesets, or keep appending them until further decision.

[ValuedMammal]

Perhaps an easy fix is to change the file mode to append here instead of only write

bdk/crates/file_store/src/store.rs

Lines 73 to 77 in 2cf46a2

	pub fn open<P>(magic: &[u8], file_path: P) -> Result<Self, FileError>
	where
	P: AsRef<Path>,
	{
	let mut f = OpenOptions::new().read(true).write(true).open(file_path)?;

After quick testing this appears to fix the issue in the original post, however it caused other test failures, in particular last_write_is_short and write_after_short_read.

[nymius]

In particular, write_after_short_read is clearly proving what @evanlinjin stated above: never read after write . It asserts explicitly that the remaining changesets after the "short read" are overwritten.

[nymius]

This is the idea. I have tested it against current file store tests and a modification of the test above to check the expected behavior and it works.

Pros:

• Doesn't modify current API

Cons:

• Extra method
• Extra enum to wrap IterErrors inside FileError

diff --git a/crates/file_store/src/lib.rs b/crates/file_store/src/lib.rs
index 7c943ca2..236995e9 100644
--- a/crates/file_store/src/lib.rs
+++ b/crates/file_store/src/lib.rs
@@ -18,6 +18,8 @@ pub enum FileError {
     Io(io::Error),
     /// Magic bytes do not match what is expected.
     InvalidMagicBytes { got: Vec<u8>, expected: Vec<u8> },
+    /// Something went wrong while decoding file content
+    WrongEncoding(entry_iter::IterError)
 }
 
 impl core::fmt::Display for FileError {
@@ -29,6 +31,7 @@ impl core::fmt::Display for FileError {
                 "file has invalid magic bytes: expected={:?} got={:?}",
                 expected, got,
             ),
+            Self::WrongEncoding(e) => e.fmt(f)
         }
     }
 }
diff --git a/crates/file_store/src/store.rs b/crates/file_store/src/store.rs
index 62c3d91b..fdd7523c 100644
--- a/crates/file_store/src/store.rs
+++ b/crates/file_store/src/store.rs
@@ -92,6 +92,31 @@ where
         })
     }
 
+    /// Open [`Store`] and recover state after last write.
+    ///
+    /// Use [`open`] to open `Store` and manually handle the previous state if any.
+    /// Use [`create_new`] to create a new `Store`.
+    ///
+    /// # Errors
+    ///
+    /// If the prefixed bytes of the opened file does not match the provided `magic`, the
+    /// [`FileError::InvalidMagicBytes`] error variant will be returned.
+    ///
+    /// [`create_new`]: Store::create_new
+    pub fn rewind<P>(magic: &[u8], file_path: P) -> Result<Self, FileError>
+    where
+        P: AsRef<Path>,
+    {
+        let mut store = Self::open(magic, file_path)?;
+        for next_changeset in store.iter_changesets() {
+            if let Err(iter_error) = next_changeset {
+                return Err(FileError::WrongEncoding(iter_error));
+            }
+        }
+
+        Ok(store)
+    }
+
     /// Attempt to open existing [`Store`] file; create it if the file is non-existent.
     ///
     /// Internally, this calls either [`open`] or [`create_new`].