Add BlockHeader library for parsing and verifying RLP-encoded block headers#6395
Add BlockHeader library for parsing and verifying RLP-encoded block headers#6395Amxx wants to merge 15 commits into
Conversation
🦋 Changeset detectedLatest commit: d6eb09e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a new BlockHeader library for parsing and verifying RLP-encoded Ethereum block headers. The library includes enums for supported hardfork versions and header fields, an error definition, a verification function, and multiple getter functions to extract individual header fields. It also includes helper functions to parse RLP-encoded header data. The changes are accompanied by a corresponding test suite and documentation updates. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/utils/BlockHeader.sol (1)
182-188: Consider a parse-once API to avoid repeated full RLP decodes.Each getter re-runs
_parseHeader, so multi-field consumers pay repeated decode/allocation cost. A parse-once overload would reduce cost for real integrations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/utils/BlockHeader.sol` around lines 182 - 188, The current getters repeatedly call _parseHeader (which calls RLP.decodeList) causing repeated full RLP decodes and allocations; add a parse-once API by creating a new function (e.g., parseHeader or parseHeaderCached) that calls RLP.decodeList once and returns Memory.Slice[] memory, then add overloaded variants of the existing getters (or internal helpers) that accept Memory.Slice[] memory fields so callers can decode once and reuse the slices; update callers to call parseHeader(...) once and then call the new getters that take the pre-parsed Memory.Slice[] to avoid repeated RLP.decodeList work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/utils/BlockHeader.sol`:
- Around line 143-145: The nonce bytes are taken from readBytes32() which places
the 8-byte nonce in the low (right-most) 64 bits, but bytes8(bytes32) extracts
the high bytes so non-zero nonces are lost; in getNonce(bytes memory headerRLP)
call _parseHeader(...)[uint8(HeaderField.Nonce)].readBytes32(), cast that result
to uint64 first (e.g. uint64(...)) and then to bytes8 so you extract the low 8
bytes correctly (or use an existing readUint64 helper if present), and add a
unit test that decodes a header with a non-zero nonce (e.g. 0x0102030405060708)
to verify the fix.
In `@test/utils/BlockHeader.test.js`:
- Around line 46-69: The test suite misses coverage for $verifyBlockHeader and
the Amsterdam boundary ($getBlockAccessListHash) on non-Amsterdam headers; add
unit tests in BlockHeader.test.js that (1) call
this.mock.$verifyBlockHeader(headerRLP) with a valid headerRLP (and an invalid
one) to assert success and expected failure behavior, and (2) create a
non-Amsterdam headerRLP (no accessList/baseTxType fields) and assert
this.mock.$getBlockAccessListHash(headerRLP) returns the expected
empty/undefined/default value, plus a separate test for an Amsterdam header that
does return the access list hash. Reference the existing headerRLP variable and
block fixtures used in the file when composing these tests.
---
Nitpick comments:
In `@contracts/utils/BlockHeader.sol`:
- Around line 182-188: The current getters repeatedly call _parseHeader (which
calls RLP.decodeList) causing repeated full RLP decodes and allocations; add a
parse-once API by creating a new function (e.g., parseHeader or
parseHeaderCached) that calls RLP.decodeList once and returns Memory.Slice[]
memory, then add overloaded variants of the existing getters (or internal
helpers) that accept Memory.Slice[] memory fields so callers can decode once and
reuse the slices; update callers to call parseHeader(...) once and then call the
new getters that take the pre-parsed Memory.Slice[] to avoid repeated
RLP.decodeList work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3009e47-0b41-430d-81f8-ba56f8560974
📒 Files selected for processing (4)
contracts/mocks/Stateless.solcontracts/utils/BlockHeader.solcontracts/utils/README.adoctest/utils/BlockHeader.test.js
Amxx
force-pushed
the
feature/blockheaders
branch
from
ba51e79 to
0a37a29
Compare
| /// @dev Verifies that the given block header RLP corresponds to a valid block header for the current chain. | ||
| function verifyBlockHeader(bytes memory headerRLP) internal view returns (bool) { | ||
| return Blockhash.blockHash(getNumber(headerRLP)) == keccak256(headerRLP); | ||
| } |
There was a problem hiding this comment.
Block headers always originate as user-supplied calldata, so bytes memory here forces an unnecessary memory expansion. All getters should take bytes calldata meaning RLP read methods should also have calldata variants. On a personal repo I was seeing massive savings from using calldata relative to the bytes memory approach. The only reason headerRlp should ever be in memory is to hash.
There was a problem hiding this comment.
The RLP decoder operates on memory buffers, so calldata to memory copy will have to happen down the line anyway.
Writing calldata here could delay the calldata to memory copy, but it would still happen. On the other hand, it would prevent the (rare) case where the header may be in memory
| describe('historical blocks', function () { | ||
| for (const block of [ | ||
| { | ||
| hash: '0x244dd4312575b2c8846837086cf4f1669d026f1db376d1ad700ed77782c11b75', |
There was a problem hiding this comment.
Shall we add a TODO to include a block with blockAccessListHash once Amsterdam is out?
Co-authored-by: Ernesto García <ernestognw@gmail.com>
| /// @dev Variant of {verifyBlockHeader} that takes a pre-parsed list of fields. | ||
| function verifyBlockHeader(Memory.Slice[] memory fields, bytes memory headerRLP) internal view returns (bool) { | ||
| return Blockhash.blockHash(getNumber(fields)) == keccak256(headerRLP); | ||
| } |
There was a problem hiding this comment.
Here we don't parse the headerRLP. All we need is the hash. Should we get the hash directly instead ?
- A user that has the full header can easily hash it
- A user that already has the hash (and may not have the encoded format) can us it, and doesn't need to re-encode.
| } | |
| function verifyBlockHeader(Memory.Slice[] memory fields, bytes32 headerHash) internal view returns (bool) { | |
| return Blockhash.blockHash(getNumber(fields)) == headerHash; | |
| } |
3 participants
Fixes #5803
PR Checklist
npx changeset add)