Replace bincode by postcard by nymius · Pull Request #2258 · bitcoindevkit/bdk

nymius · 2026-08-13T20:20:28Z

bincode is no longer maintained.
postcard is the closest maintained project with >52M downloads on crates.io, regular releases, and activity on its repo.

BREAKING CHANGE:

Magic Bytes should be changed to avoid accidentaly modifying old file store blobs.
The internal encoding has changed as a result of using postcard. Old file stores won't be recoverable using the latest file_store version.
As this is a development environment store, we don't provide migration utilities.
StoreError::Bincode has been renamed to StoreError::Decode, and now contains postcard::Errors
From now on, trailing bytes after decoding are rejected.
append always tries to attach changesets to the latest valid end of the file.

These changes were LLM assisted.

Changelog notice

Changed

Replaced bincode (unmaintained) with postcard for on-disk (de)serialization.
- BREAKING CHANGE: the on-disk format has changed. Existing store files are not readable by this
  version; callers should bump their magic bytes so old files fail fast with
  StoreError::InvalidMagicBytes instead of a decode error.
- BREAKING CHANGE: StoreError::Bincode(bincode::ErrorKind) is replaced by
  StoreError::Decode(postcard::Error).
BREAKING CHANGE: Entry length prefix changed from an 8-byte little-endian integer to a
postcard varint (1-10 bytes). Covered by the same on-disk format change and
magic-byte-bump guidance above.

Fixed

Store::append now seeks to the end of the file before writing, so appending through a
stale handle no longer overwrites changesets written via another handle. A failed append
also truncates the partial frame, leaving the file unchanged.
Entries whose payload doesn't consume its whole declared frame length are now rejected instead of ignored.

Checklists

All Submissions:

I followed the contribution guidelines

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

codecov · 2026-08-14T12:55:24Z

Codecov Report

❌ Patch coverage is 97.59036% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.65%. Comparing base (5108b5c) to head (eb5f3ea).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/file_store/src/entry_iter.rs	98.59%	1 Missing ⚠️
crates/file_store/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2258      +/-   ##
==========================================
+ Coverage   78.36%   78.65%   +0.28%     
==========================================
  Files          30       30              
  Lines        5945     6001      +56     
  Branches      281      286       +5     
==========================================
+ Hits         4659     4720      +61     
+ Misses       1210     1207       -3     
+ Partials       76       74       -2

Flag	Coverage Δ
rust	`78.65% <97.59%> (+0.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

chukwudiikeh · 2026-08-16T11:19:08Z

Breaking compatibility with older bincode-encoded stores without supplying a conversion tool leaves users stranded. Regardless of whether file_store is intended for dev environments, dropping support without providing a migration path or prior notice inevitably leads to silent data loss.

evanlinjin · 2026-08-17T02:31:07Z

@chukwudiikeh There is no data loss. Everything to persist in bdk_chain is blockchain data, which is always available in the blockchain.

evanlinjin

I haven't reviewed the tests yet.

EntryIter changes are well written and very thorough - handling all situations elegantly and returning useful errors.

I disagree with the Store::append changes as a corrupted tail becomes unrecoverable.

evanlinjin · 2026-08-17T03:34:35Z

+        let mut payload = Vec::new();
+        // Reserve exactly `len` bytes up front. Fail fast on a corrupt, oversized length prefix.
+        // Avoids unnecessary reads and allocations.
+        let alloc_failed = usize::try_from(len)
+            .map_err(|_| ())
+            .and_then(|len| payload.try_reserve_exact(len).map_err(|_| ()))
+            .is_err();


Is there no better way to write this? 😅

Could we use postcard::from_io instead?

A better way to write it:

let alloc_failed = match usize::try_from(len) { Ok(len) => payload.try_reserve_exact(len).is_err(), Err(_) => true, };

postcard::from_io would require a buffer allocation that must account for the largest possible decodable value. That's impossible to know without looking at the first varint. We could use a large enough scratch buffer, but I prefer the other way that just creates the vector, without the size considerations.
Also, from_io, maps all io::Errors to DeserializeUnexpectedEnd, making the distinction between torn data of allocation failure impossible, and I would like to keep it.

evanlinjin · 2026-08-17T03:46:43Z

+        // Always write at the current end of the file. This handle's cursor may be stale if
+        // another handle has appended since we last read, and writing at a stale offset would
+        // overwrite those changesets.


I don't think this is the behavior we want. If we failed to read the last entry due to DeserializeUnexpectedRead, then further writes will also be corrupted and unreadable due to the len_prefixes being unaligned.

I didn't have that present while working on this, but what I have in mind was to not leave space for intermediate faulty states, except for concurrency (that as far as I know is not supported nor intended to do so), and bit flipping (which should be remediated by redundancy, i.e., backups).
There is a flaw in the current implementation though, because the file end setting can fail too, leaving the torn bytes behind. What if I fail loudly in that case? So the user knows the append failed and there are torn bytes at the end, and he should retry the append of fix his changesets and retry later.
In this case all intermediate faulty states (without considering concurrency and bit flipping) will be covered.

I think we need to assume 1 handle per file and use a lockfile to enforce it. I propose reverting to the old logic in master; append should only append after valid data.

One thing that is missing in master, is actually rolling back the data (as you have done with File::set_len).

I was trying to not get into locks, but as it seems we are headed in that direction, I can roll back the stale head change, and document the expectations in this PR, and leave the lock changes for another PR.

evanlinjin · 2026-08-17T04:11:11Z

Entry length prefix changed from an 8-byte little-endian integer to a
postcard varint (1-10 bytes). Covered by the same on-disk format change and
magic-byte-bump guidance above.

The version on master used varint length prefixes as well.

luisschwab · 2026-08-17T22:59:35Z

Breaking compatibility with older bincode-encoded stores without supplying a conversion tool leaves users stranded. Regardless of whether file_store is intended for dev environments, dropping support without providing a migration path or prior notice inevitably leads to silent data loss.

This crate is explicitly for testing only, per the README.

nymius · 2026-08-18T12:34:12Z

Entry length prefix changed from an 8-byte little-endian integer to a
postcard varint (1-10 bytes). Covered by the same on-disk format change and
magic-byte-bump guidance above.

The version on master used varint length prefixes as well.

Yes, this is a left over from an early iteration that I never published.

`bincode` is no longer maintained. `postcard` is the closest maintained project with >52M downloads on crates.io, regular releases, and activity on its repo. BREAKING CHANGE: - Magic Bytes should be changed to avoid accidentaly modifying old file store blobs. - The internal encoding has changed as a result of using postcard. Old file stores won't be recoverable using the latest file_store version. - As this is a development environment store, we don't provide migration utilities. - `StoreError::Bincode` has been renamed to `StoreError::Decode`, and now contains `postcard::Error`s - From now on, trailing bytes after decoding are rejected. - `append` always tries to attach changesets to the latest valid end of the file.

evanlinjin · 2026-09-10T09:03:55Z

+    #[test]
+    fn append_from_stale_handle_does_not_overwrite_existing_changeset() {
+        let temp_dir = tempfile::tempdir().unwrap();
+        let file_path = temp_dir.path().join("db_file");
+        let initial = TestChangeSet::from(["initial".to_string()]);
+        let first_update = TestChangeSet::from(["first".to_string()]);
+        let second_update = TestChangeSet::from(["other".to_string()]);
+
+        let mut first =
+            Store::<TestChangeSet>::create(&TEST_MAGIC_BYTES, &file_path).expect("must create");
+        first.append(&initial).expect("must append initial state");
+
+        let (mut stale, _) = Store::<TestChangeSet>::load(&TEST_MAGIC_BYTES, &file_path)
+            .expect("must open second handle");
+
+        first
+            .append(&first_update)
+            .expect("must append first update");
+        stale
+            .append(&second_update)
+            .expect("must append from second handle");
+        drop(first);
+        drop(stale);
+
+        let (_, recovered) = Store::<TestChangeSet>::load(&TEST_MAGIC_BYTES, &file_path)
+            .expect("both appends must remain decodable");
+        let mut expected = initial;
+        expected.extend(first_update);
+        expected.extend(second_update);
+        assert_eq!(
+            recovered,
+            Some(expected),
+            "a stale handle overwrote an append"
+        );
+    }


Can we just assume that there is always only one handle at one time per file? As you mentioned, we don't need to support concurrent access. So the only time this would occur is either a bug in the caller's code, or the user of the wallet tries to open the same instance of the wallet twice. I think this should be handled with a lock file (separate PR imo).

Yes, as I said above, I was trying to not get into locks. I will document the expectations, and leave this out of this PR.

Brijesh-Thakkar · 2026-09-30T09:34:48Z

I ran the reproduction from #2306, plus variants, against head eb5f3ea (AI-assisted testing). All of these return Err(Decode(DeserializeUnexpectedEnd)) with no panic or abort, and memory stays flat:

the issue's literal bytes read as postcard
declared frame lengths of u64::MAX, 2^63, 2^62, 1 GiB, 1 MiB
an 11-byte 0xFF length prefix
an inner String length of u64::MAX

The existing test at store.rs:626 also fails with capacity overflow if I replace the guarded reserve in entry_iter.rs with vec![0; len as usize]. So #2258 looks like it fixes #2306. Could you add "Fixes #2306" to the description? The "linking the issue being fixed" checklist item is unticked.

Minor, no change requested: try_reserve_exact(len) (entry_iter.rs:127) runs before len is compared to the remaining file bytes. It's fallible, so not a bug; capping by remaining bytes would just be a small hardening if you want it.

I can send a one-test patch asserting Decode (not Io) for a u64::MAX prefix if that's useful.

`bincode` is deprecated. Replace it with `postcard` and frame each entry as a `u64` varint length prefix followed by the `postcard`-encoded changeset. The length prefix makes the end of an entry explicit instead of relying on the decoder hitting `UnexpectedEof`. This also fixes `Store::load` never terminating when a changeset type encodes to zero bytes (e.g. `()`): every entry now occupies at least the length prefix, so the file offset always advances. A clean end-of-file is detected by peeking the buffer before reading an entry. The `Ok`-arm progress check from the previous commit is superseded by this and is removed. Add regression tests for zero-width changesets. The entry_iter, lib and Cargo.toml changes are taken from bitcoindevkit#2258. BREAKING CHANGE: the on-disk format changes, so existing store files written with `bincode` cannot be loaded. `StoreError::Bincode` is replaced by `StoreError::Decode(postcard::Error)`. Fixes bitcoindevkit#2307 Co-authored-by: nymius <155548262+nymius@users.noreply.github.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

`bincode` is deprecated. Migrate the file store's on-disk encoding to `postcard`, as part of the fix for bitcoindevkit#2308 (see also bitcoindevkit#2258). Each entry is now stored as a `postcard` `u64` varint length prefix followed by the `postcard`-encoded changeset. The length prefix frames entries, so a torn or corrupt entry can be told apart from a clean end of file, and a corrupt, huge length cannot trigger a huge allocation. Bytes left in a frame after decoding are rejected. Real I/O errors are reported as `StoreError::Io`, which `Store::append` now returns when scanning past entries written by another handle. BREAKING CHANGE: - The on-disk format changed. Files written by earlier versions cannot be read; use new magic bytes so old files fail with `StoreError::InvalidMagicBytes`. - `StoreError::Bincode(bincode::ErrorKind)` is replaced by `StoreError::Decode(postcard::Error)`. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

…oindevkit#2258 Clarify that this fix for the zero-width changeset hang is independent of the bincode -> postcard migration tracked in bitcoindevkit#2258. If bitcoindevkit#2258 lands first this check becomes moot, but the regression test is still worth keeping.

`()` decodes at end of file as well as before trailing bytes, so a store holding no entries at all hung on the same offset check. Cover it, and assert the loaded changeset rather than only that `load` returned. Also correct the comment above the check. It claimed the check becomes unnecessary once the postcard migration in bitcoindevkit#2258 lands. It does not: a collection of zero-sized elements with a corrupt length prefix loops inside the decoder and never reaches this point, and postcard has that same hole, so neither this check nor a byte-counting size limit catches it. Say what the check actually covers instead.

github-project-automation Bot added this to BDK Chain Aug 13, 2026

nymius changed the title ~~refactor(file_store)!: replace bincode by postcard~~ Replace bincode by postcard Aug 13, 2026

luisschwab assigned nymius Aug 13, 2026

luisschwab moved this to Needs Review in BDK Chain Aug 13, 2026

nymius force-pushed the push-ronzxprlmnpx branch from 4ba69d3 to 007c1d2 Compare August 14, 2026 12:49

nymius force-pushed the push-ronzxprlmnpx branch 3 times, most recently from 28a4592 to 1bde283 Compare August 14, 2026 16:13

evanlinjin requested changes Aug 17, 2026

View reviewed changes

nymius force-pushed the push-ronzxprlmnpx branch from 1bde283 to eb5f3ea Compare August 24, 2026 21:10

evanlinjin reviewed Sep 10, 2026

View reviewed changes

wangzhengzhuo05 mentioned this pull request Sep 14, 2026

fix(file_store): reject an entry length prefix larger than the file #2313

Closed

This was referenced Sep 30, 2026

fix(file_store): don't overwrite entries appended via another handle #2332

Draft

fix(file_store): stop iterating when a decoded entry consumes no bytes #2333

Draft

Manuel1234477 mentioned this pull request Oct 9, 2026

fix(file_store): prevent infinite loop on zero-width changeset types #2352

Open

luhrhenz mentioned this pull request Oct 10, 2026

file_store: Store::load never terminates for a collection of zero-sized elements with a corrupt length prefix #2364

Open

Conversation

nymius commented Aug 13, 2026

Changelog notice

Changed

Fixed

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

codecov Bot commented Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chukwudiikeh commented Aug 16, 2026

Uh oh!

evanlinjin commented Aug 17, 2026

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Aug 17, 2026

Uh oh!

luisschwab commented Aug 17, 2026

Uh oh!

nymius commented Aug 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brijesh-Thakkar commented Sep 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Aug 14, 2026 •

edited

Loading