chaincfg/chainhash: add strict parsing#2520
Conversation
utafrali
left a comment
There was a problem hiding this comment.
Clean, well-structured addition. The refactor of shared logic into decodeHash is a good DRY improvement, the strict variants correctly reject anything shorter or longer than 64 chars, and the doc notes steering callers to the strict helpers are a nice touch. Minor gaps: a missing test for even-length too-short input in TestDecodeStrict, and a small avoidable allocation in DecodeStrict.
chaincfg/chainhash/hash.go
Outdated
| // ErrHashStrSizeMismatch describes an error that indicates the caller | ||
| // specified a hash string that does not meet the exact length required for | ||
| // strict parsing. | ||
| var ErrHashStrSizeMismatch = fmt.Errorf("hash string must be exactly %v bytes", |
There was a problem hiding this comment.
The error message says "bytes" but MaxHashStringSize (64) is a count of hex characters, not bytes. The word "characters" would be more precise. That said, the existing ErrHashStrSize uses the same wording, so if you want consistency just leave it, but this would be a good opportunity to use the more accurate term in the new error.
There was a problem hiding this comment.
Good catch! Thanks! Fixed.
| } | ||
|
|
||
| // TestDecodeStrict executes tests against the DecodeStrict function. | ||
| func TestDecodeStrict(t *testing.T) { |
There was a problem hiding this comment.
TestDecodeStrict covers odd-length, non-hex, and too-long inputs, but there's no test for a valid even-length hex string that is simply too short (e.g., a 32-character hex string). That's likely the most common real-world mistake this function is meant to catch (someone passing an abbreviated hash). Adding a case like {"deadbeef" + strings.Repeat("0", 24), ErrHashStrSizeMismatch} (a 32-char string) would make the intent clearer.
There was a problem hiding this comment.
Added "deadbeef" + strings.Repeat("0", 24) case
| // decodeHash decodes the provided byte-reversed hexadecimal bytes into dst. | ||
| // The caller is responsible for applying any caller-specific length validation | ||
| // before invoking this helper. | ||
| func decodeHash(dst *Hash, src []byte) error { |
There was a problem hiding this comment.
The decodeHash helper accepts []byte so that Decode can pass its already-allocated srcBytes slice without an extra copy. However, DecodeStrict calls it with []byte(src), which allocates on every call. Since DecodeStrict only accepts exactly 64-char strings, you could declare a [MaxHashStringSize]byte on the stack and copy into it (or keep a local var b [MaxHashStringSize]byte; copy(b[:], src); return decodeHash(dst, b[:])) to avoid the heap allocation on the hot path.
There was a problem hiding this comment.
Go already optimizes such cases - no need to use a stack array for this.
I added a benchmark and ensured there is already no memory allocation here:
go test -run '^$' -bench '^BenchmarkDecodeStrict$' -benchmem -count=5
goos: linux
goarch: amd64
pkg: github.com/btcsuite/btcd/chaincfg/chainhash
BenchmarkDecodeStrict-2 15530290 78.59 ns/op 0 B/op 0 allocs/op
BenchmarkDecodeStrict-2 14514126 78.90 ns/op 0 B/op 0 allocs/op
BenchmarkDecodeStrict-2 19157900 77.40 ns/op 0 B/op 0 allocs/op
BenchmarkDecodeStrict-2 14660510 78.14 ns/op 0 B/op 0 allocs/op
BenchmarkDecodeStrict-2 18853245 78.26 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/btcsuite/btcd/chaincfg/chainhash 8.575s
Add NewHashFromStrStrict and DecodeStrict for callers that must parse full txids or block hashes exactly. Keep NewHashFromStr and Decode lenient for compatibility, but add NOTE docs steering typical parsing to the strict helpers. Add tests for the new strict behavior and preserve coverage for the existing lenient short and odd hex behavior.
8192c02 to
5fefcc6
Compare
Change Description
Add NewHashFromStrStrict and DecodeStrict for callers that must parse full txids or block hashes exactly.
Keep NewHashFromStr and Decode lenient for compatibility, but add NOTE docs steering typical parsing to the strict helpers.
Add tests for the new strict behavior and preserve coverage for the existing lenient short and odd hex behavior.
I also patched call sites of the affected functions, keeping it in a branch for now, because it depends on a sub-module version bump.
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.