You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(eth/protocols/eth): cut HashOrNumber decode 145ns to 96ns #22841 - #3051
HashOrNumber.DecodeRLP decodes the origin of every GetBlockHeaders request. This ports the eth/protocols/eth half of geth ethereum#22841 (c0e201b69), which is still the decoder on geth master 78af8c312 — unchanged through eth/67–71, delayed p2p decoding (ethereum#33835) and the removal of les (ethereum#28586).
DecodeRLP(s *rlp.Stream)
- _, size, _ := s.Kind()- origin, err := s.Raw()- rlp.DecodeBytes(origin, &hn.Hash) / (&hn.Number)+ _, size, err := s.Kind()+ return s.Decode(&hn.Hash) / (&hn.Number) after clearing the unused field
The upstream les/protocol.go half does not apply: this tree has no les package, and upstream removed light client code in ethereum#28586. The port is 12 lines in one file; the HashOrNumber type, EncodeRLP, imports and the xdc/166 request-id wrapper are untouched, so the wire format, peer negotiation, consensus state and RPC surface do not change.
Evidence
Before: the old decoder copies the RLP value with s.Raw() and decodes it a second time with rlp.DecodeBytes; a temporary benchmark over hash and number origins measured 144.6–146.1 ns/op, 100 B/op, 4 allocs/op. The existing TestGetBlockHeadersDataEncodeDecode round-trip test passes with the old decoder. After:95.6–96.5 ns/op, 72 B/op, 2 allocs/op on the same probe (removed afterwards; not part of this PR).
Mutation probe: reverting DecodeRLP to the old implementation leaves the existing round-trip test green but turns the two new clearing cases red:
number_clears_stale_hash: have {Hash:0x0001…1f Number:314}, want {Hash:0x0000…00 Number:314}
hash_clears_stale_number: have {Hash:0x0001…1f Number:7}, want {Hash:0x0001…1f Number:0}
TestHashOrNumberDecodeRLP adds four cases: number/hash decode into a reused value, an invalid 9-byte origin and empty input. The clearing is defensive in production — handler.go creates a zero-value query per request — but it restores the union's exclusivity for reused values, so EncodeRLP cannot reject a packet as "both origin hash and number provided" and a hash lookup cannot carry a stale number.
Upstream comparison:git show c0e201b69 -- eth/protocols/eth/protocol.go matches this diff line for line (only the hunk line number and the surrounding baseline comment differ). git log -S 'hn.Number = 0' -- eth/protocols/eth/protocol.go on local geth master returns only eth/protocols/eth, les: avoid Raw() when decoding HashOrNumber ethereum/go-ethereum#22841, so this is the stable upstream end state and will not be undone by later protocol ports.
Validation:make all, make generate, make tidy, clean tree, make lint (0 issues), go vet ./..., make test all pass in a detached /tmp worktree at this commit (log /tmp/verify-logs/port-hashornumber-decode-c569e7736c-VwTxpb.log).
Merge Danger
Door: two-way — 12 lines in one function, no wire, state or API change; reverting restores the old decoder exactly.
Blast Radius: narrow. Only eth/protocols/eth header-request decoding is affected. handler.go builds a fresh zero-value query per request, and clearing the unused field can only remove stale data that the old decoder would have retained; successful decodes produce the same hash or number as before.
Proposed changes
Port geth ethereum#22841's HashOrNumber.DecodeRLP to decode the origin directly from the RLP stream instead of copying the raw value. One production function changes (+12/-12), plus a focused table test (+43). The les half of the upstream change is not applicable to this tree.
Types of changes
What types of changes does your code introduce to XDC network? Put an ✅ in the boxes that apply
build: Changes that affect the build system or external dependencies
ci: Changes to CI configuration files and scripts
chore: Changes that don't change source code or tests
docs: Documentation only changes
feat: A new feature
fix: A bug fix
perf: A code change that improves performance
refactor: A code change that neither fixes a bug nor adds a feature
revert: Revert something
style: Changes that do not affect the meaning of the code
test: Adding missing tests or correcting existing tests
Impacted Components
Which parts of the codebase does this PR touch? Put an ✅ in the boxes that apply
Consensus
Account
Network
Geth
Smart Contract
External components
Not sure (Please specify below)
eth/protocols/eth only (the eth protocol RLP decoder), with no consensus, account, contract or RPC surface.
Checklist
Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that
This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
Tested the backwards compatibility. — The RLP origin encoding is unchanged; TestGetBlockHeadersDataEncodeDecode round-trips both hash and number origins through the new decoder, and the xdc/164–166 packet shapes are untouched.
Tested with XDC nodes running this version co-exist with those running the previous version.
Relevant documentation has been updated as part of this PR
N/A
Unchecked items: this is an internal RLP decoding path with no wire format, negotiation, consensus, state or RPC change, so a genesis/epoch private-network run and a mixed-version coexistence run cannot exercise anything beyond the round-trip and table tests above; no documentation describes this decoder.
End-to-end test plan: bring up two nodes on a private network (Local_DPoS_Setup), one with this build and one with the previous build, and confirm header sync converges while requesting headers by both number and hash (a GetBlockHeaders origin of 314 and of a known block hash). Confirm both peers stay connected and return identical header sets; no new RPC or observable behavior is introduced.
Reviewing files that changed from the base of the PR and between 9d9524b and 7f44e03.
Files selected for processing: 2
eth/protocols/eth/protocol.go
eth/protocols/eth/protocol_test.go
No actionable comments were generated in the recent review.
Walkthrough
HashOrNumber.DecodeRLP now decodes directly from the RLP stream, propagates stream-kind errors, and clears the unused field. Tests cover stale values, invalid-size input, and empty input.
Changes: HashOrNumber RLP decoding
Layer / File(s)
Summary
Decode values and validate input eth/protocols/eth/protocol.go, eth/protocols/eth/protocol_test.go
DecodeRLP decodes hash and number values directly from the stream and clears the unused field. Tests check stale values and errors for invalid-size and empty inputs.
This change makes header-request decoding more efficient and prevents stale hash or number data when a decoded value is reused. The wire format is unchanged and tests cover the edge cases, so there is no remaining merge-blocking risk.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: a performance fix to HashOrNumber decoding in eth/protocols/eth, with the before and after timings. It matches the changeset, which rewrites HashOrNumber.DecodeRLP to …
Description check
The description follows the repository template. It fills in the Proposed changes section, the Types of changes (perf and test), the Impacted Components (Network, Geth), and the checklist with reasons…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: a performance fix to HashOrNumber decoding in eth/protocols/eth, with the before and after timings. It matches the changeset, which rewrites HashOrNumber.DecodeRLP to decode directly from the RLP stream and adds table tests. The title is specific and concise.
Full details: Description check
Explanation
The description follows the repository template. It fills in the Proposed changes section, the Types of changes (perf and test), the Impacted Components (Network, Geth), and the checklist with reasons for unchecked items. It gives benchmark data, validation results, a mutation probe, and an end-to-end test plan. The template's Proposed changes heading is present, and the description adds a Merge Danger section that the template does not include. That extra section does not hurt completeness.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between 94e5265 and 9d9524b.
Files selected for processing: 2
eth/protocols/eth/protocol.go
eth/protocols/eth/protocol_test.go
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…um#22841
Every GetBlockHeaders request copied its origin into a temporary RLP value
and decoded that copy a second time. Decoding straight from the stream drops
one copy and one decoder pass per request, and propagates the error from Kind
instead of discarding it.
The old decoder also wrote only the field it selected. Reusing a HashOrNumber
across decodes left the other field set, so EncodeRLP could reject the packet
with "both origin hash and number provided" and the header handler could mix
a stale number into a hash lookup. Clearing the unused field restores the
union's exclusivity.
The existing round-trip test passes with the old decoder, so it cannot cover
either clearing case. The new table test turns red when the implementation is
reverted, covering both origins, an invalid size and an empty input.
Ref: ethereum#22841
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
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
HashOrNumber.DecodeRLPdecodes the origin of everyGetBlockHeadersrequest. This ports theeth/protocols/ethhalf of geth ethereum#22841 (c0e201b69), which is still the decoder on geth master78af8c312— unchanged through eth/67–71, delayed p2p decoding (ethereum#33835) and the removal ofles(ethereum#28586).The upstream
les/protocol.gohalf does not apply: this tree has nolespackage, and upstream removed light client code in ethereum#28586. The port is 12 lines in one file; theHashOrNumbertype,EncodeRLP, imports and the xdc/166 request-id wrapper are untouched, so the wire format, peer negotiation, consensus state and RPC surface do not change.Evidence
s.Raw()and decodes it a second time withrlp.DecodeBytes; a temporary benchmark over hash and number origins measured144.6–146.1 ns/op,100 B/op,4 allocs/op. The existingTestGetBlockHeadersDataEncodeDecoderound-trip test passes with the old decoder.After:
95.6–96.5 ns/op,72 B/op,2 allocs/opon the same probe (removed afterwards; not part of this PR).DecodeRLPto the old implementation leaves the existing round-trip test green but turns the two new clearing cases red:TestHashOrNumberDecodeRLPadds four cases: number/hash decode into a reused value, an invalid 9-byte origin and empty input. The clearing is defensive in production —handler.gocreates a zero-value query per request — but it restores the union's exclusivity for reused values, soEncodeRLPcannot reject a packet as "both origin hash and number provided" and a hash lookup cannot carry a stale number.git show c0e201b69 -- eth/protocols/eth/protocol.gomatches this diff line for line (only the hunk line number and the surrounding baseline comment differ).git log -S 'hn.Number = 0' -- eth/protocols/eth/protocol.goon local geth master returns only eth/protocols/eth, les: avoid Raw() when decoding HashOrNumber ethereum/go-ethereum#22841, so this is the stable upstream end state and will not be undone by later protocol ports.make all,make generate,make tidy, clean tree,make lint(0 issues),go vet ./...,make testall pass in a detached/tmpworktree at this commit (log/tmp/verify-logs/port-hashornumber-decode-c569e7736c-VwTxpb.log).Merge Danger
Door: two-way — 12 lines in one function, no wire, state or API change; reverting restores the old decoder exactly.
Blast Radius: narrow. Only
eth/protocols/ethheader-request decoding is affected.handler.gobuilds a fresh zero-value query per request, and clearing the unused field can only remove stale data that the old decoder would have retained; successful decodes produce the same hash or number as before.Proposed changes
Port geth ethereum#22841's
HashOrNumber.DecodeRLPto decode the origin directly from the RLP stream instead of copying the raw value. One production function changes (+12/-12), plus a focused table test (+43). Theleshalf of the upstream change is not applicable to this tree.Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyeth/protocols/ethonly (the eth protocol RLP decoder), with no consensus, account, contract or RPC surface.Checklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) thatTestGetBlockHeadersDataEncodeDecoderound-trips both hash and number origins through the new decoder, and the xdc/164–166 packet shapes are untouched.Unchecked items: this is an internal RLP decoding path with no wire format, negotiation, consensus, state or RPC change, so a genesis/epoch private-network run and a mixed-version coexistence run cannot exercise anything beyond the round-trip and table tests above; no documentation describes this decoder.
End-to-end test plan: bring up two nodes on a private network (
Local_DPoS_Setup), one with this build and one with the previous build, and confirm header sync converges while requesting headers by both number and hash (aGetBlockHeadersorigin of314and of a known block hash). Confirm both peers stay connected and return identical header sets; no new RPC or observable behavior is introduced.Ref: ethereum#22841
Summary by CodeRabbit