fix(utxovalidator): consume fileformat magic at the reader source, not twice in validate#979
Merged
Merged
Conversation
…t twice in validate ValidateUTXOFile routes through two reader sources: getLocalFileReader -> os.Open(path) getBlobStoreReader -> blockStore.GetIoReader(...) Only the second consumes the 8-byte fileformat magic before returning (via the file store's validateFileHeader, or the memory store's strip-by-size). validateUTXOSet was then unconditionally calling fileformat.ReadHeader(br), which: - worked for local-file callers (file still has the magic), but - against the blob store, read the *next* 8 bytes of the body — the first bytes of the block hash field — and failed with "unknown magic: [<random hash bytes>]" The blob-store path is the same anti-pattern as the verifyLastSet / CreateUTXOSet fix in bsv-blockchain#971; flagged in its review as a separate follow-up because this is a CLI tool not on the hot path. Fix: uniformise the two readers so both hand validateUTXOSet a body-only reader. - getLocalFileReader now consumes + validates the magic itself (returns a reader positioned past the magic, same shape as GetIoReader). - validateUTXOSet drops its ReadHeader call. Both reader sources are now equivalent. - The FileType==FileTypeUtxoSet assertion moves into getLocalFileReader since that's where the read happens. The blob-store path keeps its existing FileType check via the file store's validateFileHeader. Regression tests on the local-file path: - LocalDoesNotDoubleReadMagic: a tempfile with valid magic + metadata + no wrappers; without the fix, fails with the exact production error chain. - LocalRejectsWrongFileType: a tempfile with subtree magic; pins that the FileType check moved into getLocalFileReader still rejects mistyped files with a clear error.
Contributor
|
🤖 Claude Code Review Status: Complete No issues found. The fix correctly unifies the reader contract between local-file and blob-store paths. Summary:
|
| // not be confused with the original double-read bug. | ||
| if err != nil { | ||
| assert.NotContains(t, err.Error(), "unknown magic") | ||
| } |
Contributor
There was a problem hiding this comment.
[Minor] Lines 82-84 are unreachable since line 79 uses require.NoError which exits the test on error. Consider removing:
if err != nil {
assert.NotContains(t, err.Error(), "unknown magic")
}The regression documentation value is already covered by the require statement message at line 79.
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-28 18:54 UTC |
The if err != nil { assert.NotContains(...) } block sat below
require.NoError, which exits the test on error — so the branch was
unreachable dead code. The require.NoError message already names the
"unknown magic" regression mode, so dropping the dead block doesn't
lose any documentation value.
|
sugh01
approved these changes
May 29, 2026
freemans13
approved these changes
May 29, 2026
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
3 participants
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.
Summary
Same double-read anti-pattern as #971, this time in the
utxovalidatorCLI tool. Flagged in #971's review as a separate follow-up because this isn't on a hot path.ValidateUTXOFileaccepts a path and produces anio.ReadCloserfrom one of two sources:Only the second consumes the 8-byte fileformat magic before returning (via the file store's
validateFileHeaderor the memory store's strip-by-size).validateUTXOSetthen unconditionally calledfileformat.ReadHeader(br), which:unknown magic: [<random hash bytes>].The blob-store path is reachable when the CLI tool is pointed at a 64-char block hash instead of a file path (
utxo_validator.go:170-176).Fix
Uniformise the two readers so both hand
validateUTXOSeta body-only reader:getLocalFileReadernow consumes and validates the magic itself (returns a reader positioned past the magic — same shape asGetIoReader).validateUTXOSetdrops theReadHeadercall entirely.FileType == FileTypeUtxoSetassertion moves intogetLocalFileReader. The blob-store path keeps its existing FileType check via the file store'svalidateFileHeader.Regression tests
TestValidateUTXOFile_LocalDoesNotDoubleReadMagic— writes a minimal valid UTXO set file to a tempdir and verifiesValidateUTXOFileparses the block hash / height / previous hash correctly. Without the fix, restoring theReadHeadercall invalidateUTXOSetcauses this test to fail with the exact production error chain (PROCESSING (4): error reading file header -> UNKNOWN (0): unknown magic: [...]).TestValidateUTXOFile_LocalRejectsWrongFileType— writes a file with subtree magic and verifies the FileType check that moved intogetLocalFileReaderstill rejects mistyped files with a clear error.Test plan
go test ./cmd/utxovalidator/...cleango test -race -run TestValidateUTXOFile_Local ./cmd/utxovalidator/cleango build ./cmd/utxovalidator/...cleangci diffcleanReadHeaderline restoredRelated
Same anti-pattern, different site as #971 (
fix(utxopersister): drop double-read of fileformat magic in verifyLastSet). Reviewer in #971 explicitly asked to land this as a separate small PR.