common: share fixed-size byte type helpers#22173
Merged
Merged
Conversation
The Bytes4/48/64/96 copies of Hash.SetBytes kept length.Hash (32) in the crop and right-align arithmetic, so SetBytes panicked or misplaced bytes for most input lengths. Use the receiver's own length, matching Hash.SetBytes semantics (crop from the left, right-align short inputs).
Bytes4/48/64/96, Hash, and Address carried near-identical copies of Format, SetBytes, TerminalString, MarshalText, and the testing/quick Generate loop. Move the shared bodies to unexported []byte helpers in bytesn.go (fixedFormat, fixedSetBytes, fixedTerminalString, fixedGenerate) and reuse hexutil.Bytes.MarshalText instead of the hand-rolled hex encoding; each method becomes a one-liner passing b[:]. Kept bespoke: Address Hex/String/Format EIP-55 checksum paths, Bytes4.TerminalString (whole-array %x; the head…tail form would overlap at 4 bytes), Hash.Generate (math/rand/v2), and the Hash/Address-only methods (Scan, Value, Cmp, Big, GraphQL). TestFixedBytesGolden pins byte-for-byte output of every affected method per type, with goldens captured from the pre-refactor implementations.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors fixed-size byte types in common to deduplicate repeated method bodies by introducing shared unexported helpers (formatting, SetBytes, terminal string, and quick-check generation), while keeping public APIs and type shapes unchanged.
Changes:
- Added shared helpers (
fixedFormat,fixedSetBytes,fixedTerminalString,fixedGenerate) and rewiredHash/BytesNmethods to delegate to them. - Simplified
MarshalTextimplementations to usehexutil.Bytes(...).MarshalText()across types. - Added regression coverage via
TestBytesNSetBytesand a comprehensive golden test (TestFixedBytesGolden) pinning behavior across fixed-size byte types.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| common/hash.go | Delegates Format, TerminalString, and SetBytes to shared helpers. |
| common/bytesn.go | Introduces shared helper implementations for fixed-size byte types. |
| common/bytesn_test.go | Adds semantic test coverage for SetBytes behavior across byte types. |
| common/bytesn_golden_test.go | Adds golden tests pinning formatting/marshalling/error-string behavior across types. |
| common/bytes4.go | Refactors formatting/marshalling/set/generate via helpers; keeps bespoke TerminalString. |
| common/bytes48.go | Refactors formatting/marshalling/set/generate/terminal string via helpers. |
| common/bytes64.go | Refactors formatting/marshalling/set/terminal string via helpers. |
| common/bytes96.go | Refactors formatting/marshalling/set/generate/terminal string via helpers. |
| common/address.go | Refactors SetBytes and MarshalText while preserving EIP-55 formatting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+54
to
+59
| func fixedSetBytes(dst, src []byte) { | ||
| if len(src) > len(dst) { | ||
| src = src[len(src)-len(dst):] | ||
| } | ||
| copy(dst[len(dst)-len(src):], src) | ||
| } |
… SetBytes fixedGenerate took a *math/rand.Rand, so Hash.Generate (math/rand/v2) kept its own byte-identical loop. Pass the intn/u32 primitives instead and all four types plus Hash share one body. TestBytesNSetBytes now also exercises Hash and Address.
awskii
approved these changes
Jul 3, 2026
awskii
left a comment
awskii
left a comment
Member
There was a problem hiding this comment.
Behavior-preserving consolidation. Verified equivalent for all six types — build/vet clean, golden + SetBytes tests green, and the SetBytes semantic change is unreachable (no callers of the four fixed types; Hash/Address were already byte-identical).
Pushed a small follow-up: folded Hash.Generate into the shared helper (it was still a duplicate loop because of math/rand/v2) and extended TestBytesNSetBytes to Hash/Address.
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 8, 2026
…#22165) Fixes eleven defects that all share one root cause: code duplicated between siblings and then patched or mutated in only one copy. Found during a whole-repo duplication audit; each fix is surgical and independent. (A twelfth — `Bytes4/48/64/96.SetBytes` keeping `length.Hash` from the `Hash.SetBytes` original — was absorbed by the shared-helper refactor in erigontech#22173 and is no longer part of this diff.) With regression tests (written first, red → green): - **cl/cltypes**: `BlindedBeaconBody.EncodingSizeSSZ` counted `ExecutionChanges` twice and never `BlobKzgCommitments`; `NewBlindedBeaconBody` dropped its `version` argument (`Version: 0`), unlike `NewBeaconBody` — `block_production.go` was already working around it with an explicit `SetVersion` (`TestBlindedBeaconBodyEncodingSizeTracksEncoding`). - **cl/beacon/handler**: of the three cloned `pending_{consolidations,deposits,partial_withdrawals}` endpoints only the third set `Eth-Consensus-Version`/version; now all three do (`TestGetPendingQueuesConsensusVersionHeader`; forkchoice mock's pending getters made settable for it). Without new tests (log/metric/dead-code corrections, or not reasonably reproducible as a unit test — noted per item): - **db/state**: `IntegrityInvertedIndexAllValuesAreInRange` had the `StorageHistoryIdx`/`CodeHistoryIdx` arms crossed, so a targeted `erigon seg integrity` check validated the other domain's inverted index. A red repro test would need a corrupt-II fixture; the fix is a label/index correction verifiable by inspection. - **execution/exec**: AA-bundle log printed `startIdx-endIdx` (uint64 underflow); the historical-trace copy of the same code already had the correct form. - **execution/stagedsync**: per-domain `exec_domain_cache_put_rate` gauges were set to raw counts while the aggregate sets a per-second rate — dashboards mixing them compared different units; also `mxExex*`/`mxCommitmentBrancg*` variable typos, and the serial executor re-inlined the extracted-and-tested `shouldMarkExhaustedAtBlock` instead of calling it (behavior identical: `(!a || b) ≡ (a → b)`). - **p2p/sentry**: `getBlockHeaders66` returned the same error on both branches of its `IsPeerNotFoundErr` check; a peer disconnecting mid-answer now returns nil like the bodies/BAL handlers. - **cmd/txpool**: the daemon logged under the `"integration"` prefix, pasted from cmd/integration. - **db/kv/mdbx**: pseudo-dupsort `LastDup` wrapped errors as `"in FirstDup"`. - **db/snapshotsync/freezeblocks**: deleted `SegmentsCaplin`, a dead near-verbatim copy of `snapshotsync.SegmentsCaplin` with zero callers. Verification: affected-package tests green, scoped `golangci-lint` clean (×2), `make erigon integration` builds. --------- Co-authored-by: Alex Sharov <AskAlexSharov@gmail.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.
bytes48.goandbytes96.gowere byte-identical whole files modulo the type name;bytes4.go,bytes64.go,hash.go, andaddress.gorepeated most of the same method bodies. Since Go can't parameterize[N]byteby length and these types must stay comparable arrays, the bodies move to unexported helpers over[]byte(fixedFormat,fixedSetBytes,fixedTerminalString,fixedGenerate); each method becomes a one-liner passingb[:]. Net −169 production lines; no exported API change.Kept bespoke on purpose:
Address's EIP-55 checksummedHex/String/Format;Bytes4.TerminalString(%xof the whole array — head…tail would overlap at N=4);Hash.Generate(math/rand/v2, incompatible with the v1 quick-check helper); all Hash/Address-only methods.Safety net:
TestFixedBytesGolden(+377 lines) pins per type — every Format verb incl. bad-verb labels, String/Hex/MarshalText/Value, TerminalString, SetBytes placement, UnmarshalText/JSON error strings (incl. theBLSSignaturelabel), seededGenerate, and the EIP-55 canonical example for Address. Goldens were captured from the pre-refactor implementation and stay green through the refactor.First commit is a cherry-pick of the
SetBytesfix from #22165 (this consolidation builds on it); it will drop out on rebase once that PR merges.Verified:
go test ./common/...,go build ./..., scopedgolangci-lintclean (×2),make erigon.Part of a dedup series; siblings #22165–#22172.