fix(file_store): stop iterating when a decoded entry consumes no bytes - #2333
Open
Brijesh-Thakkar wants to merge 2 commits into
Open
Brijesh-Thakkar wants to merge 2 commits into
Brijesh-Thakkar wants to merge 2 commits into
Conversation
EntryIter::next only detects end-of-file through an UnexpectedEof error raised when zero bytes are consumed. A changeset type whose bincode encoding is zero bytes (e.g. `()`) never raises that error: every decode succeeds without advancing the file offset, so Store::load never returns. Compare the stream position before and after a successful decode. If it did not advance, finish the iterator and return a StoreError::Bincode error. Normal changesets always consume at least one byte, so their behavior is unchanged. There are no public API changes. Add regression tests that time out without this change. Fixes bitcoindevkit#2307
`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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2307.
Store::loadnever returns when the changeset type encodes to zero bytes (e.g.Store::<()>).EntryIter::nextonly detects end-of-file through anUnexpectedEoferror raised when zero bytes are consumed. A zero-width type decodes successfully without reading anything, so that error never fires. The file offset never advances, andload→dumpiterates forever.The fix compares
stream_position()before and after a successful decode in theOkarm. If the offset did not advance, the iterator is finished and aStoreError::Bincodeerror is returned. Normal changesets always consume at least one byte, so their behavior is unchanged. There are no public API, dependency or on-disk format changes.Notes to the reviewers
Store::<()>. Both cases are covered by tests.Store::<()>returns an error in both cases instead of hanging, so it can no longer be loaded at all. I think this is acceptable becauseappendnever writes empty changesets (is_emptyis checked first) and no real BDK changeset is zero-width. ReturningOkfor a magic-only file would need an EOF peek like the one in Replace bincode by postcard #2258, which is a larger diff.StoreError::Bincodevariant withErrorKind::Customto avoid a breaking change, sinceStoreErroris not#[non_exhaustive].Io(InvalidData)would also work if you prefer it.Okarm ofnext(). I put the tests in a new file (crates/file_store/tests/test_zero_width.rs) to avoid conflicts with thestore.rstests. If Replace bincode by postcard #2258 lands first, this code change can be dropped and the tests kept as a regression guard. I'm happy to rebase or close this in favor of Replace bincode by postcard #2258, whichever you prefer.loadon a detached thread with a 5-second timeout. Both time out without the fix. On failure, the thread keeps spinning until the test process exits.Changelog notice
Fixed
Store::loadno longer loops forever when the changeset type encodes to zero bytes (e.g.()). It now returnsStoreError::Bincodewhen a decoded entry consumes no bytes.Checklists
All Submissions:
Bugfixes: