chore(utxopersister): clarify footer-read match per #985 review#994
Merged
liam merged 1 commit intoJun 1, 2026
Merged
Conversation
…review Addresses review comments on bsv-blockchain#985 that merged before they were applied. No behavior change. - The previous comment claimed the wrapper stream 'carries no record count'; it does - the 16-byte footer is txCount + utxoCount. Reworded to say the loop simply does not consult it. - The match read as a clean structural errors.Is, but teranode's errors.New flattens a non-*Error cause to its message string (errors/errors.go:334-336), so errors.Is(err, io.ErrUnexpectedEOF) degrades to strings.Contains(err.Error(), "unexpected EOF"). Switched to the explicit strings.Contains so the substring nature is visible, and documented why - including that errors.Is(err, io.EOF) would also match this footer error ("EOF" is a substring of "unexpected EOF"), so the io.EOF clause stays a pointer compare. - Noted the silent-truncation limitation and the structural follow-up (typed sentinel + records-read == txCount validation). - Fixed the stale 'Read the next 36 bytes...' comment (a txid is 32).
Contributor
|
🤖 Claude Code Review Status: Complete Current Review:
This is a documentation and clarity improvement PR with no behavior change. The code changes are sound:
The one issue found is a minor documentation claim that contradicts the actual |
| // | ||
| // Consequence: a genuinely truncated tail is | ||
| // indistinguishable from the footer and is silently | ||
| // accepted (pre-existing; same as utxovalidator). Matching |
Contributor
There was a problem hiding this comment.
[Minor] Documentation inaccuracy — The comment states this code behaves "same as utxovalidator" (line 659) and claims it matches "only 'unexpected EOF' - not the broader 'failed to read txid' utxovalidator also matches" (lines 660-662).
However, cmd/utxovalidator/utxo_validator.go:127 matches BOTH patterns:
if strings.Contains(errStr, "failed to read txid") || strings.Contains(errStr, "unexpected EOF") {
This code is actually more restrictive than utxovalidator (intentionally, for better error visibility per the comment). The documentation should reflect this difference rather than claiming equivalence.
Suggested fix: Replace "same as utxovalidator" with "like utxovalidator, though this matches only the footer-specific error for better diagnostics".
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-31 19:17 UTC |
ordishs
approved these changes
Jun 1, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Verified no behavior change: traced both the old errors.Is(err, io.ErrUnexpectedEOF) and the new strings.Contains(err.Error(), "unexpected EOF") to the identical string comparison via the custom Error.Is substring fallback (errors/errors.go:154-164) and errors.New cause-flattening (errors/errors.go:330-333). Confirmed the bare io.EOF return path (UTXO.go:191-193) and the footer short-read message (UTXO.go:212-218). gofmt and go vet clean.
The swap makes the substring dependency explicit at the call site rather than hidden behind errors.Is, and the expanded comment correctly documents the brittleness and the structural follow-up. Approve.
sugh01
approved these changes
Jun 1, 2026
oskarszoon
approved these changes
Jun 1, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
No P0/P1. Traced the equivalence end to end and it's exact: after errors.New flattens the io.ErrUnexpectedEOF cause to its message string (errors/errors.go:334-336), the old errors.Is(err, io.ErrUnexpectedEOF) resolves through (*Error).Is to strings.Contains(e.Error(), "unexpected EOF") — byte-identical to the new explicit call. The "no behavior change" claim holds, and the footer branch is pinned by TestCreateUTXOSet_PreviousSetWithFooterTerminatesCleanly. Verified the cmd/utxovalidator cross-reference too: matching only "unexpected EOF" here (vs utxovalidator's broader "failed to read txid") is the correct call — it keeps a real non-EOF read error loud instead of swallowing it. gofmt/vet/build clean. LGTM.
Nit, non-blocking: the errors/errors.go:334-336 line ref in the comment will rot on the next edit there — the symbol name carries it, so leave it or drop the line numbers.
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
4 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.
Follow-up to #985, addressing review comments that merged before they could be applied. No behavior change — purely clarity plus one stale-comment fix.
The reviewer correctly pointed out that the match in #985 is not the clean structural check it appeared to be:
txCount+utxoCount. The loop just doesn't consult it. Reworded.errors.Iswas masquerading as structural.teranode'serrors.Newflattens a non-*Errorcause to its message string (errors/errors.go:334-336), discarding theio.ErrUnexpectedEOFsentinel — soerrors.Is(err, io.ErrUnexpectedEOF)reduces tostrings.Contains(err.Error(), "unexpected EOF"). Switched to the explicitstrings.Containsso the substring nature is visible in the code, with a comment explaining it. Also documented the footgun the reviewer flagged:errors.Is(err, io.EOF)would also match this footer error ("EOF"⊂"unexpected EOF"), so theio.EOFclause stays a pointer comparison.cmd/utxovalidator). The real structural fix —FromReaderreturning a typed sentinel, plus validatingrecords-read == txCountacross the consolidation loop, the seeder, andutxovalidator— is noted as a follow-up.// Read the next 36 bytes...comment (a txid is 32 bytes; pre-existing, sat right above the change).Verification
go test ./services/utxopersister/— passgo vet,gofmt,gci— clean