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. https://github.com/bitcoindevkit/bdk/blob/d99b3ef4b449a5c7b8facf04db3cb9be829a0ac1/crates/file_store/src/store.rs#L14

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

https://github.com/bitcoindevkit/bdk/blob/d99b3ef4b449a5c7b8facf04db3cb9be829a0ac1/crates/file_store/src/store.rs#L73 https://github.com/bitcoindevkit/bdk/blob/d99b3ef4b449a5c7b8facf04db3cb9be829a0ac1/crates/file_store/src/store.rs#L162

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: d99b3ef4b449a5c7b8facf04db3cb9be829a0ac1
• 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.

https://github.com/bitcoindevkit/bdk/blob/d99b3ef4b449a5c7b8facf04db3cb9be829a0ac1/crates/file_store/src/store.rs#L135

https://github.com/bitcoindevkit/bdk/blob/d99b3ef4b449a5c7b8facf04db3cb9be829a0ac1/crates/file_store/src/store.rs#L238

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